Closed Bug 719607 Opened 8 years ago Closed 8 years ago

The HTML Tree Panel should NOT use the registerTools mechanism

Categories

(DevTools :: Inspector, defect)

x86
All
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 13

People

(Reporter: paul, Unassigned)

References

Details

Attachments

(2 files, 4 obsolete files)

The HTML Tree is not a Node Centric tool but just a feature of the Inspector. Also, since it uses the registerTools API, we can't move the <HTML> button where ever we want.

See also bug 717924
Attached patch patch v1 (obsolete) — Splinter Review
No longer depends on: 707809
Attachment #590411 - Flags: review?(rcampbell)
Depends on: 717924
Blocks: 696755
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #590411 - Attachment is obsolete: true
Attachment #590411 - Flags: review?(rcampbell)
I forgot to save the Tree state if the inspector get "hidden". And I actually added a preference to save the latest state of the Tree (and that's gonna fix bug 696755).
Attachment #592140 - Flags: review?(rcampbell)
Comment on attachment 592140 [details] [diff] [review]
Patch v2

This looks good to me. Thanks!
Attachment #592140 - Flags: review?(rcampbell) → review+
Whiteboard: [land-in-fx-team]
Whiteboard: [land-in-fx-team] → [waiting-on-blocking-bug]
Attached patch addendum (obsolete) — Splinter Review
To be applied on top of patch v2:

* Fixes tests.
* Fixes another conflated "opened" and "loaded" mess.  The current patch exacerbates an existing problem (with this patch, try opening the html panel and then toggling the inspector on and off really quickly with the keybinding, and watch a stack of splitters build up).
* Gets rid of !openInDock. That codepath was never used, and made fixing the previous bullet point more difficult.
Attachment #592442 - Flags: review?(rcampbell)
Comment on attachment 592442 [details] [diff] [review]
addendum

clean.
Attachment #592442 - Flags: review?(rcampbell) → review+
Comment on attachment 592442 [details] [diff] [review]
addendum

>--- a/browser/devtools/highlighter/test/head.js
>+++ b/browser/devtools/highlighter/test/head.js
>@@ -34,17 +34,26 @@
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> const Cu = Components.utils;
> let tempScope = {};
> Cu.import("resource:///modules/devtools/LayoutHelpers.jsm", tempScope);
>+Cu.import("resource://gre/modules/Services.jsm", tempScope);
> let LayoutHelpers = tempScope.LayoutHelpers;
>+let Services = tempScope.Services;

Services is already defined here.

>+// Clear preferences that may be set during the course of tests.
>+function clearUserPrefs()
>+{
>+  Services.prefs.clearUserPref("devtools.inspector.htmlPanelOpen");
>+}
>+clearUserPrefs();

I don't know why exactly this is needed, but you generally need to clean up after tests rather than before them. I.e.:

registerCleanupFunction(function () {
  Services.prefs.clearUserPref("devtools.inspector.htmlPanelOpen");
});
Attached patch addendum v2 (obsolete) — Splinter Review
Attachment #592442 - Attachment is obsolete: true
Blocks: 724507
Blocks: 726518
I think these can land. I'm going to send them through try server today to make sure.
Status: NEW → ASSIGNED
Attachment #592140 - Attachment is obsolete: true
Attachment #592739 - Attachment is obsolete: true
Whiteboard: [waiting-on-blocking-bug] → [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/c573765690ce
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/c573765690ce
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 13
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.