Closed Bug 1034036 Opened 10 years ago Closed 6 years ago

[Session Restore] Load windows by descending z-order

Categories

(Firefox :: Session Restore, defect, P2)

defect

Tracking

()

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: Yoric, Assigned: mikedeboer)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [fxperf:p1])

Attachments

(7 files, 60 obsolete files)

59 bytes, text/x-review-board-request
dao
: review+
Details
59 bytes, text/x-review-board-request
dao
: review+
Details
59 bytes, text/x-review-board-request
dao
: review+
Details
59 bytes, text/x-review-board-request
dao
: review+
Details
59 bytes, text/x-review-board-request
dao
: review+
Details
59 bytes, text/x-review-board-request
dao
: review+
Details
59 bytes, text/x-review-board-request
dao
: review+
Details
To minimize web clutter, we should first load the front-most windows, moving progressively to the background ones.
Yeah that would be nice, we currently however restore the windows in creation order to keep the taskbar order the same. There's a few bugs for that - I'll see if I can find them. The only solution would be to restore in reverse z-order and then use WM APIs to re-order them again, I have no idea if those APIs exist at all.
So there's bug 712763 and bug 669272. This here is a dupe of the latter.
I'm not completely sure I understand your point about taskbar order. Wouldn't it be ok to always restore the most recently used windows so that they initially appear first in the taskbar?
Assignee: nobody → dteller
No, please read the two bugs I linked. There was a reason we reverted the change.
Got it.

So we might need to:
1/ [under Windows only?] open windows in consistent order;
2/ actually load them by decreasing MRU order.

That looks weird.

I haven't found on MSDN an API that would let us reorder windows on the taskbar.
Just fyi: I'm using 7+ Taskbar Tweaker to reorder my windows when they got messed up by session restore again. Unfortunately this isn't persistent, so a once broken session can't be fixed by that (Probably we don't update z-order from the Windows API on save for our internal representation? Separate bug?).
Anyway, the developer recently put up a library for taskbar manipulation, because the documentation from Microsoft really is bad. Unfortunately it's closed source but if you'd like some information I'm pretty sure he can help you:

http://rammichael.com/7-taskbar-tweaking-library
No time to work on this at the moment.

Johannes: thanks, I hadn't seen your comment. If I pick this bug again, I'll get in touch with the developer.
Assignee: dteller → nobody
See Also: → 1331935
Blocks: 1331935
See Also: 1331935
Attached patch z_order.v1.patch (obsolete) — Splinter Review
I tested the patch locally on my window machine. It's hit-and-miss. Sometimes the z-index of windows is correct, sometimes it isn't. Mike, do we have a reliable way to obtain z-index of a window?

I'm not sure what the isFollowUp flag is used for. It looks like it just makes the newly created window in focus. We probably want the previous recently used window in focus so I commented all code snippets relate to isFollowUp.

This patch will mess windows order in taskbar up. But bug 1235231 (cool number :)) will solve that.
Attachment #8880260 - Flags: feedback?(mdeboer)
Comment on attachment 8880260 [details] [diff] [review]
z_order.v1.patch

Review of attachment 8880260 [details] [diff] [review]:
-----------------------------------------------------------------

I see you picked the right components already, now to only get it into the right structure! ;-)

::: browser/components/sessionstore/SessionStore.jsm
@@ +620,4 @@
>    },
>  
>    /**
> +   * Sort windows in reverse z order.

Please also explain briefly _why_ we're doing that.

@@ +625,5 @@
> +   * @param aWindows
> +   *        array of windows that will be sorted
> +   * @returns a flag indicates whether windows array is sorted
> +   */
> +  sortWindowsInReverseZOrder(aWindows) {

nit: arguments to new methods don't need to be prefixed with 'a'; that's an old coding style.

@@ +626,5 @@
> +   *        array of windows that will be sorted
> +   * @returns a flag indicates whether windows array is sorted
> +   */
> +  sortWindowsInReverseZOrder(aWindows) {
> +    let windowsDontHaveZIndex = aWindows.filter((window) => {

This looks a bit confusing, since I expect a boolean flag here. You might as well:

```js
// If there are entries with z-indices, we can sort it.
if (windows.some(window => !!window.zIndex)) {
  //...
}

return false;
```

@@ +2757,5 @@
> +        // the state we're trying to restore and then fallback to the last selected
> +        // window.
> +        windowToUse = windows[lastSessionWindowID];
> +        if (!windowToUse && canUseLastWindow) {
> +          windowToUse = lastWindow;

Please find a way to merge this inside the if-statement above, because it's doing the same thing.

@@ +3051,5 @@
> +      let mostRecentWindow = this._getMostRecentBrowserWindow();
> +      return mostRecentWindow === aWindow ? 1 : 0;
> +    }
> +
> +    let windowList = Services.wm.getZOrderDOMWindowEnumerator("navigator:browser", true);

I think you want to add behavior to cache the most recent browser window and the list of windows this enumerator returns during the run of `_forEachBrowserWindow`, which iterates over all windows.
In fact, it might be quite handy to use this iterator in that method instead and somehow pass the z-index data along with it.

@@ +3519,5 @@
>        return;
>      }
>  
> +    // Keep the focus on the first window.
> +    if (isFirstWindow) {

Yeah, this makes sense. The others won't be correct anymore.

@@ +4238,4 @@
>        }
>      }
>  
> +

nit: superfluous newline.
Attachment #8880260 - Flags: feedback?(mdeboer)
Assignee: nobody → nnn_bikiu0707
Status: NEW → ASSIGNED
(In reply to Mike de Boer [:mikedeboer] from comment #12)
> Comment on attachment 8880260 [details] [diff] [review]
> z_order.v1.patch
> 
> Review of attachment 8880260 [details] [diff] [review]:
> -----------------------------------------------------------------
> 

> @@ +3051,5 @@
> > +      let mostRecentWindow = this._getMostRecentBrowserWindow();
> > +      return mostRecentWindow === aWindow ? 1 : 0;
> > +    }
> > +
> > +    let windowList = Services.wm.getZOrderDOMWindowEnumerator("navigator:browser", true);
> 
> I think you want to add behavior to cache the most recent browser window and
> the list of windows this enumerator returns during the run of
> `_forEachBrowserWindow`, which iterates over all windows.
> In fact, it might be quite handy to use this iterator in that method instead
> and somehow pass the z-index data along with it.
> 

So you want me to use _forEachBrowserWindow in that function. But I think that enumerator doesn't iterate windows in z-index order. Maybe I should modified the behaviour of _forEachBrowserWindow?
Flags: needinfo?(mdeboer)
(In reply to Bao Quan [:beekill] from comment #13)
> So you want me to use _forEachBrowserWindow in that function. But I think
> that enumerator doesn't iterate windows in z-index order. Maybe I should
> modified the behaviour of _forEachBrowserWindow?

Indeed, that's what I meant! :-)
Flags: needinfo?(mdeboer)
Attached patch z_order.v2.patch (obsolete) — Splinter Review
This patch will mess windows taskbar order up. And since bug 1235231 is not going to be an easy fix - (there may be no solution for MacOS and Linux), can I do like this [1]:
+ Open window in creation order
+ Restore tab and data in those windows in reverse z-index

This approach will ensure the taskbar order is the same for all platforms and we don't have to dealt with cross-platform code and C++. However, it's going to be complex and the bug 669272 may not be fixed, at least I don't have anything in mind.

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1034036#c5
Attachment #8880260 - Attachment is obsolete: true
Flags: needinfo?(mdeboer)
Attachment #8882985 - Flags: feedback?(mdeboer)
As per discussion during 1:1, good idea! Let's move forward with separating window and tabs restore in a neat way so that bug 669272 will be an additional improvement we can add later on top of this.
Flags: needinfo?(mdeboer)
Attachment #8882985 - Flags: feedback?(mdeboer)
Attachment #8883887 - Flags: review?(mdeboer) → feedback?(mdeboer)
Comment on attachment 8883887 [details]
Bug 1034036 - Part 1: Separate windows opening and windows restoration process. Make windows restored in reversed z-order.

https://reviewboard.mozilla.org/r/154880/#review159852

::: browser/components/sessionstore/SessionStore.jsm:1260
(Diff revision 1)
>    onBeforeBrowserWindowShown(aWindow) {
>      // Register the window.
>      this.onLoad(aWindow);
>  
> +    // resolve window opened
> +    if (aWindow.__SS_windowOpenedPromise) {

I cannot use function like this [1] to observe for opened window. The observed window is about:blank whereas the expected window is .../browser.xul

[1]: http://searchfox.org/mozilla-central/source/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm#539
With your patch applied, only one window is restored for me. Can you investigate that?
Flags: needinfo?(nnn_bikiu0707)
Attachment #8883887 - Flags: feedback?(mdeboer)
Flags: needinfo?(nnn_bikiu0707)
Attachment #8883887 - Flags: review?(mdeboer) → feedback?(mdeboer)
Comment on attachment 8883887 [details]
Bug 1034036 - Part 1: Separate windows opening and windows restoration process. Make windows restored in reversed z-order.

https://reviewboard.mozilla.org/r/154880/#review164118

This approach is looking very promising when I try it locally! I have a couple of question below - can you please take a look? Thanks!

::: browser/components/sessionstore/SessionStore.jsm:1307
(Diff revision 2)
>          return;
>        }
>  
>        if (this._sessionInitialized) {
>          this.initializeWindow(aWindow);
> +        return;

Please explain the return here with a comment.

::: browser/components/sessionstore/SessionStore.jsm:3027
(Diff revision 2)
> +   *        Window reference
> +   * @return z-index of that window
> +   */
> +  _getZIndex(window) {
> +    let isMinimized = this._getWindowDimension(window, "sizemode") == "minimized";
> +    let zIndex = window.zIndex;

Shouldn't this be `window.__SS_zIndex`?

::: browser/components/sessionstore/SessionStore.jsm:3248
(Diff revision 2)
> +   *
> +   * @param root
> +   *        Windows data
> +   * @returns a promise resolved when all windows is opened
> +   */
> +  openWindows(root) {

I'm curious to know if it makes sense to re-focus the `windowToFocus` once it's opened here, also after opening consecutive windows. Can you think about that?

::: browser/components/sessionstore/SessionStore.jsm:3580
(Diff revision 2)
> -    }
> -
> -    // open new windows for all further window entries of a multi-window session
> -    // (unless they don't contain any tab data)
> -    let winData;
> -    for (var w = 1; w < root.windows.length; w++) {
> +      aWindow.__SS_zIndex = root.windows[0].zIndex;
> +    }
> +
> +    // Store state to restore.
> +    let firstWindowData = root.windows.splice(0, 1);
> +    this._storeRestoreState(aWindow, {windows: firstWindowData});

This is not storing a `windows` array... is that intentional?

::: browser/components/sessionstore/SessionStore.jsm:3582
(Diff revision 2)
> -    // open new windows for all further window entries of a multi-window session
> -    // (unless they don't contain any tab data)
> -    let winData;
> -    for (var w = 1; w < root.windows.length; w++) {
> -      winData = root.windows[w];
> -      if (winData && winData.tabs && winData.tabs[0]) {
> +
> +    // Store state to restore.
> +    let firstWindowData = root.windows.splice(0, 1);
> +    this._storeRestoreState(aWindow, {windows: firstWindowData});
> +
> +    // Store restore options

nit: please make this comment more informative.

::: browser/components/sessionstore/SessionStore.jsm:4177
(Diff revision 2)
>     * (might miss the most recent one)
>     * @param aFunc
>     *        Callback each window is passed to
>     */
>    _forEachBrowserWindow: function ssi_forEachBrowserWindow(aFunc) {
> -    var windowsEnum = Services.wm.getEnumerator("navigator:browser");
> +    let broken_wm_z_order = AppConstants.platform != "macosx" && AppConstants.platform != "win";

Please make this a lazy getter.

::: browser/components/sessionstore/SessionStore.jsm:4183
(Diff revision 2)
> +    let windowsEnum = broken_wm_z_order ?
> +                      Services.wm.getEnumerator("navigator:browser") :
> +                      Services.wm.getZOrderDOMWindowEnumerator("navigator:browser", false);
> +    let mostRecentWindow = broken_wm_z_order ? this._getMostRecentBrowserWindow() : null;
>  
> +    let zIndex = 1;

Why does this start at 1 and not at 0?
Attachment #8883887 - Flags: feedback?(mdeboer) → feedback+
Comment on attachment 8883887 [details]
Bug 1034036 - Part 1: Separate windows opening and windows restoration process. Make windows restored in reversed z-order.

https://reviewboard.mozilla.org/r/154880/#review164118

> Please explain the return here with a comment.

It was my mistake!

> Shouldn't this be `window.__SS_zIndex`?

Yeah, this should be `window.__SS_zIndex`. For some reasons, I though I should use different variables for restoration and for collecting window data.

> I'm curious to know if it makes sense to re-focus the `windowToFocus` once it's opened here, also after opening consecutive windows. Can you think about that?

I think that we shouldn't re-focus `windowToFocus` here as it will be overriden when we enter `restoreWindowsFeaturesAndTabs`.

> This is not storing a `windows` array... is that intentional?

firstWindowData is actually an array [1]

[1]: https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Array/splice

> Why does this start at 1 and not at 0?

Yeah, this should start at 0. I though I should leave some rooms for minimized windows. However, I can use -1 for those.
For testing, I think we could test the following:

Preparation:
+ Open 3 windows, each windows should have two tabs.

Testing:
1. Close all three windows and test whether the z-order is stored correctly.
2. Reopen all windows and the restoration order should be in reversed z-order.
(In reply to Bao Quan [:beekill] from comment #24)
> For testing, I think we could test the following:
> 
> Preparation:
> + Open 3 windows, each windows should have two tabs.
> 
> Testing:
> 1. Close all three windows and test whether the z-order is stored correctly.
> 2. Reopen all windows and the restoration order should be in reversed
> z-order.

Agreed, this sounds like a good test.
Attachment #8883887 - Flags: review?(mdeboer) → review?(dao+bmo)
Comment on attachment 8890264 [details]
Bug 1034036 - Part 3: Tests that use set state should wait until window is restored to continue.

https://reviewboard.mozilla.org/r/161384/#review166680

::: commit-message-97573:1
(Diff revision 1)
> +Bug 1034036 - Part 3: WIP Every tests that use setBrowserState should wait until window is restored to continue. r?dao
> +
> +MozReview-Commit-ID: 5SZ9ePGMKF1

Before I make any further changes, I want to ask you if this is a behaviour we should expect? I mean is there any possilbe problems besides these tests?
Comment on attachment 8883887 [details]
Bug 1034036 - Part 1: Separate windows opening and windows restoration process. Make windows restored in reversed z-order.

https://reviewboard.mozilla.org/r/154880/#review166688

::: commit-message-e6a7a:1
(Diff revision 4)
> +Bug 1034036 - Part 1: Separate windows opening and windows restoration process. Make windows restored in reversed z-order. r?dao.
> +
> +MozReview-Commit-ID: Faa8fnHRVvw

This is not ready for review. However, I have a few questions so I decided to publish it anyway.

::: browser/components/sessionstore/SessionStore.jsm:2468
(Diff revision 4)
> +    window.__SS_windowOpenedPromise = (win) => {
> +      this.restoreWindows(win, state, {overwriteTabs: true});
> +    };

We separate between window opening and window restoration process. So when window is opened (`_openWindowWithState`), they are not going to be restored unless we tell it.

This is a bit ugly but there I don't know how to initiate restoration process after window is opened.

Maybe I can create a custom open event and dispatch it in `onBeforeBrowserWindowShown`. What do you think?
Attachment #8883887 - Flags: review?(mdeboer)
Attachment #8890263 - Flags: review?(mdeboer)
Attachment #8890264 - Flags: review?(mdeboer)
Comment on attachment 8883887 [details]
Bug 1034036 - Part 1: Separate windows opening and windows restoration process. Make windows restored in reversed z-order.

Sorry, I'm too swamped with review requests and other work. Hopefully Mike can get to this sooner.
Attachment #8883887 - Flags: review?(dao+bmo)
Attachment #8890264 - Flags: review?(dao+bmo)
Attachment #8890263 - Flags: review?(dao+bmo)
Comment on attachment 8883887 [details]
Bug 1034036 - Part 1: Separate windows opening and windows restoration process. Make windows restored in reversed z-order.

https://reviewboard.mozilla.org/r/154880/#review166688

> We separate between window opening and window restoration process. So when window is opened (`_openWindowWithState`), they are not going to be restored unless we tell it.
> 
> This is a bit ugly but there I don't know how to initiate restoration process after window is opened.
> 
> Maybe I can create a custom open event and dispatch it in `onBeforeBrowserWindowShown`. What do you think?

Indeed, this looks a bit ugly. So there are two common patterns to solve these kind of issues:

1. Use a lookup-table (Map/ Hash) to keep track of callbacks that should be called in `onBeforeBrowserWindowShown` by iterating of the entries there.
2. Use custom events that you can emit on the window object, which is possible because it's a DOM element:
```js
// In onBeforeBrowserWindowShown:
let evt = new window.CustomEvent("SSBrowserWindowShowing");
window.dispatchEvent(evt);

// Then, in other areas:
window.addEventListener("SSBrowserWindowShowing", evt =>
  this.restoreWindows(evt.target, state { overwriteTabs: true }), { once: true });
// Or:
window.addEventListener("SSBrowserWindowShowing", resolve, { once: true });
```

At the moment I'm thinking number two might be the most elegant, but I'm not sure. Please make your own pick ;)
Comment on attachment 8883887 [details]
Bug 1034036 - Part 1: Separate windows opening and windows restoration process. Make windows restored in reversed z-order.

https://reviewboard.mozilla.org/r/154880/#review172224

I'd like to see the patch again with the changes you already suggested ;-)

::: browser/components/sessionstore/SessionStore.jsm:687
(Diff revision 4)
>            // make sure that at least the first window doesn't have anything hidden
>            delete state.windows[0].hidden;
>            // Since nothing is hidden in the first window, it cannot be a popup
>            delete state.windows[0].isPopup;
>            // We don't want to minimize and then open a window at startup.
> -          if (state.windows[0].sizemode == "minimized")
> +          if (state.windows[0].sizemode == "minimized" && !ss.doRestore())

Please add a comment on why you added `!ss.doRestore()` here.

::: browser/components/sessionstore/SessionStore.jsm:3553
(Diff revision 4)
> +   *
> +   * @param windows
> +   *        array of windows to be restored into
> +   * @param statesToRestore
> +   *        states of windows to be restored
> +   * @param areFollowUp

nit: s/areFollowUp/areFollowUps/
Attachment #8883887 - Flags: review?(mdeboer) → review-
Comment on attachment 8890263 [details]
Bug 1034036 - Part 2: Test that we stored correct z-indices and windows creation order, ensure we restore windows in reversed z-index.

https://reviewboard.mozilla.org/r/161382/#review172228

::: browser/components/sessionstore/test/browser_restore_reversed_z_order.js:8
(Diff revision 1)
> +  "about:license": "Licenses",
> +  "about:profiles": "About Profiles",
> +  "about:crashes": "Crash Reports"
> +};
> +const TEST_URLS = Object.keys(TEST_URLS_MAP);
> +const TEST_LABELS = TEST_URLS.map((key, index) => {

Please use `Object.values(TEST_URLS_MAP);`

::: browser/components/sessionstore/test/browser_restore_reversed_z_order.js:13
(Diff revision 1)
> +const TEST_LABELS = TEST_URLS.map((key, index) => {
> +  return TEST_URLS_MAP[key];
> +});
> +
> +const Paths = SessionFile.Paths;
> +const BROKEN_VM_Z_ORDER = AppConstants.platform != "macosx" && AppConstants.platform != "win";

nit: Shouldn't this be BROKEN_WM_Z_ORDER?

::: browser/components/sessionstore/test/browser_restore_reversed_z_order.js:38
(Diff revision 1)
> +      window.focus();
> +    }
> +  }
> +
> +  // Wait for some time for everything to settle down.
> +  await wait(2000);

Hmm, red flag! This usually means that we're not waiting for the right things... Did you copy this from another test, perhaps?
On the other hand, if we're only waiting for the one window to be minimized, I'm OK with this timeout.

::: browser/components/sessionstore/test/browser_restore_reversed_z_order.js:116
(Diff revision 1)
> +      newWindow.addEventListener("load", function() {
> +        if (++windowsOpened == 3) {
> +          Services.ww.unregisterNotification(windowObserver);
> +        }
> +
> +        // Track this window so we can remove the progress listener later

nit: missing dot.

::: browser/components/sessionstore/test/browser_restore_reversed_z_order.js:118
(Diff revision 1)
> +          Services.ww.unregisterNotification(windowObserver);
> +        }
> +
> +        // Track this window so we can remove the progress listener later
> +        windows.push(newWindow);
> +        // Add the progress listener

nit: missing dot.

::: browser/components/sessionstore/test/browser_restore_reversed_z_order.js:143
(Diff revision 1)
> +}
> +
> +function test() {
> +  waitForExplicitFinish();
> +
> +  test_z_indices_are_saved_correctly().then(() => {

Please use `add_task()`, so you don't need to use this structure.
Attachment #8890263 - Flags: review?(mdeboer) → review-
Comment on attachment 8890264 [details]
Bug 1034036 - Part 3: Tests that use set state should wait until window is restored to continue.

https://reviewboard.mozilla.org/r/161384/#review172246

If necessary, I'd be OK with a change like this. Might be worth refactoring the test file a bit to use add_task and async/ await.

::: browser/components/sessionstore/test/browser_461634.js:41
(Diff revision 1)
>    promiseWindowLoaded(newWin).then(() => {
>      gPrefService.setIntPref("browser.sessionstore.max_tabs_undo",
>                              test_state.windows[0]._closedTabs.length);
>      ss.setWindowState(newWin, JSON.stringify(test_state), true);
>  
> +    promiseWindowRestored(newWin).then(() => {

ITYM `setWindowState` and not `setBrowserState` in your commit message above then? Because there are a lot more tests that use `setBrowserState` than just this one.

::: browser/components/sessionstore/test/head.js:190
(Diff revision 1)
>    ss.setTabState(tab, state);
>    return promise;
>  }
>  
> +function promiseWindowRestored(win) {
> +  return new Promise((resolve, reject) => {

You can write this more briefly, like:
```js
return new Promise(resolve => win.addEventListener("SSWindowRestored", resolve, { once: true }));
```
Attachment #8890264 - Flags: review?(mdeboer) → review+
Comment on attachment 8883887 [details]
Bug 1034036 - Part 1: Separate windows opening and windows restoration process. Make windows restored in reversed z-order.

https://reviewboard.mozilla.org/r/154880/#review166688

> Indeed, this looks a bit ugly. So there are two common patterns to solve these kind of issues:
> 
> 1. Use a lookup-table (Map/ Hash) to keep track of callbacks that should be called in `onBeforeBrowserWindowShown` by iterating of the entries there.
> 2. Use custom events that you can emit on the window object, which is possible because it's a DOM element:
> ```js
> // In onBeforeBrowserWindowShown:
> let evt = new window.CustomEvent("SSBrowserWindowShowing");
> window.dispatchEvent(evt);
> 
> // Then, in other areas:
> window.addEventListener("SSBrowserWindowShowing", evt =>
>   this.restoreWindows(evt.target, state { overwriteTabs: true }), { once: true });
> // Or:
> window.addEventListener("SSBrowserWindowShowing", resolve, { once: true });
> ```
> 
> At the moment I'm thinking number two might be the most elegant, but I'm not sure. Please make your own pick ;)

Mike, thank for your suggestion. I think you're right. The second solution is more elegant and I'll go with that way :)
Comment on attachment 8890263 [details]
Bug 1034036 - Part 2: Test that we stored correct z-indices and windows creation order, ensure we restore windows in reversed z-index.

https://reviewboard.mozilla.org/r/161382/#review172228

> nit: Shouldn't this be BROKEN_WM_Z_ORDER?

Yeah, it should be BROKEN_WM_Z_ORDER :))

> Hmm, red flag! This usually means that we're not waiting for the right things... Did you copy this from another test, perhaps?
> On the other hand, if we're only waiting for the one window to be minimized, I'm OK with this timeout.

I feel it a little weird too! But at that moment, waiting is the only solution to get the lastest history. Without it, the session that is saved doesn't have the lastest addresses (some tabs are saved with address about:blank).

I thought about it and decided to wait until we receive a `SessionStore:update` message from content process instead. On my local machine, the test passes but one time it failed because about:blank is saved again. Not sure if this is going to cause intermittent in the future.
Comment on attachment 8890264 [details]
Bug 1034036 - Part 3: Tests that use set state should wait until window is restored to continue.

https://reviewboard.mozilla.org/r/161384/#review172246

> ITYM `setWindowState` and not `setBrowserState` in your commit message above then? Because there are a lot more tests that use `setBrowserState` than just this one.

Yeah, the problem mostly comes from `setWindowState`. However, some tests that use `setBrowserState` needed to wait until a window is restored to continue.
Comment on attachment 8883887 [details]
Bug 1034036 - Part 1: Separate windows opening and windows restoration process. Make windows restored in reversed z-order.

https://reviewboard.mozilla.org/r/154880/#review172694

Alright, sweet! r=me with comments addressed. This was the easy part, now let's make sure all the tests still work! ;-)

::: browser/components/sessionstore/SessionStore.jsm:235
(Diff revision 5)
> +/**
> + * Return a promise that will be resolved once it receives event
> + * |SSBrowserWindowShowing| which is dispatched in onBrowserWindowShown.
> + * */
> +function promiseWindowShowing(window) {
> +  return new Promise(resolve => window.addEventListener("SSBrowserWindowShowing", event => resolve(event.target), {once: true}));

nit: this might read easier if you spread this statement over two lines.
Also, you can just pass `window` into `resolve` and allow the properties inside the options objects to breathe a bit;
```js
return new Promise(resolve => window.addEventListener("SSBrowserWindowShowing",
  () => resolve(window), { once: true }));
```

::: browser/components/sessionstore/SessionStore.jsm:3265
(Diff revision 5)
>    /**
> +   * Open windows with data
> +   *
> +   * @param root
> +   *        Windows data
> +   * @returns a promise resolved when all windows is opened

nit: s/is/have been/

::: browser/components/sessionstore/SessionStore.jsm:3269
(Diff revision 5)
> +   *        Windows data
> +   * @returns a promise resolved when all windows is opened
> +   */
> +  openWindows(root) {
> +    let openedWindowPromises = [];
> +    for (let w = 0; w < root.windows.length; w++) {

While you're here, can you change this to use `for (let winData of root.windows) {`?

::: browser/components/sessionstore/SessionStore.jsm:3562
(Diff revision 5)
> +   * @param statesToRestore
> +   *        states of windows to be restored
> +   * @param areFollowUps
> +   *        a flag indicate these windows are follow-up windows
> +   */
> +  restoreWindowsInReversedZIndex(windows, statesToRestore, areFollowUps) {

nit: I think 'restoreWindowsInReversedZOrder' sounds better.

::: browser/components/sessionstore/SessionStore.jsm:3628
(Diff revision 5)
> +
> +    // Begin the restoration: First open all windows in creation order.
> +    // After all windows are opened, we restore states to windows in
> +    // reversed z-order.
> +    let openedWindowPromises = this.openWindows(root);
> +    openedWindowPromises.then((windows) => {

nit: for arrow-functions, when there's only one argument, you can omit the braces: `.then(windows => {})`
Attachment #8883887 - Flags: review?(mdeboer) → review+
Comment on attachment 8890263 [details]
Bug 1034036 - Part 2: Test that we stored correct z-indices and windows creation order, ensure we restore windows in reversed z-index.

https://reviewboard.mozilla.org/r/161382/#review172698

Deferring review for a bit, pending my question below:

::: browser/components/sessionstore/test/browser_restore_reversed_z_order.js:35
(Diff revision 2)
> +    // We want to get the lastest history from each window.
> +    promises.push(promiseContentMessage(window, "SessionStore:update"));
> +  }
> +
> +  // Wait until we get the lastest history from all windows.
> +  await Promise.all(promises);

If need be, I think it's OK to add a 2sec wait after these promises resolve.
There's also the 'sessionstore-browser-state-restored' and 'sessionstore-windows-restored' notifications... Have you looked into using these?
Attachment #8890263 - Flags: review?(mdeboer)
I also think it'd be good at this point to start pushing this to try - or run the sessionstore suite locally - to see what this change breaks. (And look into fixing the tests, naturally ;-) )
Comment on attachment 8890263 [details]
Bug 1034036 - Part 2: Test that we stored correct z-indices and windows creation order, ensure we restore windows in reversed z-index.

https://reviewboard.mozilla.org/r/161382/#review172698

> If need be, I think it's OK to add a 2sec wait after these promises resolve.
> There's also the 'sessionstore-browser-state-restored' and 'sessionstore-windows-restored' notifications... Have you looked into using these?

I changed the test to use `TabStateFlusher.flushWindow()` instead. This is much more nicer and I tested on my local machine and everything works fine.

The 'sessionstore-browser-state-restored' and 'sessionstore-windows-restored' notifications are not suitable for this test, I'm afraid. We need to keep track of the windows' restoration order. I think listen for 'SSWindowRestored' notification for each window is the best to do that.
hg error in cmd: hg push -r . -f try: pushing to ssh://hg.mozilla.org/try
searching for changes
remote: abort: abandoned transaction found!
remote: (run 'hg recover' to clean up transaction)
abort: stream ended unexpectedly (got 0 bytes, expected 4)
Comment on attachment 8890263 [details]
Bug 1034036 - Part 2: Test that we stored correct z-indices and windows creation order, ensure we restore windows in reversed z-index.

https://reviewboard.mozilla.org/r/161382/#review174042
Attachment #8890263 - Flags: review?(mdeboer) → review+
(In reply to Bao Quan [:beekill] from comment #52)
> Comment on attachment 8890263 [details]
> Bug 1034036 - Part 2: Test that we stored correct z-indices and windows
> creation order, ensure we restore windows in reversed z-index.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/161382/diff/3-4/

I added a new statement `promiseAllButPrimaryWindowClosed()` to ensure that we start the test with only primary window (the test previously failed on macOS because of another window existed when the test started).

For some reason that I don't know, this test keeps failing in Window 7 debug and sometimes in Window 7 optimized. Looking at the log, it is waiting for `promiseAllButPrimaryWindowClosed()` to resolve [1].

I run the test on my machine (Window 10) and everything is fine. Not sure what is wrong with Window 7 and `promiseAllButPrimaryWindowClosed()`.

Mike, can you take a look at this?

[1]: https://treeherder.mozilla.org/logviewer.html#?job_id=123179567&repo=try&lineNumber=34556-34561
Flags: needinfo?(mdeboer)
Attachment #8882985 - Attachment is obsolete: true
Flags: needinfo?(mdeboer)
I updated the patch and redid the try server. Here is the result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f22188b50d47&selectedJob=125187793

The result looks ok to me. Mike, is this ok to land this?
Flags: needinfo?(mdeboer)
Quan, when I open multiple windows and remember which window and tab had focus the last, I don't have that window and tab focused in front of me when I do 'Restore Last Session'... why is that?
This is on OSX, but this is so generic that it should be visible on Windows too... Perhaps you need to keep calling `.focus()` on the window or call `.focus()` after a very short delay?

I'd like to have this issue sorted out, before we land this, because it's kind of the whole point of this bug ;) Thanks!
Flags: needinfo?(mdeboer) → needinfo?(nnn_bikiu0707)
Comment on attachment 8883887 [details]
Bug 1034036 - Part 1: Separate windows opening and windows restoration process. Make windows restored in reversed z-order.

https://reviewboard.mozilla.org/r/154880/#review177748

Updated the patch: revert to the original behavior - restoreLastSession also restores focus to most recently used window in last session.

::: browser/components/sessionstore/SessionStore.jsm:2779
(Diff revisions 9 - 10)
> +        if ("zIndex" in winState) {
> +          windowToUse.__SS_zIndex = winState.zIndex;
> +        }
> +
> +        this._storeRestoreState(windowToUse, {windows: [winState]});
> +        windowToUse.__SS_restoreOptions = {overwriteTabs: canOverwriteTabs};
> +        openedWindows.push(windowToUse);

I changed the `restoreLastSession` logic a little bit: when we find out that a window can be re-use, we don't restore to that window right away. Instead, we will store its data. These windows will be restored later with newly opened windows.

::: browser/components/sessionstore/SessionStore.jsm:3073
(Diff revisions 9 - 10)
> +    if (zIndex) {
> -    winData.zIndex = this._getZIndex(aWindow);
> +      winData.zIndex = this._getZIndex(aWindow);
> +    }

When I closed all windows with Ctrl+Q, `flushAllWindowsAsync` is called. This function will call `_updateWindowFeatures` outside of `_forEachBrowserWindow`, therefore, `_getZIndex` will return undefined and thus, no zIndex is stored. 

To fix this, I only update zIndex when a valid zIndex is found - no null, undefined and 0.

::: browser/components/sessionstore/SessionStore.jsm:4255
(Diff revisions 9 - 10)
> +    let zIndex = 1;
>      while (windowsEnum.hasMoreElements()) {
>        let window = windowsEnum.getNext();
>        if (window.__SSi && !window.closed) {
>          if (this.broken_wm_z_order) {
> -          window.__SS_zIndex = mostRecentWindow.__SSi === window.__SSi ? 1 : 0;
> +          window.__SS_zIndex = mostRecentWindow.__SSi === window.__SSi ? 2 : 1;
>          } else {
>            window.__SS_zIndex = zIndex++;
>          }

With zIndex equals 0, the condition with only zIndex always returns false. So I decided to start zIndex with 1.

So we are going to have:
+ -1 -> minimized windows
+ 1, 2, 3, ... -> other windows with increasing z-order (not broken_wm_z_order)
+ with broken_wm_z_order: 2 -> most recent window, 1 -> other windows
Requesting a review from Mike so you can see my lastest changes.
Flags: needinfo?(nnn_bikiu0707)
Attachment #8883887 - Flags: review+ → review?(mdeboer)
Comment on attachment 8883887 [details]
Bug 1034036 - Part 1: Separate windows opening and windows restoration process. Make windows restored in reversed z-order.

https://reviewboard.mozilla.org/r/154880/#review177874

This works fantastic! I'm so happy when this lands... o boy.
r=me, but please fix the nits I picked below before landing.

::: browser/components/sessionstore/SessionStore.jsm:215
(Diff revision 10)
>  var gResistFingerprintingEnabled = false;
>  
> +/**
> + * Return a promise that will be resolved once it receives event
> + * |SSBrowserWindowShowing| which is dispatched in onBrowserWindowShown.
> + * */

nit: Please fix this docblock by added documentation for the `window` param and replacing '* */' with '*/'.

::: browser/components/sessionstore/SessionStore.jsm:1282
(Diff revision 10)
>    onBeforeBrowserWindowShown(aWindow) {
>      // Register the window.
>      this.onLoad(aWindow);
>  
> +    // Dispatch a custome event to tell that a new window
> +    // is about to be shown

nit: Please make this a one-line comment and end with a dot. Also, s/custome/custom/

::: browser/components/sessionstore/SessionStore.jsm:3542
(Diff revision 10)
> +   *        array of windows to be restored into
> +   * @param statesToRestore
> +   *        states of windows to be restored
> +   */
> +  restoreWindowsFeaturesAndTabs(windows, statesToRestore) {
> +    // First, we restore window features,

nit: Please make comment fit on two lines.

::: browser/components/sessionstore/SessionStore.jsm:3637
(Diff revision 10)
> +
> +    // Begin the restoration: First open all windows in creation order.
> +    // After all windows are opened, we restore states to windows in
> +    // reversed z-order.
> +    let openedWindowPromises = this.openWindows(root);
> +    openedWindowPromises.then(windows => {

You can shorten this to:
```js
this.openWindows(root).then(windows => {
  // ...
});
```

(So no need for the temporary variable.)

::: browser/components/sessionstore/SessionStore.jsm:3638
(Diff revision 10)
> +    // Begin the restoration: First open all windows in creation order.
> +    // After all windows are opened, we restore states to windows in
> +    // reversed z-order.
> +    let openedWindowPromises = this.openWindows(root);
> +    openedWindowPromises.then(windows => {
> +      // We want to add current window to opened window,

nit: Please make this comment fit on three lines, instead of four.

::: browser/components/sessionstore/SessionStore.jsm:4232
(Diff revision 10)
>    },
>  
>    /**
> -   * call a callback for all currently opened browser windows
> +   * A boolean flag indicates whether we can iterate over all windows
> +   * in their z order.
> +   * */

nit: Please replace '* */' with '*/'.

::: browser/components/sessionstore/SessionStore.jsm:4233
(Diff revision 10)
>  
>    /**
> -   * call a callback for all currently opened browser windows
> +   * A boolean flag indicates whether we can iterate over all windows
> +   * in their z order.
> +   * */
> +  get broken_wm_z_order() {

nit: Please rename this getter to `isWMZOrderBroken`.

::: browser/components/sessionstore/SessionStore.jsm:4300
(Diff revision 10)
>  
>    /**
> +   * Store a restore state of a window to this._statesToRestore. The window
> +   * will be given an id that can be used to get the restore state from
> +   * this._statesToRestore.
> +   * */

nit: Please replace '* */' with '*/' and also document the `window` and `state` params.

::: browser/components/sessionstore/SessionStore.jsm:4301
(Diff revision 10)
>    /**
> +   * Store a restore state of a window to this._statesToRestore. The window
> +   * will be given an id that can be used to get the restore state from
> +   * this._statesToRestore.
> +   * */
> +  _storeRestoreState(window, state) {

nit: Please rename this function to `_updateWindowRestoreState`
Attachment #8883887 - Flags: review?(mdeboer) → review+
(In reply to Bao Quan [:beekill] from comment #72)
> Comment on attachment 8883887 [details]
> Bug 1034036 - Part 1: Separate windows opening and windows restoration
> process. Make windows restored in reversed z-order.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/154880/diff/10-11/

Updated to lastest build, fixed nits and redid try server to make sure I'm not making any mistake. :)
(In reply to Bao Quan [:beekill] from comment #76)
> Comment on attachment 8890263 [details]
> Bug 1034036 - Part 2: Test that we stored correct z-indices and windows
> creation order, ensure we restore windows in reversed z-index.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/161382/diff/9-10/

Hehe, I fixed the failures on Win 7 and stylo build. It actually was freeze at `await Promise.all(promises)`. I did a few logging and found that some tabs were flushed when they were about:newtab. And thus, when about pages were loaded into tabs, those flushed message were ignored [1] and [2] and thus, the promises were never resolved.

The result from try server look goods, I think this is ready to land. Mike, can you push this?

I wonder, should [1] and [2] do something more like resolve a promise when a flush is requested, not just a plain return statement? That way, we could possibly remove a pending promise blocking something.

[1]: http://searchfox.org/mozilla-central/source/browser/components/sessionstore/content/content-sessionStore.js#823
[2]: http://searchfox.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#845
Flags: needinfo?(mdeboer)
(In reply to Bao Quan [:beekill] from comment #78)
> (In reply to Bao Quan [:beekill] from comment #76)
> > Comment on attachment 8890263 [details]
> > Bug 1034036 - Part 2: Test that we stored correct z-indices and windows
> > creation order, ensure we restore windows in reversed z-index.
> > 
> > Review request updated; see interdiff:
> > https://reviewboard.mozilla.org/r/161382/diff/9-10/
> 
> Hehe, I fixed the failures on Win 7 and stylo build. It actually was freeze
> at `await Promise.all(promises)`. I did a few logging and found that some
> tabs were flushed when they were about:newtab. And thus, when about pages
> were loaded into tabs, those flushed message were ignored [1] and [2] and
> thus, the promises were never resolved.
> 
> The result from try server look goods, I think this is ready to land. Mike,
> can you push this?
> 
> I wonder, should [1] and [2] do something more like resolve a promise when a
> flush is requested, not just a plain return statement? That way, we could
> possibly remove a pending promise blocking something.
> 
> [1]:
> http://searchfox.org/mozilla-central/source/browser/components/sessionstore/
> content/content-sessionStore.js#823
> [2]:
> http://searchfox.org/mozilla-central/source/browser/components/sessionstore/
> SessionStore.jsm#845

So what I did to solve the problem is to ensure we already loaded about pages before request a flush.
Quan, can you rebase and push your patches to MozReview so I can land them for you? Thanks!
Flags: needinfo?(mdeboer) → needinfo?(nnn_bikiu0707)
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1576132e98a3
Part 1: Separate windows opening and windows restoration process. Make windows restored in reversed z-order. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/b9fdf5d1b402
Part 2: Test that we stored correct z-indices and windows creation order, ensure we restore windows in reversed z-index. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/39a94e16a369
Part 3: Tests that use set state should wait until window is restored to continue. r=mikedeboer
Flags: needinfo?(nnn_bikiu0707)
Backed out for frequently failing browser-chrome's browser/components/sessionstore/test/browser_restore_reversed_z_order.js on OS X:

https://hg.mozilla.org/mozilla-central/rev/2a9cffb19ab58a7875aee5492c565c549c037511
https://hg.mozilla.org/mozilla-central/rev/716e6c452539e2d99d7af913e20aa9c38101addb
https://hg.mozilla.org/mozilla-central/rev/9506d68397d7646ab7b668d5e624da7639107dfc

Range with push: https://treeherder.mozilla.org/#/jobs?repo=autoland&tochange=39a94e16a3694c443a985081ac1c6cd761363da2&fromchange=6af4edb9979ef0c3f2d6ae9fe0afc9249d34c32c&filter-searchStr=10.10%20browser-chrome
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=129773419&repo=autoland

03:24:27     INFO - Leaving test bound init
03:24:27     INFO - Entering test bound test_z_indices_are_saved_correctly
03:24:27     INFO - Buffered messages finished
03:24:27     INFO - TEST-UNEXPECTED-FAIL | browser/components/sessionstore/test/browser_restore_reversed_z_order.js | Correct number of windows saved - Got 5, expected 4
03:24:27     INFO - Stack trace:
03:24:27     INFO -     chrome://mochikit/content/browser-test.js:test_is:1007
03:24:27     INFO -     chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_restore_reversed_z_order.js:test_z_indices_are_saved_correctly:50
03:24:27     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:794:9
03:24:27     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:694:7
03:24:27     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59
Status: RESOLVED → REOPENED
Flags: needinfo?(nnn_bikiu0707)
Resolution: FIXED → ---
Target Milestone: Firefox 57 → ---
Backout brought back previous perf baselines:

== Change summary for alert #9349 (as of September 09 2017 15:18 UTC) ==

Improvements:

 25%  sessionrestore_many_windows windows10-64 opt e10s     4,027.83 -> 3,022.67
 25%  sessionrestore_many_windows windows7-32 opt e10s      4,146.67 -> 3,128.00
  6%  sessionrestore_many_windows osx-10-10 opt e10s        3,177.92 -> 2,995.17

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=9349
Won't this change necessarily cause a slow-down in startup, since we no longer want to open the windows in burst and let the OS handle the the layering and ordering?
:palswim You are right.
Sorry, I pasted the results in the wrong bug. Thanks for the notice!
Quan hasn't been able to work on Firefox bugs for a while now. Unassigning to allow others to finish this up.
Assignee: nnn_bikiu0707 → nobody
Status: REOPENED → NEW
Flags: needinfo?(nnn_bikiu0707)
@palswim

Speed is important, but it's not everything. Reproducible behaviour is more important. Opening all windows in burst is the cause of the "random window order at every start" problem.
Driving this home.
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Priority: -- → P2
Attachment #8883887 - Attachment is obsolete: true
Attachment #8890263 - Attachment is obsolete: true
Attachment #8890264 - Attachment is obsolete: true
Dão, sorry to throw this on your plate, but thought it would be real good to have a second pair of eyes look at this quite intricate set of patches.
I haven't touched part 1, rewrote the test (part 2) and unbitrotted the adjustments to other tests (part 3). Thanks for taking a look!
Comment on attachment 8946675 [details]
Bug 1034036 - Part 1: Separate the window opening logic and window restoration process. Make sure windows restore in reverse z-order.

https://reviewboard.mozilla.org/r/216652/#review225016

::: browser/components/sessionstore/SessionStore.jsm:3728
(Diff revision 1)
> -    // open new windows for all further window entries of a multi-window session
> -    // (unless they don't contain any tab data)
> -    let winData;
> -    for (var w = 1; w < root.windows.length; w++) {
> -      winData = root.windows[w];
> -      if (winData && winData.tabs && winData.tabs[0]) {
> +    }
> +
> +    // Store the restore state and restore option of the current window,
> +    // so that the window can be restored in reversed z-order.
> +    this._updateWindowRestoreState(aWindow, {windows: firstWindowData});
> +    aWindow.__SS_restoreOptions = aOptions;

Re: __SS_restoreOptions and __SS_zIndex

I believe we were at some point trying to move away from these external yet private properties in favor of fully-private WeakMaps.

::: browser/components/sessionstore/SessionStore.jsm:4353
(Diff revision 1)
> +    let zIndex = 1;
>      while (windowsEnum.hasMoreElements()) {
> -      var window = windowsEnum.getNext();
> +      let window = windowsEnum.getNext();
>        if (window.__SSi && !window.closed) {
> +        if (this.isWMZOrderBroken) {
> +          window.__SS_zIndex = mostRecentWindow.__SSi === window.__SSi ? 2 : 1;

I tested this on Ubuntu and the result is pretty confusing. Should we maybe track the z-index ourselves e.g. using the activate event? It might even make sense to introduce a helper module for this so that we can generally move away from get[ZOrderDOMWindow]Enumerator.
Attachment #8946675 - Flags: review?(dao+bmo)
(In reply to Dão Gottwald [::dao] from comment #99)
> Should we maybe
> track the z-index ourselves e.g. using the activate event? It might even
> make sense to introduce a helper module for this so that we can generally
> move away from get[ZOrderDOMWindow]Enumerator.

Or I guess this could be part of RecentWindow.jsm.
Comment on attachment 8946676 [details]
Bug 1034036 - Part 2: Test that we stored window z-indices correctly and in order of creation. Ensure that we restore windows in reverse z-index order.

https://reviewboard.mozilla.org/r/216654/#review226526
Attachment #8946676 - Flags: review?(dao+bmo)
Comment on attachment 8946677 [details]
Bug 1034036 - Part 3: Tests that use ss.setBrowserState() or ss.setWindowState() should wait until the window is restored to continue.

https://reviewboard.mozilla.org/r/216656/#review226530

::: browser/components/extensions/test/browser/browser_ext_tabs_lazy.js:17
(Diff revision 1)
>    }],
>  };
>  
>  add_task(async function() {
>    SessionStore.setBrowserState(JSON.stringify(SESSION));
> +  await promiseWindowRestored(window);

How about adding async setBrowserState and setWindowState helper functions and use those in tests instead of calling SessionStore directly?
Attachment #8946677 - Flags: review?(dao+bmo)
Attachment #8946675 - Attachment is obsolete: true
Attachment #8946676 - Attachment is obsolete: true
Attachment #8946677 - Attachment is obsolete: true
The commit message for each patch contains some important remarks and findings. I'm still working on a few tests that Try is choking on, but the first four patches should be in good shape.
So merging these two modules into one (Patch 1) saves us a JS compartment, which is probably nice for perf.

I tried refactoring `getMostRecentBrowserWindow` to peruse the z-index tracking as well, but that appeared to be so hard that I need to move that to a follow-up. The Captive Portal tests are choking and I already spent too much time to try and figure out why.
I did write unit tests for that method (which demonstrate that it behaves well after the refactor too), which I might as well add to one of the patches here.
Comment on attachment 8953440 [details]
Bug 1034036 - Part 1: Merge RecentWindow.jsm and UpdateTopLevelContentWindowIDHelper.jsm into one module called 'WindowTracker.jsm'.

https://reviewboard.mozilla.org/r/222700/#review228654

::: browser/base/content/browser.js:1484
(Diff revision 1)
>      // Setup click-and-hold gestures access to the session history
>      // menus if global click-and-hold isn't turned on
>      if (!getBoolPref("ui.click_hold_context_menus", false))
>        SetClickAndHoldHandlers();
>  
> -    ChromeUtils.import("resource:///modules/UpdateTopLevelContentWindowIDHelper.jsm", {})
> +    WindowTracker.track(window);

We should probably do this way earlier, not in _delayedStartup, to better match nsIWindowMediator.

::: browser/modules/WindowTracker.jsm:3
(Diff revision 1)
>  /* This Source Code Form is subject to the terms of the Mozilla Public
> - * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> - * You can obtain one at http://mozilla.org/MPL/2.0/. */
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Can you rename UpdateTopLevelContentWindowIDHelper.jsm rather than RecentWindow.jsm? I expect this will produce a better patch with less code churn. The only part this patch leaves intact is RecentWindow.jsm's getMostRecentBrowserWindow implementation, which I assume we're going to replace to avoid nsIWindowMediator, right?

I'd also suggest 1) renaming this to BrowserWindowTracker.jsm or 2) making it generic so it can track any window type if we think that would be useful (but I suspect not).

::: browser/modules/WindowTracker.jsm:101
(Diff revision 1)
> +    messageManager.addMessageListener("Browser:Init", _handleMessage);
> +
> +    // This gets called AFTER activate event, so if this is the focused window
> +    // we want to activate it.
> +    if (aWindow == _focusManager.activeWindow)
> +      this.handleFocusedWindow(aWindow);

I think we can assume that the newly added window is the top-most one, get rid of the activeWindow check, and not wait for an initial activate event. I expect this will make this a more reliable replacement for nsIWindowMediator.

::: browser/modules/WindowTracker.jsm:191
(Diff revision 1)
> +   *        * private: true to restrict the search to private windows
> +   *            only, false to restrict the search to non-private only.
> +   *            Omit the property to search in both groups.
> +   *        * allowPopups: true if popup windows are permissable.
> +   */
> +  getMostRecentBrowserWindow(options) {

If you rename WindowTracker to BrowserWindowTracker, this can be shortened to getMostRecentWindow or getTopWindow.
Attachment #8953440 - Flags: review?(dao+bmo)
Attachment #8953441 - Flags: review?(dao+bmo)
Attachment #8953442 - Flags: review?(dao+bmo)
Attachment #8953443 - Flags: review?(dao+bmo)
Attachment #8953444 - Flags: review?(dao+bmo)
Attachment #8953440 - Attachment is obsolete: true
Attachment #8953441 - Attachment is obsolete: true
Attachment #8953442 - Attachment is obsolete: true
Attachment #8953443 - Attachment is obsolete: true
Attachment #8953444 - Attachment is obsolete: true
Attachment #8954354 - Attachment is obsolete: true
Attachment #8954354 - Flags: review?(dao+bmo)
Attachment #8954355 - Attachment is obsolete: true
Attachment #8954355 - Flags: review?(dao+bmo)
Attachment #8954356 - Attachment is obsolete: true
Attachment #8954356 - Flags: review?(dao+bmo)
Attachment #8954357 - Attachment is obsolete: true
Attachment #8954357 - Flags: review?(dao+bmo)
Attachment #8954358 - Attachment is obsolete: true
Attachment #8954358 - Flags: review?(dao+bmo)
Attachment #8954359 - Attachment is obsolete: true
Attachment #8954359 - Flags: review?(dao+bmo)
Comment on attachment 8954403 [details]
Bug 1034036 - Part 3: start tracking windows activations to always be aware of their respective order. This allows consumers to iterate over a set of windows in order of appearance (e.g. z-order).

https://reviewboard.mozilla.org/r/223484/#review229818

::: browser/modules/BrowserWindowTracker.jsm:84
(Diff revision 1)
>        browser === browser.ownerGlobal.gBrowser.selectedBrowser) {
>      _updateCurrentContentOuterWindowID(browser);
>    }
>  }
>  
> +function _updateZIndex(window) {

There seems to be a surprising amount of complexity related to the z-index tracking. I haven't studied the specifics here yet, but could we instead just use an array? We do this for tabs for instance here:

https://searchfox.org/mozilla-central/rev/14d933246211b02f5be21d2e730a57cf087c6606/browser/base/content/browser-ctrlTab.js#498-500
(In reply to Dão Gottwald [::dao] from comment #129)
> There seems to be a surprising amount of complexity related to the z-index
> tracking. I haven't studied the specifics here yet, but could we instead
> just use an array? We do this for tabs for instance here:
> 
> https://searchfox.org/mozilla-central/rev/
> 14d933246211b02f5be21d2e730a57cf087c6606/browser/base/content/browser-
> ctrlTab.js#498-500

That would be easier, but I didn't want to risk leaking windows. Therefore I wanted to see if this could be done simply by using WeakMaps.
(In reply to Mike de Boer [:mikedeboer] from comment #130)
> (In reply to Dão Gottwald [::dao] from comment #129)
> > There seems to be a surprising amount of complexity related to the z-index
> > tracking. I haven't studied the specifics here yet, but could we instead
> > just use an array? We do this for tabs for instance here:
> > 
> > https://searchfox.org/mozilla-central/rev/
> > 14d933246211b02f5be21d2e730a57cf087c6606/browser/base/content/browser-
> > ctrlTab.js#498-500
> 
> That would be easier, but I didn't want to risk leaking windows. Therefore I
> wanted to see if this could be done simply by using WeakMaps.

We're already listening to the unload event, and removing a window there should be trivial. I don't think there's some meaningful risk here.
Attachment #8954401 - Attachment is obsolete: true
Attachment #8954401 - Flags: review?(dao+bmo)
Attachment #8954402 - Attachment is obsolete: true
Attachment #8954402 - Flags: review?(dao+bmo)
Attachment #8954403 - Attachment is obsolete: true
Attachment #8954403 - Flags: review?(dao+bmo)
Attachment #8954404 - Attachment is obsolete: true
Attachment #8954404 - Flags: review?(dao+bmo)
Attachment #8954405 - Attachment is obsolete: true
Attachment #8954405 - Flags: review?(dao+bmo)
Attachment #8954406 - Attachment is obsolete: true
Attachment #8954406 - Flags: review?(dao+bmo)
Comment on attachment 8955468 [details]
Bug 1034036 - Part 6: Tests that use ss.setBrowserState() or ss.setWindowState() should wait until the window is restored to continue.

https://reviewboard.mozilla.org/r/224642/#review231272

::: browser/components/sessionstore/test/browser_477657.js:51
(Diff revision 1)
> -           "the window remains maximized");
> +    "the window remains maximized");
> -        newState.windows[0].sizemode = "normal";
> +  newState.windows[0].sizemode = "normal";
> -        ss.setWindowState(newWin, JSON.stringify(newState), true);
>  
> -        newWin.setTimeout(function() {
> +  await setWindowState(newWin, newState, true);
> +  await new Promise(resolve => newWin.setTimeout(resolve, 0));

Do we still need the setTimeout now that setWindowState is async?
Attachment #8955468 - Flags: review?(dao+bmo) → review+
Comment on attachment 8955463 [details]
Bug 1034036 - Part 1: Merge RecentWindow.jsm and UpdateTopLevelContentWindowIDHelper.jsm into one module called 'BrowserWindowTracker.jsm'.

https://reviewboard.mozilla.org/r/224632/#review235030
Attachment #8955463 - Flags: review?(dao+bmo) → review+
Comment on attachment 8955464 [details]
Bug 1034036 - Part 2: Rename and shorten `getMostRecentBrowserWindow` to `getTopWindow` and modernize the style used in BrowserWindowTracker.jsm.

https://reviewboard.mozilla.org/r/224634/#review235032
Attachment #8955464 - Flags: review?(dao+bmo) → review+
Comment on attachment 8955465 [details]
Bug 1034036 - Part 3: start tracking windows activations to always be aware of their respective order. This allows consumers to iterate over a set of windows in order of appearance (e.g. z-order).

https://reviewboard.mozilla.org/r/224636/#review235040

::: browser/modules/BrowserWindowTracker.jsm:230
(Diff revision 1)
> +   * baseline.
> +   *
> +   * @param  {DOMWindow} window
> +   * @return {Number}    ranging from 0 - [number of open browser windows]
> +   */
> +  getZIndex(window) {

I'd rather not expose this and let the test get the indeces via orderedWindows.

::: browser/modules/test/browser/browser_BrowserWindowTracker.js:62
(Diff revision 1)
> +      "Window focused before the private window should be the most recent one.");
> +    window = BrowserWindowTracker.getTopWindow({ allowPopups: false });
> +    Assert.equal(window, windows[expectedMostRecentIndex],
> +      "Window focused before the private window should be the most recent one.");
> +
> +    // Somehow on Linux, transforming an existing window to a popup doesn't work.

It's also not really supported by our front-end code either. Can you just open a proper popup instead?

::: browser/modules/test/browser/browser_BrowserWindowTracker.js:79
(Diff revision 1)
> +}).skip();
> +
> +add_task(async function test_orderedWindows() {
> +  await withOpenWindows(10, async function(windows) {
> +    let ordered = [...BrowserWindowTracker.orderedWindows].filter(w => w != TEST_WINDOW);
> +    Assert.deepEqual([9, 8, 7, 6, 5, 4, 3, 2, 1, 0], ordered.map(w => w[TEST_WINDOWID_PROP]),

Can you get rid of the TEST_WINDOWID_PROP thingy and use windows.indexOf(w) instead?
Attachment #8955465 - Flags: review?(dao+bmo) → review+
Comment on attachment 8955465 [details]
Bug 1034036 - Part 3: start tracking windows activations to always be aware of their respective order. This allows consumers to iterate over a set of windows in order of appearance (e.g. z-order).

https://reviewboard.mozilla.org/r/224636/#review235042

::: browser/base/content/browser.js:1200
(Diff revision 1)
>            .QueryInterface(Ci.nsIInterfaceRequestor)
>            .getInterface(Ci.nsIXULWindow)
>            .XULBrowserWindow = window.XULBrowserWindow;
>      window.QueryInterface(Ci.nsIDOMChromeWindow).browserDOMWindow =
>        new nsBrowserAccess();
> +    BrowserWindowTracker.track(window);

FYI, this needs to happen after gBrowser is initialized since bug 1443849 landed in the meantime.
Comment on attachment 8955466 [details]
Bug 1034036 - Part 4: move away from keeping state on the living objects, like windows, tabs and browsers, but keep it truly privately stored in WeakMaps.

https://reviewboard.mozilla.org/r/224638/#review235044

::: browser/components/sessionstore/SessionStore.jsm:17
(Diff revision 1)
>  const TAB_STATE_NEEDS_RESTORE = 1;
>  const TAB_STATE_RESTORING = 2;
>  const TAB_STATE_WILL_RESTORE = 3;
> +const TAB_STATES = new WeakMap();
> +const TAB_LAZY_STATES = new WeakMap();
> +const BROWSER_STATES = new WeakMap();

I don't understand the difference between TAB_STATES and BROWSER_STATES without digging deep into the code. Can these names be clearer or is there room for more fundamental cleanup?

::: browser/components/sessionstore/SessionStore.jsm:1911
(Diff revision 1)
>      if (browser.frameLoader) {
>        this._lastKnownFrameLoader.set(browser.permanentKey, browser.frameLoader);
>      }
>  
>      // Only restore if browser has been lazy.
> -    if (aTab.__SS_lazyData && !browser.__SS_restoreState && TabStateCache.get(browser)) {
> +    if (TAB_LAZY_STATES.get(aTab) && !BROWSER_STATES.get(browser) && TabStateCache.get(browser)) {

.has()

::: browser/components/sessionstore/SessionStore.jsm:2207
(Diff revision 1)
>  
>      // If we hadn't yet restored, or were still in the midst of
>      // restoring this browser at the time of the crash, we need
>      // to reset its state so that we can try to restore it again
>      // when the user revives the tab from the crash.
> -    if (browser.__SS_restoreState) {
> +    if (BROWSER_STATES.get(browser)) {

.has()

::: browser/components/sessionstore/SessionStore.jsm:2374
(Diff revision 1)
>      let window = aTab.ownerGlobal;
>      if (!("__SSi" in window)) {
>        throw Components.Exception("Window is not tracked", Cr.NS_ERROR_INVALID_ARG);
>      }
>  
> -    if (aTab.linkedBrowser.__SS_restoreState) {
> +    if (BROWSER_STATES.get(aTab.linkedBrowser)) {

.has()

::: browser/components/sessionstore/SessionStore.jsm:3023
(Diff revision 1)
>        } else {
>          options.loadArguments = recentLoadArguments;
>        }
>  
>        // Need to reset restoring tabs.
> -      if (tab.linkedBrowser.__SS_restoreState) {
> +      if (BROWSER_STATES.get(tab.linkedBrowser)) {

.has()

::: browser/components/sessionstore/SessionStore.jsm:3710
(Diff revision 1)
>  
>    // Restores the given tab state for a given tab.
>    restoreTab(tab, tabData, options = {}) {
>      let browser = tab.linkedBrowser;
>  
> -    if (browser.__SS_restoreState) {
> +    if (BROWSER_STATES.get(browser)) {

.has()

::: browser/components/sessionstore/SessionStore.jsm:4774
(Diff revision 1)
>    },
>  
>    _resetTabRestoringState(tab) {
>      let browser = tab.linkedBrowser;
>  
> -    if (!browser.__SS_restoreState) {
> +    if (!BROWSER_STATES.get(browser)) {

.has()
Attachment #8955466 - Flags: review?(dao+bmo) → review+
Comment on attachment 8955467 [details]
Bug 1034036 - Part 5: restore windows in reverse z-order, so that the last focused window is restored first and stays in front.

https://reviewboard.mozilla.org/r/224640/#review235046

::: browser/components/sessionstore/SessionStore.jsm:1609
(Diff revision 1)
>     */
>    onQuitApplicationGranted: function ssi_onQuitApplicationGranted(syncShutdown = false) {
>      // Collect an initial snapshot of window data before we do the flush
> -    this._forEachBrowserWindow((win) => {
> -      this._collectWindowData(win);
> -    });
> +    for (let window of this._browserWindows) {
> +      this._collectWindowData(window);
> +      this._windows[window.__SSi].zIndex = BrowserWindowTracker.getZIndex(window);

Again, I'd prefer if BrowserWindowTracker and SessionStore didn't need a shared understanding of what a window's z-index is, e.g. whether it's 0 or 1-based.

::: browser/components/sessionstore/SessionStore.jsm:1609
(Diff revision 1)
>     */
>    onQuitApplicationGranted: function ssi_onQuitApplicationGranted(syncShutdown = false) {
>      // Collect an initial snapshot of window data before we do the flush
> -    this._forEachBrowserWindow((win) => {
> -      this._collectWindowData(win);
> -    });
> +    for (let window of this._browserWindows) {
> +      this._collectWindowData(window);
> +      this._windows[window.__SSi].zIndex = BrowserWindowTracker.getZIndex(window);

So this means the indices will be off in case of a crash? Should this be part of _collectWindowData instead?
Whiteboard: [fxperf]
Attachment #8955467 - Flags: review?(dao+bmo)
Comment on attachment 8955465 [details]
Bug 1034036 - Part 3: start tracking windows activations to always be aware of their respective order. This allows consumers to iterate over a set of windows in order of appearance (e.g. z-order).

https://reviewboard.mozilla.org/r/224636/#review237948

::: browser/base/content/browser.js:1200
(Diff revision 1)
>            .QueryInterface(Ci.nsIInterfaceRequestor)
>            .getInterface(Ci.nsIXULWindow)
>            .XULBrowserWindow = window.XULBrowserWindow;
>      window.QueryInterface(Ci.nsIDOMChromeWindow).browserDOMWindow =
>        new nsBrowserAccess();
> +    BrowserWindowTracker.track(window);

I don't understand what you mean I should change here... should I move it to 
1. the end of the init() method in tabbrowser.js, or
2. above this line, right after gBrowser.init(), or
3. somewhere else?
Whiteboard: [fxperf] → [fxperf:p1]
Comment on attachment 8955467 [details]
Bug 1034036 - Part 5: restore windows in reverse z-order, so that the last focused window is restored first and stays in front.

https://reviewboard.mozilla.org/r/224640/#review237972

::: browser/components/sessionstore/SessionStore.jsm:1609
(Diff revision 1)
>     */
>    onQuitApplicationGranted: function ssi_onQuitApplicationGranted(syncShutdown = false) {
>      // Collect an initial snapshot of window data before we do the flush
> -    this._forEachBrowserWindow((win) => {
> -      this._collectWindowData(win);
> -    });
> +    for (let window of this._browserWindows) {
> +      this._collectWindowData(window);
> +      this._windows[window.__SSi].zIndex = BrowserWindowTracker.getZIndex(window);

I wanted to persist the z-index at the time when we're _certain_ that the list is correct. This means that a crash is not that time.
The correct times, as I see it, are:
1. During regular sessionstore saves whilst the browser is running normally (in `getCurrentState()`) and
2. during a normal shutdown when nothing crashed and we know for sure that the list of tracked windows is a correct representation.

This precaution is mainly there to ensure that the lastly focused window is always restored on top and is in fact the window the user expects as well.
Attachment #8955463 - Attachment is obsolete: true
Attachment #8955464 - Attachment is obsolete: true
Attachment #8955465 - Attachment is obsolete: true
Attachment #8955466 - Attachment is obsolete: true
Attachment #8955467 - Attachment is obsolete: true
Attachment #8955468 - Attachment is obsolete: true
Attachment #8955469 - Attachment is obsolete: true
Attachment #8955469 - Flags: review?(dao+bmo)
Sorry Dão, Reviewboard decided to give you a hard time and clear all the flags you set before... Grrr!
A needinfo for comment 146 and comment 147!
Flags: needinfo?(dao+bmo)
Comment on attachment 8963675 [details]
Bug 1034036 - Part 1: Merge RecentWindow.jsm and UpdateTopLevelContentWindowIDHelper.jsm into one module called 'BrowserWindowTracker.jsm'.

https://reviewboard.mozilla.org/r/232556/#review237976


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/downloads/DownloadsTaskbar.jsm:170
(Diff revision 1)
>        this.onSummaryChanged();
>      }
>  
>      aWindow.addEventListener("unload", () => {
>        // Locate another browser window, excluding the one being closed.
>        let browserWindow = RecentWindow.getMostRecentBrowserWindow();

Error: 'recentwindow' is not defined. [eslint: no-undef]
Comment on attachment 8963676 [details]
Bug 1034036 - Part 2: Rename and shorten `getMostRecentBrowserWindow` to `getTopWindow` and modernize the style used in BrowserWindowTracker.jsm.

https://reviewboard.mozilla.org/r/232558/#review237978


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/downloads/DownloadsTaskbar.jsm:170
(Diff revision 1)
>        this.onSummaryChanged();
>      }
>  
>      aWindow.addEventListener("unload", () => {
>        // Locate another browser window, excluding the one being closed.
>        let browserWindow = RecentWindow.getMostRecentBrowserWindow();

Error: 'recentwindow' is not defined. [eslint: no-undef]
Attachment #8963675 - Attachment is obsolete: true
Attachment #8963675 - Flags: review?(dao+bmo)
Attachment #8963676 - Attachment is obsolete: true
Attachment #8963676 - Flags: review?(dao+bmo)
Attachment #8963677 - Attachment is obsolete: true
Attachment #8963677 - Flags: review?(dao+bmo)
Attachment #8963678 - Attachment is obsolete: true
Attachment #8963678 - Flags: review?(dao+bmo)
Attachment #8963679 - Attachment is obsolete: true
Attachment #8963679 - Flags: review?(dao+bmo)
Attachment #8963680 - Attachment is obsolete: true
Attachment #8963680 - Flags: review?(dao+bmo)
Attachment #8963681 - Attachment is obsolete: true
Attachment #8963681 - Flags: review?(dao+bmo)
(In reply to Mike de Boer [:mikedeboer] (Back! Processing backlog...) from comment #146)
> Comment on attachment 8955465 [details]
> Bug 1034036 - Part 3: start tracking windows activations to always be aware
> of their respective order. This allows consumers to iterate over a set of
> windows in order of appearance (e.g. z-order).
> 
> https://reviewboard.mozilla.org/r/224636/#review237948
> 
> ::: browser/base/content/browser.js:1200
> (Diff revision 1)
> >            .QueryInterface(Ci.nsIInterfaceRequestor)
> >            .getInterface(Ci.nsIXULWindow)
> >            .XULBrowserWindow = window.XULBrowserWindow;
> >      window.QueryInterface(Ci.nsIDOMChromeWindow).browserDOMWindow =
> >        new nsBrowserAccess();
> > +    BrowserWindowTracker.track(window);
> 
> I don't understand what you mean I should change here... should I move it to 
> 1. the end of the init() method in tabbrowser.js, or
> 2. above this line, right after gBrowser.init(), or
> 3. somewhere else?

This was just an FYI that this needs to be called after gBrowser.init. I don't think this belongs in tabbrowser.js, and if init is already called before the above lines then nothing needs to change.

(In reply to Mike de Boer [:mikedeboer] (Back! Processing backlog...) from comment #147)
> Comment on attachment 8955467 [details]
> Bug 1034036 - Part 5: restore windows in reverse z-order, so that the last
> focused window is restored first and stays in front.
> 
> https://reviewboard.mozilla.org/r/224640/#review237972
> 
> ::: browser/components/sessionstore/SessionStore.jsm:1609
> (Diff revision 1)
> >     */
> >    onQuitApplicationGranted: function ssi_onQuitApplicationGranted(syncShutdown = false) {
> >      // Collect an initial snapshot of window data before we do the flush
> > -    this._forEachBrowserWindow((win) => {
> > -      this._collectWindowData(win);
> > -    });
> > +    for (let window of this._browserWindows) {
> > +      this._collectWindowData(window);
> > +      this._windows[window.__SSi].zIndex = BrowserWindowTracker.getZIndex(window);
> 
> I wanted to persist the z-index at the time when we're _certain_ that the
> list is correct. This means that a crash is not that time.

Why not?

> The correct times, as I see it, are:
> 1. During regular sessionstore saves whilst the browser is running normally
> (in `getCurrentState()`) and

I don't understand the difference between "when the browser crashes" and "while the browser is running normally". When the browser crashes, it was presumably running normally up until that point, and so the z-indices should be saved along with the windows. Am I missing something?
Flags: needinfo?(dao+bmo)
Comment on attachment 8963688 [details]
Bug 1034036 - Part 6: Tests that use ss.setBrowserState() or ss.setWindowState() should wait until the window is restored to continue.

https://reviewboard.mozilla.org/r/232580/#review239190

::: browser/components/sessionstore/test/browser_477657.js:41
(Diff revision 1)
> -      delete newState.windows[0]._closedTabs;
> +  delete newState.windows[0]._closedTabs;
> -      delete newState.windows[0].sizemode;
> +  delete newState.windows[0].sizemode;
> -      ss.setWindowState(newWin, JSON.stringify(newState), true);
>  
> -      newWin.setTimeout(function() {
> +  await setWindowState(newWin, newState, true);
> +  await new Promise(resolve => newWin.setTimeout(resolve, 0));

Do we still need the setTimeout now that setWindowState is async?
Attachment #8963688 - Flags: review?(dao+bmo) → review+
Comment on attachment 8963683 [details]
Bug 1034036 - Part 1: Merge RecentWindow.jsm and UpdateTopLevelContentWindowIDHelper.jsm into one module called 'BrowserWindowTracker.jsm'.

https://reviewboard.mozilla.org/r/232570/#review239192
Attachment #8963683 - Flags: review?(dao+bmo) → review+
Comment on attachment 8963684 [details]
Bug 1034036 - Part 2: Rename and shorten `getMostRecentBrowserWindow` to `getTopWindow` and modernize the style used in BrowserWindowTracker.jsm.

https://reviewboard.mozilla.org/r/232572/#review239194
Attachment #8963684 - Flags: review?(dao+bmo) → review+
Comment on attachment 8963685 [details]
Bug 1034036 - Part 3: start tracking windows activations to always be aware of their respective order. This allows consumers to iterate over a set of windows in order of appearance (e.g. z-order).

https://reviewboard.mozilla.org/r/232574/#review239198
Attachment #8963685 - Flags: review?(dao+bmo) → review+
Comment on attachment 8963685 [details]
Bug 1034036 - Part 3: start tracking windows activations to always be aware of their respective order. This allows consumers to iterate over a set of windows in order of appearance (e.g. z-order).

https://reviewboard.mozilla.org/r/232574/#review239204

::: browser/modules/test/browser/browser_BrowserWindowTracker.js:63
(Diff revision 1)
> +    window = BrowserWindowTracker.getTopWindow({ allowPopups: false });
> +    Assert.equal(window, windows[expectedMostRecentIndex],
> +      "Window focused before the private window should be the most recent one.");
> +    let features = "location=no, personalbar=no, toolbar=no, scrollbars=no, menubar=no, status=no";
> +    let popupWindowPromise = BrowserTestUtils.waitForNewWindow();
> +    window.open("about:blank", "_blank", features);

Hmm, how does this work? AFAIK window.open("about:blank", ...) in a chrome context won't open browser.xul.
needinfo for comment 166
Flags: needinfo?(mdeboer)
Comment on attachment 8963686 [details]
Bug 1034036 - Part 4: move away from keeping state on the living objects, like windows, tabs and browsers, but keep it truly privately stored in WeakMaps.

https://reviewboard.mozilla.org/r/232576/#review239590
Attachment #8963686 - Flags: review?(dao+bmo) → review+
Comment on attachment 8963689 [details]
Bug 1034036 - Part 7: Test that we stored window z-indices correctly and in order of creation. Ensure that we restore windows in reverse z-index order.

https://reviewboard.mozilla.org/r/232582/#review239592

::: browser/components/sessionstore/test/browser_restore_reversed_z_order.js:3
(Diff revision 1)
> +"use strict";
> +
> +ChromeUtils.import("resource:///modules/BrowserWindowTracker.jsm");

You already put this in browser.js, so no need to import again.

::: browser/components/sessionstore/test/browser_restore_reversed_z_order.js:17
(Diff revision 1)
> +]);
> +let gBrowserState;
> +
> +add_task(async function setup() {
> +  // Make sure that we start with only primary window.
> +  await promiseAllButPrimaryWindowClosed();

The test framework should enforce this between tests, so doing this at the beginning of a test shouldn't be necessary.

::: browser/components/sessionstore/test/browser_restore_reversed_z_order.js:23
(Diff revision 1)
> +
> +  let windows = [];
> +  let count = 0;
> +  for (let url of gTestURLsMap.keys()) {
> +    let window = !count ? PRIMARY_WINDOW : await BrowserTestUtils.openNewBrowserWindow();
> +    let promise = BrowserTestUtils.browserLoaded(window.gBrowser.selectedBrowser);

s/promise/browserLoaded/

::: browser/components/sessionstore/test/browser_restore_reversed_z_order.js:31
(Diff revision 1)
> +    // Capture the title.
> +    gTestURLsMap.set(url, window.gBrowser.selectedTab.label);
> +    // Minimize the before-last window, to have a different window feature added
> +    // to the test.
> +    if (count == gTestURLsMap.size - 1) {
> +      promise = BrowserTestUtils.waitForEvent(windows[count - 1], "activate");

s/promise/let activated/
Attachment #8963689 - Flags: review?(dao+bmo) → review+
(In reply to Dão Gottwald [::dao] from comment #167)
> Do we still need the setTimeout now that setWindowState is async?

Unfortunately, we still do :(
(In reply to Dão Gottwald [::dao] from comment #166)
> > I wanted to persist the z-index at the time when we're _certain_ that the
> > list is correct. This means that a crash is not that time.
> 
> Why not?
> 
> > The correct times, as I see it, are:
> > 1. During regular sessionstore saves whilst the browser is running normally
> > (in `getCurrentState()`) and
> 
> I don't understand the difference between "when the browser crashes" and
> "while the browser is running normally". When the browser crashes, it was
> presumably running normally up until that point, and so the z-indices should
> be saved along with the windows. Am I missing something?

I think you're right; I now remember that the reason I didn't put this in `_collectWindowData`, is because it is also run when a window closes at a point where the window is not inside the BrowserWindowTracker WeakMap anymore. This means that the z-order is off-by-one at that point. I considered moving this functionality there using a boolean flag passed along that signals when to record the z-order too, but that seemed too messy.
I also see now that you've been looking at an older patch version, which still uses `BrowserWindowTracker.getZIndex(window)` - I removed it upon your request and indeed looked much better! My local m-c checkout was messed up big time, so I'll make sure to push the right ones next time. Sorry about that!
Flags: needinfo?(mdeboer)
Attachment #8963683 - Attachment is obsolete: true
Attachment #8963684 - Attachment is obsolete: true
Attachment #8963685 - Attachment is obsolete: true
Attachment #8963686 - Attachment is obsolete: true
Attachment #8963687 - Attachment is obsolete: true
Attachment #8963687 - Flags: review?(dao+bmo)
Attachment #8963688 - Attachment is obsolete: true
Attachment #8963689 - Attachment is obsolete: true
*Sigh* Why were these patches obsoleted again? Which parts can I rubber-stamp and which ones actually changed?
(In reply to Dão Gottwald [::dao] from comment #184)
> *Sigh* Why were these patches obsoleted again? Which parts can I
> rubber-stamp and which ones actually changed?

Sorry - it was probably because my local repo was in tatters. So you already r+'d all the patches, except Part 5.
Comment on attachment 8965316 [details]
Bug 1034036 - Part 3: start tracking windows activations to always be aware of their respective order. This allows consumers to iterate over a set of windows in order of appearance (e.g. z-order).

https://reviewboard.mozilla.org/r/234048/#review240150

::: browser/modules/test/browser/browser_BrowserWindowTracker.js:64
(Diff revision 1)
> +    Assert.equal(window, windows[expectedMostRecentIndex],
> +      "Window focused before the private window should be the most recent one.");
> +    let popupWindowPromise = BrowserTestUtils.waitForNewWindow();
> +    ContentTask.spawn(gBrowser.selectedBrowser, null, function() {
> +      let features = "location=no, personalbar=no, toolbar=no, scrollbars=no, menubar=no, status=no";
> +      content.window.open("about:blank", "_blank", features);

content == content.window
Attachment #8965316 - Flags: review?(dao+bmo) → review+
Comment on attachment 8965320 [details]
Bug 1034036 - Part 7: Test that we stored window z-indices correctly and in order of creation. Ensure that we restore windows in reverse z-index order.

https://reviewboard.mozilla.org/r/234056/#review240152
Attachment #8965320 - Flags: review?(dao+bmo) → review+
Comment on attachment 8965315 [details]
Bug 1034036 - Part 2: Rename and shorten `getMostRecentBrowserWindow` to `getTopWindow` and modernize the style used in BrowserWindowTracker.jsm.

https://reviewboard.mozilla.org/r/234046/#review240156
Attachment #8965315 - Flags: review?(dao+bmo) → review+
Comment on attachment 8965314 [details]
Bug 1034036 - Part 1: Merge RecentWindow.jsm and UpdateTopLevelContentWindowIDHelper.jsm into one module called 'BrowserWindowTracker.jsm'.

https://reviewboard.mozilla.org/r/234044/#review240154
Attachment #8965314 - Flags: review?(dao+bmo) → review+
Comment on attachment 8965319 [details]
Bug 1034036 - Part 6: Tests that use ss.setBrowserState() or ss.setWindowState() should wait until the window is restored to continue.

https://reviewboard.mozilla.org/r/234054/#review240160
Attachment #8965319 - Flags: review?(dao+bmo) → review+
Comment on attachment 8965317 [details]
Bug 1034036 - Part 4: move away from keeping state on the living objects, like windows, tabs and browsers, but keep it truly privately stored in WeakMaps.

https://reviewboard.mozilla.org/r/234050/#review240158
Attachment #8965317 - Flags: review?(dao+bmo) → review+
Comment on attachment 8965318 [details]
Bug 1034036 - Part 5: restore windows in reverse z-order, so that the last focused window is restored first and stays in front.

https://reviewboard.mozilla.org/r/234052/#review240162

::: browser/components/sessionstore/SessionStore.jsm:708
(Diff revision 1)
>            this._updateSessionStartTime(state);
>  
>            // make sure that at least the first window doesn't have anything hidden
>            delete state.windows[0].hidden;
>            // Since nothing is hidden in the first window, it cannot be a popup
>            delete state.windows[0].isPopup;

Is this still correct?

::: browser/components/sessionstore/SessionStore.jsm:712
(Diff revision 1)
>            // Since nothing is hidden in the first window, it cannot be a popup
>            delete state.windows[0].isPopup;
> -          // We don't want to minimize and then open a window at startup.
> -          if (state.windows[0].sizemode == "minimized")
> +          // We don't want to minimize and then open a window at startup. However,
> +          // when we're restoring, we should preserve previous windows sizemode.
> +          if (state.windows[0].sizemode == "minimized" && !ss.doRestore())
>              state.windows[0].sizemode = "normal";

What's the difference between "at startup" and "when we're restoring"? It sounds like these lines should just be removed?

::: browser/components/sessionstore/SessionStore.jsm:1209
(Diff revision 1)
>        }
>      // this window was opened by _openWindowWithState
>      } else if (!this._isWindowLoaded(aWindow)) {
> -      let state = this._statesToRestore[WINDOW_RESTORE_IDS.get(aWindow)];
> -      let options = {overwriteTabs: true, isFollowUp: state.windows.length == 1};
> -      this.restoreWindow(aWindow, state.windows[0], options);
> +      // We used to restore window when it is opened. However, we want to restore
> +      // windows after all windows have opened (bug 1034036). Thus, the restoration
> +      // process has been moved to `restoreWindowsFeaturesAndTabs`.

I'm not sure how useful it is to document how code used to work.

Also, can this empty if-branch be converted to an early return?

::: browser/components/sessionstore/SessionStore.jsm:1302
(Diff revision 1)
>    onBeforeBrowserWindowShown(aWindow) {
>      // Register the window.
>      this.onLoad(aWindow);
>  
> +    // Dispatch a custom event to tell that a new window is about to be shown.
> +    let evt = new aWindow.CustomEvent("SSBrowserWindowShowing");

Is this going to be useful to outside consumers? SessionStore.jsm communicating with itself through public DOM events seems a bit weird and unprecedented, but I'm not totally against it this is the simplest way to do it.

::: browser/components/sessionstore/SessionStore.jsm:3362
(Diff revision 1)
> +   *
> +   * @param root
> +   *        Windows data
> +   * @returns a promise resolved when all windows have been opened
> +   */
> +  openWindows(root) {

Prefix with underscore? SessionStore.jsm seems to do this inconsistently...

::: browser/components/sessionstore/SessionStore.jsm:3629
(Diff revision 1)
> +   * This function will restore window features and then retore window data.
> +   *
> +   * @param windows
> +   *        ordered array of windows to restore
> +   */
> +  restoreWindowsFeaturesAndTabs(windows) {

_restoreWindowsFeaturesAndTabs?

::: browser/components/sessionstore/SessionStore.jsm:3652
(Diff revision 1)
> +   * be presented with most recently used window first.
> +   *
> +   * @param windows
> +   *        unordered array of windows to restore
> +   */
> +  restoreWindowsInReversedZOrder(windows) {

_restoreWindowsInReversedZOrder?
Comment on attachment 8965318 [details]
Bug 1034036 - Part 5: restore windows in reverse z-order, so that the last focused window is restored first and stays in front.

https://reviewboard.mozilla.org/r/234052/#review240860
Attachment #8965318 - Flags: review?(dao+bmo)
Comment on attachment 8965318 [details]
Bug 1034036 - Part 5: restore windows in reverse z-order, so that the last focused window is restored first and stays in front.

https://reviewboard.mozilla.org/r/234052/#review240914

::: browser/components/sessionstore/SessionStore.jsm:712
(Diff revision 1)
>            // Since nothing is hidden in the first window, it cannot be a popup
>            delete state.windows[0].isPopup;
> -          // We don't want to minimize and then open a window at startup.
> -          if (state.windows[0].sizemode == "minimized")
> +          // We don't want to minimize and then open a window at startup. However,
> +          // when we're restoring, we should preserve previous windows sizemode.
> +          if (state.windows[0].sizemode == "minimized" && !ss.doRestore())
>              state.windows[0].sizemode = "normal";

Great point! All this code has become obsolete indeed. Nice.
Attachment #8965314 - Attachment is obsolete: true
Attachment #8965315 - Attachment is obsolete: true
Attachment #8965316 - Attachment is obsolete: true
Attachment #8965317 - Attachment is obsolete: true
Attachment #8965318 - Attachment is obsolete: true
Attachment #8965319 - Attachment is obsolete: true
Attachment #8965320 - Attachment is obsolete: true
Attachment #8966522 - Flags: review?(dao+bmo) → review+
Attachment #8966523 - Flags: review?(dao+bmo) → review+
Attachment #8966524 - Flags: review?(dao+bmo) → review+
Attachment #8966525 - Flags: review?(dao+bmo) → review+
Attachment #8966527 - Flags: review?(dao+bmo) → review+
Attachment #8966528 - Flags: review?(dao+bmo) → review+
Comment on attachment 8966526 [details]
Bug 1034036 - Part 5: restore windows in reverse z-order, so that the last focused window is restored first and stays in front.

https://reviewboard.mozilla.org/r/235250/#review240970
Attachment #8966526 - Flags: review?(dao+bmo) → review+
Attachment #8966522 - Attachment is obsolete: true
Attachment #8966523 - Attachment is obsolete: true
Attachment #8966524 - Attachment is obsolete: true
Attachment #8966525 - Attachment is obsolete: true
Attachment #8966526 - Attachment is obsolete: true
Attachment #8966527 - Attachment is obsolete: true
Attachment #8966528 - Attachment is obsolete: true
Comment on attachment 8966941 [details]
Bug 1034036 - Part 1: Merge RecentWindow.jsm and UpdateTopLevelContentWindowIDHelper.jsm into one module called 'BrowserWindowTracker.jsm'.

https://reviewboard.mozilla.org/r/235616/#review241328
Attachment #8966941 - Flags: review?(dao+bmo) → review+
Comment on attachment 8966942 [details]
Bug 1034036 - Part 2: Rename and shorten `getMostRecentBrowserWindow` to `getTopWindow` and modernize the style used in BrowserWindowTracker.jsm.

https://reviewboard.mozilla.org/r/235618/#review241330
Attachment #8966942 - Flags: review?(dao+bmo) → review+
Comment on attachment 8966943 [details]
Bug 1034036 - Part 3: start tracking windows activations to always be aware of their respective order. This allows consumers to iterate over a set of windows in order of appearance (e.g. z-order).

https://reviewboard.mozilla.org/r/235620/#review241332
Attachment #8966943 - Flags: review?(dao+bmo) → review+
Comment on attachment 8966944 [details]
Bug 1034036 - Part 4: move away from keeping state on the living objects, like windows, tabs and browsers, but keep it truly privately stored in WeakMaps.

https://reviewboard.mozilla.org/r/235622/#review241334
Attachment #8966944 - Flags: review?(dao+bmo) → review+
Comment on attachment 8966947 [details]
Bug 1034036 - Part 7: Test that we stored window z-indices correctly and in order of creation. Ensure that we restore windows in reverse z-index order.

https://reviewboard.mozilla.org/r/235628/#review241336
Attachment #8966947 - Flags: review?(dao+bmo) → review+
Comment on attachment 8966945 [details]
Bug 1034036 - Part 5: restore windows in reverse z-order, so that the last focused window is restored first and stays in front.

https://reviewboard.mozilla.org/r/235624/#review241338
Attachment #8966945 - Flags: review?(dao+bmo) → review+
Comment on attachment 8966946 [details]
Bug 1034036 - Part 6: Tests that use ss.setBrowserState() or ss.setWindowState() should wait until the window is restored to continue.

https://reviewboard.mozilla.org/r/235626/#review241340
Attachment #8966946 - Flags: review?(dao+bmo) → review+
Comment on attachment 8966944 [details]
Bug 1034036 - Part 4: move away from keeping state on the living objects, like windows, tabs and browsers, but keep it truly privately stored in WeakMaps.

https://reviewboard.mozilla.org/r/235622/#review241598


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/sessionstore/test/browser_speculative_connect.js:32
(Diff revision 2)
>    await BrowserTestUtils.closeWindow(win);
>  
>    // Reopen a window.
>    let newWin = undoCloseWindow(0);
>    // Make sure we wait until this window is restored.
> -  await BrowserTestUtils.waitForEvent(newWin, "load");
> +  await promiseWindowRestored(newWin);

Error: 'promisewindowrestored' is not defined. [eslint: no-undef]

::: browser/components/sessionstore/test/browser_speculative_connect.js:82
(Diff revision 2)
>    await BrowserTestUtils.closeWindow(win);
>  
>    // Reopen a window.
>    let newWin = undoCloseWindow(0);
>    // Make sure we wait until this window is restored.
> -  await BrowserTestUtils.waitForEvent(newWin, "load");
> +  await promiseWindowRestored(newWin);

Error: 'promisewindowrestored' is not defined. [eslint: no-undef]
Comment on attachment 8966944 [details]
Bug 1034036 - Part 4: move away from keeping state on the living objects, like windows, tabs and browsers, but keep it truly privately stored in WeakMaps.

https://reviewboard.mozilla.org/r/235622/#review243012


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/sessionstore/test/browser_speculative_connect.js:32
(Diff revision 3)
>    await BrowserTestUtils.closeWindow(win);
>  
>    // Reopen a window.
>    let newWin = undoCloseWindow(0);
>    // Make sure we wait until this window is restored.
> -  await BrowserTestUtils.waitForEvent(newWin, "load");
> +  await promiseWindowRestored(newWin);

Error: 'promisewindowrestored' is not defined. [eslint: no-undef]

::: browser/components/sessionstore/test/browser_speculative_connect.js:82
(Diff revision 3)
>    await BrowserTestUtils.closeWindow(win);
>  
>    // Reopen a window.
>    let newWin = undoCloseWindow(0);
>    // Make sure we wait until this window is restored.
> -  await BrowserTestUtils.waitForEvent(newWin, "load");
> +  await promiseWindowRestored(newWin);

Error: 'promisewindowrestored' is not defined. [eslint: no-undef]
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/39baa8f0bc04
Part 1: Merge RecentWindow.jsm and UpdateTopLevelContentWindowIDHelper.jsm into one module called 'BrowserWindowTracker.jsm'. r=dao
https://hg.mozilla.org/integration/autoland/rev/b86203bcf975
Part 2: Rename and shorten `getMostRecentBrowserWindow` to `getTopWindow` and modernize the style used in BrowserWindowTracker.jsm. r=dao
https://hg.mozilla.org/integration/autoland/rev/7b590a257f65
Part 3: start tracking windows activations to always be aware of their respective order. This allows consumers to iterate over a set of windows in order of appearance (e.g. z-order). r=dao
https://hg.mozilla.org/integration/autoland/rev/3eb2e99b13c7
Part 4: move away from keeping state on the living objects, like windows, tabs and browsers, but keep it truly privately stored in WeakMaps. r=dao
https://hg.mozilla.org/integration/autoland/rev/6ce9efe2b20d
Part 5: restore windows in reverse z-order, so that the last focused window is restored first and stays in front. r=dao
https://hg.mozilla.org/integration/autoland/rev/dec31c64321f
Part 6: Tests that use ss.setBrowserState() or ss.setWindowState() should wait until the window is restored to continue. r=dao
https://hg.mozilla.org/integration/autoland/rev/74478b16e546
Part 7: Test that we stored window z-indices correctly and in order of creation. Ensure that we restore windows in reverse z-index order. r=dao
See Also: → 1455602
What about the 52.x.x version? (The last one for Win XP).
Blocks: 1489552
Depends on: 1509847
See Also: → 1235231
Regressions: 1551671
You need to log in before you can comment on or make changes to this bug.