Last Comment Bug 663831 - Style inspector should be controllable from the highlighter
: Style inspector should be controllable from the highlighter
Status: VERIFIED FIXED
[minotaur][styleinspector][testday-20...
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: x86 All
: P1 normal (vote)
: Firefox 9
Assigned To: Rob Campbell [:rc] (:robcee)
:
: J. Ryan Stinnett [:jryans] (use ni?)
Mentors:
Depends on: 582596 660606 681653
Blocks: 650794 663830 683954 689164
  Show dependency treegraph
 
Reported: 2011-06-13 08:35 PDT by Michael Ratcliffe [:miker] [:mratcliffe]
Modified: 2011-11-25 08:04 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Style inspector registerTools patch (5.37 KB, patch)
2011-06-15 06:23 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
dolske: review+
Details | Diff | Splinter Review
Style inspector registerTools patch 2 (6.43 KB, patch)
2011-07-06 03:45 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
Style inspector registerTools patch 3 (18.23 KB, patch)
2011-07-26 07:26 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
Style inspector registerTools patch 4 (5.51 KB, patch)
2011-07-28 12:27 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
Style inspector registerTools patch 5 (5.54 KB, patch)
2011-08-09 09:39 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
Style inspector registerTools patch 6 (5.37 KB, patch)
2011-08-14 10:35 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
Style inspector registerTools patch 7 (3.03 KB, patch)
2011-08-19 04:09 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
Style inspector registerTools patch 8 (4.49 KB, patch)
2011-08-25 02:18 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
dolske: review+
Details | Diff | Splinter Review
Style inspector registerTools patch 9 (11.95 KB, patch)
2011-08-29 07:34 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
Style inspector registerTools patch 10 (10.43 KB, patch)
2011-08-31 13:07 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
Style inspector registerTools patch 11 WIP (9.19 KB, patch)
2011-09-06 07:41 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
Style inspector registerTools patch 12 WIP (14.45 KB, patch)
2011-09-06 14:19 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
Style inspector registerTools patch 13 (14.83 KB, patch)
2011-09-07 13:48 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
Style inspector registerTools patch 14 (15.00 KB, patch)
2011-09-21 06:42 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
[in-fx-team] Style inspector registerTools patch 14 (requires patch from bug 687854) (15.03 KB, patch)
2011-09-21 07:45 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review

Description Michael Ratcliffe [:miker] [:mratcliffe] 2011-06-13 08:35:20 PDT
When InspectorUI.registerTool() is complete we should plug in the Style Inspector
Comment 1 Michael Ratcliffe [:miker] [:mratcliffe] 2011-06-15 06:23:23 PDT
Created attachment 539491 [details] [diff] [review]
Style inspector registerTools patch
Comment 2 Rob Campbell [:rc] (:robcee) 2011-06-24 11:31:02 PDT
Comment on attachment 539491 [details] [diff] [review]
Style inspector registerTools patch

+    panel.setAttribute("height", 400);

magic number. Does this make sense on all screens? honestly, it probably does. My smallest is a netbook with 600px vertical. Going to be small on bigger screens though.

+   * @param [aNode] Node to open

nit: don't need [] around aNode

-  show: function SI_show()
+  show: function SI_show(aNode)
   {
-    if (!this.isOpen) {
+    if (aNode) {
+      this.selectNode(aNode, true);
+    } else if (!this.isOpen) {

I'll need to look at the actual style panel patch, but this looks a little hinky. I think we can clean up this method so it's not full of a bunch of nested ifs.

I think we can do that in the originating patch though, so I won't hold this against you here.

Otherwise, looks good. r+
Comment 3 Justin Dolske [:Dolske] 2011-06-29 18:38:03 PDT
Comment on attachment 539491 [details] [diff] [review]
Style inspector registerTools patch

Review of attachment 539491 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/styleinspector.js
@@ +114,2 @@
>      panel.setAttribute("width", 200);
> +    panel.setAttribute("height", 400);

Suggestion: it would be good to make the height/width persistent, so that when you resize on the rest will open at that size. This could be a followup.

@@ +309,5 @@
> +window.addEventListener("load", function SI_onBrowserLoad() {
> +  window.removeEventListener("load", arguments.callee, false);
> +  let styleInspector = new StyleInspector();
> +  if (styleInspector.isEnabled) {
> +    InspectorUI.registerTool({

I suspect this will need to change based on my comments to the CSS Inspector bug. But what you're doing here is pretty straightwforward, so I assume the corrections will be too. Flag me again if they're not. :)

::: browser/locales/en-US/chrome/browser/styleinspector.properties
@@ +60,5 @@
> +# html tree of the highlighter for the style inspector button
> +style.highlighter.button.label=Style
> +style.highlighter.button.tooltip=Inspect element styles
> +
> +style.highlighter.accesskey=S

Move this up so it's below .label?
Comment 4 Michael Ratcliffe [:miker] [:mratcliffe] 2011-07-06 03:45:40 PDT
Created attachment 544184 [details] [diff] [review]
Style inspector registerTools patch 2

Rebased and made size calculation according to screen size.
Comment 5 Michael Ratcliffe [:miker] [:mratcliffe] 2011-07-21 01:21:35 PDT
> this doesn't apply cleanly. Looks like it's based on an out-of-date tree. (the test is going into browser/base/content/test instead of browser/base/content/test/inspector, for example).

It applies cleanly for me even on a brand new clean repo. Those tests are for the highlighter so I believe that browser/base/content/test is the correct directory

> there is also a segment failing to apply to HUDService.jsm I'm not sure is supposed to be there.
> 
> +    panels = popupset.querySelectorAll("panel[hudToolId=" + id + "]");
> +    for (let i = 0; i < panels.length; i++) {
> +      panels[i].hidePopup();
> +      popupset.removeChild(panels[i]);
> +    }

Applies cleanly for me. The modifications to HUDService add the inspectstyle(node) command (similar to the inspect() command). This particular section removes the style inspector from the DOM on console / tab close ready for garbage collection.

> um, disregard that last comment, though I'm still a bit puzzled why there are modifications to HUDService for this.

See my description above

> This doesn't appear to be using the registerTools API at all. Did that change?

This patch is the style inspector itself and it's integration with the HUD via inspectstyle(node) ... for the full flavor behaviour you will need:
Bug 660606 - Highlighter should allow registration of developer tools (I believe that you are planning on landing this soon)
Bug 663831 - Style inspector should be controllable from the highlighter

##################

Are you reviewing this now instead of Dolske?
Comment 6 Michael Ratcliffe [:miker] [:mratcliffe] 2011-07-21 03:10:09 PDT
Sorry, ignore my comment above ... wrong bug
Comment 7 Michael Ratcliffe [:miker] [:mratcliffe] 2011-07-26 07:26:35 PDT
Created attachment 548444 [details] [diff] [review]
Style inspector registerTools patch 3

Rebased and moved test patch over to bug 660606
Comment 8 Michael Ratcliffe [:miker] [:mratcliffe] 2011-07-28 12:27:55 PDT
Created attachment 549195 [details] [diff] [review]
Style inspector registerTools patch 4
Comment 9 Michael Ratcliffe [:miker] [:mratcliffe] 2011-08-09 09:39:42 PDT
Created attachment 551796 [details] [diff] [review]
Style inspector registerTools patch 5

Rebased
Comment 10 Mihai Sucan [:msucan] 2011-08-11 11:45:00 PDT
Please note that this patch has the following commit message:

"Register Tools & Integrated Style Inspector patches try: -b do -p linux,linuxqt,linux64,macosx64,win32,macosx -u all -t none"

It includes the try chooser options. :)
Comment 11 Michael Ratcliffe [:miker] [:mratcliffe] 2011-08-14 10:35:17 PDT
Created attachment 552993 [details] [diff] [review]
Style inspector registerTools patch 6

Not any more
Comment 12 Michael Ratcliffe [:miker] [:mratcliffe] 2011-08-19 04:09:59 PDT
Created attachment 554359 [details] [diff] [review]
Style inspector registerTools patch 7

Rebased
Comment 13 Justin Dolske [:Dolske] 2011-08-23 18:50:30 PDT
Comment on attachment 554359 [details] [diff] [review]
Style inspector registerTools patch 7

Review of attachment 554359 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/styleinspector/StyleInspector.jsm
@@ +184,5 @@
> +
> +// Register the style inspector with the highlighter
> +win.addEventListener("load", function SI_onBrowserLoad() {
> +  win.removeEventListener("load", arguments.callee, false);
> +  win.setTimeout(function() {

Oof, setTimeout?!

@@ +185,5 @@
> +// Register the style inspector with the highlighter
> +win.addEventListener("load", function SI_onBrowserLoad() {
> +  win.removeEventListener("load", arguments.callee, false);
> +  win.setTimeout(function() {
> +    if (StyleInspector.isEnabled) {

It would be helpful to avoid as _much_ work as possible during window creation.

For example, I'd suggest:

1) Check StyleInspector.isEnabled() before even adding the event listener

2) Check Services.prefs() instead of StyleInspector.isEnabled(), so that you can avoid the first-access cost of importing StyleInspector.

3) We might want to look at having something in devtools/mumble or browser.js handle the general pattern of watching for newly-loaded-windows to kick off needed services/modules interested in knowing about that.

4) Might as well start thinking about all that in the context of E10S, too! :)
Comment 14 Michael Ratcliffe [:miker] [:mratcliffe] 2011-08-25 02:18:11 PDT
Created attachment 555679 [details] [diff] [review]
Style inspector registerTools patch 8

(In reply to Justin Dolske [:Dolske] from comment #13)
> Comment on attachment 554359 [details] [diff] [review]
> Style inspector registerTools patch 7
> 
> Review of attachment 554359 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/styleinspector/StyleInspector.jsm
> @@ +184,5 @@
> > +
> > +// Register the style inspector with the highlighter
> > +win.addEventListener("load", function SI_onBrowserLoad() {
> > +  win.removeEventListener("load", arguments.callee, false);
> > +  win.setTimeout(function() {
> 
> Oof, setTimeout?!
> 

Yeah, an awful hack because I had the impression that StyleInspector stuff should stay inside StyleInspector.jsm ... I have now moved the initialization into the highlighter itself so the event listener is no longer used.

> @@ +185,5 @@
> > +// Register the style inspector with the highlighter
> > +win.addEventListener("load", function SI_onBrowserLoad() {
> > +  win.removeEventListener("load", arguments.callee, false);
> > +  win.setTimeout(function() {
> > +    if (StyleInspector.isEnabled) {
> 
> It would be helpful to avoid as _much_ work as possible during window
> creation.
> 
> For example, I'd suggest:
> 
> 1) Check StyleInspector.isEnabled() before even adding the event listener
> 

How about no work? Done.

> 2) Check Services.prefs() instead of StyleInspector.isEnabled(), so that you
> can avoid the first-access cost of importing StyleInspector.
> 

Done

> 3) We might want to look at having something in devtools/mumble or
> browser.js handle the general pattern of watching for newly-loaded-windows
> to kick off needed services/modules interested in knowing about that.
> 

Maybe, but we don't need that at the moment.

> 4) Might as well start thinking about all that in the context of E10S, too!
> :)

True, very true
Comment 15 Michael Ratcliffe [:miker] [:mratcliffe] 2011-08-29 02:33:17 PDT
I was asked to make some changes and base this upon robcee's changes to the registerTools API (bug 681653)
Comment 16 Michael Ratcliffe [:miker] [:mratcliffe] 2011-08-29 07:34:04 PDT
Created attachment 556548 [details] [diff] [review]
Style inspector registerTools patch 9
Comment 17 Rob Campbell [:rc] (:robcee) 2011-08-31 10:17:24 PDT
I don't think we want to initialize the tools inside the highlighter object which is itself just a component of the InspectorUI.
Comment 18 Rob Campbell [:rc] (:robcee) 2011-08-31 13:07:55 PDT
Created attachment 557289 [details] [diff] [review]
Style inspector registerTools patch 10

preliminary update based on unfinished registerTools augmentation.

I've taken some of the bits from Mike's patch and am incorporating it backwards into bug 681653.
Comment 19 Rob Campbell [:rc] (:robcee) 2011-09-06 07:41:54 PDT
Created attachment 558473 [details] [diff] [review]
Style inspector registerTools patch 11 WIP

updated fiddly bits.
Comment 20 Rob Campbell [:rc] (:robcee) 2011-09-06 14:19:59 PDT
Created attachment 558608 [details] [diff] [review]
Style inspector registerTools patch 12 WIP
Comment 21 Rob Campbell [:rc] (:robcee) 2011-09-07 13:48:12 PDT
Created attachment 558943 [details] [diff] [review]
Style inspector registerTools patch 13

Updated patch to reflect the new API. All tests passing.
Comment 22 Mihai Sucan [:msucan] 2011-09-08 05:14:57 PDT
Comment on attachment 558943 [details] [diff] [review]
Style inspector registerTools patch 13

Review of attachment 558943 [details] [diff] [review]:
-----------------------------------------------------------------

Things we forgot yesterday:

::: browser/base/content/inspector.js
@@ +796,5 @@
>    {
> +    // Style inspector
> +    if (Services.prefs.getBoolPref("devtools.styleinspector.enabled") &&
> +        !this.toolRegistered("styleinspector")) {
> +      let stylePanel = this.StyleInspector.createPanel(true);

The tree panel object instance is stored in InspectorUI.treePanel.

Shouldn't we also store the stylePanel instance? It would make it accessible for tests and whatever we may need.

@@ +1597,5 @@
>      btn.setAttribute("tooltiptext", aRegObj.tooltiptext);
>      btn.setAttribute("accesskey", aRegObj.accesskey);
>      // btn.setAttribute("class", "toolbarbutton-text");
>      btn.setAttribute("image", aRegObj.icon || "");
> +    // btn.setAttribute("type", "radio");

In bug 681653 we add this line and here we comment it out. We should be better decided here. :)
Comment 23 Rob Campbell [:rc] (:robcee) 2011-09-21 06:42:49 PDT
Created attachment 561452 [details] [diff] [review]
Style inspector registerTools patch 14

cleaned and polished. Removed debugging chatter.
Comment 24 Rob Campbell [:rc] (:robcee) 2011-09-21 07:45:33 PDT
Created attachment 561464 [details] [diff] [review]
[in-fx-team] Style inspector registerTools patch 14 (requires patch from bug 687854)
Comment 25 Rob Campbell [:rc] (:robcee) 2011-09-23 15:34:22 PDT
Comment on attachment 561464 [details] [diff] [review]
[in-fx-team] Style inspector registerTools patch 14 (requires patch from bug 687854)

https://hg.mozilla.org/integration/fx-team/rev/f1409901573a
Comment 26 Rob Campbell [:rc] (:robcee) 2011-09-23 15:49:20 PDT
https://hg.mozilla.org/mozilla-central/rev/f1409901573a
Comment 27 Dão Gottwald [:dao] 2011-09-24 03:38:06 PDT
Backed out because of various new mochitest-browser-chrome leaks. Unfortunately, this landed with a bunch of other interweaved patches. I only felt comfortable ruling out f297a626dd13 and 7d5311c92e04.
Comment 28 Tim Taubert [:ttaubert] 2011-09-27 04:24:56 PDT
https://hg.mozilla.org/mozilla-central/rev/264e504e0ce9
Comment 29 Teodosia Pop 2011-11-25 08:04:59 PST
Verified fixed using
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:10.0a2) Gecko/20111124 Firefox/10.0a2
and
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a2) Gecko/20111123 Firefox/10.0a2

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