Closed Bug 1233463 Opened 9 years ago Closed 8 years ago

Allow loading toolbox in a tab

Categories

(DevTools :: Framework, defect)

Product:
DevTools
Component:
Framework
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox48 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 48
Tracking Flags:
Tracking Status
firefox48 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 12 obsolete files)

Ensure waiting for toolbox initialization in browser_cmd_csscoverage_startstop.js
1.24 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
Expose about:devtools-toolbox shortcut to load the toolbox.
5.90 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
Support outerWindowID in getTab request for tabs in child processes.
1.37 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
Allow loading devtools in a tab or an html:iframe.
14.94 KB, patch
ochameau
: review+
Details | Diff | Splinter Review 
If I hear correctly from the attempt to rewrite the toolbox in html, on big advantage of working on an html version was being able to load it in a tab and have devtools next to it, like a webpage.
We can actually do that quite easily.
Actually the current code is quite weird. We have toolbox.js which is a module that controls toolbox.xul which doesn't have any script.
If it was the other way around it would feel more natural,
but anyway we can load the toolbox in a tab.
Note that it may have some stuff broken as in the app-manager due to content docshell issues...
The main thing is about coming up with same query parameters allowing to select a particular tab/target.
We discussed about that in bug 1212802 and would already like to do that for about:debugging. We should keep this in sync in order to support the same parameters.
Assignee: nobody → poirot.alex
Speaking about contribution friction, this would help as it ease contributing to the UI code.
You don't need to play around two windows and the browser toolbox (pref+connection prompts, multi windows)
Blocks: dt-contribute
Attached patch patch v1 (obsolete) — — Splinter Review 
This patch does two things:
- allow loading from a tab
- allow loading in an html:iframe (useful for browser.html)

1/ It register a new about:devtools-panel protocol to have decent urls
2/ It adds a JS script in toolbox.xul
3/ This scripts looks at query parameters,
   if there is a valid one it create the target
   for the context we would like to debug
   (this is where we should sync up with about:debugging)
4/ Once we have the target we init the toolbox with it
Attached patch patch v2 (obsolete) — — Splinter Review 
With a missing file
Attachment #8699534 - Attachment is obsolete: true
Depends on: 1236452
Attached patch patch v3 (obsolete) — — Splinter Review 
Rebase with root support.
I would like to move the "URL to target/toolbox" code into a module
that could be used by about:debugging.
And have a test also for this.

Open firefox with two tabs, the first one being a webpage.
In the second tab, open about:devtools-panel?tab-id=1
This will open a toolbox debugging your first tab,
now you can also open a toolbox for your toolbox by hitting CTRL+MAJ+K.
Type "location" in each console to figure out who is who.

Now, you can open another tab with about:devtools-panel?root
This time, it opens a browser toolbox like one.

Feedback is welcomed.
Attachment #8700612 - Attachment is obsolete: true
Attached patch patch v4 (obsolete) — — Splinter Review 
With a module that focus on handling query parameters.
Hopefully, about:debugging can reuse this one.
And with a test.

First, the short url.
I'm adding a new about:devtools-panel shortcut for toolbox.xul.
I'm not sure that's the best name we can come up with.
May be thinking around about:debugging possible future usage may help?
I don't know if we are going to do that, but about:debugging
may open tabs with toolbox preconfigured to connect to a precise target
(this tab on fennec, this service worker, [this app on b2g], ...)

Then the parameters. I added a bunch, I remove some if needed.

When debugging a local tab:
  tab-idx: the tab index, starting from 1 to refer to a local tab in your firefox
  window-id: optional, allows to specify which top level window is targeted, by index starting from 1

When debugging a remote tab:
  tab-id: for OOP tabs, the "tabId" field of the frame loader
  tab-window-id, for tabs in parent process, its outer window id

When debugging the parent process
  root: spawn a kind of browser toolbox, but in the same process

Optional parameters that can be passed:
  chrome: force the creation of a chrome target, allows to spawn a chrome toolbox for a tab
          (handy for platform devs)

A parameter dedicated to experiment on browser.html
  target: allows to debug an arbitrary iframe. Most likely a mozbrowser iframe
          or also a xul:browser.
          The html browser create a toolbox iframe on which it sets a "target"
          attributes which refers to the tab we want to debug.
  this parameter is only handled by toolbox.xul, this isn't part of the helper module
  as it is quite specific to the toolbox iframe.
Necessary patch to get green try.
Attachment #8722522 - Flags: review?(jryans)
Attachment #8717089 - Attachment is obsolete: true
Attachment #8722521 - Flags: review?(jryans)
Comment on attachment 8722522 [details] [diff] [review]
Ensure waiting for toolbox initialization in browser_cmd_csscoverage_startstop.js.

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

::: devtools/client/commandline/test/browser_cmd_csscoverage_startstop.js
@@ +49,5 @@
>    yield helpers.listenOnce(options.browser, "load", true);
>    is(options.window.location.href, PAGE_3, "page 3 loaded");
>  
> +  let toolboxReady = new Promise(done => {
> +    gDevTools.once("toolbox-ready", done);

`once` already returns a promise, let's use it! :)
Attachment #8722522 - Flags: review?(jryans) → review+
Comment on attachment 8722521 [details] [diff] [review]
patch v4

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

It seems quite cool overall! :)

::: devtools/client/framework/about-devtools-panel.js
@@ +11,5 @@
> +const Cm = components.manager.QueryInterface(Ci.nsIComponentRegistrar);
> +const { XPCOMUtils } = Cu.import("resource://gre/modules/XPCOMUtils.jsm", {});
> +const { Services } = Cu.import("resource://gre/modules/Services.jsm", {});
> +
> +function AboutURL() {

Seems like we should use this same approach to register about:debugging, instead of throwing it in C++ files like we do today.  (Just a thought for the future.)

@@ +16,5 @@
> +}
> +
> +AboutURL.prototype = {
> +  uri: Services.io.newURI("chrome://devtools/content/framework/toolbox.xul", null, null),
> +  classDescription: "about:devtools-panel",

Why not use "about:devtools-toolbox"?  Usually we think of a "panel" as one tool in our toolbox.

@@ +22,5 @@
> +  contractID: "@mozilla.org/network/protocol/about;1?what=devtools-panel",
> +
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIAboutModule]),
> +
> +  newChannel : function(aURI, aLoadInfo) {

Nit: newChannel:

@@ +29,5 @@
> +    return chan;
> +  },
> +
> +  getURIFlags: function(aURI) {
> +    return 0;

What about some flags?

https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/about/nsIAboutModule.idl

Maybe ALLOW_SCRIPT | URI_CAN_LOAD_IN_CHILD | ENABLE_INDEXED_DB?

@@ +36,5 @@
> +
> +let cls = AboutURL;
> +const factory = {
> +  _cls: cls,
> +  createInstance: function(outer, iid) {

Seems like this can go in AboutURL instead?

https://dxr.mozilla.org/mozilla-central/rev/d848a5628d801a460a7244cbcdea22d328d8b310/browser/extensions/pocket/bootstrap.js#113

::: devtools/client/framework/toolbox-init.js
@@ +11,5 @@
> +// Only try to attach the toolbox if some query parameters are given
> +if (url.search.length > 1) {
> +  const Cu = Components.utils;
> +  const Ci = Components.interfaces;
> +  const {gDevTools} = Cu.import("resource://devtools/client/framework/gDevTools.jsm", {});

Nit: Seems like you should use spaces inside braces here since you use them below when making objects.

::: devtools/client/main.js
@@ +8,5 @@
>  Cu.import("resource://gre/modules/Services.jsm");
>  const { gDevTools } = require("devtools/client/framework/devtools");
>  
>  const { defaultTools, defaultThemes } = require("devtools/client/definitions");
> +const AboutDevtools = require("devtools/client/framework/about-devtools-panel");

Nit: AboutDevTools

::: devtools/client/shared/moz.build
@@ +40,5 @@
>      'telemetry.js',
>      'theme-switching.js',
>      'theme.js',
>      'undo.js',
> +    'url2target.js',

This belongs in framework next to the target, etc.

::: devtools/client/shared/url2target.js
@@ +20,5 @@
> + *        Optional object with following attributes:
> + *        - {Window} window
> + *        When querying a local tab, look for it in this browser window.
> + *        - {IframeElement} tab
> + *        Allows to directly target one precise tab. 

Nit: trailing whitespace

@@ +24,5 @@
> + *        Allows to directly target one precise tab. 
> + *
> + * @return A target object
> + */
> +exports.url2target = Task.async(function *(url, options = {}) {

Maybe call it targetFromURL?  Not sure I like the "2" in the name.  Maybe this should just live in the target.js module, since it's basically a factory for making Targets?

@@ +31,5 @@
> +  let client = createClient();
> +
> +  yield client.connect();
> +
> +  let params = url.searchParams;

Add a large comment block that clearly describes each param all in one place.  I will review each param more closely once this comment exists.
Attachment #8722521 - Flags: review?(jryans) → feedback+
Attached patch patch v5 (obsolete) — — Splinter Review 
With comment addressed and all parameters documented in code:

 - renamed to url-to-target.js
   Kept a distinct module. I expect it to become bigger with client creation support.
   New arguments should allow specifying a precise remote (local pipe, remote connection, ...)
   It won't be just the target, it will also have a client part, but at the end
   you want the final target object.
 
 - used slightly tweaked uri flags to not allow it to be loaded in child process.
   We would need to work on that to make it work. but that could be a great way to debug the parent process from child :)
 
 - used about:devtools-toolbox
Attachment #8723570 - Flags: review?(jryans)
Attachment #8722521 - Attachment is obsolete: true
Comment on attachment 8723570 [details] [diff] [review]
patch v5

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

Wait. I have an exception.
Attachment #8723570 - Flags: review?(jryans)
Attached patch patch v6 (obsolete) — — Splinter Review 
Should be good now. Try will confirm.
I have some issues with the toolbox document location.
In the common codepath, when we open a regular toolbox,
toolbox.js has to set the iframe location.
Whereas we really shouldn't update the location when we open it in a tab.
There is also some issues with the DOMContentLoaded listener
where we have to ensure ignoring the event for the about:blank document
loaded before our expected document.
See my comments in Toolbox.open.
Attachment #8723593 - Flags: review?(jryans)
Attachment #8723570 - Attachment is obsolete: true
Attachment #8722522 - Attachment is obsolete: true
I addressed the comment poorly and broke the test, here is a better version.
Attachment #8724406 - Flags: review+
Attachment #8723594 - Attachment is obsolete: true
Comment on attachment 8723593 [details] [diff] [review]
patch v6

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

::: devtools/client/framework/toolbox-init.js
@@ +7,5 @@
> +// URL constructor doesn't support about: scheme
> +let href = window.location.href.replace("about:", "http://");
> +let url = new window.URL(href);
> +
> +// Only try to attach the toolbox if some query parameters are given

Maybe something like "Only use this method to attach..." since we still load the document and this script when opening a normal page toolbox.

::: devtools/client/framework/url-to-target.js
@@ +11,5 @@
> +const {DebuggerClient} = require("devtools/shared/client/main");
> +const {Task} = require("resource://gre/modules/Task.jsm");
> +
> +/**
> + * Construct a Target for a given URL object having various query parameters:

My initial reaction is there has to be a simpler way to describe all this stuff.  Can we add some notion of "target type" in here and use that to select what path to go down, instead of so many similar (but not the same!) IDs and indexes?

What about something like this?

```
Construct a Target for a given URL object having various query parameters:

* type: tab, process
    {String} The type of target to connect to.  Currently tabs and processes are supported types.
* remote: true, false
    {Boolean} Is the target in a remote process or the current process?
* process: parent, child
    {String} Is the target in the parent or child process?
* id: the ID / index of the target
    {String / Integer} An identifier that denotes the particular target, such as:
      * the index of local tab inside it's window (or ID, see other review notes)
      * the tab ID from the child process (frameElement.frameLoader.tabParent.tabId)
      * the tab ID from the parent process (frameElement.contentWindow.DOMWindowUtils.outerWindowID)
```

I think this gives a little more structure to it.  I can imagine it will only get even more complex as devices and such are added from about:debugging.

There may be better properties to use instead of the above, feel free to suggest a different grouping!

@@ +15,5 @@
> + * Construct a Target for a given URL object having various query parameters:
> + *
> + * When debugging a local tab, in the same firefox desktop instance:
> + * tab-idx:
> + *   Tab index, starting from 1.

Can we use tadIds for local tabs too?  It's unfortunate that we use indexes sometimes and IDs for others.

@@ +53,5 @@
> + *        Allows to directly target one precise tab.
> + *
> + * @return A target object
> + */
> +exports.targetFromURL = Task.async(function *(url, options = {}) {

Since this is main export, should the module name be "target-from-url" instead of "url-to-target"?

@@ +119,5 @@
> +    // Creates a target for a given browser iframe.
> +    let response = yield client.getTab({ tab: options.tab });
> +    form = response.tab;
> +  } else {
> +    throw new Error("Url2Target, missing supported URL argument");

Nit: targetFromURL
Attachment #8723593 - Flags: review?(jryans) → feedback+
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #22)
> @@ +15,5 @@
> > + * Construct a Target for a given URL object having various query parameters:
> > + *
> > + * When debugging a local tab, in the same firefox desktop instance:
> > + * tab-idx:
> > + *   Tab index, starting from 1.
> 
> Can we use tadIds for local tabs too?  It's unfortunate that we use indexes
> sometimes and IDs for others.

Ok. So the thing is that we don't have to support all this.

There is two very different usages here:
 * manually crafting the URL to target a local tab
 * connecting to a remote from about:debugging or something else that craft the right URL for you

Tab indexes are only helpful when manually crafting the URL, where you can't easily figure out outerWindowID or tabId.
But on the other hand, I don't think tab indexes are safe enough to be used against a remote target. We used to only support very precise identifiers: outerWindowID for in parent process and tabId for in childProcess.

Unfortunately tabId is something specific to e10s. So we can't use that for tabs in parent process.
May be we should use tab indexes for remotes instead? May be that's stable enough? But it would require various changes to the protocol. We would need a notion of windows and indexes in listTabs and getTab.
I'm fine doing that, but given that this patch is entangled with many others of my cleaning up patch queue, I wish I could do that after. If so, I could just support local tabs for now and support remote ones once listTabs and getTab are improved?
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #22)
> Comment on attachment 8723593 [details] [diff] [review]
> * remote: true, false
>     {Boolean} Is the target in a remote process or the current process?
> * process: parent, child
>     {String} Is the target in the parent or child process?

remote and process are redundant. And if you want to manually craft the query parameters, it will be hard to figure out if the tab you are targetting is going to be remote or not.

I think it is easier to think by figuring out how you would manually craft parameters to target any given target somewhat easily? And then ensuring that it maps to RDP requests.
Blocks: 1247203
I'm not providing a new patch until we agree on a plan/parameters, suggested in comment 23.
Your suggested parameters don't seem to work per comment 24.

Here is a possible doc if we switch remote to use indexes instead of tabId/outerWindowId:

 * When debugging a browser tab, [in the same firefox desktop instance][to-be-removed]:
 * tab-idx:
 *   Tab index, starting from 1.
 * window-idx:
 *   Optional option used in addition to tab-idx, allows to specify which top
 *   level window is targeted. Index of the window starting from 1.
 *   This isn't z-index based, instead this is by order or creation/opening.
 *
 [* Both these identifiers are return by client.listTabs() request.][to-be-added]
 *
 * When debugging the browser:
 * root:
 *   Without any value, spawn a privileged target to debug the whole browser.
 *   Returns a target connected to the ChromeActor.
 *
 * Additional optional parameters:
 * chrome:
 *   Force the creation of a chrome target. Gives more privileges to the tab
 *   actor. Allows chrome execution in the webconsole and see chrome files in
 *   the debugger. (handy when contributing to firefox)

This is a simplified version of my current patch, thanks to a simplification of RDP listTabs/getTab.
But I could also come up with something closer to what you suggested:

* type: tab, process
*    {String} The type of target to connect to.  Currently tabs and processes are supported types.
I think we would then need, per type parameters:
* If type="tab":
* index: (or idx?)
*    {Number} the index of local[to-be-removed] tab inside it's window
* window-index: (or window-idx?) Optional
*    {Number} the index of local[to-be-removed] top level browser window. Defaults to active window.
* chrome: Optional
*    {Boolean} Force the creation of a chrome target. Gives more privileges to the tab
*    actor. Allows chrome execution in the webconsole and see chrome files in
*    the debugger. (handy when contributing to firefox)
*
* If type="process":
* id:
*    {Number} the process id to debug. Default to 0, which is the parent process.
Flags: needinfo?(jryans)
(In reply to Alexandre Poirot [:ochameau] PTO, back on 14th from comment #23)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #22)
> > @@ +15,5 @@
> > > + * Construct a Target for a given URL object having various query parameters:
> > > + *
> > > + * When debugging a local tab, in the same firefox desktop instance:
> > > + * tab-idx:
> > > + *   Tab index, starting from 1.
> > 
> > Can we use tadIds for local tabs too?  It's unfortunate that we use indexes
> > sometimes and IDs for others.
> 
> Ok. So the thing is that we don't have to support all this.
> 
> There is two very different usages here:
>  * manually crafting the URL to target a local tab
>  * connecting to a remote from about:debugging or something else that craft
> the right URL for you
> 
> Tab indexes are only helpful when manually crafting the URL, where you can't
> easily figure out outerWindowID or tabId.
> But on the other hand, I don't think tab indexes are safe enough to be used
> against a remote target. We used to only support very precise identifiers:
> outerWindowID for in parent process and tabId for in childProcess.
> 
> Unfortunately tabId is something specific to e10s. So we can't use that for
> tabs in parent process.
> May be we should use tab indexes for remotes instead? May be that's stable
> enough? But it would require various changes to the protocol. We would need
> a notion of windows and indexes in listTabs and getTab.
> I'm fine doing that, but given that this patch is entangled with many others
> of my cleaning up patch queue, I wish I could do that after. If so, I could
> just support local tabs for now and support remote ones once listTabs and
> getTab are improved?

I think I would prefer stronger identifiers like the IDs (tabId / outerWindowId) instead of indices.  Will listTabs() give you the proper IDs today...?  Seems like in-process tabs give you outerWindowId

Using "opaque tokens" like this seems more robust than array indices.

(In reply to Alexandre Poirot [:ochameau] PTO, back on 14th from comment #24)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #22)
> > Comment on attachment 8723593 [details] [diff] [review]
> > * remote: true, false
> >     {Boolean} Is the target in a remote process or the current process?
> > * process: parent, child
> >     {String} Is the target in the parent or child process?
> 
> remote and process are redundant. And if you want to manually craft the
> query parameters, it will be hard to figure out if the tab you are
> targetting is going to be remote or not.
> 
> I think it is easier to think by figuring out how you would manually craft
> parameters to target any given target somewhat easily? And then ensuring
> that it maps to RDP requests.

Tell me more about the use case you're imagining where someone would manually craft the URL.  So far, I don't really see that happening.  I would expect they would all be made programmatically by about:debugging or similar.

To me, the main features of having this URL are:

* It makes it easy to load the toolbox in a tab, you just create a tab with the proper URL
* It can be bookmarked, etc. to reconnect to the same thing again

It might also be _neat_ if people can see how they might tweak the URL to connect to something else, but so far I don't see people actually wanting to do so.

(In reply to Alexandre Poirot [:ochameau] PTO, back on 14th from comment #25)
> I'm not providing a new patch until we agree on a plan/parameters, suggested
> in comment 23.

How about an ID based version:

* type: tab, process
*    {String} The type of target to connect to.  Currently tabs and processes are supported types.
* If type="tab":
* id:
*    {Number} the outerWindowId of local[to-be-removed] tab inside it's window
* browser-window: Optional (Do we need this at all if we are using outerWindowId?)
*    {Number} the index of local[to-be-removed] top level browser window. Defaults to active window.
* chrome: Optional
*    {Boolean} Force the creation of a chrome target. Gives more privileges to the tab
*    actor. Allows chrome execution in the webconsole and see chrome files in
*    the debugger. (handy when contributing to firefox)
*
* If type="process":
* id:
*    {Number} the process id to debug. Default to 0, which is the parent process.

So, I made the following modifications:

* I think it's fine to use an "id" param for multiple purposes, since we have a "type" to clarify.  I would expect most targetable things to have some kind of ID
* I don't think we need to say "-idx" or "-index" in the param name.  Since you can't put objects in a URL, it seems redundant, as you pretty much have to identify things by numbers.
* I changed to prefer IDs instead of indices.
* Maybe browser-window can still be removed, since we are not using indices?

What do you think?
Flags: needinfo?(jryans)
(please ignore my previous comment)

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #26)
> Tell me more about the use case you're imagining where someone would
> manually craft the URL.  So far, I don't really see that happening.  I would
> expect they would all be made programmatically by about:debugging or similar.
> ...
> It might also be _neat_ if people can see how they might tweak the URL to
> connect to something else, but so far I don't see people actually wanting to
> do so.
> 

Given that there isn't much progress on supporting remotes in about:debugging, as well as tabs and processes.
So I gave some thoughts on how to manually craft these URLs to not depend on that.
But yes, ideally, they would easily be automatically crafted by about:debugging.
As you say, it is also a way to manually craft these URLs, but not a key goal here.


Ok. So if we forget support of manual URL crafting, here is what we could do:

* type: tab, process
*    {String} The type of target to connect to.  Currently tabs and processes are supported types.
*
* If type="tab":
* id:
*    {Number} the tab outerWindowID
* chrome: Optional
*    {Boolean} Force the creation of a chrome target. Gives more privileges to the tab
*    actor. Allows chrome execution in the webconsole and see chrome files in
*    the debugger. (handy when contributing to firefox)
*
* If type="process":
* id:
*    {Number} the process id to debug. Default to 0, which is the parent process.


I changed that:
 + removed browser-window, outerwindowid is unique per process, not per window.
 + rephrased "id" description for tabs
   And I didn't knew it seems to be even unique across processes (bug 1126042)!
     http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentChild.cpp#3201
   So that we do not necessarely need tabId. I would have to check that listTabs and getTab work fine
   with outerWindowID for tabs in child processes, and try to fix it if that doesn't.
   We could also drop support of tabId?

Given that we reached something quite similar I'll start updating the patch.
Let's split this patch in meaningful pieces.
A first one, just to introduce the about: page.
Attachment #8730690 - Flags: review?(jryans)
Attachment #8723593 - Attachment is obsolete: true
We need this patch to ensure that getTab works for tabs in child process when we pass its outerWindowID.
Attachment #8730691 - Flags: review?(jryans)
The main part of the patch, that introduce target-from-url and use it from toolbox.xul.
I had to tweak a couple of things in toolbox.js in order to ensure it work with an already loaded toolbox document.
Attachment #8730692 - Flags: review?(jryans)
Equivalent of the other patch regarding outerWindowID but on the client side.
(windowutils doesn't work for remote tabs, instead there is a magic outerWindowID attribute set on the browser element)
Also, this patch removes the support of tabId as it is not longer useful.

I can keep this patch for a followup...
BTW, if you want to test the patch for tabs, you can fetch the current tab outerWindowID via the browser console, with this:
  top.gBrowser.mCurrentBrowser.outerWindowID
Comment on attachment 8730690 [details] [diff] [review]
Expose about:devtools-toolbox shortcut to load the toolbox.

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

::: devtools/client/framework/about-devtools-toolbox.js
@@ +8,5 @@
> +// in a Firefox tab or a custom html iframe in browser.html
> +
> +const { Ci, Cu, components } = require("chrome");
> +const Services = require("Services");
> +const Cm = components.manager.QueryInterface(Ci.nsIComponentRegistrar);

Cm is available from `require("chrome")`, are you able to use that one instead?

@@ +44,5 @@
> +exports.register = function () {
> +  if (Cm.isCIDRegistered(AboutURL.prototype.classID)) {
> +    console.error("Trying to register " + AboutURL.prototype.classDescription + " more than once.");
> +  } else {
> +    Cm.registerFactory(AboutURL.prototype.classID, AboutURL.prototype.classDescription, AboutURL.prototype.contractID, AboutURL);

Nit: Wrap after 80 chars
Attachment #8730690 - Flags: review?(jryans) → review+
Comment on attachment 8730692 [details] [diff] [review]
Allow loading devtools in a tab or an html:iframe

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

::: devtools/client/framework/target-from-url.js
@@ +33,5 @@
> + *        The url to fetch query params from.
> + *
> + * @return A target object
> + */
> +exports.targetFromURL = Task.async(function *(url) {

Nit: function*

@@ +62,5 @@
> +      let response = yield client.getTab({ outerWindowID: id })
> +      form = response.tab;
> +    } catch(ex) {
> +      if (ex.error == "noTab") {
> +        throw new Error("targetFromURL, tab with outerWindowID:" + id+ " doesn't exists");

Nit: exist

@@ +88,5 @@
> +      }
> +      throw ex;
> +    }
> +  } else {
> +    throw new Error("targetFromURL, unsupported type='" + type + "' parameter");

Pick one messaging syntax, either

"id: " + id

or

"id='" + id + "'"

but not both in the same file.  Also, template strings probably helps here.

::: devtools/client/framework/toolbox-init.js
@@ +39,5 @@
> +    iframe.QueryInterface(Ci.nsIFrameLoaderOwner);
> +
> +    if (iframe) {
> +      // Fake a xul:tab object as we don't have one.
> +      // linkedBrowser is the only one attribute being queried by targetFromURL

No longer passed to targetFromURL

@@ +48,5 @@
> +        DebuggerServer.init();
> +        DebuggerServer.addBrowserActors();
> +      }
> +      let client = new DebuggerClient(DebuggerServer.connectPipe());
> +      Task.spawn(function *() {

Nit: function*
Attachment #8730692 - Flags: review?(jryans) → review+
Comment on attachment 8730693 [details] [diff] [review]
Remove support of tabId in getTab request.

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

Need more context, up to you if you want to move to a follow up.

I think it makes sense, but want to understand what was using this before.

::: devtools/shared/client/main.js
@@ -1613,5 @@
>  
>      if (aFilter) {
>        if (typeof(aFilter.outerWindowID) == "number") {
>          packet.outerWindowID = aFilter.outerWindowID;
> -      } else if (typeof(aFilter.tabId) == "number") {

Where is the code that used this feature of passing tabIds?
Attachment #8730693 - Flags: feedback+
Comment on attachment 8730691 [details] [diff] [review]
Support outerWindowID in getTab request for tabs in child processes.

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

::: devtools/server/actors/webbrowser.js
@@ +358,5 @@
>  };
>  
>  BrowserTabList.prototype.getTab = function({ outerWindowID, tabId }) {
>    if (typeof(outerWindowID) == "number") {
>      // Tabs in parent process

This comment seems incorrect, remove it?
Attachment #8730691 - Flags: review?(jryans) → review+
Blocks: 1257133
Attachment #8730690 - Attachment is obsolete: true
Attachment #8730691 - Attachment is obsolete: true
Attachment #8730692 - Attachment is obsolete: true
Attachment #8730693 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/8f18f3f516826c912bf675a494bd4a826dd9488b
Bug 1233463 - Ensure waiting for toolbox initialization in browser_cmd_csscoverage_startstop.js. r=jryans

https://hg.mozilla.org/integration/fx-team/rev/56d89c9653ccdf6c5f77892760fc5518bdd91ec7
Bug 1233463 - Expose about:devtools-toolbox shortcut to load the toolbox. r=jryans

https://hg.mozilla.org/integration/fx-team/rev/2e14be77e34126931a80b7f8a22355a7af67d4e1
Bug 1233463 - Support outerWindowID in getTab request for tabs in child processes. r=jryans

https://hg.mozilla.org/integration/fx-team/rev/360e21e14e09be0c9aa607a0d7d256b0f5a2bc5b
Bug 1233463 - Allow loading devtools in a tab or an html:iframe. r=jryans
Depends on: 1261272
Depends on: 1262144
Blocks: 1266128
Depends on: 1281731
Depends on: 1281726
Depends on: 1286082
Blocks: 1297758
Blocks: 1299503
Blocks: 1360196
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: