[Session Restore] Load windows by descending z-order

RESOLVED FIXED in Firefox 61

Status

()

defect
P2
normal
RESOLVED FIXED
5 years ago
2 days ago

People

(Reporter: Yoric, Assigned: mikedeboer)

Tracking

(Blocks 2 bugs)

unspecified
Firefox 61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

(Whiteboard: [fxperf:p1])

Attachments

(7 attachments, 60 obsolete attachments)

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

Updated

3 years ago
Duplicate of this bug: 497655

Updated

3 years ago
Duplicate of this bug: 1243361
Assignee

Updated

2 years ago
See Also: → 1331935
Assignee

Updated

2 years ago
Blocks: 1331935
See Also: 1331935
Posted 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)
Assignee

Comment 12

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

Updated

2 years ago
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)
Assignee

Comment 14

2 years ago
(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)
Posted 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)
Assignee

Comment 16

2 years ago
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)
Assignee

Updated

2 years ago
Attachment #8882985 - Flags: feedback?(mdeboer)

Updated

2 years ago
Attachment #8883887 - Flags: review?(mdeboer) → feedback?(mdeboer)

Comment 18

2 years 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
Assignee

Comment 19

2 years ago
With your patch applied, only one window is restored for me. Can you investigate that?
Flags: needinfo?(nnn_bikiu0707)
Assignee

Updated

2 years ago
Attachment #8883887 - Flags: feedback?(mdeboer)
Comment hidden (mozreview-request)

Updated

2 years ago
Flags: needinfo?(nnn_bikiu0707)
Attachment #8883887 - Flags: review?(mdeboer) → feedback?(mdeboer)
Assignee

Comment 21

2 years 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/#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?
Assignee

Updated

2 years ago
Attachment #8883887 - Flags: feedback?(mdeboer) → feedback+

Comment 22

2 years 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)
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.
Assignee

Comment 25

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

Updated

2 years ago
Attachment #8883887 - Flags: review?(mdeboer) → review?(dao+bmo)
Comment hidden (mozreview-request)

Comment 29

2 years 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

2 years 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

2 years ago
Attachment #8883887 - Flags: review?(mdeboer)

Updated

2 years ago
Attachment #8890263 - Flags: review?(mdeboer)

Updated

2 years 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)
Attachment #8890264 - Flags: review?(dao+bmo)
Attachment #8890263 - Flags: review?(dao+bmo)
Assignee

Comment 32

2 years 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 ;)
Assignee

Comment 33

2 years 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/#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-
Assignee

Comment 34

2 years ago
mozreview-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-
Assignee

Comment 35

2 years 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/#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

2 years 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

2 years 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

2 years 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)
Assignee

Comment 42

2 years 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/#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+
Assignee

Comment 43

2 years ago
mozreview-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)
Assignee

Comment 44

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

2 years 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

2 years 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)
Assignee

Comment 51

2 years ago
mozreview-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/#review174042
Attachment #8890263 - Flags: review?(mdeboer) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(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)
Assignee

Updated

2 years ago
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)
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)
Assignee

Comment 65

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

2 years 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
Requesting a review from Mike so you can see my lastest changes.
Flags: needinfo?(nnn_bikiu0707)

Updated

2 years ago
Attachment #8883887 - Flags: review+ → review?(mdeboer)
Assignee

Comment 71

2 years 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/#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)
(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)
(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.
Assignee

Comment 80

2 years ago
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 years 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 years ago
Flags: needinfo?(nnn_bikiu0707)

Comment 85

2 years 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 years ago
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
Flags: needinfo?(nnn_bikiu0707)
Resolution: FIXED → ---
Target Milestone: Firefox 57 → ---
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 years 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!
Assignee

Updated

2 years ago
Duplicate of this bug: 1411485
Assignee

Comment 92

2 years ago
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)

Comment 93

a year ago
@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.
Assignee

Comment 94

a year ago
Driving this home.
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Assignee

Updated

a year ago
Priority: -- → P2
Assignee

Updated

a year ago
Attachment #8883887 - Attachment is obsolete: true
Assignee

Updated

a year ago
Attachment #8890263 - Attachment is obsolete: true
Assignee

Updated

a year ago
Attachment #8890264 - Attachment is obsolete: true
Assignee

Comment 98

a year ago
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 99

a year ago
mozreview-review
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 101

a year ago
mozreview-review
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 102

a year ago
mozreview-review
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)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

a year ago
Attachment #8946675 - Attachment is obsolete: true
Assignee

Updated

a year ago
Attachment #8946676 - Attachment is obsolete: true
Assignee

Updated

a year ago
Attachment #8946677 - Attachment is obsolete: true
Assignee

Comment 112

a year ago
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 113

a year ago
mozreview-review
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)
Assignee

Updated

a year ago
Attachment #8953441 - Flags: review?(dao+bmo)
Assignee

Updated

a year ago
Attachment #8953442 - Flags: review?(dao+bmo)
Assignee

Updated

a year ago
Attachment #8953443 - Flags: review?(dao+bmo)
Assignee

Updated

a year ago
Attachment #8953444 - Flags: review?(dao+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

a year ago
Attachment #8953440 - Attachment is obsolete: true
Assignee

Updated

a year ago
Attachment #8953441 - Attachment is obsolete: true
Assignee

Updated

a year ago
Attachment #8953442 - Attachment is obsolete: true
Assignee

Updated

a year ago
Attachment #8953443 - Attachment is obsolete: true
Assignee

Updated

a year ago
Attachment #8953444 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

a year ago
Attachment #8954354 - Attachment is obsolete: true
Attachment #8954354 - Flags: review?(dao+bmo)
Assignee

Updated

a year ago
Attachment #8954355 - Attachment is obsolete: true
Attachment #8954355 - Flags: review?(dao+bmo)
Assignee

Updated

a year ago
Attachment #8954356 - Attachment is obsolete: true
Attachment #8954356 - Flags: review?(dao+bmo)
Assignee

Updated

a year ago
Attachment #8954357 - Attachment is obsolete: true
Attachment #8954357 - Flags: review?(dao+bmo)
Assignee

Updated

a year ago
Attachment #8954358 - Attachment is obsolete: true
Attachment #8954358 - Flags: review?(dao+bmo)
Assignee

Updated

a year ago
Attachment #8954359 - Attachment is obsolete: true
Attachment #8954359 - Flags: review?(dao+bmo)

Comment 129

a year ago
mozreview-review
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
Assignee

Comment 130

a year ago
(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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

a year ago
Attachment #8954401 - Attachment is obsolete: true
Attachment #8954401 - Flags: review?(dao+bmo)
Assignee

Updated

a year ago
Attachment #8954402 - Attachment is obsolete: true
Attachment #8954402 - Flags: review?(dao+bmo)
Assignee

Updated

a year ago
Attachment #8954403 - Attachment is obsolete: true
Attachment #8954403 - Flags: review?(dao+bmo)
Assignee

Updated

a year ago
Attachment #8954404 - Attachment is obsolete: true
Attachment #8954404 - Flags: review?(dao+bmo)
Assignee

Updated

a year ago
Attachment #8954405 - Attachment is obsolete: true
Attachment #8954405 - Flags: review?(dao+bmo)
Assignee

Updated

a year ago
Attachment #8954406 - Attachment is obsolete: true
Attachment #8954406 - Flags: review?(dao+bmo)

Comment 139

a year ago
mozreview-review
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 140

a year ago
mozreview-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 141

a year ago
mozreview-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 142

a year ago
mozreview-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 143

a year ago
mozreview-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 144

a year ago
mozreview-review
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 145

a year ago
mozreview-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)
Assignee

Comment 146

a year ago
mozreview-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/#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]
Assignee

Comment 147

a year ago
mozreview-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/#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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

a year ago
Attachment #8955463 - Attachment is obsolete: true
Assignee

Updated

a year ago
Attachment #8955464 - Attachment is obsolete: true
Assignee

Updated

a year ago
Attachment #8955465 - Attachment is obsolete: true
Assignee

Updated

a year ago
Attachment #8955466 - Attachment is obsolete: true
Assignee

Updated

a year ago
Attachment #8955467 - Attachment is obsolete: true
Assignee

Updated

a year ago
Attachment #8955468 - Attachment is obsolete: true
Assignee

Updated

a year ago
Attachment #8955469 - Attachment is obsolete: true
Attachment #8955469 - Flags: review?(dao+bmo)
Assignee

Comment 155

a year ago
Sorry Dão, Reviewboard decided to give you a hard time and clear all the flags you set before... Grrr!
Assignee

Comment 156

a year ago
A needinfo for comment 146 and comment 147!
Flags: needinfo?(dao+bmo)

Comment 157

a year ago
mozreview-review
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 158

a year ago
mozreview-review
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]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

a year ago
Attachment #8963675 - Attachment is obsolete: true
Attachment #8963675 - Flags: review?(dao+bmo)
Assignee

Updated

a year ago
Attachment #8963676 - Attachment is obsolete: true
Attachment #8963676 - Flags: review?(dao+bmo)
Assignee

Updated

a year ago
Attachment #8963677 - Attachment is obsolete: true
Attachment #8963677 - Flags: review?(dao+bmo)
Assignee

Updated

a year ago
Attachment #8963678 - Attachment is obsolete: true
Attachment #8963678 - Flags: review?(dao+bmo)
Assignee

Updated

a year ago
Attachment #8963679 - Attachment is obsolete: true
Attachment #8963679 - Flags: review?(dao+bmo)
Assignee

Updated

a year ago
Attachment #8963680 - Attachment is obsolete: true
Attachment #8963680 - Flags: review?(dao+bmo)
Assignee

Updated

a year ago
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 167

a year ago
mozreview-review
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 168

a year ago
mozreview-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 169

a year ago
mozreview-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 170

a year ago
mozreview-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 171

a year ago
mozreview-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 173

a year ago
mozreview-review
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 174

a year ago
mozreview-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+
Assignee

Comment 175

a year ago
(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 :(
Assignee

Comment 176

a year ago
(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)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

a year ago
Attachment #8963683 - Attachment is obsolete: true
Assignee

Updated

a year ago
Attachment #8963684 - Attachment is obsolete: true
Assignee

Updated

a year ago
Attachment #8963685 - Attachment is obsolete: true
Assignee

Updated

a year ago
Attachment #8963686 - Attachment is obsolete: true
Assignee

Updated

a year ago
Attachment #8963687 - Attachment is obsolete: true
Attachment #8963687 - Flags: review?(dao+bmo)
Assignee

Updated

a year ago
Attachment #8963688 - Attachment is obsolete: true
Assignee

Updated

a year ago
Attachment #8963689 - Attachment is obsolete: true
*Sigh* Why were these patches obsoleted again? Which parts can I rubber-stamp and which ones actually changed?
Assignee

Comment 185

a year ago
(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 186

a year ago
mozreview-review
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 187

a year ago
mozreview-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 188

a year ago
mozreview-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 189

a year ago
mozreview-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 190

a year ago
mozreview-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 191

a year ago
mozreview-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 192

a year ago
mozreview-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 193

a year ago
mozreview-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/#review240860
Attachment #8965318 - Flags: review?(dao+bmo)
Assignee

Comment 194

a year ago
mozreview-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/#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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

a year ago
Attachment #8965314 - Attachment is obsolete: true
Assignee

Updated

a year ago
Attachment #8965315 - Attachment is obsolete: true
Assignee

Updated

a year ago
Attachment #8965316 - Attachment is obsolete: true
Assignee

Updated

a year ago
Attachment #8965317 - Attachment is obsolete: true
Assignee

Updated

a year ago
Attachment #8965318 - Attachment is obsolete: true