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

RESOLVED FIXED in Firefox 7

Status

()

Firefox
Developer Tools
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: rc, Assigned: rc)

Tracking

unspecified
Firefox 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [best: 1h, likely: 2h, worst: 1d][killthem][fixed-in-devtools][merged-to-mozilla-central])

Attachments

(1 attachment, 8 obsolete attachments)

(Assignee)

Description

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

Updated

6 years ago
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [best: 1h, likely: 2h, worst: 1d]
(Assignee)

Updated

6 years ago
Whiteboard: [best: 1h, likely: 2h, worst: 1d] → [best: 1h, likely: 2h, worst: 1d][killthem]
(Assignee)

Updated

6 years ago
Assignee: nobody → rcampbell
(Assignee)

Comment 1

6 years ago
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.
Attachment #529775 - Flags: feedback?(mihai.sucan)
(Assignee)

Updated

6 years ago
Depends on: 593460
Created attachment 530045 [details] [diff] [review]
tests fixed

Updated the patch to fix the tests.
Attachment #529775 - Attachment is obsolete: true
Attachment #529775 - Flags: feedback?(mihai.sucan)
(Assignee)

Comment 3

6 years ago
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.
Attachment #530045 - Attachment is obsolete: true
Attachment #530306 - Flags: review?(gavin.sharp)
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.
(Assignee)

Comment 5

6 years ago
(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!
Status: NEW → ASSIGNED
(Assignee)

Comment 6

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

Comment 7

6 years ago
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.
Attachment #530306 - Attachment is obsolete: true
Attachment #530306 - Flags: review?(gavin.sharp)
Attachment #530433 - Flags: review?(gavin.sharp)
(Assignee)

Comment 8

6 years ago
Created attachment 530650 [details] [diff] [review]
remove style and dom panels from inspector 4

Updated based on yesterday's review comments. Passed tests.
Attachment #530433 - Attachment is obsolete: true
Attachment #530433 - Flags: review?(gavin.sharp)
Attachment #530650 - Flags: review?(gavin.sharp)
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.
Attachment #530650 - Flags: review?(gavin.sharp) → review-
(Assignee)

Comment 10

6 years ago
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.
Attachment #532298 - Flags: review?(gavin.sharp)
(Assignee)

Comment 11

6 years ago
Created attachment 532303 [details] [diff] [review]
remove style and dom panels from inspector 6

removed accidentally included comment
Attachment #530650 - Attachment is obsolete: true
Attachment #532298 - Attachment is obsolete: true
Attachment #532298 - Flags: review?(gavin.sharp)
Attachment #532303 - Flags: review?(gavin.sharp)
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.
Attachment #532303 - Flags: review?(gavin.sharp) → review-
(Assignee)

Comment 13

6 years ago
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.
Attachment #532303 - Attachment is obsolete: true
Attachment #532445 - Flags: review?(gavin.sharp)
Attachment #532445 - Attachment is patch: true
Attachment #532445 - Attachment mime type: text/x-patch → text/plain
Attachment #532445 - Flags: review?(gavin.sharp) → review+
(Assignee)

Updated

6 years ago
Whiteboard: [best: 1h, likely: 2h, worst: 1d][killthem] → [best: 1h, likely: 2h, worst: 1d][killthem][fixed-in-devtools]
(Assignee)

Comment 14

6 years ago
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
Attachment #532445 - Attachment description: remove style and dom panels from inspector 7 → [in-devtools] remove style and dom panels from inspector 7
(Assignee)

Updated

6 years ago
Whiteboard: [best: 1h, likely: 2h, worst: 1d][killthem][fixed-in-devtools] → [best: 1h, likely: 2h, worst: 1d][killthem][backed-out]
(Assignee)

Comment 15

6 years ago
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
Attachment #532445 - Attachment description: [in-devtools] remove style and dom panels from inspector 7 → [backed-out] remove style and dom panels from inspector 7
(Assignee)

Comment 16

6 years ago
this appears to be a problem only on linux. I'll have to spin up my VM to do a little testing.
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 ?
(Assignee)

Comment 18

6 years ago
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?
Read the above comments , comments 13 to 16
(Assignee)

Comment 20

6 years ago
"inspector 7" is the name of the patch that was backed-out.
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. ;)
Attachment #532445 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Whiteboard: [best: 1h, likely: 2h, worst: 1d][killthem][backed-out] → [best: 1h, likely: 2h, worst: 1d][killthem][fixed-in-devtools]
(Assignee)

Comment 22

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

http://hg.mozilla.org/projects/devtools/rev/6d63840e3a49
Attachment #535320 - Attachment description: iframeTest fixes for Linux → [in-devtools] iframeTest fixes for Linux

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [best: 1h, likely: 2h, worst: 1d][killthem][fixed-in-devtools] → [best: 1h, likely: 2h, worst: 1d][killthem][fixed-in-devtools][merged-to-mozilla-central]
Target Milestone: --- → Firefox 7

Comment 23

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

http://hg.mozilla.org/mozilla-central/rev/6d63840e3a49
Attachment #535320 - Attachment description: [in-devtools] iframeTest fixes for Linux → [in-devtools][checked-in] iframeTest fixes for Linux
You need to log in before you can comment on or make changes to this bug.