Closed
Bug 1144782
Opened 8 years ago
Closed 8 years ago
LoopUI should listen to "TabSelect" rather than the "select" event
Categories
(Hello (Loop) :: Client, defect)
Hello (Loop)
Client
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla39
People
(Reporter: dao, Assigned: johnkang.h, Mentored)
References
Details
(Whiteboard: [good first bug][lang=js])
Attachments
(2 files, 2 obsolete files)
2.52 KB,
patch
|
dao
:
review+
|
Details | Diff | Splinter Review |
888 bytes,
patch
|
dao
:
review+
|
Details | Diff | Splinter Review |
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•8 years ago
|
||
(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?
Reporter | ||
Comment 2•8 years ago
|
||
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.
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)
Reporter | ||
Comment 5•8 years ago
|
||
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)
Reporter | ||
Comment 7•8 years ago
|
||
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
Reporter | ||
Comment 9•8 years ago
|
||
Comment on attachment 8581182 [details] [diff] [review] bug1144782.diff Looks good :)
Attachment #8581182 -
Flags: review+
Reporter | ||
Comment 10•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/86f331b93e30
Assignee: nobody → johnkang.h
Comment 11•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/86f331b93e30
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Comment 12•8 years ago
|
||
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?
Assignee | ||
Comment 13•8 years ago
|
||
Great! May I ask for a vouch for Mozillians? My username is jkang8 and my email is johnkang.h@gmail.com.
Assignee | ||
Comment 14•8 years ago
|
||
Oops, my account is at https://mozillians.org/en-US/u/jkang8/
Reporter | ||
Comment 15•8 years ago
|
||
(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.
Reporter | ||
Comment 16•8 years ago
|
||
(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)
Assignee | ||
Comment 17•8 years ago
|
||
Oops, my mistake! Here's the fix.
Attachment #8581182 -
Attachment is obsolete: true
Flags: needinfo?(johnkang.h)
Reporter | ||
Updated•8 years ago
|
Attachment #8581182 -
Attachment is obsolete: false
Reporter | ||
Updated•8 years ago
|
Attachment #8582133 -
Attachment description: bug1144782_fix.diff → typo fix
Attachment #8582133 -
Flags: review+
Reporter | ||
Comment 18•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/983493507fe4
You need to log in
before you can comment on or make changes to this bug.
Description
•