[Docking] Persists docked state on Chrome OS

[Docking] Persists docked state for chrome app and extension windows on Chrome OS. Browser windows should not be affected - the browser will start normal or maximized but never initially docked.

BUG=271582
Committed: https://crrev.com/74014e0e926e2170188c9d7cc4706ccba36448c9
Cr-Commit-Position: refs/heads/master@{#328356}

[varkha, 2015-04-20 16:24:39]

Patchset #2 (id:20001) has been deleted

[varkha, 2015-04-20 16:32:30]

Patchset #2 (id:40001) has been deleted

[varkha, 2015-04-20 16:32:36]

Patchset #1 (id:1) has been deleted

[varkha, 2015-04-20 16:33:03]

The CQ bit was checked by varkha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-04-20 16:33:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1056793006/60001

[varkha, 2015-04-20 17:15:37]

The CQ bit was unchecked by varkha@chromium.org

[varkha, 2015-04-20 17:19:14]

Patchset #1 (id:60001) has been deleted

[varkha, 2015-04-20 17:19:34]

The CQ bit was checked by varkha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-04-20 17:19:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1056793006/70012

[varkha, 2015-04-20 17:54:07]

Patchset #1 (id:70012) has been deleted

[varkha, 2015-04-20 17:54:18]

The CQ bit was checked by varkha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-04-20 17:54:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1056793006/90001

[varkha, 2015-04-20 18:10:45]

varkha@chromium.org changed reviewers:
+ oshima@chromium.org

[varkha, 2015-04-20 18:10:46]

oshima@, can you please take a look? This makes docked state for panels and
chrome app and extension windows (but not browser windows) persistent on Chrome
OS. Let me know what you think of it - I personally use the docking for a few
things like Hangouts and Keep and it is bothersome to restore the session every
time I need to reboot. Thanks!

[varkha, 2015-04-20 21:03:28]

Patchset #2 (id:110001) has been deleted

[oshima, 2015-04-23 17:26:06]

CL looks reasonable, but getting buy-in from apps team and session restore
owners would be more important. If they're fine, then I'm all for it!

I assume the window will still stay docked even if the display metrics has
changed over session restore?

https://codereview.chromium.org/1056793006/diff/130001/chrome/browser/ui/view...
File chrome/browser/ui/views/apps/chrome_native_app_window_views_aura.cc
(right):

https://codereview.chromium.org/1056793006/diff/130001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/chrome_native_app_window_views_aura.cc:167: typedef
AppWindow::BoundsSpecification BoundsSpecification;
using BoundsSpecification = AppWindow::BoundsSpecification;

is better now.

[varkha, 2015-04-24 16:17:32]

varkha@chromium.org changed reviewers:
+ benwells@chromium.org, sadrul@chromium.org

[varkha, 2015-04-24 16:17:33]

Yes, a window will still stay docked even if the display metrics has changed
over session restore (or even in the scenario when a window is closed, display
config is changed and an app window created again). It will pick a side that was
the closest to the absolute coordinates where it was when closed while docked.

+benwells for sanity in chrome/browser/extensions, chrome/common/extensions and
chrome/browser/ui/views/apps.

+sadrul for ui/base/* and ui/views/*.

https://codereview.chromium.org/1056793006/diff/130001/chrome/browser/ui/view...
File chrome/browser/ui/views/apps/chrome_native_app_window_views_aura.cc
(right):

https://codereview.chromium.org/1056793006/diff/130001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/chrome_native_app_window_views_aura.cc:167: typedef
AppWindow::BoundsSpecification BoundsSpecification;
On 2015/04/23 17:26:06, oshima wrote:
> using BoundsSpecification = AppWindow::BoundsSpecification;
> 
> is better now.

Done. Here and elsewhere.

[varkha, 2015-04-24 16:22:45]

The CQ bit was checked by varkha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-04-24 16:23:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1056793006/150001

[commit-bot: I haz the power, 2015-04-24 17:29:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-24 17:29:13]

Dry run: This issue passed the CQ dry run.

[benwells, 2015-04-27 01:25:08]

What exactly are docked windows? Sorry for the dumb question, I just want to
make sure I properly understand.

[varkha, 2015-04-27 01:56:59]

On 2015/04/27 01:25:08, benwells wrote:
> What exactly are docked windows? Sorry for the dumb question, I just want to
> make sure I properly understand.

Try opening a small app like a calculator or keep and drag its window to the
screen edge pressing a cursor against a side edge. The window will dock and any
maximized window (if you had one) will shrink.
You can also dock a window by pressing Alt+] twice. Not any window can be docked
but small windows can if they are allowed to shrink to 360 pix wide or less.

[benwells, 2015-04-27 22:09:22]

On 2015/04/27 01:56:59, varkha wrote:
> On 2015/04/27 01:25:08, benwells wrote:
> > What exactly are docked windows? Sorry for the dumb question, I just want to
> > make sure I properly understand.
> 
> Try opening a small app like a calculator or keep and drag its window to the
> screen edge pressing a cursor against a side edge. The window will dock and
any
> maximized window (if you had one) will shrink.
> You can also dock a window by pressing Alt+] twice. Not any window can be
docked
> but small windows can if they are allowed to shrink to 360 pix wide or less.

Are you planning to restore docked browser windows?

[varkha, 2015-04-27 23:00:00]

> Are you planning to restore docked browser windows?
No. Browser windows are more of generic containers of tabs and assigning enough
identity to them in order to restore sessions is tricky since we keep state in
the tabs / apps for the most part.

[benwells, 2015-04-27 23:12:57]

On 2015/04/27 23:00:00, varkha wrote:
> > Are you planning to restore docked browser windows?
> No. Browser windows are more of generic containers of tabs and assigning
enough
> identity to them in order to restore sessions is tricky since we keep state in
> the tabs / apps for the most part.

Sorry for all the questions, I'm just trying to make sure I understand the
behavior.

If you have a browser on the left taking up most of the screen, and a small app
docked on the right of the screen, are they both docked? Will they both be
restored correctly in this case?

Do we restore the open browser windows and their positions now? E.g. if I have
two browser windows with different tabs / positions, will they be restored the
same way?

[varkha, 2015-04-28 01:36:52]

1. The browser on the left is maximized (not docked) and takes over all of the
workspace which shrinks if there are any docked windows. This state will now be
accurately restored because the maximized state is restored (and was before) as
well as the docked state (new with this CL). Previously the docked window
position would have been restored but not the docked state resulting in it
getting positioned above the maximized window rather than tiled at the edge
(maximized window wouldn't shrink).

2. No, for browser windows we only restore the state (e.g. maximized or normal)
and dimensions plus vertical offset (and center the first normal window
horizontally + auto position any additional windows as they get created by a
user) at least from what I remember.

[sadrul, 2015-04-28 17:11:23]

ui/ lgtm

[benwells, 2015-04-28 23:46:02]

https://codereview.chromium.org/1056793006/diff/150001/chrome/browser/ui/view...
File chrome/browser/ui/views/apps/chrome_native_app_window_views.cc (right):

https://codereview.chromium.org/1056793006/diff/150001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:222:
params.bounds = gfx::Rect(initial_window_bounds.origin(), preferred_size_);
This logic is a bit hard to follow. I can see that the unspecified position is
looked for specifically in OnBeforePanelWidgetInit. Could you instead pass a new
parameter to that function?

https://codereview.chromium.org/1056793006/diff/150001/chrome/browser/ui/view...
File chrome/browser/ui/views/apps/chrome_native_app_window_views_aura.cc
(right):

https://codereview.chromium.org/1056793006/diff/150001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/chrome_native_app_window_views_aura.cc:125: #if
defined(OS_CHROMEOS)
why only chromeos / not ash?

[varkha, 2015-04-29 16:54:16]

Thanks, you were right, I guess I should try using Windows more often than once
a year. PTAL.

https://codereview.chromium.org/1056793006/diff/150001/chrome/browser/ui/view...
File chrome/browser/ui/views/apps/chrome_native_app_window_views.cc (right):

https://codereview.chromium.org/1056793006/diff/150001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:222:
params.bounds = gfx::Rect(initial_window_bounds.origin(), preferred_size_);
On 2015/04/28 23:46:01, benwells wrote:
> This logic is a bit hard to follow. I can see that the unspecified position is
> looked for specifically in OnBeforePanelWidgetInit. Could you instead pass a
new
> parameter to that function?

Done.

https://codereview.chromium.org/1056793006/diff/150001/chrome/browser/ui/view...
File chrome/browser/ui/views/apps/chrome_native_app_window_views_aura.cc
(right):

https://codereview.chromium.org/1056793006/diff/150001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/chrome_native_app_window_views_aura.cc:125: #if
defined(OS_CHROMEOS)
On 2015/04/28 23:46:01, benwells wrote:
> why only chromeos / not ash?

Done. You are right, since I've last worked on this feature ash has matured
enough to support docked windows (which was not the case then). I checked that
it does work on Windows in ash ("Windows 8") mode.

[varkha, 2015-04-29 18:16:41]

The CQ bit was checked by varkha@chromium.org to run a CQ dry run

[varkha, 2015-04-29 18:16:41]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/1056793006/#ps170001
(title: "[Docking] Makes docked state persistent on Chrome OS (comments)")

[commit-bot: I haz the power, 2015-04-29 18:16:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1056793006/170001

[commit-bot: I haz the power, 2015-04-29 19:35:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-29 19:35:55]

Dry run: This issue passed the CQ dry run.

[benwells, 2015-04-29 21:46:42]

https://codereview.chromium.org/1056793006/diff/170001/chrome/browser/ui/view...
File chrome/browser/ui/views/apps/chrome_native_app_window_views.cc (right):

https://codereview.chromium.org/1056793006/diff/170001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:220: bool
position_specified =
I'm not sure what this check versus BoundsSpecification::kUnspecifiedPosition is
for. This logic is still pretty confusing.

It seems to be:
set bounds to origin from gfx::Insets(), preferred size.
If not docked, override with default bounds (in OnBeforePanelWidgetInit)
if bounds position is not kUnspecified, set bounds to be zero origin and,
preferred size.

Can that be simplified further? If not please explain in comments more as it
isn't clear to slow people like me :)

https://codereview.chromium.org/1056793006/diff/170001/chrome/browser/ui/view...
File chrome/browser/ui/views/apps/chrome_native_app_window_views.h (right):

https://codereview.chromium.org/1056793006/diff/170001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/chrome_native_app_window_views.h:35: virtual void
OnBeforePanelWidgetInit(bool use_default_origin,
Should use_default_origin be use_default_bounds? It seems to change the size as
well as the origin.

[varkha, 2015-04-30 02:13:45]

Patchset #5 (id:190001) has been deleted

[varkha, 2015-04-30 02:15:31]

PTAL, I've changed a flow a bit although the logic seems correct to me. Please
see if you like this change better.

https://codereview.chromium.org/1056793006/diff/170001/chrome/browser/ui/view...
File chrome/browser/ui/views/apps/chrome_native_app_window_views.cc (right):

https://codereview.chromium.org/1056793006/diff/170001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:220: bool
position_specified =
On 2015/04/29 21:46:42, benwells wrote:
> I'm not sure what this check versus BoundsSpecification::kUnspecifiedPosition
is
> for. This logic is still pretty confusing.
> 
> It seems to be:
> set bounds to origin from gfx::Insets(), preferred size.
> If not docked, override with default bounds (in OnBeforePanelWidgetInit)
> if bounds position is not kUnspecified, set bounds to be zero origin and,
> preferred size.
> 
> Can that be simplified further? If not please explain in comments more as it
> isn't clear to slow people like me :)

See if you like this flow better. I need to sanitize the position after getting
it from GetInitialWindowBounds similar to how it is done in
InitializeDefaultWindow (not panel).

https://codereview.chromium.org/1056793006/diff/170001/chrome/browser/ui/view...
File chrome/browser/ui/views/apps/chrome_native_app_window_views.h (right):

https://codereview.chromium.org/1056793006/diff/170001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/chrome_native_app_window_views.h:35: virtual void
OnBeforePanelWidgetInit(bool use_default_origin,
On 2015/04/29 21:46:42, benwells wrote:
> Should use_default_origin be use_default_bounds? It seems to change the size
as
> well as the origin.

Done.

[benwells, 2015-04-30 07:24:48]

lgtm

[varkha, 2015-04-30 15:54:37]

varkha@chromium.org changed reviewers:
+ skuhne@chromium.org

[varkha, 2015-04-30 15:54:38]

+skuhne@ for OWNERS in components/sessions (new value in the show type enum that
is used on ash platforms - Chrome OS and Windows Metro mode).

[varkha, 2015-05-04 14:07:30]

skuhne@, ping?

[Mr4D (OOO till 08-26), 2015-05-04 14:46:42]

lgtm

(sorry had a conjunctivitis and was not able to do anything)

[varkha, 2015-05-04 14:49:01]

Thanks all, oshima@, do you still have comments about this CL (+ OWNERS for
ash/*)?

[varkha, 2015-05-04 14:49:38]

The CQ bit was checked by varkha@chromium.org to run a CQ dry run

[varkha, 2015-05-04 14:49:38]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/1056793006/#ps210001
(title: "[Docking] Makes docked state persistent on Chrome OS (comments)")

[commit-bot: I haz the power, 2015-05-04 14:49:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1056793006/210001

[commit-bot: I haz the power, 2015-05-04 15:48:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-04 15:48:55]

Dry run: This issue passed the CQ dry run.

[varkha, 2015-05-05 16:13:34]

oshima@, ping?

[oshima, 2015-05-05 16:48:19]

lgtm

[varkha, 2015-05-05 16:48:47]

The CQ bit was checked by varkha@chromium.org

[commit-bot: I haz the power, 2015-05-05 16:49:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1056793006/210001

[commit-bot: I haz the power, 2015-05-05 18:12:46]

Committed patchset #5 (id:210001)

[commit-bot: I haz the power, 2015-05-05 18:13:44]

Patchset 5 (id:??) landed as
https://crrev.com/74014e0e926e2170188c9d7cc4706ccba36448c9
Cr-Commit-Position: refs/heads/master@{#328356}