Last Comment Bug 660606 - Highlighter should allow registration of developer tools
: Highlighter should allow registration of developer tools
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 8
Assigned To: Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe]
:
Mentors:
Depends on: 684803
Blocks: 663831 663834 666650
  Show dependency treegraph
 
Reported: 2011-05-30 03:57 PDT by Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe]
Modified: 2011-09-06 01:39 PDT (History)
5 users (show)
gavin.sharp: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Register tools patch (20.05 KB, patch)
2011-06-15 06:21 PDT, Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe]
rcampbell: review-
Details | Diff | Review
Register tools patch 2 (17.67 KB, patch)
2011-06-27 04:37 PDT, Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe]
rcampbell: review+
Details | Diff | Review
Register tools patch 3 (18.15 KB, patch)
2011-06-27 10:17 PDT, Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe]
dolske: review+
Details | Diff | Review
test fix (1.75 KB, patch)
2011-06-28 09:17 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Review
[bounced] Register tools patch 4 - rebased (18.19 KB, patch)
2011-07-06 02:56 PDT, Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe]
no flags Details | Diff | Review
Register tools patch 5 - rebased (18.24 KB, patch)
2011-07-07 09:30 PDT, Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe]
no flags Details | Diff | Review
Register tools patch 6 (18.23 KB, patch)
2011-07-08 01:19 PDT, Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe]
no flags Details | Diff | Review
Register tools patch 7 (18.10 KB, patch)
2011-07-18 09:56 PDT, Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe]
no flags Details | Diff | Review
Register tools patch 8 (19.09 KB, patch)
2011-07-19 03:12 PDT, Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe]
no flags Details | Diff | Review
Register tools patch 9 (18.10 KB, patch)
2011-07-20 01:11 PDT, Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe]
no flags Details | Diff | Review
[in-fx-team] Register tools patch 10 (18.23 KB, patch)
2011-07-26 07:19 PDT, Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe]
no flags Details | Diff | Review

Description Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe] 2011-05-30 03:57:56 PDT
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] 2011-05-30 04:03:42 PDT
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] 2011-05-30 04:06:52 PDT
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 Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe] 2011-05-30 06:55:25 PDT
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) 2011-05-30 08:22:19 PDT
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] 2011-05-30 08:37:13 PDT
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 Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe] 2011-06-08 09:08:01 PDT
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] 2011-06-08 09:18:36 PDT
(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 Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe] 2011-06-15 06:19:07 PDT
We have split the docking behaviour off to another bug 663834.
Comment 9 Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe] 2011-06-15 06:21:04 PDT
Created attachment 539489 [details] [diff] [review]
Register tools patch
Comment 10 Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe] 2011-06-15 06:27:03 PDT
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) 2011-06-24 09:41:22 PDT
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 Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe] 2011-06-27 04:37:12 PDT
Created attachment 542122 [details] [diff] [review]
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) 2011-06-27 07:48:40 PDT
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 Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe] 2011-06-27 10:17:22 PDT
Created attachment 542190 [details] [diff] [review]
Register tools patch 3
Comment 15 Rob Campbell [:rc] (:robcee) 2011-06-28 09:04:21 PDT
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) 2011-06-28 09:17:22 PDT
Created attachment 542499 [details] [diff] [review]
test fix

necessary fix for applying on top of bug 664436 and bug 663898.
Comment 17 Mihai Sucan [:msucan] 2011-06-29 11:17:02 PDT
nit: the patch has trailing white spaces.
Comment 18 Justin Dolske [:Dolske] 2011-06-29 18:32:37 PDT
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 Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe] 2011-07-06 02:56:30 PDT
Created attachment 544174 [details] [diff] [review]
[bounced] Register tools patch 4 - rebased

Rebased patch and in reply to Dolske ... no leaks.
Comment 20 Rob Campbell [:rc] (:robcee) 2011-07-06 07:33:03 PDT
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) 2011-07-06 10:48:34 PDT
Comment on attachment 544174 [details] [diff] [review]
[bounced] Register tools patch 4 - rebased

http://hg.mozilla.org/integration/fx-team/rev/420d04706fef
Comment 23 Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe] 2011-07-06 12:39:14 PDT
robcee: Did you include your test fix?
Comment 24 Rob Campbell [:rc] (:robcee) 2011-07-07 06:01:32 PDT
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) 2011-07-07 06:47:35 PDT
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 Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe] 2011-07-07 09:30:59 PDT
Created attachment 544517 [details] [diff] [review]
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) 2011-07-07 13:12:44 PDT
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 Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe] 2011-07-08 01:19:42 PDT
Created attachment 544747 [details] [diff] [review]
Register tools patch 6

Done
Comment 29 Rob Campbell [:rc] (:robcee) 2011-07-13 17:34:23 PDT
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) 2011-07-13 17:50:20 PDT
I ran this with the "test fix" patch applied and it still fails locally.
Comment 31 Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe] 2011-07-18 09:56:00 PDT
Created attachment 546561 [details] [diff] [review]
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 Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe] 2011-07-19 03:12:39 PDT
Created attachment 546739 [details] [diff] [review]
Register tools patch 8
Comment 33 Rob Campbell [:rc] (:robcee) 2011-07-19 08:17:27 PDT
has this one made it through try successfully? I'm not pushing this without a successful run.
Comment 34 Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe] 2011-07-19 08:27:43 PDT
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 Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe] 2011-07-20 01:11:50 PDT
Created attachment 547004 [details] [diff] [review]
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) 2011-07-20 13:51:12 PDT
yeah, those results look promising. I think we can land this!
Comment 37 Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe] 2011-07-26 07:19:21 PDT
Created attachment 548441 [details] [diff] [review]
[in-fx-team] Register tools patch 10
Comment 38 Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe] 2011-07-26 07:19:45 PDT
^ Rebased
Comment 39 Rob Campbell [:rc] (:robcee) 2011-07-26 09:04:30 PDT
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: gavin@gavinsharp.com] 2011-07-28 09:56:55 PDT
http://hg.mozilla.org/mozilla-central/rev/fe060dc4ee3d

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