653681 - TabMatcher._getTabsForOtherWindows() must not use nsIWindowMediator.getMostRecentWindow()

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

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] (inactive)]

See also bug 618470.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 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] (inactive)]

Attachment: patch v1 (without white space changes)

Same patch as above, without white space changes.

Comment 4

[Dão Gottwald [:dao]]

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] (inactive)]

Attachment: patch v2

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

Looks much better, thanks :)

Comment 6

[Dão Gottwald [:dao]]

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] (inactive)]

Attachment: 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] (inactive)]

Comment on attachment 529091 [details] [diff] [review]
patch for checkin

Forgot to mention that it passed try:

http://tbpl.mozilla.org/?tree=Try&pusher=[e-mail]&rev=4d856e555a4b

Comment 9

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/mozilla-central/rev/9a959f4d17e6

Comment 10

[Matt Brubeck (:mbrubeck)]

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] (inactive)]

Setting to checkin-needed again as this was not causing the m-oth orange.

Comment 12

[Matt Brubeck (:mbrubeck)]

Re-landed: http://hg.mozilla.org/mozilla-central/rev/af5c044d29ac