Closed Bug 1196785 (about:debugging) Opened 5 years ago Closed 4 years ago

Create an about:debugging page to list all debuggable targets.

Categories

(DevTools :: about:debugging, defect)

defect
Not set

Tracking

(firefox44 verified)

VERIFIED FIXED
Firefox 44
Tracking Status
firefox44 --- verified

People

(Reporter: janx, Assigned: janx)

References

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

Details

(Keywords: DevAdvocacy, Whiteboard: [devtools-ux])

Attachments

(7 files, 13 obsolete files)

8.19 KB, patch
jst
: review+
janx
: checkin+
Details | Diff | Splinter Review
2.27 KB, patch
janx
: review+
janx
: checkin+
Details | Diff | Splinter Review
172.07 KB, image/png
Details
677 bytes, patch
janx
: review+
janx
: checkin+
Details | Diff | Splinter Review
8.92 KB, patch
janx
: review+
janx
: checkin+
Details | Diff | Splinter Review
1.24 KB, patch
janx
: review+
janx
: checkin+
Details | Diff | Splinter Review
26.02 KB, patch
janx
: review+
Details | Diff | Splinter Review
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...).
(Renaming to "about:debug", as bikeshedded in https://etherpad.mozilla.org/about-inspect)
Alias: about:inspect → about:debug
Summary: Implement an about:inspect page to list all inspectable targets, local and remote → Implement an about:debug page to list all debuggable targets, local and remote.
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?
Attachment #8654846 - Flags: review?(jlong)
Comment on attachment 8654848 [details] [diff] [review]
Alphabetically sort redirected about:pages.

David, this was bothering me more than it should have, sorry!
Attachment #8654848 - Flags: review?(dteller)
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.
Attachment #8654849 - Flags: review?(jaws)
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.
Attachment #8654848 - Flags: review?(dteller) → review?(jst)
Attachment #8654848 - Attachment is obsolete: true
Attachment #8654848 - Flags: review?(jst)
Attachment #8654875 - Flags: review?(jst)
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] {
Attachment #8654849 - Flags: review?(jaws) → review+
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?
Attachment #8654846 - Flags: review?(jlong) → review-
(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
Changed second 'xul' to wildcard (see comment 10).
Attachment #8654849 - Attachment is obsolete: true
Attachment #8655304 - Flags: review+
Depends on: 1200534
Attachment #8654846 - Attachment is obsolete: true
Attachment #8654875 - Flags: review?(jst) → review+
Bikeshedding follow-up: "about:debugging" better respects the apparent naming conventions of about:pages.
Alias: about:debug → about:debugging
Summary: Implement an about:debug page to list all debuggable targets, local and remote. → Implement an about:debugging page to list all debuggable targets, local and remote.
All green, ready for launch.
Keywords: checkin-needed
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).
Attachment #8657108 - Flags: feedback?(jryans)
[WIP] Screenshot of "Workers" page.
Attachment #8657111 - Attachment description: aboutdebugging.png → [WIP] Screenshot of "Workers" page.
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.
Attachment #8657108 - Flags: feedback?(jryans)
Attachment #8657108 - Flags: feedback?(jlong)
Attachment #8657108 - Flags: feedback+
(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).
I thought I was ni? on this bug but just noticed it was for feedback. I can glance this patch tomorrow if you want.
(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 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.
Attachment #8657108 - Flags: feedback?(jlong)
(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?
(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. :)
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.
Whiteboard: [devtools-ux]
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.
Attachment #8660851 - Flags: review?(ejpbruel)
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.
Attachment #8660852 - Flags: review?(ejpbruel)
Blocks: 1204601
Blocks: 1204603
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.
Attachment #8660852 - Flags: review?(ejpbruel) → review?(past)
Comment on attachment 8660851 [details] [diff] [review]
Add worker type to WorkerActor form.

Just adding the worker type to the WorkerActor form.
Attachment #8660851 - Flags: review?(ejpbruel) → review?(past)
Attachment #8654875 - Flags: checkin+
Attachment #8655304 - Flags: checkin+
Attachment #8660851 - Flags: review?(past) → review+
Thank you Panos! Edited the "r=" flag in the description, keeping r+.
Attachment #8660851 - Attachment is obsolete: true
Attachment #8661779 - Flags: review+
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.
Attachment #8660852 - Flags: review?(past)
> 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 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?
Attachment #8660852 - Flags: feedback?(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.
Attachment #8660852 - Flags: feedback?(ally)
Depends on: 1207506
Rebased on top of devtools earthquake, keeping Panos' r+.
Attachment #8661779 - Attachment is obsolete: true
Attachment #8664819 - Flags: review+
Rebased, addressed nits.
Attachment #8660852 - Attachment is obsolete: true
Rebased, addressed nits.
Attachment #8657108 - Attachment is obsolete: true
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.
Attachment #8664822 - Flags: review?(past)
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.
Attachment #8664822 - Flags: review?(ally)
Fix a split-patch issue.
Attachment #8664823 - Attachment is obsolete: true
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).
Attachment #8664840 - Flags: review?(jryans)
Summary: Implement an about:debugging page to list all debuggable targets, local and remote. → Create an "about:debugging" page to list all debuggable targets.
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.
Attachment #8664840 - Flags: review?(jryans)
Blocks: 1207997
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.
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
Flags: needinfo?(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.
Flags: needinfo?(ejpbruel)
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.
(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 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
Attachment #8664822 - Flags: review?(ally) → review+
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 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!
Attachment #8664822 - Flags: review?(past) → review+
Rebased.
Attachment #8664819 - Attachment is obsolete: true
Attachment #8666642 - Flags: review+
Rebased and added "alert_emails" field as suggested in comment 55.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a7993a6170c
Attachment #8664822 - Attachment is obsolete: true
Attachment #8666643 - Flags: review+
Sorted globals list, added "React", fixed bad reference to "/devtools/client/.eslintrc.xpcshell".

Patrick, please have a look.
Attachment #8666656 - Flags: review?(pbrosset)
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.
Attachment #8666656 - Flags: review?(pbrosset) → review+
Thanks Patrick! I removed the "React" global from the list.
Attachment #8666656 - Attachment is obsolete: true
Attachment #8666667 - Flags: review+
More complete try push for all three patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=98cf8406c237
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.
Keywords: checkin-needed
(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
(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.)
Depends on: 1209559
Depends on: 1209581
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?
Attachment #8664840 - Attachment is obsolete: true
Attachment #8667346 - Flags: review?(jryans)
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?
Attachment #8667346 - Flags: review?(gijskruitbosch+bugs)
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?
Attachment #8667346 - Flags: review?(gijskruitbosch+bugs) → feedback+
Needinfo Jeff about the treeherder link with Mac OS builds.
Flags: needinfo?(jgriffiths)
Blocks: 1209699
(In reply to Jan Keromnes [:janx] from comment #70)
> Needinfo Jeff about the treeherder link with Mac OS builds.

Flags: needinfo?(jgriffiths)
(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.
Blocks: 1209746
Attachment #8666642 - Flags: checkin+
Attachment #8666643 - Flags: checkin+
Attachment #8666667 - Flags: checkin+
Summary: Create an "about:debugging" page to list all debuggable targets. → Create an about:debugging page to list all debuggable targets.
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.
Attachment #8667346 - Flags: review?(jryans) → review+
Keywords: leave-open
Blocks: 1209369, 1209559
No longer depends on: 1209369, 1209559
Blocks: 1210778
Depends on: 1210790
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"
Attachment #8667346 - Attachment is obsolete: true
Attachment #8668972 - Flags: review+
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
(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.
(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
Sheriffs, please land the last patch of this series just after the patch in bug 1200534.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f7d061945bbc
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Blocks: sw-devtools
Blocks: 1212797
Blocks: 1212802
Blocks: 1214248
Duplicate of this bug: 1178531
Blocks: 1215594
Component: Developer Tools → Developer Tools: about:debugging
Depends on: 1220243
Blocks: 1220747
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]
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.
Status: RESOLVED → VERIFIED
QA Whiteboard: [bugday-20151125]
Blocks: 1231385
Blocks: 1233997
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.