Save and restore State for ShellWindows, including panels

Save and restore State for ShellWindows, including panels

This replaces ShellWindow::CreateParams::State with
ui::WindowShowState for simplicty and consistency with
Browser session restore.

BUG=233556
R=flackr@chromium.org, skuhne@chromium.org, sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=198918

[stevenjb, 2013-04-25 23:05:17]

https://codereview.chromium.org/14031021/diff/1/chrome/browser/extensions/she...
File chrome/browser/extensions/shell_window_geometry_cache.h (right):

https://codereview.chromium.org/14031021/diff/1/chrome/browser/extensions/she...
chrome/browser/extensions/shell_window_geometry_cache.h:17: #include
"chrome/browser/ui/extensions/shell_window.h"
unnecessary

[flackr, 2013-04-26 03:44:38]

https://codereview.chromium.org/14031021/diff/1/chrome/browser/extensions/she...
File chrome/browser/extensions/shell_window_geometry_cache_unittest.cc (right):

https://codereview.chromium.org/14031021/diff/1/chrome/browser/extensions/she...
chrome/browser/extensions/shell_window_geometry_cache_unittest.cc:121: // Test
that loading geometry from the store works correctly.
nit: s/geometry/geometry and state

https://codereview.chromium.org/14031021/diff/1/chrome/browser/ui/extensions/...
File chrome/browser/ui/extensions/shell_window.h (right):

https://codereview.chromium.org/14031021/diff/1/chrome/browser/ui/extensions/...
chrome/browser/ui/extensions/shell_window.h:18: #include
"ui/base/ui_base_types.h"  // WindowShowState
However helpful, commenting what you use from the include seems highly
irregular.

https://codereview.chromium.org/14031021/diff/1/chrome/browser/ui/views/exten...
File chrome/browser/ui/views/extensions/native_app_window_views.cc (left):

https://codereview.chromium.org/14031021/diff/1/chrome/browser/ui/views/exten...
chrome/browser/ui/views/extensions/native_app_window_views.cc:277:
window_->SetBounds(window_bounds);
You seem to be removing restoring bounds for non ash.

https://codereview.chromium.org/14031021/diff/1/ui/base/ui_base_types.h
File ui/base/ui_base_types.h (right):

https://codereview.chromium.org/14031021/diff/1/ui/base/ui_base_types.h#newco...
ui/base/ui_base_types.h:20: SHOW_STATE_DETACHED   = 6,  // Views only; detached
panel.
I'm not sure about this. When panels are detached they behave like "normal"
windows so SHOW_STATE_NORMAL seems to make sense. How would this work with
panels attached to the side of a workspace later? I wonder if it wouldn't be
simpler to have SHOW_STATE_ATTACHED which seems less ambiguous.

[stevenjb, 2013-04-26 16:12:17]

+sky@ for ui/base/ui_base_types.h OWNER and input.

https://codereview.chromium.org/14031021/diff/1/chrome/browser/extensions/she...
File chrome/browser/extensions/shell_window_geometry_cache_unittest.cc (right):

https://codereview.chromium.org/14031021/diff/1/chrome/browser/extensions/she...
chrome/browser/extensions/shell_window_geometry_cache_unittest.cc:121: // Test
that loading geometry from the store works correctly.
On 2013/04/26 03:44:38, flackr wrote:
> nit: s/geometry/geometry and state

Done.

https://codereview.chromium.org/14031021/diff/1/chrome/browser/ui/extensions/...
File chrome/browser/ui/extensions/shell_window.h (right):

https://codereview.chromium.org/14031021/diff/1/chrome/browser/ui/extensions/...
chrome/browser/ui/extensions/shell_window.h:18: #include
"ui/base/ui_base_types.h"  // WindowShowState
On 2013/04/26 03:44:38, flackr wrote:
> However helpful, commenting what you use from the include seems highly
> irregular.
We don't do this a huge amount, but there is some precedent in chrome/ and ash/,
generally when the include file name does not match the class or enum it is
needed for.

https://codereview.chromium.org/14031021/diff/1/chrome/browser/ui/views/exten...
File chrome/browser/ui/views/extensions/native_app_window_views.cc (left):

https://codereview.chromium.org/14031021/diff/1/chrome/browser/ui/views/exten...
chrome/browser/ui/views/extensions/native_app_window_views.cc:277:
window_->SetBounds(window_bounds);
On 2013/04/26 03:44:38, flackr wrote:
> You seem to be removing restoring bounds for non ash.
As I commented, this is not implemented for non-ash, so the current code in the
ifdef may or may not be useful (I suspect it is not useful, and thus is
misleading, which is why I removed it).

https://codereview.chromium.org/14031021/diff/1/ui/base/ui_base_types.h
File ui/base/ui_base_types.h (right):

https://codereview.chromium.org/14031021/diff/1/ui/base/ui_base_types.h#newco...
ui/base/ui_base_types.h:20: SHOW_STATE_DETACHED   = 6,  // Views only; detached
panel.
On 2013/04/26 03:44:38, flackr wrote:
> I'm not sure about this. When panels are detached they behave like "normal"
> windows so SHOW_STATE_NORMAL seems to make sense. How would this work with
> panels attached to the side of a workspace later? I wonder if it wouldn't be
> simpler to have SHOW_STATE_ATTACHED which seems less ambiguous.

I considered adding ATTACHED, but it actually makes the code more complex since
attached is the "normal" state for panels.

Also, I don't expect we will need to store "guttered" explicitly; if we restore
one ore more windows to an edge we should be able to automatically create a
gutter at that point.

All that said, if the consensus is that having an explicit ATTACHED state would
be more clear and worth the additional code, I can certainly make that change.

[sky, 2013-04-26 16:19:02]

LGTM

[flackr, 2013-04-26 16:47:25]

LGTM

On 2013/04/26 16:12:17, stevenjb (chromium) wrote:
> +sky@ for ui/base/ui_base_types.h OWNER and input.
> 
>
https://codereview.chromium.org/14031021/diff/1/chrome/browser/extensions/she...
> File chrome/browser/extensions/shell_window_geometry_cache_unittest.cc
(right):
> 
>
https://codereview.chromium.org/14031021/diff/1/chrome/browser/extensions/she...
> chrome/browser/extensions/shell_window_geometry_cache_unittest.cc:121: // Test
> that loading geometry from the store works correctly.
> On 2013/04/26 03:44:38, flackr wrote:
> > nit: s/geometry/geometry and state
> 
> Done.
> 
>
https://codereview.chromium.org/14031021/diff/1/chrome/browser/ui/extensions/...
> File chrome/browser/ui/extensions/shell_window.h (right):
> 
>
https://codereview.chromium.org/14031021/diff/1/chrome/browser/ui/extensions/...
> chrome/browser/ui/extensions/shell_window.h:18: #include
> "ui/base/ui_base_types.h"  // WindowShowState
> On 2013/04/26 03:44:38, flackr wrote:
> > However helpful, commenting what you use from the include seems highly
> > irregular.
> We don't do this a huge amount, but there is some precedent in chrome/ and
ash/,
> generally when the include file name does not match the class or enum it is
> needed for.
> 
>
https://codereview.chromium.org/14031021/diff/1/chrome/browser/ui/views/exten...
> File chrome/browser/ui/views/extensions/native_app_window_views.cc (left):
> 
>
https://codereview.chromium.org/14031021/diff/1/chrome/browser/ui/views/exten...
> chrome/browser/ui/views/extensions/native_app_window_views.cc:277:
> window_->SetBounds(window_bounds);
> On 2013/04/26 03:44:38, flackr wrote:
> > You seem to be removing restoring bounds for non ash.
> As I commented, this is not implemented for non-ash, so the current code in
the
> ifdef may or may not be useful (I suspect it is not useful, and thus is
> misleading, which is why I removed it).
> 

Ah sorry, I missed that connection.

> https://codereview.chromium.org/14031021/diff/1/ui/base/ui_base_types.h
> File ui/base/ui_base_types.h (right):
> 
>
https://codereview.chromium.org/14031021/diff/1/ui/base/ui_base_types.h#newco...
> ui/base/ui_base_types.h:20: SHOW_STATE_DETACHED   = 6,  // Views only;
detached
> panel.
> On 2013/04/26 03:44:38, flackr wrote:
> > I'm not sure about this. When panels are detached they behave like "normal"
> > windows so SHOW_STATE_NORMAL seems to make sense. How would this work with
> > panels attached to the side of a workspace later? I wonder if it wouldn't be
> > simpler to have SHOW_STATE_ATTACHED which seems less ambiguous.
> 
> I considered adding ATTACHED, but it actually makes the code more complex
since
> attached is the "normal" state for panels.
> 
> Also, I don't expect we will need to store "guttered" explicitly; if we
restore
> one ore more windows to an edge we should be able to automatically create a
> gutter at that point.
> 
> All that said, if the consensus is that having an explicit ATTACHED state
would
> be more clear and worth the additional code, I can certainly make that change.

My only reservation with this is that the bounds could change. If we trusted the
bounds to remain constant for the gutter we could also trust them to remain
constant and reattach the panel if it's restored next to the launcher. In any
event, if the code is simpler this way I don't imagine it would make things too
awkward later if we add a state such as SHOW_STATE_DOCKED for the gutter.

[stevenjb, 2013-04-30 15:28:12]

jeremya@ or benwells@, please take a look when you have a chance.

[benwells, 2013-05-01 08:30:59]

On 2013/04/30 15:28:12, stevenjb (chromium) wrote:
> jeremya@ or benwells@, please take a look when you have a chance.

Does this mean if an app window is closed while minimised, it will start
minimised when it is reshown?

[stevenjb, 2013-05-01 19:02:36]

On 2013/05/01 08:30:59, benwells wrote:
> On 2013/04/30 15:28:12, stevenjb (chromium) wrote:
> > jeremya@ or benwells@, please take a look when you have a chance.
> 
> Does this mean if an app window is closed while minimised, it will start
> minimised when it is reshown?

So, it turns out that the code was intending to restore minimized, but actually
failed to do so.

I fixed the code and added tests (which required some fixes to
app_window_custom_bindings.js and app_window_api.cc, PTAL).

My thinking on restoring minimized windows as minimized is both useful and
confusing. My personal preference is to restore minimized, I think this would be
useful, especially for things like Talk panels, but I could be persuaded either
way. If we disable restore minimized, I will explicitly disable it in
ShellWindow and comment the code.

[benwells, 2013-05-01 23:16:32]

On 2013/05/01 19:02:36, stevenjb (chromium) wrote:
> On 2013/05/01 08:30:59, benwells wrote:
> > On 2013/04/30 15:28:12, stevenjb (chromium) wrote:
> > > jeremya@ or benwells@, please take a look when you have a chance.
> > 
> > Does this mean if an app window is closed while minimised, it will start
> > minimised when it is reshown?
> 
> So, it turns out that the code was intending to restore minimized, but
actually
> failed to do so.
> 
> I fixed the code and added tests (which required some fixes to
> app_window_custom_bindings.js and app_window_api.cc, PTAL).
> 
> My thinking on restoring minimized windows as minimized is both useful and
> confusing. My personal preference is to restore minimized, I think this would
be
> useful, especially for things like Talk panels, but I could be persuaded
either
> way. If we disable restore minimized, I will explicitly disable it in
> ShellWindow and comment the code.

I'm not sure what the correct behavior for panels is; maybe it is different to
apps. For an app, if the app is launched it should not start minimized. I think
this would be confusing; e.g. imagine starting chrome and it is minimized, that
would seem weird.

This will be tricky to get right. This is what I would expect:
1. Have a window at 50, 50 sized 100, 100.
2. Maximize it.
3. Minimize it.
4. Close it.
5. Restart the app.

I would now expect it to be maximized.

6. Unmaximize it.

I would now expect it to be at 50, 50, size 100, 100.

Does that make sense?

[stevenjb, 2013-05-03 00:17:15]

On 2013/05/01 23:16:32, benwells wrote:

> I'm not sure what the correct behavior for panels is; maybe it is different to
> apps. For an app, if the app is launched it should not start minimized. I
think
> this would be confusing; e.g. imagine starting chrome and it is minimized,
that
> would seem weird.
> 
> This will be tricky to get right. This is what I would expect:
> 1. Have a window at 50, 50 sized 100, 100.
> 2. Maximize it.
> 3. Minimize it.
> 4. Close it.
> 5. Restart the app.
> 
> I would now expect it to be maximized.
> 
> 6. Unmaximize it.
> 
> I would now expect it to be at 50, 50, size 100, 100.
> 
> Does that make sense?

Yes, that does make sense, and it mostly worked, except it turns out on Ash I
had to add a method to BaseWindow in order to correctly get the restored state
for "minimized" windows (which are actually hidden in Ash, not minimized).

I went ahead and made the behavior for Apps not to restore minimized state. An
app can always override that behavior, and of course we can change it later if
we decide we want to.

PTAL

[benwells, 2013-05-03 08:52:03]

Can you explain how in the given situation the window will be restored
maximized, as opposed to in a normal state? From looking at the restore code it
looks like it will revert to normal if the remembered state was minimised.

+miket
Mike, I am happy to keep reviewing this but maybe someone from your side should
take over, given you guys are taking over the window api stuff.

https://codereview.chromium.org/14031021/diff/21001/chrome/browser/extensions...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/14031021/diff/21001/chrome/browser/extensions...
chrome/browser/extensions/api/app_window/app_window_api.cc:158:
SetCreateResultFromShellWindow(window, result);
Why is this now being done here?

(Wow this is a long function. Nice to see something get factored out of it :))

https://codereview.chromium.org/14031021/diff/21001/chrome/browser/extensions...
chrome/browser/extensions/api/app_window/app_window_api.cc:285:
create_params.state = ui::SHOW_STATE_MINIMIZED;
Why is this changing? Seems counter intuitive. Maybe a comment would help.

[stevenjb, 2013-05-03 17:41:14]

re: Restoring a maximized + minimized window maximized:

That's going to depend on how IsMaximized() works on a platform. I copied the
logic from SessionService::BuildCommandsForBrowser() to GetRestoredState() for
platforms other than Ash, under the assumption that it was correct, but it may
not be. It was incorrect on Ash, so I fixed it (for ShellWindow anyway; I'll ask
the designers and follow up on what we want to do for browser windows).

I do not have an osx or windows environment handy, so was only able to test on
chromeos and linux, which both work correctly (gtk can have both minimized and
maximized bits set, Ash required special handling in
NativeAppWindowViews::GetRestoredState()).

I figured this CL would at least be a reasonable starting point.

https://codereview.chromium.org/14031021/diff/21001/chrome/browser/extensions...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/14031021/diff/21001/chrome/browser/extensions...
chrome/browser/extensions/api/app_window/app_window_api.cc:158:
SetCreateResultFromShellWindow(window, result);
On 2013/05/03 08:52:03, benwells wrote:
> Why is this now being done here?
> 
> (Wow this is a long function. Nice to see something get factored out of it :))

This code path occurs if we call app.window.create() with an existing singleton
id. While it may not be unreasonable to only populate "viewId",
"existingWindow", and "injectTitlebar", it seems more reasonable to me to also
set the bounds and state properties of the result, the same as if a new window
had been created.

If there is a reason why this wouldn't be desired, I can add a comment and
remove this.

https://codereview.chromium.org/14031021/diff/21001/chrome/browser/extensions...
chrome/browser/extensions/api/app_window/app_window_api.cc:285:
create_params.state = ui::SHOW_STATE_MINIMIZED;
On 2013/05/03 08:52:03, benwells wrote:
> Why is this changing? Seems counter intuitive. Maybe a comment would help.
typo. Thanks for catching.

[stevenjb, 2013-05-06 18:22:48]

skuhne@ - can you take a look at this CL? We're going to need more reviewers on
ShellWindow / NativeAppWindow, and you're at least peripherally familiar with
this code.

[Mr4D (OOO till 08-26), 2013-05-06 21:40:15]

Could you check with sky / mukai about the introduction of the new window flag?
I would guess that we should instead have a more general "unmanaged" property
which could then also be used by other windows. That would make the
centralization of window management logic much easier.

Beside that only a few nits.

https://codereview.chromium.org/14031021/diff/37001/chrome/browser/extensions...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/14031021/diff/37001/chrome/browser/extensions...
chrome/browser/extensions/api/app_window/app_window_api.cc:88:
result->SetBoolean("fullscreen", window->GetBaseWindow()->IsFullscreen());
I see here are lots of constants for the result (as well as in "old code" like
line 160). Should we "collect them" in a constants section?

https://codereview.chromium.org/14031021/diff/37001/chrome/browser/extensions...
File chrome/browser/extensions/shell_window_geometry_cache_unittest.cc (right):

https://codereview.chromium.org/14031021/diff/37001/chrome/browser/extensions...
chrome/browser/extensions/shell_window_geometry_cache_unittest.cc:97:
ASSERT_FALSE(cache_->GetGeometry(extension_id, kWindowId, &bounds, NULL));
Not that this really changes anything, but why do you pass NULL here (and in the
other 4 locations below)? If you wanted to test that the value does not get
written (or the function does not crash) you could have done the same to the
bounds parameter.

https://codereview.chromium.org/14031021/diff/37001/chrome/browser/ui/views/e...
File chrome/browser/ui/views/extensions/native_app_window_views.cc (right):

https://codereview.chromium.org/14031021/diff/37001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/native_app_window_views.cc:277: aura::Window*
win = GetNativeWindow();
Please no abbrev

https://codereview.chromium.org/14031021/diff/37001/ui/base/ui_base_types.h
File ui/base/ui_base_types.h (right):

https://codereview.chromium.org/14031021/diff/37001/ui/base/ui_base_types.h#n...
ui/base/ui_base_types.h:20: SHOW_STATE_DETACHED   = 6,  // Views only; detached
panel.
I don't think that sky would agree on this. We discussed last week the
centralization of the window management. A panel is yet another window type and
should also be able to be managed by this central manager. Since I am not
working on this, you should possibly quickly chat with mukai and or sky before
introducing the flag. I think that a more general "unmanaged" state would be
much better and could also be used for other cases.

[sky, 2013-05-06 22:19:46]

https://codereview.chromium.org/14031021/diff/37001/ui/base/ui_base_types.h
File ui/base/ui_base_types.h (right):

https://codereview.chromium.org/14031021/diff/37001/ui/base/ui_base_types.h#n...
ui/base/ui_base_types.h:20: SHOW_STATE_DETACHED   = 6,  // Views only; detached
panel.
On 2013/05/06 21:40:16, Mr4D wrote:
> I don't think that sky would agree on this. We discussed last week the
> centralization of the window management. A panel is yet another window type
and
> should also be able to be managed by this central manager. Since I am not
> working on this, you should possibly quickly chat with mukai and or sky before
> introducing the flag. I think that a more general "unmanaged" state would be
> much better and could also be used for other cases.

I'm ok with this (reviewed this patch earlier on).

[Mr4D (OOO till 08-26), 2013-05-06 22:27:42]

Fascinating - okay. In that case lgtm. please have a short look at my nits.

[miket_OOO, 2013-05-06 22:51:59]

> +miket
> Mike, I am happy to keep reviewing this but maybe someone from your side
should
> take over, given you guys are taking over the window api stuff.

Ben, that's fine -- I'm adding mek to have a look.

Marijn: would you mind giving this a look? It's getting to be in good shape so
it should be straightforward. Thanks.

[stevenjb, 2013-05-07 16:06:31]

https://codereview.chromium.org/14031021/diff/37001/chrome/browser/extensions...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/14031021/diff/37001/chrome/browser/extensions...
chrome/browser/extensions/api/app_window/app_window_api.cc:88:
result->SetBoolean("fullscreen", window->GetBaseWindow()->IsFullscreen());
On 2013/05/06 21:40:16, Mr4D wrote:
> I see here are lots of constants for the result (as well as in "old code" like
> line 160). Should we "collect them" in a constants section?
The precedent is split in extensions/api between named constants - Set*(k - and
inlined strings - Set*(". I'd prefer not to change the pattern for this file
with this CL.

https://codereview.chromium.org/14031021/diff/37001/chrome/browser/extensions...
File chrome/browser/extensions/shell_window_geometry_cache_unittest.cc (right):

https://codereview.chromium.org/14031021/diff/37001/chrome/browser/extensions...
chrome/browser/extensions/shell_window_geometry_cache_unittest.cc:97:
ASSERT_FALSE(cache_->GetGeometry(extension_id, kWindowId, &bounds, NULL));
On 2013/05/06 21:40:16, Mr4D wrote:
> Not that this really changes anything, but why do you pass NULL here (and in
the
> other 4 locations below)? If you wanted to test that the value does not get
> written (or the function does not crash) you could have done the same to the
> bounds parameter.
Changed bounds to be NULL also.

https://codereview.chromium.org/14031021/diff/37001/chrome/browser/ui/views/e...
File chrome/browser/ui/views/extensions/native_app_window_views.cc (right):

https://codereview.chromium.org/14031021/diff/37001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/native_app_window_views.cc:277: aura::Window*
win = GetNativeWindow();
On 2013/05/06 21:40:16, Mr4D wrote:
> Please no abbrev

Done.

[commit-bot: I haz the power, 2013-05-07 16:06:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/14031021/55001

[commit-bot: I haz the power, 2013-05-07 16:31:44]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[stevenjb, 2013-05-08 16:07:18]

Committed patchset #11 manually as r198918 (presubmit successful).