Closed
Bug 660606
Opened 14 years ago
Closed 13 years ago
Highlighter should allow registration of developer tools
Categories
(DevTools :: General, defect)
DevTools
General
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 8
People
(Reporter: miker, Assigned: miker)
References
Details
Attachments
(1 file, 10 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
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•14 years ago
|
||
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
});
Assignee | ||
Comment 3•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
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.
Assignee | ||
Comment 6•14 years ago
|
||
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•14 years ago
|
||
(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?
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → mratcliffe
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•14 years ago
|
||
We have split the docking behaviour off to another bug 663834.
Assignee | ||
Comment 9•14 years ago
|
||
Assignee | ||
Comment 10•14 years ago
|
||
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
})
Assignee | ||
Updated•14 years ago
|
Attachment #539489 -
Flags: review?(rcampbell)
Comment 11•14 years ago
|
||
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.
Attachment #539489 -
Flags: review?(rcampbell) → review-
Assignee | ||
Comment 12•14 years ago
|
||
> 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.
Attachment #539489 -
Attachment is obsolete: true
Attachment #542122 -
Flags: review?(rcampbell)
Comment 13•14 years ago
|
||
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.
Attachment #542122 -
Flags: review?(rcampbell) → review+
Assignee | ||
Comment 14•14 years ago
|
||
Attachment #542122 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Attachment #542190 -
Flags: review?(dolske)
Comment 15•14 years ago
|
||
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•14 years ago
|
||
necessary fix for applying on top of bug 664436 and bug 663898.
Comment 17•14 years ago
|
||
nit: the patch has trailing white spaces.
Comment 18•14 years ago
|
||
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?
Attachment #542190 -
Flags: review?(dolske) → review+
Assignee | ||
Comment 19•13 years ago
|
||
Rebased patch and in reply to Dolske ... no leaks.
Attachment #542190 -
Attachment is obsolete: true
Comment 20•13 years ago
|
||
Comment on attachment 544174 [details] [diff] [review]
[bounced] Register tools patch 4 - rebased
http://hg.mozilla.org/integration/fx-team/rev/091f9b61d326
Attachment #544174 -
Attachment description: Register tools patch 4 - rebased → [in-fx-team] Register tools patch 4 - rebased
Updated•13 years ago
|
Whiteboard: [fixed-in-fx-team]
Updated•13 years ago
|
Whiteboard: [fixed-in-fx-team]
Comment 21•13 years ago
|
||
Comment on attachment 544174 [details] [diff] [review]
[bounced] Register tools patch 4 - rebased
http://hg.mozilla.org/integration/fx-team/rev/420d04706fef
Attachment #544174 -
Attachment description: [in-fx-team] Register tools patch 4 - rebased → [bounced] Register tools patch 4 - rebased
Comment 22•13 years ago
|
||
Assignee | ||
Comment 23•13 years ago
|
||
robcee: Did you include your test fix?
Comment 24•13 years ago
|
||
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•13 years ago
|
||
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
Assignee | ||
Comment 26•13 years ago
|
||
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.
Attachment #544174 -
Attachment is obsolete: true
Comment 27•13 years ago
|
||
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 29•13 years ago
|
||
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•13 years ago
|
||
I ran this with the "test fix" patch applied and it still fails locally.
Assignee | ||
Comment 31•13 years ago
|
||
Rob's test patch has now been merged into the registertools patch as the iframe removal is now landed in fx-team.
Attachment #542499 -
Attachment is obsolete: true
Attachment #544747 -
Attachment is obsolete: true
Assignee | ||
Comment 32•13 years ago
|
||
Attachment #546561 -
Attachment is obsolete: true
Comment 33•13 years ago
|
||
has this one made it through try successfully? I'm not pushing this without a successful run.
Assignee | ||
Comment 34•13 years ago
|
||
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
Assignee | ||
Comment 35•13 years ago
|
||
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
Attachment #546739 -
Attachment is obsolete: true
Comment 36•13 years ago
|
||
yeah, those results look promising. I think we can land this!
Assignee | ||
Comment 37•13 years ago
|
||
Attachment #547004 -
Attachment is obsolete: true
Assignee | ||
Comment 38•13 years ago
|
||
^ Rebased
Comment 39•13 years ago
|
||
Comment on attachment 548441 [details] [diff] [review]
[in-fx-team] Register tools patch 10
http://hg.mozilla.org/integration/fx-team/rev/eee7a94c44e2
Attachment #548441 -
Attachment description: Register tools patch 10 → [in-fx-team] Register tools patch 10
Updated•13 years ago
|
Whiteboard: [fixed-in-fx-team]
Comment 40•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 8
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•