653528 - Strip out Style and DOM panels and support code from Inspector.

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

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

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)]

Attachment: 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]]

Attachment: tests fixed

Updated the patch to fix the tests.

Comment 3

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

Attachment: remove style and dom panels from inspector

back-ported panelshown and TabSelect fixes from highlighter patch. All tests pass.

Comment 4

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

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)]

(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)]

(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)]

Attachment: 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)]

Attachment: remove style and dom panels from inspector 4

Updated based on yesterday's review comments. Passed tests.

Comment 9

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

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)]

Attachment: 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)]

Attachment: remove style and dom panels from inspector 6

removed accidentally included comment

Comment 12

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

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)]

Attachment: [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)]

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 15

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

Comment on attachment 532445 [details] [diff] [review]
[backed-out] remove style and dom panels from inspector 7

http://hg.mozilla.org/projects/devtools/rev/694bda5443fc

http://tinderbox.mozilla.org/showlog.cgi?log=Devtools/1305742311.1305743612.26649.gz&fulltext=1

screenshot: http://www.cl.ly/6r1S

Comment 16

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

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]]

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)]

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]]

Read the above comments , comments 13 to 16

Comment 20

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

"inspector 7" is the name of the patch that was backed-out.

Comment 21

[Mihai Sucan [:msucan]]

Attachment: [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)]

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)]

Comment on attachment 535320 [details] [diff] [review]
[in-devtools][checked-in] iframeTest fixes for Linux

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