1196785 - (about:debugging) Create an about:debugging page to list all debuggable targets.

Status: VERIFIED FIXED

(DevTools :: about:debugging, defect)

Description

[Jan Keromnes [:janx]]

This page should act as a comprehensive catalog of all the targets that the developer tools can connect to:
- Tabs / Apps
- Add-ons / Extensions
- Workers (shared workers, service workers...)

In addition to local targets, it should also display remote targets from:
- Other browsers (Chrome, Safari...)
- Other devices (over USB / Wi-Fi, running Firefox OS / Android / iOS)
- Firefox OS Simulators

... with appropriate controls for device discovery, connection and management (e.g. configuration, settings, preferences...).

Comment 1

[Jan Keromnes [:janx]]

(Renaming to "about:debug", as bikeshedded in https://etherpad.mozilla.org/about-inspect)

Comment 2

[Jan Keromnes [:janx]]

Attachment: Make React compatible with XHTML.

Comment 3

[Jan Keromnes [:janx]]

Attachment: Alphabetically sort redirected about:pages.

Comment 4

[Jan Keromnes [:janx]]

Attachment: Make the common category style work in HTML.

Comment 5

[Jan Keromnes [:janx]]

Comment on attachment 8654846 [details] [diff] [review]
Make React compatible with XHTML.

Review of attachment 8654846 [details] [diff] [review]:
-----------------------------------------------------------------

James, we talked about this earlier in #devtools. Should I add an `UPDATING` file? Can I weigh in on a React issue somewhere, or should I file my own?

Comment 6

[Jan Keromnes [:janx]]

Comment on attachment 8654848 [details] [diff] [review]
Alphabetically sort redirected about:pages.

David, this was bothering me more than it should have, sorry!

Comment 7

[Jan Keromnes [:janx]]

Comment on attachment 8654849 [details] [diff] [review]
Make the common category style work in HTML.

Review of attachment 8654849 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Jared, I hope you're the right person to ask for this (if not, please punt the review to someone else).

The `#categories` and `.category` styles are declared as XUL-only, but I'd like to use them in an XHTML page. These are the changes I had to make for my page to display properly.

::: toolkit/themes/shared/in-content/common.inc.css
@@ +461,5 @@
>  /* Checkboxes and radio buttons */
>  
>  /* Hide the actual checkbox */
>  html|input[type="checkbox"] {
> +  visibility: hidden;

Note: This is a drive-by fix. When hovering over the small hidden checkbox element, the fake checkbox loses highlight (reproducible in about:performance, when hovering towards top-left inside a fake checkbox). Changing `opacity:0` to `visibility:hidden` fixes that.

Comment 8

[Jan Keromnes [:janx]]

Comment on attachment 8654848 [details] [diff] [review]
Alphabetically sort redirected about:pages.

Or maybe docshell-peer Johnny? I simply sorted the (redirection) about:pages alphabetically.

Comment 9

[Jan Keromnes [:janx]]

Attachment: Alphabetically sort redirected about:pages.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8654849 [details] [diff] [review]
Make the common category style work in HTML.

Review of attachment 8654849 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/themes/shared/in-content/common.inc.css
@@ +588,5 @@
>    -moz-padding-start: 11px; /* compensate the 4px border */
>    -moz-border-start: solid 4px var(--in-content-border-highlight);
>  }
>  
> +*|*#categories[keyboard-navigation="true"]:-moz-focusring > xul|*.category[current] {

The second 'xul' should also have been replaced with the wildcard. This should be:
> *|*#categories[keyboard-navigation="true"]:-moz-focusring > *|*.category[current] {

Comment 11

[James Long (:jlongster)]

Comment on attachment 8654846 [details] [diff] [review]
Make React compatible with XHTML.

Review of attachment 8654846 [details] [diff] [review]:
-----------------------------------------------------------------

Is there any way you can just use the resource:// URL for loading React? We are already building React into `resource:///modules/devtools/shared/content/react.js`, so you're building it in twice. You're also not building in the dev version here.

You might also want to look into my new `BrowserLoader`: https://github.com/mozilla/gecko-dev/blob/master/browser/devtools/shared/browser-loader.js

This allows you to write frontend code as CommonJS modules, but the modules have access to the `window` global scope. Here's an example of how it's used: https://github.com/mozilla/gecko-dev/blob/fx-team/browser/devtools/debugger/debugger-controller.js#L106 Basically, it only loads modules under that URL you give to it, and everything else is loaded with the devtools loader.

If you're using a loader, you can just `require("devtools/shared/content/react")` and you'll get the React from the resource:// URL. An additional benefit is that the dev version of React will be loaded if the DEBUG_JS_MODULES config value is set.

If you don't want to use the loader or look into any of that, at least just load React at `resource:///modules/devtools/shared/content/react.js` and don't add it to jar.mn.

Also, can you do two things regarding patching React:

* Make a diff with the original version and save it as `react-0.13.3.patch` (or whatever filename)
* Add an UPGRADING file that lists the reasons for the patch, and say that the patch file needs to be applied to newer versions (or new versions need to be tested to make sure it works)

I wonder if we should also just go ahead and make a mochitest in browser/devtools/shared/test that tests the specific things we are changing. What do you think?

Comment 12

[James Long (:jlongster)]

(In reply to Jan Keromnes [:janx] from comment #5)
> Comment on attachment 8654846 [details] [diff] [review]
> Make React compatible with XHTML.
> 
> Review of attachment 8654846 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> James, we talked about this earlier in #devtools. Should I add an `UPDATING`
> file? Can I weigh in on a React issue somewhere, or should I file my own?

Sorry just saw your actual comment :) Here's the React issue: https://github.com/facebook/react/issues/4138

Comment 13

[Jan Keromnes [:janx]]

Attachment: Make the common category style work in HTML.

Changed second 'xul' to wildcard (see comment 10).

Comment 14

[Jan Keromnes [:janx]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1a84252f2a0

Comment 15

[Jan Keromnes [:janx]]

Bikeshedding follow-up: "about:debugging" better respects the apparent naming conventions of about:pages.

Comment 16

[Jan Keromnes [:janx]]

All green, ready for launch.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ed4e41f08f0a
https://hg.mozilla.org/integration/fx-team/rev/3639f5216ac0

Comment 18

[Jan Keromnes [:janx]]

Attachment: [WIP] Implement an about:debugging page to list all debuggable devtools targets. r=

Work in progress.

Comment 19

[Jan Keromnes [:janx]]

Comment on attachment 8657108 [details] [diff] [review]
[WIP] Implement an about:debugging page to list all debuggable devtools targets. r=

Ryan, this is the work-in-progress patch for my about:debugging prototype.

I'm uploading it so you can get a general idea of where it's headed, and give feedback early on. I'm looking for general comments like "please factor this out / re-use that module", not for nits like "please rename this variable / file a bug for this / remove this comment".

Features:
- "about:debugging" page with three target categories
- Checkboxes to quickly flip relevant prefs
- Debuggable addons (if prefs are correctly set up)

Missing:
- Connecting to Devices/Simulators: I'll start to re-implement relevant parts of app-manager.js
- Debuggable workers: This patch is able to list WorkerDebuggers, but not open toolboxes on them
- Connecting to local chrome-level targets (e.g. open BrowserToolbox)
- Tests

Once I'm able to properly connect to Devices/Simulators (offering a list of tabs that you can really debug), I'll upload a prototype patch for someone to review (e.g. you, if you'd like).

Comment 20

[Jan Keromnes [:janx]]

Attachment: [WIP] Screenshot of "Workers" page.

[WIP] Screenshot of "Workers" page.

Comment 21

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8657108 [details] [diff] [review]
[WIP] Implement an about:debugging page to list all debuggable devtools targets. r=

Review of attachment 8657108 [details] [diff] [review]:
-----------------------------------------------------------------

Jan, overall this seems nice so far!

I am hopeful you'll be constructing reusable module(s) for the connecting bits you reimplement from app-manager.js.  I can imagine other consumers using this down the road, so it would help to keep it clearly separated from UI code.

I know it's not too big for now, but seeing all the code in one file is scaring me a bit, since I watched WebIDE grow to an enormous tangled blob.

James, are there typical modularity patterns that we should use / that the React community uses?  I guess on extreme is "separate file per UI component".  A middle ground is groups similar things in a file.

Does it make sense to use some kind of store pattern for each of the "sets of things", like add-ons, runtimes, workers, etc.?

::: browser/devtools/aboutdebugging/aboutdebugging.css
@@ +45,5 @@
> +}
> +
> +/* RUNTIMES */
> +
> +.runtimes {

I'd put these separate sections in new files.  Don't see anything wrong with using files to organize concepts.

::: browser/devtools/aboutdebugging/aboutdebugging.js
@@ +15,5 @@
> +const { RuntimeScanners, RuntimeTypes } = require("devtools/webide/runtimes");
> +const TabStore = require("devtools/webide/tab-store");
> +
> +
> +// HELPERS.

Some kind of utils module local to your tool seems good for stuff like this.

@@ +161,5 @@
> +      Services.prefs.addObserver(pref, updateCheckbox, false);
> +      updateCheckbox();
> +    });
> +
> +    // RUNTIMES

Can we move each of these "section header" blocks out to their own modules?  Then this step can just call each module to init, and each one on its own will simpler and easier to follow.

Comment 22

[James Long (:jlongster)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #21)
> 
> I am hopeful you'll be constructing reusable module(s) for the connecting
> bits you reimplement from app-manager.js.  I can imagine other consumers
> using this down the road, so it would help to keep it clearly separated from
> UI code.
> 
> I know it's not too big for now, but seeing all the code in one file is
> scaring me a bit, since I watched WebIDE grow to an enormous tangled blob.
> 
> James, are there typical modularity patterns that we should use / that the
> React community uses?  I guess on extreme is "separate file per UI
> component".  A middle ground is groups similar things in a file.

Yep, one UI component per file is definitely standard. I'd say it's very typical for components to be reused anyway, so it's good for that to be the default. A more practical reason is it makes unit testing a lot simpler and clearer. Projects like react-hot-reload work better when it's modular like that too.

Another reason (I haven't actually looked at this patch yet) is that you should not use `React.createFactory` creating a component. Since we are not using JSX, we need to manually create "factories" which just are functions that instanstiate components. So `Foo({ bar: baz })` is equivalent to `<Foo bar=baz>`. Without "factories" you'd use the raw React API, `React.createElement(Foo, { bar: baz })`.

We should separate components out individually into separate files, and call `React.createFactory` when *importing* them (not exporting). The reason for this is, again, testing: if you export a factory you tests cannot access the normal class instance, which you use for stuff like `assert(instance.type === ComponentClass)`.

Anyway, I went off on a tangent, and I haven't even looked at this patch. I can if you all want me too. I just wanted to explain the general guidelines for how to structure components.

> 
> Does it make sense to use some kind of store pattern for each of the "sets
> of things", like add-ons, runtimes, workers, etc.?

Not sure what you mean exactly. Do you mean for component types? I'd like to just use the module system, and require components as needed (and they can be grouped by folder/location).

Comment 23

[James Long (:jlongster)]

I thought I was ni? on this bug but just noticed it was for feedback. I can glance this patch tomorrow if you want.

Comment 24

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to James Long (:jlongster) from comment #22)
> Anyway, I went off on a tangent, and I haven't even looked at this patch. I
> can if you all want me too. I just wanted to explain the general guidelines
> for how to structure components.

This was all quite helpful info, at least for me since my React experience is still minimal so far.  Maybe we could make a small "React Tips / Style Guide" to document things like this?  Doesn't have to be strict rules, but it's nice to have some guidelines to direct people to.

> > Does it make sense to use some kind of store pattern for each of the "sets
> > of things", like add-ons, runtimes, workers, etc.?
> 
> Not sure what you mean exactly. Do you mean for component types? I'd like to
> just use the module system, and require components as needed (and they can
> be grouped by folder/location).

I think it will make more sense when you look at the patch...  There are various data models tracking lists of things, where a thing might be an add-on or a runtime.  Any of the things this page can debug.  I think there may some smarter components we could use to notice changes more intelligently.  Maybe your fluxify lib can help here?

Comment 25

[James Long (:jlongster)]

Comment on attachment 8657108 [details] [diff] [review]
[WIP] Implement an about:debugging page to list all debuggable devtools targets. r=

Review of attachment 8657108 [details] [diff] [review]:
-----------------------------------------------------------------

Oh I see, you literally call `React.createElement`. It works for now, but it is quite verbose and I would recommend separating the ui components into different files in a `components` folder and wrapping them with `createFactory` when importing them. I don't know if we should block this on that or not, at least file a follow-up bug I guess.

I didn't look too closely at the code, but if you aren't running into too many issues passing things deep into the component tree, I think storing state at the top-level as component state is OK. I think as an app grows it's nice to eventually move that state out into something like a simple flux store, but I don't think you need to do that now if passing state down and events up isn't messy yet. With a flux store you also get stuff like replayability (https://www.youtube.com/watch?v=xsSnOQynTHs) but it's fine if you want to wait on that.

Comment 26

[James Long (:jlongster)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #24)
> (In reply to James Long (:jlongster) from comment #22)
> > Anyway, I went off on a tangent, and I haven't even looked at this patch. I
> > can if you all want me too. I just wanted to explain the general guidelines
> > for how to structure components.
> 
> This was all quite helpful info, at least for me since my React experience
> is still minimal so far.  Maybe we could make a small "React Tips / Style
> Guide" to document things like this?  Doesn't have to be strict rules, but
> it's nice to have some guidelines to direct people to.

Yeah, I've just been blocked on trying to figure out where the heck to put stuff like that. Have we decided that MDN is our de facto place for docs?

Comment 27

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to James Long (:jlongster) from comment #26)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #24)
> > (In reply to James Long (:jlongster) from comment #22)
> > > Anyway, I went off on a tangent, and I haven't even looked at this patch. I
> > > can if you all want me too. I just wanted to explain the general guidelines
> > > for how to structure components.
> > 
> > This was all quite helpful info, at least for me since my React experience
> > is still minimal so far.  Maybe we could make a small "React Tips / Style
> > Guide" to document things like this?  Doesn't have to be strict rules, but
> > it's nice to have some guidelines to direct people to.
> 
> Yeah, I've just been blocked on trying to figure out where the heck to put
> stuff like that. Have we decided that MDN is our de facto place for docs?

We did seem to agree to MDN, but then we haven't moved anything else there yet.  Personally, I would say put it on the wiki, since that's where similar documents are for now.  But writing the content is the big thing, I don't personally care where it is as long as it has a URL. :)

Comment 28

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Tagging this bug for UX help for now, but I would assume this will happen in a follow up, so we can also move the tag there instead.

Comment 29

[Jan Keromnes [:janx]]

Attachment: Add worker type to WorkerActor form.

Comment 30

[Jan Keromnes [:janx]]

Attachment: Add rootActor.listWorkers() to list all registered workers.

Comment 31

[Jan Keromnes [:janx]]

Comment on attachment 8660851 [details] [diff] [review]
Add worker type to WorkerActor form.

I needed this to display {service,shared,other}workers in separate lists.

Comment 32

[Jan Keromnes [:janx]]

Comment on attachment 8660852 [details] [diff] [review]
Add rootActor.listWorkers() to list all registered workers.

As requested, I'm flagging you for the rootActor/listWorkers plumbing. Please let me know what you think.

I considered adding a "workerList" to the RootActor constructor parameters (like "tabList" and "addonList") instead of creating and using a "workerActorList" directly, but didn't see much value to doing that.

Comment 33

[Jan Keromnes [:janx]]

Comment on attachment 8660852 [details] [diff] [review]
Add rootActor.listWorkers() to list all registered workers.

Ping timeout. Panos, could you have a look please? I also wrote a small note in comment 32.

Comment 34

[Jan Keromnes [:janx]]

Comment on attachment 8660851 [details] [diff] [review]
Add worker type to WorkerActor form.

Just adding the worker type to the WorkerActor form.

Comment 35

[Jan Keromnes [:janx]]

Attachment: Add worker type to WorkerActor form.

Thank you Panos! Edited the "r=" flag in the description, keeping r+.

Comment 36

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8660852 [details] [diff] [review]
Add rootActor.listWorkers() to list all registered workers.

Review of attachment 8660852 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not giving this an r+ yet due to the question about embeddings below. If you can test all of them and become convinced that this approach is fine, flag me again and I'll r+ it.

::: toolkit/components/telemetry/Histograms.json
@@ +5940,5 @@
> +    "kind": "exponential",
> +    "high": "10000",
> +    "n_buckets": "1000",
> +    "description": "The time (in milliseconds) that it took a 'listWorkers' request to go round trip."
> +  },

This needs an f? from ally, vladan or bsmedberg.

::: toolkit/devtools/server/actors/root.js
@@ +9,5 @@
>  const { Cc, Ci, Cu } = require("chrome");
>  const Services = require("Services");
>  const { ActorPool, appendExtraActors, createExtraActors } = require("devtools/server/actors/common");
>  const { DebuggerServer } = require("devtools/server/main");
> +const { WorkerActorList } = require("devtools/server/actors/worker");

This should be loaded lazily, the regular toolbox will not often send a listWorkers request.

@@ +94,5 @@
>    this.conn = aConnection;
>    this._parameters = aParameters;
>    this._onTabListChanged = this.onTabListChanged.bind(this);
>    this._onAddonListChanged = this.onAddonListChanged.bind(this);
> +  this._onWorkerActorListChanged = this.onWorkerActorListChanged.bind(this);

"Actor" is implied here, how about onWorkerListChanged for consistency with onTabListChanged and onAddonListChanged?

@@ +351,5 @@
>    },
>  
> +  onListWorkers: function () {
> +    if (this._workerActorList == null) {
> +      this._workerActorList = new WorkerActorList({});

The reason tabList and addonList are provided as root actor parameters is that they may not be present in some embeddings or need to work differently. I am not entirely sure about workers, but I think they are present (and the same) in desktop, Fennec, b2g and WebappRT, but maybe not on xpcshell?

You have to verify this (just grep the tree for "new RootActor" and read the code), but we should use the common paradigm if one of the embeddings is special. Otherwise calling listWorkers() in that scenario will lead to confusing errors, compared to noTabs or noAddons.

Another thought that comes to mind is, what if at some point workers are implemented differently in, say Android or iOS? Different in the sense of having to use a different API (than desktop) to e.g. enumerate them. In that case we would have to override the generic method and the tabList/addonList approach provides this already (cf. MobileTabList in dbg-browser-actors.js).

@@ +375,5 @@
> +  },
> +
> +  onWorkerActorListChanged: function () {
> +    this._workerActorList.onListChanged = null;
> +    this.conn.send({ from: this.actorID, type: "workerListChanged" });

I think it would be best to follow the convention in the other onFooChanged methods and only nullify it after the event is sent. At least failing delivery wouldn't result in loss of information about the occurrence of the event.

Comment 37

[Panos Astithas (he/him) [:past] (please ni?)]

> At least failing delivery wouldn't result in loss of information about the occurrence of the
> event.

What I meant to say is "failing delivery wouldn't *permanently* result in loss of information about the occurrence of the event", as the next such event would trigger another delivery attempt.

Comment 38

[Jan Keromnes [:janx]]

Comment on attachment 8660852 [details] [diff] [review]
Add rootActor.listWorkers() to list all registered workers.

Thanks Panos!

Ally, I'd like to add telemetry to track the time (in ms) it took to complete a "listWorkers" request, which is called when one of our developer tools wants to know about the {service,shared,dedicated}workers living in a target (the local browser, an attached device, a simulator, etc). Is that OK?

Comment 39

[Allison Naaktgeboren :ally]

Comment on attachment 8660852 [details] [diff] [review]
Add rootActor.listWorkers() to list all registered workers.

Review of attachment 8660852 [details] [diff] [review]:
-----------------------------------------------------------------

There are some problems with your probes and nothing in this patch pushes data to any of the probes defined here. The former is easy enough to change, the latter is worrisome.

Theres an additional r? field on the bottom of the bugzilla page for patches. Please use that to let me/us know that what you need is a review to ensure that it gets prioritized in my queue properly. :)

::: toolkit/components/telemetry/Histograms.json
@@ +5928,5 @@
>      "n_buckets": "1000",
>      "description": "The time (in milliseconds) that it took a 'listAddons' request to go round trip."
>    },
> +  "DEVTOOLS_DEBUGGER_RDP_LOCAL_LISTWORKERS_MS": {
> +    "expires_in_version": "never",

We no longer allow expiration: never. How about version 55?

@@ +5935,5 @@
> +    "n_buckets": "1000",
> +    "description": "The time (in milliseconds) that it took a 'listWorkers' request to go round trip."
> +  },
> +  "DEVTOOLS_DEBUGGER_RDP_REMOTE_LISTWORKERS_MS": {
> +    "expires_in_version": "never",

same

@@ +5938,5 @@
> +  "DEVTOOLS_DEBUGGER_RDP_REMOTE_LISTWORKERS_MS": {
> +    "expires_in_version": "never",
> +    "kind": "exponential",
> +    "high": "10000",
> +    "n_buckets": "1000",

1000 buckets is too large of a number. We usually top off at a 100. Why do you need some many buckets if the high is only 10,000? I imagine you'd be fine with less than 50 buckets.

@@ +5945,5 @@
>    "DEVTOOLS_DEBUGGER_RDP_LOCAL_LISTPROCESSES_MS": {
>      "expires_in_version": "never",
>      "kind": "exponential",
>      "high": "10000",
>      "n_buckets": "1000",

all the above applies to this probe too.

Comment 40

[Jan Keromnes [:janx]]

Attachment: Add worker type to WorkerActor form.

Rebased on top of devtools earthquake, keeping Panos' r+.

Comment 41

[Jan Keromnes [:janx]]

Attachment: Add rootActor.listWorkers() to list all registered workers. r=past

Rebased, addressed nits.

Comment 42

[Jan Keromnes [:janx]]

Attachment: Create an about:debugging page to list debuggable devtools targets.

Rebased, addressed nits.

Comment 43

[Jan Keromnes [:janx]]

Comment on attachment 8664822 [details] [diff] [review]
Add rootActor.listWorkers() to list all registered workers. r=past

Panos, thanks again for your previous review. I believe I addressed all of your comments.

(In reply to Panos Astithas [:past] from comment #36)
> ::: toolkit/devtools/server/actors/root.js
> @@ +351,5 @@
> >    },
> >  
> > +  onListWorkers: function () {
> > +    if (this._workerActorList == null) {
> > +      this._workerActorList = new WorkerActorList({});
> 
> The reason tabList and addonList are provided as root actor parameters is
> that they may not be present in some embeddings or need to work differently.
> I am not entirely sure about workers, but I think they are present (and the
> same) in desktop, Fennec, b2g and WebappRT, but maybe not on xpcshell?
> 
> You have to verify this (just grep the tree for "new RootActor" and read the
> code), but we should use the common paradigm if one of the embeddings is
> special. Otherwise calling listWorkers() in that scenario will lead to
> confusing errors, compared to noTabs or noAddons.
> 
> Another thought that comes to mind is, what if at some point workers are
> implemented differently in, say Android or iOS? Different in the sense of
> having to use a different API (than desktop) to e.g. enumerate them. In that
> case we would have to override the generic method and the tabList/addonList
> approach provides this already (cf. MobileTabList in dbg-browser-actors.js).

I had a chat with Eddy about this, and he seems to think that it's unlikely we'll have different worker implementations across embeddings (even though I'm also unsure about xpcshell).

However, to err on the side of caution, I changed the WorkerActorList into a RootActor parameter anyway. Furthermore, I only set this "workerList" parameter for the RootActor created in webbrowser.js, so that we can gradually add more embeddings later on, while verifying that they all work.

Comment 44

[Jan Keromnes [:janx]]

Comment on attachment 8664822 [details] [diff] [review]
Add rootActor.listWorkers() to list all registered workers. r=past

Ally, thanks for your telemetry review earlier! I believe that I addressed all of your comments, and I filed bug 1207557 so that we fix all the other Remote Debugging Protocol probes (I based mine on these without really knowing what I was doing).

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #39)
> There are some problems with your probes and nothing in this patch pushes
> data to any of the probes defined here. The former is easy enough to change,
> the latter is worrisome.

The pushing of data to our "DEVTOOLS_DEBUGGER_RDP_.*" probes happens in the templated functions we create with DebuggerClient.requester[1]. My new listWorkers() function is created that way, and the `telemetry: "LISTWORKERS"` parameter will make it push data to the new probes, according to the current transport type (i.e. "LOCAL" or "REMOTE").

[1] https://dxr.mozilla.org/mozilla-central/source/devtools/shared/client/main.js#249

> ::: toolkit/components/telemetry/Histograms.json
> @@ +5928,5 @@
> >      "n_buckets": "1000",
> >      "description": "The time (in milliseconds) that it took a 'listAddons' request to go round trip."
> >    },
> > +  "DEVTOOLS_DEBUGGER_RDP_LOCAL_LISTWORKERS_MS": {
> > +    "expires_in_version": "never",
> 
> We no longer allow expiration: never. How about version 55?

Both versions are now set to 55.

> @@ +5938,5 @@
> > +  "DEVTOOLS_DEBUGGER_RDP_REMOTE_LISTWORKERS_MS": {
> > +    "expires_in_version": "never",
> > +    "kind": "exponential",
> > +    "high": "10000",
> > +    "n_buckets": "1000",
> 
> 1000 buckets is too large of a number. We usually top off at a 100. Why do
> you need some many buckets if the high is only 10,000? I imagine you'd be
> fine with less than 50 buckets.

Both buckets are now sized at 50.

Comment 45

[Jan Keromnes [:janx]]

Attachment: Create an about:debugging page to list debuggable devtools targets.

Fix a split-patch issue.

Comment 46

[Jan Keromnes [:janx]]

Comment on attachment 8664840 [details] [diff] [review]
Create an about:debugging page to list debuggable devtools targets.

Ryan, this patch adds a hidden "about:debugging" page, which only offers Add-on and Worker debugging for now.

I've split my runtime connection & debugging work into a follow-up patch that's not finished yet, but I'm indeed hoping to come up with a modular, UI-independent solution that will allow future consumers to reuse it.

Things have gotten somewhat more modular since your last review, and almost everything is now a React component, because it made sense. I haven't separated these into different files, because I don't think it's useful for such simple UI components, but feel free to insist if you really think they should be separated.

I have some ideas on how to separate out the functional parts (e.g. creating a Target and launching a special Toolbox for something) from the UI components, but I'll probably try these out in follow-up patches, once a basic page is in place. In any case, if you have thoughts on the matter, please share.

Known issues:
- It's not possible to open two Add-on debugging toolboxes at the same time, because all BrowserToolboxProcesses use the same debuggingProfileDir, "ProfLD". Maybe we could make them create dedicated, temporary profile dirs instead?
- In Worker toolboxes, Console and Scratchpad don't work.
- Service Workers sometimes die (by design), so any associated toolbox then points to nothing (non-trivial problem which Eddy is trying to fix).
- Opening a toolbox on an "Other Worker" (dedicated chrome worker) only works the first time. Re-opening a toolbox breaks.
- "TypeError: Services.io is undefined /devtools/shared/path.js:13" in the logs (bug 1207506).

Comment 47

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8664840 [details] [diff] [review]
Create an about:debugging page to list debuggable devtools targets.

Review of attachment 8664840 [details] [diff] [review]:
-----------------------------------------------------------------

What do you think of :jlongster's feedback in comment 25?

I believe I correctly applied the 3 patches that haven't landed yet, but I didn't get to try anything really, because all the lists were empty.

Loading the page gives me this stack:

Handler function DebuggerClient.requester request callback threw an exception: SyntaxError: An invalid or illegal string was specified
Stack: createNodesFromMarkup@resource:///modules/devtools/client/shared/vendor/react.js:19022:5
[13]</Danger.dangerouslyRenderMarkup@resource:///modules/devtools/client/shared/vendor/react.js:2225:25
[10]</DOMChildrenOperations.processUpdates@resource:///modules/devtools/client/shared/vendor/react.js:1593:26
[50]</ReactDOMIDOperations.dangerouslyProcessChildrenUpdates@resource:///modules/devtools/client/shared/vendor/react.js:8625:5
processQueue@resource:///modules/devtools/client/shared/vendor/react.js:13359:1
[78]</ReactMultiChild.Mixin.updateChildren@resource:///modules/devtools/client/shared/vendor/react.js:13481:13
[48]</ReactDOMComponent.Mixin._updateDOMChildren@resource:///modules/devtools/client/shared/vendor/react.js:8388:7
[48]</ReactDOMComponent.Mixin.updateComponent@resource:///modules/devtools/client/shared/vendor/react.js:8237:5
[48]</ReactDOMComponent.Mixin.receiveComponent@resource:///modules/devtools/client/shared/vendor/react.js:8221:5
[89]</ReactReconciler.receiveComponent@resource:///modules/devtools/client/shared/vendor/react.js:14840:5
[43]</ReactCompositeComponentMixin._updateRenderedComponent@resource:///modules/devtools/client/shared/vendor/react.js:7404:7
[43]</ReactCompositeComponentMixin._performComponentUpdate@resource:///modules/devtools/client/shared/vendor/react.js:7382:5
[43]</ReactCompositeComponentMixin.updateComponent@resource:///modules/devtools/client/shared/vendor/react.js:7298:7
[43]</ReactCompositeComponentMixin.receiveComponent@resource:///modules/devtools/client/shared/vendor/react.js:7162:1
[89]</ReactReconciler.receiveComponent@resource:///modules/devtools/client/shared/vendor/react.js:14840:5
[43]</ReactCompositeComponentMixin._updateRenderedComponent@resource:///modules/devtools/client/shared/vendor/react.js:7404:7
[43]</ReactCompositeComponentMixin._performComponentUpdate@resource:///modules/devtools/client/shared/vendor/react.js:7382:5
[43]</ReactCompositeComponentMixin.updateComponent@resource:///modules/devtools/client/shared/vendor/react.js:7298:7
[43]</ReactCompositeComponentMixin.performUpdateIfNecessary@resource:///modules/devtools/client/shared/vendor/react.js:7195:1
[89]</ReactReconciler.performUpdateIfNecessary@resource:///modules/devtools/client/shared/vendor/react.js:14858:5
runBatchedUpdates@resource:///modules/devtools/client/shared/vendor/react.js:16674:1
[116]</Mixin.perform@resource:///modules/devtools/client/shared/vendor/react.js:18402:13
[116]</Mixin.perform@resource:///modules/devtools/client/shared/vendor/react.js:18402:13
[100]</<.perform@resource:///modules/devtools/client/shared/vendor/react.js:16618:1
[100]</flushBatchedUpdates@resource:///modules/devtools/client/shared/vendor/react.js:16698:7
[116]</Mixin.closeAll@resource:///modules/devtools/client/shared/vendor/react.js:18475:11
[116]</Mixin.perform@resource:///modules/devtools/client/shared/vendor/react.js:18416:11
[59]</ReactDefaultBatchingStrategy.batchedUpdates@resource:///modules/devtools/client/shared/vendor/react.js:9669:7
enqueueUpdate@resource:///modules/devtools/client/shared/vendor/react.js:16738:5
enqueueUpdate@resource:///modules/devtools/client/shared/vendor/react.js:16256:5
[99]</ReactUpdateQueue.enqueueSetState@resource:///modules/devtools/client/shared/vendor/react.js:16434:5
[39]</ReactComponent.prototype.setState@resource:///modules/devtools/client/shared/vendor/react.js:6418:3
update/<@chrome://devtools/content/aboutdebugging/aboutdebugging.js:179:7
DebuggerClient.requester/</<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/shared/client/main.js:284:9
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/shared/DevToolsUtils.js:86:14
emitOnObject@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/event/core.js:112:9
emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/event/core.js:89:38
Request.prototype.emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/shared/client/main.js:1158:29
DebuggerClient.prototype.onPacket/emitReply@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/shared/client/main.js:969:29
DevTools RDP*DebuggerClient.prototype.request@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/shared/client/main.js:675:5
DebuggerClient.requester/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/shared/client/main.js:272:1
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/shared/DevToolsUtils.js:86:14
update@chrome://devtools/content/aboutdebugging/aboutdebugging.js:158:5
eventSource/aProto.emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/shared/client/main.js:125:9
DebuggerClient.prototype.onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/shared/client/main.js:965:7
LocalDebuggerTransport.prototype.send/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/shared/transport/transport.js:569:11
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/shared/DevToolsUtils.js:86:14
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/shared/DevToolsUtils.js:86:14
Line: 19022, column: 0

(In reply to Jan Keromnes [:janx] from comment #46)
> Comment on attachment 8664840 [details] [diff] [review]
> Create an about:debugging page to list debuggable devtools targets.
> 
> Ryan, this patch adds a hidden "about:debugging" page, which only offers
> Add-on and Worker debugging for now.

Overall, it looks good so far.

> I've split my runtime connection & debugging work into a follow-up patch
> that's not finished yet, but I'm indeed hoping to come up with a modular,
> UI-independent solution that will allow future consumers to reuse it.

Awesome, I am excited to see it!

> Things have gotten somewhat more modular since your last review, and almost
> everything is now a React component, because it made sense. I haven't
> separated these into different files, because I don't think it's useful for
> such simple UI components, but feel free to insist if you really think they
> should be separated.

At the very least, let's separate the components from the main `AboutDebugging` object.  But really, why not separate files for everything?  Small files are great!

> I have some ideas on how to separate out the functional parts (e.g. creating
> a Target and launching a special Toolbox for something) from the UI
> components, but I'll probably try these out in follow-up patches, once a
> basic page is in place. In any case, if you have thoughts on the matter,
> please share.

Okay, no immediate thoughts yet, but will think about it.

> Known issues:
> - It's not possible to open two Add-on debugging toolboxes at the same time,
> because all BrowserToolboxProcesses use the same debuggingProfileDir,
> "ProfLD". Maybe we could make them create dedicated, temporary profile dirs
> instead?

Hmm!  Well, I don't think we really need a process per add-on.  So, may we should construct a way to single to the existing process to open another add-on toolbox?

Anyway, something for a follow up bug to think about.

> - In Worker toolboxes, Console and Scratchpad don't work.

Okay, but I guess that's not specific to your work, right?  Is there are bug filed to make it work?

We should at least only show tools that do actually work.

> - Service Workers sometimes die (by design), so any associated toolbox then
> points to nothing (non-trivial problem which Eddy is trying to fix).

What happens to the toolbox then?  Do they spontaneously disappear, or stay open but disconnected?  Something else, perhaps...?

> - Opening a toolbox on an "Other Worker" (dedicated chrome worker) only
> works the first time. Re-opening a toolbox breaks.

Any idea why?

::: browser/locales/en-US/chrome/browser/devtools/aboutdebugging.dtd
@@ +1,5 @@
> +<!-- 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/. -->
> +
> +<!ENTITY aboutDebugging.title                      "about:debugging">

Now we get to bikeshed the user-visible name for the page!

Should it be different from the URL?

::: devtools/client/aboutdebugging/aboutdebugging.js
@@ +1,5 @@
> +/* 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/. */
> +
> +'use strict';

DevTools style is "" only.

See also: Use ESLint, it's your pote (I learned this word from a French Coke can...).[1]

[1]: https://wiki.mozilla.org/DevTools/CodingStandards

@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +'use strict';
> +
> +const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;

Nit: { c

@@ +5,5 @@
> +'use strict';
> +
> +const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
> +
> +Cu.import("resource://gre/modules/AddonManager.jsm");

We should generally prefer lazy requires and imports in most cases unless you know for sure something is used immediately in the global scope.

The `loader.lazy<X>` methods make this pretty easy to do.

@@ +10,5 @@
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource:///modules/devtools/client/framework/ToolboxProcess.jsm");
> +
> +const { gDevTools } = Cu.import('resource:///modules/devtools/client/framework/gDevTools.jsm', {});
> +const { loader, require } = Cu.import("resource://gre/modules/devtools/shared/Loader.jsm", {});

Nit: `loader` seems unused for now?

@@ +16,5 @@
> +const { DebuggerClient } = require("devtools/shared/client/main");
> +const { DebuggerServer } = require("devtools/server/main");
> +const { TargetFactory } = require("devtools/client/framework/target");
> +const { Toolbox } = require("devtools/client/framework/toolbox");
> +const TabStore = require("devtools/client/webide/modules/tab-store");

Consider carefully if you want this module... it's quite of awkward, but fine to use if think it works for your case.

@@ +23,5 @@
> +const LocaleCompare = (a, b) => {
> +  return a.name.toLowerCase().localeCompare(b.name.toLowerCase());
> +};
> +
> +

Nit: Don't need the double blank line

@@ +77,5 @@
> +    );
> +  }
> +});
> +
> +let AddonsComponent = React.createClass({ displayName: "AddonsComponent",

Neat!  This component seems to link nicely this with the add-on manager.

@@ +179,5 @@
> +      this.setState({ workers });
> +    });
> +  }
> +});
> +

Nit: No double line

@@ +185,5 @@
> +let AboutDebugging = {
> +  _categories: [],
> +  showTab(category) {
> +    let categories = this._categories;
> +    if (categories.length < 1) {

Seems like this could be moved to a getter that populates on first get, and you could leave `_categories` null instead of [] in the initial case.

@@ +206,5 @@
> +    document.querySelector(".tab.active").classList.remove("active");
> +    document.querySelector("#tab-" + category).classList.add("active");
> +    document.querySelector(".category[selected]").removeAttribute("selected");
> +    document.querySelector(".category[value=" + category + "]").setAttribute("selected", "true");
> +    location.hash = "#" + category;

We should make this page detect hash changes too (perhaps via the `hashchange` event).  For example, if I:

1. Click on add-ons (URL is about:debugging#addons)
2. Click on workers (URL is about:debugging#workers)
3. Go back

The URL correctly goes back to about:debugging#addons, but the selected tab is not updated.

@@ +214,5 @@
> +    // Show the first available tab.
> +    this.showTab();
> +
> +    // Link checkboxes to prefs.
> +    let elements = document.querySelectorAll("input[type=checkbox][data-pref]");

Do the checkboxes make sense as React components as well?

::: devtools/client/aboutdebugging/aboutdebugging.xhtml
@@ +1,5 @@
> +<?xml version="1.0" encoding="UTF-8"?>
> +<!-- 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/. -->
> +

Nit: One blank line seems like enough

@@ +15,5 @@
> +    <title>&aboutDebugging.title;</title>
> +    <link rel="stylesheet" href="chrome://global/skin/global.css" type="text/css"/>
> +    <link rel="stylesheet" href="chrome://global/skin/in-content/common.css" type="text/css"/>
> +    <link rel="stylesheet" href="chrome://devtools/content/aboutdebugging/aboutdebugging.css"  type="text/css"/>
> +    <script type="application/javascript" src="resource:///modules/devtools/client/shared/vendor/react.js"></script>

Are you interested in using the :jlongster's new `BrowserLoader`[1] (require with window support) instead?

I guess you'd use it by making a small "main" script file in a single script tag and then require other things from there.  I'm sure :jlongster has more advice.

Another advantage besides the require() API is you can skip `jar.mn` (and use `moz.build`) for your page scripts, because they are loaded just like other JS modules (no chrome:// URLs).

[1]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/debugger/debugger-controller.js#106

@@ +25,5 @@
> +        <img class="category-icon" src="chrome://mozapps/skin/extensions/category-extensions.png"/>
> +        <div class="category-name">&aboutDebugging.addons;</div>
> +      </div>
> +      <div class="category" value="workers">
> +        <img class="category-icon" src="chrome://browser/skin/preferences/in-content/icons.svg#applications"/>

Let's file a follow up to find the right icon for workers.

@@ +34,5 @@
> +      <div id="tab-addons" class="tab active">
> +        <div class="header">
> +          <h1 class="header-name">&aboutDebugging.addons;</h1>
> +        </div>
> +        <input id="enable-addon-debugging" type="checkbox" data-pref="devtools.chrome.enabled"/>

I believe we talked previously about whether to require this pref at all...  what did we decide?  I forgot. :)

::: docshell/base/nsAboutRedirector.cpp
@@ +51,5 @@
>      "credits", "http://www.mozilla.org/credits/",
>      nsIAboutModule::URI_SAFE_FOR_UNTRUSTED_CONTENT
>    },
>    {
> +    "debugging", "chrome://devtools/content/aboutdebugging/aboutdebugging.xhtml",

This should go in the browser-specific about module[1] instead.

Also, I don't know that I can review that file, so perhaps ask a browser peer?

[1]: https://dxr.mozilla.org/mozilla-central/source/browser/components/about/AboutRedirector.cpp#102

::: docshell/build/nsDocShellModule.cpp
@@ +160,5 @@
>  #ifdef MOZ_CRASHREPORTER
>    { NS_ABOUT_MODULE_CONTRACTID_PREFIX "crashes", &kNS_ABOUT_REDIRECTOR_MODULE_CID },
>  #endif
>    { NS_ABOUT_MODULE_CONTRACTID_PREFIX "credits", &kNS_ABOUT_REDIRECTOR_MODULE_CID },
> +  { NS_ABOUT_MODULE_CONTRACTID_PREFIX "debugging", &kNS_ABOUT_REDIRECTOR_MODULE_CID },

This shouldn't be needed once you move to the browser version.

Comment 48

[Jan Keromnes [:janx]]

Thanks Ryan!

I'll work through your comments shortly, but first:

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #47)
> What do you think of :jlongster's feedback in comment 25?

That comment had two parts:
- Regarding the `createFactory` style, James suggested considering it in a follow-up bug. I just filed bug 1207997.
- Regarding using `flux`, we might consider that later as the project grows, but for now passing state down and events up isn't messy.

> I believe I correctly applied the 3 patches that haven't landed yet, but I
> didn't get to try anything really, because all the lists were empty.

You also need to update React to 0.14 by applying the patch in bug 1200534.

What happens is that before that version, React tries to append void elements like "<img>" without their optional closing tag as XHTML, which obviously fails because it's not valid XHTML.

Comment 49

[Jan Keromnes [:janx]]

Ryan, I'll forward some of your worker-related questions to Eddy.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #47)
> > - In Worker toolboxes, Console and Scratchpad don't work.
> 
> Okay, but I guess that's not specific to your work, right?  Is there are bug
> filed to make it work?

I think this is related to Eddy's work. Eddy, is there a bug for making Console and Scratchpad work with WorkerTargets?

> > - Service Workers sometimes die (by design), so any associated toolbox then
> > points to nothing (non-trivial problem which Eddy is trying to fix).
> 
> What happens to the toolbox then?  Do they spontaneously disappear, or stay
> open but disconnected?  Something else, perhaps...?

The toolbox stays open but disconnected. To address this problem, Eddy is considering saving the Debugger state somewhere so that when a Service Worker dies, but one of our tools needs to do something with it, it can be fired up and restored (from the DevTools' standpoint, it would look as if the Service Worker never dies). However, this is somewhat messy, and I think Eddy would prefer it if there was a cleaner solution.

> > - Opening a toolbox on an "Other Worker" (dedicated chrome worker) only
> > works the first time. Re-opening a toolbox breaks.
> 
> Any idea why?

I don't know why. I suspect that calling `DebuggerClient.attachWorker()`[1] twice creates problems, even though `attachWorker` is supposed to know about existing `workerClients` and re-return them if they already exist.

I can investigate this more, and file a bug if I manage to track down the problem. Eddy, any ideas what the problem might be with opening toolboxes more than once on chrome-privileged workers?

[1] https://dxr.mozilla.org/mozilla-central/source/devtools/shared/client/main.js#439

Comment 50

[Eddy Bruel [:ejpbruel]]

(In reply to Jan Keromnes [:janx] from comment #49)
> Ryan, I'll forward some of your worker-related questions to Eddy.
> 
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #47)
> > > - In Worker toolboxes, Console and Scratchpad don't work.
> > 
> > Okay, but I guess that's not specific to your work, right?  Is there are bug
> > filed to make it work?
> 
> I think this is related to Eddy's work. Eddy, is there a bug for making
> Console and Scratchpad work with WorkerTargets?

Not at the moment. I've just begun reading through the console code to understand what needs to happen. I still have to open a bug.

> 
> > > - Service Workers sometimes die (by design), so any associated toolbox then
> > > points to nothing (non-trivial problem which Eddy is trying to fix).
> > 
> > What happens to the toolbox then?  Do they spontaneously disappear, or stay
> > open but disconnected?  Something else, perhaps...?
> 
> The toolbox stays open but disconnected. To address this problem, Eddy is
> considering saving the Debugger state somewhere so that when a Service
> Worker dies, but one of our tools needs to do something with it, it can be
> fired up and restored (from the DevTools' standpoint, it would look as if
> the Service Worker never dies). However, this is somewhat messy, and I think
> Eddy would prefer it if there was a cleaner solution.

We've decided that we're going to keep service workers alive if a debugger is attached to them. The alternative of saving the debugger state is just too much effort to be worth it (even if we saved the debugger state somewhere, if a user was debugging that service worker when it went away, the debugger server would suddenly become unresponsive. Not what you want).

> 
> > > - Opening a toolbox on an "Other Worker" (dedicated chrome worker) only
> > > works the first time. Re-opening a toolbox breaks.
> > 
> > Any idea why?
> 
> I don't know why. I suspect that calling `DebuggerClient.attachWorker()`[1]
> twice creates problems, even though `attachWorker` is supposed to know about
> existing `workerClients` and re-return them if they already exist.
> 
> I can investigate this more, and file a bug if I manage to track down the
> problem. Eddy, any ideas what the problem might be with opening toolboxes
> more than once on chrome-privileged workers?
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/devtools/shared/client/main.
> js#439

AFAIK, there is no fundamental difference between attaching to a chrome privileged worker and a content privileged worker. I should be able to tell you more if I had a stack trace or some other error output.

Comment 51

[Jan Keromnes [:janx]]

Thanks for your input Eddy.

(In reply to Eddy Bruel [:ejpbruel] from comment #50)
> AFAIK, there is no fundamental difference between attaching to a chrome
> privileged worker and a content privileged worker. I should be able to tell
> you more if I had a stack trace or some other error output.

The only thing I see showing up in the console is bug 1207506, not sure if related. Also, the Toolbox title always ends with "(null)", which is worrisome.

Comment 52

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Jan Keromnes [:janx] from comment #51)
> Also, the Toolbox title always ends with "(null)", which is worrisome.

Browser Content Toolbox has this too.  It's trying to show a URL, but the target does not have one.  Should just be a cosmetic thing (nice to fix if we can, but not terrible either).

Comment 53

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ed4e41f08f0a
https://hg.mozilla.org/mozilla-central/rev/3639f5216ac0

Comment 54

[Allison Naaktgeboren :ally]

Comment on attachment 8664822 [details] [diff] [review]
Add rootActor.listWorkers() to list all registered workers. r=past

Review of attachment 8664822 [details] [diff] [review]:
-----------------------------------------------------------------

p=ally for telemetry review

Comment 55

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8664822 [details] [diff] [review]
Add rootActor.listWorkers() to list all registered workers. r=past

Review of attachment 8664822 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/Histograms.json
@@ +5988,5 @@
>      "n_buckets": "1000",
>      "description": "The time (in milliseconds) that it took a 'listAddons' request to go round trip."
>    },
> +  "DEVTOOLS_DEBUGGER_RDP_LOCAL_LISTWORKERS_MS": {
> +    "expires_in_version": "55",

You may want set some value of `alert_emails`, so we are told about the expiration, see example[1].

[1]: https://dxr.mozilla.org/mozilla-central/rev/5abe3c4deab94270440422c850bbeaf512b1f38d/toolkit/components/telemetry/Histograms.json#7431

Comment 56

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8664822 [details] [diff] [review]
Add rootActor.listWorkers() to list all registered workers. r=past

Review of attachment 8664822 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!

Comment 57

[Jan Keromnes [:janx]]

Attachment: Add worker type to WorkerActor form.

Rebased.

Comment 58

[Jan Keromnes [:janx]]

Attachment: Add rootActor.listWorkers() to list all registered workers.

Rebased and added "alert_emails" field as suggested in comment 55.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a7993a6170c

Comment 59

[Jan Keromnes [:janx]]

Attachment: Drive-by fix DevTools ESLint config.

Sorted globals list, added "React", fixed bad reference to "/devtools/client/.eslintrc.xpcshell".

Patrick, please have a look.

Comment 60

[Patrick Brosset <:pbro>]

Comment on attachment 8666656 [details] [diff] [review]
Drive-by fix DevTools ESLint config.

Review of attachment 8666656 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with my comment addressed.

::: devtools/.eslintrc
@@ +14,5 @@
>      "EventEmitter": true,
>      "exports": true,
>      "loader": true,
>      "module": true,
> +    "React": true,

I'm not sure we should add this global here. 2 reasons:
- as of now, no other files in devtools need React,
- bug 1203520 is removing a bunch of globals from .eslintrc

Since we're only just starting sprinkling some react components here and there, I would suggest adding 
/* globals React */
only to those files that need it instead of globally.

Comment 61

[Jan Keromnes [:janx]]

Attachment: Drive-by fix DevTools ESLint config.

Thanks Patrick! I removed the "React" global from the list.

Comment 62

[Jan Keromnes [:janx]]

More complete try push for all three patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=98cf8406c237

Comment 63

[Jan Keromnes [:janx]]

Please check in my three latest patches:

1. Add worker type to WorkerActor form.
2. Add rootActor.listWorkers() to list all registered workers.
3. Drive-by fix DevTools ESLint config.

Try push looks green enough.

Comment 64

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Jan Keromnes [:janx] from comment #62)
> More complete try push for all three patches:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=98cf8406c237

For try pushes it would be great to include OS X as well. Once you have something I can use to 'kick the tires' with, needinfo me with another try push

Comment 65

[Jan Keromnes [:janx]]

(Details about comment 64 for the Sheriff: Jeff is asking for an OS X try push including the "about:debugging" patch, so he can try it out. I'll do that after the three r+'d patches land.)

Comment 66

[Jan Keromnes [:janx]]

Attachment: Create an about:debugging page to list debuggable devtools targets.

Ryan, thanks again for your review earlier. It took me a few days, but I believe I've addressed all your nits.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #47)
> At the very least, let's separate the components from the main
> `AboutDebugging` object.  But really, why not separate files for everything?
> Small files are great!

As per our discussion on IRC, I separated all React components into individual files inside a "components/" folder. Hopefully the entailed modularity will be worth the extra overhead.

> > - Opening a toolbox on an "Other Worker" (dedicated chrome worker) only
> > works the first time. Re-opening a toolbox breaks.
> 
> Any idea why?

I filed follow-up bug 1209559 to investigate this more.

> ::: browser/locales/en-US/chrome/browser/devtools/aboutdebugging.dtd
> @@ +1,5 @@
> > +<!-- 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/. -->
> > +
> > +<!ENTITY aboutDebugging.title                      "about:debugging">
> 
> Now we get to bikeshed the user-visible name for the page!
> 
> Should it be different from the URL?

Yay, bikeshedding!

The title formats used by the 34 existing "about:topic" pages are:
- "Description of Topic" (10 pages)
- "Topic" (6 pages)
- "about:topic" (6 pages)
- "About Topic" (5 pages)
- Unrelated Title (4 pages, including "about:robots" with "Gort! Klaatu barada nikto!")
- "Topic Manager" (3 pages)

I really like the "about:topic" and "About Topic" formats, because they help you remember the URL for quick access.

I guess you'd prefer something more like "Description of Topic"?

> See also: Use ESLint, it's your pote (I learned this word from a French Coke
> can...).[1]
> 
> [1]: https://wiki.mozilla.org/DevTools/CodingStandards

Haha, that's a cool and spot-on use of colloquial French vocabulary! :D Made me laugh.

Anyway, I nogicated with ESLint and I believe we came to an agreement of terms (in which I fixed almost all the issues).

> We should generally prefer lazy requires and imports in most cases unless
> you know for sure something is used immediately in the global scope.
> 
> The `loader.lazy<X>` methods make this pretty easy to do.

I started out with lazifying only the not-immediately-used imports, but then I had a look at the performance actor's code and decided it was better to lazify all the things.

> @@ +16,5 @@
> > +const { DebuggerClient } = require("devtools/shared/client/main");
> > +const { DebuggerServer } = require("devtools/server/main");
> > +const { TargetFactory } = require("devtools/client/framework/target");
> > +const { Toolbox } = require("devtools/client/framework/toolbox");
> > +const TabStore = require("devtools/client/webide/modules/tab-store");
> 
> Consider carefully if you want this module... it's quite of awkward, but
> fine to use if think it works for your case.

Agreed, that module is somewhat akward. I used it mainly because it did what I needed: List all tabs from a given connection, sometimes using known workarounds or different code paths according to runtime version.

I could also refactor or reimplement something similar to make it cleaner (e.g. without the single "selectedTab" I don't need). That's something I'll consider for my upcoming re-work of runtime connections.

> We should make this page detect hash changes too (perhaps via the
> `hashchange` event).  For example, if I:
> 
> 1. Click on add-ons (URL is about:debugging#addons)
> 2. Click on workers (URL is about:debugging#workers)
> 3. Go back
> 
> The URL correctly goes back to about:debugging#addons, but the selected tab
> is not updated.

Nice catch! Fixed.

> @@ +214,5 @@
> > +    // Show the first available tab.
> > +    this.showTab();
> > +
> > +    // Link checkboxes to prefs.
> > +    let elements = document.querySelectorAll("input[type=checkbox][data-pref]");
> 
> Do the checkboxes make sense as React components as well?

I started writing a pref-checkbox React component just to see what it would look like, but I stopped when it grew over 50 lines of code.

I believe the current lightweight (9 LOC) plumbing code is fine for now, but I'll continue improving on what I wrote, hoping to end up with a component that works with different pref types (not just bool).

> @@ +15,5 @@
> > +    <title>&aboutDebugging.title;</title>
> > +    <link rel="stylesheet" href="chrome://global/skin/global.css" type="text/css"/>
> > +    <link rel="stylesheet" href="chrome://global/skin/in-content/common.css" type="text/css"/>
> > +    <link rel="stylesheet" href="chrome://devtools/content/aboutdebugging/aboutdebugging.css"  type="text/css"/>
> > +    <script type="application/javascript" src="resource:///modules/devtools/client/shared/vendor/react.js"></script>
> 
> Are you interested in using the :jlongster's new `BrowserLoader`[1] (require
> with window support) instead?
> 
> I guess you'd use it by making a small "main" script file in a single script
> tag and then require other things from there.  I'm sure :jlongster has more
> advice.
> 
> Another advantage besides the require() API is you can skip `jar.mn` (and
> use `moz.build`) for your page scripts, because they are loaded just like
> other JS modules (no chrome:// URLs).
> 
> [1]:
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/debugger/
> debugger-controller.js#106

While refactoring, I started using BrowserLoader, but then I decided not to use the cool new thing if it wasn't really useful. I don't seem to need it now.

> @@ +25,5 @@
> > +        <img class="category-icon" src="chrome://mozapps/skin/extensions/category-extensions.png"/>
> > +        <div class="category-name">&aboutDebugging.addons;</div>
> > +      </div>
> > +      <div class="category" value="workers">
> > +        <img class="category-icon" src="chrome://browser/skin/preferences/in-content/icons.svg#applications"/>
> 
> Let's file a follow up to find the right icon for workers.

Filed as bug 1209581.

> @@ +34,5 @@
> > +      <div id="tab-addons" class="tab active">
> > +        <div class="header">
> > +          <h1 class="header-name">&aboutDebugging.addons;</h1>
> > +        </div>
> > +        <input id="enable-addon-debugging" type="checkbox" data-pref="devtools.chrome.enabled"/>
> 
> I believe we talked previously about whether to require this pref at all... 
> what did we decide?  I forgot. :)

I believe that we do need that pref today, and I forgot that we even debated that. Are you saying that we should get rid of this pref?

Comment 67

[Jan Keromnes [:janx]]

Comment on attachment 8667346 [details] [diff] [review]
Create an about:debugging page to list debuggable devtools targets.

Gijs, I need a rubberstamp on my changes to AboutRedirector.cpp and nsModule.cpp in order to register the "about:debugging" page. Could you please have a look?

Comment 68

[:Gijs (he/him)]

Comment on attachment 8667346 [details] [diff] [review]
Create an about:debugging page to list debuggable devtools targets.

Review of attachment 8667346 [details] [diff] [review]:
-----------------------------------------------------------------

You will also need to change the stupid list of "chrome" pages in browser.js ( https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#6958 ).

With that, the browser bits look OK to me. I'll leave reviewing the rest to jryans.

Note that this page now won't be available on mobile or other things, while AIUI part of the point of moving the tools to /devtools/ was to enable sharing and such... are we OK with this page only being available on desktop?

Comment 69

[Jan Keromnes [:janx]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=28dca49812a5

Comment 70

[Jan Keromnes [:janx]]

Needinfo Jeff about the treeherder link with Mac OS builds.

Comment 71

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Jan Keromnes [:janx] from comment #70)
> Needinfo Jeff about the treeherder link with Mac OS builds.

Comment 72

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Jan Keromnes [:janx] from comment #69)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=28dca49812a5

It looks great so far. I logged bug 1209746 ( feel free to dupe if you already have this logged ) as a followup.

Comment 73

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e0cdd102f9bb
https://hg.mozilla.org/integration/fx-team/rev/3703fe90c372
https://hg.mozilla.org/integration/fx-team/rev/873bd3a675a9

Comment 74

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e0cdd102f9bb
https://hg.mozilla.org/mozilla-central/rev/3703fe90c372
https://hg.mozilla.org/mozilla-central/rev/873bd3a675a9

Comment 75

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8667346 [details] [diff] [review]
Create an about:debugging page to list debuggable devtools targets.

Review of attachment 8667346 [details] [diff] [review]:
-----------------------------------------------------------------

I really like the components!  It feels much nicer, each one is of a manageable size.

Overall, the features implemented so far work well.

It may nice as a follow up to improve the page for RTL users.  You can check it somewhat easily with the "Force RTL" add-on from AMO.  Checking what about:addons looks like in this mode is a good comparison, I think.  We haven't focused much on RTL in the past for DevTools, but I thought it might be nice try thinking about it here, since the page is not too complex.

> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #47)
> > At the very least, let's separate the components from the main
> > `AboutDebugging` object.  But really, why not separate files for everything?
> > Small files are great!
> 
> As per our discussion on IRC, I separated all React components into
> individual files inside a "components/" folder. Hopefully the entailed
> modularity will be worth the extra overhead.

Hopefully you will grow to like it too. :)

> > ::: browser/locales/en-US/chrome/browser/devtools/aboutdebugging.dtd
> > @@ +1,5 @@
> > > +<!-- 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/. -->
> > > +
> > > +<!ENTITY aboutDebugging.title                      "about:debugging">
> > 
> > Now we get to bikeshed the user-visible name for the page!
> > 
> > Should it be different from the URL?
> 
> Yay, bikeshedding!
> 
> The title formats used by the 34 existing "about:topic" pages are:
> - "Description of Topic" (10 pages)
> - "Topic" (6 pages)
> - "about:topic" (6 pages)
> - "About Topic" (5 pages)
> - Unrelated Title (4 pages, including "about:robots" with "Gort! Klaatu
> barada nikto!")
> - "Topic Manager" (3 pages)
> 
> I really like the "about:topic" and "About Topic" formats, because they help
> you remember the URL for quick access.
> 
> I guess you'd prefer something more like "Description of Topic"?

Thanks for the analysis!  I... don't really feel strongly, just wanted to make sure we think about it, which you've clearly now done.

I'm not sure if it matters to much if that title reminds you of the URL, since you can also see the URL in the URL bar.

As additional data for consideration, Chrome's chrome://inspect uses "Inspect with Chrome Developer Tools" as the title.

Do what seems best to you. :)

> > See also: Use ESLint, it's your pote (I learned this word from a French Coke
> > can...).[1]
> > 
> > [1]: https://wiki.mozilla.org/DevTools/CodingStandards
> 
> Haha, that's a cool and spot-on use of colloquial French vocabulary! :D Made
> me laugh.

I'm glad to hear Coke cans are useful teaching tools (and also about ESLint use!). :)

> > @@ +16,5 @@
> > > +const { DebuggerClient } = require("devtools/shared/client/main");
> > > +const { DebuggerServer } = require("devtools/server/main");
> > > +const { TargetFactory } = require("devtools/client/framework/target");
> > > +const { Toolbox } = require("devtools/client/framework/toolbox");
> > > +const TabStore = require("devtools/client/webide/modules/tab-store");
> > 
> > Consider carefully if you want this module... it's quite of awkward, but
> > fine to use if think it works for your case.
> 
> Agreed, that module is somewhat akward. I used it mainly because it did what
> I needed: List all tabs from a given connection, sometimes using known
> workarounds or different code paths according to runtime version.
> 
> I could also refactor or reimplement something similar to make it cleaner
> (e.g. without the single "selectedTab" I don't need). That's something I'll
> consider for my upcoming re-work of runtime connections.

Great, sounds reasonable to me.

> > @@ +214,5 @@
> > > +    // Show the first available tab.
> > > +    this.showTab();
> > > +
> > > +    // Link checkboxes to prefs.
> > > +    let elements = document.querySelectorAll("input[type=checkbox][data-pref]");
> > 
> > Do the checkboxes make sense as React components as well?
> 
> I started writing a pref-checkbox React component just to see what it would
> look like, but I stopped when it grew over 50 lines of code.
> 
> I believe the current lightweight (9 LOC) plumbing code is fine for now, but
> I'll continue improving on what I wrote, hoping to end up with a component
> that works with different pref types (not just bool).

Okay, makes sense to me.

> > @@ +34,5 @@
> > > +      <div id="tab-addons" class="tab active">
> > > +        <div class="header">
> > > +          <h1 class="header-name">&aboutDebugging.addons;</h1>
> > > +        </div>
> > > +        <input id="enable-addon-debugging" type="checkbox" data-pref="devtools.chrome.enabled"/>
> > 
> > I believe we talked previously about whether to require this pref at all... 
> > what did we decide?  I forgot. :)
> 
> I believe that we do need that pref today, and I forgot that we even debated
> that. Are you saying that we should get rid of this pref?

I think overall the pref's existence is a bit strange...  It's not a security protection really.  It hides browser debugging so as to not overwhelm people (I guess?).

Even then, I'm not sure it's worth it.  Let's file a follow up bug and ni? :canuckistani so we can re-debate this somewhere that I can find again.

My current opinion is that it would be nice not to need this pref, at least for the about:debugging page (but maybe also in other places too), since it's pretty clear from the categories what you are looking at, and you just want to use the thing.  As it is today, the check box is basically saying "should this page function correctly?" which is odd question to ask, I think.

::: devtools/client/aboutdebugging/aboutdebugging.xhtml
@@ +13,5 @@
> +  <head>
> +    <title>&aboutDebugging.title;</title>
> +    <link rel="stylesheet" href="chrome://global/skin/global.css" type="text/css"/>
> +    <link rel="stylesheet" href="chrome://global/skin/in-content/common.css" type="text/css"/>
> +    <link rel="stylesheet" href="chrome://devtools/content/aboutdebugging/aboutdebugging.css"  type="text/css"/>

I think you can use a relative href, like "aboutdebugging.css"

@@ +15,5 @@
> +    <link rel="stylesheet" href="chrome://global/skin/global.css" type="text/css"/>
> +    <link rel="stylesheet" href="chrome://global/skin/in-content/common.css" type="text/css"/>
> +    <link rel="stylesheet" href="chrome://devtools/content/aboutdebugging/aboutdebugging.css"  type="text/css"/>
> +    <script type="application/javascript" src="resource:///modules/devtools/client/shared/vendor/react.js"></script>
> +    <script type="application/javascript;version=1.8" src="chrome://devtools/content/aboutdebugging/aboutdebugging.js"></script>

I think you can use a relative src, like "aboutdebugging.js"

@@ +34,5 @@
> +        <div class="header">
> +          <h1 class="header-name">&aboutDebugging.addons;</h1>
> +        </div>
> +        <input id="enable-addon-debugging" type="checkbox" data-pref="devtools.chrome.enabled"/>
> +        <label for="enable-addon-debugging" title="&options.enableChrome.tooltip3;">&options.enableChrome.label5;</label>

Reusing this existing tooltip is a little odd, since it mentions other tools that are not about:debugging.  Double-check the text when in the UI and see what you think about it.

Comment 76

[Jan Keromnes [:janx]]

Attachment: Create an about:debugging page to list debuggable devtools targets.

Thanks a lot Ryan! I addressed some of your nits (details below).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6da56b6cd2e9

Before landing this patch, let's wait for bug 1200534 to land (blocked on :jlongster's review).

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #75)
> Overall, the features implemented so far work well.

\o/

> It may nice as a follow up to improve the page for RTL users.  You can check
> it somewhat easily with the "Force RTL" add-on from AMO.  Checking what
> about:addons looks like in this mode is a good comparison, I think.  We
> haven't focused much on RTL in the past for DevTools, but I thought it might
> be nice try thinking about it here, since the page is not too complex.

Great idea! I filed bug 1210778 for that. Also, it wouldn't hurt to verify the page's accessibility.

> > As per our discussion on IRC, I separated all React components into
> > individual files inside a "components/" folder. Hopefully the entailed
> > modularity will be worth the extra overhead.
> 
> Hopefully you will grow to like it too. :)

I do like it, but splitting the files and then refactoring each file more than 10 times took a lot of time. But that was a one-time problem, not to mention that my workflow might be to blame.

> I think overall the pref's existence is a bit strange...  It's not a
> security protection really.  It hides browser debugging so as to not
> overwhelm people (I guess?).
> 
> Even then, I'm not sure it's worth it.  Let's file a follow up bug and ni?
> :canuckistani so we can re-debate this somewhere that I can find again.

Makes sense, I filed bug 1210790 with the rest of your comment.

> ::: devtools/client/aboutdebugging/aboutdebugging.xhtml
> @@ +13,5 @@
> > +  <head>
> > +    <title>&aboutDebugging.title;</title>
> > +    <link rel="stylesheet" href="chrome://global/skin/global.css" type="text/css"/>
> > +    <link rel="stylesheet" href="chrome://global/skin/in-content/common.css" type="text/css"/>
> > +    <link rel="stylesheet" href="chrome://devtools/content/aboutdebugging/aboutdebugging.css"  type="text/css"/>
> 
> I think you can use a relative href, like "aboutdebugging.css"

The reason I used an absolute path here is that when you access the page with the "about:debugging" alias, the relative path is wrong.

> @@ +15,5 @@
> > +    <link rel="stylesheet" href="chrome://global/skin/global.css" type="text/css"/>
> > +    <link rel="stylesheet" href="chrome://global/skin/in-content/common.css" type="text/css"/>
> > +    <link rel="stylesheet" href="chrome://devtools/content/aboutdebugging/aboutdebugging.css"  type="text/css"/>
> > +    <script type="application/javascript" src="resource:///modules/devtools/client/shared/vendor/react.js"></script>
> > +    <script type="application/javascript;version=1.8" src="chrome://devtools/content/aboutdebugging/aboutdebugging.js"></script>
> 
> I think you can use a relative src, like "aboutdebugging.js"

Same as above.

> @@ +34,5 @@
> > +        <div class="header">
> > +          <h1 class="header-name">&aboutDebugging.addons;</h1>
> > +        </div>
> > +        <input id="enable-addon-debugging" type="checkbox" data-pref="devtools.chrome.enabled"/>
> > +        <label for="enable-addon-debugging" title="&options.enableChrome.tooltip3;">&options.enableChrome.label5;</label>
> 
> Reusing this existing tooltip is a little odd, since it mentions other tools
> that are not about:debugging.  Double-check the text when in the UI and see
> what you think about it.

You're right, the label was a little odd, but the tooltip was misleading.

I replaced both label and tooltip with something about:debugging-specific:
- "Enable add-on debugging"
- "Turning this on will allow you to debug add-ons and various other parts of the browser chrome"

Comment 77

[Brian Grinstead [:bgrins]]

I'm not sure if this is just something in my environment but I have the latest patch applied and when going to about:debugging#workers and then loading http://bgrins.github.io/devtools-demos/worker/webworker.html I'm seeing a bunch of errors in the browser console (and no workers showing up on the page).  Do I need anything besides Attachment #8668972 [details] [diff] applied?

Handler function DebuggerClient.requester request callback threw an exception: ReferenceError: document is not defined
Stack: getActiveElement@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/client/shared/vendor/react.js:19461:5
[71]</ReactInputSelection.getSelectionInformation@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/client/shared/vendor/react.js:11697:23
[116]</Mixin.initializeAll@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/client/shared/vendor/react.js:18436:11
[116]</Mixin.perform@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/client/shared/vendor/react.js:18401:7
[116]</Mixin.perform@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/client/shared/vendor/react.js:18402:13
[100]</<.perform@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/client/shared/vendor/react.js:16618:1
[100]</flushBatchedUpdates@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/client/shared/vendor/react.js:16698:7
[116]</Mixin.closeAll@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/client/shared/vendor/react.js:18475:11
[116]</Mixin.perform@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/client/shared/vendor/react.js:18416:11
[59]</ReactDefaultBatchingStrategy.batchedUpdates@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/client/shared/vendor/react.js:9669:7
enqueueUpdate@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/client/shared/vendor/react.js:16738:5
enqueueUpdate@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/client/shared/vendor/react.js:16256:5
[99]</ReactUpdateQueue.enqueueSetState@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/client/shared/vendor/react.js:16434:5
[39]</ReactComponent.prototype.setState@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/client/shared/vendor/react.js:6418:3
update/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/client/aboutdebugging/components/workers.js:82:7
DebuggerClient.requester/</<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/shared/client/main.js:284:9
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/shared/DevToolsUtils.js:87:14
emitOnObject@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/event/core.js:112:9
emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/event/core.js:89:38
Request.prototype.emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/shared/client/main.js:1158:29
DebuggerClient.prototype.onPacket/emitReply@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/shared/client/main.js:969:29
DevTools RDP*DebuggerClient.prototype.request@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/shared/client/main.js:675:5
DebuggerClient.requester/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/shared/client/main.js:272:1
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/shared/DevToolsUtils.js:87:14
update@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/client/aboutdebugging/components/workers.js:61:5
eventSource/aProto.emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/shared/client/main.js:125:9
DebuggerClient.prototype.onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/shared/client/main.js:965:7
LocalDebuggerTransport.prototype.send/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/shared/transport/transport.js:569:11
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/shared/DevToolsUtils.js:87:14
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/shared/DevToolsUtils.js:87:14
Line: 19461, column: 5

Comment 78

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Brian Grinstead [:bgrins] from comment #77)
> I'm not sure if this is just something in my environment but I have the
> latest patch applied and when going to about:debugging#workers and then
> loading http://bgrins.github.io/devtools-demos/worker/webworker.html I'm
> seeing a bunch of errors in the browser console (and no workers showing up
> on the page).  Do I need anything besides Attachment #8668972 [details] [diff]
> [diff] applied?

Yes, you need the React upgrade from bug 1200534 as well.

Comment 79

[Brian Grinstead [:bgrins]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #78)
> Yes, you need the React upgrade from bug 1200534 as well.

Ah thanks, that did the trick

Comment 80

[Jan Keromnes [:janx]]

Sheriffs, please land the last patch of this series just after the patch in bug 1200534.

Comment 81

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f7d061945bbc

Comment 82

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f7d061945bbc

Comment 83

[Stefan Plewako [:stef]]

In future please use better string identifiers,
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Choose_good_key_IDs

Comment 85

[Khalid Syfullah Zaman [:khalid32]]

I have reproduced this bug on Nightly 43.0a1 (2015-08-20) on ubuntu 14.04 LTS, 32 bit!

The bug's fix is now verified on Latest Aurora 44.0a2!

Build ID: 20151125004042
User Agent: Mozilla/5.0 (X11; Linux i686; rv:44.0) Gecko/20100101 Firefox/44.0

[bugday-20151125]

Comment 86

[Mohammad Maruf Islam]

I have reproduced this bug with Firefox Nightly 43.0a1 (Build ID: 20150820030202) on 
windows 8.1 64-bit with the instructions from comment 0 .

Verified as fixed with Firefox Aurora 44.0a2 (Build ID: 20151125004042)

Mozilla/5.0 (Windows NT 6.3; WOW64; rv:44.0) Gecko/20100101 Firefox/44.0

As this bug is also verified on Linux (Comment 85) ,I am marking this verified.