Restoring a session should restore window minimization state on Windows

Restoring a session should restore window minimization state on Windows

These changes enable save and restore of minimized window state on Windows.  (Mac and Linux came prior).  The window state is now saved whenever size-changed notifications fire (in addition to the original activation and close cases).

BUG=43274
TEST=Manual.  Launch Chrome on Windows, create two browser windows, minimize "a" window, quit.  Relaunch, expect "a" to be minimized.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98679

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=99077

[dhollowa, 2011-08-26 02:02:01]

sky: I searched back in time and it appears that the issues around "close" not
recording window position during "quit" goes back a ways.  The problem is
certainly present in the stable (m13) channel.

I looked at messing with the shutdown logic, but it seems a more reliable
solution to save position information when the window moves.  This gets reliably
triggered during maximize, minimize, and normal-ize operations.

The |Widget::OnNativeWidgetSizeChanged| signal is a bit chatty, but not overly. 
It seems the best solution IMO.

Thanks.

[sky, 2011-08-26 16:26:20]

I tend to think that on windows we always want to restore at least one window
non-minimized. For example, consider the following: don't have session restore
enabled, launch chrome, minimize chrome, right click on the taskbar icon choose
close, then double click on the desktop icon for chrome. Would you expect it to
appear minimized? I tend to think not. What happens with your code?

I suspect your change might result in events getting fired at a different time.
A recent change I did that tweaked restoring maximized caused crashes. You
should add a browser test that restores minimized. See
BrowserTest.StartMaximized for the test I added. Hopefully you could tweak that.

http://codereview.chromium.org/7748036/diff/1/chrome/browser/sessions/session...
File chrome/browser/sessions/session_service.cc (right):

http://codereview.chromium.org/7748036/diff/1/chrome/browser/sessions/session...
chrome/browser/sessions/session_service.cc:976: payload.show_state !=
ui::SHOW_STATE_INACTIVE) {
Can you add a comment here saying INACTIVE isn't persisted. I know you have it
in the enum too, but it's easily missed.

http://codereview.chromium.org/7748036/diff/1/views/views_delegate.h
File views/views_delegate.h (right):

http://codereview.chromium.org/7748036/diff/1/views/views_delegate.h#newcode64
views/views_delegate.h:64: virtual bool GetSavedWindowShowState(
Since you're here, is it possible to combine GetSavedWindowBounds and
GetSavedWindowShowState into one method that takes the gfx::Rect* and
windowshowstate* ? I don't know why we ended up with two.

http://codereview.chromium.org/7748036/diff/1/views/widget/native_widget_win.cc
File views/widget/native_widget_win.cc (right):

http://codereview.chromium.org/7748036/diff/1/views/widget/native_widget_win....
views/widget/native_widget_win.cc:661: *show_state = ui::SHOW_STATE_NORMAL;
nit: put this as the last 'else' branch of the if.

http://codereview.chromium.org/7748036/diff/1/views/widget/native_widget_win....
views/widget/native_widget_win.cc:877: void
NativeWidgetWin::ShowMaximizedWithBounds(
Are you sure we don't need something similar for minimized?

http://codereview.chromium.org/7748036/diff/1/views/widget/widget.cc
File views/widget/widget.cc (right):

http://codereview.chromium.org/7748036/diff/1/views/widget/widget.cc#newcode896
views/widget/widget.cc:896: if (HasFocusManager())
This seems a bit fragile. Could you instead add an explicit boolean as to
whether we've gotten far enough along in initialization.

[dhollowa, 2011-08-26 22:13:50]

> I tend to think that on windows we always want to restore at least one window
non-minimized.

Done.

Yes, I'd planned to do this in a subsequent patch.  But now is fine too.  I'll
need to circle back on this for a special case on Mac, but it seems like the
right behavior for all platforms.


> You should add a browser test that restores minimized. See
BrowserTest.StartMaximized for the test I added. Hopefully you could tweak that.

Done.

Yes, thanks for the tip.

http://codereview.chromium.org/7748036/diff/1/chrome/browser/sessions/session...
File chrome/browser/sessions/session_service.cc (right):

http://codereview.chromium.org/7748036/diff/1/chrome/browser/sessions/session...
chrome/browser/sessions/session_service.cc:976: payload.show_state !=
ui::SHOW_STATE_INACTIVE) {
On 2011/08/26 16:26:20, sky wrote:
> Can you add a comment here saying INACTIVE isn't persisted. I know you have it
> in the enum too, but it's easily missed.

Done.

http://codereview.chromium.org/7748036/diff/1/views/views_delegate.h
File views/views_delegate.h (right):

http://codereview.chromium.org/7748036/diff/1/views/views_delegate.h#newcode64
views/views_delegate.h:64: virtual bool GetSavedWindowShowState(
On 2011/08/26 16:26:20, sky wrote:
> Since you're here, is it possible to combine GetSavedWindowBounds and
> GetSavedWindowShowState into one method that takes the gfx::Rect* and
> windowshowstate* ? I don't know why we ended up with two.

Done.

I've gone with |GetSavedWindowPlacement| throughout.  This applies to the
ViewsDelegate hierarchy as well as the WidgetDelegate hierarchy.

I've also propagated the "Placement" terminology into
|NativeWidget::GetWindowPlacement| (it was
|NativeWidget::GetWindowBoundsAndShowState|).

There are further opportunities in |WindowSizer| and |Browser| but I stopped
short because the use-patterns with those are a touch complex.  A follow-up
patch possibly.

http://codereview.chromium.org/7748036/diff/1/views/widget/native_widget_win.cc
File views/widget/native_widget_win.cc (right):

http://codereview.chromium.org/7748036/diff/1/views/widget/native_widget_win....
views/widget/native_widget_win.cc:661: *show_state = ui::SHOW_STATE_NORMAL;
On 2011/08/26 16:26:20, sky wrote:
> nit: put this as the last 'else' branch of the if.

Done.

http://codereview.chromium.org/7748036/diff/1/views/widget/native_widget_win....
views/widget/native_widget_win.cc:877: void
NativeWidgetWin::ShowMaximizedWithBounds(
Yes, my understanding is that with maximize the |SetBounds| call separate from
|Show| causes a visual jump from normal to maximized.  But with minimize the
discontinuity does not show up because the window is hidden.  This seems to be
the case when I test it too.

On 2011/08/26 16:26:20, sky wrote:
> Are you sure we don't need something similar for minimized?

http://codereview.chromium.org/7748036/diff/1/views/widget/widget.cc
File views/widget/widget.cc (right):

http://codereview.chromium.org/7748036/diff/1/views/widget/widget.cc#newcode896
views/widget/widget.cc:896: if (HasFocusManager())
On 2011/08/26 16:26:20, sky wrote:
> This seems a bit fragile. Could you instead add an explicit boolean as to
> whether we've gotten far enough along in initialization.

Done.

[sky, 2011-08-26 22:29:55]

http://codereview.chromium.org/7748036/diff/6001/chrome/browser/chromeos/fram...
File chrome/browser/chromeos/frame/panel_browser_view.cc (right):

http://codereview.chromium.org/7748036/diff/6001/chrome/browser/chromeos/fram...
chrome/browser/chromeos/frame/panel_browser_view.cc:120: show_state =
ui::SHOW_STATE_NORMAL;
*show_state = ...

http://codereview.chromium.org/7748036/diff/6001/chrome/browser/sessions/sess...
File chrome/browser/sessions/session_restore.cc (right):

http://codereview.chromium.org/7748036/diff/6001/chrome/browser/sessions/sess...
chrome/browser/sessions/session_restore.cc:619: // Ensure that at least one
restored window is visible.
The first window to be restored is the window the user launched first. So, I
think we should restore that to normal. This will require a loop before we enter
this for loop to determine that.

[dhollowa, 2011-08-26 23:18:23]

http://codereview.chromium.org/7748036/diff/6001/chrome/browser/chromeos/fram...
File chrome/browser/chromeos/frame/panel_browser_view.cc (right):

http://codereview.chromium.org/7748036/diff/6001/chrome/browser/chromeos/fram...
chrome/browser/chromeos/frame/panel_browser_view.cc:120: show_state =
ui::SHOW_STATE_NORMAL;
On 2011/08/26 22:29:56, sky wrote:
> *show_state = ...

Done.

http://codereview.chromium.org/7748036/diff/6001/chrome/browser/sessions/sess...
File chrome/browser/sessions/session_restore.cc (right):

http://codereview.chromium.org/7748036/diff/6001/chrome/browser/sessions/sess...
chrome/browser/sessions/session_restore.cc:619: // Ensure that at least one
restored window is visible.
On 2011/08/26 22:29:56, sky wrote:
> The first window to be restored is the window the user launched first. So, I
> think we should restore that to normal. This will require a loop before we
enter
> this for loop to determine that.

Done.  As discussed, we may need to evaluate this Mac-side, as the notion there
is most-recently-minimized gets restored first.

[sky, 2011-08-26 23:40:05]

LGTM

[commit-bot: I haz the power, 2011-08-27 01:32:11]

Can't apply patch for file chrome/browser/ui/views/chrome_views_delegate.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/ui/views/chrome_views_delegate.cc
Hunk #1 FAILED at 65.
Hunk #2 succeeded at 73 (offset -7 lines).
Hunk #3 succeeded at 86 (offset -7 lines).
Hunk #4 succeeded at 105 (offset -7 lines).
1 out of 4 hunks FAILED -- saving rejects to file
chrome/browser/ui/views/chrome_views_delegate.cc.rej

[commit-bot: I haz the power, 2011-08-29 19:49:09]

Change committed as 98679

[dhollowa, 2011-08-31 21:03:14]

sky: PTAL.  Only changes are to widget.h/cc regarding "init" versus "create"
that tweaked XP.  Thanks.

[sky, 2011-08-31 21:09:40]

LGTM