Last Comment Bug 681653 - Augment RegisterTools API in Highlighter to deregister tools
: Augment RegisterTools API in Highlighter to deregister tools
Status: RESOLVED FIXED
[minotaur]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: x86 Mac OS X
: P1 normal (vote)
: Firefox 9
Assigned To: Rob Campbell [:rc] (:robcee)
:
Mentors:
: 681975 (view as bug list)
Depends on:
Blocks: 663831 684803
  Show dependency treegraph
 
Reported: 2011-08-24 08:26 PDT by Rob Campbell [:rc] (:robcee)
Modified: 2011-09-27 04:23 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
augment registerTools API (6.61 KB, patch)
2011-08-24 08:42 PDT, Rob Campbell [:rc] (:robcee)
mihai.sucan: review-
Details | Diff | Splinter Review
augment registerTools API 2 (11.22 KB, patch)
2011-08-25 07:45 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
augment registerTools API 3 (11.46 KB, patch)
2011-08-25 07:58 PDT, Rob Campbell [:rc] (:robcee)
mihai.sucan: review+
gavin.sharp: review-
Details | Diff | Splinter Review
augment registerTools API 4 WIP (15.46 KB, patch)
2011-09-06 07:38 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
augment registerTools API 5 WIP (16.64 KB, patch)
2011-09-06 14:19 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
augment registerTools API 6 (25.07 KB, patch)
2011-09-07 13:46 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
augment registerTools API 7 (24.68 KB, patch)
2011-09-21 06:44 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
[in-fx-team] augment registerTools API 8 (requires patch from bug 687854) (24.66 KB, patch)
2011-09-21 07:48 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review

Description Rob Campbell [:rc] (:robcee) 2011-08-24 08:26:21 PDT
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.
Comment 1 Rob Campbell [:rc] (:robcee) 2011-08-24 08:42:43 PDT
Created attachment 555417 [details] [diff] [review]
augment registerTools API
Comment 2 Mihai Sucan [:msucan] 2011-08-24 13:17:08 PDT
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.
Comment 3 Rob Campbell [:rc] (:robcee) 2011-08-25 04:50:32 PDT
No, I didn't mean for this to land alone. The registerTools test needs to be updated to cover the additional API.
Comment 4 Rob Campbell [:rc] (:robcee) 2011-08-25 07:14:13 PDT
(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...
Comment 5 Rob Campbell [:rc] (:robcee) 2011-08-25 07:45:00 PDT
Created attachment 555732 [details] [diff] [review]
augment registerTools API 2

updated incorporating suggestions from review. Fixed tests. All passing.

Thanks!
Comment 6 Rob Campbell [:rc] (:robcee) 2011-08-25 07:58:49 PDT
Created attachment 555734 [details] [diff] [review]
augment registerTools API 3

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.
Comment 7 Mihai Sucan [:msucan] 2011-08-26 04:26:03 PDT
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.
Comment 8 Mihai Sucan [:msucan] 2011-08-26 04:30:35 PDT
(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. :)
Comment 9 Rob Campbell [:rc] (:robcee) 2011-08-26 08:15:41 PDT
Comment on attachment 555734 [details] [diff] [review]
augment registerTools API 3

requesting browser peer review.
Comment 10 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-08-26 14:17:52 PDT
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.
Comment 11 Rob Campbell [:rc] (:robcee) 2011-08-29 04:46:14 PDT
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 Michael Ratcliffe [:miker] [:mratcliffe] 2011-08-29 07:22:00 PDT
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().
Comment 13 Michael Ratcliffe [:miker] [:mratcliffe] 2011-08-30 04:14:34 PDT
Taking this to speed up style inspector bugs
Comment 14 Michael Ratcliffe [:miker] [:mratcliffe] 2011-08-30 05:46:51 PDT
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.
Comment 15 Mihai Sucan [:msucan] 2011-08-30 07:53:23 PDT
(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)
Comment 16 Rob Campbell [:rc] (:robcee) 2011-09-06 07:38:36 PDT
Created attachment 558472 [details] [diff] [review]
augment registerTools API 4 WIP
Comment 17 Rob Campbell [:rc] (:robcee) 2011-09-06 14:19:03 PDT
Created attachment 558606 [details] [diff] [review]
augment registerTools API 5 WIP
Comment 18 Rob Campbell [:rc] (:robcee) 2011-09-07 13:46:47 PDT
Created attachment 558942 [details] [diff] [review]
augment registerTools API 6

all tests are passing now.
Comment 19 Rob Campbell [:rc] (:robcee) 2011-09-07 13:49:36 PDT
*** Bug 681975 has been marked as a duplicate of this bug. ***
Comment 20 Mihai Sucan [:msucan] 2011-09-08 04:40:11 PDT
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 Mihai Sucan [:msucan] 2011-09-08 04:55:25 PDT
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.
Comment 22 Rob Campbell [:rc] (:robcee) 2011-09-21 06:44:13 PDT
Created attachment 561453 [details] [diff] [review]
augment registerTools API 7

cleaned, polished and rebased.
Comment 23 Rob Campbell [:rc] (:robcee) 2011-09-21 07:48:56 PDT
Created attachment 561465 [details] [diff] [review]
[in-fx-team] augment registerTools API 8 (requires patch from bug 687854)

technically obsoletes previous patch. Removed btn.setAttribute("type", "radio") from the registerTools API. Doesn't work.
Comment 24 Rob Campbell [:rc] (:robcee) 2011-09-23 15:31:40 PDT
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
Comment 25 Rob Campbell [:rc] (:robcee) 2011-09-23 15:48:39 PDT
https://hg.mozilla.org/mozilla-central/rev/ce51db254f18
Comment 26 Dão Gottwald [:dao] 2011-09-24 03:38:21 PDT
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.
Comment 27 Rob Campbell [:rc] (:robcee) 2011-09-26 10:38:29 PDT
relanded in fx-team:

https://hg.mozilla.org/integration/fx-team/rev/168f1c6b69dc
Comment 28 Tim Taubert [:ttaubert] 2011-09-27 04:23:55 PDT
https://hg.mozilla.org/mozilla-central/rev/168f1c6b69dc

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