Last Comment Bug 669272 - Session Restore moves windows around unnecessarily
: Session Restore moves windows around unnecessarily
Status: REOPENED
:
Product: Firefox
Classification: Client Software
Component: Session Restore (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: Firefox 9
Assigned To: Jez Ng [:int3]
:
Mentors:
Depends on: 694378 712763
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-05 02:18 PDT by Jez Ng [:int3]
Modified: 2016-07-02 19:18 PDT (History)
9 users (show)
paul: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (3.09 KB, patch)
2011-07-13 14:42 PDT, Jez Ng [:int3]
no flags Details | Diff | Splinter Review
Patch v1 (5.00 KB, patch)
2011-07-13 14:43 PDT, Jez Ng [:int3]
paul: review+
Details | Diff | Splinter Review
Patch v2 (5.28 KB, patch)
2011-09-04 05:35 PDT, Jez Ng [:int3]
no flags Details | Diff | Splinter Review

Description Jez Ng [:int3] 2011-07-05 02:18:40 PDT
STR:

1. Set Preferences -> When Firefox starts -> Show my home page, with about:home as the home page
2. Open two firefox windows on different parts of the screen. Let us label the the two positions as A and B.
3. Quit firefox with the last focused window being located at position A.
4. Reopen firefox, and click on the 'Restore Previous Session' button that appears.

Actual Results:
When firefox is reopened, the new window is positioned at A. When the 'Restore Previous Session' button is clicked, that window is moved to B, and another window is opened at A. This new window grabs the focus.

Expected Results:
When firefox is reopened, the new window is positioned at A. When the 'Restore Previous Session' button is clicked, another window is opened at B. The window at A retains the focus throughout.

The current behavior produces a noticeable 'flicker' as the window at A is moved away and a new one created in its place. Also, since the user cannot effectively interact with the browser until the focus stops changing, and since creating new windows is a relatively slow step, this makes the restore process feel slower.

This behavior is also observed if one selects the preference 'Show my windows and tabs from last time'.
Comment 1 Vlad [QA] 2011-07-05 23:39:21 PDT
WFM on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0a1) Gecko/20110705 Firefox/7.0a1.

When I've restored the session, the windows retained their original position and focus.

Also, could you see if the issue occurs if using Firefox in safe mode:
http://support.mozilla.com/kb/Safe+Mode

Could you see if the issue occurs if using an empty testing profile? (Don't install any addons into it)
http://support.mozilla.com/kb/Basic+Troubleshooting#w_8-make-a-new-profile
Comment 2 Jez Ng [:int3] 2011-07-13 14:42:18 PDT
Created attachment 545753 [details] [diff] [review]
Patch v1
Comment 3 Jez Ng [:int3] 2011-07-13 14:43:51 PDT
Created attachment 545754 [details] [diff] [review]
Patch v1
Comment 4 Vlad [QA] 2011-08-08 05:17:52 PDT
Considering comment!, setting resolution to Resolved WFM.
Comment 5 Jez Ng [:int3] 2011-08-08 05:41:23 PDT
Vlad: Sorry for not replying with more details -- yes, the issue occurs with a clean profile. The issue might not occur 100% of the time though -- I think there's the possibility that the windows indeed get restored without unnecessary movement; however, the code definitely doesn't guarantee that this happens. My patch should fix that.
Comment 6 Vlad [QA] 2011-08-08 07:49:40 PDT
Ok. Waiting for the patch.
I have tried on the latest nightly and is still WFM.
Comment 7 Jez Ng [:int3] 2011-08-08 11:40:42 PDT
The patch has already been submitted and is awaiting review. See above.
Comment 8 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-09-01 11:16:16 PDT
Comment on attachment 545754 [details] [diff] [review]
Patch v1

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

Just a couple nits.

::: browser/components/sessionstore/src/nsSessionStore.js
@@ +1474,5 @@
>      // the lastWindow.
>      let lastWindow = this._getMostRecentBrowserWindow();
>      let canUseLastWindow = lastWindow &&
>                             !lastWindow.__SS_lastSessionWindowID;
> +    let lastSessionFocusedWindow = null;

My gut wants to say that we don't actually need lastSessionFocusedWindow, but we do.

@@ +1518,5 @@
>          //        in _preWindowToRestoreInto will prevent most (all?) Panorama
>          //        weirdness but we will still merge other extData.
>          //        Bug 588217 should make this go away by merging the group data.
>          this.restoreWindow(windowToUse, { windows: [winState] }, canOverwriteTabs, true);
> +        if (i == 0) lastSessionFocusedWindow = windowToUse;

Nit: 2 line that if

@@ +1528,4 @@
>        }
>        else {
> +        let win = this._openWindowWithState({ windows: [winState] });
> +        if (i == 0) lastSessionFocusedWindow = win;

Nit: 2 line that if

@@ +2485,5 @@
>      var winData;
> +    if (!root.selectedWindow) {
> +      root.selectedWindow = 0;
> +    } else {
> +      root.windows.unshift(root.windows.splice(root.selectedWindow - 1, 1)[0]);

Add a comment here that this makes sure the selected window gets restored first
Comment 9 Jez Ng [:int3] 2011-09-04 05:35:46 PDT
Created attachment 558137 [details] [diff] [review]
Patch v2

Patch with comments addressed.
Comment 10 Ed Morley [:emorley] 2011-09-04 17:37:42 PDT
In my queue :-)
https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=feaf94ae787f
Comment 11 Ed Morley [:emorley] 2011-09-04 19:06:33 PDT
Attempt 2 (one of the other checkin-neededs broke that try push):
https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=189de6fe4c76
Comment 14 Vlad [QA] 2011-09-07 07:12:04 PDT
Hi guys.
I am trying to verify this on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:9.0a1) Gecko/20110907 Firefox/9.0a1

Every thing works as expected except the fact that after step 4 from the description, I don't have the 'Restore Previous Session' button in the page, only in the menu. If have try this on a clean profile.

Is this the intended behavior?

Thanks
Comment 15 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-07 08:23:33 PDT
(In reply to Vlad [QA] from comment #14)
> Hi guys.
> I am trying to verify this on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6;
> rv:9.0a1) Gecko/20110907 Firefox/9.0a1
> 
> Every thing works as expected except the fact that after step 4 from the
> description, I don't have the 'Restore Previous Session' button in the page,
> only in the menu. If have try this on a clean profile.
> 
> Is this the intended behavior?
> 
> Thanks

This bug is more about the behaviour of windows when restored. Use whatever method you can to restore the session. It's not clear to me whether step 4 refers to the Restore Session link in the home page or the Restore Previous Session menuitem in the History menu.
Comment 16 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-09-07 10:27:13 PDT
(In reply to Vlad [QA] from comment #14)
> Is this the intended behavior?

No, but it's not directly related to this bug. It may be bug 682944.

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #15)
> It's not clear to me whether step 4
> refers to the Restore Session link in the home page or the Restore Previous
> Session menuitem in the History menu.

For the record, it doesn't actually matter.
Comment 17 Vlad [QA] 2011-09-07 23:15:04 PDT
Considering comment14, comment15 and comment16, setting resolution to Verified Fixed.
Comment 18 Tim Taubert [:ttaubert] 2013-02-01 10:16:45 PST
Backed out:

https://hg.mozilla.org/integration/fx-team/rev/66295fcfee13
Comment 19 Tim Taubert [:ttaubert] 2013-02-04 11:22:17 PST
Back out is now on m-c:

https://hg.mozilla.org/mozilla-central/rev/66295fcfee13
Comment 20 Wayne Mery (:wsmwk, NI for questions) 2016-07-02 19:18:06 PDT
jez, anyone,
do you still see this?  or Bug 1235231?
personally, I haven't. on Windows anyway

Note You need to log in before you can comment on or make changes to this bug.