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

usecache to updateviacache #2515

Merged
merged 6 commits into from
Jun 6, 2017

Conversation

jakearchibald
Copy link
Contributor

This is part of w3c/ServiceWorker#1104

@jakearchibald
Copy link
Contributor Author

jakearchibald commented Apr 7, 2017

I suggest @sideshowbarker for review. I've yet to write tests.

source Outdated
@@ -3770,7 +3770,7 @@ a.setAttribute('href', 'https://example.com/'); // change the content attribute
<li><dfn data-x-href="https://w3c.github.io/ServiceWorker/#dfn-serviceworker-link">serviceworker link</dfn></li>
<li><dfn data-x-href="https://w3c.github.io/ServiceWorker/#serviceworker"><code>ServiceWorker</code></dfn> interface</li>
<li><dfn data-x-href="https://w3c.github.io/ServiceWorker/#serviceworkercontainer"><code>ServiceWorkerContainer</code></dfn> interface</li>
<li><dfn data-x-href="https://w3c.github.io/ServiceWorker/#dfn-use-cache">use cache</dfn></li>
<li><dfn data-x-href="https://w3c.github.io/ServiceWorker/#dfn-update-via-cache">update via cache mode</dfn></li>
</ul>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should add this:

<li><dfn data-x-href="https://w3c.github.io/ServiceWorker/#enumdef-serviceworkerupdateviacache">ServiceWorkerUpdateViaCache</dfn> enumeration</li>

source Outdated
@@ -12847,7 +12847,7 @@ interface <dfn>HTMLLinkElement</dfn> : <span>HTMLElement</span> {
[<span>CEReactions</span>] attribute DOMString <span data-x="dom-link-referrerPolicy">referrerPolicy</span>;
[<span>CEReactions</span>] attribute USVString <span data-x="dom-link-scope">scope</span>;
[<span>CEReactions</span>] attribute <span>WorkerType</span> <span data-x="dom-link-workertype">workerType</span>;
[<span>CEReactions</span>] attribute boolean <span data-x="dom-link-useCache">useCache</span>;
[<span>CEReactions</span>] attribute boolean <span data-x="dom-link-updateViaCache">updateViaCache</span>;
Copy link
Contributor

@sideshowbarker sideshowbarker Apr 7, 2017

Choose a reason for hiding this comment

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

That should be:

 [<span>CEReactions</span>] attribute <span>ServiceWorkerUpdateViaCache</span> updateViaCache

Copy link
Member

Choose a reason for hiding this comment

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

It should actually be a DOMString, not a ServiceWorkerUpdateViaCache, since the latter implies case-sensitivity which is controversial and doesn't fit with any other attribute on the platform (see #1665).

Copy link
Member

Choose a reason for hiding this comment

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

So you're just deciding that new attributes should not be case-sensitive while that argument isn't sorted? That seems flawed.

source Outdated
data-x="rel-serviceworker">serviceworker</code> keyword.</p>
<p>The <dfn><code data-x="attr-link-updateviacache">updateviacache</code></dfn> attribute is an
<span>enumerated attribute</span> that specifies the <span>update via cache mode</span> for a
<span>service worker registration</span>. The attribute must not be specified on <code>link</code>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should add the following:

The attribute's keywords are "<code data-x="">imports</code>", "<code data-x="">all</code>",
and "<code data-x="">never</code>, which map to the respective states <i>imports</i>, <i>all</i>,
and <i>never</i>. The <span>missing value default</span> is the <i>imports</i> state.

@domenic
Copy link
Member

domenic commented Apr 8, 2017

Another model you could base this on is referrerpolicy="", which takes great pains to not duplicate the states inside HTML, so that it's easier to update the source document. However, that might not be very important here, since in theory this list should not change much (as opposed to referrer policies where new ones seem to get added often).

@jakearchibald
Copy link
Contributor Author

@sideshowbarker thanks for the feedback - I've made the corrections.

@domenic @annevk I don't have any opinion on the "case sensitive or not" thing. I guess this is a recent change, as workerType (which I was copying) appears to be case sensitive.

@sideshowbarker sideshowbarker added do not merge yet Pull request must not be merged per rationale in comment needs tests Moving the issue forward requires someone to write tests labels Apr 13, 2017
@sideshowbarker
Copy link
Contributor

I think this awaits resolution on #2515 (comment) and I guess also on tests, so added labels for those

@wanderview
Copy link
Member

I think case-sensitive would be preferable.

@domenic
Copy link
Member

domenic commented Apr 13, 2017

We're not going to be inconsistent with the rest of HTML here. See discussion in #1665.

@wanderview
Copy link
Member

wanderview commented Apr 13, 2017

I forgot this was a <link> attribute. We haven't implemented that feature yet. I thought we were talking about the js enumeration. Sorry for my confusion.

@jakearchibald
Copy link
Contributor Author

@domenic
Copy link
Member

domenic commented Jun 5, 2017

@jakearchibald yep, I guess I didn't realize that got merged... will fix...

@jakearchibald
Copy link
Contributor Author

Updated PR. It should now be case insensitive.

source Outdated

<p>The <dfn><code data-x="dom-link-workerType">workerType</code></dfn> IDL attribute must
<span>reflect</span> the <code data-x="attr-link-workertype">workertype</code> content
attribute.</p>
attribute, <span>limited to only known values</span>.</p>
Copy link
Member

Choose a reason for hiding this comment

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

This change seems superfluous? We need to fix this section anyway so not sure why we'd change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh, copy-paste error, sorry. Fixing.

source Outdated
<th> <code data-x="">updateviacache</code>
<td> <code data-x="attr-link-updateviacache">link</code>
<td> <span>update via cache mode</span> for a <span>service worker registration</span> (for <code data-x="attr-link-rel">rel</code>="<code data-x="rel-serviceworker">serviceworker</code>")
<td> "<code data-x="">imports</code>"; "<code data-x="">all</code>"; "<code data-x="">never</code>"
Copy link
Member

Choose a reason for hiding this comment

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

In the interest of not duplicating stuff across specs, let's replace this with <span>Update via cache mode</span> (cf. referrer policy)

@domenic domenic removed the do not merge yet Pull request must not be merged per rationale in comment label Jun 6, 2017
@domenic
Copy link
Member

domenic commented Jun 6, 2017

We're still waiting on tests for this, right?

Be sure to update https://github.com/w3c/web-platform-tests/blob/master/html/dom/elements-metadata.js :)

@jakearchibald
Copy link
Contributor Author

Yep, will do the tests. I wasn't aware of https://github.com/w3c/web-platform-tests/blob/master/html/dom/elements-metadata.js. Cheers!

@jakearchibald
Copy link
Contributor Author

I've rolled the tests into web-platform-tests/wpt#5515. I wasn't sure what to do with elements-metadata.js (I couldn't see how it was used), but I gave it a shot.

@domenic
Copy link
Member

domenic commented Jun 6, 2017

Looks good. I'll merge the spec change since those tests are still underway.

@domenic domenic added topic: workers normative change and removed needs tests Moving the issue forward requires someone to write tests labels Jun 6, 2017
@domenic domenic merged commit b5fcec0 into whatwg:master Jun 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

5 participants