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

RESOLVED FIXED in mozilla39

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dao, Assigned: johnkang.h, Mentored)

Tracking

unspecified
mozilla39
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(2 attachments, 2 obsolete attachments)

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.
(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?
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.
Posted file bug1144782.diff (obsolete) —
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.
Flags: needinfo?(dao)
Attachment #8581095 - Flags: review?(dao)
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.
>      *
Attachment #8581095 - Attachment is obsolete: true
Attachment #8581095 - Attachment is patch: false
Attachment #8581095 - Flags: review?(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
Flags: needinfo?(dao)
Posted file bug1144782.diff (obsolete) —
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".
Oops, I corrected it to TabSelect. I changed the one you just mentioned too. Is there anything else?
Attachment #8581170 - Attachment is obsolete: true
Attachment #8581170 - Attachment is patch: false
Comment on attachment 8581182 [details] [diff] [review]
bug1144782.diff

Looks good :)
Attachment #8581182 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/86f331b93e30
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Blocks: 1146252
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?
Great! May I ask for a vouch for Mozillians? My username is jkang8 and my email is johnkang.h@gmail.com.
Oops, my account is at https://mozillians.org/en-US/u/jkang8/
(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.
(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?
Flags: needinfo?(johnkang.h)
Posted patch typo fixSplinter Review
Oops, my mistake! Here's the fix.
Attachment #8581182 - Attachment is obsolete: true
Flags: needinfo?(johnkang.h)
Attachment #8581182 - Attachment is obsolete: false
Attachment #8582133 - Attachment description: bug1144782_fix.diff → typo fix
Attachment #8582133 - Flags: review+
You need to log in before you can comment on or make changes to this bug.