Last Comment Bug 653681 - TabMatcher._getTabsForOtherWindows() must not use nsIWindowMediator.getMostRecentWindow()
: TabMatcher._getTabsForOtherWindows() must not use nsIWindowMediator.getMostRe...
Status: RESOLVED FIXED
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All Linux
: -- normal
: Firefox 6
Assigned To: Tim Taubert [:ttaubert]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-29 02:26 PDT by Tim Taubert [:ttaubert]
Modified: 2016-04-12 14:00 PDT (History)
3 users (show)
bzbarsky: in‑testsuite?
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1 (2.50 KB, patch)
2011-04-29 02:31 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
patch v1 (without white space changes) (2.15 KB, patch)
2011-04-29 02:32 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
patch v2 (2.46 KB, patch)
2011-04-29 03:31 PDT, Tim Taubert [:ttaubert]
dao+bmo: review+
Details | Diff | Splinter Review
patch for checkin (3.50 KB, patch)
2011-04-29 07:26 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review

Description Tim Taubert [:ttaubert] 2011-04-29 02:26:18 PDT
browser_tabview_multwindow_search.js is constantly failing for me when running tabview tests locally. TabMatcher._getTabsForOtherWindows() uses nsIWindowMediator.getMostRecentWindow() to determine the currently focused (the searching) window. We should just use gWindow here.
Comment 1 Tim Taubert [:ttaubert] 2011-04-29 02:27:59 PDT
See also bug 618470.
Comment 2 Tim Taubert [:ttaubert] 2011-04-29 02:31:54 PDT
Created attachment 529062 [details] [diff] [review]
patch v1

Asking Ehsan for review, to distribute Panorama reviews a bit :)

Note: I'll attach a second patch soon that doesn't contain white space changes because I made some coding style corrections, too.
Comment 3 Tim Taubert [:ttaubert] 2011-04-29 02:32:31 PDT
Created attachment 529063 [details] [diff] [review]
patch v1 (without white space changes)

Same patch as above, without white space changes.
Comment 4 Dão Gottwald [:dao] 2011-04-29 03:25:32 PDT
Comment on attachment 529062 [details] [diff] [review]
patch v1

Review of attachment 529062 [details] [diff] [review]:

::: browser/base/content/tabview/search.js
@@ +224,5 @@
   // least once, while <xul:tab>s will be return for windows in which
   // Panorama has never been activated.
   _getTabsForOtherWindows: function TabMatcher__getTabsForOtherWindows(){
+    var enumerator = Cc["@mozilla.org/appshell/window-mediator;1"]
+                     .getService(Ci.nsIWindowMediator)

You should use Services.wm here.
Comment 5 Tim Taubert [:ttaubert] 2011-04-29 03:31:59 PDT
Created attachment 529069 [details] [diff] [review]
patch v2

(In reply to comment #4)
> You should use Services.wm here.

Looks much better, thanks :)
Comment 6 Dão Gottwald [:dao] 2011-04-29 04:28:55 PDT
Comment on attachment 529069 [details] [diff] [review]
patch v2

Review of attachment 529069 [details] [diff] [review]:

::: browser/base/content/tabview/search.js
@@ +219,4 @@
   // ---------
   // Function: _getTabsForOtherWindows
   // Returns an array of <TabItem>s and <xul:tabs>s representing that
   // tabs from all windows but the currently focused window. <TabItem>s

Please replace "currently focused window" with "current window".

@@ +230,4 @@
     while (enumerator.hasMoreElements()) {
       var win = enumerator.getNext();
       // This function gets tabs from other windows: not the one you currently
       // have focused.

ditto
Comment 7 Tim Taubert [:ttaubert] 2011-04-29 07:26:30 PDT
Created attachment 529091 [details] [diff] [review]
patch for checkin

(In reply to comment #6)
> Please replace "currently focused window" with "current window".

Fixed. Thanks for your review!
Comment 8 Tim Taubert [:ttaubert] 2011-04-29 07:27:36 PDT
Comment on attachment 529091 [details] [diff] [review]
patch for checkin

Forgot to mention that it passed try:

http://tbpl.mozilla.org/?tree=Try&pusher=tim.taubert@gmx.de&rev=4d856e555a4b
Comment 9 Boris Zbarsky [:bz] 2011-05-05 13:34:50 PDT
http://hg.mozilla.org/mozilla-central/rev/9a959f4d17e6
Comment 10 Matt Brubeck (:mbrubeck) 2011-05-05 14:58:04 PDT
Backed out for possibly causing a frequent mochitest-other tabview test failure on Windows debug:
http://hg.mozilla.org/mozilla-central/rev/359643f58412
Comment 11 Tim Taubert [:ttaubert] 2011-05-08 23:55:10 PDT
Setting to checkin-needed again as this was not causing the m-oth orange.
Comment 12 Matt Brubeck (:mbrubeck) 2011-05-09 09:36:29 PDT
Re-landed: http://hg.mozilla.org/mozilla-central/rev/af5c044d29ac

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