Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update the “Link type "serviceworker"” section #1110

Merged
merged 2 commits into from
Jun 24, 2017

Conversation

sideshowbarker
Copy link
Contributor

@sideshowbarker sideshowbarker commented Apr 9, 2017

  • Add new section “Declaring a "serviceworker" Link header”
  • Drop normative requirement to fire an error if link[workertype] value is
    not a valid worker type; instead just abort without firing an error.
  • Drop the partial interface IDL for the HTMLLinkElement interface (the
    definitions for the IDL attributes have already been merged directly
    into the HTMLLinkElement interface IDL definition in the HTML spec).

Fixes #1073

@sideshowbarker
Copy link
Contributor Author

This needs to wait for #1107 (Changing useCache boolean to updateViaCache enum) and needs a further tweak/update if/when whatwg/html#2517 is merged.

@sideshowbarker
Copy link
Contributor Author

I am wondering if there’s an inconsistency in behavior here that we should fix.

Specifically, in this PR we’re dropping the following normative requirement:

If workerType is not a valid WorkerType value, queue a task to fire an event named error at the link element, and abort these steps.

That’s for the case of the link element with an a workertype attribute whose value is not a valid WorkerType value. But in contrast, in the case of Link header with a workertype attribute whose value is not a valid WorkerType value, the spec still says this:

Let workerType be the "workertype" target attribute of the Link header, or "classic" if no such attribute is present.
If workerType is not a valid WorkerType value, abort these steps.

So we’re aborting when the Link header provides a workertype value that’s not valid, but we’re not aborting in when a link element provides a workertype value that’s not valid.

Is that an inconsistency in behavior that we should fix?

@domenic
Copy link
Contributor

domenic commented Apr 9, 2017

Is that an inconsistency in behavior that we should fix?

I think we could go either way. It's very common for HTML to have an invalid value default for enumerated attributes; I can't think of any that fall back to an error behavior. But I'm not sure about HTTP link parameters. Maybe @mnot can lend us his HTTP knowledge.

Re-stating things: I think an inconsistency is fine if each behavior makes sense within its own domain (HTML vs. HTTP).

@domenic
Copy link
Contributor

domenic commented Apr 9, 2017

Wait, no, now I am not so sure, because I started comparing it to HTML on another dimension: script type="" attributes. Those are no-ops for unknown types. (They are not errors though.) Maybe we should be consistent with that...

@ylafon
Copy link
Member

ylafon commented Apr 9, 2017

Marked as non-substantive for IPR from ash-nazg.

@sideshowbarker sideshowbarker force-pushed the sideshowbarker/link-rel-serviceworker branch 2 times, most recently from 959067b to c09d3bd Compare April 11, 2017 12:39
@jungkees
Copy link
Collaborator

@mkruisselbrink, do you have bandwidth to review this PR?

@@ -1778,8 +1803,7 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
1. Let |scriptURL| be the result of <a lt="URL parser">parsing</a> the <{link/href}> attribute with the <{link}> element's <a>node document</a>'s <a>document base URL</a>.
1. Let |scopeURL| be null.
1. If the <{link/scope}> attribute is present, set |scopeURL| to the result of <a lt="URL parser">parsing</a> the <{link/scope}> attribute with the <{link}> element's <a>node document</a>'s <a>document base URL</a>.
1. Let |workerType| be the <{link/workertype}> attribute, or "<code>classic</code>" if the <{link/workertype}> attribute is omitted.
1. If |workerType| is not a valid {{WorkerType}} value, <a>queue a task</a> to <a>fire an event</a> named <code>error</code> at the <{link}> element, and abort these steps.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing this condition entirely seems not desirable. Keep it as-is or at least abort would be needed I guess.

@jungkees
Copy link
Collaborator

jungkees commented Jun 7, 2017

@sideshowbarker, both #1107 and whatwg/html#2517 were merged. So, I think we can continue on this PR. It looks good with needing to have a decision on #1110 (comment).

/cc @domenic, @mnot

@sideshowbarker
Copy link
Contributor Author

both #1107 and whatwg/html#2517 were merged. So, I think we can continue on this PR. It looks good with needing to have a decision on #1110 (comment)

OK, great—so @domenic, what do we want to do for #1110 (comment)?

@domenic
Copy link
Contributor

domenic commented Jun 8, 2017

Hmm, I did seem to forget that.

I have started to think that the best thing to do is to no-op on invalid worker types. This means we need another HTML spec tweak (maybe update whatwg/html#2740) to add a third state, invalid, and make that the invalid value default. Then this spec can no-op when it encounters the invalid state.

@domenic
Copy link
Contributor

domenic commented Jun 8, 2017

whatwg/html#2740 is updated so you can now count on the attribute being in one of three states: classic, module, or invalid.

(At least, once that PR is merged.)

@sideshowbarker
Copy link
Contributor Author

-      1. Let |workerType| be the <{link/workertype}> attribute, or "<code>classic</code>" if the <{link/workertype}> attribute is omitted.
-      1. If |workerType| is not a valid {{WorkerType}} value, <a>queue a task</a> to <a>fire an event</a> named <code>error</code> at the <{link}> element, and abort these steps.

Removing this condition entirely seems not desirable. Keep it as-is or at least abort would be needed I guess.

I have started to think that the best thing to do is to no-op on invalid worker types. This means we need another HTML spec tweak (maybe update whatwg/html#2740) to add a third state, invalid, and make that the invalid value default. Then this spec can no-op when it encounters the invalid state.
whatwg/html#2740 is updated so you can now count on the attribute being in one of three states: classic, module, or invalid.

OK, see 18e52c5 for my attempt at hooking into that

Copy link
Contributor

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HTML parts LGTM. I guess it needs a rebase though.

@sideshowbarker
Copy link
Contributor Author

@jungkees after you (re)review this I’ll rebase so it’ll be ready to merge

docs/index.bs Outdated
A <a>serviceworker link</a> can be declared using a `Link` header</a> [[!RFC5988]] with "`serviceworker`" as the value of the "`rel`" parameter and a [=service worker/script url=] as the <a>target IRI</a>, and the following optional <a>target attributes</a>:

: `scope`
:: Value: A [=service worker registration/scope url=].
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure but would like to check if some serialized url should be used for this? scope url is a URL record.

Can @annevk help?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sideshowbarker, checking https://tools.ietf.org/html/rfc5988#section-5, I think we can just say ":: Value: A [=service worker registration/scope url=] as a URI-Reference" or something like that. WDYT?

And for the script url above, "a [=service worker/script url=] as a URI-Reference inside angle brackets ("<>")" seems clearer I guess. Please correct me if I'm wrong.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better not to reference obsoleted concepts like URI. Instead I'd do ", serialized", linking to https://url.spec.whatwg.org/#concept-url-serializer (so I think that's [=URL serializer|serialized=]).

@jungkees
Copy link
Collaborator

@sideshowbarker, LGTM with checking #1110 (comment). Thanks!

* Add new section “Declaring a "serviceworker" Link header”
* Drop normative requirement to fire an error if link[workertype] value
  isn’t a valid worker type; instead just abort.
* Drop the partial interface IDL for the HTMLLinkElement interface (the
  definitions for the IDL attributes have already been merged directly
  into the HTMLLinkElement interface IDL definition in the HTML spec).

Fixes #1073
@sideshowbarker sideshowbarker force-pushed the sideshowbarker/link-rel-serviceworker branch from a77b22f to 79437cd Compare June 12, 2017 08:57
@mnot
Copy link
Member

mnot commented Jun 14, 2017

Sorry, missed that. There aren't any special considerations for Link header attributes regarding defaults; if you want to define them, and make them different in different places, that's up to you.

@jungkees
Copy link
Collaborator

@sideshowbarker, could you address #1110 (comment) to merge?

@sideshowbarker
Copy link
Contributor Author

sideshowbarker commented Jun 24, 2017

@sideshowbarker, could you address #1110 (comment) to merge?

Done in f3ffc85

Sorry for having taken so long to get back to this

@sideshowbarker sideshowbarker force-pushed the sideshowbarker/link-rel-serviceworker branch from 555acd3 to f3ffc85 Compare June 24, 2017 08:37
@jungkees jungkees merged commit f8e8103 into master Jun 24, 2017
@jungkees
Copy link
Collaborator

Merged. Thanks a lot @sideshowbarker!

@sideshowbarker sideshowbarker deleted the sideshowbarker/link-rel-serviceworker branch June 24, 2017 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants