Closed
Bug 1034036
Opened 10 years ago
Closed 7 years ago
[Session Restore] Load windows by descending z-order
Categories
(Firefox :: Session Restore, defect, P2)
Firefox
Session Restore
Tracking
()
RESOLVED
FIXED
Firefox 61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: Yoric, Assigned: mikedeboer)
References
(Blocks 2 open bugs)
Details
(Whiteboard: [fxperf:p1])
Attachments
(7 files, 60 obsolete files)
(deleted),
text/x-review-board-request
|
dao
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
dao
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
dao
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
dao
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
dao
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
dao
:
review+
|
Details |
(deleted),
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.
Comment 1•10 years ago
|
||
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.
Comment 2•10 years ago
|
||
So there's bug 712763 and bug 669272. This here is a dupe of the latter.
Reporter | ||
Comment 3•10 years ago
|
||
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
Comment 4•10 years ago
|
||
No, please read the two bugs I linked. There was a reason we reverted the change.
Reporter | ||
Comment 5•10 years ago
|
||
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.
Reporter | ||
Comment 6•10 years ago
|
||
The article at http://www.codeproject.com/Articles/10497/A-tool-to-order-the-window-buttons-in-your-taskbar suggests that `ShowWindow` would do the trick - http://msdn.microsoft.com/en-us/library/windows/desktop/ms633548%28v=vs.85%29.aspx
Comment 7•10 years ago
|
||
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
Reporter | ||
Comment 8•10 years ago
|
||
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
Assignee | ||
Updated•7 years ago
|
Comment 11•7 years ago
|
||
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•7 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•7 years ago
|
Assignee: nobody → nnn_bikiu0707
Status: NEW → ASSIGNED
Comment 13•7 years ago
|
||
(In reply to Mike de Boer [:mikedeboer] from comment #12)
> Comment on attachment 8880260 [details] [diff] [review]
> z_order.v1.patch
>
> Review of attachment 8880260 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> @@ +3051,5 @@
> > + let mostRecentWindow = this._getMostRecentBrowserWindow();
> > + return mostRecentWindow === aWindow ? 1 : 0;
> > + }
> > +
> > + let windowList = Services.wm.getZOrderDOMWindowEnumerator("navigator:browser", true);
>
> I think you want to add behavior to cache the most recent browser window and
> the list of windows this enumerator returns during the run of
> `_forEachBrowserWindow`, which iterates over all windows.
> In fact, it might be quite handy to use this iterator in that method instead
> and somehow pass the z-index data along with it.
>
So you want me to use _forEachBrowserWindow in that function. But I think that enumerator doesn't iterate windows in z-index order. Maybe I should modified the behaviour of _forEachBrowserWindow?
Flags: needinfo?(mdeboer)
Assignee | ||
Comment 14•7 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)
Comment 15•7 years ago
|
||
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•7 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•7 years ago
|
Attachment #8882985 -
Flags: feedback?(mdeboer)
Comment hidden (mozreview-request) |
Updated•7 years ago
|
Attachment #8883887 -
Flags: review?(mdeboer) → feedback?(mdeboer)
Comment 18•7 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•7 years ago
|
||
With your patch applied, only one window is restored for me. Can you investigate that?
Flags: needinfo?(nnn_bikiu0707)
Assignee | ||
Updated•7 years ago
|
Attachment #8883887 -
Flags: feedback?(mdeboer)
Comment hidden (mozreview-request) |
Updated•7 years ago
|
Flags: needinfo?(nnn_bikiu0707)
Attachment #8883887 -
Flags: review?(mdeboer) → feedback?(mdeboer)
Assignee | ||
Comment 21•7 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•7 years ago
|
Attachment #8883887 -
Flags: feedback?(mdeboer) → feedback+
Comment 22•7 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) |
Comment 24•7 years ago
|
||
For testing, I think we could test the following:
Preparation:
+ Open 3 windows, each windows should have two tabs.
Testing:
1. Close all three windows and test whether the z-order is stored correctly.
2. Reopen all windows and the restoration order should be in reversed z-order.
Assignee | ||
Comment 25•7 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•7 years ago
|
Attachment #8883887 -
Flags: review?(mdeboer) → review?(dao+bmo)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 29•7 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•7 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•7 years ago
|
Attachment #8883887 -
Flags: review?(mdeboer)
Updated•7 years ago
|
Attachment #8890263 -
Flags: review?(mdeboer)
Updated•7 years ago
|
Attachment #8890264 -
Flags: review?(mdeboer)
Comment 31•7 years ago
|
||
Comment on attachment 8883887 [details]
Bug 1034036 - Part 1: Separate windows opening and windows restoration process. Make windows restored in reversed z-order.
Sorry, I'm too swamped with review requests and other work. Hopefully Mike can get to this sooner.
Attachment #8883887 -
Flags: review?(dao+bmo)
Updated•7 years ago
|
Attachment #8890264 -
Flags: review?(dao+bmo)
Updated•7 years ago
|
Attachment #8890263 -
Flags: review?(dao+bmo)
Assignee | ||
Comment 32•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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) |
Comment 54•7 years ago
|
||
(In reply to Bao Quan [:beekill] from comment #52)
> Comment on attachment 8890263 [details]
> Bug 1034036 - Part 2: Test that we stored correct z-indices and windows
> creation order, ensure we restore windows in reversed z-index.
>
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/161382/diff/3-4/
I added a new statement `promiseAllButPrimaryWindowClosed()` to ensure that we start the test with only primary window (the test previously failed on macOS because of another window existed when the test started).
For some reason that I don't know, this test keeps failing in Window 7 debug and sometimes in Window 7 optimized. Looking at the log, it is waiting for `promiseAllButPrimaryWindowClosed()` to resolve [1].
I run the test on my machine (Window 10) and everything is fine. Not sure what is wrong with Window 7 and `promiseAllButPrimaryWindowClosed()`.
Mike, can you take a look at this?
[1]: https://treeherder.mozilla.org/logviewer.html#?job_id=123179567&repo=try&lineNumber=34556-34561
Flags: needinfo?(mdeboer)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 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) |
Comment 64•7 years ago
|
||
I updated the patch and redid the try server. Here is the result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f22188b50d47&selectedJob=125187793
The result looks ok to me. Mike, is this ok to land this?
Flags: needinfo?(mdeboer)
Assignee | ||
Comment 65•7 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•7 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
Comment 70•7 years ago
|
||
Requesting a review from Mike so you can see my lastest changes.
Flags: needinfo?(nnn_bikiu0707)
Updated•7 years ago
|
Attachment #8883887 -
Flags: review+ → review?(mdeboer)
Assignee | ||
Comment 71•7 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) |
Comment 75•7 years ago
|
||
(In reply to Bao Quan [:beekill] from comment #72)
> Comment on attachment 8883887 [details]
> Bug 1034036 - Part 1: Separate windows opening and windows restoration
> process. Make windows restored in reversed z-order.
>
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/154880/diff/10-11/
Updated to lastest build, fixed nits and redid try server to make sure I'm not making any mistake. :)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 78•7 years ago
|
||
(In reply to Bao Quan [:beekill] from comment #76)
> Comment on attachment 8890263 [details]
> Bug 1034036 - Part 2: Test that we stored correct z-indices and windows
> creation order, ensure we restore windows in reversed z-index.
>
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/161382/diff/9-10/
Hehe, I fixed the failures on Win 7 and stylo build. It actually was freeze at `await Promise.all(promises)`. I did a few logging and found that some tabs were flushed when they were about:newtab. And thus, when about pages were loaded into tabs, those flushed message were ignored [1] and [2] and thus, the promises were never resolved.
The result from try server look goods, I think this is ready to land. Mike, can you push this?
I wonder, should [1] and [2] do something more like resolve a promise when a flush is requested, not just a plain return statement? That way, we could possibly remove a pending promise blocking something.
[1]: http://searchfox.org/mozilla-central/source/browser/components/sessionstore/content/content-sessionStore.js#823
[2]: http://searchfox.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#845
Flags: needinfo?(mdeboer)
Comment 79•7 years ago
|
||
(In reply to Bao Quan [:beekill] from comment #78)
> (In reply to Bao Quan [:beekill] from comment #76)
> > Comment on attachment 8890263 [details]
> > Bug 1034036 - Part 2: Test that we stored correct z-indices and windows
> > creation order, ensure we restore windows in reversed z-index.
> >
> > Review request updated; see interdiff:
> > https://reviewboard.mozilla.org/r/161382/diff/9-10/
>
> Hehe, I fixed the failures on Win 7 and stylo build. It actually was freeze
> at `await Promise.all(promises)`. I did a few logging and found that some
> tabs were flushed when they were about:newtab. And thus, when about pages
> were loaded into tabs, those flushed message were ignored [1] and [2] and
> thus, the promises were never resolved.
>
> The result from try server look goods, I think this is ready to land. Mike,
> can you push this?
>
> I wonder, should [1] and [2] do something more like resolve a promise when a
> flush is requested, not just a plain return statement? That way, we could
> possibly remove a pending promise blocking something.
>
> [1]:
> http://searchfox.org/mozilla-central/source/browser/components/sessionstore/
> content/content-sessionStore.js#823
> [2]:
> http://searchfox.org/mozilla-central/source/browser/components/sessionstore/
> SessionStore.jsm#845
So what I did to solve the problem is to ensure we already loaded about pages before request a flush.
Assignee | ||
Comment 80•7 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•7 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•7 years ago
|
Flags: needinfo?(nnn_bikiu0707)
Comment 85•7 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
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Comment 86•7 years ago
|
||
Backed out for frequently failing browser-chrome's browser/components/sessionstore/test/browser_restore_reversed_z_order.js on OS X:
https://hg.mozilla.org/mozilla-central/rev/2a9cffb19ab58a7875aee5492c565c549c037511
https://hg.mozilla.org/mozilla-central/rev/716e6c452539e2d99d7af913e20aa9c38101addb
https://hg.mozilla.org/mozilla-central/rev/9506d68397d7646ab7b668d5e624da7639107dfc
Range with push: https://treeherder.mozilla.org/#/jobs?repo=autoland&tochange=39a94e16a3694c443a985081ac1c6cd761363da2&fromchange=6af4edb9979ef0c3f2d6ae9fe0afc9249d34c32c&filter-searchStr=10.10%20browser-chrome
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=129773419&repo=autoland
03:24:27 INFO - Leaving test bound init
03:24:27 INFO - Entering test bound test_z_indices_are_saved_correctly
03:24:27 INFO - Buffered messages finished
03:24:27 INFO - TEST-UNEXPECTED-FAIL | browser/components/sessionstore/test/browser_restore_reversed_z_order.js | Correct number of windows saved - Got 5, expected 4
03:24:27 INFO - Stack trace:
03:24:27 INFO - chrome://mochikit/content/browser-test.js:test_is:1007
03:24:27 INFO - chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_restore_reversed_z_order.js:test_z_indices_are_saved_correctly:50
03:24:27 INFO - Tester_execTest@chrome://mochikit/content/browser-test.js:794:9
03:24:27 INFO - Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:694:7
03:24:27 INFO - SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59
Status: RESOLVED → REOPENED
status-firefox57:
fixed → ---
Flags: needinfo?(nnn_bikiu0707)
Resolution: FIXED → ---
Target Milestone: Firefox 57 → ---
Comment 88•7 years ago
|
||
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•7 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?
Comment 90•7 years ago
|
||
:palswim You are right.
Sorry, I pasted the results in the wrong bug. Thanks for the notice!
Assignee | ||
Comment 92•7 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•7 years 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•7 years ago
|
||
Driving this home.
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Assignee | ||
Updated•7 years ago
|
Priority: -- → P2
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8883887 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8890263 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8890264 -
Attachment is obsolete: true
Assignee | ||
Comment 98•7 years 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•7 years 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)
Comment 100•7 years ago
|
||
(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•7 years 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•7 years 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)
Assignee | ||
Comment 103•7 years ago
|
||
Assignee | ||
Comment 104•7 years ago
|
||
Assignee | ||
Comment 105•7 years ago
|
||
Assignee | ||
Comment 106•7 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8946675 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8946676 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8946677 -
Attachment is obsolete: true
Assignee | ||
Comment 112•7 years 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•7 years 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•7 years ago
|
Attachment #8953441 -
Flags: review?(dao+bmo)
Assignee | ||
Updated•7 years ago
|
Attachment #8953442 -
Flags: review?(dao+bmo)
Assignee | ||
Updated•7 years ago
|
Attachment #8953443 -
Flags: review?(dao+bmo)
Assignee | ||
Updated•7 years ago
|
Attachment #8953444 -
Flags: review?(dao+bmo)
Assignee | ||
Comment 114•7 years ago
|
||
Assignee | ||
Comment 115•7 years ago
|
||
Assignee | ||
Comment 116•7 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8953440 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8953441 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8953442 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8953443 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8953444 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8954354 -
Attachment is obsolete: true
Attachment #8954354 -
Flags: review?(dao+bmo)
Assignee | ||
Updated•7 years ago
|
Attachment #8954355 -
Attachment is obsolete: true
Attachment #8954355 -
Flags: review?(dao+bmo)
Assignee | ||
Updated•7 years ago
|
Attachment #8954356 -
Attachment is obsolete: true
Attachment #8954356 -
Flags: review?(dao+bmo)
Assignee | ||
Updated•7 years ago
|
Attachment #8954357 -
Attachment is obsolete: true
Attachment #8954357 -
Flags: review?(dao+bmo)
Assignee | ||
Updated•7 years ago
|
Attachment #8954358 -
Attachment is obsolete: true
Attachment #8954358 -
Flags: review?(dao+bmo)
Assignee | ||
Updated•7 years ago
|
Attachment #8954359 -
Attachment is obsolete: true
Attachment #8954359 -
Flags: review?(dao+bmo)
Comment 129•7 years 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•7 years 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.
Comment 131•7 years ago
|
||
(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) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8954401 -
Attachment is obsolete: true
Attachment #8954401 -
Flags: review?(dao+bmo)
Assignee | ||
Updated•7 years ago
|
Attachment #8954402 -
Attachment is obsolete: true
Attachment #8954402 -
Flags: review?(dao+bmo)
Assignee | ||
Updated•7 years ago
|
Attachment #8954403 -
Attachment is obsolete: true
Attachment #8954403 -
Flags: review?(dao+bmo)
Assignee | ||
Updated•7 years ago
|
Attachment #8954404 -
Attachment is obsolete: true
Attachment #8954404 -
Flags: review?(dao+bmo)
Assignee | ||
Updated•7 years ago
|
Attachment #8954405 -
Attachment is obsolete: true
Attachment #8954405 -
Flags: review?(dao+bmo)
Assignee | ||
Updated•7 years ago
|
Attachment #8954406 -
Attachment is obsolete: true
Attachment #8954406 -
Flags: review?(dao+bmo)
Comment 139•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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?
Updated•7 years ago
|
Whiteboard: [fxperf]
Updated•7 years ago
|
Attachment #8955467 -
Flags: review?(dao+bmo)
Assignee | ||
Comment 146•7 years 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?
Updated•7 years ago
|
Whiteboard: [fxperf] → [fxperf:p1]
Assignee | ||
Comment 147•7 years 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) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8955463 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8955464 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8955465 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8955466 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8955467 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8955468 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8955469 -
Attachment is obsolete: true
Attachment #8955469 -
Flags: review?(dao+bmo)
Assignee | ||
Comment 155•7 years ago
|
||
Sorry Dão, Reviewboard decided to give you a hard time and clear all the flags you set before... Grrr!
Assignee | ||
Comment 156•7 years ago
|
||
A needinfo for comment 146 and comment 147!
Flags: needinfo?(dao+bmo)
Comment 157•7 years 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•7 years 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) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8963675 -
Attachment is obsolete: true
Attachment #8963675 -
Flags: review?(dao+bmo)
Assignee | ||
Updated•7 years ago
|
Attachment #8963676 -
Attachment is obsolete: true
Attachment #8963676 -
Flags: review?(dao+bmo)
Assignee | ||
Updated•7 years ago
|
Attachment #8963677 -
Attachment is obsolete: true
Attachment #8963677 -
Flags: review?(dao+bmo)
Assignee | ||
Updated•7 years ago
|
Attachment #8963678 -
Attachment is obsolete: true
Attachment #8963678 -
Flags: review?(dao+bmo)
Assignee | ||
Updated•7 years ago
|
Attachment #8963679 -
Attachment is obsolete: true
Attachment #8963679 -
Flags: review?(dao+bmo)
Assignee | ||
Updated•7 years ago
|
Attachment #8963680 -
Attachment is obsolete: true
Attachment #8963680 -
Flags: review?(dao+bmo)
Assignee | ||
Updated•7 years ago
|
Attachment #8963681 -
Attachment is obsolete: true
Attachment #8963681 -
Flags: review?(dao+bmo)
Comment 166•7 years ago
|
||
(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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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.
Comment 173•7 years 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•7 years 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•7 years 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•7 years 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) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8963683 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8963684 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8963685 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8963686 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8963687 -
Attachment is obsolete: true
Attachment #8963687 -
Flags: review?(dao+bmo)
Assignee | ||
Updated•7 years ago
|
Attachment #8963688 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8963689 -
Attachment is obsolete: true
Comment 184•7 years ago
|
||
*Sigh* Why were these patches obsoleted again? Which parts can I rubber-stamp and which ones actually changed?
Assignee | ||
Comment 185•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8965314 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8965315 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8965316 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8965317 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8965318 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8965319 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8965320 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8966522 -
Flags: review?(dao+bmo) → review+
Assignee | ||
Updated•7 years ago
|
Attachment #8966523 -
Flags: review?(dao+bmo) → review+
Assignee | ||
Updated•7 years ago
|
Attachment #8966524 -
Flags: review?(dao+bmo) → review+
Assignee | ||
Updated•7 years ago
|
Attachment #8966525 -
Flags: review?(dao+bmo) → review+
Assignee | ||
Updated•7 years ago
|
Attachment #8966527 -
Flags: review?(dao+bmo) → review+
Assignee | ||
Updated•7 years ago
|
Attachment #8966528 -
Flags: review?(dao+bmo) → review+
Comment 202•7 years ago
|
||
mozreview-review |
Comment on attachment 8966526 [details]
Bug 1034036 - Part 5: restore windows in reverse z-order, so that the last focused window is restored first and stays in front.
https://reviewboard.mozilla.org/r/235250/#review240970
Attachment #8966526 -
Flags: review?(dao+bmo) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8966522 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8966523 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8966524 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8966525 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8966526 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8966527 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8966528 -
Attachment is obsolete: true
Comment 210•7 years ago
|
||
mozreview-review |
Comment on attachment 8966941 [details]
Bug 1034036 - Part 1: Merge RecentWindow.jsm and UpdateTopLevelContentWindowIDHelper.jsm into one module called 'BrowserWindowTracker.jsm'.
https://reviewboard.mozilla.org/r/235616/#review241328
Attachment #8966941 -
Flags: review?(dao+bmo) → review+
Comment 211•7 years ago
|
||
mozreview-review |
Comment on attachment 8966942 [details]
Bug 1034036 - Part 2: Rename and shorten `getMostRecentBrowserWindow` to `getTopWindow` and modernize the style used in BrowserWindowTracker.jsm.
https://reviewboard.mozilla.org/r/235618/#review241330
Attachment #8966942 -
Flags: review?(dao+bmo) → review+
Comment 212•7 years ago
|
||
mozreview-review |
Comment on attachment 8966943 [details]
Bug 1034036 - Part 3: start tracking windows activations to always be aware of their respective order. This allows consumers to iterate over a set of windows in order of appearance (e.g. z-order).
https://reviewboard.mozilla.org/r/235620/#review241332
Attachment #8966943 -
Flags: review?(dao+bmo) → review+
Comment 213•7 years ago
|
||
mozreview-review |
Comment on attachment 8966944 [details]
Bug 1034036 - Part 4: move away from keeping state on the living objects, like windows, tabs and browsers, but keep it truly privately stored in WeakMaps.
https://reviewboard.mozilla.org/r/235622/#review241334
Attachment #8966944 -
Flags: review?(dao+bmo) → review+
Comment 214•7 years ago
|
||
mozreview-review |
Comment on attachment 8966947 [details]
Bug 1034036 - Part 7: Test that we stored window z-indices correctly and in order of creation. Ensure that we restore windows in reverse z-index order.
https://reviewboard.mozilla.org/r/235628/#review241336
Attachment #8966947 -
Flags: review?(dao+bmo) → review+
Comment 215•7 years ago
|
||
mozreview-review |
Comment on attachment 8966945 [details]
Bug 1034036 - Part 5: restore windows in reverse z-order, so that the last focused window is restored first and stays in front.
https://reviewboard.mozilla.org/r/235624/#review241338
Attachment #8966945 -
Flags: review?(dao+bmo) → review+
Comment 216•7 years ago
|
||
mozreview-review |
Comment on attachment 8966946 [details]
Bug 1034036 - Part 6: Tests that use ss.setBrowserState() or ss.setWindowState() should wait until the window is restored to continue.
https://reviewboard.mozilla.org/r/235626/#review241340
Attachment #8966946 -
Flags: review?(dao+bmo) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 221•7 years ago
|
||
mozreview-review |
Comment on attachment 8966944 [details]
Bug 1034036 - Part 4: move away from keeping state on the living objects, like windows, tabs and browsers, but keep it truly privately stored in WeakMaps.
https://reviewboard.mozilla.org/r/235622/#review241598
Code analysis found 2 defects in this patch:
- 2 defects found by mozlint
You can run this analysis locally with:
- `./mach lint path/to/file` (JS/Python)
If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx
::: browser/components/sessionstore/test/browser_speculative_connect.js:32
(Diff revision 2)
> await BrowserTestUtils.closeWindow(win);
>
> // Reopen a window.
> let newWin = undoCloseWindow(0);
> // Make sure we wait until this window is restored.
> - await BrowserTestUtils.waitForEvent(newWin, "load");
> + await promiseWindowRestored(newWin);
Error: 'promisewindowrestored' is not defined. [eslint: no-undef]
::: browser/components/sessionstore/test/browser_speculative_connect.js:82
(Diff revision 2)
> await BrowserTestUtils.closeWindow(win);
>
> // Reopen a window.
> let newWin = undoCloseWindow(0);
> // Make sure we wait until this window is restored.
> - await BrowserTestUtils.waitForEvent(newWin, "load");
> + await promiseWindowRestored(newWin);
Error: 'promisewindowrestored' is not defined. [eslint: no-undef]
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 229•7 years ago
|
||
mozreview-review |
Comment on attachment 8966944 [details]
Bug 1034036 - Part 4: move away from keeping state on the living objects, like windows, tabs and browsers, but keep it truly privately stored in WeakMaps.
https://reviewboard.mozilla.org/r/235622/#review243012
Code analysis found 2 defects in this patch:
- 2 defects found by mozlint
You can run this analysis locally with:
- `./mach lint path/to/file` (JS/Python)
If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx
::: browser/components/sessionstore/test/browser_speculative_connect.js:32
(Diff revision 3)
> await BrowserTestUtils.closeWindow(win);
>
> // Reopen a window.
> let newWin = undoCloseWindow(0);
> // Make sure we wait until this window is restored.
> - await BrowserTestUtils.waitForEvent(newWin, "load");
> + await promiseWindowRestored(newWin);
Error: 'promisewindowrestored' is not defined. [eslint: no-undef]
::: browser/components/sessionstore/test/browser_speculative_connect.js:82
(Diff revision 3)
> await BrowserTestUtils.closeWindow(win);
>
> // Reopen a window.
> let newWin = undoCloseWindow(0);
> // Make sure we wait until this window is restored.
> - await BrowserTestUtils.waitForEvent(newWin, "load");
> + await promiseWindowRestored(newWin);
Error: 'promisewindowrestored' is not defined. [eslint: no-undef]
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 251•7 years ago
|
||
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/39baa8f0bc04
Part 1: Merge RecentWindow.jsm and UpdateTopLevelContentWindowIDHelper.jsm into one module called 'BrowserWindowTracker.jsm'. r=dao
https://hg.mozilla.org/integration/autoland/rev/b86203bcf975
Part 2: Rename and shorten `getMostRecentBrowserWindow` to `getTopWindow` and modernize the style used in BrowserWindowTracker.jsm. r=dao
https://hg.mozilla.org/integration/autoland/rev/7b590a257f65
Part 3: start tracking windows activations to always be aware of their respective order. This allows consumers to iterate over a set of windows in order of appearance (e.g. z-order). r=dao
https://hg.mozilla.org/integration/autoland/rev/3eb2e99b13c7
Part 4: move away from keeping state on the living objects, like windows, tabs and browsers, but keep it truly privately stored in WeakMaps. r=dao
https://hg.mozilla.org/integration/autoland/rev/6ce9efe2b20d
Part 5: restore windows in reverse z-order, so that the last focused window is restored first and stays in front. r=dao
https://hg.mozilla.org/integration/autoland/rev/dec31c64321f
Part 6: Tests that use ss.setBrowserState() or ss.setWindowState() should wait until the window is restored to continue. r=dao
https://hg.mozilla.org/integration/autoland/rev/74478b16e546
Part 7: Test that we stored window z-indices correctly and in order of creation. Ensure that we restore windows in reverse z-index order. r=dao
Comment 252•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/39baa8f0bc04
https://hg.mozilla.org/mozilla-central/rev/b86203bcf975
https://hg.mozilla.org/mozilla-central/rev/7b590a257f65
https://hg.mozilla.org/mozilla-central/rev/3eb2e99b13c7
https://hg.mozilla.org/mozilla-central/rev/6ce9efe2b20d
https://hg.mozilla.org/mozilla-central/rev/dec31c64321f
https://hg.mozilla.org/mozilla-central/rev/74478b16e546
Status: ASSIGNED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Updated•7 years ago
|
Comment 255•6 years ago
|
||
What about the 52.x.x version? (The last one for Win XP).
Updated•5 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•