Closed
Bug 1175163
Opened 9 years ago
Closed 7 years ago
Add more tests for Clients.claim()
Categories
(Core :: DOM: Service Workers, defect)
Core
DOM: Service Workers
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: ehsan.akhgari, Assigned: bhsu)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 14 obsolete files)
9.07 KB,
patch
|
bhsu
:
review+
|
Details | Diff | Splinter Review |
7.27 KB,
patch
|
bhsu
:
review+
|
Details | Diff | Splinter Review |
This is a follow-up to bug 1130684.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → bhsu
Assignee | ||
Comment 1•7 years ago
|
||
I think claim_fetch_worker is the testcase requested for testing a fetch request should be intercepted by a service worker after its originating window is claimed by the service worker. In addition to the migration, I slightly update the testcase to make it a little more reasonable. The test iframe is now created before registering a service worker. Otherwise, once the registration becomes ready, the iframe will be created with its controller set to the active service worker of the registration, and I don't think that's what we want.
Assignee | ||
Comment 2•7 years ago
|
||
re-upload with mercurial format :P Please refer to comment 1 for more detailed information.
Attachment #8823181 -
Attachment is obsolete: true
Assignee | ||
Comment 3•7 years ago
|
||
This is the testcase for a service worker registration should be affected if the only client of its active service worker is claimed away by another service worker. For example, its waiting service worker should become the active service worker as [0] suggests. I'd like to implement the scenario [1] suggesting as well. However, once a registration is unregistered, we cannot find it via `navigator.serviceWorker.getRegistration(<SCOPE>)`, because its uninstalling flag is set. [0] https://github.com/w3c/ServiceWorker/issues/682#issuecomment-95229504 [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1130684#c7
Assignee | ||
Comment 4•7 years ago
|
||
Upload a passable version.
Attachment #8823185 -
Attachment is obsolete: true
Assignee | ||
Comment 5•7 years ago
|
||
Attachment #8823199 -
Attachment is obsolete: true
Assignee | ||
Comment 6•7 years ago
|
||
Hello Ben, Sorry for keep spamming, the details of the two testcases remain the same as mentioned in comment 1 and comment 3, respectively. The two testcases are now uploaded in a passable form, though I personally don't suggest using the two testcases directly. There is a test in both testcases rewrote in a passable form, IMHO, we should compare the service worker object instead of their scriptURL in the following code snippet, but doing so fails the two testcases. ``` assert_equals( frame.contentWindow.navigator.serviceWorker.controller.scriptURL, worker.scriptURL, 'Test iframe should be claimed.'); ``` I think we can file a following bug to fix this, and update the testcases correspondingly in that bug. How do you think?
Flags: needinfo?(bkelly)
Comment 7•7 years ago
|
||
So, the reason you have to compare scriptURL here is that: frame.contentWindow.navigator.serviceWorker.controller Was created with a different global from `worker`. We can't have js object equality across compartments. So there are two different js objects representing the same worker. So if you want to do direct object comparison you need to get the registration.installing from the frame global in order to compare later with controller. Not sure how easy that is to do with the test, though.
Flags: needinfo?(bkelly)
Assignee | ||
Comment 8•7 years ago
|
||
Thanks, It's great to know that it is because of JS compartment, which I've never thought about. I believe getting the service worker within the iframe is totally doable, and I'll update the patches correspondingly since comparing the objects is much more meaningful than comparing merely the URLs.
Assignee | ||
Comment 9•7 years ago
|
||
Attachment #8823203 -
Attachment is obsolete: true
Attachment #8823549 -
Flags: review?(bkelly)
Assignee | ||
Comment 10•7 years ago
|
||
Attachment #8823204 -
Attachment is obsolete: true
Attachment #8823550 -
Flags: review?(bkelly)
Updated•7 years ago
|
Updated•7 years ago
|
Attachment #8823549 -
Flags: review?(bkelly) → review+
Updated•7 years ago
|
Attachment #8823550 -
Flags: review?(bkelly) → review+
Assignee | ||
Comment 11•7 years ago
|
||
rebased
Attachment #8823549 -
Attachment is obsolete: true
Attachment #8826101 -
Flags: review+
Assignee | ||
Comment 12•7 years ago
|
||
rebased
Attachment #8823550 -
Attachment is obsolete: true
Attachment #8826102 -
Flags: review+
Assignee | ||
Comment 13•7 years ago
|
||
The previous version fails intermittently, since service workers move from activating to activated automatically, and thus it maybe not appropriate to check whether a service worker is of activating state.
Attachment #8826102 -
Attachment is obsolete: true
Attachment #8829716 -
Flags: review+
Assignee | ||
Comment 14•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4326042f6e44c6dc30a8fe1ff28a7ba29623eee
Keywords: checkin-needed
Comment 16•7 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #15) > Needs rebasing. HoPang, it's a kind reminder in case you didn't notice comment 15.
Flags: needinfo?(bhsu)
Assignee | ||
Comment 17•7 years ago
|
||
Thanks, I've already rebased them and push the to try last week.
Flags: needinfo?(bhsu)
Assignee | ||
Comment 18•7 years ago
|
||
Attachment #8826101 -
Attachment is obsolete: true
Attachment #8839305 -
Flags: review+
Assignee | ||
Comment 19•7 years ago
|
||
Attachment #8829716 -
Attachment is obsolete: true
Attachment #8839306 -
Flags: review+
Assignee | ||
Comment 20•7 years ago
|
||
Assignee | ||
Comment 21•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8839305 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8839306 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8841162 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8841164 -
Attachment is obsolete: true
Assignee | ||
Comment 22•7 years ago
|
||
Assignee | ||
Comment 23•7 years ago
|
||
Assignee | ||
Comment 24•7 years ago
|
||
Comment on attachment 8842273 [details] [diff] [review] (V4) Part 1: Migrate claim_fetch_worker to wpt Another rebased version.
Attachment #8842273 -
Attachment description: Part 1: Migrate claim_fetch_worker to wpt → (V4) Part 1: Migrate claim_fetch_worker to wpt
Attachment #8842273 -
Flags: review+
Assignee | ||
Comment 25•7 years ago
|
||
Comment on attachment 8842275 [details] [diff] [review] (V5) Part 2: Create a testcase for claim() should affect other registrations Another rebased version.
Attachment #8842275 -
Attachment description: Part 2: Create a testcase for claim() should affect other registrations → (V5) Part 2: Create a testcase for claim() should affect other registrations
Assignee | ||
Updated•7 years ago
|
Attachment #8842275 -
Flags: review+
Assignee | ||
Comment 26•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=70c6647ae39a44263c845cb660d73b7c69d5ad52
Keywords: checkin-needed
Comment 27•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b719e03aa6f8 Part 1: Migrate claim_fetch_worker to wpt. r=bkelly https://hg.mozilla.org/integration/mozilla-inbound/rev/e7b9fda58dd5 Part 2: Create a testcase for claim() should affect other registrations. r=bkelly
Keywords: checkin-needed
Comment 28•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b719e03aa6f8 https://hg.mozilla.org/mozilla-central/rev/e7b9fda58dd5
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•