660606 - Highlighter should allow registration of developer tools

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Tools should be able to register themselves with the highlighter so that as nodes are highlighted the tools can be passed the node. I would suggest something like:

InspectorUI.registerTool("StylePanel", icon, onSelect, onShow, onHide);

Where:
- "StylePanel" is the localized string name for the button
- icon is an optional button icon
- onSelect is a method that will be passed a node on node select
- onShow is the tools show method
- onHide is the tools hide method

I suppose all of these params should be optional.

Of course it is still up to the tools to call InspectorUI.inspectNode(node) then they need to change the highlighted element.

Comment 1

[Cedric Vivier [:cedricv]]

As you(?) pointed out elsewhere, we might better pass an object perhaps.

Makes the registerTool API more extensible and explicit (order of multiple [optional] arguments can quickly become messy/hard to remember)

Comment 2

[Cedric Vivier [:cedricv]]

Including name and icon so that we can add optional things like "tooltip" (and while not having an icon).

InspectorUI.registerTool({
  name: "New feature X",
  tooltip: "This is the super cool new feature X",
  onSelect: FeatureX.onSelect
});

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

We just need to be sure to cover all of the necessary params:
- name: the name of the localized string for the button's label
- tooltip: the name of the localized string for the button's tooltip
- icon: optional button icon
- onSelect: tool's method that will be passed a node on node highlight
- onShow: tool's show method
- onHide: tool's hide method

I think that covers just about everything we need at this point.

Comment 4

[Rob Campbell [:rc] (:robcee)]

That's the beauty of using a parameter object: we don't have to have all of them initially. :)

I think those are a good start. We might want to include a label: property as well in case we display a text button instead of an icon and it differs from the name. Otherwise, good.

Comment 5

[Cedric Vivier [:cedricv]]

I guess we'd better use tooltipText: rather than just tooltip, so that it better match the xul attribute (principle of least surprise) and that we can later support tooltip: "id_of_a_tooltip_popup_element" if we need to.

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

We plan on the tools being docked with the browser when they are controled by the highlighter. In order to accomplish this we need to pass the dock location (top, right, bottom or left) to the highlighter along with the initial dimensions (only width or height would be needed).

The API will now look something like this?
InspectorUI.registerTool({
  name: "New feature X",         // Tool name
  label: "Button label",         // Button label
  icon: "chrome://somepath.png", // Button Icon
  tooltipText: "This is the super cool new feature X", // Button tooltip
  onSelect: object.method, // Tool's method that will be passed a node on node highlight
  onShow: object.method,   // Tool's show method
  onHide: object.method,   // Tool's hide method
  dockLocation: "top", "right", "bottom" or "left", // Position for the docked tool
  defaultWidth: "200px",   // Either width or height will be needed to dock the tool
  defaultHeight: "400px", // Either width or height will be needed to dock the tool
});

Obviously if 2 panels are docked on the right we need to display them half the height of the window and we should take the largest defaultWidth value.

Comment 7

[Mihai Sucan [:msucan]]

(In reply to comment #6)
> We plan on the tools being docked with the browser when they are controled
> by the highlighter. In order to accomplish this we need to pass the dock
> location (top, right, bottom or left) to the highlighter along with the
> initial dimensions (only width or height would be needed).
> 
> The API will now look something like this?
> InspectorUI.registerTool({
>   name: "New feature X",         // Tool name
>   label: "Button label",         // Button label
>   icon: "chrome://somepath.png", // Button Icon
>   tooltipText: "This is the super cool new feature X", // Button tooltip
>   onSelect: object.method, // Tool's method that will be passed a node on
> node highlight
>   onShow: object.method,   // Tool's show method
>   onHide: object.method,   // Tool's hide method
>   dockLocation: "top", "right", "bottom" or "left", // Position for the
> docked tool
>   defaultWidth: "200px",   // Either width or height will be needed to dock
> the tool
>   defaultHeight: "400px", // Either width or height will be needed to dock
> the tool
> });

This looks good. I would only suggest that defaultWidth/height take numbers without the "px" suffix. I doubt we'll really support other units. ... unless you have specific reasons for having the px unit in the value?

Comment 8

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

We have split the docking behaviour off to another bug 663834.

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Register tools patch

Comment 10

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

The complete API is now:
InspectorUI.registerTools({
  id: "toolname",
  label: "Button label",
  icon: "chrome://somepath.png",
  tooltiptext: "Button tooltip",
  accesskey: "S",
  onSelect: object.method,
  onShow: object.method,
  onHide: object.method,
  context: myTool,
  panel: myTool.panel
})

Comment 11

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 539489 [details] [diff] [review]
Register tools patch

                        class="toolbarbutton-text"
-                       command="Inspector:Inspect"/>
+                       command="Inspector:Inspect"
+                       checked="true"/>

Why do we need to add this to a register tools patch?

--   // open inspector UI
+    // open inspector UI
     this.openTreePanel();

is that extra hyphen some kind of diff bustage? Looks wrong.

+      this.prevWinID = this.winID;

why is this needed?

+    for each (let tool in this.tools) {
+      if (tool.panel)
+        tool.panel.hidePopup();
+    }
+

I see this pattern in a couple of places. Might want to add a toolsDo() method that takes a function as an argument.

+  /**
+   * Register an external tool with the highlighter
+   *
+   * @param aRegObj

nit: Punctuation for your comments! Also, I'd present the format of the configuration object before the parameter name. Also, we should probably refer to this as the "inspector" since this lives in the InspectorUI class.

+    aRegObj.id = aRegObj.id.replace(" ", "").toLowerCase();

that seems a bit funky. Tool ids are not-case-sensitive and should have no spaces. You should mention this explicitly in your method comment.

+      let self = this;

unused?

+      aRegObj.panel
+          .addEventListener("popuphiding", function IUI_toolPanelHiding() {
+        btn.setAttribute("checked", "false");
+      }, false);

This formatting is a bit funny. You want to do:

aRegObj.panel.addEventListener("popuphiding",
  function IUI_toolPanelHiding() {
    ...
  }, false);

same for the following addEventListener. You can put the function declaration on the same line as the addEventListener if you have room (i.e., < 80 chars).

+    btn.addEventListener("click", function IUI_ToolButtonClick(aEvent) {
+      if (btn.getAttribute("checked") == "true") {

that's a bit redundant isn't it?

if (btn.getAttribute("checked")) should suffice.

unittests look decent.

r- for now because of the above, but don't let that discourage you. I think this'll be good with these issues addressed.

Comment 12

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Register tools patch 2

>                         class="toolbarbutton-text"
> -                       command="Inspector:Inspect"/>
> +                       command="Inspector:Inspect"
> +                       checked="true"/>
> 
> Why do we need to add this to a register tools patch?
> 

Nope, removed

> --   // open inspector UI
> +    // open inspector UI
>      this.openTreePanel();
> 
> is that extra hyphen some kind of diff bustage? Looks wrong.
> 

Fixed

> +      this.prevWinID = this.winID;
> 
> why is this needed?
> 

It isn't ... changed

> +    for each (let tool in this.tools) {
> +      if (tool.panel)
> +        tool.panel.hidePopup();
> +    }
> +
> 
> I see this pattern in a couple of places. Might want to add a toolsDo() method that takes a function as an argument.
> 

Done

> +  /**
> +   * Register an external tool with the highlighter
> +   *
> +   * @param aRegObj
> 
> nit: Punctuation for your comments! Also, I'd present the format of the configuration object before the parameter name. Also, we should probably refer to this as the "inspector" since this lives in the InspectorUI class.
> 

Done

> +    aRegObj.id = aRegObj.id.replace(" ", "").toLowerCase();
> 
> that seems a bit funky. Tool ids are not-case-sensitive and should have no spaces. You should mention this explicitly in your method comment.
> 

There is no reason to limit the names. let's allow people to use spaces too. Removed.

> +      let self = this;
> 
> unused?
> 

Removed

> +      aRegObj.panel
> +          .addEventListener("popuphiding", function IUI_toolPanelHiding() {
> +        btn.setAttribute("checked", "false");
> +      }, false);
> 
> This formatting is a bit funny. You want to do:
> 
> aRegObj.panel.addEventListener("popuphiding",
>   function IUI_toolPanelHiding() {
>     ...
>   }, false);
> 
> same for the following addEventListener. You can put the function declaration on the same line as the addEventListener if you have room (i.e., < 80 chars).
> 

I do not like that format but ... done.

> +    btn.addEventListener("click", function IUI_ToolButtonClick(aEvent) {
> +      if (btn.getAttribute("checked") == "true") {
> 
> that's a bit redundant isn't it?
> 
> if (btn.getAttribute("checked")) should suffice.

Then it would always evaluate to true ... attributes are always strings.

Comment 13

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 542122 [details] [diff] [review]
Register tools patch 2

+    this.saveToolState(this.winID);
+    this.toolsDo(function IUI_toolsHide(aTool) {
+      if (aTool.panel) {

nit: you could check that the panel is open before sending the hidePopup(). Probably no real benefit though / hidePopup will do the check for you.

   stopInspecting: function IUI_stopInspecting()
   {
     if (!this.inspecting) {
       return;
     }
 
+
+    document.getElementById("inspector-inspect-toolbutton").checked = false;
     this.detachPageListeners();
     this.inspecting = false;
     if (this.highlighter.node) {

looks like extra whitespace there.

+  saveToolState: function IUI_saveToolState(aWinID)

this and subsequent methods need doc comments.

looks good with those changes.

Comment 14

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Register tools patch 3

Comment 15

[Rob Campbell [:rc] (:robcee)]

one comment, this patch's unittest is currently failing for me on mac. I expect it's starting up too soon.

I can't push to try until this passes.

Comment 16

[Rob Campbell [:rc] (:robcee)]

Attachment: test fix

necessary fix for applying on top of bug 664436 and bug 663898.

Comment 17

[Mihai Sucan [:msucan]]

nit: the patch has trailing white spaces.

Comment 18

[Justin Dolske [:Dolske]]

Comment on attachment 542190 [details] [diff] [review]
Register tools patch 3

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

r+ from a light review. One question...

::: browser/base/content/inspector.js
@@ +1312,5 @@
> +        }, false);
> +      aRegObj.panel.addEventListener("popupshowing",
> +        function IUI_toolPanelShowing() {
> +          btn.setAttribute("checked", "true");
> +        }, false);

Are these event listeners being removed anywhere? I don't see it.

If not, have you ensured this isn't leaking (ie leak-until-shutdown / bloat) upon repeated invocations?

Comment 19

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: [bounced] Register tools patch 4 - rebased

Rebased patch and in reply to Dolske ... no leaks.

Comment 20

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 544174 [details] [diff] [review]
[bounced] Register tools patch 4 - rebased

http://hg.mozilla.org/integration/fx-team/rev/091f9b61d326

Comment 21

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 544174 [details] [diff] [review]
[bounced] Register tools patch 4 - rebased

http://hg.mozilla.org/integration/fx-team/rev/420d04706fef

Comment 22

[Rob Campbell [:rc] (:robcee)]

test failures here: http://tinderbox.mozilla.org/showlog.cgi?log=Fx-Team/1309967730.1309969433.6534.gz&fulltext=1

see:

http://tbpl.mozilla.org/?tree=Fx-Team&rev=091f9b61d326

Comment 23

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

robcee: Did you include your test fix?

Comment 24

[Rob Campbell [:rc] (:robcee)]

no, I thought that test fix was strictly for use over paul's "remove iframe" patch.

It looks like it should fix the problem without it as well though.

I'll try it and see and if it passes here, I'll repush.

Comment 25

[Rob Campbell [:rc] (:robcee)]

tests still fail with the testfix patch applied.

TEST-INFO |  |  - maybe run tests <load:true, focus:true> -- loaded: complete active window: ([object ChromeWindow]) chrome://browser/content/browser.xul focused window: ([object Window]) data:text/html,registertool%20new%20tab%20test%20for%20inspector desired window: ([object Window]) data:text/html,registertool%20new%20tab%20test%20for%20inspector child window: ([object Window]) data:text/html,registertool%20new%20tab%20test%20for%20inspector docshell visible: true
TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/inspector/browser_inspector_registertools.js | Opened second tab
TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/inspector/browser_inspector_registertools.js | Checking panel states 5
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/inspector/browser_inspector_registertools.js | Panel 1 is closed
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/inspector/browser_inspector_registertools.js | Panel 2 is closed
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/inspector/browser_inspector_registertools.js | Panel 3 is closed
TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/inspector/browser_inspector_registertools.js | Closing current tab
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/inspector/browser_inspector_registertools.js | Currently selected node was passed: tool_1
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/inspector/browser_inspector_registertools.js | Currently selected node was passed: tool_3
TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/inspector/browser_inspector_registertools.js | Checking panel states 6
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/inspector/browser_inspector_registertools.js | Panel 1 is open
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/inspector/browser_inspector_registertools.js | Panel 2 is closed
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/inspector/browser_inspector_registertools.js | Panel 3 is open
INFO TEST-END | chrome://mochitests/content/browser/browser/base/content/test/inspector/browser_inspector_registertools.js | finished in 565ms

Comment 26

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Register tools patch 5 - rebased

robcee: you are right, you don't need your test fix patch without Paul's iframe fix.

Seems like something was making this test pass on my PC until I deleted my Firefox source and rebuilt.

It works fine now.

Comment 27

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 544517 [details] [diff] [review]
Register tools patch 5 - rebased

+  selectNode: function BIR_selectNode(aNode) {
+    ok(InspectorUI.selection == aNode,
+       "selectNode: currently selected node was passed: " + this.id);
+  },
+
+  show: function BIR_show(aNode) {
+    this.panel.openPopup(gBrowser.selectedBrowser,
+                         "end_before", 0, 20, false, false);
+    ok(InspectorUI.selection == aNode,
+       "show: currently selected node was passed: " + this.id);
+  },
+

Could you replace these tests with is() instead of ok() functions? Test failures with is() give us better information in the logs.

I'll checkin with those fixed, thanks!

Comment 28

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Register tools patch 6

Done

Comment 29

[Rob Campbell [:rc] (:robcee)]

backed out due to test failures. iframe removal patch landed on central. lol

http://tinderbox.mozilla.org/showlog.cgi?log=Devtools/1310599510.1310600924.985.gz&fulltext=1

Comment 30

[Rob Campbell [:rc] (:robcee)]

I ran this with the "test fix" patch applied and it still fails locally.

Comment 31

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Register tools patch 7

Rob's test patch has now been merged into the registertools patch as the iframe removal is now landed in fx-team.

Comment 32

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Register tools patch 8

Comment 33

[Rob Campbell [:rc] (:robcee)]

has this one made it through try successfully? I'm not pushing this without a successful run.

Comment 34

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

All tests run fine locally but there are some failures on debug builds on the try server. As far as I can see, *none* of the oranges are anything to do with my patch ... just some try server funkiness.

I will ping you when this push finishes. It is at:
http://tbpl.mozilla.org/?tree=Try&rev=de595f697be7

Comment 35

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Register tools patch 9

Updated patch.

Still a couple of oranges but I am convinced that they are not caused by this patch (of course the JP errors should be ignored):
http://tbpl.mozilla.org/?tree=Try&rev=b8425087c95a

Comment 36

[Rob Campbell [:rc] (:robcee)]

yeah, those results look promising. I think we can land this!

Comment 37

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: [in-fx-team] Register tools patch 10

Comment 38

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

^ Rebased

Comment 39

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 548441 [details] [diff] [review]
[in-fx-team] Register tools patch 10

http://hg.mozilla.org/integration/fx-team/rev/eee7a94c44e2

Comment 40

[:Gavin Sharp [email: [e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/fe060dc4ee3d