Closed Bug 1328293 Opened 3 years ago Closed 3 years ago

show if SW will receive fetch events in about:debugging

Categories

(DevTools :: about:debugging, defect)

defect
Not set

Tracking

(firefox53 fixed)

RESOLVED FIXED
Firefox 53
Tracking Status
firefox53 --- fixed

People

(Reporter: bkelly, Assigned: catalinb)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 4 obsolete files)

In bug 1181127 and bug 1325101 we implemented an optimization where the service worker will only receive fetch events if there is a fetch event handler registered.  To help developers understand what is going on we should show this state on about:debugging.

Catalin, any interest in working on this as a follow-on to the no-fetch optimization?
Flags: needinfo?(catalin.badea392)
I can take this.
Assignee: nobody → catalin.badea392
Flags: needinfo?(catalin.badea392)
Open to suggestions on the right UI for displaying this information.
Attachment #8827431 - Flags: review?(jdescottes)
Comment on attachment 8827431 [details] [diff] [review]
Show if a service worker is listening for fetch events in about:debugging.

Review of attachment 8827431 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, thanks! Some comments to address, and this needs an additional browser mochitest.
The aboutdebugging tests are quite verbose and painful to write so sorry about asking this :/

But I think we should be able to create one easily for this feature, by doing something similar as what is done in browser_service_workers_push_service.
Give it a try and ping me if you have any issue with the test.

Adding :bkelly to review the c++ and idl changes.

::: devtools/client/aboutdebugging/components/workers/panel.js
@@ +91,5 @@
>          };
>          switch (form.type) {
>            case Ci.nsIWorkerDebugger.TYPE_SERVICE:
>              let registration = this.getRegistrationForWorker(form, workers.service);
> +            worker.fetch = form.fetch;

I think this is never defined because we get WorkerActor forms here and never ServiceWorkerActor forms.

::: devtools/client/locales/en-US/aboutdebugging.properties
@@ +20,5 @@
>  unregister = unregister
>  
>  pushService = Push Service
>  
> +fetch = Fetch

Maybe localization notes for the three new strings?
I don't think 'fetch' should be translated as its a technical event type, and we could make that clear in the notes.

::: devtools/server/actors/worker.js
@@ +295,5 @@
>        url: registration.scriptSpec,
>        installingWorker,
>        waitingWorker,
>        activeWorker,
> +      fetch: newestWorker.fetch,

I am seeing several errors in the browser console when a new sw is registering, apparently because newestWorker is null.
(and I am also seeing new unrelated errors but which seem unrelated to your change)

To be safe newestWorker && newestWorker.fetch ?
Attachment #8827431 - Flags: review?(jdescottes)
Attachment #8827431 - Flags: review?(bkelly)
Attachment #8827431 - Flags: feedback+
Comment on attachment 8827431 [details] [diff] [review]
Show if a service worker is listening for fetch events in about:debugging.

Review of attachment 8827431 [details] [diff] [review]:
-----------------------------------------------------------------

c++ and idl changes look good to me.
Attachment #8827431 - Flags: review?(bkelly) → review+
Status: NEW → ASSIGNED
Comment on attachment 8828319 [details] [diff] [review]
Show if a service worker is listening for fetch events in about:debugging.

Review of attachment 8828319 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/server/actors/worker.js
@@ +55,5 @@
>        form.scope = registration.scope;
> +      let newestWorker = (registration.activeWorker ||
> +                          registration.waitingWorker ||
> +                          registration.installingWorker);
> +      form.fetch = newestWorker && newestWorker.handlesFetchEvents;

I don't really understand how we get on this code path. In the testing I've done, SWs are added from iterating the registrations and no WorkerActors of the SW type are created.
Attachment #8828319 - Flags: review?(jdescottes)
Attachment #8828320 - Flags: review?(jdescottes)
Comment on attachment 8828320 [details] [diff] [review]
Add test for new entry in about:debugging#workers.

Review of attachment 8828320 [details] [diff] [review]:
-----------------------------------------------------------------

R+ here, but be sure to read and apply my comment about empty-sw.html before landing.

::: devtools/client/aboutdebugging/test/browser.ini
@@ +15,5 @@
>    service-workers/empty-sw.js
>    service-workers/push-sw.html
>    service-workers/push-sw.js
> +  service-workers/fetch-sw.js
> +  service-workers/fetch-sw.html

list is alphabetically sorted, move lines after service-workers/empty-sw.js

@@ +42,5 @@
>  [browser_service_workers_timeout.js]
>  skip-if = true # Bug 1232931
>  [browser_service_workers_unregister.js]
>  [browser_tabs.js]
> +[browser_service_workers_fetch_flag.js]

list is alphabetically sorted, move lines after [browser_service_workers.js]

::: devtools/client/aboutdebugging/test/browser_service_workers_fetch_flag.js
@@ +21,5 @@
> +
> +  let swTab = yield addTab(url);
> +
> +  let serviceWorkersElement = getServiceWorkerList(document);
> +  yield waitForMutation(serviceWorkersElement, { childList: true });

I guess the reason you added scopes that don't include the test pages we visit is to avoid the sw to start activating.

From what I could see, waitForMutation will yield when we transition between two states, when "newestWorker" is undefined and fetch is then false.
Leaving the workers stopped works around this issue.

To correctly implement this, we would need to :
- fix the scopes
- wait for the service worker to start  
- check the flag

I think this calls for some refactoring of the aboutdebugging test code (at least so that we can share a helper to wait for a given sw to start).
So I won't ask you to modify it here. 

But at least remove the scope from empty-sw.html, otherwise tests will fail.

I will log a follow up to do what I mentioned above.

@@ +23,5 @@
> +
> +  let serviceWorkersElement = getServiceWorkerList(document);
> +  yield waitForMutation(serviceWorkersElement, { childList: true });
> +
> +  let names = [...document.querySelectorAll("#service-workers .service-worker-fetch-flag")];

nit: names -> fetchFlags

::: devtools/client/aboutdebugging/test/service-workers/empty-sw.html
@@ +7,5 @@
>  <body>
>  <script type="text/javascript">
>  "use strict";
>  
> +var sw = navigator.serviceWorker.register("empty-sw.js", {scope: "empty-sw/"});

adding this scope makes two other tests fail :
- browser_service_workers_start.js
- browser_service_workers_unregister.js

This scope is never ok for our tests, so the sw never start, which we need for some of the tests.
Attachment #8828320 - Flags: review?(jdescottes) → review+
Comment on attachment 8828319 [details] [diff] [review]
Show if a service worker is listening for fetch events in about:debugging.

Review of attachment 8828319 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, just a small comment but you can go ahead!

::: devtools/client/aboutdebugging/components/workers/panel.js
@@ +91,5 @@
>          };
>          switch (form.type) {
>            case Ci.nsIWorkerDebugger.TYPE_SERVICE:
>              let registration = this.getRegistrationForWorker(form, workers.service);
> +            worker.fetch = form.fetch;

Can you move this in the else statement below ?

It's really on ever useful in the e10s only case, that will get fixed eventually (if not already fixed).

::: devtools/server/actors/worker.js
@@ +55,5 @@
>        form.scope = registration.scope;
> +      let newestWorker = (registration.activeWorker ||
> +                          registration.waitingWorker ||
> +                          registration.installingWorker);
> +      form.fetch = newestWorker && newestWorker.handlesFetchEvents;

We independently get both:
- the list of all workers
- the list of al registrations
 
See getWorkerForms in devtools/client/aboutdebugging/modules/worker.js
We then mix and match to have all the information we need.
Attachment #8828319 - Flags: review?(jdescottes) → review+
Pushed by catalin.badea392@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/42823a93cd16
Show if a service worker is listening for fetch events in about:debugging. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/a76a454541fc
Add test for new entry in about:debugging#workers. r=jdescottes
Fixed lint errors.
Attachment #8829152 - Attachment is obsolete: true
Pushed by catalin.badea392@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/abe4ae295d86
Show if a service worker is listening for fetch events in about:debugging. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/b108c67ae385
Add test for new entry in about:debugging#workers. r=jdescottes
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/abe4ae295d86
https://hg.mozilla.org/mozilla-central/rev/b108c67ae385
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.