Closed
Bug 1489552
Opened 6 years ago
Closed 6 years ago
Let BrowserWindowTracker.getTopWindow use the module's internal window list instead of nsIWindowMediator::getZOrderDOMWindowEnumerator / getMostRecentWindow
Categories
(Firefox :: General, enhancement, P1)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 64
Tracking | Status | |
---|---|---|
firefox64 | --- | fixed |
People
(Reporter: dao, Assigned: dao)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
25.80 KB,
patch
|
nhnt11
:
review+
|
Details | Diff | Splinter Review |
Assignee | ||
Comment 1•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=deb80668fcfeb0f394f932a08930ab8836ead01f
Attachment #9007276 -
Attachment is obsolete: true
Attachment #9007447 -
Flags: review?(nhnt11)
Assignee | ||
Comment 2•6 years ago
|
||
Some context: BrowserWindowTracker's internal window tracking was added in bug 1034036, and this is the followup mentioned in bug 1034036 comment 112.
Comment 3•6 years ago
|
||
Comment on attachment 9007447 [details] [diff] [review] patch v2 Review of attachment 9007447 [details] [diff] [review]: ----------------------------------------------------------------- These are some nice changes, thanks! I had some minor comments already and would like to look at the captive portal stuff again when I have some more time/on Monday. ::: browser/base/content/browser.js @@ +1558,5 @@ > } > } > }); > > + CaptivePortalWatcher.delayedStartup(); Instead of this, I wonder if it's possible to simply do CaptivePortalWatcher.init() here instead of where we do it currently, that would eliminate the need for the delayedStartup() function in CaptivePortalWatcher. ::: browser/base/content/test/captivePortal/head.js @@ +135,1 @@ > * before this notification has a chance to fire, CaptivePortalWatcher picks s/notification/event/g in this comment. @@ +149,4 @@ > }); > } > > async function closeWindowAndWaitForXulWindowVisible(win) { Let's rename this function to "closeWindowAndWaitForWindowActivate" or something. ::: browser/modules/BrowserWindowTracker.jsm @@ +181,1 @@ > * This means that the lastly focused window will the first item that is yielded. s/that is yielded/in the array/ ::: browser/modules/test/browser/browser_BrowserWindowTracker.js @@ +79,5 @@ > > add_task(async function test_orderedWindows() { > await withOpenWindows(10, async function(windows) { > + Assert.equal(BrowserWindowTracker.windowCount, 11, > + "Number of tracked windows, including the test wind"); s/wind/window/ ?
Attachment #9007447 -
Flags: review?(nhnt11) → review-
Assignee | ||
Comment 4•6 years ago
|
||
(In reply to Nihanth Subramanya [:nhnt11] from comment #3) > Instead of this, I wonder if it's possible to simply do > CaptivePortalWatcher.init() here instead of where we do it currently, that > would eliminate the need for the delayedStartup() function in > CaptivePortalWatcher. This breaks tests...
Attachment #9007447 -
Attachment is obsolete: true
Attachment #9007462 -
Flags: review?(nhnt11)
Comment 5•6 years ago
|
||
(In reply to Dão Gottwald [::dao] from comment #4) > Created attachment 9007462 [details] [diff] [review] > patch v3 > > (In reply to Nihanth Subramanya [:nhnt11] from comment #3) > > Instead of this, I wonder if it's possible to simply do > > CaptivePortalWatcher.init() here instead of where we do it currently, that > > would eliminate the need for the delayedStartup() function in > > CaptivePortalWatcher. > > This breaks tests... Ah, that's unfortunate... probably needs more refactoring and is out of the scope of this bug.
Comment 6•6 years ago
|
||
Comment on attachment 9007462 [details] [diff] [review] patch v3 Review of attachment 9007462 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!
Attachment #9007462 -
Flags: review?(nhnt11) → review+
Pushed by dgottwald@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/354003262d9a Let BrowserWindowTracker.getTopWindow use the module's internal window list instead of nsIWindowMediator::getZOrderDOMWindowEnumerator / getMostRecentWindow. r=nhnt11
Comment 8•6 years ago
|
||
This was something I kinda waved off as 'too complicated', but I'm very happy you did it, Dão! Very cool :-)
Comment 9•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/354003262d9a
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Comment 10•6 years ago
|
||
some perf wins here: == Change summary for alert #15791 (as of Tue, 11 Sep 2018 00:37:48 GMT) == Improvements: 47% tp5n main_normal_fileio windows7-32 opt e10s stylo 1,968,405.83 -> 1,051,278.67 15% tp5n time_to_session_store_window_restored_ms windows7-32 pgo e10s stylo 1,071.59 -> 915.15 14% tp5n time_to_session_store_window_restored_ms windows7-32 opt e10s stylo 1,106.99 -> 952.97 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=15791
Comment 11•5 years ago
|
||
I think this causes the regression I’m currently observing. Basically, links from external applications open in a window other than the one most recently active. Versions of software other than Firefox: * Ubuntu 16.04 * i3 window manager 4.11 The use of i3 may or may not be relevant but i3 is a tiling manager and I think it internally marks windows minimized when they are not visible. In my reproduction steps below, both Firefox windows and the terminal window belong to the same stacked container, so activating Firefox window A makes Firefox window B invisible; activating the terminal makes both Firefox windows invisible. To reproduce: 1. Close all Firefox windows. 2. Open a terminal window. 3. Start Firefox with a clean profile. 4. In the initial window, open about:config. 5. Press Ctrl+N to open a new window. 6. In the window opened in step 5, open about:preferences. 7. Switch to the terminal window. 8. Enter the command: $ firefox http://google.com/ . Note in which window it opened. 9. Press Ctrl+W to close the Google tab. 10. Switch to the initial window containing about:config. 11. Switch to the terminal window. 12. Repeat the command: $ firefox http://google.com/ . Note in which window it opened. Expected behavior: * In step 8, the new tab opens in the last active window, which was the one with about:preferences (created in step 5). * In step 12, the new tab opens in the last active window, which was the one with about:config (last activated in step 10). Observed behavior: * In both steps, the new tab opens in the same window; or the window in which the tab opens is unpredictable. Regression range: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b38e10e9dcf8008b7f33cc5cef8e695d5a0bf98f&tochange=354003262d9a9091304f2c4b0b8ac7df25cb8fe9 Additional observations, and why this is important for me: * With the new behavior, Firefox seems to favor currently visible windows when opening external links. * On my desktop at my job, a typical window layout is: ** One Firefox window on the left monitor, behind a code editor or terminal. ** One or more Firefox windows on the right monitor, behind the mail client. ** One very small Firefox window displaying Slack, always visible. * As a direct consequence of the above, links from the mail client tend to open in the very small window dedicated to Slack. A better solution would let me configure which links (by domain or regexp) are opened in which windows, but for now it would just be very nice to fix the regression.
Comment 13•5 years ago
|
||
(In reply to Dão Gottwald [::dao] from comment #12) > Could you please file a new bug? Filed bug 1509847 for regression.
Flags: needinfo?(yurivkhan)
You need to log in
before you can comment on or make changes to this bug.
Description
•