Add multi-global tests for service worker URL parsing #3449
Conversation
|
Reviewers for this pull request are: @ehsan. |
| if (event.request.url.includes('test.txt')) { | ||
| event.respondWith(new Response('current')); | ||
| } else { | ||
| event.respondWith(fetch(event.request)); |
There was a problem hiding this comment.
You could remove these for simplicity: not calling respondWith will fallback to network.
Fixes w3c#922. Web platform tests at web-platform-tests/wpt#3449. Part of the overall effort in whatwg/html#1431. In the process, fixes some ambiguity about how to parse URLs in the Link header, as previous it parsed relative to the entry settings object, which did not exist during this algorithm.
|
Why close and delete my PR? |
domenic
force-pushed
the
service-worker-multi-globals
branch
from
64f2812 to
4b90437
Compare
Fixes w3c#922. Web platform tests at web-platform-tests/wpt#3449. Part of the overall effort in whatwg/html#1431. In the process, fixes some ambiguity about how to parse URLs in the Link header, as previous it parsed relative to the entry settings object, which did not exist during this algorithm.
|
I can't figure out how to restore the branch at this point, so please re-push it if you still have it locally in case I don't wind up finding a solution. (I deleted it by mistake.) |
FirefoxTesting revision b1c22eb All results/service-workers/service-worker/multi-globals/url-parsing.https.html
|
ChromeTesting revision b1c22eb All results/service-workers/service-worker/multi-globals/url-parsing.https.html
|
| return frames[0].testRegister(); | ||
| }).then(r => { | ||
| registration = r; | ||
| return fetch('test.txt'); |
There was a problem hiding this comment.
This fetch is not intercepted as the document hasn't gotten its controller yet. We should make sure the registration has advanced to "activated" state before this fetch. For that, add a promise returning utility function wait_for_state before this line. It would look like:
const newestWorker = r.installing || r.waiting || r.active;
return wait_for_state(test, newestWorker, 'activated').then(_ => {
// Fetch here
});Also, the following code is needed in the service worker script to promote the worker to active worker and to make all of its clients controlled right away (without reloading them:)
this.addEventListener('install', event => {
this.skipWaiting();
});
this.addEventListener('activate', event => {
clients.claim();
});And I think the fetch should fetch('relevant/test.txt') instead of test.txt as we expect the relevant scoped worker intercept this request.
| @@ -0,0 +1,5 @@ | |||
| this.addEventListener('fetch', event => { | |||
There was a problem hiding this comment.
The second snippet in 4b90437#r100237660 needs to be added in this script and to other service worker scripts I guess.
|
Thanks @jungkees! Fixed; PTAL. |
Chrome (unstable channel)Testing web-platform-tests at revision 30c4331f8d618ddf5ebff47e0b7b3ce6603757ee All results/service-workers/service-worker/multi-globals/url-parsing.https.html
|
Firefox (nightly channel)Testing web-platform-tests at revision 30c4331f8d618ddf5ebff47e0b7b3ce6603757ee All results/service-workers/service-worker/multi-globals/url-parsing.https.html
|
| }) | ||
| .then(gottenRegistration => { | ||
| assert_not_equals(registration, null, 'the registration should not be null'); | ||
| assert_equals(gottenRegistration, registration, 'the retrieved registration should be equal to the original'); |
There was a problem hiding this comment.
This seems to fail as it compares objects gotten from different realm I guess. Comparing gottenRegistration.scope === registration.scope would do here.
|
@domenic, it LGTM with the above comment. JS objects from different realms won't pass equality check, right? |
The wording is adapted from the WHATWG contributor guidelines: https://github.com/whatwg/meta/blob/master/CONTRIBUTING.md This is sometimes already happening: web-platform-tests/wpt#3449 web-platform-tests/wpt#5628 Drive-by: whitespace
The wording is adapted from the WHATWG contributor guidelines: https://github.com/whatwg/meta/blob/master/CONTRIBUTING.md This is sometimes already happening: web-platform-tests/wpt#3449 web-platform-tests/wpt#5628 Drive-by: whitespace
The wording is adapted from the WHATWG contributor guidelines: https://github.com/whatwg/meta/blob/master/CONTRIBUTING.md This is sometimes already happening: web-platform-tests/wpt#3449 web-platform-tests/wpt#5628 Drive-by: whitespace
See w3c/ServiceWorker#922.
This should not be merged until we have consensus on that spec fix.