Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Reporter
	Description • 13 years ago

When InspectorUI.registerTool() is complete we should plug in the Style Inspector

	Rob Campbell [:rc] (:robcee) Assignee
	Comment 2 • 13 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+

	Justin Dolske [:Dolske]
	Comment 3 • 13 years ago

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?

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Reporter
	Comment 4 • 13 years ago

Rebased and made size calculation according to screen size.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Reporter
	Comment 5 • 13 years ago

> 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?

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Reporter
	Comment 6 • 13 years ago

Sorry, ignore my comment above ... wrong bug

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Reporter
	Comment 7 • 13 years ago

Rebased and moved test patch over to bug 660606

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Reporter
	Comment 9 • 13 years ago

Rebased

	Mihai Sucan [:msucan]
	Comment 10 • 13 years ago

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. :)

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Reporter
	Comment 11 • 13 years ago

Not any more

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Reporter
	Comment 12 • 13 years ago

Rebased

	Justin Dolske [:Dolske]
	Comment 13 • 13 years ago

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! :)

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Reporter
	Comment 14 • 13 years ago

(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

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Reporter
	Comment 15 • 13 years ago

I was asked to make some changes and base this upon robcee's changes to the registerTools API (bug 681653)

	Rob Campbell [:rc] (:robcee) Assignee
	Comment 17 • 13 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.

	Rob Campbell [:rc] (:robcee) Assignee
	Comment 18 • 13 years ago

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.

	Rob Campbell [:rc] (:robcee) Assignee
	Comment 19 • 13 years ago

updated fiddly bits.

	Rob Campbell [:rc] (:robcee) Assignee
	Comment 21 • 13 years ago

Updated patch to reflect the new API. All tests passing.

	Mihai Sucan [:msucan]
	Comment 22 • 13 years ago

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. :)

	Rob Campbell [:rc] (:robcee) Assignee
	Comment 23 • 13 years ago

cleaned and polished. Removed debugging chatter.

	Rob Campbell [:rc] (:robcee) Assignee
	Comment 25 • 13 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

	Rob Campbell [:rc] (:robcee) Assignee
	Comment 26 • 13 years ago

https://hg.mozilla.org/mozilla-central/rev/f1409901573a

	Dão Gottwald [:dao]
	Comment 27 • 13 years ago

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.

	Tim Taubert [:ttaubert] (inactive)
	Comment 28 • 13 years ago

https://hg.mozilla.org/mozilla-central/rev/264e504e0ce9

	Teodosia Pop
	Comment 29 • 13 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