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)

enhancement

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)

Some context: BrowserWindowTracker's internal window tracking was added in bug 1034036, and this is the followup mentioned in bug 1034036 comment 112.
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-
Attached patch patch v3Splinter Review
(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)
(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.
Blocks: 1489833
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
This was something I kinda waved off as 'too complicated', but I'm very happy you did it, Dão! Very cool :-)
https://hg.mozilla.org/mozilla-central/rev/354003262d9a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
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
Blocks: 1492396
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.
Could you please file a new bug?
Flags: needinfo?(yurivkhan)
(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)
Depends on: 1509847
Blocks: 1431821
Blocks: 1466735
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: