Skip to content

Improve "update via cache" and "worker type" in register algorithms. #1411

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jun 3, 2019
Merged

Improve "update via cache" and "worker type" in register algorithms. #1411

merged 2 commits into from
Jun 3, 2019

Conversation

mfalken
Copy link
Member

@mfalken mfalken commented Jun 3, 2019 •
edited by pr-preview bot
Loading

This makes the following changes:

  • updateViaCache is a live property. This is covered by the WPT
    registration-updateviacache.https.html. However we are missing tests for a
    register() that rejects with a new updateViaCache property. The desired
    behavior is to only update the property if register() resolves.
  • Changing worker type bypasses the byte-for-byte update check. This is covered
    by WPT update-registration-with-type.https.html.
  • Adds Worker type and updateViaCache to the job equivalence check. This
    probably can have a WPT for successive register() calls that vary these
    properties don’t get coalesced into a single job.

Addresses #1189, #1408, #1359, and #1358.

Preview | Diff

@mfalken
Improve "update via cache" and "worker type" in register algorithms.
6584563 
This makes the following changes:
* updateViaCache is a live property. This is covered by the WPT
registration-updateviacache.https.html. However we are missing tests for a
register() that rejects with a new updateViaCache property. The desired
behavior is to only update the property if register() resolves.
* Changing worker type bypasses the byte-for-byte update check. This is covered
by WPT update-registration-with-type.https.html.
* Adds Worker type and updateViaCache to the job equivalence check. This
probably can have a WPT for successive register() calls that vary these
properties don’t get coalesced into a single job.

Addresses w3c#1189, w3c#1408, w3c#1359, and w3c#1358.
@mfalken mfalken requested a review from jakearchibald June 3, 2019 04:29
mfalken added a commit to mfalken/web-platform-tests that referenced this pull request Jun 3, 2019
@mfalken
service worker: Add more tests for updateViaCache and type in register.
abf6d3f 
Tests for w3c/ServiceWorker#1411.

* updateViaCache only updates if register() resolves.
* Changes to updateViaCache and type prevent the job from being
  coalesced to the previous job.
@mfalken mfalken mentioned this pull request Jun 3, 2019
Merged
@mfalken
Copy link
Member Author

mfalken commented Jun 3, 2019

Adding the missing tests at web-platform-tests/wpt#17131

jakearchibald
jakearchibald reviewed Jun 3, 2019
View reviewed changes
Copy link
Contributor

@jakearchibald jakearchibald left a comment

Choose a reason for hiding this comment

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

One small nit.

docs/index.bs
@@ -637,7 +637,7 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe

Note: The {{ServiceWorkerContainer/register(scriptURL, options)}} method creates or updates a [=/service worker registration=] for the given [=service worker registration/scope url=]. If successful, a [=/service worker registration=] ties the provided |scriptURL| to a [=service worker registration/scope url=], which is subsequently used for <a lt="handle fetch">navigation matching</a>.

<dfn method for="ServiceWorkerContainer"><code>register(|scriptURL|, |options|)</code></dfn> method *must* run these steps:
The <dfn method for="ServiceWorkerContainer"><code>register(|scriptURL|, |options|)</code></dfn> method *must* run these steps:
Copy link
Contributor

Choose a reason for hiding this comment

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

Hah we're pretty inconsistent about this. Fine with adding them as we go along. I agree "The" is better.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, agreed.

docs/index.bs Outdated
1. Set |hasUpdatedResources| to true if any of the following are true:
1. |newestWorker| is null.
1. |newestWorker|'s [=service worker/script url=] is not |url| or |newestWorker|'s [=service worker/type=] is not |job|'s [=worker type=].
1. |newestWorker|'s [=script resource map=][|url|]'s [=response/body=] is not byte-for-byte identical with |response|'s [=response/body=].
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Since it's "any of the following", these should start "*" not "1.", since the order doesn't matter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@mfalken
Copy link
Member Author

mfalken commented Jun 3, 2019

Updated, and also revised slightly to set updateViaCache even when there is no byte-for-byte change (though we can discuss whether updateViaCache should trigger the update at all in another issue).

jakearchibald
jakearchibald approved these changes Jun 3, 2019
View reviewed changes
Copy link
Contributor

@jakearchibald jakearchibald left a comment

Choose a reason for hiding this comment

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

LGTM. Can you create an issue to discuss the change?

@mfalken mfalken merged commit 7f2b6e1 into w3c:master Jun 3, 2019
mfalken added a commit to web-platform-tests/wpt that referenced this pull request Jun 3, 2019
@mfalken
service worker: Add more tests for updateViaCache and type in regist…
8603f09 
…er. (#17131)

Tests for w3c/ServiceWorker#1411.

* updateViaCache only updates if register() resolves.
* Changes to updateViaCache and type prevent the job from being
  coalesced to the previous job.
@mfalken mfalken mentioned this pull request Jun 3, 2019
Open
@mfalken
Copy link
Member Author

mfalken commented Jun 3, 2019

Thanks, filed #1414 !

This was referenced Jun 3, 2019
Closed
Closed
Closed
Closed
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jun 19, 2019
@mfalken @jgraham
Bug 1556298 [wpt PR 17131] - service worker: Add more tests for updat…
80a9ae4 
…eViaCache and type in register., a=testonly

Automatic update from web-platform-tests
 service worker: Add more tests for updateViaCache and type in register. (#17131)

Tests for w3c/ServiceWorker#1411.

* updateViaCache only updates if register() resolves.
* Changes to updateViaCache and type prevent the job from being
  coalesced to the previous job.
--

wp5At-commits: 8603f095d72afd7cbd78e875167ddabf38266fa0
wpt-pr: 17131
xeonchen pushed a commit to xeonchen/gecko that referenced this pull request Jun 19, 2019
@mfalken @jgraham
Bug 1556298 [wpt PR 17131] - service worker: Add more tests for updat…
f30e366 
…eViaCache and type in register., a=testonly

Automatic update from web-platform-tests
 service worker: Add more tests for updateViaCache and type in register. (#17131)

Tests for w3c/ServiceWorker#1411.

* updateViaCache only updates if register() resolves.
* Changes to updateViaCache and type prevent the job from being
  coalesced to the previous job.
--

wp5At-commits: 8603f095d72afd7cbd78e875167ddabf38266fa0
wpt-pr: 17131
marcoscaceres pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 23, 2019
@mfalken
service worker: Add more tests for updateViaCache and type in regist…
fc13466 
…er. (#17131)

Tests for w3c/ServiceWorker#1411.

* updateViaCache only updates if register() resolves.
* Changes to updateViaCache and type prevent the job from being
  coalesced to the previous job.
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 4, 2019
@marco-c
Bug 1556298 [wpt PR 17131] - service worker: Add more tests for updat…
9dd2645 
…eViaCache and type in register., a=testonly

Automatic update from web-platform-tests
 service worker: Add more tests for updateViaCache and type in register. (#17131)

Tests for w3c/ServiceWorker#1411.

* updateViaCache only updates if register() resolves.
* Changes to updateViaCache and type prevent the job from being
  coalesced to the previous job.
--

wp5At-commits: 8603f095d72afd7cbd78e875167ddabf38266fa0
wpt-pr: 17131

UltraBlame original commit: 02fa74b9185dcf933168e8541f346268053515dd
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 4, 2019
@marco-c
Bug 1556298 [wpt PR 17131] - service worker: Add more tests for updat…
19c46e5 
…eViaCache and type in register., a=testonly

Automatic update from web-platform-tests
 service worker: Add more tests for updateViaCache and type in register. (#17131)

Tests for w3c/ServiceWorker#1411.

* updateViaCache only updates if register() resolves.
* Changes to updateViaCache and type prevent the job from being
  coalesced to the previous job.
--

wp5At-commits: 8603f095d72afd7cbd78e875167ddabf38266fa0
wpt-pr: 17131

UltraBlame original commit: 02fa74b9185dcf933168e8541f346268053515dd
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 4, 2019
@marco-c
Bug 1556298 [wpt PR 17131] - service worker: Add more tests for updat…
ad3f019 
…eViaCache and type in register., a=testonly

Automatic update from web-platform-tests
 service worker: Add more tests for updateViaCache and type in register. (#17131)

Tests for w3c/ServiceWorker#1411.

* updateViaCache only updates if register() resolves.
* Changes to updateViaCache and type prevent the job from being
  coalesced to the previous job.
--

wp5At-commits: 8603f095d72afd7cbd78e875167ddabf38266fa0
wpt-pr: 17131

UltraBlame original commit: 02fa74b9185dcf933168e8541f346268053515dd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakearchibald jakearchibald jakearchibald approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mfalken @jakearchibald