Closed Bug 1175163 Opened 9 years ago Closed 7 years ago

Add more tests for Clients.claim()

Categories

(Core :: DOM: Service Workers, defect)

defect
Not set
normal

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.
Blocks: 1130684
Assignee: nobody → bhsu
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.
re-upload with mercurial format :P

Please refer to comment 1 for more detailed information.
Attachment #8823181 - Attachment is obsolete: true
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
Upload a passable version.
Attachment #8823185 - Attachment is obsolete: true
Attachment #8823199 - Attachment is obsolete: true
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)
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)
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.
Attachment #8823203 - Attachment is obsolete: true
Attachment #8823549 - Flags: review?(bkelly)
Attachment #8823204 - Attachment is obsolete: true
Attachment #8823550 - Flags: review?(bkelly)
Attachment #8823549 - Flags: review?(bkelly) → review+
Attachment #8823550 - Flags: review?(bkelly) → review+
rebased
Attachment #8823549 - Attachment is obsolete: true
Attachment #8826101 - Flags: review+
rebased
Attachment #8823550 - Attachment is obsolete: true
Attachment #8826102 - Flags: review+
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+
Needs rebasing.
Keywords: checkin-needed
(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)
Thanks, I've already rebased them and push the to try last week.
Flags: needinfo?(bhsu)
Attachment #8826101 - Attachment is obsolete: true
Attachment #8839305 - Flags: review+
Attachment #8829716 - Attachment is obsolete: true
Attachment #8839306 - Flags: review+
Attachment #8839305 - Attachment is obsolete: true
Attachment #8839306 - Attachment is obsolete: true
Attachment #8841162 - Attachment is obsolete: true
Attachment #8841164 - Attachment is obsolete: true
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+
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
Attachment #8842275 - Flags: review+
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
https://hg.mozilla.org/mozilla-central/rev/b719e03aa6f8
https://hg.mozilla.org/mozilla-central/rev/e7b9fda58dd5
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.