-
Notifications
You must be signed in to change notification settings - Fork 315
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
Should add a “script type” check in the register algorithm. #1359
Comments
Yes, this looks like just an oversight. It should be easy to patch the spec. Did you have WPT ready to test this? |
Yes. I just added a new test for updating the registration with different script type. |
Just concern:
Update Job keeps to fail until a user accesses a top-level page having navigator.serviceWorker.register(‘sw2.js’, {type: ‘module’}); Is this behavior intentional? |
If I'm understanding the concern, it's that the server has changed the script from a classic script to a module script and so Soft Update would fail because the registration is still set to classic script. I think that's fine. To change the script type you should probably change the script URL to avoid this problem. I'm assuming there is no plan to do something like add info to HTTP response headers to let the browser know whether it's a module or classic script? @domenic |
There's whatwg/html#3205, but that's really about confirming the request and hasn't gotten support (though maybe it could be used for this as well). |
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.
…1411) This makes the following changes: * updateViaCache is a live property. This is mostly covered by the WPT registration-updateviacache.https.html. A test that the updateViaCache change is only committed if the register() call resolves is added in web-platform-tests/wpt#17131. * 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. A test for this is added in web-platform-tests/wpt#17131. Addresses #1189, #1408, #1359, and #1358.
Addressed in #1411. |
…gorithm This CL updates the spec comment since the spec issue was resolved. w3c/ServiceWorker#1359 Also, 1) updates a TODO comment from using a specific person's name to the crbug link. 2) Rename the method from "ContinueWithRegistrationForSameScriptUrl" to "ContinueWithRegistrationWithSameRegistrationOptions" because it requires the equality of the script type and the update-via-cache option too. No behavior change. Bug: 824647 Change-Id: I44e146554010e77b9833abc1d2df55e0b1bb78a4 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2732746 Commit-Queue: Asami Doi <[email protected]> Reviewed-by: Hiroki Nakagawa <[email protected]> Cr-Commit-Position: refs/heads/master@{#860113}
In the current spec, we finish a new register job when it equals newestWorker. Spec says (Register 5. 3. ):
However, a new register job should be alive when its script url/update via cache equal newestWorker’s ones but its script type does not equal newestWorker’s one. I think script type should also be checked.
The text was updated successfully, but these errors were encountered: