Fix incorrect window size on session restore.

Fix incorrect window size on session restore.

Under the right conditions, a window that is restored during a browser
restart and never moved during that session will have an empty bounds
in session restore during the next browser restart. As a result that
window will be assigned the default browser window bounds, which could
be much different (in the steps in the referenced bug the restored
window jumps from its original size to the full size of the screen).

Normally whenever a window's size is changed, even during window
restoration, its size is sent to session restore, but at start-up the
window may not be fully initialized with the session service and so
session restore ignores the update. The solution is to ensure that
the window always updates its size with session restore when it
becomes main (it was almost always doing that previously; the case
where it wasn't is what led to the bug).

BUG=536280
Committed: https://crrev.com/61d7597133612988ae9980482da6199c7ad23e8a
Cr-Commit-Position: refs/heads/master@{#355803}

[shrike, 2015-09-26 00:28:16]

shrike@chromium.org changed reviewers:
+ rohitaro@chromium.org

[shrike, 2015-10-05 16:58:39]

This is a similar one-line fix to another issue - PTAL.

[Jason Robbins -- corp, 2015-10-09 18:56:20]

jrobbins@google.com changed reviewers:
+ rohitrao@chromium.org
- rohitaro@chromium.org

[shrike, 2015-10-20 21:58:46]

rohitrao@ - PTAL at this one line bug fix so that I can land it.

[shrike, 2015-10-23 03:01:48]

shrike@chromium.org changed reviewers:
+ avi@chromium.org
- rohitrao@chromium.org

[shrike, 2015-10-23 03:02:00]

Hi avi@ - PTAL

[Avi (use Gerrit), 2015-10-23 04:42:46]

I don't mind this fix; lgtm. I was reading the bug, and you say that the restore
service ignores the position saving because the window type isn't set that
early. Is that possibly the bug, or something to address?

[shrike, 2015-10-23 06:00:30]

On 2015/10/23 04:42:46, Avi wrote:
> I don't mind this fix; lgtm. I was reading the bug, and you say that the
restore
> service ignores the position saving because the window type isn't set that
> early. Is that possibly the bug, or something to address?

That's a reasonable question. However, the issue is a little bit
chicken-and-egg. session_restore.cc's ProcessSessionWindows() does the work of
reconstituting windows from the previous session. Inside that method, I believe
CreateRestoredBrowser() creates each individual browser window, and takes a Rect
that specifies the window frame. Somewhere within CreateRestoredBrowser() the
frame gets set, which triggers a call to saveWindowPositionIfNeeded(), which
ultimately calls SessionService::SetWindowBounds().

So far so good, except SessionService maintains a set that contains window ids.
SessionService::SetWindowBounds() calls
SessionService::ShouldTrackChangesToWindow(), which checks the set for the
window id and returns false if the window id is not present. window ids get
added to the set as a result of a call to SessionService::SetWindowType(), which
is called from within SessionService::WindowOpened().
SessionService::WindowOpened() gets called after the browser window has been
fully created by ProcessSessionWindows().

SessionService::WindowOpened() first checks to make sure that the browser window
should be tracked at all (i.e. added to the set), with a call to
SessionService::ShouldTrackBrowser(). So if we were going to attempt a change we
would want to call SessionService::ShouldTrackBrowser() and then
SessionService::SetWindowType() from somewhere within CreateRestoredBrowser(),
where the window bounds gets set. SessionService::ShouldTrackBrowser() feels
like it's expecting the browser window to have been fully restored, and I don't
think that occurs at least until after ProcessSessionWindows() has returned
(there appears to be more initialization performed after the call to
CreateRestoredBrowser()).  I would be very nervous about trying to call
ShouldTrackBrowser() from within CreateRestoredBrowser().

[Avi (use Gerrit), 2015-10-23 13:39:00]

On 2015/10/23 06:00:30, shrike wrote:
> On 2015/10/23 04:42:46, Avi wrote:
> > I don't mind this fix; lgtm. I was reading the bug, and you say that the
> restore
> > service ignores the position saving because the window type isn't set that
> > early. Is that possibly the bug, or something to address?
> 
> That's a reasonable question. However, the issue is a little bit
> chicken-and-egg. session_restore.cc's ProcessSessionWindows() does the work of
> reconstituting windows from the previous session. Inside that method, I
believe
> CreateRestoredBrowser() creates each individual browser window, and takes a
Rect
> that specifies the window frame. Somewhere within CreateRestoredBrowser() the
> frame gets set, which triggers a call to saveWindowPositionIfNeeded(), which
> ultimately calls SessionService::SetWindowBounds().
> 
> So far so good, except SessionService maintains a set that contains window
ids.
> SessionService::SetWindowBounds() calls
> SessionService::ShouldTrackChangesToWindow(), which checks the set for the
> window id and returns false if the window id is not present. window ids get
> added to the set as a result of a call to SessionService::SetWindowType(),
which
> is called from within SessionService::WindowOpened().
> SessionService::WindowOpened() gets called after the browser window has been
> fully created by ProcessSessionWindows().
> 
> SessionService::WindowOpened() first checks to make sure that the browser
window
> should be tracked at all (i.e. added to the set), with a call to
> SessionService::ShouldTrackBrowser(). So if we were going to attempt a change
we
> would want to call SessionService::ShouldTrackBrowser() and then
> SessionService::SetWindowType() from somewhere within CreateRestoredBrowser(),
> where the window bounds gets set. SessionService::ShouldTrackBrowser() feels
> like it's expecting the browser window to have been fully restored, and I
don't
> think that occurs at least until after ProcessSessionWindows() has returned
> (there appears to be more initialization performed after the call to
> CreateRestoredBrowser()).  I would be very nervous about trying to call
> ShouldTrackBrowser() from within CreateRestoredBrowser().

Makes sense, then.

Thanks for looking at this so closely.

[shrike, 2015-10-23 16:08:30]

The CQ bit was checked by shrike@chromium.org

[commit-bot: I haz the power, 2015-10-23 16:09:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1370753003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1370753003/1

[commit-bot: I haz the power, 2015-10-23 16:43:32]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-10-23 16:44:26]

Patchset 1 (id:??) landed as
https://crrev.com/61d7597133612988ae9980482da6199c7ad23e8a
Cr-Commit-Position: refs/heads/master@{#355803}