1144782 - LoopUI should listen to "TabSelect" rather than the "select" event

Status: RESOLVED FIXED

Description

[Dão Gottwald [::dao]]

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-loop.js

There's code listening to the "select" event on gBrowser. It should instead listen to the "TabSelect" event on gBrowser.tabContainer.

Comment 1

[Mark Banner (:standard8)]

(In reply to Dão Gottwald [:dao] from comment #0)
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-
> loop.js
> 
> There's code listening to the "select" event on gBrowser. It should instead
> listen to the "TabSelect" event on gBrowser.tabContainer.

Hi Dão, please can you explain the difference, and why one is preferred over the other?

Comment 2

[Dão Gottwald [::dao]]

TabSelect is the standard high-level API for reacting to tab switches. select is implemented at a lower level with slightly different semantics and should basically never be used unless you know exactly what you're doing.

Comment 3

[John Kang]

Attachment: bug1144782.diff

Hello! I took a look at browser-loop.js and made some changes and attached a diff, but I'm not exactly sure how to test them. The developer guidelines mention that patches should be tested, whether automated or manually. I apologize if my questions are naive. 

Thanks.

Comment 4

[John Kang]

Comment on attachment 8581095 [details]
bug1144782.diff

>diff --git a/browser/base/content/browser-loop.js b/browser/base/content/browser-loop.js
>--- a/browser/base/content/browser-loop.js
>+++ b/browser/base/content/browser-loop.js
>@@ -358,16 +358,17 @@ XPCOMUtils.defineLazyModuleGetter(this, 
>      *
>      * @param {Function} listener The listener to receive information on when the
>      *                            windowId changes.
>      */
>     addBrowserSharingListener: function(listener) {
>       if (!this._tabChangeListeners) {
>         this._tabChangeListeners = new Set();
>-        gBrowser.addEventListener("select", this);
>+        gBrowser.tabContainer.addEventListener("SelectTab", this);
>       }
> 
>       this._tabChangeListeners.add(listener);
>       this._maybeShowBrowserSharingInfoBar();
> 
>       // Get the first window Id for the listener.
>       listener(null, gBrowser.selectedBrowser.outerWindowID);
>     },
>@@ -383,17 +384,17 @@ XPCOMUtils.defineLazyModuleGetter(this, 
>       }
> 
>       if (this._tabChangeListeners.has(listener)) {
>         this._tabChangeListeners.delete(listener);
>       }
> 
>       if (!this._tabChangeListeners.size) {
>         this._hideBrowserSharingInfoBar();
>-        gBrowser.removeEventListener("select", this);
>+        gBrowser.tabContainer.removeEventListener("SelectTab", this);
>         delete this._tabChangeListeners;
>       }
>     },
> 
>     /**
>      * Helper function to fetch a localized string via the MozLoopService API.
>      * It's currently inconveniently wrapped inside a string of stringified JSON.
>      *

Comment 5

[Dão Gottwald [::dao]]

Looks good so far, but you'll also need to update lines 493 and 494 here to use event.detail.previousTab instead of event.fromTab:
http://hg.mozilla.org/mozilla-central/annotate/2a404169de2d/browser/base/content/browser-loop.js#l493

Comment 6

[John Kang]

Attachment: bug1144782.diff

Comment 7

[Dão Gottwald [::dao]]

Comment on attachment 8581170 [details]
bug1144782.diff

>-        gBrowser.addEventListener("select", this);
>+        gBrowser.tabContainer.addEventListener("SelectTab", this);

"TabSelect", not "SelectTab".

>-        gBrowser.removeEventListener("select", this);
>+        gBrowser.tabContainer.removeEventListener("SelectTab", this);

same here

>     handleEvent: function(event) {
>       // We only should get "select" events.
>       if (event.type != "select") {
>         return;
>       }

Oops, I missed this earlier: You'll also update the above code to look for "TabSelect" rather than "select".

Comment 8

[John Kang]

Attachment: bug1144782.diff

Oops, I corrected it to TabSelect. I changed the one you just mentioned too. Is there anything else?

Comment 9

[Dão Gottwald [::dao]]

Comment on attachment 8581182 [details] [diff] [review]
bug1144782.diff

Looks good :)

Comment 10

[Dão Gottwald [::dao]]

https://hg.mozilla.org/integration/fx-team/rev/86f331b93e30

Comment 11

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/86f331b93e30

Comment 12

[Mike de Boer [:mikedeboer] - PTO until Aug 19th]

John, thanks a lot for fixing this! It was silly of me to forget about the TabSelect event.

Dão, thanks for catching this! Since code is also on Aurora right now, should we consider uplifting this as well, or is it more a code-cosmetic update?

Comment 13

[John Kang]

Great! May I ask for a vouch for Mozillians? My username is jkang8 and my email is [e-mail].

Comment 14

[John Kang]

Oops, my account is at https://mozillians.org/en-US/u/jkang8/

Comment 15

[Dão Gottwald [::dao]]

(In reply to John Kang from comment #13)
> Great! May I ask for a vouch for Mozillians?

Judging by the vouching rules that I just read, I think we're only supposed to vouch for people who've been involved for a little longer. How about you take bug 1146252 too and then I vouch for you? :)

(In reply to Mike de Boer [:mikedeboer] from comment #12)
> Dão, thanks for catching this! Since code is also on Aurora right now,
> should we consider uplifting this as well, or is it more a code-cosmetic
> update?

Uplifting this plus bug 1146252 before add-ons start using the fromTab and toTab properties is probably a good idea.

Comment 16

[Dão Gottwald [::dao]]

(In reply to Dão Gottwald [:dao] from comment #15)
> (In reply to Mike de Boer [:mikedeboer] from comment #12)
> > Dão, thanks for catching this! Since code is also on Aurora right now,
> > should we consider uplifting this as well, or is it more a code-cosmetic
> > update?
> 
> Uplifting this plus bug 1146252 before add-ons start using the fromTab and
> toTab properties is probably a good idea.

Except that I just spotted a typo in the patch... event.detail.previousTabfromTab should have been event.detail.previousTab. John, would you mind submitting a patch that fixes this?

Comment 17

[John Kang]

Attachment: typo fix

Oops, my mistake! Here's the fix.

Comment 18

[Dão Gottwald [::dao]]

https://hg.mozilla.org/integration/fx-team/rev/983493507fe4

Comment 19

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/983493507fe4