Julian Descottes [:jdescottes] Reporter
	Description • 7 years ago

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.

	Julian Descottes [:jdescottes] Reporter
	Comment 8 • 6 years 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

	Julian Descottes [:jdescottes] Reporter
	Comment 9 • 6 years 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)

	Tim Nguyen :ntim
	Comment 10 • 6 years 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.

	Julian Descottes [:jdescottes] Reporter
	Comment 13 • 6 years 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

	Julian Descottes [:jdescottes] Reporter
	Comment 16 • 6 years 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.

	Pulsebot
	Comment 18 • 6 years 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

	Andrei Ciure[:aciure]
	Comment 19 • 6 years ago
bugherder

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

	Patrick Brosset <:pbro>
	Comment 20 • 6 years ago

Mass move to the new application panel component.