Closed
Bug 681653
Opened 13 years ago
Closed 13 years ago
Augment RegisterTools API in Highlighter to deregister tools
Categories
(DevTools :: General, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 9
People
(Reporter: rcampbell, Assigned: rcampbell)
References
Details
(Whiteboard: [minotaur])
Attachments
(1 file, 7 obsolete files)
The registerTools API in the Highlighter currently doesn't unregister tools that are loaded there. We should augment this with methods to unregister and remove associated listeners.
Assignee | ||
Updated•13 years ago
|
Assignee | ||
Comment 1•13 years ago
|
||
Comment 2•13 years ago
|
||
Comment on attachment 555417 [details] [diff] [review]
augment registerTools API
Review of attachment 555417 [details] [diff] [review]:
-----------------------------------------------------------------
Patch looks fine. I have mostly stylistic comments, see below.
Giving the patch r-, because the registerTools test fails:
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/inspector/browser_inspector_registertools.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/inspector/browser_inspector_registertools.js | Found a tab after previous test timed out: data:text/html,registertool%20tests%20for%20inspector
Perhaps this is intentional, if this patch is not intended to land alone.
The registertools.js test should be updated to also call unregisterTool(), and check results. Or maybe add a new test file specifically for unregisterTool().
::: browser/base/content/inspector.js
@@ +1517,5 @@
> + */
> + getToolbarButtonId: function IUI_createButtonId(anId)
> + {
> + return "inspector-" + anId + "-toolbutton";
> + },
This looks to me like overkill. :) Why would we really need a method to construct the button ID?
@@ +1527,5 @@
> + * @param aCallback Function the click event handler for the button
> + */
> + bindToolButtonEvent: function IUI_bindButtonEvent(aButton, aCallback)
> + {
> + this.toolButtonEvents[aButton.id] = aCallback;
The toolButtonEvents and toolPanelEvents objects are not initialized in this patch. This code likely throws.
@@ +1538,5 @@
> + */
> + unbindToolButtonEvent: function IUI_unbindToolButtonEvent(aButton)
> + {
> + if (!this.toolButtonEvents[aButton.id])
> + return;
Do we want to enforce curly braces for one liners? I would say yes, but then all our reviews should be consistent wrt. code style. I knew inspector.js uses the curly braces for one liners.
@@ +1542,5 @@
> + return;
> +
> + aButton.removeEventListener("click", this.toolButtonEvents[aButton.id], false);
> + this.toolButtonEvents[aButton.id] = null;
> + delete this.toolButtonEvents[aButton.id]
I believe there's no need to set the object to null, then delete. Just delete.
(same comment applies to other places in this patch where this pattern is used: foo = null; delete foo;)
@@ +1554,5 @@
> + * @param aShowingEvent Function
> + */
> + bindToolPanelEvents: function IUI_bindToolPanelEvents(aPanel, aHidingEvent, aShowingEvent)
> + {
> + this.toolPanelEvents[aPanel.id + "popuphiding"] = aHidingEvent;
Why not use a single object that holds all our callbacks for later removal?
For example this.eventHandlers[id + "_" + event]. This could be used here, in the other method, in the bindEditorEvent() method, and so on.
We seem to be adding lots of little objects and methods, for every specific purpose. We should do this in a more unified approach.
@@ +1625,1 @@
> if (btn.getAttribute("checked") == "true") {
Please use btn.hasAttribute("checked").
@@ +1633,5 @@
> + });
> +
> + this.bindToolPanelEvents(aRegObj.panel,
> + function IUI_toolPanelHiding() {
> + btn.setAttribute("checked", "false");
Please do btn.removeAttribute("checked") here.
@@ +1637,5 @@
> + btn.setAttribute("checked", "false");
> + },
> + function IUI_toolPanelShowing() {
> + btn.setAttribute("checked", "true");
> + });
I would inline the bindToolButtonEvent() and bindToolPanelEvents() methods. They are not used by other methods, they are minimal, and it makes for less going back-and-forward for a new comer when he reads the code.
@@ +1650,5 @@
> + unregisterTool: function IUI_unregisterTool(aRegObj)
> + {
> + let button = document.getElementById(this.getToolbarButtonId(aRegObj.id));
> + this.unbindToolButtonEvent(button);
> + this.unbindToolPanelEvents(aRegObj.panel);
Similar to above, I would inline these methods. They take more space in the code with comments, with function () { } wrapping and so on, than they add benefits.
If you believe we should keep them, please let me know.
Attachment #555417 -
Flags: review?(mihai.sucan) → review-
Assignee | ||
Comment 3•13 years ago
|
||
No, I didn't mean for this to land alone. The registerTools test needs to be updated to cover the additional API.
Assignee | ||
Comment 4•13 years ago
|
||
(In reply to Mihai Sucan [:msucan] from comment #2)
> ::: browser/base/content/inspector.js
> @@ +1517,5 @@
> > + */
> > + getToolbarButtonId: function IUI_createButtonId(anId)
> > + {
> > + return "inspector-" + anId + "-toolbutton";
> > + },
>
> This looks to me like overkill. :) Why would we really need a method to
> construct the button ID?
It is a bit silly, but this is what the original registerTools method did to create the button id. If we just the RegObj's ID, there's a potential conflict if we use that same ID for more than one DOM node. I can take it out if you really want it gone.
> @@ +1527,5 @@
> > + * @param aCallback Function the click event handler for the button
> > + */
> > + bindToolButtonEvent: function IUI_bindButtonEvent(aButton, aCallback)
> > + {
> > + this.toolButtonEvents[aButton.id] = aCallback;
>
> The toolButtonEvents and toolPanelEvents objects are not initialized in this
> patch. This code likely throws.
sure did (and caused the test to fail). Added them. I forgot them when extracting this as a separate patch.
> @@ +1538,5 @@
> > + */
> > + unbindToolButtonEvent: function IUI_unbindToolButtonEvent(aButton)
> > + {
> > + if (!this.toolButtonEvents[aButton.id])
> > + return;
>
> Do we want to enforce curly braces for one liners? I would say yes, but then
> all our reviews should be consistent wrt. code style. I knew inspector.js
> uses the curly braces for one liners.
only after others got their hands on inspector.js. ;)
I'll add it here.
> @@ +1542,5 @@
> > + return;
> > +
> > + aButton.removeEventListener("click", this.toolButtonEvents[aButton.id], false);
> > + this.toolButtonEvents[aButton.id] = null;
> > + delete this.toolButtonEvents[aButton.id]
>
> I believe there's no need to set the object to null, then delete. Just
> delete.
OK.
> (same comment applies to other places in this patch where this pattern is
> used: foo = null; delete foo;)
>
> @@ +1554,5 @@
> > + * @param aShowingEvent Function
> > + */
> > + bindToolPanelEvents: function IUI_bindToolPanelEvents(aPanel, aHidingEvent, aShowingEvent)
> > + {
> > + this.toolPanelEvents[aPanel.id + "popuphiding"] = aHidingEvent;
>
> Why not use a single object that holds all our callbacks for later removal?
>
> For example this.eventHandlers[id + "_" + event]. This could be used here,
> in the other method, in the bindEditorEvent() method, and so on.
>
> We seem to be adding lots of little objects and methods, for every specific
> purpose. We should do this in a more unified approach.
/me unifies.
> @@ +1625,1 @@
> > if (btn.getAttribute("checked") == "true") {
>
> Please use btn.hasAttribute("checked").
presumably I should be removing the attribute entirely when unchecking it then?
>
> @@ +1633,5 @@
> > + });
> > +
> > + this.bindToolPanelEvents(aRegObj.panel,
> > + function IUI_toolPanelHiding() {
> > + btn.setAttribute("checked", "false");
>
> Please do btn.removeAttribute("checked") here.
ah. yes. :)
> @@ +1637,5 @@
> > + btn.setAttribute("checked", "false");
> > + },
> > + function IUI_toolPanelShowing() {
> > + btn.setAttribute("checked", "true");
> > + });
>
> I would inline the bindToolButtonEvent() and bindToolPanelEvents() methods.
> They are not used by other methods, they are minimal, and it makes for less
> going back-and-forward for a new comer when he reads the code.
ah ok.
>
> @@ +1650,5 @@
> > + unregisterTool: function IUI_unregisterTool(aRegObj)
> > + {
> > + let button = document.getElementById(this.getToolbarButtonId(aRegObj.id));
> > + this.unbindToolButtonEvent(button);
> > + this.unbindToolPanelEvents(aRegObj.panel);
>
> Similar to above, I would inline these methods. They take more space in the
> code with comments, with function () { } wrapping and so on, than they add
> benefits.
>
> If you believe we should keep them, please let me know.
I've just moved their declarations inside register and unregisterTool methods. This keeps them close to where they're being used and removes unnecessary API from InspectorUI.
cleaning up the unittest and resubmitting...
Assignee | ||
Comment 5•13 years ago
|
||
updated incorporating suggestions from review. Fixed tests. All passing.
Thanks!
Attachment #555417 -
Attachment is obsolete: true
Attachment #555732 -
Flags: review?(mihai.sucan)
Assignee | ||
Comment 6•13 years ago
|
||
Noticed that the test file wasn't hiding its popups and removing them.
Consumers of this API are going to be responsible for cleaning up any created panels registered in this way.
Attachment #555732 -
Attachment is obsolete: true
Attachment #555732 -
Flags: review?(mihai.sucan)
Attachment #555734 -
Flags: review?(mihai.sucan)
Comment 7•13 years ago
|
||
Comment on attachment 555734 [details] [diff] [review]
augment registerTools API 3
Review of attachment 555734 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks for the updates. Patch looks fine, giving it r+!
::: browser/base/content/inspector.js
@@ +1610,5 @@
> + */
> + function unbindToolEvent(aWidget, aEvent)
> + {
> + let toolEvent = aWidget.id + "_" + aEvent;
> + if (!InspectorUI.toolEvents[toolEvent]) {
You could bind the unbinToolEvent() function to the this object, and thus avoid the use of InspectorUI. You could access this.toolEvents.
@@ +1615,5 @@
> + return;
> + }
> +
> + aWidget.removeEventListener(aEvent, InspectorUI.toolEvents[toolEvent], false);
> + delete InspectorUI.toolEvents[toolEvent]
Nit: missing semicolon at the end.
@@ +1622,5 @@
> + unbindToolEvent(button, "click");
> + unbindToolEvent(aRegObj.panel, "popuphiding");
> + unbindToolEvent(aRegObj.panel, "popupshowing");
> + this.toolbar.removeChild(button);
> + delete this.tools[aRegObj.id];
It would make a lot of sense inside this method to call some tool.unregister() callback. The tool needs to be notified when it is unregistered.
Attachment #555734 -
Flags: review?(mihai.sucan) → review+
Comment 8•13 years ago
|
||
(In reply to Rob Campbell [:rc] (robcee) from comment #4)
> (In reply to Mihai Sucan [:msucan] from comment #2)
> > ::: browser/base/content/inspector.js
> > @@ +1517,5 @@
> > > + */
> > > + getToolbarButtonId: function IUI_createButtonId(anId)
> > > + {
> > > + return "inspector-" + anId + "-toolbutton";
> > > + },
> >
> > This looks to me like overkill. :) Why would we really need a method to
> > construct the button ID?
>
> It is a bit silly, but this is what the original registerTools method did to
> create the button id. If we just the RegObj's ID, there's a potential
> conflict if we use that same ID for more than one DOM node. I can take it
> out if you really want it gone.
I would say yes to the removal of the method. Sure, generate any kind of IDs, to avoid conflicts. It's just that having a method that only concatenates three strings into one, to generate a toolbar button ID is silly. :)
Assignee | ||
Comment 9•13 years ago
|
||
Comment on attachment 555734 [details] [diff] [review]
augment registerTools API 3
requesting browser peer review.
Attachment #555734 -
Flags: review?(gavin.sharp)
Comment 10•13 years ago
|
||
Comment on attachment 555734 [details] [diff] [review]
augment registerTools API 3
Seems like these buttons should be type="radio", to get the proper behavior (might also require some button styling changes). Then you can also use .checked rather than messing with the attribute directly, and I think it removes the need for the hiding/showing listeners.
Attachment #555734 -
Flags: review?(gavin.sharp) → review-
Assignee | ||
Comment 11•13 years ago
|
||
buttons definitely need styling changes. We were planning on doing those over in bugs 676253 & 676255.
I'll try out some radio buttons and see if they work better. Thanks!
Comment 12•13 years ago
|
||
It would also be good to be able to pass a destructor in when registering a tool. This way we could simply call aRegObj.destructor() just before the the end of unregisterTool().
Updated•13 years ago
|
Priority: -- → P1
Comment 13•13 years ago
|
||
Taking this to speed up style inspector bugs
Assignee: rcampbell → mratcliffe
Comment 14•13 years ago
|
||
We used to save the tool state. If we unregister tools when you switch to a new tab then the tool author needs to know to register the tool again for the current tab and we need to restore it to it's previous state when you switch back. I need to try to work out why my patches are not working so I will assign this back to Rob.
Assignee: mratcliffe → rcampbell
Comment 15•13 years ago
|
||
(In reply to Michael Ratcliffe from comment #14)
> We used to save the tool state. If we unregister tools when you switch to a
> new tab then the tool author needs to know to register the tool again for
> the current tab and we need to restore it to it's previous state when you
> switch back. I need to try to work out why my patches are not working so I
> will assign this back to Rob.
I would suggest the problem is in IUI_handleEvent(), in switch case "TabSelect" where the code adds a notification observer for CLOSED. In the observer handler openInspectorUI() is invoked, but restoreToolState() is not.
(I might be wrong, didn't test this)
Assignee | ||
Comment 16•13 years ago
|
||
Attachment #555734 -
Attachment is obsolete: true
Assignee | ||
Comment 17•13 years ago
|
||
Attachment #558472 -
Attachment is obsolete: true
Assignee | ||
Comment 18•13 years ago
|
||
all tests are passing now.
Attachment #558606 -
Attachment is obsolete: true
Attachment #558942 -
Flags: review?(gavin.sharp)
Comment 20•13 years ago
|
||
Something we forgot yesterday. In inspector.js:
+ TOOL_SHOWN_SUFFIX: "-inspector-tool-shown",
+ TOOL_HIDDEN_SUFFIX: "-inspector-tool-hidden",
These are unused. Maybe we should just remove them, to let tools handle their own notifications.
Comment 21•13 years ago
|
||
In IUI_registerTool():
- btn.setAttribute("class", "toolbarbutton-text");
+ // btn.setAttribute("class", "toolbarbutton-text");
Why is this change here? Shouldn't this line be removed?
Also, the IUI_toolShow() and IUI_toolHide() functions should have jsdoc comments.
Assignee | ||
Comment 22•13 years ago
|
||
cleaned, polished and rebased.
Attachment #558942 -
Attachment is obsolete: true
Attachment #558942 -
Flags: review?(gavin.sharp)
Attachment #561453 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 23•13 years ago
|
||
technically obsoletes previous patch. Removed btn.setAttribute("type", "radio") from the registerTools API. Doesn't work.
Attachment #561465 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•13 years ago
|
Attachment #561465 -
Attachment is patch: true
Updated•13 years ago
|
Attachment #561465 -
Flags: review?(gavin.sharp)
Updated•13 years ago
|
Attachment #561453 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 24•13 years ago
|
||
Comment on attachment 561465 [details] [diff] [review]
[in-fx-team] augment registerTools API 8 (requires patch from bug 687854)
https://hg.mozilla.org/integration/fx-team/rev/ce51db254f18
Attachment #561465 -
Attachment description: augment registerTools API 8 (requires patch from bug 687854) → [in-fx-team] augment registerTools API 8 (requires patch from bug 687854)
Assignee | ||
Updated•13 years ago
|
Whiteboard: [minotaur] → [minotaur][fixed-in-fx-team]
Assignee | ||
Updated•13 years ago
|
Attachment #561453 -
Attachment is obsolete: true
Assignee | ||
Comment 25•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
Comment 26•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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 27•13 years ago
|
||
relanded in fx-team:
https://hg.mozilla.org/integration/fx-team/rev/168f1c6b69dc
Status: REOPENED → ASSIGNED
Target Milestone: Firefox 9 → ---
Comment 28•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Whiteboard: [minotaur][fixed-in-fx-team] → [minotaur]
Target Milestone: --- → Firefox 9
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•