Closed Bug 681653 Opened 13 years ago Closed 13 years ago

Augment RegisterTools API in Highlighter to deregister tools

Categories

(DevTools :: General, defect, P1)

x86
macOS
defect

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.
Blocks: 650794
No longer depends on: 650794
Whiteboard: [minotaur]
Attached patch augment registerTools API (obsolete) — Splinter Review
Assignee: nobody → rcampbell
Status: NEW → ASSIGNED
Attachment #555417 - Flags: review?(mihai.sucan)
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-
No, I didn't mean for this to land alone. The registerTools test needs to be updated to cover the additional API.
(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...
Attached patch augment registerTools API 2 (obsolete) — Splinter Review
updated incorporating suggestions from review. Fixed tests. All passing.

Thanks!
Attachment #555417 - Attachment is obsolete: true
Attachment #555732 - Flags: review?(mihai.sucan)
Attached patch augment registerTools API 3 (obsolete) — Splinter Review
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 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+
(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 on attachment 555734 [details] [diff] [review]
augment registerTools API 3

requesting browser peer review.
Attachment #555734 - Flags: review?(gavin.sharp)
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-
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!
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().
Priority: -- → P1
Taking this to speed up style inspector bugs
Assignee: rcampbell → mratcliffe
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
(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)
Attached patch augment registerTools API 4 WIP (obsolete) — Splinter Review
Attachment #555734 - Attachment is obsolete: true
Attached patch augment registerTools API 5 WIP (obsolete) — Splinter Review
Attachment #558472 - Attachment is obsolete: true
Blocks: 684803
Attached patch augment registerTools API 6 (obsolete) — Splinter Review
all tests are passing now.
Attachment #558606 - Attachment is obsolete: true
Attachment #558942 - Flags: review?(gavin.sharp)
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.
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.
No longer blocks: 650794
Attached patch augment registerTools API 7 (obsolete) — Splinter Review
cleaned, polished and rebased.
Attachment #558942 - Attachment is obsolete: true
Attachment #558942 - Flags: review?(gavin.sharp)
Attachment #561453 - Flags: review?(gavin.sharp)
technically obsoletes previous patch. Removed btn.setAttribute("type", "radio") from the registerTools API. Doesn't work.
Attachment #561465 - Flags: review?(gavin.sharp)
Attachment #561465 - Attachment is patch: true
Attachment #561465 - Flags: review?(gavin.sharp)
Attachment #561453 - Flags: review?(gavin.sharp)
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)
Whiteboard: [minotaur] → [minotaur][fixed-in-fx-team]
Attachment #561453 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/ce51db254f18
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
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 → ---
relanded in fx-team:

https://hg.mozilla.org/integration/fx-team/rev/168f1c6b69dc
Status: REOPENED → ASSIGNED
Target Milestone: Firefox 9 → ---
https://hg.mozilla.org/mozilla-central/rev/168f1c6b69dc
Status: ASSIGNED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Whiteboard: [minotaur][fixed-in-fx-team] → [minotaur]
Target Milestone: --- → Firefox 9
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: