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

Status: RESOLVED FIXED

(DevTools :: Application Panel, enhancement, P3)

Description

[Julian Descottes [:jdescottes]]

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.

Comment 1

[Belén [:ladybenko]]

Attachment: Bug 1450064 - Show only service workers for current domain.

Review commit: https://reviewboard.mozilla.org/r/239592/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/239592/

Comment 2

[Belén [:ladybenko]]

Comment on attachment 8970811 [details]
Bug 1450064 - Show only service workers for current domain.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/239592/diff/1-2/

Comment 3

[Belén [:ladybenko]]

Comment on attachment 8970811 [details]
Bug 1450064 - Show only service workers for current domain.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/239592/diff/2-3/

Comment 4

[Belén [:ladybenko]]

Comment on attachment 8970811 [details]
Bug 1450064 - Show only service workers for current domain.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/239592/diff/3-4/

Comment 5

[Belén [:ladybenko]]

Comment on attachment 8970811 [details]
Bug 1450064 - Show only service workers for current domain.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/239592/diff/4-5/

Comment 6

[Belén [:ladybenko]]

Comment on attachment 8970811 [details]
Bug 1450064 - Show only service workers for current domain.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/239592/diff/5-6/

Comment 7

[Belén [:ladybenko]]

Attachment: Bug 1450064 - Add test for showing service workers from the current domain only.

Review commit: https://reviewboard.mozilla.org/r/240286/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/240286/

Comment 8

[Julian Descottes [:jdescottes]]

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

Comment 9

[Julian Descottes [:jdescottes]]

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)

Comment 10

[Tim Nguyen :ntim]

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 11

[Belén [:ladybenko]]

Comment on attachment 8970811 [details]
Bug 1450064 - Show only service workers for current domain.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/239592/diff/6-7/

Comment 12

[Belén [:ladybenko]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/240286/diff/1-2/

Comment 13

[Julian Descottes [:jdescottes]]

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

Comment 14

[Belén [:ladybenko]]

Comment on attachment 8970811 [details]
Bug 1450064 - Show only service workers for current domain.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/239592/diff/7-8/

Comment 15

[Belén [:ladybenko]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/240286/diff/2-3/

Comment 16

[Julian Descottes [:jdescottes]]

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 17

[Belén [:ladybenko]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/240286/diff/3-4/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
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

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/ee5fa77bd63b
https://hg.mozilla.org/mozilla-central/rev/e840531c844f

Comment 20

[Patrick Brosset <:pbro>]

Mass move to the new application panel component.