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

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

	Julian Descottes [:jdescottes] Assignee
	Comment 7 • 7 years ago

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

	Nicolas Chevobbe [:nchevobbe]
	Comment 8 • 7 years ago
mozreview-review

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` ?

	Nicolas Chevobbe [:nchevobbe]
	Comment 9 • 7 years ago
mozreview-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`

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 10 • 7 years ago
mozreview-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.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 11 • 7 years ago
mozreview-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?

	Alexandre Poirot [:ochameau]
	Comment 12 • 7 years ago
mozreview-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.

	Alexandre Poirot [:ochameau]
	Comment 13 • 7 years ago
mozreview-review

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.

	Soledad Penades [:sole] [:spenades]
	Comment 14 • 7 years ago
mozreview-review

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

	Julian Descottes [:jdescottes] Assignee
	Comment 15 • 7 years ago
mozreview-review-reply

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.

	Julian Descottes [:jdescottes] Assignee
	Comment 16 • 7 years ago
mozreview-review-reply

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.

	Julian Descottes [:jdescottes] Assignee
	Comment 17 • 7 years ago
mozreview-review-reply

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.

	Julian Descottes [:jdescottes] Assignee
	Comment 18 • 7 years ago

(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).

	Julian Descottes [:jdescottes] Assignee
	Comment 19 • 7 years ago
mozreview-review-reply

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?

	Julian Descottes [:jdescottes] Assignee
	Comment 20 • 7 years ago

> 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 ...

	Julian Descottes [:jdescottes] Assignee
	Comment 21 • 7 years ago

Opened https://github.com/devtools-html/debugger.html/pull/5929 to fix the sw blank debugger issue.

	Julian Descottes [:jdescottes] Assignee
	Comment 27 • 7 years ago
mozreview-review-reply

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.

	Julian Descottes [:jdescottes] Assignee
	Comment 28 • 7 years ago
mozreview-review-reply

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.!

	Julian Descottes [:jdescottes] Assignee
	Comment 29 • 7 years ago

Thanks everyone for the reviews so far! Updated the patches with normally all the current comments addressed.

	Nicolas Chevobbe [:nchevobbe]
	Comment 30 • 7 years ago
mozreview-review

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 ?

	Julian Descottes [:jdescottes] Assignee
	Comment 33 • 7 years ago

(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!

	Alexandre Poirot [:ochameau]
	Comment 34 • 7 years ago
mozreview-review

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?

	Soledad Penades [:sole] [:spenades]
	Comment 43 • 7 years ago
mozreview-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.

	Julian Descottes [:jdescottes] Assignee
	Comment 44 • 7 years ago

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.]

	Alexandre Poirot [:ochameau]
	Comment 45 • 7 years ago

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.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 46 • 7 years ago

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

	Julian Descottes [:jdescottes] Assignee
	Comment 47 • 7 years ago

Thank you both for your replies! I will log a separate bug to set allowChromeProcess to true in makeRemote().

	Julian Descottes [:jdescottes] Assignee
	Comment 48 • 7 years ago

Moved part 5 (help screen when no service worker is displayed) to Bug 1454931

	Soledad Penades [:sole] [:spenades]
	Comment 50 • 7 years ago
mozreview-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

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?"

	Julian Descottes [:jdescottes] Assignee
	Comment 51 • 7 years ago
mozreview-review-reply

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.

	Pulsebot
	Comment 53 • 7 years ago

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

	Andreea Pavel [:apavel]
	Comment 54 • 7 years ago

Backed out for failing  devtools/client/aboutdebugging/test/browser_service_workers.js and devtools/client/framework/test/browser_toolbox_tool_ready.js and devtools/client/aboutdebugging/test/browser_service_workers_fetch_flag.js

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=4b6f77cfd686930e3be525c51d10b825549c5eae

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=174338392&repo=autoland&lineNumber=2539

https://treeherder.mozilla.org/logviewer.html#?job_id=174338406&repo=autoland&lineNumber=4833

https://treeherder.mozilla.org/logviewer.html#?job_id=174337739&repo=autoland&lineNumber=2380

Backout:  https://hg.mozilla.org/integration/autoland/rev/03c304d9325beea53dad9c40b3979317b29ab475

	Julian Descottes [:jdescottes] Assignee
	Comment 57 • 7 years ago

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.

	Pulsebot
	Comment 58 • 7 years ago

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

	Pulsebot
	Comment 61 • 7 years ago

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

	Julian Descottes [:jdescottes] Assignee
	Comment 65 • 7 years ago

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

	Julian Descottes [:jdescottes] Assignee
	Comment 66 • 7 years ago

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

	Julian Descottes [:jdescottes] Assignee
	Comment 77 • 7 years ago

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)

	Pulsebot
	Comment 78 • 7 years ago

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

	Dorel Luca [:dluca]
	Comment 79 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/58b37794b1b8
https://hg.mozilla.org/mozilla-central/rev/b4a9a31af41e
https://hg.mozilla.org/mozilla-central/rev/42759c236661
https://hg.mozilla.org/mozilla-central/rev/f0e5fbd39274

	Sebastian Zartner [:sebo]
	Comment 80 • 6 years ago

You shouldn't forget to set the dev-doc-needed keyword for new or changed features, so the changes get documented.

Sebastian

	Julian Descottes [:jdescottes] Assignee
	Comment 81 • 6 years ago

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.

	Sebastian Zartner [:sebo]
	Comment 82 • 6 years ago

(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

	Julian Descottes [:jdescottes] Assignee
	Comment 83 • 6 years ago

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?

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

Mass move to the new application panel component.