Last Comment Bug 663714 - Subscribable API doesn't seem sane
: Subscribable API doesn't seem sane
Status: RESOLVED FIXED
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: -- normal
: Firefox 8
Assigned To: Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
:
Mentors:
Depends on:
Blocks: 660175
  Show dependency treegraph
 
Reported: 2011-06-12 10:52 PDT by Dão Gottwald [:dao]
Modified: 2016-04-12 14:00 PDT (History)
2 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1 (65.95 KB, patch)
2011-07-07 17:27 PDT, Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
dao+bmo: review-
raymond: feedback+
Details | Diff | Splinter Review
patch v2 (66.67 KB, patch)
2011-07-08 05:49 PDT, Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
dao+bmo: review-
Details | Diff | Splinter Review
patch v3 (66.67 KB, patch)
2011-07-08 07:55 PDT, Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
dao+bmo: review+
Details | Diff | Splinter Review
patch v4 (65.88 KB, patch)
2011-07-13 17:25 PDT, Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
dao+bmo: review+
Details | Diff | Splinter Review

Description Dão Gottwald [:dao] 2011-06-12 10:52:53 PDT
addSubscriber registers a callback for a given refObject and eventName. refObject is almost always the object that you're calling addSubscriber on, so it's not clear why that argument is needed. In the cases where it is different from the object you're calling addSubscriber on, it's not clear /why/ it's different. Presumably it's because you can only have one callback for a given refObject, but then refObject shouldn't exist in the first place... Why isn't this a simple eventName -> [callbacks] relation, pretty much like addEventListener / removeEventListener work?
Comment 1 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-06-13 08:29:08 PDT
I think all these arguments are valid - I also had to get used to it when I joined the project. We shouldn't invent a new subscriber system - we should offer one similar to addEventListener() to make contributions and understanding the code somewhat easier.
Comment 2 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-07-07 17:27:21 PDT
Created attachment 544664 [details] [diff] [review]
patch v1
Comment 3 Raymond Lee [:raymondlee] 2011-07-07 20:04:57 PDT
Comment on attachment 544664 [details] [diff] [review]
patch v1

Looks good
Comment 4 Dão Gottwald [:dao] 2011-07-07 23:09:33 PDT
Comment on attachment 544664 [details] [diff] [review]
patch v1

>     var subsCopy = this.subscribers[eventName].concat();
>-    subsCopy.forEach(function(object) {
>+    subsCopy.forEach(function(callback) {
>       try {
>-        object.callback(this, eventInfo);
>+        callback(this, eventInfo);
>       } catch(e) {
>         Utils.log(e);
>       }
>     }, this);

I think forEach already copies the array. I might be wrong.

>--- a/browser/base/content/tabview/ui.js
>+++ b/browser/base/content/tabview/ui.js
>@@ -389,29 +389,32 @@ let UI = {
>   // arrow keys lets you navigate between open tabs.
>   //
>   // Parameters:
>   //  - Takes a <TabItem>
>   _setActiveTab: function UI__setActiveTab(tabItem) {
>     if (tabItem == this._activeTab)
>       return;
> 
>+    let self = this;
>+
>+    function onActiveTabClosed(tabItem) {
>+      if (self._activeTab == tabItem)
>+        self._setActiveTab(null);
>+    }
>+
>     if (this._activeTab) {
>       this._activeTab.makeDeactive();
>-      this._activeTab.removeSubscriber(this, "close");
>+      this._activeTab.removeSubscriber("close", onActiveTabClosed);
>     }
>+
>     this._activeTab = tabItem;
> 
>     if (this._activeTab) {
>-      let self = this;
>-      this._activeTab.addSubscriber(this, "close", function(closedTabItem) {
>-        if (self._activeTab == closedTabItem)
>-          self._setActiveTab(null);
>-      });
>-
>+      this._activeTab.addSubscriber("close", onActiveTabClosed);
>       this._activeTab.makeActive();
>     }
>   },

removeSubscriber doesn't work here, as you're dealing with a different onActiveTabClosed instance. Maybe removeSubscriber should throw to prevent such mistakes.
Comment 5 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-07-08 05:49:55 PDT
Created attachment 544787 [details] [diff] [review]
patch v2

(In reply to comment #4)
> I think forEach already copies the array. I might be wrong.

"If existing elements of the array are changed, or deleted, their value as passed to callback will be the value at the time forEach visits them; elements that are deleted are not visited."

I have some tests failing when removing the .concat() so I guess we still need this.

> removeSubscriber doesn't work here, as you're dealing with a different
> onActiveTabClosed instance.

Fixed.

> Maybe removeSubscriber should throw to prevent such mistakes.

Done.
Comment 6 Dão Gottwald [:dao] 2011-07-08 07:44:19 PDT
Comment on attachment 544787 [details] [diff] [review]
patch v2

>     if (shouldRemoveTabItems.length != toClose.length) {
>       // remove children without the assiciated tab and show the group item
>       shouldRemoveTabItems.forEach(function(child) {
>-        self.remove(child, { dontArrange: true });
>+        self.remove(child, {dontArrange: true, dontRemoveSubscriber: true});

Why should it not be removed here?

>+      if (!options && !options.dontRemoveSubscriber)
>+        item.removeSubscriber("close", this._onChildClose);

Looks like && should be ||.
Comment 7 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-07-08 07:55:34 PDT
Created attachment 544817 [details] [diff] [review]
patch v3

(In reply to comment #6)
> >     if (shouldRemoveTabItems.length != toClose.length) {
> >       // remove children without the assiciated tab and show the group item
> >       shouldRemoveTabItems.forEach(function(child) {
> >-        self.remove(child, { dontArrange: true });
> >+        self.remove(child, {dontArrange: true, dontRemoveSubscriber: true});

When calling GroupItem.destroy() it closes all children in the group. For every child it first removes the subscriber, then closes the tab and if that was successful calls groupItem.remove(child, {dontRemoveSubscriber: true}). child.removeSubscriber() would throw here because the subscriber isn't registered anymore.

> Why should it not be removed here?
> >+      if (!options && !options.dontRemoveSubscriber)
> >+        item.removeSubscriber("close", this._onChildClose);
> 
> Looks like && should be ||.

Oops :/
Comment 8 Dão Gottwald [:dao] 2011-07-08 08:01:27 PDT
> When calling GroupItem.destroy() it closes all children in the group. For
> every child it first removes the subscriber, then closes the tab and if that
> was successful [snip]

Why would it not be successful?
Comment 9 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-07-08 08:03:26 PDT
(In reply to comment #8)
> > When calling GroupItem.destroy() it closes all children in the group. For
> > every child it first removes the subscriber, then closes the tab and if that
> > was successful [snip]
> 
> Why would it not be successful?

Because the page could have an onbeforeunload handler registered and the user cancels the closing of that tab.
Comment 10 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-07-13 17:25:17 PDT
Created attachment 545786 [details] [diff] [review]
patch v4

When running the tests again I noticed lots of assertions fail because we seem to remove the "close" event listener from tabs in a lot more places. We would need to pass the dontRemoveSubscriber flag from destroy()->close()->removeAll()->remove() and I don't know whether that additional flag and noise in the code is really worth it.

IMHO it's totally ok to not throw if you provide "invalid" arguments to .removeSubscriber() as we would behave like .removeEventListener() [1].

[1] https://developer.mozilla.org/en/DOM/element.removeEventListener
Comment 11 Dão Gottwald [:dao] 2011-07-13 17:34:13 PDT
Comment on attachment 545786 [details] [diff] [review]
patch v4

>-      item.removeSubscriber(this, "close");
>+
>+      if (!options || !options.dontRemoveSubscriber)
>+        item.removeSubscriber("close", this._onChildClose);

still need to revert this
Comment 12 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-07-14 11:03:44 PDT
http://hg.mozilla.org/integration/fx-team/rev/6d45f8181b55
Comment 13 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-07-17 09:09:22 PDT
http://hg.mozilla.org/mozilla-central/rev/6d45f8181b55

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