Closed Bug 1445197 Opened 7 years ago Closed 7 years ago

Create DevTools application panel to show the list of service workers

Categories

(DevTools :: Application Panel, enhancement)

Product:
DevTools
Component:
Application Panel
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox61 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

Attachments

(4 files, 1 obsolete file)

Bug 1445197 - part 1: Create an application panel for DevTools;
59 bytes, text/x-review-board-request
nchevobbe
: review+
Honza
: review+
sole
: review+
Details
Bug 1445197 - part 2: Add a temporary icon for the application panel;
59 bytes, text/x-review-board-request
sole
: review+
Details
Bug 1445197 - part 3: Move about:debugging worker module to a shared module;
59 bytes, text/x-review-board-request
ochameau
: review+
Details
Bug 1445197 - part 4: Implement application panel UI to display all workers;
59 bytes, text/x-review-board-request
nchevobbe
: review+
Honza
: review+
Details
The goal of this bug is to introduce a new Application panel. Guidelines: - panel should use react - panel should be off by default (and not visible in the DevTools options if possible) - panel should use localization mechanisms but the new localization files should *not* be picked up by localization teams, as we expect the strings to be updated frequently in the beginning (see Bug 1408368 for an example) - panel should only show the list of service workers (no filtering, this will be handled in a separate bug) - list of service workers shoudl be read only
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Blocks: 1450064
Blocks: 1450065
Blocks: 1450067
Blocks: 1450070
Blocks: 1450071
Blocks: 1450073
Blocks: 1451734
Blocks: 1451737
Some additional context for the reviews: - the panel is preffed off for now. You can enable it with devtools.application.enabled (e.g. `./mach run --devtools --setpref devtools.application.enabled=true`) - my intention is to focus on the initial code for the panel here. I intentionally left out localization and tests (Bug 1450073 and Bug 1450071). I can fold that in if needed. - when testing, service worker debugging normally only works at the moment in "single child process" e10s. As opposed to about:debugging where we check if service worker debugging can be used, here we assume debugging "should work" before the panel is shipped. You might want to force single e10s by setting dom.ipc.multiOptOut or by using about:debugging#workers - the panel currently shows all workers (eg exactly the same list as about:debugging#workers!). Showing only the workers for the domain (or page) will be done in Bug 1450064
Comment on attachment 8964976 [details] Bug 1445197 - part 1: Create an application panel for DevTools; https://reviewboard.mozilla.org/r/233710/#review240028 This seems good to me. I do have some questions and nits, but nothing big ::: devtools/client/application/initializer.js:21 (Diff revision 2) > +const App = createFactory(require("./src/components/App")); > + > +window.Application = { > + bootstrap({ toolbox, panel }) { > + this.mount = document.querySelector("#mount"); > + this.toolbox = toolbox; it there any need to have the toolbox reference here ? (maybe in another patch in this review ?) ::: devtools/client/application/initializer.js:30 (Diff revision 2) > + > + render(app, this.mount); > + }, > + > + destroy() { > + unmountComponentAtNode(this.mount); not sure about this but should we "release" properties ? ```js this.mount = null; this.toolbox = null; ``` ::: devtools/client/application/panel.js:7 (Diff revision 2) > +function ApplicationPanel(panelWin, toolbox) { > + this.panelWin = panelWin; > + this.toolbox = toolbox; > +} > + > +ApplicationPanel.prototype = { > + async open() { > + if (!this.toolbox.target.isRemote) { > + await this.toolbox.target.makeRemote(); > + } > + await this.panelWin.Application.bootstrap({ > + toolbox: this.toolbox, > + panel: this, > + }); > + this.emit("ready"); > + this.isReady = true; > + return this; > + }, > + > + async destroy() { > + await this.panelWin.Application.destroy(); > + this.emit("destroyed"); > + return this; > + }, > +}; I know it's consistent with the other panel, but do you think we could make this an ES6 class instead ? ::: devtools/client/application/panel.js:7 (Diff revision 2) > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +"use strict"; > + > +function ApplicationPanel(panelWin, toolbox) { nit: Maybe add some jsdoc ? ::: devtools/client/application/panel.js:7 (Diff revision 2) > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +"use strict"; > + > +function ApplicationPanel(panelWin, toolbox) { nit: would it make sense to name it `Panel` so we're consistent with the filename ? ::: devtools/client/application/panel.js:27 (Diff revision 2) > + this.isReady = true; > + return this; > + }, > + > + async destroy() { > + await this.panelWin.Application.destroy(); If I read the code well, I don't think Application#destroy is async. ::: devtools/client/application/panel.js:29 (Diff revision 2) > + }, > + > + async destroy() { > + await this.panelWin.Application.destroy(); > + this.emit("destroyed"); > + return this; should we also release `this.panelWin` and `this.toolbox` ?
Attachment #8964976 - Flags: review?(nchevobbe) → review+
Comment on attachment 8965479 [details] Bug 1445197 - part 4: Implement application panel UI to display all workers; https://reviewboard.mozilla.org/r/234204/#review240040 I have mainly nits and questions here, but this looks like a solid start :) Happy to r+ this since you have tests on your todo list ::: devtools/client/application/index.html:11 (Diff revision 1) > <head> > - <script src="chrome://devtools/content/shared/theme-switching.js"></script> > + <link rel="stylesheet" type="text/css" href="resource://devtools/client/application/application.css" /> > </head> > <body class="theme-body" role="application"> > <div id="mount"></div> > + <script src="chrome://devtools/content/shared/theme-switching.js"></script> nit: would it make sense to have this script file here in part1 patch ? **Really** not important though ::: devtools/client/application/initializer.js:25 (Diff revision 1) > +// Inject to global window for testing > +window.store = store; > +window.actions = actions; or maybe we could expose them through methods on Application (`getStore`, `getActions` )? Not feeling strongly about this though ::: devtools/client/application/initializer.js:52 (Diff revision 1) > > - render(app, this.mount); > + await this.updateWorkers(); > + }, > + > + async updateWorkers() { > + let client = this.toolbox.target.client; looks like we can use `this.client` here ::: devtools/client/application/src/components/App.css:5 (Diff revision 1) > + .application { > + height: 100%; > + padding: 0 0 0 20px; > + overflow: auto; > + display: flex; > + flex-direction: column; > +} Lately I tried to make sense of "complex" layouts (flex, grid) in css files by adding ascii visualisations of what it should look like (see https://searchfox.org/mozilla-central/rev/d0413b711da4dac72b237d0895daba2537f1514c/devtools/client/themes/webconsole.css#706-722 or https://hg.mozilla.org/try/diff/e976bc9e281c/devtools/client/themes/webconsole.css#l1.14 ) in order to make it easier for contributors to know what to expect. here it might make sense to do so (even more I'd say because we don't know which children .application has since they're declared in separate css files) ::: devtools/client/application/src/components/Worker.css:13 (Diff revision 1) > + > +.service-worker-container { > + margin-bottom: 20px; > + width: 100%; > + max-width: 600px; > + box-sizing: border-box; should we put the `box-sizing: border-box` on `*` in App.css so we don't have unexpected behaviour when introducing new elements ? ::: devtools/client/application/src/components/Worker.css:17 (Diff revision 1) > + max-width: 600px; > + box-sizing: border-box; > + position: relative; > +} > + > +.service-worker-container > div { maybe this is a bit brittle ? Could we add a classname to this div we want to style, and here replace the rule with `.service-worker-container .the-super-div` ? ::: devtools/client/application/src/components/Worker.js:45 (Diff revision 1) > + this.unregister = this.unregister.bind(this); > + } > + > + debug() { > + if (!this.isRunning()) { > + // If the worker is not running, we can't debug it. should we have a console.warn or something here ? ::: devtools/client/application/src/components/Worker.js:55 (Diff revision 1) > + debugWorker(client, worker.workerActor); > + } > + > + start() { > + if (!this.isActive() || this.isRunning()) { > + // If the worker is not active or if it is already running, we can't start it. should we have a console.warn or something here ? ::: devtools/client/application/src/components/WorkerList.js:22 (Diff revision 1) > + let { toolbox } = this.props; > + let win = toolbox.doc.defaultView.top; > + win.openUILinkIn("about:debugging#workers", "tab", { relatedToCurrent: true }); here, instead of passing the whole toolbox, could we only pass an openAboutDebugging function in the props, that the render function will be able to call ? This way we keep props the to the minimum amount of things needed (which can also make component testing easier) ::: devtools/client/application/src/components/WorkerList.js:31 (Diff revision 1) > + > + render() { > + let { workers, client } = this.props; > + > + return [ > + div({ className: "application-workers-container" }, maybe we could make this an `<ul>`, and either create the `<li>`s in workers.map here (and return an array from the Worker component), or make the worker component top-element (`.service-worker-container`) an `li` ? ::: devtools/client/application/src/constants.js:12 (Diff revision 1) > +const actionTypes = { > + UPDATE_WORKERS: "UPDATE_WORKERS", > +}; > + > +// flatten constants > +module.exports = Object.assign({}, actionTypes); nit: we could use the object spread operator ::: devtools/client/application/src/reducers/workers.js:11 (Diff revision 1) > + > +const { > + UPDATE_WORKERS, > +} = require("../constants"); > + > +function Workers() { I wonder if this could be misleading in some way (like we are trying to create actual workers). Maybe we could rename it WorkersState or something ? ::: devtools/client/application/src/reducers/workers.js:14 (Diff revision 1) > +} = require("../constants"); > + > +function Workers() { > + return { > + // Array of all service workers > + workers: [], nit: could we name it `list` or `all` or something else than `workers` ? It feels a bit strange when accessing the store to have `state.workers.workers`
Attachment #8965479 - Flags: review?(nchevobbe) → review+
Comment on attachment 8964976 [details] Bug 1445197 - part 1: Create an application panel for DevTools; https://reviewboard.mozilla.org/r/233710/#review240534 Looks good to me! Honza ::: devtools/client/application/initializer.js:18 (Diff revision 2) > +const { createFactory } = require("devtools/client/shared/vendor/react"); > +const { render, unmountComponentAtNode } = require("devtools/client/shared/vendor/react-dom"); > + > +const App = createFactory(require("./src/components/App")); > + > +window.Application = { A comment explaining the purpose of the Application objecdt would be nice here.
Attachment #8964976 - Flags: review?(odvarko) → review+
Comment on attachment 8965479 [details] Bug 1445197 - part 4: Implement application panel UI to display all workers; https://reviewboard.mozilla.org/r/234204/#review240536 Looks good to me, just some minor inline comments. Thanks for working on this! Honza ::: devtools/client/application/initializer.js:8 (Diff revision 1) > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > "use strict"; > > const { BrowserLoader } = ChromeUtils.import("resource://devtools/client/shared/browser-loader.js", {}); > const require = window.windowRequire = BrowserLoader({ Note that `window.windowRequire` is also for tests, so perhaps an explanatory comment would be nice here. ::: devtools/client/application/initializer.js:25 (Diff revision 1) > +// Inject to global window for testing > +window.store = store; > +window.actions = actions; Yes, I like this too. Network panel goes in the same direction. ::: devtools/client/application/src/components/Worker.js:153 (Diff revision 1) > + : null; > + > + const debugLinkDisabled = this.isRunning() ? "" : "disabled"; > + const debugLink = dom.a({ > + onClick: this.isRunning() ? this.debug : null, > + title: this.isRunning() ? null : "Only running service workers can be debugged", Not localized on purpose? ::: devtools/client/application/src/components/WorkerList.css:5 (Diff revision 1) > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > + .application-aboutdebugging-plug { Remove whitespace at the beggining of the line ::: devtools/client/application/src/components/WorkerList.js:12 (Diff revision 1) > +const PropTypes = require("devtools/client/shared/vendor/react-prop-types"); > +const { createFactory, Component } = require("devtools/client/shared/vendor/react"); > +const { a, div, h1 } = require("devtools/client/shared/vendor/react-dom-factories"); > +const Worker = createFactory(require("./Worker")); > + > +class WorkerList extends Component { I think that it's nice if every component has a comment explaining it's purpose. But, I'll let you to decide if it's needed. ::: devtools/client/application/src/components/WorkerList.js:40 (Diff revision 1) > + debugDisabled: false, > + worker, > + })) > + ), > + div({ className: "application-aboutdebugging-plug" }, > + "See about:debugging for Service Workers from other domains", Not localized on purpose?
Attachment #8965479 - Flags: review?(odvarko) → review+
Comment on attachment 8964976 [details] Bug 1445197 - part 1: Create an application panel for DevTools; https://reviewboard.mozilla.org/r/233708/#review240630 I gave the patch a try (note that it needs rebase after accessibility panel landed), it seems to work great but I always get a broken debugging with this exception: console.error: (new TypeError("form is undefined", "resource://devtools/shared/base-loader.js -> resource://devtools/shared/fronts/device.js", 53)) ::: devtools/client/application/initializer.js:46 (Diff revision 2) > + render(Provider({ store }, app), this.mount); > + > + this.client.addListener("workerListChanged", this.updateWorkers); > + this.client.addListener("serviceWorkerRegistrationListChanged", this.updateWorkers); > + this.client.addListener("registration-changed", this.updateWorkers); > + this.client.addListener("processListChanged", this.updateWorkers); Note about perf: You are calling updateWorkers in various events which may fire frequently. getWorkerForms is called from updateWorkers and spawns various RDP requests: - listServiceWorkerRegistrations and listWorkers - but also listProcesses and for all returned content processes: getProcess, listWorkers. I don't think that's something to worry about in the first iteration, but definitely something to keep in mind. ::: devtools/client/application/src/components/WorkerListEmpty.js:37 (Diff revision 2) > + } > + > + openAboutDebugging() { > + let { toolbox } = this.props; > + let win = toolbox.doc.defaultView.top; > + win.openUILinkIn("about:debugging#workers", "tab", { relatedToCurrent: true }); Brian just added an helper module to more easily open links: https://searchfox.org/mozilla-central/source/devtools/client/shared/link.js ::: devtools/client/definitions.js:444 (Diff revision 2) > } > }; > > +Tools.application = { > + id: "application", > + ordinal: 14, Looks like accessibility panel won for #14 ;) ::: devtools/client/definitions.js:455 (Diff revision 2) > + tooltip: "Application", > + inMenu: false, > + hiddenInOptions: true, > + > + isTargetSupported: function(target) { > + return true; Do you really aim to enable this panel on all toolboxes? Shouldn't you disable it from the browser toolbox (target.chrome)? from the addon toolbox (target.isAddon)? Or only enable it for common toolboxes (target.isLocalTab)? Right now, it is displayed into SW toolboxes, which looks unexpected.
Comment on attachment 8965478 [details] Bug 1445197 - part 3: Move about:debugging worker module to a shared module; https://reviewboard.mozilla.org/r/234202/#review240620 ::: devtools/client/aboutdebugging/components/workers/Panel.js:122 (Diff revision 1) > - forms.registrations.forEach(form => { > - workers.service.push({ > + > + // Use the combined information from registrations and workers for service workers. > + workers.service = forms.mergedServiceWorkers.map(form => Object.assign({}, form, { > - icon: WorkerIcon, > + icon: WorkerIcon, > - name: form.url, > - url: form.url, > + name: form.url > + })); nit: In theory, getWorkerForms doesn't set icon and name, so you could do: Object.assign({ icon: WorkerIcon, name: form.url }, form); ::: devtools/client/shared/workers/worker.js:112 (Diff revision 1) > - * @param {DebuggerClient} client > + * @param {DebuggerClient} client > * @return {Object} > * - {Array} registrations > * Array of ServiceWorkerRegistrationActor forms > * - {Array} workers > * Array of WorkerActor forms This comment should be updated. ::: devtools/client/shared/workers/worker.js:149 (Diff revision 1) > > - return { registrations, workers }; > + // Combine information from registrations and workers to create a usable list of workers > + // for consumer code. > + let mergedServiceWorkers = mergeServiceWorkerForms(registrations, workers); > + > + return { registrations, workers, mergedServiceWorkers }; I'm not sure "merged" means anything for the callsites of this method? Should we wall this attribute "serviceWorkers"? Also, `registrations` isn't used in this changeset. It looks like an internal for this method and not something to export. ::: devtools/client/shared/workers/worker.js:154 (Diff revision 1) > + return { registrations, workers, mergedServiceWorkers }; > +} > + > +module.exports = { > + debugWorker, > + getWorkerForms, I'm not convinced about the location of this code. Do you have plans about this new "devtools/client/shared/workers" folder? getWorkerForms typically looks like a Front method. Now, root actor is still an old fashion actor and has an old fashion client. May be it is better to put such code there: https://searchfox.org/mozilla-central/source/devtools/shared/client/root-client.js#30 Or is it introducing too much worker specifics in RootClient? Regarding debugWorker, gDevToolsBrowser has been an historical choice for toolbox opening: https://searchfox.org/mozilla-central/search?q=.showToolbox(&case=false&regexp=false&path=devtools-browser.js We get code related to all common toolbox opening (selectToolCommand/toggleToolboxCommand) and content process toolbox. Now, if you plan to have more worker related code, it may make sense to have this new folder.
Attachment #8965478 - Flags: review?(poirot.alex)
Comment on attachment 8964976 [details] Bug 1445197 - part 1: Create an application panel for DevTools; https://reviewboard.mozilla.org/r/233710/#review241042 Nicolas and Jan already raised the same concerns I was going to--except for the path (/devtools/client/panels/application instead of /devtools/client/application) which I will discuss on an RFC as it's out of the scope for this bug. Thank you Julian
Attachment #8964976 - Flags: review?(spenades) → review+
Comment on attachment 8964976 [details] Bug 1445197 - part 1: Create an application panel for DevTools; https://reviewboard.mozilla.org/r/233710/#review240028 Thanks for the review Nicolas! Applied all comments and suggestions except renaming ApplicationPanel to Panel > it there any need to have the toolbox reference here ? (maybe in another patch in this review ?) Yes that will be used in another patch so I removed it from this changeset, thanks for catching this! > nit: would it make sense to name it `Panel` so we're consistent with the filename ? Other panels (webconsole, netmonitor) use the same pattern, so I think I will stick to this for consistency. I agree about the added value of keeping filenames in sync with Classes, but in this case my vote would be towards renaming the files. > nit: Maybe add some jsdoc ? Sure. I quickly described the role of the panel, but I didn't write a detailed description as these things tend to go out of sync quickly. > If I read the code well, I don't think Application#destroy is async. Good catch, fixed! > should we also release `this.panelWin` and `this.toolbox` ? Sure, done.
Comment on attachment 8964976 [details] Bug 1445197 - part 1: Create an application panel for DevTools; https://reviewboard.mozilla.org/r/233710/#review240534 > A comment explaining the purpose of the Application objecdt would be nice here. Thanks for the review Honza! Just added a comment, and interestingly it made me realize something. I guess we could have a simpler structure for this panel. Splitting the init code between panel.js and initializer.js mostly makes sense if we need to support Launchpad on top of the regular dev workflow. I'll keep it for now, but maybe we can simplify later.
Comment on attachment 8964976 [details] Bug 1445197 - part 1: Create an application panel for DevTools; https://reviewboard.mozilla.org/r/233710/#review241042 Thanks! As discussed I agree that the path devtools/client/application is somewhat confusing and introducting a "panels" level in the hierarachy would make it much clearer.
(In reply to Alexandre Poirot [:ochameau] from comment #12) > Comment on attachment 8964976 [details] > Bug 1445197 - part 1: Create an application panel for DevTools; > > https://reviewboard.mozilla.org/r/233708/#review240630 > > I gave the patch a try (note that it needs rebase after accessibility panel > landed), it seems to work great but I always get a broken debugging with > this exception: > console.error: (new TypeError("form is undefined", > "resource://devtools/shared/base-loader.js -> > resource://devtools/shared/fronts/device.js", 53)) > Thanks for testing! Do you have STRs for this issue? In my tests, the application panel worked on-par with about:debugging. Just trying to see if this should be logged as a different bug or should block this one. > ::: devtools/client/application/initializer.js:46 > (Diff revision 2) > > + render(Provider({ store }, app), this.mount); > > + > > + this.client.addListener("workerListChanged", this.updateWorkers); > > + this.client.addListener("serviceWorkerRegistrationListChanged", this.updateWorkers); > > + this.client.addListener("registration-changed", this.updateWorkers); > > + this.client.addListener("processListChanged", this.updateWorkers); > > Note about perf: > You are calling updateWorkers in various events which may fire frequently. > getWorkerForms is called from updateWorkers and spawns various RDP requests: > - listServiceWorkerRegistrations and listWorkers > - but also listProcesses and for all returned content processes: getProcess, > listWorkers. > > I don't think that's something to worry about in the first iteration, but > definitely something to keep in mind. > Beyond throttling, do you have anything in mind to make this better? I will log a follow up to discuss that in more details but feel free to share insights here in the meantime. > ::: devtools/client/application/src/components/WorkerListEmpty.js:37 > (Diff revision 2) > > + } > > + > > + openAboutDebugging() { > > + let { toolbox } = this.props; > > + let win = toolbox.doc.defaultView.top; > > + win.openUILinkIn("about:debugging#workers", "tab", { relatedToCurrent: true }); > > Brian just added an helper module to more easily open links: > https://searchfox.org/mozilla-central/source/devtools/client/shared/link.js > Great thanks! > ::: devtools/client/definitions.js:444 > (Diff revision 2) > > } > > }; > > > > +Tools.application = { > > + id: "application", > > + ordinal: 14, > > Looks like accessibility panel won for #14 ;) > > ::: devtools/client/definitions.js:455 > (Diff revision 2) > > + tooltip: "Application", > > + inMenu: false, > > + hiddenInOptions: true, > > + > > + isTargetSupported: function(target) { > > + return true; > > Do you really aim to enable this panel on all toolboxes? > Shouldn't you disable it from the browser toolbox (target.chrome)? from the > addon toolbox (target.isAddon)? Or only enable it for common toolboxes > (target.isLocalTab)? > > Right now, it is displayed into SW toolboxes, which looks unexpected. Good point! Switched it to target.isLocalTab (we can revisit later if needed).
Comment on attachment 8965478 [details] Bug 1445197 - part 3: Move about:debugging worker module to a shared module; https://reviewboard.mozilla.org/r/234202/#review240620 > I'm not sure "merged" means anything for the callsites of this method? > Should we wall this attribute "serviceWorkers"? > > Also, `registrations` isn't used in this changeset. > It looks like an internal for this method and not something to export. Sounds good, fixed. > I'm not convinced about the location of this code. > Do you have plans about this new "devtools/client/shared/workers" folder? > > getWorkerForms typically looks like a Front method. > Now, root actor is still an old fashion actor and has an old fashion client. > May be it is better to put such code there: > https://searchfox.org/mozilla-central/source/devtools/shared/client/root-client.js#30 > Or is it introducing too much worker specifics in RootClient? > > Regarding debugWorker, gDevToolsBrowser has been an historical choice for toolbox opening: > https://searchfox.org/mozilla-central/search?q=.showToolbox(&case=false&regexp=false&path=devtools-browser.js > We get code related to all common toolbox opening (selectToolCommand/toggleToolboxCommand) and > content process toolbox. > > Now, if you plan to have more worker related code, it may make sense to have this new folder. Ok to move debugWorker to devtools-browser (maybe a dedicated module with an explicit name would be nice) I don't like devtools/client/shared/workers either :) I want to obviously share this logic between about:debugging and the application panel, because it has some complexity. On the other hand I'm not sure if all the workarounds we have at the moment will stay after the multi-e10s sw refactor. That's the main reason I wanted a separate file here. Although it could have been a worker-utils.js in devtools/client/shared rather than having its own folder. I will move it to root-client for now. I really don't know what name to pick though. Maybe listAllWorkers with a comment to explain how it differs from listWorkers?
> Thanks for testing! Do you have STRs for this issue? In my tests, > the application panel worked on-par with about:debugging. Just trying > to see if this should be logged as a different bug or should block this one. Actually seems like service worker debugging is broken right now. I am guessing debugger v32 with https://github.com/devtools-html/debugger.html/pull/5761 broke it ...
Opened https://github.com/devtools-html/debugger.html/pull/5929 to fix the sw blank debugger issue.
Comment on attachment 8965479 [details] Bug 1445197 - part 4: Implement application panel UI to display all workers; https://reviewboard.mozilla.org/r/234204/#review240040 > or maybe we could expose them through methods on Application (`getStore`, `getActions` )? > Not feeling strongly about this though That sounds good, I didn't add the getters yet since they are test only. I will add them together with the tests! I moved the creation of `store` and `actions` to the bootstrap method. They are now stored on window.Application (let me know if I should keep them out, I tested quickly and didn't see any issue but you never know!) > Lately I tried to make sense of "complex" layouts (flex, grid) in css files by adding ascii visualisations of what it should look like (see https://searchfox.org/mozilla-central/rev/d0413b711da4dac72b237d0895daba2537f1514c/devtools/client/themes/webconsole.css#706-722 or https://hg.mozilla.org/try/diff/e976bc9e281c/devtools/client/themes/webconsole.css#l1.14 ) in order to make it easier for contributors to know what to expect. > > here it might make sense to do so (even more I'd say because we don't know which children .application has since they're declared in separate css files) Sounds like a good idea! Gave it a try both here and in Worker.css. > should we put the `box-sizing: border-box` on `*` in App.css so we don't have unexpected behaviour when introducing new elements ? Good catch, that should be defined in application.css once and for all. > maybe this is a bit brittle ? Could we add a classname to this div we want to style, and here replace the rule with `.service-worker-container .the-super-div` ? I can actually just move it to .service-worker-container here i think. I will do that for now, but feel free to push back! > here, instead of passing the whole toolbox, could we only pass an openAboutDebugging function in the props, that the render function will be able to call ? > This way we keep props the to the minimum amount of things needed (which can also make component testing easier) Sure. will make testing easier. Reused a "serviceContainer" approach similar to what the console does. > maybe we could make this an `<ul>`, and either create the `<li>`s in workers.map here (and return an array from the Worker component), or make the worker component top-element (`.service-worker-container`) an `li` ? Done (worker component returns a li) > I wonder if this could be misleading in some way (like we are trying to create actual workers). Maybe we could rename it WorkersState or something ? renamed to WorkersState > nit: could we name it `list` or `all` or something else than `workers` ? It feels a bit strange when accessing the store to have `state.workers.workers` Sure, renamed to list.
Comment on attachment 8965479 [details] Bug 1445197 - part 4: Implement application panel UI to display all workers; https://reviewboard.mozilla.org/r/234204/#review240536 > Note that `window.windowRequire` is also for tests, so perhaps an explanatory comment would be nice here. Good catch. I removed it and will reintroduce it with tests then! > Not localized on purpose? Yes, I'm planning to add localization in 2 steps: first use l10n APIs in Bug 1450071, and open it to localization teams in a second step (once strings are stable enough) > Remove whitespace at the beggining of the line Fixed! > I think that it's nice if every component has a comment explaining it's purpose. But, I'll let you to decide if it's needed. Sure I'll try to have a short description for each component.!
Thanks everyone for the reviews so far! Updated the patches with normally all the current comments addressed.
Comment on attachment 8965479 [details] Bug 1445197 - part 4: Implement application panel UI to display all workers; https://reviewboard.mozilla.org/r/234204/#review241694 ::: devtools/client/application/src/components/Worker.css:25 (Diff revision 2) > +.service-worker-container > div { > +} leftover ?
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #30) > Comment on attachment 8965479 [details] > Bug 1445197 - part 4: Implement application panel UI to display all workers; > > https://reviewboard.mozilla.org/r/234204/#review241694 > > ::: devtools/client/application/src/components/Worker.css:25 > (Diff revision 2) > > +.service-worker-container > div { > > +} > > leftover ? Yep good catch!
Comment on attachment 8965478 [details] Bug 1445197 - part 3: Move about:debugging worker module to a shared module; https://reviewboard.mozilla.org/r/234202/#review241718 Thanks Julian, it looks great! ::: devtools/client/framework/devtools-browser.js:402 (Diff revision 2) > + * worker actor form to debug > + */ > + openWorkerToolbox(client, workerActor) { > + client.attachWorker(workerActor, (response, workerClient) => { > + let workerTarget = TargetFactory.forWorker(workerClient); > + gDevTools.showToolbox(workerTarget, "jsdebugger", Toolbox.HostType.WINDOW) You may pass `null` instead of `"jsdebugger"` here in order to open the toolbox against whatever was you last tool opened. It is especially useful to do that when you click on `Debug` while a toolbox is already opened. Instead of focusing the toolbox *and* switching to the debugger, it will only focus the toolbox. ::: devtools/shared/client/root-client.js:166 (Diff revision 2) > return this.request(packet); > }, > > /** > + * Retrieve the service worker registrations currently available for the provided > + * DebuggerClient. for the provided DebuggerClient => for this client? ::: devtools/shared/client/root-client.js:239 (Diff revision 2) > + /** > + * Retrieve all service worker registrations as well as workers from the parent > + * and child processes. Listing service workers involves merging information coming from > + * registrations and workers, this method will combine this information to present a > + * unified array of serviceWorkers. If you are only interested in other workers, use > + * listWorkers. `listAllWorkers` sounds like a good name and this comment is helpful. ::: devtools/shared/client/root-client.js:254 (Diff revision 2) > + let workers = []; > + > + try { > + // List service worker registrations > + ({ registrations } = > + await this.listServiceWorkerRegistrations()); We may move this into `_mergeServiceWorkerForms` as it isn't used in this method. ::: devtools/shared/client/root-client.js:279 (Diff revision 2) > + } catch (e) { > + // Something went wrong, maybe our client is disconnected? > + } > + > + return { > + // Filter out service workers that will returned in the serviceWorkers property. will +be+ returned?
Attachment #8965478 - Flags: review?(poirot.alex) → review+
Comment on attachment 8965477 [details] Bug 1445197 - part 2: Add a temporary icon for the application panel; https://reviewboard.mozilla.org/r/234200/#review243042 Perfect, we can do with this in the meantime. Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1454678 to replace this icon with a final one.
Attachment #8965477 - Flags: review?(spenades) → review+
As mentioned to :ochameau and :jryans on slack, there is an issue with the current approach when it comes to: - hitting breakpoints in service worker scripts - getting the accurate status of the service worker To retrieve service workers we call listWorkers both: - on the Root actor - on all the ChildProcess actors And to get child process actors we call https://searchfox.org/mozilla-central/source/devtools/server/actors/root.js#524-527 . This is guarded by `if (!DebuggerServer.allowChromeProcess)`. For about:debugging this is true, but not for a regular toolbox. The consequence is that the application panel is not aware about the worker instances running in content processes. If you start a service worker manually (with the Start link), a worker will now be returned from the Root actor's listWorkers method (?). It can be used to open a Toolbox, and will show the correct sources, but setting a breakpoint will never break as expected. I am not sure how to proceed. I doubt we want to set allowChromeProcess to true for all toolboxes, or remove the limitation on getProcess. Maybe introduce a new dedicated API on the root actor that would return all workers, not blocked on allowChromeProcess? [Note that this works almost by "chance" for about:debugging because we first grab the WorkerActor from the RootActor and override it with the correct one from the ChildProcessActor.]
I imagine we are going to sync with fission work as I get to understand that the client is going to connect to many target at ones, these targets could be these workers living in other processes. So I would like to hear from :jryans here first before looking into your suggestions.
The main security concern with `allowChromeProcess` as I understand it is ensuring the user has opted into allowing _remote_ debugging of chrome processes. Looking at the main vectors that start remote servers: * GCLI listen[1]: Tied to devtools.chrome.enabled * --start-debugger-server[2]: Requires devtools.chrome.enabled = true (a bit strange, but good enough) Both remote paths create separate loaders and servers, so they are isolated from whatever choices we might make about local debugging. Given this info, I think we can safely change `target#makeRemote` to set `allowChromeProcess = true` for `isLocalTab`, which seems like it will solve the issues you are hitting. (Also, as part of Fission I am planning a new discovery mechanism for things like workers, so I'd rather not and something temporary here unless there is a strong need for it.) [1]: https://searchfox.org/mozilla-central/rev/f65d7528e34ef1a7665b4a1a7b7cdb1388fcd3aa/devtools/shared/gcli/commands/listen.js#28 [2]: https://searchfox.org/mozilla-central/rev/f65d7528e34ef1a7665b4a1a7b7cdb1388fcd3aa/devtools/startup/devtools-startup.js#727
Thank you both for your replies! I will log a separate bug to set allowChromeProcess to true in makeRemote().
See Also: → 1454895
Blocks: 1454931
Moved part 5 (help screen when no service worker is displayed) to Bug 1454931
Attachment #8965480 - Attachment is obsolete: true
Attachment #8965480 - Flags: review?(spenades)
Comment on attachment 8965479 [details] Bug 1445197 - part 4: Implement application panel UI to display all workers; https://reviewboard.mozilla.org/r/234204/#review243050 Made a couple non breaking comments - mostly about CSS! but looks good to land I think! Thank you ::: devtools/client/application/application.css:23 (Diff revision 5) > + > +ul { > + list-style: none; > + margin: 0; > + padding: 0; > +} These styles look as something that should possibly be shared across [React-based] panels; don't want to block this review on this, but I wonder if we've considered extracting them or if it would be not trivial / too much work. ::: devtools/client/application/src/components/App.css:20 (Diff revision 5) > + * | Link to about:debugging | > + * +---------------------------------------------+ > + */ > +.application { > + height: 100%; > + padding: 0 0 0 20px; This comment relates to all the other values in px in this file: why do we hard code these px values? Can we use variables or is this a common practice in DevTools? ::: devtools/client/application/src/components/Worker.css:8 (Diff revision 5) > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > + /* > + * The current layout of a service worker item is > + * > + * +----------------------------+----------------+ This diagram is a very nice touch! ::: devtools/client/application/src/components/Worker.css:21 (Diff revision 5) > +.service-worker-container { > + margin-bottom: 20px; > + width: 100%; > + max-width: 600px; > + position: relative; > + line-height: 20px; Could we use a unitless value here? to avoid this scenario: https://developer.mozilla.org/en-US/docs/Web/CSS/line-height#Prefer_unitless_numbers_for_line-height_values ::: devtools/client/application/src/components/Worker.css:39 (Diff revision 5) > + font-weight: bold; > +} > + > +.service-worker-meta-name { > + color: var(--grey-50); > + width: 60px; how is this going to work with long names? will they break? I guess we'll have to test/adapt in the future ::: devtools/client/application/src/components/Worker.js:52 (Diff revision 5) > + this.unregister = this.unregister.bind(this); > + } > + > + debug() { > + if (!this.isRunning()) { > + console.log("Non-running service workers can not be debugged"); What does this message mean? It sounds a bit "too technical", did you mean "Service Workers which are not running cannot be debugged?"
Attachment #8965479 - Flags: review+
Comment on attachment 8965479 [details] Bug 1445197 - part 4: Implement application panel UI to display all workers; https://reviewboard.mozilla.org/r/234204/#review243050 > This comment relates to all the other values in px in this file: why do we hard code these px values? Can we use variables or is this a common practice in DevTools? We generally use variables for things that need to change depending on the style or platform. I believe this is consistent with the rest of DevTools, which doesn't necessarily make it right. A RFC would probably be a good fit for a proposal related to this. > This diagram is a very nice touch! Suggested by Nicolas, but I agree :) > Could we use a unitless value here? to avoid this scenario: https://developer.mozilla.org/en-US/docs/Web/CSS/line-height#Prefer_unitless_numbers_for_line-height_values Sure, switched to 1.5 > how is this going to work with long names? will they break? I guess we'll have to test/adapt in the future Good point, for now we just have "Source" and "Status" but in other locales this might go over 60px. To be safe for now let's switch to min-width: 60px; > What does this message mean? It sounds a bit "too technical", did you mean "Service Workers which are not running cannot be debugged?" Yes. I wanted to keep the sentence consistent in its structure with the other log "Running or inactive service workers can not be started", but it's true that "Non-running" is hard to grasp.
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3e9acd2c8bd3 part 1: Create an application panel for DevTools;r=Honza,nchevobbe,sole https://hg.mozilla.org/integration/autoland/rev/a0345830e1cf part 2: Add a temporary icon for the application panel;r=sole https://hg.mozilla.org/integration/autoland/rev/ba9ffea6d216 part 3: Move about:debugging worker module to a shared module;r=ochameau https://hg.mozilla.org/integration/autoland/rev/4b6f77cfd686 part 4: Implement application panel UI to display all workers;r=Honza,nchevobbe,sole
Part of the code to list workers that moved to root-client.js was no longer protected by the try catch, hence the failures in debug mode.
Flags: needinfo?(jdescottes)
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/eae47509bc27 part 1: Create an application panel for DevTools;r=Honza,nchevobbe,sole https://hg.mozilla.org/integration/autoland/rev/11f5214dc193 part 2: Add a temporary icon for the application panel;r=sole https://hg.mozilla.org/integration/autoland/rev/086638cc851c part 3: Move about:debugging worker module to a shared module;r=ochameau https://hg.mozilla.org/integration/autoland/rev/422bbd33b4e3 part 4: Implement application panel UI to display all workers;r=Honza,nchevobbe,sole
Backout by aciure@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/582de61479ed Backed out 4 changesets for ESlint failure, on a CLOSED TREE a=backout
On top of the eslint failure, there was still an intermittent aboutdebugging test. New try with several rebuilds to hopefully catch this: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad952b16da6b93b449bdbb103a0f55a7318c7c8c
Depends on: 1455146
Still getting various about:debugging failures. Reverting the listWorkers code to an earlier implementation, performing the calls in the same sequence as before. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c613e741adec9848609c39dc00d6ea03099a83f9
New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1107e3c41af5a9da8bb2bfff1ccc6ac559068bba (this last version is much closer to the initial implementation in about:debugging, but seems to be green on try)
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/58b37794b1b8 part 1: Create an application panel for DevTools;r=Honza,nchevobbe,sole https://hg.mozilla.org/integration/autoland/rev/b4a9a31af41e part 2: Add a temporary icon for the application panel;r=sole https://hg.mozilla.org/integration/autoland/rev/42759c236661 part 3: Move about:debugging worker module to a shared module;r=ochameau https://hg.mozilla.org/integration/autoland/rev/f0e5fbd39274 part 4: Implement application panel UI to display all workers;r=Honza,nchevobbe,sole
You shouldn't forget to set the dev-doc-needed keyword for new or changed features, so the changes get documented. Sebastian
Keywords: dev-doc-needed
While I agree this should be documented, the panel is preffed off, can only be enable via about:config, and will change significantly before we advertise it.
Keywords: dev-doc-needed
(In reply to Julian Descottes [:jdescottes][:julian] from comment #81) > While I agree this should be documented, the panel is preffed off, can only > be enable via about:config, and will change significantly before we > advertise it. Fine, though I think it should still be listed at https://developer.mozilla.org/en-US/Firefox/Experimental_features#Developer_Tools at least. Do you agree? Sebastian
Flags: needinfo?(jdescottes)
I wasn't aware of this section - which is completely outdated for DevTools. I would prefer not advertising the panel at all until we complete https://bugzilla.mozilla.org/show_bug.cgi?id=devtools-app-panel-m1 . Maybe a follow up bug blocked on devtools-app-panel-m1 to discuss documentation plans?
Flags: needinfo?(jdescottes)
Mass move to the new application panel component.
Component: Developer Tools → Developer Tools: Application Panel
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: