Restoring a session should restore window minimization state

Restoring a session should restore window minimization state

Extends session service to support more generalized "show" state encompassing maximized, minimized, and normal window states.  This includes forward-migration logic from old stored session data.  Mac changes to save and restore minimized windows as well as fixes for window position.  Linux/GTK changes to save and restore minimized windows.  Windows/Views changes to follow.

BUG=43274
TEST=Manual. SessionServiceTest.*

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

[dhollowa, 2011-08-19 16:41:12]

This CL gets Mac and Linux session restore working with minimized windows.  I'm
planning to follow with Windows/Views implementation in a subsequent patch since
this one was getting large.

sky -> Session Restore and GTK
mark -> Mac

[prasadt, 2011-08-19 16:55:56]

LGMT for panels change.

[prasadt, 2011-08-19 16:56:30]

On 2011/08/19 16:55:56, prasadt wrote:
> LGMT for panels change.

Looked at it again.  Upgrading to LGTM :-)

[sky, 2011-08-19 16:59:54]

erg or estade should review the linux side.

Also, does this have any effect on syncing session restore data?

  -Scott

http://codereview.chromium.org/7621061/diff/4001/chrome/browser/sessions/sess...
File chrome/browser/sessions/session_service.cc (right):

http://codereview.chromium.org/7621061/diff/4001/chrome/browser/sessions/sess...
chrome/browser/sessions/session_service.cc:961: ui::SHOW_STATE_MAXIMIZED :
ui::SHOW_STATE_NORMAL;
Should this be _DEFAULT?

http://codereview.chromium.org/7621061/diff/4001/chrome/browser/sessions/sess...
chrome/browser/sessions/session_service.cc:974: switch (payload.show_state) {
How about defining a min/max value and making sure payload.show_state is within
that range?

http://codereview.chromium.org/7621061/diff/4001/chrome/browser/sessions/sess...
chrome/browser/sessions/session_service.cc:1181: ui::WindowShowState show_state
= ui::SHOW_STATE_DEFAULT;
nit: make this normal and remove last else clause.

http://codereview.chromium.org/7621061/diff/4001/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/browser_view.cc (right):

http://codereview.chromium.org/7621061/diff/4001/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_view.cc:789: return false;
You should be able to call into frame_->IsMinimized() here.

http://codereview.chromium.org/7621061/diff/4001/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_view.cc:1637:
browser_->SaveWindowPlacement(bounds,
Can you add a TODO to update this as well.

http://codereview.chromium.org/7621061/diff/4001/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_view.cc:1683: *maximized =
browser_->GetSavedWindowShowState() == ui::SHOW_STATE_MAXIMIZED;
And a TODO here too.

http://codereview.chromium.org/7621061/diff/4001/ui/base/ui_base_types.h
File ui/base/ui_base_types.h (right):

http://codereview.chromium.org/7621061/diff/4001/ui/base/ui_base_types.h#newc...
ui/base/ui_base_types.h:14: SHOW_STATE_DEFAULT,
Because these values are written to disk can you assign specific values to them
as well as adding a comment that the value should not be changed.

[dhollowa, 2011-08-19 17:02:52]

+erg

[dhollowa, 2011-08-19 17:58:53]

> Also, does this have any effect on syncing session restore data?

The sync code does not currently saves neither window geometry nor maximized
state AFAICT.

http://codereview.chromium.org/7621061/diff/4001/chrome/browser/sessions/sess...
File chrome/browser/sessions/session_service.cc (right):

http://codereview.chromium.org/7621061/diff/4001/chrome/browser/sessions/sess...
chrome/browser/sessions/session_service.cc:961: ui::SHOW_STATE_MAXIMIZED :
ui::SHOW_STATE_NORMAL;
No, the DEFAULT state is an indeterminate state, used by the clients for
managing restoration logic.  The saved state here is explicit.

On 2011/08/19 16:59:54, sky wrote:
> Should this be _DEFAULT?

http://codereview.chromium.org/7621061/diff/4001/chrome/browser/sessions/sess...
chrome/browser/sessions/session_service.cc:974: switch (payload.show_state) {
On 2011/08/19 16:59:54, sky wrote:
> How about defining a min/max value and making sure payload.show_state is
within
> that range?

Done.

http://codereview.chromium.org/7621061/diff/4001/chrome/browser/sessions/sess...
chrome/browser/sessions/session_service.cc:1181: ui::WindowShowState show_state
= ui::SHOW_STATE_DEFAULT;
On 2011/08/19 16:59:54, sky wrote:
> nit: make this normal and remove last else clause.

Done.

http://codereview.chromium.org/7621061/diff/4001/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/browser_view.cc (right):

http://codereview.chromium.org/7621061/diff/4001/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_view.cc:789: return false;
Yes, I'd like to wait until I have both save/restore working before turning this
on.

On 2011/08/19 16:59:54, sky wrote:
> You should be able to call into frame_->IsMinimized() here.

http://codereview.chromium.org/7621061/diff/4001/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_view.cc:1637:
browser_->SaveWindowPlacement(bounds,
On 2011/08/19 16:59:54, sky wrote:
> Can you add a TODO to update this as well.

Done.

http://codereview.chromium.org/7621061/diff/4001/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_view.cc:1683: *maximized =
browser_->GetSavedWindowShowState() == ui::SHOW_STATE_MAXIMIZED;
On 2011/08/19 16:59:54, sky wrote:
> And a TODO here too.

Done.

http://codereview.chromium.org/7621061/diff/4001/ui/base/ui_base_types.h
File ui/base/ui_base_types.h (right):

http://codereview.chromium.org/7621061/diff/4001/ui/base/ui_base_types.h#newc...
ui/base/ui_base_types.h:14: SHOW_STATE_DEFAULT,
On 2011/08/19 16:59:54, sky wrote:
> Because these values are written to disk can you assign specific values to
them
> as well as adding a comment that the value should not be changed.

Done.

[dhollowa, 2011-08-19 18:00:20]

On 2011/08/19 17:58:53, dhollowa wrote:
> > Also, does this have any effect on syncing session restore data?
> 

Gah!  Hit submit too quickly...:

The sync currently saves neither window geometry nor maximized state AFAICT.

[Mark Mentovai, 2011-08-19 19:17:27]

LGTM. I only looked carefully at files whose names ended in .mm.

http://codereview.chromium.org/7621061/diff/10003/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/7621061/diff/10003/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:50: // Provide the
forward-declarations of new 10.7 SDK symbols so they can be
People keep copying and pasting this comment, but it’s wrong.

1. There isn’t anything forward about these declarations.

2. You’re not really working with symbols, just say “things.” enums don’t have
symbols.

http://codereview.chromium.org/7621061/diff/10003/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:101: // then |miniaturize:| when
restoring windows in the minimized state.
Glad you figured this out.

http://codereview.chromium.org/7621061/diff/10003/chrome/browser/ui/gtk/brows...
File chrome/browser/ui/gtk/browser_window_gtk.h (right):

http://codereview.chromium.org/7621061/diff/10003/chrome/browser/ui/gtk/brows...
chrome/browser/ui/gtk/browser_window_gtk.h:23: #include
"ui/base/ui_base_types.h"
Nit: sort.

http://codereview.chromium.org/7621061/diff/10003/ui/base/ui_base_types.h
File ui/base/ui_base_types.h (right):

http://codereview.chromium.org/7621061/diff/10003/ui/base/ui_base_types.h#new...
ui/base/ui_base_types.h:25: 
Blank line at EOF.

(Ignore the “error” that Rietveld shows for added files. It’s wrong.)

[sky, 2011-08-19 19:27:29]

LGTM

[dhollowa, 2011-08-19 20:38:16]

http://codereview.chromium.org/7621061/diff/10003/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/7621061/diff/10003/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:50: // Provide the
forward-declarations of new 10.7 SDK symbols so they can be
Done.  And I've fixed it in the other copy/paste sites as well.

On 2011/08/19 19:17:27, Mark Mentovai wrote:
> People keep copying and pasting this comment, but it’s wrong.
> 
> 1. There isn’t anything forward about these declarations.
> 
> 2. You’re not really working with symbols, just say “things.” enums don’t have
> symbols.

http://codereview.chromium.org/7621061/diff/10003/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:101: // then |miniaturize:| when
restoring windows in the minimized state.
Me too!

On 2011/08/19 19:17:27, Mark Mentovai wrote:
> Glad you figured this out.

http://codereview.chromium.org/7621061/diff/10003/chrome/browser/ui/gtk/brows...
File chrome/browser/ui/gtk/browser_window_gtk.h (right):

http://codereview.chromium.org/7621061/diff/10003/chrome/browser/ui/gtk/brows...
chrome/browser/ui/gtk/browser_window_gtk.h:23: #include
"ui/base/ui_base_types.h"
On 2011/08/19 19:17:27, Mark Mentovai wrote:
> Nit: sort.

Done.

http://codereview.chromium.org/7621061/diff/10003/ui/base/ui_base_types.h
File ui/base/ui_base_types.h (right):

http://codereview.chromium.org/7621061/diff/10003/ui/base/ui_base_types.h#new...
ui/base/ui_base_types.h:25: 
On 2011/08/19 19:17:27, Mark Mentovai wrote:
> Blank line at EOF.
> 
> (Ignore the “error” that Rietveld shows for added files. It’s wrong.)

Done.

[Elliot Glaysher, 2011-08-19 21:31:02]

The *_gtk.* files LGTM.

[dhollowa, 2011-08-20 00:53:13]

+tim for sync change

[tim (not reviewing), 2011-08-22 15:24:10]

On 2011/08/20 00:53:13, dhollowa wrote:
> +tim for sync change

LGTM