Style inspector should be controllable from the highlighter

VERIFIED FIXED in Firefox 9

Status

()

Firefox
Developer Tools
P1
normal
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: miker, Assigned: rc)

Tracking

unspecified
Firefox 9
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [minotaur][styleinspector][testday-20111125])

Attachments

(1 attachment, 14 obsolete attachments)

15.03 KB, patch
Details | Diff | Splinter Review
When InspectorUI.registerTool() is complete we should plug in the Style Inspector
Depends on: 660606
Assignee: nobody → mratcliffe
Created attachment 539491 [details] [diff] [review]
Style inspector registerTools patch
Attachment #539491 - Flags: review?(rcampbell)
(Assignee)

Comment 2

6 years ago
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+
Attachment #539491 - Flags: review?(rcampbell) → review+
Attachment #539491 - Flags: review+ → review?(dolske)
(Assignee)

Updated

6 years ago
Blocks: 663830
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?
Attachment #539491 - Flags: review?(dolske) → review+
Created attachment 544184 [details] [diff] [review]
Style inspector registerTools patch 2

Rebased and made size calculation according to screen size.
Attachment #539491 - Attachment is obsolete: true
Depends on: 582596

Updated

6 years ago
Whiteboard: minotaur
Whiteboard: minotaur → [minotaur], [hydra]
Blocks: 582596
No longer depends on: 582596
> 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?
Sorry, ignore my comment above ... wrong bug
Whiteboard: [minotaur], [hydra] → [minotaur][styleinspector]
(Assignee)

Updated

6 years ago
Priority: -- → P1
Whiteboard: [minotaur][styleinspector] → [minotaur][styleinspector][has-patch]
Created attachment 548444 [details] [diff] [review]
Style inspector registerTools patch 3

Rebased and moved test patch over to bug 660606
Attachment #544184 - Attachment is obsolete: true
Status: NEW → ASSIGNED
No longer blocks: 582596
Depends on: 582596
Created attachment 549195 [details] [diff] [review]
Style inspector registerTools patch 4
Attachment #548444 - Attachment is obsolete: true
Whiteboard: [minotaur][styleinspector][has-patch] → [minotaur][styleinspector][has-patch][review+]
Created attachment 551796 [details] [diff] [review]
Style inspector registerTools patch 5

Rebased
Attachment #549195 - Attachment is obsolete: true
Blocks: 672743
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. :)
Created attachment 552993 [details] [diff] [review]
Style inspector registerTools patch 6

Not any more
Attachment #551796 - Attachment is obsolete: true
Created attachment 554359 [details] [diff] [review]
Style inspector registerTools patch 7

Rebased
Attachment #552993 - Attachment is obsolete: true
Blocks: 680111
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! :)
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
Attachment #554359 - Attachment is obsolete: true
Attachment #555679 - Flags: review?(dolske)
Blocks: 674856
Attachment #555679 - Flags: review?(dolske) → review+
Depends on: 650794
I was asked to make some changes and base this upon robcee's changes to the registerTools API (bug 681653)
Depends on: 681653
No longer depends on: 650794
Created attachment 556548 [details] [diff] [review]
Style inspector registerTools patch 9
Attachment #555679 - Attachment is obsolete: true
No longer blocks: 680111
No longer blocks: 672743
(Assignee)

Comment 17

6 years ago
I don't think we want to initialize the tools inside the highlighter object which is itself just a component of the InspectorUI.
Whiteboard: [minotaur][styleinspector][has-patch][review+] → [minotaur][styleinspector][has-patch]
(Assignee)

Comment 18

6 years ago
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.
Attachment #556548 - Attachment is obsolete: true

Updated

6 years ago
Blocks: 683954
No longer blocks: 674856
(Assignee)

Comment 19

6 years ago
Created attachment 558473 [details] [diff] [review]
Style inspector registerTools patch 11 WIP

updated fiddly bits.
(Assignee)

Comment 20

6 years ago
Created attachment 558608 [details] [diff] [review]
Style inspector registerTools patch 12 WIP
Assignee: mratcliffe → rcampbell
Attachment #557289 - Attachment is obsolete: true
Attachment #558473 - Attachment is obsolete: true
(Assignee)

Comment 21

6 years ago
Created attachment 558943 [details] [diff] [review]
Style inspector registerTools patch 13

Updated patch to reflect the new API. All tests passing.
Attachment #558608 - Attachment is obsolete: true
Attachment #558943 - Flags: review?(gavin.sharp)
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. :)

Updated

6 years ago
Blocks: 650794
(Assignee)

Comment 23

6 years ago
Created attachment 561452 [details] [diff] [review]
Style inspector registerTools patch 14

cleaned and polished. Removed debugging chatter.
Attachment #558943 - Attachment is obsolete: true
Attachment #558943 - Flags: review?(gavin.sharp)
Attachment #561452 - Flags: review?(gavin.sharp)
(Assignee)

Comment 24

6 years ago
Created attachment 561464 [details] [diff] [review]
[in-fx-team] Style inspector registerTools patch 14 (requires patch from bug 687854)
Attachment #561464 - Flags: review?(gavin.sharp)
Attachment #561452 - Flags: review?(gavin.sharp)
Attachment #561464 - Flags: review?(gavin.sharp)
(Assignee)

Updated

6 years ago
Whiteboard: [minotaur][styleinspector][has-patch] → [minotaur][styleinspector][fixed-in-fx-team]
(Assignee)

Updated

6 years ago
Attachment #561452 - Attachment is obsolete: true
(Assignee)

Comment 25

6 years ago
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
Attachment #561464 - Attachment description: Style inspector registerTools patch 14 (requires patch from bug 687854) → [in-fx-team] Style inspector registerTools patch 14 (requires patch from bug 687854)
(Assignee)

Comment 26

6 years ago
https://hg.mozilla.org/mozilla-central/rev/f1409901573a
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

6 years ago
Blocks: 689164
https://hg.mozilla.org/mozilla-central/rev/264e504e0ce9
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [minotaur][styleinspector][fixed-in-fx-team] → [minotaur][styleinspector]

Comment 29

6 years ago
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
Status: RESOLVED → VERIFIED
Whiteboard: [minotaur][styleinspector] → [minotaur][styleinspector][testday-20111125]
You need to log in before you can comment on or make changes to this bug.