Last Comment Bug 653528 - Strip out Style and DOM panels and support code from Inspector.
: Strip out Style and DOM panels and support code from Inspector.
Status: RESOLVED FIXED
[best: 1h, likely: 2h, worst: 1d][kil...
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 7
Assigned To: Rob Campbell [:rc] (:robcee)
:
: J. Ryan Stinnett [:jryans] (use ni?)
Mentors:
Depends on: 593460
Blocks: 642471
  Show dependency treegraph
 
Reported: 2011-04-28 12:13 PDT by Rob Campbell [:rc] (:robcee)
Modified: 2011-06-10 18:05 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
remove style and dom panels from inspector (wip) (47.81 KB, patch)
2011-05-03 11:32 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
tests fixed (51.52 KB, patch)
2011-05-04 09:45 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
remove style and dom panels from inspector (53.13 KB, patch)
2011-05-05 07:21 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
remove style and dom panels from inspector 2 (54.21 KB, patch)
2011-05-05 14:32 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
remove style and dom panels from inspector 4 (54.21 KB, patch)
2011-05-06 09:21 PDT, Rob Campbell [:rc] (:robcee)
gavin.sharp: review-
Details | Diff | Splinter Review
remove style and dom panels from inspector 5 (56.36 KB, patch)
2011-05-13 11:55 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
remove style and dom panels from inspector 6 (56.02 KB, patch)
2011-05-13 12:02 PDT, Rob Campbell [:rc] (:robcee)
gavin.sharp: review-
Details | Diff | Splinter Review
[backed-out] remove style and dom panels from inspector 7 (55.04 KB, patch)
2011-05-14 10:10 PDT, Rob Campbell [:rc] (:robcee)
gavin.sharp: review+
Details | Diff | Splinter Review
[in-devtools][checked-in] iframeTest fixes for Linux (56.00 KB, patch)
2011-05-26 06:23 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review

Description Rob Campbell [:rc] (:robcee) 2011-04-28 12:13:46 PDT
The Style panel is going to be replaced by a different solution. We don't need the code or the panel in the source code anymore. The DOM panel is accessible elsewhere and doesn't really fit with our plans for the highlighter.

These should be removed.
Comment 1 Rob Campbell [:rc] (:robcee) 2011-05-03 11:32:32 PDT
Created attachment 529775 [details] [diff] [review]
remove style and dom panels from inspector (wip)

WIP patch.

This works interactively, but breaks some of our unittests. In InspectorUI.openInspectorUI() I've played around with different positions of openTreePanel and initializeHighlighter. One of the worst failing tests, browser_inspector_tab_switch.js seems to be running too quickly.

Any suggestions on how to fix this to keep these tests running? I could use some mihai magic.
Comment 2 Mihai Sucan [:msucan] 2011-05-04 09:45:47 PDT
Created attachment 530045 [details] [diff] [review]
tests fixed

Updated the patch to fix the tests.
Comment 3 Rob Campbell [:rc] (:robcee) 2011-05-05 07:21:08 PDT
Created attachment 530306 [details] [diff] [review]
remove style and dom panels from inspector

back-ported panelshown and TabSelect fixes from highlighter patch. All tests pass.
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-05 11:20:47 PDT
Comment on attachment 530306 [details] [diff] [review]
remove style and dom panels from inspector

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

> function PanelHighlighter(aBrowser)
> {
>+  if (!aBrowser) {
>+    InspectorUI._trace("PanelHighlighter - aBrowser undefined");
>+  }

This doesn't seem that useful, won't this already throw an obvious exception below (assigning to this.win)? Was this just for debugging?

I don't understand much of the observer/notifyReady changes in openInspectorUI/initializeTreePanel (or the seemingly related changes in browser_inspector_tab_switch). What's the reasoning for those?

>+      this.treePanel.addEventListener("popuphidden", function() {
>+        InspectorUI.treePanel.removeEventListener("popuphidden",
>+          arguments.callee, false);

We should avoid using arguments.callee in the future, see comments in bug 434494. Name the listener function and just refer to it by name. (This also applies to other code added in this patch)

>   handleEvent: function IUI_handleEvent(event)

>       case "TabSelect":

Also don't understand how any of these changes are related to removing the DOM/Style panels.

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

> function inspectorUIOpen1()

>+  /*
>   gBrowser.selectedBrowser.addEventListener("load", function(evt) {
>     gBrowser.selectedBrowser.removeEventListener(evt.type, arguments.callee,
>       true);
>     waitForFocus(inspectorTabOpen2, content);
>   }, true);
>+  */

why?

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

You can also remove object.objectPanelTitle in inspector.properties.
Comment 5 Rob Campbell [:rc] (:robcee) 2011-05-05 13:17:01 PDT
(In reply to comment #4)
> Comment on attachment 530306 [details] [diff] [review] [review]
> remove style and dom panels from inspector
> 
> >diff --git a/browser/base/content/inspector.js b/browser/base/content/inspector.js
> 
> > function PanelHighlighter(aBrowser)
> > {
> >+  if (!aBrowser) {
> >+    InspectorUI._trace("PanelHighlighter - aBrowser undefined");
> >+  }
> 
> This doesn't seem that useful, won't this already throw an obvious exception
> below (assigning to this.win)? Was this just for debugging?

yes, missed that.

> I don't understand much of the observer/notifyReady changes in
> openInspectorUI/initializeTreePanel (or the seemingly related changes in
> browser_inspector_tab_switch). What's the reasoning for those?

tab_switch was showing some timing-related issues that were previously masked by the style and dom panel startup notifications (popupshown). The resulting changes to notifyReady were needed to ensure startup completed successfully during this test (and presumably others).

> >+      this.treePanel.addEventListener("popuphidden", function() {
> >+        InspectorUI.treePanel.removeEventListener("popuphidden",
> >+          arguments.callee, false);
> 
> We should avoid using arguments.callee in the future, see comments in bug
> 434494. Name the listener function and just refer to it by name. (This also
> applies to other code added in this patch)

will fix.

> >   handleEvent: function IUI_handleEvent(event)
> 
> >       case "TabSelect":
> 
> Also don't understand how any of these changes are related to removing the
> DOM/Style panels.

It was surprising to me as well, but they affected the entire way the InspectorUI started up.

> >diff --git a/browser/base/content/test/browser_inspector_tab_switch.js b/browser/base/content/test/browser_inspector_tab_switch.js
> 
> > function inspectorUIOpen1()
> 
> >+  /*
> >   gBrowser.selectedBrowser.addEventListener("load", function(evt) {
> >     gBrowser.selectedBrowser.removeEventListener(evt.type, arguments.callee,
> >       true);
> >     waitForFocus(inspectorTabOpen2, content);
> >   }, true);
> >+  */
> 
> why?

Added during debugging. Not sure if we need that anymore or not.

> >diff --git a/browser/locales/en-US/chrome/browser/browser.dtd b/browser/locales/en-US/chrome/browser/browser.dtd
> 
> You can also remove object.objectPanelTitle in inspector.properties.

will do. Thanks!
Comment 6 Rob Campbell [:rc] (:robcee) 2011-05-05 13:48:45 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > >+  /*
> > >   gBrowser.selectedBrowser.addEventListener("load", function(evt) {
> > >     gBrowser.selectedBrowser.removeEventListener(evt.type, arguments.callee,
> > >       true);
> > >     waitForFocus(inspectorTabOpen2, content);
> > >   }, true);
> > >+  */
> > 
> > why?
> 
> Added during debugging. Not sure if we need that anymore or not.

Oops, didn't see the comments around that block when I responded. I think I can safely remove the comments now.

Done, retesting and resubmitting...
Comment 7 Rob Campbell [:rc] (:robcee) 2011-05-05 14:32:17 PDT
Created attachment 530433 [details] [diff] [review]
remove style and dom panels from inspector 2

updated patch. Removed argments.callee, named functions for observers. Removed commented block in unittest. Passed mochitest-browser-chrome.
Comment 8 Rob Campbell [:rc] (:robcee) 2011-05-06 09:21:40 PDT
Created attachment 530650 [details] [diff] [review]
remove style and dom panels from inspector 4

Updated based on yesterday's review comments. Passed tests.
Comment 9 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-12 12:59:57 PDT
Comment on attachment 530650 [details] [diff] [review]
remove style and dom panels from inspector 4

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

>+    this.treePanel.addEventListener("popupshown", this.treePanelShown, false);

Just use a named function here rather than adding treePanelShown to the prototype.

>+  loadedInitializeTreePanel: function IUI_loadedInitializeTreePanel()

>+  treePanelShown: function IUI_treePanelShown()

>+    if (src != "chrome://browser/content/inspector.html") {
>+      InspectorUI.treeIFrame.addEventListener("DOMContentLoaded",
>+        InspectorUI.loadedInitializeTreePanel, true);

Same here - don't add loadedInitializeTreePanel, just pass initializeTreePanel directly (using bind(), or just make it a named method inside of openTreePanel).

>+  treePanelHidden: function IUI_treePanelHidden()

Here too.

>+  reopenInspectorForTab: function IUI_reopenInspectorForTab()

Is this related to this bug at all, or did it just get thrown in from some other change?

This patch is still missing the removal of object.objectPanelTitle.
Comment 10 Rob Campbell [:rc] (:robcee) 2011-05-13 11:55:32 PDT
Created attachment 532298 [details] [diff] [review]
remove style and dom panels from inspector 5

removed extraneous callback functions, replaced them with inlined, named functions.

Able to remove inspector.properties as well, entities no longer required.

Found and fixed a bug by reverting to previous TabSelect handling code. Resulting fix is much more resilient on tab switching. Passes all unittests.
Comment 11 Rob Campbell [:rc] (:robcee) 2011-05-13 12:02:37 PDT
Created attachment 532303 [details] [diff] [review]
remove style and dom panels from inspector 6

removed accidentally included comment
Comment 12 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-13 14:29:56 PDT
Comment on attachment 532303 [details] [diff] [review]
remove style and dom panels from inspector 6

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

>   openInspectorUI: function IUI_openInspectorUI()

>+    Services.obs.addObserver(this.notifyReady, "highlighter-ready", false);

Does this actually work? this.notifyReady seems to rely on |this| being correct when invoked... Also you're already calling notifyReady from initializeTreePanel, which is what calls initializeHighlighter (and fires "highlighter-ready"), so this call seems entirely redundant even if it did work. Seems like you can get rid of "highlighter-ready".

>-    this.closing = false;
>+    InspectorUI.closing = false;

Why this change?

>   notifyReady: function IUI_notifyReady()
>   {
>+    if (!this.isTreePanelOpen || !this.treeLoaded) {
>+      return;
>+    }

These checks shouldn't be needed once you remove highlighter-ready, since the only way notifyReady can be called is if they are both true.

>+    if (!this.highlighter.notified) {
>+      Services.obs.removeObserver(this.notifyReady, "highlighter-ready", false);
>+      this.highlighter.notified = true;
>+    }

This wouldn't be needed either.

>     document.removeEventListener("popupshowing", this, false);

Perhaps unrelated to this patch, but what does this line do? I don't see a popupshowing handler added anywhere.

>   handleEvent: function IUI_handleEvent(event)

>         } else if (InspectorStore.isEmpty()) {
>           gBrowser.tabContainer.removeEventListener("TabSelect", this, false);
>         }
>+
>+        if (InspectorStore.isEmpty()) {
>+          gBrowser.tabContainer.removeEventListener("TabSelect", this, false);
>+        }

This doesn't look right.
Comment 13 Rob Campbell [:rc] (:robcee) 2011-05-14 10:10:23 PDT
Created attachment 532445 [details] [diff] [review]
[backed-out] remove style and dom panels from inspector 7

good suggestions! Ripped out extraneous notifications and code per your suggestions. Resulting code works really well.
Comment 14 Rob Campbell [:rc] (:robcee) 2011-05-18 10:33:00 PDT
Comment on attachment 532445 [details] [diff] [review]
[backed-out] remove style and dom panels from inspector 7

http://hg.mozilla.org/projects/devtools/rev/6b0c16915edd
Comment 16 Rob Campbell [:rc] (:robcee) 2011-05-19 03:38:46 PDT
this appears to be a problem only on linux. I'll have to spin up my VM to do a little testing.
Comment 17 Girish Sharma [:Optimizer] 2011-05-22 04:28:28 PDT
using the latest tinderbox builds , I still have both the styles and object panel for inspector devtool.

Also please explain what is inspector 7 and where to get it ?
Comment 18 Rob Campbell [:rc] (:robcee) 2011-05-22 04:54:53 PDT
yes, this patch was backed out because of test failures, so the panels are still present.

I don't know what "inspector 7" is. Where did you hear about it?
Comment 19 Girish Sharma [:Optimizer] 2011-05-22 05:06:07 PDT
Read the above comments , comments 13 to 16
Comment 20 Rob Campbell [:rc] (:robcee) 2011-05-22 05:07:14 PDT
"inspector 7" is the name of the patch that was backed-out.
Comment 21 Mihai Sucan [:msucan] 2011-05-26 06:23:49 PDT
Created attachment 535320 [details] [diff] [review]
[in-devtools][checked-in] iframeTest fixes for Linux

This updated patch fixes the iframeTest on Linux.

I was able to reproduce the tryserver failures on my machine. After several hours of debugging yesterday with robcee, and a few more today we figured it out. All tests pass.

I suggest a tryserver push is in order. ;)
Comment 22 Rob Campbell [:rc] (:robcee) 2011-05-26 11:19:56 PDT
Comment on attachment 535320 [details] [diff] [review]
[in-devtools][checked-in] iframeTest fixes for Linux

http://hg.mozilla.org/projects/devtools/rev/6d63840e3a49
Comment 23 Dave Camp (:dcamp) 2011-06-10 18:05:01 PDT
Comment on attachment 535320 [details] [diff] [review]
[in-devtools][checked-in] iframeTest fixes for Linux

http://hg.mozilla.org/mozilla-central/rev/6d63840e3a49

Note You need to log in before you can comment on or make changes to this bug.