Clients.get: block on reserved clients. #1323

mfalken · 2018-06-07T08:41:39Z

Clients.get: block on reserved clients.

Follow-up to 07e9487 (#1259).

Follow-up to 07e9487 (w3c#1259).

mfalken · 2018-06-07T08:42:50Z

@jakearchibald @wanderview @jungkees please take a look (I failed to link to the newly added steps in the HTML spec, but maybe need to just update Bikeshed or wait a little longer).

annevk · 2018-06-07T08:48:33Z

Did you explicitly add data-export="" to the definitions in HTML you wanted to use here?

wanderview

Thanks!

wanderview · 2018-06-07T13:47:36Z

docs/index.bs

+                1. If |client|'s [=environment/id=] is not |id|, [=continue=].
+                1. Wait for either |client|'s [=environment/execution ready flag=] to be set or for |client|'s [=discarded flag=] to be set.
+                1. If |client|'s [=environment/execution ready flag=] is set, then invoke [=Resolve Get Client Promise=] with |client| and |promise|.
+                1. Else, resolve |promise| with undefined.


nit: Technically I think you could leave this "resolve with undefined" out as the one following the loop would take effect. What you have might be clearer about intent, though.

Ah actually I think I have this wrong: the break will break out of the "for each" loop only and we'll double resolve the promise after the loop. I'll adjust this to "abort these steps" which I think breaks out of the "Run these substeps".

wanderview · 2018-06-07T13:48:48Z

docs/index.bs

+              1. Set |visibilityState| to |browsingContext|'s <a>active document</a>'s {{Document/visibilityState}} attribute value.
+              1. Set |focusState| to the result of running the <a>has focus steps</a> with |browsingContext|'s <a>active document</a> as the argument.
+              1. If |client| is a <a>window client</a>, set |ancestorOriginsList| to |browsingContext|'s <a>active document</a>'s <a>relevant global object</a>'s {{Location}} object's [=Location/ancestor origins list=]'s associated list.
+          1. Wait for |task| to have executed.


I realize this part of the PR is mostly preserving the original algorithm, but should we check to see if the client was discarded or destroyed between running the task and getting the result back? Or otherwise say that if any error occurs here we resolve undefined, etc?

Ohhh that's a great point. Yes I'll add that.

mfalken · 2018-06-07T23:58:38Z

@annevk Updating Bikeshed fixed it!

mfalken · 2018-06-08T00:49:24Z

Thanks all, I've updated the PR.

jungkees · 2018-06-12T05:48:28Z

@mattto, thanks for the commits! I think they look good. Waiting on either the execution ready flag set or the discarded flag set sounds good to me.

I just want to clarify who will invoke setting the client's discarded flag step. And we may need to change some related WTP tests for this change, right?

mfalken · 2018-06-12T08:35:46Z

@jungkees Thanks for the review! As for WPT tests, this is being actively developed in Chrome so we'll have tests soon (I find it'd be hard to write WPT tests without a working implementation to test the test, but I see others have been able to do so.)

The discarded flag is set by HTML: https://html.spec.whatwg.org/multipage/webappapis.html#environment-discarding-steps

jakearchibald

My comment are mostly nits. This is looking good!

jakearchibald · 2018-06-12T13:59:58Z

docs/index.bs

@@ -222,6 +222,13 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe

    A <dfn export id="dfn-service-worker-client" for="">service worker client</dfn> is an [=environment=].

+    A [=/service worker client=] has an associated <dfn export>discarded flag</dfn>. It is initially unset.
+
+   This specification defines the following [=environment discarding steps=] for a [=/service worker client=] |client|:


I think this should be written

Each [=/service worker client=] has the following [=environment discarding steps=]:

This avoids "this specification defines" which is a little redundant, because everything normative in this spec is something this spec defines.

jakearchibald · 2018-06-12T14:01:21Z

docs/index.bs

+   This specification defines the following [=environment discarding steps=] for a [=/service worker client=] |client|:
+        1. Set |client|'s [=discarded flag=].
+
+    Note: Implementations can essentially discard clients whose [=discarded flag=] is set.


Nit: lose "essentially". Also, is this defined somewhere?

Removed essentially. Does "this" mean [=environment discarding steps=]? They are defined in HTML: https://html.spec.whatwg.org/multipage/webappapis.html#environment-discarding-steps

jakearchibald · 2018-06-12T14:03:36Z

docs/index.bs

+      :: none
+
+      1. If |client| is an [=environment settings object=], then:
+          1. If |client| is not a <a>secure context</a>, reject |promise| with a "{{SecurityError}}" {{DOMException}} and abort these steps.


For consistency, can we go with [=secure context=]?

Yep! This whole block was just copied from the original. Done.

jakearchibald · 2018-06-12T14:04:01Z

docs/index.bs

+      1. If |client| is an [=environment settings object=], then:
+          1. If |client| is not a <a>secure context</a>, reject |promise| with a "{{SecurityError}}" {{DOMException}} and abort these steps.
+      1. Else:
+          1. If |client|’s <a>creation URL</a> is not a <a>potentially trustworthy URL</a>, reject |promise| with a "{{SecurityError}}" {{DOMException}} and abort these steps.


Same here, can we use [=term=] instead of <a>term</a>?

jakearchibald · 2018-06-12T14:04:18Z

docs/index.bs

+      1. Else:
+          1. If |client|’s <a>creation URL</a> is not a <a>potentially trustworthy URL</a>, reject |promise| with a "{{SecurityError}}" {{DOMException}} and abort these steps.
+      1. If |client| is an [=environment settings object=] and is not a [=window client=], then:
+          1. Let |clientObject| be the result of running <a>Create Client</a> algorithm with |client| as the argument.


[=Create Client=]

There are a few more in this algo too.

mfalken · 2018-06-13T04:57:08Z

Thanks @jakearchibald , I've updated the PR.

jakearchibald

Only got questions this time round.

jakearchibald · 2018-06-13T14:30:56Z

docs/index.bs

+   Each [=/service worker client=] has the following [=environment discarding steps=]:
+        1. Set |client|'s [=discarded flag=].
+
+    Note: Implementations can discard clients whose [=discarded flag=] is set.


"Implementations can discard clients" sounds normative, but maybe it's defined somewhere else? Otherwise, add it to the definition of the flag. Eg:

A [=/service worker client=] has an associated <dfn export>discarded flag</dfn>. It is initially unset. User agents may discard clients whose [=discarded flag=] is set.

(it looks like we tend to say "user agents" rather than "implementations")

Ah no, it was just supposed to be a hint for implementors. It just says "don't worry about this flag. you don't have to keep state around for a client with this flag set". I can just remove the non-normative note if it's confusing.

Ohhh I get it now. Yeah that's fine then.

@mattto, so what do you expect the implementations actually do when the discarded flag is set? That bit sounds like a normative to me. Let me know if closing/terminating of the clients is rather implicit so we don't have to specify.

(And now we auto-bikeshed the .bs file in Nightly. So you can just upload .bs without .html next time. For this PR, don't worry about it.)

@jungkees I think implementations would resolve any open clients.get(id) promise with undefined, then completely forget about that id. If clients.get(id) were called again, it would resolve with undefined immediately because the id wouldn't be found. So they don't need to keep state of the client around. The flag is just to make the spec algorithm work.

Sounds like I should just remove the note.

Let's keep the note. I'll merge the PR. Maybe you can update the note later with what you explained if that'll make it clearer.

jakearchibald · 2018-06-13T14:37:45Z

docs/index.bs

+          1. Wait for |task| to have executed.
+          1. If |client|'s [=discarded flag=] is set, resolve |promise| with undefined and abort these steps.
+          1. Let |windowClient| be the result of running [=Create Window Client=] algorithm with |client|, |visibilityState|, |focusState|, and |ancestorOriginsList| as the arguments.
+          1. Resolve |promise| with |windowClient|.


This might be a silly question, but is it possible for a client to end up representing another origin, or is the client discarded and a new one created in that case?

That's a great question! See #1316, it turns out the HTML and/or Fetch spec has a bug around clients persisting across cross-origin redirects. I was planning on tackling that next.

In the case where the client is discarded, it's expected to never reach execution ready so will never be exposed.

Gotcha. In that case, LGTM.

jungkees · 2018-06-13T22:58:57Z

Thanks @mattto!

mfalken · 2018-06-13T23:07:25Z

Thanks!

mfalken added 2 commits June 7, 2018 13:44

Clients.get: block on reserved clients.

8465e06

Follow-up to 07e9487 (w3c#1259).

ok

35b973a

wanderview approved these changes Jun 7, 2018

View reviewed changes

fix

7e3820d

mfalken mentioned this pull request Jun 8, 2018

Clients.get: block on reserved clients. #1315

Closed

jakearchibald requested changes Jun 12, 2018

View reviewed changes

revise

43e425b

jakearchibald reviewed Jun 13, 2018

View reviewed changes

jakearchibald approved these changes Jun 13, 2018

View reviewed changes

Merge branch 'master' into clients-get-try2

fdd9c51

jungkees merged commit cca62e6 into w3c:master Jun 13, 2018

Clients.get: block on reserved clients. #1323

Clients.get: block on reserved clients. #1323

Conversation

mfalken commented Jun 7, 2018 • edited by pr-preview bot Loading

mfalken commented Jun 7, 2018

annevk commented Jun 7, 2018

wanderview left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfalken commented Jun 7, 2018

mfalken commented Jun 8, 2018

jungkees commented Jun 12, 2018

mfalken commented Jun 12, 2018

jakearchibald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfalken commented Jun 13, 2018

jakearchibald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jungkees commented Jun 13, 2018

mfalken commented Jun 13, 2018

mfalken commented Jun 7, 2018 •

edited by pr-preview bot

Loading