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

RESOLVED FIXED in Firefox 6

Status

Firefox Graveyard
Panorama
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

Trunk
Firefox 6
All
Linux
Bug Flags:
in-testsuite ?

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
See also bug 618470.
Assignee: nobody → tim.taubert
See Also: → bug 618470
(Assignee)

Comment 2

6 years ago
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.
Attachment #529062 - Flags: review?(ehsan)
(Assignee)

Comment 3

6 years ago
Created attachment 529063 [details] [diff] [review]
patch v1 (without white space changes)

Same patch as above, without white space changes.
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.
(Assignee)

Comment 5

6 years ago
Created attachment 529069 [details] [diff] [review]
patch v2

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

Looks much better, thanks :)
Attachment #529062 - Attachment is obsolete: true
Attachment #529062 - Flags: review?(ehsan)
Attachment #529069 - Flags: review?(ehsan)
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
Attachment #529069 - Flags: review?(ehsan) → review+
(Assignee)

Comment 7

6 years ago
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!
Attachment #529063 - Attachment is obsolete: true
Attachment #529069 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
(Assignee)

Comment 8

6 years ago
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
Keywords: checkin-needed
Whiteboard: [fixed in cedar]

Comment 9

6 years ago
http://hg.mozilla.org/mozilla-central/rev/9a959f4d17e6
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [fixed in cedar]
Target Milestone: --- → Firefox 6
Backed out for possibly causing a frequent mochitest-other tabview test failure on Windows debug:
http://hg.mozilla.org/mozilla-central/rev/359643f58412
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 11

6 years ago
Setting to checkin-needed again as this was not causing the m-oth orange.
Keywords: checkin-needed
Re-landed: http://hg.mozilla.org/mozilla-central/rev/af5c044d29ac
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Keywords: checkin-needed
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.