687854 - Move the Inspector code to browser/devtools

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Mihai Sucan [:msucan]]

We need to move the Inspector code to browser/devtools.

Comment 1

[Mihai Sucan [:msucan]]

Attachment: proposed patch

Proposed patch. Please let me know if anything needs to be changed.

Thanks!

Comment 2

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 561209 [details] [diff] [review]
proposed patch

in browser/devtools/highlighter/test/Makefile.in:

-DEPTH		= ../../../../..
+DEPTH     = ../../../..
 topsrcdir	= @top_srcdir@
 srcdir		= @srcdir@
 VPATH		= @srcdir@

DEPTH should be indented to match the surrounding lines.

otherwise good!

Comment 3

[Dão Gottwald [:dao]]

(In reply to Mihai Sucan [:msucan] from comment #0)
> We need to move the Inspector code to browser/devtools.

Please elaborate...

As long as inspector.js and highlighter.css are pulled directly into browser/base/, they need careful review from browser peers. Letting them live in browser/devtools/ seems to obfuscate this bond. Also note that browser/themes/winstripe/*/browser.css still styles some of this stuff.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 561209 [details] [diff] [review]
proposed patch

(In reply to Dão Gottwald [:dao] from comment #3)
> As long as inspector.js and highlighter.css are pulled directly into
> browser/base/, they need careful review from browser peers. Letting them
> live in browser/devtools/ seems to obfuscate this bond.

I agree. The modularization is happening in another bug, right?

Comment 5

[Mihai Sucan [:msucan]]

The inspector.js %include will be removed in bug 562168 - because we'll switch to a .jsm.

The highlighter.css will still stay %included in browser.css. Shall I not move it outside from browser/base/content? I can do that.

Comment 6

[Rob Campbell [:rc] (:robcee)]

(In reply to Dão Gottwald [:dao] from comment #3)
> (In reply to Mihai Sucan [:msucan] from comment #0)
> > We need to move the Inspector code to browser/devtools.
> 
> Please elaborate...
> 
> As long as inspector.js and highlighter.css are pulled directly into
> browser/base/, they need careful review from browser peers. Letting them
> live in browser/devtools/ seems to obfuscate this bond. Also note that
> browser/themes/winstripe/*/browser.css still styles some of this stuff.

This is kind of an artificial boundary. By transitive closure you could say that everything in gecko needs review by a browser peer.

As Mihai mentions, we're planning on converting inspector.js to a jsm in a subsequent patch. We wanted to do this as a stop-gap to get some extra dependent patches landed with only minimal rebasing first.

Leaving out highlighter.css from the move does seem like the right thing to do though. I wondered about that on my pass.

Comment 7

[Mihai Sucan [:msucan]]

Attachment: [in-fx-team] updated patch

Updated the patch to not move highlighter.css.

We keep inspector.js in /devtools temporarily, until we also get bug 562168 landed. The plan is for both patches to land together, iianm.

Thank you!

Comment 8

[Rob Campbell [:rc] (:robcee)]

Attachment: megapatch

this is a megapatch which includes the contents of bugs 681653, 663831, 650794, 672002.

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Just a quick heads up that we do have the following folders for devtools css:
browser/themes/*stripe/browser/devtools/

I was asked to create these early on in the development of the style inspector.

Comment 10

[Rob Campbell [:rc] (:robcee)]

(In reply to Michael Ratcliffe from comment #9)
> Just a quick heads up that we do have the following folders for devtools css:
> browser/themes/*stripe/browser/devtools/
> 
> I was asked to create these early on in the development of the style
> inspector.

yes. I'm still not sure about the reasoning behind that.

Comment 11

[Rob Campbell [:rc] (:robcee)]

Attachment: megapatch 2

cleaned up megapatch based on review comments from individual bugs.

registerupdate: Bug 681653 - Augment RegisterTools API in Highlighter to deregister tools
registerStyle: Bug 663831 - Style inspector should be controllable from the highlighter
disablehtml: Bug 650794 - Disable HTML panel and make it use registerTools API
dockabletreepanel: Bug 672002 - Move HTML panel to a docked panel in the main browser window

Comment 12

[Rob Campbell [:rc] (:robcee)]

Attachment: megapatch rebased on attachment 561225

rebased on top of the move inspector->devtools code. Updated MEGAPATCH!

Comment 13

[Mihai Sucan [:msucan]]

Attachment: megapatch + jsm from bug 562168

This is attachment 561461 [details] [diff] [review] + the patch from bug 562168, as requested by Gavin.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 561503 [details] [diff] [review]
megapatch + jsm from bug 562168

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+XPCOMUtils.defineLazyGetter(this, "INSPECTOR_NOTIFICATIONS", function() {

Could this not just be InspectorUI.INSPECTOR_NOTIFICATIONS ?

>diff --git a/browser/base/content/inspector.js b/browser/devtools/highlighter/inspector.jsm

>+var EXPORTED_SYMBOLS = ["INSPECTOR_NOTIFICATIONS", "InspectorUI",
>+                        "InspectorStore"];

InspectStore seems to only be used for a test, do we really need it? Can the test use InspectorUI.store?

>+function InspectorUI(aWindow)

>+  this.gBrowser = aWindow.gBrowser;

this.tabbrowser (gBrowser is a horrible name that shouldn't be propagated any further :)

>+  registerTool: function IUI_registerTool(aRegObj)

>+    let bindToolEvent = function(aWidget, aEvent, aCallback) {
>+      let toolEvent = aWidget.id + "_" + aEvent;
>+      this.toolEvents[toolEvent] = aCallback;
>+      aWidget.addEventListener(aEvent, aCallback, false);
>+    }.bind(this);

How about:
let toolEvents = this.toolEvents;
function bindToolEvent(widget, event, callback) {
  toolEvents[widget.id + "_" + event] = callback;
  widget.addEventListener(event, callback, false);
}

>+  unregisterTool: function IUI_unregisterTool(aRegObj)

>+    let unbindToolEvent = function(aWidget, aEvent) {

(could do the same here)

>   restoreToolState: function IUI_restoreToolState(aWinID)

>+  toolsSelect: function IUI_toolsSelect(aScroll)

These methods have closures that refer to a single member - rather than using "bind(this)" I would tend to prefer just accessing those properties by closure over a local variable.

>+  destroy: function IUI_destroy()
>+  {
>+    if (this.isInspectorOpen && !this.closing) {
>+      this.closeInspectorUI();

This !closing check seems redundant - closeInspectorUI already does that.

>diff --git a/browser/base/content/test/inspector/Makefile.in b/browser/devtools/highlighter/test/Makefile.in
>rename from browser/base/content/test/inspector/Makefile.in
>rename to browser/devtools/highlighter/test/Makefile.in

>+# 		browser_inspector_treePanel_click.js \

Mistake?

>diff --git a/browser/devtools/styleinspector/StyleInspector.jsm b/browser/devtools/styleinspector/StyleInspector.jsm

>+    panel.showTool = function SI_showTool(aSelection)
>+    {
>+      this.selectNode(aSelection);
>+      let win = Services.wm.getMostRecentWindow("navigator:browser");
>+      this.openPopup(win.gBrowser.selectedBrowser, "end_before", 0, 0,

This doesn't look right (getMostRecentWindow should basically be avoided at all costs). What patch is this from?

>diff --git a/browser/locales/en-US/chrome/browser/inspector.properties b/browser/locales/en-US/chrome/browser/inspector.properties

>+# LOCALIZATION NOTE (htmlPanel.label): A button label that appears on the
>+# InspectorUI's toolbar.

This not isn't particularly helpful to localizers - what element in the UI does "InspectorUI toolbar" correspond to? Localization notes need to be written more from a user point of view than a developer point of view. And beyond just explaining where it appears, you need to explain what it describes, e.g. "this is a label for a button that activates the Web Developer->Inspect UI's HTML Panel".

I did not review much of the /devtools/ code or tests, I trust that you will ensure those have been sufficiently reviewed (particularly TreePanel.jsm).

r=me, but that getMostRecentWindow thing really needs to get fixed - can followup in whatever bug that code is being added in.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Hrm, getMostRecentWindow seems to have creeped into a bunch of devtools code (e.g. StyleInspector.jsm). I guess that should be fixed in a followup then :(

Comment 16

[David Dahl :ddahl]

(In reply to Gavin Sharp from comment #15)
> Hrm, getMostRecentWindow seems to have creeped into a bunch of devtools code
> (e.g. StyleInspector.jsm). I guess that should be fixed in a followup then :(

Gavin: what is the best solution for replacing getMostRecentWindow?

Comment 17

[David Dahl :ddahl]

Gavin: can we change the underlying platform method so all of the callers do the right thing?

Comment 18

[Mihai Sucan [:msucan]]

(In reply to David Dahl :ddahl from comment #16)
> (In reply to Gavin Sharp from comment #15)
> > Hrm, getMostRecentWindow seems to have creeped into a bunch of devtools code
> > (e.g. StyleInspector.jsm). I guess that should be fixed in a followup then :(
> 
> Gavin: what is the best solution for replacing getMostRecentWindow?

The problem is with the StyleInspector code there.

Gavin is right about the problem he pointed out. That code must not use getMostRecentWindow(), and the Style Inspector does that a few times. It did that because it wasn't really a JSM, and the Inspector wasn't a JSM either.

The correct solution is to pass the InspectorUI object to the StyleInspector constructor and store a ref there, such that whenever the window object is needed, the StyleInspector can avoid the use of getMostRecentWindow(). These are mostly trivial changes, but they are best left for a follow up bug, I would say.

There's bug 685893 where I think this work best fits in. The Style Panel code has a very-much related problem with the way it creates an instance of itself. Part of the fix for bug 685093 would also be the use of InspectorUI instance and getting rid of getMostRecentWindow().

(an investigation is also needed for the case where there's no IUI instance, eg. when it opens from the HUDService)

Thank you Gavin for your review!

Comment 19

[:Gavin Sharp [email: [e-mail]]]

(In reply to David Dahl :ddahl from comment #17)
> Gavin: can we change the underlying platform method so all of the callers do
> the right thing?

It's not *how* it's doing it that's the problem, it's that it's doing the wrong thing to begin with (you shouldn't need to retrieve the "most recent window", because you're UI code, so you should know which window you're dealing with directly).

Fixing all of this in a followup sounds fine.

Comment 20

[Rob Campbell [:rc] (:robcee)]

(In reply to Gavin Sharp from comment #14)
> Comment on attachment 561503 [details] [diff] [review] [diff] [details] [review]
> megapatch + jsm from bug 562168
> 
> >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
> 
> >+XPCOMUtils.defineLazyGetter(this, "INSPECTOR_NOTIFICATIONS", function() {
> 
> Could this not just be InspectorUI.INSPECTOR_NOTIFICATIONS ?

done.

> >diff --git a/browser/base/content/inspector.js b/browser/devtools/highlighter/inspector.jsm
> 
> >+var EXPORTED_SYMBOLS = ["INSPECTOR_NOTIFICATIONS", "InspectorUI",
> >+                        "InspectorStore"];
> 
> InspectStore seems to only be used for a test, do we really need it? Can the
> test use InspectorUI.store?

done.

> >+function InspectorUI(aWindow)
> 
> >+  this.gBrowser = aWindow.gBrowser;
> 
> this.tabbrowser (gBrowser is a horrible name that shouldn't be propagated
> any further :)

much better! done.

> >+  registerTool: function IUI_registerTool(aRegObj)
> 
> >+    let bindToolEvent = function(aWidget, aEvent, aCallback) {
> >+      let toolEvent = aWidget.id + "_" + aEvent;
> >+      this.toolEvents[toolEvent] = aCallback;
> >+      aWidget.addEventListener(aEvent, aCallback, false);
> >+    }.bind(this);
> 
> How about:
> let toolEvents = this.toolEvents;
> function bindToolEvent(widget, event, callback) {
>   toolEvents[widget.id + "_" + event] = callback;
>   widget.addEventListener(event, callback, false);
> }

Yes!

> >+  unregisterTool: function IUI_unregisterTool(aRegObj)
> 
> >+    let unbindToolEvent = function(aWidget, aEvent) {
> 
> (could do the same here)

Did.

> >   restoreToolState: function IUI_restoreToolState(aWinID)
> 
> >+  toolsSelect: function IUI_toolsSelect(aScroll)
> 
> These methods have closures that refer to a single member - rather than
> using "bind(this)" I would tend to prefer just accessing those properties by
> closure over a local variable.

I had trouble getting this to work with restoreToolState. Can try again, but I expect it's because the methods called therein refer to "this" in the callee.

> >+  destroy: function IUI_destroy()
> >+  {
> >+    if (this.isInspectorOpen && !this.closing) {
> >+      this.closeInspectorUI();
> 
> This !closing check seems redundant - closeInspectorUI already does that.

Removed! (I think this crept back in along the way)

> >diff --git a/browser/base/content/test/inspector/Makefile.in b/browser/devtools/highlighter/test/Makefile.in
> >rename from browser/base/content/test/inspector/Makefile.in
> >rename to browser/devtools/highlighter/test/Makefile.in
> 
> >+# 		browser_inspector_treePanel_click.js \
> 
> Mistake?

No, intentional (and predates these patches). Added a comment. While file a followup to investigate and reactivate it.

> >diff --git a/browser/devtools/styleinspector/StyleInspector.jsm b/browser/devtools/styleinspector/StyleInspector.jsm
> 
> >+    panel.showTool = function SI_showTool(aSelection)
> >+    {
> >+      this.selectNode(aSelection);
> >+      let win = Services.wm.getMostRecentWindow("navigator:browser");
> >+      this.openPopup(win.gBrowser.selectedBrowser, "end_before", 0, 0,
> 
> This doesn't look right (getMostRecentWindow should basically be avoided at
> all costs). What patch is this from?

StyleInspector stuff. It's been filed.

> >diff --git a/browser/locales/en-US/chrome/browser/inspector.properties b/browser/locales/en-US/chrome/browser/inspector.properties
> 
> >+# LOCALIZATION NOTE (htmlPanel.label): A button label that appears on the
> >+# InspectorUI's toolbar.
> 
> This not isn't particularly helpful to localizers - what element in the UI
> does "InspectorUI toolbar" correspond to? Localization notes need to be
> written more from a user point of view than a developer point of view. And
> beyond just explaining where it appears, you need to explain what it
> describes, e.g. "this is a label for a button that activates the Web
> Developer->Inspect UI's HTML Panel".

Fair enough. Updated the comment.

> I did not review much of the /devtools/ code or tests, I trust that you will
> ensure those have been sufficiently reviewed (particularly TreePanel.jsm).

Mihai and I have been well over TreePanel. Feel free to drop in with drivebys!

> r=me, but that getMostRecentWindow thing really needs to get fixed - can
> followup in whatever bug that code is being added in.

Yep.

Comment 21

[Rob Campbell [:rc] (:robcee)]

Attachment: [in-fx-team] inspector.jsm + review followup

Comment 22

[Mihai Sucan [:msucan]]

(In reply to Mihai Sucan [:msucan] from comment #18)
> itself. Part of the fix for bug 685093 would also be the use of InspectorUI

s/685093/685893/

(sorry for the typo!)

Comment 23

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 561225 [details] [diff] [review]
[in-fx-team] updated patch

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

Comment 24

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 562113 [details] [diff] [review]
[in-fx-team] inspector.jsm + review followup

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

Comment 25

[Rob Campbell [:rc] (:robcee)]

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

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

Comment 26

[Dão Gottwald [:dao]]

(In reply to Rob Campbell [:rc] (robcee) from comment #25)
> https://hg.mozilla.org/mozilla-central/rev/d9989de45bd9

Backed out because of various new mochitest-browser-chrome leaks. Unfortunately, this landed with a bunch of other interweaved patches. I only felt comfortable ruling out f297a626dd13 and 7d5311c92e04.

Comment 27

[Rob Campbell [:rc] (:robcee)]

relanded:

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

Comment 28

[Rob Campbell [:rc] (:robcee)]

Attachment: [in-fx-team] move inspector followup

follow up patch for an incorrectly referenced notification name in highlighter.

Comment 29

[Mihai Sucan [:msucan]]

Comment on attachment 562498 [details] [diff] [review]
[in-fx-team] move inspector followup

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

Thanks for the fix!

Comment 30

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 562498 [details] [diff] [review]
[in-fx-team] move inspector followup

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

Comment 31

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/34f444294220

Comment 32

[Tim Taubert [:ttaubert] (inactive)]

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