Closed Bug 1297758 Opened 8 years ago Closed 8 years ago

Allow loading the inspector individually in a tab

Categories

(DevTools :: Framework, enhancement, P1)

Product:
DevTools
Component:
Framework
Type:
enhancement

Tracking

(firefox52 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 52
Iteration:
52.1 - Oct 3
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

(Whiteboard: [devtools-html])

Attachments

(5 files, 4 obsolete files)

Bug 1297758 - Make the inspector self-loadable.
58 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1297758 - Fetch target and fronts from inspector getter rather than toolbox.
58 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1297758 - Rename inspector-panel.js to inspector.js.
58 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1297758 - Throw when accessing any unimplemented toolbox function when loading inspector in a tab.
58 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1297758 - Fix race in inspector initialization to prevent this._splitter is undefined exception.
58 bytes, text/x-review-board-request
bgrins
: review+
Details 
Bug 1233463 allowed loading devtools in a tab.
Now, it would be handy to be able to load just one tool in tab.
It basically means getting rid of the toolbox dependency when loading a tool, but also figuring out how to precise which target a given tool must debug.
We already figured out a way to define a target via query parameters in bug 1233463. So it should mostly be about allowing a tool to setup itself on load without expect a chrome call to create the XxxxPanel object and call its open() method.

Now, why would we do such thing?
It should allow us to incrementally work on making just one given tool to work in a regular tab.
Here, the goal is not to make the tool work with just web privileges, the tools are still going to be loaded via chrome URL and privileges. But it will then be easy to stop doing that and see what uses/requires chrome.
Attached patch patch v1 (obsolete) — — Splinter Review 
Here is a first step against the inspector, console and debugger.
It is very similar to the way about:devtools-toolbox work, if not identical!

This patch is fully functional, tools work fine as soon as you don't try to use a feature that relates the the toolbox.
But I don't consider it landable as I use a mock for the Toolbox object used by the tools.
It is incomplete and mostly unimplemented.
I would like to look, tool per tool what is the precise usage of the toolbox object
and act accordingly. Either provide a better mock, complete and functional,
or make it so that tools no longer depend on the toolbox object itself but something else
that can be instanciated from the tools.
With this patch you can load tools directly from a tab by opening such url:
  chrome://devtools/content/webconsole/webconsole.xul?type=tab&id=20
  chrome://devtools/content/debugger/debugger.xul?type=tab&id=20
  chrome://devtools/content/inspector/inspector.xul?type=tab&id=4294967297

You would have to use about:debutting tabs panel to figure out the tab ids to pass in the url...
Severity: normal → enhancement
Priority: -- → P3
Please acknowledge this work to prevent colliding.
I just saw your discussion with :jryans over #devtools.
It looks like you are trying to do part of what I do here, what I've been doing and plan to do.

@bgrins | jlongster: what I need is some way to get access to a target when loading code in a tab
@bgrins | what I want to do is be able to instantiate an InspectorPanel object by passing in just a target (will need some changes to work around not having a toolbox instance, but that's fine)

See the inspector-init.js file I'm introducing. That does exactly that. May be I do use some chrome privileged things that gets in your way, but that's not a goal for me to do that. If there is anything we can modify to run better in a content environement, I'm all up for that.
This patch is very conservative as it doesn't get into the regular codepath,
if that help I can get them landed sooner than later.

Also, please reuse about:debugging and especially the same query parameters we use for about:devtools-toolbox So that we don't end up with various convention on how to define a given target.
I'm all open to tweak or modify target-from-url to accomodate your needs!

My long term plan, which will happen in followups to this bug would be to use this new codepath in production. Make it so that the toolbox document just load iframes with a set of query parameters and each tool will set up by itself. I'm also trying to do that for the toolbox in bug 1299501.

If you think a vidyo session would help, do not hesitate to send me an invite.
Flags: needinfo?(bgrinstead)
Note that in comment 2 I mention chrome:// URL, but that's not a limitation of my patch. It would work with any URL the tools are loaded with. If you host them on http, the *-init.js files are also going to fire.
(In reply to Alexandre Poirot [:ochameau] from comment #3)
> Please acknowledge this work to prevent colliding.
> I just saw your discussion with :jryans over #devtools.
> It looks like you are trying to do part of what I do here, what I've been
> doing and plan to do.
> 
> @bgrins | jlongster: what I need is some way to get access to a target when
> loading code in a tab

That's basically what I need also for Bug 1291049.  See the next comment.

> @bgrins | what I want to do is be able to instantiate an InspectorPanel
> object by passing in just a target (will need some changes to work around
> not having a toolbox instance, but that's fine)
> 
> See the inspector-init.js file I'm introducing. That does exactly that. May
> be I do use some chrome privileged things that gets in your way, but that's
> not a goal for me to do that. If there is anything we can modify to run
> better in a content environement, I'm all up for that.
> This patch is very conservative as it doesn't get into the regular codepath,
> if that help I can get them landed sooner than later.

That sounds great! I was going to throw something locally in place to allow instantiating an InspectorPanel without a toolbox just so we could get a checkpoint of how much more de-chrome there is.  But if you are able to land something that changes the InspectorPanel to not require a toolbox that's even better.  Although I'm not sure how to handle cases where you actually need to toolbox to talk with other tools or do things like start the element picker.

> Also, please reuse about:debugging and especially the same query parameters
> we use for about:devtools-toolbox So that we don't end up with various
> convention on how to define a given target.
> I'm all open to tweak or modify target-from-url to accomodate your needs!
> 
> My long term plan, which will happen in followups to this bug would be to
> use this new codepath in production. Make it so that the toolbox document
> just load iframes with a set of query parameters and each tool will set up
> by itself. I'm also trying to do that for the toolbox in bug 1299501.

I think both things serve different purposes.  Medium term (once we are content-priv compatible in inspector) I'd like if we can load tools over HTTP for local development.  It makes it easier to get development efficiencies from community tooling (things like dev middleware / hot reloading).  But we'll need about:devtools-toolbox for actual deployment, like when opening toolboxes from about:debugging (we *want* chrome priv when running within Firefox for things like prefs). 

I think it makes sense to use a shared way to represent the target on a query string.  The URL scheme that debugger.html is using right now looks like: http://localhost:8000/?firefox-tab=child6/tab1.
Flags: needinfo?(bgrinstead)
Just focus on the inspector for now.
I'm just hesitant about this "fake toolbox" object.
May be we should instead remove usage of toolbox object within the inspector,
and go through a helper module or something?

On the long run, it would be great to communicate with the toolbox via DOM messages
and no longer call JS between the tool document and the toolbox one.
Brian, I would like to move forward with that, but want to ensure you will r+ such patch.
This patch is conservative, it is the same thing than I made for loading toolboxes in a tab.
https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/toolbox-init.js
It only kicks in when using query parameters, which we doesn't do in production codepath.
The only difference with the toolbox usecase is that for the tools, I craft a fake toolbox object.
Do you think that's an acceptable solution for a first iteration?

If yes, my next iteration would be to try get rid of toolbox usage
and instead use postMessage for any action to be triggered between
the toolbox and the tools.
Otherwise, for things like React, RDP clients and all JS dependencies we want to share across tools,
it would be more elegant to just duplicate them in each tools,
so that tools are completely independant documents 100% self loadable without any magic.
But it may be more reasonable to keep this shared in production codepath,
in which case, we have to keep some chrome magic and a "fake toolbox"
could be that magic salt that comes from the chrome.

To be more concrete, it would mean that inspector-init.js would craft
the fake toolbox object only when running in a tab,
otherwise, in production code, it would pull it via some chrome magic.

Finally, I imagine to make that work with content privileges,
you would mostly have to start replacing the chrome loader:
  const Cu = Components.utils;
  const { require } = Cu.import("resource://devtools/shared/Loader.jsm", {});
from inspector-init.js, with one working without Cu.import.
Looks like, we could just check if require() is already available
and only try to fetch it from Cu.import is not already available...

I'll also ask Patrick feedback when he returns from viewsource.
Attachment #8791210 - Flags: feedback?(bgrinstead)
Attachment #8784590 - Attachment is obsolete: true
Attachment #8790989 - Attachment is obsolete: true
(In reply to Alexandre Poirot [:ochameau] from comment #6)
> Created attachment 8790989 [details] [diff] [review]
> Make the inspector self-loadable - v1
> 
> Just focus on the inspector for now.
> I'm just hesitant about this "fake toolbox" object.
> May be we should instead remove usage of toolbox object within the inspector,
> and go through a helper module or something?
> 
> On the long run, it would be great to communicate with the toolbox via DOM
> messages
> and no longer call JS between the tool document and the toolbox one.

I think less reliance on the toolbox object from tools is best (i.e. at least storing things that are dynamically fetched from the toolbox on the InspectorPanel when it's initialized so we don't always have to reach up when fetching like https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/inspector-panel.js#123-133). 

We should also be more careful to not do toolbox.* in this file in places where we can instead to this.* just to be easier to track things.  In fact it'd be nice if we never stored the toolbox object at all on the InspectorPanel object, but that might take some more time to get to that point.

That said, I don't think that there's a way to completely remove access from things like the inspector/walker/selection fronts that make sense to be initialized globally.  Even with DOM messages we still need to construct the fronts and inject that functionality in a similar way that a 'fake toolbox' does, and I'm not sure that it will make things more clear.
But maybe we could attach some of these things (inspector, walker, selection) directly onto the target
Comment on attachment 8791210 [details] [diff] [review]
Make the inspector self-loadable - v1

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

I'd like to consider doing something similar to what the debugger and some other tools are doing, which is loading the current inspector-panel.js as a script on the page but pulling the 'panel' bits out into a new panel.js file.  This would also let us stick to having to bundle only a single inspector js file for loading in a tab.

So:
1) inspector-panel.js is renamed to inspector.js and InspectorPanel function is renamed to Inspector
** We don't set any exports, instead just allowing the main function in that file to be exposed onto the frame's window by loading it as a <script> tag at the bottom
** Some of the panel boilerplate goes away from that object and we have an init() call that will be called by the Panel object in step 2.
** This file can be bundled with webpack for content-priv loading
** The stuff you have in inspector-init (at least whatever we still need if we can make some of the changes discussed in recent comments) could go into this file as well.  I'm generally OK with most of this stuff and I think it'll translate fairly well to a content-priv bundle.  The benefit to putting it into the inspector.js file is that we don't need to bundle multiple files when loading in content - let's see if Patrick has an opinion about this being in the same file or different.

2) The "panel" parts are pulled into a much smaller panel.js file that is loaded by definitions, similar to https://dxr.mozilla.org/mozilla-central/source/devtools/client/debugger/new/panel.js.
** Ultimately in open() the panel digs into the window to grab Inspector from 1 and call init() on it with the target
** When loading in a tab none of this code runs

What do you think?
Attachment #8791210 - Flags: feedback?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #9)
> But maybe we could attach some of these things (inspector, walker,
> selection) directly onto the target

I opened bug 1222047 a long time ago about that, may be that's a good time to do it.

(In reply to Brian Grinstead [:bgrins] from comment #10)
> Comment on attachment 8791210 [details] [diff] [review]
>
> What do you think?

Sounds good to me, it works fine with what I have in mind. So if that help you, let's go with that.
But just to be clear about my long term goal. I would like to get rid of the panel.js/Panel.open pattern and instead always let the tools load by themself using the codepath I'm introducing in inspector-init.
Comment on attachment 8791210 [details] [diff] [review]
Make the inspector self-loadable - v1

Scroll back up to comment 7.
Attachment #8791210 - Flags: feedback?(pbrosset)
(In reply to Alexandre Poirot [:ochameau] from comment #11)
> But just to be clear about my long term goal. I would like to get rid of the
> panel.js/Panel.open pattern and instead always let the tools load by
> themself using the codepath I'm introducing in inspector-init.

I thought this was just for development - if we want to use this when hosted in the toolbox then here's a few things:
* We will end up re-requiring React and ReactDOM for each frame and we'd need some perf measurements there
* hostType will be incorrect when hosted in a real toolbox (not sure it's used / needed in the inspector though)
* There's a call to toolbox.getNotificationBox() that needs a real thing to be returned (see updateDebuggerPausedWarning).

What's the benefit we'd get to having this be a production code path?
(In reply to Brian Grinstead [:bgrins] from comment #13)
> I thought this was just for development - if we want to use this when hosted
> in the toolbox then here's a few things:
> * We will end up re-requiring React and ReactDOM for each frame and we'd
> need some perf measurements there

Just a note after talking with James -- there's a good chance we actually *should* be pulling in a new copy of React/ReactDOM anyway for correctness reasons around react doing global references to window and document.  This is something that would be updated separately from this patch, although it would simplify this patch by lowering the number of things needed from the 'fake' toolbox.
(In reply to Brian Grinstead [:bgrins] from comment #13)
> (In reply to Alexandre Poirot [:ochameau] from comment #11)
> > But just to be clear about my long term goal. I would like to get rid of the
> > panel.js/Panel.open pattern and instead always let the tools load by
> > themself using the codepath I'm introducing in inspector-init.
> 
> I thought this was just for development - if we want to use this when hosted
> in the toolbox then here's a few things:
> * We will end up re-requiring React and ReactDOM for each frame and we'd
> need some perf measurements there

I share this concern but also it feels like every website do that, each website load its own copy again and again... I heard the js team working on a shared bytecode cache, I don't know where they are at with that.

> * hostType will be incorrect when hosted in a real toolbox (not sure it's
> used / needed in the inspector though)

I'm managing the host type throught DOM messages between the toolbox and the chrome, see bug 1266134.
So that's something we can keep updated.

> * There's a call to toolbox.getNotificationBox() that needs a real thing to
> be returned (see updateDebuggerPausedWarning).

Same thing here, we could send DOM messages.

> 
> What's the benefit we'd get to having this be a production code path?

The main benefit is about having no difference between loaded in a tab and loaded in the toolbox.
If developing tools in a tab becomes a thing, we will surely end up with slightly different behaviors and bugs between the two modes.

Using direct references of object coming from the chrome (target object) or the toolbox document (toolbox) make strong dependencies between all codebases. Having each component: the chrome, the toolbox and the tools being self loadable without anything would make them more independant and prevent having to patch them all at once.
Also when it comes to memory and leaks, it is much better when we can use documents as independant containers we can kill. When we remove an iframe from DOM, we are quite confident that all objects created by the iframe document are freed. As soon as we start sharing JS between compartments, issues arise.

Note that's a long term idea. My immediate goal is to keep that for development and slowly clean up things to remove all possible dependencies between tools, toolbox and chrome. Your suggestion to keep toolbox reference hidden in the initialization code is one example.
See Also: → 1291049
Blocks: devtools-html-phase2
Status: NEW → ASSIGNED
Iteration: --- → 51.3 - Sep 19
Flags: qe-verify?
Priority: P3 → P1
Whiteboard: [devtools-html]
Brian, here is I think something that looks like what you described in comment 10.
Comment on attachment 8791679 [details] [diff] [review]
Make the inspector self-loadable - alt-patch v1

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

Thanks, this looks pretty much like what I was thinking and we'd later be able to get this running as a bundle in content by defining 'require' in an inline script tag on the inspector frame (only when running in chrome) instead of inside inspector.js. Can we start working on some of the pieces that will make the 'fake toolbox' easier / smaller (esp Bug 1222047, which will probably remove most of the complexity from it)?
Comment on attachment 8791210 [details] [diff] [review]
Make the inspector self-loadable - v1

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

The inspector, walker, selection and highlighter were moved to the toolbox 3 years ago (bug 916443 I think). The primary reason was to make it possible for other tools to get information from the DOM and highlight elements.

Right now, on top of the inspector-panel the following use these things: the style-editor, the variables-view and the console.
Now, I'm thinking, is there any sense for these panels to access the highlighter or walker if the inspector-panel does not exist? No. If there's a DOM tree in the target and nodes to be highlighted, then there will be an inspector-panel too. So these other panels could ask the inspector-panel to walk the DOM or highlight elements, not the toolbox.
It's really not the toolbox's job to do this.

So, while we did this move to make our lives easier back then, I guess the real solution should have been to leave these things in the inspector-panel and instead have some kind of eventing system to do the same thing.

In fact, things like the console and the debugger shouldn't have to care about what function to call to highlight a node if we had just one shareable DOM node renderer component that both the inspector and other panels would use when needed.

Anyway, all this to say that we should be able to make the fake toolbox simpler this way, because I agree that tools should be completely independent documents 100% self loadable without any magic :)
I also like Brian's comment 10 which should make the inspector more like other panels. The fact that either the toolbox calls an open function from a panel.js file or that the panel auto-loads itself from query parameters seems minor enough at this stage that you should be able to go to one or the other solution easily.
Attachment #8791210 - Flags: feedback?(pbrosset) → feedback+
I already made a patch to remove some usages of toolbox.* within inspector codebase.
Are you ok with landing the fake toolbox in order to followup on bug 1222047?

This patch already allows loading the inspector individually with chrome privileges with url like this: chrome://devtools/content/inspector/inspector.xul?type=tab&id=4294967297
Then feel free to tweak this patch to accomodate loading with content privileges.
(In reply to Patrick Brosset <:pbro> from comment #19)
> Comment on attachment 8791210 [details] [diff] [review]
> Make the inspector self-loadable - v1
> 
> Review of attachment 8791210 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The inspector, walker, selection and highlighter were moved to the toolbox 3
> years ago (bug 916443 I think). The primary reason was to make it possible
> for other tools to get information from the DOM and highlight elements.
> 
> Right now, on top of the inspector-panel the following use these things: the
> style-editor, the variables-view and the console.
> Now, I'm thinking, is there any sense for these panels to access the
> highlighter or walker if the inspector-panel does not exist? No. If there's
> a DOM tree in the target and nodes to be highlighted, then there will be an
> inspector-panel too. So these other panels could ask the inspector-panel to
> walk the DOM or highlight elements, not the toolbox.
> It's really not the toolbox's job to do this.
>
> So, while we did this move to make our lives easier back then, I guess the
> real solution should have been to leave these things in the inspector-panel
> and instead have some kind of eventing system to do the same thing.
> 
> In fact, things like the console and the debugger shouldn't have to care
> about what function to call to highlight a node if we had just one shareable
> DOM node renderer component that both the inspector and other panels would
> use when needed.

I think there was also a perf component to the current design.  That is, when you hover a DOM node in the console for the first time you don't need to open the inspector frontend and wait for it to load before doing anything but you instead just need to create necessary fronts (done at toolbox level).  That's one reason I like the idea of attaching the relevant fronts onto the target/client (i.e. bug 1222047) instead of messaging with the inspector frontend for things like this.  We should discuss what approach we want to take before starting to work on bug 1222047 though.
Comment on attachment 8792532 [details]
Bug 1297758 - Fetch target and fronts from inspector getter rather than toolbox.

https://reviewboard.mozilla.org/r/79542/#review78624

Typo in commit message: 'rathen' -> 'rather'
Attachment #8792532 - Flags: review?(bgrinstead) → review+
Comment on attachment 8792531 [details]
Bug 1297758 - Make the inspector self-loadable.

https://reviewboard.mozilla.org/r/79540/#review78626

::: devtools/client/inspector/inspector.js:1
(Diff revision 1)
> +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */

inspector.js should be created as a 'move' of inspector-panel.js so that history on the file is preserved.  This'll also make it easier to review what changed here

::: devtools/client/inspector/panel.js:11
(Diff revision 1)
> +
> +function InspectorPanel(iframeWindow, toolbox) {
> +  this._inspector = new iframeWindow.Inspector(toolbox);
> +}
> +InspectorPanel.prototype = {
> +  open() {

I believe the isReady and target properties need to also live on the panel instance (see browser_inspector_initialization.js)
Attachment #8792531 - Flags: review?(bgrinstead)
Iteration: 51.3 - Sep 19 → 52.1 - Oct 3
(In reply to Brian Grinstead [:bgrins] from comment #25)
> Comment on attachment 8792531 [details]
> Bug 1297758 - Make the inspector self-loadable.
> 
> https://reviewboard.mozilla.org/r/79540/#review78626
> 
> ::: devtools/client/inspector/inspector.js:1
> (Diff revision 1)
> > +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
> 
> inspector.js should be created as a 'move' of inspector-panel.js so that
> history on the file is preserved.  This'll also make it easier to review
> what changed here

Hum. I did that, but it looks like git or mozreview fails in displaying that correctly, I'll try to add an intermediate commit.

> ::: devtools/client/inspector/panel.js:11
> (Diff revision 1)
> > +
> > +function InspectorPanel(iframeWindow, toolbox) {
> > +  this._inspector = new iframeWindow.Inspector(toolbox);
> > +}
> > +InspectorPanel.prototype = {
> > +  open() {
> 
> I believe the isReady and target properties need to also live on the panel
> instance (see browser_inspector_initialization.js)

Only toolbox.js manipulate the InspectorPanel instance. At the end the inspector object everyone uses is the Inspector() instance returned by panel.open call.

https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/toolbox.js#1287
toolbox call InspectorPanel.open
https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/toolbox.js#1296
and stores the promise resolution a panel value
https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/inspector-panel.js#246
InspectorPanel.open resolves to InspectorPanel() instance
Comment on attachment 8792531 [details]
Bug 1297758 - Make the inspector self-loadable.

https://reviewboard.mozilla.org/r/79540/#review78754
It seems to be better with a dedicated commit for the rename...
but note that I had to rebase so the interdiff are quite messy on mozreview.
Just pushed fixes for eslint.
FYI this will need a rebase after Bug 1262443
Comment on attachment 8792531 [details]
Bug 1297758 - Make the inspector self-loadable.

https://reviewboard.mozilla.org/r/79540/#review79132

This seems generally OK to me but it'll need a rebase.  I don't love the amount of code needed to generate the fake toolbox but it's a good start and we can iterate on it, especially as we pull some of the fronts out of the toolbox.

::: devtools/client/inspector/inspector-panel.js:11
(Diff revision 3)
>  
> +/* global window */
> +
>  "use strict";
>  
> +var Cu = Components.utils;

To compile this in content we're going to need to move this and the BrowserLoader/browserRequire definitions out into a script tag on the page (that can conditionally include them only when loaded in chrome).  I could handle this in Bug 1291049
Attachment #8792531 - Flags: review?(bgrinstead) → review+
Comment on attachment 8793262 [details]
Bug 1297758 - Rename inspector-panel.js to inspector.js.

https://reviewboard.mozilla.org/r/80058/#review79136

I want to be extra sure that we don't overwrite history on this file.  Would it be possible to break out this rename entirely into a separate commit (so that the jar.mn reference is correct in each individual commit and the script tag referenced is correctly as well)?  So, the first commit references <script src="inspector-panel.js"> as a script tag then this:
1. one renames the file
2. modifies jar.mn
3. changes <script src="inspector-panel.js"> to <script src="inspector.js">
4. commit message reflects what it's doing (just renaming the file)
Attachment #8793262 - Flags: review?(bgrinstead) → review+
Flags: qe-verify? → qe-verify-
While working on bug 733880 I realized there was one more toolbox thing that the inspector used (and I don't think you've taken into account in the fake toolbox):

http://searchfox.org/mozilla-central/search?q=textboxContextMenuPopup&path=devtools

Basically, the toolbox has a context menu with selectAll/Copy/Cut/Paste/Undo items that is used when you right-click in a text field. The idea was, I think, that instead of having each tool redo its own menu, the toolbox would have a shared one and then panels would just open it when required.
(In reply to Patrick Brosset <:pbro> from comment #39)
> While working on bug 733880 I realized there was one more toolbox thing that
> the inspector used (and I don't think you've taken into account in the fake
> toolbox):
> 
> http://searchfox.org/mozilla-central/
> search?q=textboxContextMenuPopup&path=devtools
> 
> Basically, the toolbox has a context menu with selectAll/Copy/Cut/Paste/Undo
> items that is used when you right-click in a text field. The idea was, I
> think, that instead of having each tool redo its own menu, the toolbox would
> have a shared one and then panels would just open it when required.

I imagine it has to be done from the toolbox document? (i.e. create the popup via the toolbox document)
otherwise the solution is simple, just put an helper in devtools/client/shared and share a module instead of sharing a JS object instance on the toolbox.
Patrick, I went through all usages of toolbox.* from devtools/client/inspector/ (I may miss some from shared folder) and explicitely throw when accessed.
So that it is clearer if something fails when used in a tab. I don't expect to hold this patch from making everything to work in a tab!
(In reply to Alexandre Poirot [:ochameau] from comment #40)
> I imagine it has to be done from the toolbox document? (i.e. create the
> popup via the toolbox document)
> otherwise the solution is simple, just put an helper in
> devtools/client/shared and share a module instead of sharing a JS object
> instance on the toolbox.
Right now this menu lives in toolbox.xul, which is why we have this dependency. If we refactored this menu to be based on our new Menu/MenuItem API, then yes, we could move this to a shared module instead of having to access the toolbox.
Having said this, I think you're approach to just throw when we access something from toolbox that isn't implemented is good. When this lands, there's probably going to be a lot of things that don't work in the inspector when loaded standalone, and this menu is not the highest priority of them anyway.

In fact, the right solution might be to just do nothing here, since when the inspector is in a content tab, then right-clicking in an input field is going to show the browser default context menu for input fields, which is exactly what we need here.
Comment on attachment 8794815 [details]
Bug 1297758 - Throw when accessing any unimplemented toolbox function when loading inspector in a tab.

https://reviewboard.mozilla.org/r/81118/#review79942

::: devtools/client/inspector/inspector.js:1835
(Diff revision 1)
>  
>    Task.spawn(function* () {
>      let target = yield targetFromURL(url);
>  
> +    let notImplemented = function () {
> +      throw new Error("Not implemented in a tab");

nit: Maybe rephrase to "Not implemented when the inspector is in a tab".
Attachment #8794815 - Flags: review?(pbrosset) → review+
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1ac36ff4b601
Make the inspector self-loadable. r=bgrins
https://hg.mozilla.org/integration/autoland/rev/fee0da956618
Rename inspector-panel.js to inspector.js. r=bgrins
https://hg.mozilla.org/integration/autoland/rev/80bc957e1052
Fetch target and fronts from inspector getter rather than toolbox. r=bgrins
https://hg.mozilla.org/integration/autoland/rev/95ca02cec38d
Throw when accessing any unimplemented toolbox function when loading inspector in a tab. r=pbro
Comment on attachment 8795709 [details]
Bug 1297758 - Fix race in inspector initialization to prevent this._splitter is undefined exception.

https://reviewboard.mozilla.org/r/81674/#review80312

This is fine, but does it mean that teardownSplitter isn't being called (and the event listener removed)?  That would be a bigger issue
Attachment #8795709 - Flags: review?(bgrinstead) → review+
Summary: Allow loading tools individualy in a tab → Allow loading tools individually in a tab
(In reply to Brian Grinstead [:bgrins] from comment #57)
> Comment on attachment 8795709 [details]
> Bug 1297758 - Fix race in inspector initialization to prevent this._splitter
> is undefined exception.
> 
> https://reviewboard.mozilla.org/r/81674/#review80312
> 
> This is fine, but does it mean that teardownSplitter isn't being called (and
> the event listener removed)?  That would be a bigger issue

I just supposed `resize` event was dispatched before _splitter variable was set. I imagine we would still have exceptions if the teardown method wasn't called.
Summary: Allow loading tools individually in a tab → Allow loading the inspector individually in a tab
Attachment #8791210 - Attachment is obsolete: true
Attachment #8791679 - Attachment is obsolete: true
Looks like autoland doesn't like changeset that has been backed out, repushing after a rebase.
https://hg.mozilla.org/integration/fx-team/rev/7629395388e5e7f2312a4532779853f99288dedb
Bug 1297758 - Make the inspector self-loadable. r=bgrins

https://hg.mozilla.org/integration/fx-team/rev/c6f1c03183be31f362c030ef65c118cc3125dd57
Bug 1297758 - Rename inspector-panel.js to inspector.js. r=bgrins

https://hg.mozilla.org/integration/fx-team/rev/b56b8a65ee11ca84b1b97bb54e18a5cc06496497
Bug 1297758 - Fetch target and fronts from inspector getter rather than toolbox. r=bgrins

https://hg.mozilla.org/integration/fx-team/rev/1f0baee81813ef058a7d90e059c0b8de9f4c8919
Bug 1297758 - Throw when accessing any unimplemented toolbox function when loading inspector in a tab. r=pbro

https://hg.mozilla.org/integration/fx-team/rev/1723fbc9d9d88a5fbf1c57cdd1e226e36f04c522
Bug 1297758 - Fix race in inspector initialization to prevent this._splitter is undefined exception. r=bgrins
I had to push that manually, I don't get why autoland failed even after a rebase.
See Also: → 1307765
See Also: → 1307860
See Also: → 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: