Application panel: filter Service Workers to only show service workers for the current domain

RESOLVED FIXED in Firefox 61

Status

P3
enhancement
RESOLVED FIXED
9 months ago
6 months ago

People

(Reporter: jdescottes, Assigned: ladybenko)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 61
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(2 attachments)

Follow up to Bug 1445197. We initially show all service workers, but the panel should only display the service workers that match the current page.
(Assignee)

Updated

8 months ago
Assignee: nobody → balbeza
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

8 months ago
Attachment #8970811 - Flags: review?(jdescottes)
Comment hidden (mozreview-request)
Attachment #8970811 - Flags: review?(jdescottes)
(Reporter)

Comment 8

8 months ago
mozreview-review
Comment on attachment 8971534 [details]
Bug 1450064 - Add test for showing service workers from the current domain only.

https://reviewboard.mozilla.org/r/240286/#review246090

This test looks good, thanks Belén! 
I would like to see a last check in this test that goes to a third domain without any service worker and that checks that the empty screen is displayed.

::: devtools/client/application/test/browser.ini:15
(Diff revision 1)
>    !/devtools/client/shared/test/frame-script-utils.js
>    !/devtools/client/shared/test/shared-head.js
>  
>  [browser_application_panel_list-single-worker.js]
>  [browser_application_panel_list-several-workers.js]
> +[browser_application_panel_list-domain-workers.js]

That's the fault of my WIP patch, but we usually try to keep those lists alphabetically sorted.

::: devtools/client/application/test/browser_application_panel_list-domain-workers.js:7
(Diff revision 1)
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +/**
> + * Check that the application panel only displays service workers from the 

eslint will fail here: no-trailing-spaces

::: devtools/client/application/test/browser_application_panel_list-domain-workers.js:23
(Diff revision 1)
> +  let doc = panel.panelWin.document;
> +
> +  info("Wait until the service worker appears in the application panel");
> +  await waitUntil(() => getWorkerContainers(doc).length === 1);
> +
> +  ok(true, "First service worker registration is displayed");

Here is would be nice to check that the scope of the service worker has the expected value (or at least starts with "example.com" here and with "test1.example.com" later).

This way we make sure different service workers are displayed. In the container, we can navigate to the scope using querySelector(".service-worker-scope")

::: devtools/client/application/test/browser_application_panel_list-domain-workers.js:39
(Diff revision 1)
> +
> +  let unregisterWorkers = async function() {
> +    while (getWorkerContainers(doc).length > 0) {
> +      let count = getWorkerContainers(doc).length;
> +      info("Click on the unregister button for the first service worker");
> +      getWorkerContainers(doc)[0]

Locally the test fails here for me, because the unregister button is not available yet. This button is only available when the service worker becomes active (see render() of devtools/client/application/src/components/Worker).

This should ultimately be fixed by the bug we just logged to stop using the UI to unregister service-workers. In the meantime we can wait for the unregister-button here:
await waitUntil(() => getWorkerContainers(doc)[0].querySelector(".unregister-button"));

::: devtools/client/application/test/browser_application_panel_list-domain-workers.js:46
(Diff revision 1)
> +        .click();
> +
> +      info("Wait until the service worker is removed from the application panel");
> +      await waitUntil(() => getWorkerContainers(doc).length == count - 1);
> +    }
> +  }

eslint failure: missing semi colon
Attachment #8971534 - Flags: review?(jdescottes)
(Reporter)

Comment 9

8 months ago
mozreview-review
Comment on attachment 8970811 [details]
Bug 1450064 - Show only service workers for current domain.

https://reviewboard.mozilla.org/r/239592/#review246086

This looks great, thanks!
Just an issue with the "empty" check and the wording (sorry!) + some additional suggestions.

::: devtools/client/application/initializer.js:67
(Diff revision 6)
>      this.client.addListener("processListChanged", this.updateWorkers);
>  
> +    this.updateDomain();
>      await this.updateWorkers();
> +
> +    this.toolbox.target.on("navigate", this.updateDomain);

nit: It would be nice to attach all event listeners in the same spot. Could we move this right after the other addListener calls?

::: devtools/client/application/initializer.js:76
(Diff revision 6)
>      let { service } = await this.client.mainRoot.listAllWorkers();
>      this.actions.updateWorkers(service);
>    },
>  
> +  updateDomain(evt) {
> +    let url = evt ? evt.url : this.toolbox.target.url;

Is there a situation where we need to read url from evt? 
  If no, maybe we could only ever read it from this.toolbox.target? 
  If yes, can you add a comment describing why this is needed?

::: devtools/client/application/src/actions/index.js:8
(Diff revision 6)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
>  const workers = require("./workers");
> +const site = require("./site");

We rarely use the word site or website in DevTools. We typically talk about the current page or the current target. Could we use one of those rather than site?

::: devtools/client/application/src/components/App.js:33
(Diff revision 6)
>    render() {
> -    let { workers, client, serviceContainer } = this.props;
> +    let { workers, domain, client, serviceContainer } = this.props;
>      const isEmpty = workers.length == 0;
>  
> +    // Filter out workers from other domains
> +    workers = workers.filter((x) => new URL(x.url).hostname === domain);

Should we compute this before we compute isEmpty? Otherwise the empty screen will not be displayed when all workers are filtered out because of the domain?

::: devtools/client/application/src/create-store.js:18
(Diff revision 6)
>  
>  function configureStore() {
>    // Prepare initial state.
>    const initialState = {
>      workers: new WorkersState(),
> +    site: new SiteState()

nit: add a last `,` will make future diffs easier to read. (same comment on reducers/index.js, reducers/site-state.js)
Attachment #8970811 - Flags: review?(jdescottes) → review+
Status: NEW → ASSIGNED

Comment 10

8 months ago
mozreview-review
Comment on attachment 8970811 [details]
Bug 1450064 - Show only service workers for current domain.

https://reviewboard.mozilla.org/r/239592/#review246738

::: devtools/client/application/src/reducers/site-state.js:19
(Diff revision 6)
> +    domain: null
> +  };
> +}
> +
> +function getDomainFromUrl(url) {
> +  return new URL(url).hostname;

It might be worth testing this against issues pointed in bug 1448553 in other tools.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Depends on: 1450073
(Reporter)

Comment 13

8 months ago
mozreview-review
Comment on attachment 8971534 [details]
Bug 1450064 - Add test for showing service workers from the current domain only.

https://reviewboard.mozilla.org/r/240286/#review246984

The updated version looks great Belén, thanks! 

Next steps: your patches depend on Bug 1450073 , which I just pushed to autoland. I suggest to wait until my patches reach mozilla-central to move forward. autoland is merged to mozilla-central once or twice a day. Then we can rebase your patches on top of mozilla-central and land this.

In the meantime I pushed your patch to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8649ca759ff4cdb7d1786857a92d4a8f07c1e579
Attachment #8971534 - Flags: review?(jdescottes) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 16

8 months ago
mozreview-review
Comment on attachment 8971534 [details]
Bug 1450064 - Add test for showing service workers from the current domain only.

https://reviewboard.mozilla.org/r/240286/#review247202

::: devtools/client/application/test/browser_application_panel_list-domain-workers.js:17
(Diff revision 3)
> +const OTHER_URL = SIMPLE_URL.replace("example.com", "test1.example.com");
> +const EMPTY_URL = (URL_ROOT + "service-workers/empty.html")
> +  .replace("example.com", "test2.example.com");
> +
> +add_task(async function() {
> +  await enableApplicationPanel(); await enableServiceWorkerDebugging();

We now only need to call enableApplicationPanel(), which calls enableServiceWorkerDebugging();

No harm in keeping it, it's just redundant and shouls be cleaned up.

On a sidenode, we usually don't have several statements on the same line. Not sure why this is not picked up by eslint.
Comment hidden (mozreview-request)

Comment 18

8 months ago
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ee5fa77bd63b
Show only service workers for current domain. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/e840531c844f
Add test for showing service workers from the current domain only. r=jdescottes

Comment 19

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ee5fa77bd63b
https://hg.mozilla.org/mozilla-central/rev/e840531c844f
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Mass move to the new application panel component.
Component: Developer Tools → Developer Tools: Application Panel

Updated

6 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.