[Session Restore] Load windows by descending z-order

NEW
Unassigned

Status

()

Firefox
Session Restore
3 years ago
26 days ago

People

(Reporter: Yoric, Unassigned)

Tracking

(Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 2 obsolete attachments)

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.
Blocks: 1034534
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.
The article at http://www.codeproject.com/Articles/10497/A-tool-to-order-the-window-buttons-in-your-taskbar suggests that `ShowWindow` would do the trick - http://msdn.microsoft.com/en-us/library/windows/desktop/ms633548%28v=vs.85%29.aspx
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

Updated

2 years ago
Duplicate of this bug: 497655
Duplicate of this bug: 1243361
See Also: → bug 1331935
Blocks: 1331935
See Also: bug 1331935

Comment 11

5 months ago
Created attachment 8880260 [details] [diff] [review]
z_order.v1.patch

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

Comment 13

5 months ago
(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)

Comment 15

5 months ago
Created attachment 8882985 [details] [diff] [review]
z_order.v2.patch

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)
Comment hidden (mozreview-request)

Updated

5 months ago
Attachment #8883887 - Flags: review?(mdeboer) → feedback?(mdeboer)

Comment 18

5 months ago
mozreview-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/#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)
Comment hidden (mozreview-request)

Updated

4 months ago
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 22

4 months ago
mozreview-review-reply
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.
Comment hidden (mozreview-request)

Comment 24

4 months ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 29

4 months ago
mozreview-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/#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 30

4 months ago
mozreview-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

::: 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?

Updated

4 months ago
Attachment #8883887 - Flags: review?(mdeboer)

Updated

4 months ago
Attachment #8890263 - Flags: review?(mdeboer)

Updated

4 months ago
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)

Updated

3 months ago
Attachment #8890264 - Flags: review?(dao+bmo)

Updated

3 months ago
Attachment #8890263 - Flags: review?(dao+bmo)

Comment 32

3 months ago
mozreview-review-reply
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 36

3 months ago
mozreview-review-reply
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 37

3 months ago
mozreview-review-reply
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 38

3 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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 45

3 months ago
mozreview-review-reply
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 49

3 months ago
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 hidden (mozreview-request)
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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 54

3 months ago
(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)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8882985 - Attachment is obsolete: true
Flags: needinfo?(mdeboer)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 64

3 months ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 69

3 months ago
mozreview-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/#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

Comment 70

3 months ago
Requesting a review from Mike so you can see my lastest changes.
Flags: needinfo?(nnn_bikiu0707)

Updated

3 months ago
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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 75

3 months ago
(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. :)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 78

3 months ago
(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)

Comment 79

3 months ago
(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)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 84

2 months ago
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

Updated

2 months ago
Flags: needinfo?(nnn_bikiu0707)

Comment 85

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1576132e98a3
https://hg.mozilla.org/mozilla-central/rev/b9fdf5d1b402
https://hg.mozilla.org/mozilla-central/rev/39a94e16a369
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
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
status-firefox57: fixed → ---
Flags: needinfo?(nnn_bikiu0707)
Resolution: FIXED → ---
Target Milestone: Firefox 57 → ---
Duplicate of this bug: 1398384
Depends on: 1398464
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

Comment 89

2 months ago
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!
Duplicate of this bug: 1411485
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)
You need to log in before you can comment on or make changes to this bug.