The default bug view has changed. See this FAQ.

Move the Inspector code to browser/devtools

RESOLVED FIXED in Firefox 9

Status

()

Firefox
Developer Tools
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: msucan, Assigned: msucan)

Tracking

Trunk
Firefox 9
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 3 obsolete attachments)

(Assignee)

Description

6 years ago
We need to move the Inspector code to browser/devtools.
(Assignee)

Comment 1

6 years ago
Created attachment 561209 [details] [diff] [review]
proposed patch

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

Thanks!
Assignee: nobody → mihai.sucan
Status: NEW → ASSIGNED
Attachment #561209 - Flags: review?(rcampbell)
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!
Attachment #561209 - Flags: review?(rcampbell) → review+
Attachment #561209 - Flags: review?(gavin.sharp)
(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 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?
Attachment #561209 - Flags: review?(gavin.sharp) → review-
(Assignee)

Comment 5

6 years ago
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.
(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.
(Assignee)

Comment 7

6 years ago
Created attachment 561225 [details] [diff] [review]
[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!
Attachment #561209 - Attachment is obsolete: true
Attachment #561225 - Flags: review?(gavin.sharp)
Created attachment 561298 [details] [diff] [review]
megapatch

this is a megapatch which includes the contents of bugs 681653, 663831, 650794, 672002.
Attachment #561298 - Flags: review?(gavin.sharp)
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.
(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.
Created attachment 561449 [details] [diff] [review]
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
Attachment #561298 - Attachment is obsolete: true
Attachment #561298 - Flags: review?(gavin.sharp)
Attachment #561449 - Flags: review?(gavin.sharp)
Created attachment 561461 [details] [diff] [review]
megapatch rebased on attachment 561225 [details] [diff] [review]

rebased on top of the move inspector->devtools code. Updated MEGAPATCH!
Attachment #561461 - Flags: review?(gavin.sharp)
(Assignee)

Updated

6 years ago
Blocks: 562168
(Assignee)

Comment 13

6 years ago
Created attachment 561503 [details] [diff] [review]
megapatch + jsm from bug 562168

This is attachment 561461 [details] [diff] [review] + the patch from bug 562168, as requested by Gavin.
Attachment #561503 - Flags: review?(gavin.sharp)
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.
Attachment #561503 - Flags: review?(gavin.sharp) → review+
Attachment #561461 - Flags: review?(gavin.sharp)
Attachment #561449 - Flags: review?(gavin.sharp)
Attachment #561225 - Flags: review?(gavin.sharp)
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

6 years ago
(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

6 years ago
Gavin: can we change the underlying platform method so all of the callers do the right thing?
(Assignee)

Comment 18

6 years ago
(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!
(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.
(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.
Created attachment 562113 [details] [diff] [review]
[in-fx-team] inspector.jsm + review followup
Attachment #561225 - Attachment is obsolete: true
(Assignee)

Comment 22

6 years ago
(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 on attachment 561225 [details] [diff] [review]
[in-fx-team] updated patch

https://hg.mozilla.org/integration/fx-team/rev/f297a626dd13
Attachment #561225 - Attachment description: updated patch → [in-fx-team] updated patch
Comment on attachment 562113 [details] [diff] [review]
[in-fx-team] inspector.jsm + review followup

https://hg.mozilla.org/integration/fx-team/rev/d9989de45bd9
Attachment #562113 - Attachment description: inspector.jsm + review followup → [in-fx-team] inspector.jsm + review followup
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/f297a626dd13

https://hg.mozilla.org/mozilla-central/rev/d9989de45bd9
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
(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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
relanded:

https://hg.mozilla.org/integration/fx-team/rev/34f444294220
Status: REOPENED → ASSIGNED
Target Milestone: Firefox 9 → ---
Created attachment 562498 [details] [diff] [review]
[in-fx-team] move inspector followup

follow up patch for an incorrectly referenced notification name in highlighter.
Attachment #562498 - Flags: review?(mihai.sucan)
(Assignee)

Comment 29

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

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

Thanks for the fix!
Attachment #562498 - Flags: review?(mihai.sucan) → review+
Comment on attachment 562498 [details] [diff] [review]
[in-fx-team] move inspector followup

https://hg.mozilla.org/integration/fx-team/rev/ae65dad8d51b
Attachment #562498 - Attachment description: move inspector followup → [in-fx-team] move inspector followup
https://hg.mozilla.org/mozilla-central/rev/34f444294220
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 9
https://hg.mozilla.org/mozilla-central/rev/ae65dad8d51b
Blocks: 703275
You need to log in before you can comment on or make changes to this bug.