Closed
Bug 1450064
Opened 7 years ago
Closed 6 years ago
Application panel: filter Service Workers to only show service workers for the current domain
Categories
(DevTools :: Application Panel, enhancement, P3)
DevTools
Application Panel
Tracking
(firefox61 fixed)
RESOLVED
FIXED
Firefox 61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: jdescottes, Assigned: ladybenko)
References
Details
Attachments
(2 files)
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•7 years 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•7 years ago
|
Attachment #8970811 -
Flags: review?(jdescottes)
Comment hidden (mozreview-request) |
Reporter | ||
Updated•6 years ago
|
Attachment #8970811 -
Flags: review?(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
Attachment #8971534 -
Flags: review?(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)
Attachment #8970811 -
Flags: review?(jdescottes) → review+
Reporter | ||
Updated•6 years ago
|
Status: NEW → ASSIGNED
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
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
Attachment #8971534 -
Flags: review?(jdescottes) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
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.
Comment hidden (mozreview-request) |
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
Comment 19•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ee5fa77bd63b https://hg.mozilla.org/mozilla-central/rev/e840531c844f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Comment 20•6 years ago
|
||
Mass move to the new application panel component.
Component: Developer Tools → Developer Tools: Application Panel
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•