Last Comment Bug 719607 - The HTML Tree Panel should NOT use the registerTools mechanism
: The HTML Tree Panel should NOT use the registerTools mechanism
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: Firefox 13
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on: 717924
Blocks: 584407 696755 717922 722552 724507 726518
  Show dependency treegraph
 
Reported: 2012-01-19 14:39 PST by Paul Rouget [:paul]
Modified: 2012-02-27 01:18 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (12.65 KB, patch)
2012-01-20 19:09 PST, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
Patch v2 (16.31 KB, patch)
2012-01-27 08:16 PST, Paul Rouget [:paul]
rcampbell: review+
Details | Diff | Splinter Review
addendum (11.65 KB, patch)
2012-01-28 14:12 PST, Dave Camp (:dcamp)
rcampbell: review+
Details | Diff | Splinter Review
addendum v2 (8.33 KB, patch)
2012-01-30 09:10 PST, Dave Camp (:dcamp)
no flags Details | Diff | Splinter Review
Patch v2 - rebased (9.88 KB, patch)
2012-02-21 02:56 PST, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
addendum v2 - rebased (8.37 KB, patch)
2012-02-21 02:57 PST, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review

Description Paul Rouget [:paul] 2012-01-19 14:39:28 PST
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
Comment 1 Paul Rouget [:paul] 2012-01-20 19:09:58 PST
Created attachment 590411 [details] [diff] [review]
patch v1
Comment 2 Paul Rouget [:paul] 2012-01-27 08:16:16 PST
Created attachment 592140 [details] [diff] [review]
Patch v2
Comment 3 Paul Rouget [:paul] 2012-01-27 08:19:21 PST
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).
Comment 4 Rob Campbell [:rc] (:robcee) 2012-01-28 06:45:17 PST
Comment on attachment 592140 [details] [diff] [review]
Patch v2

This looks good to me. Thanks!
Comment 5 Dave Camp (:dcamp) 2012-01-28 14:12:22 PST
Created attachment 592442 [details] [diff] [review]
addendum

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.
Comment 6 Rob Campbell [:rc] (:robcee) 2012-01-29 08:04:58 PST
Comment on attachment 592442 [details] [diff] [review]
addendum

clean.
Comment 7 Dão Gottwald [:dao] 2012-01-29 08:47:50 PST
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");
});
Comment 8 Dave Camp (:dcamp) 2012-01-30 09:10:48 PST
Created attachment 592739 [details] [diff] [review]
addendum v2
Comment 9 Rob Campbell [:rc] (:robcee) 2012-02-21 02:29:12 PST
I think these can land. I'm going to send them through try server today to make sure.
Comment 10 Rob Campbell [:rc] (:robcee) 2012-02-21 02:56:49 PST
Created attachment 599077 [details] [diff] [review]
Patch v2 - rebased
Comment 11 Rob Campbell [:rc] (:robcee) 2012-02-21 02:57:19 PST
Created attachment 599078 [details] [diff] [review]
addendum v2 - rebased
Comment 13 Tim Taubert [:ttaubert] 2012-02-27 01:18:53 PST
https://hg.mozilla.org/mozilla-central/rev/c573765690ce

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