Support additional mash shelf alignments.

Support additional mash shelf alignments.

Support left/bottom/right alignments in ash::wm::ShelfLayout.
Propagate alignment changes with a new ShelfLayout interface.
Add RootWindowController::GetLayoutManagerForContainer helper.

TODO: Update preferred sizes with alignment to avoid flicker.

BUG=595852
TEST=mash shelf supports left/bottom/right alignments (with some flicker).
R=jamescook@chromium.org,sky@chromium.org

Committed: https://crrev.com/68b18edb1ca481397ac2ccd95c40e201f657adfe
Cr-Commit-Position: refs/heads/master@{#387679}

[James Cook, 2016-03-29 00:28:29]

jamescook@chromium.org changed reviewers:
+ jamescook@chromium.org

[James Cook, 2016-03-29 00:28:30]

I didn't really review this, and we talked offline, so just a couple things.

https://codereview.chromium.org/1835403002/diff/1/mash/wm/public/interfaces/s...
File mash/wm/public/interfaces/shelf_layout.mojom (right):

https://codereview.chromium.org/1835403002/diff/1/mash/wm/public/interfaces/s...
mash/wm/public/interfaces/shelf_layout.mojom:11: // TODO(msw): Why even bother
including? SHELF_ALIGNMENT_TOP,
Don't bother, ALIGNMENT_TOP has never been supported. It was taking too long to
build and didn't work well with the tab strip right below it, so we scrapped it
in the middle of Ash development.

https://codereview.chromium.org/1835403002/diff/1/mash/wm/public/interfaces/s...
mash/wm/public/interfaces/shelf_layout.mojom:20: // Used to push desired shelf
state to the shelf container's layout manager.
nit: Is this really "desired"? It seems like sysui is just telling the WM --
hey, this is the current shelf layout, deal with it.  Also, what do you think
for these sorts of mojom interfaces with including in the comment who the
typical caller will be? Like, "Used by sysui to push the shelf state to...."

[msw, 2016-04-05 22:38:00]

Patchset #2 (id:20001) has been deleted

[msw, 2016-04-11 20:12:42]

Patchset #3 (id:60001) has been deleted

[msw, 2016-04-11 20:12:48]

Patchset #2 (id:40001) has been deleted

[msw, 2016-04-11 22:14:16]

Patchset #3 (id:100001) has been deleted

[msw, 2016-04-11 22:17:31]

Description was changed from

==========
Support additional mash shelf alignments.

BUG=595852
==========

to

==========
Support additional mash shelf alignments.

Support left/bottom/right alignments in ash::wm::ShelfLayout.
Propagate alignment changes with a new ShelfLayout interface.
Add RootWindowController::GetLayoutManagerForContainer helper.

TODO: Update preferred sizes with alignment to avoid flicker.

BUG=595852
TEST=mash shelf supports left/bottom/right alignments (with some flicker).
R=jamescook@chromium.org
==========

[msw, 2016-04-11 22:20:27]

Hey James, please take a look; thanks!
(Scott said flickering on re-alignment seems ok for now)

https://codereview.chromium.org/1835403002/diff/1/mash/wm/public/interfaces/s...
File mash/wm/public/interfaces/shelf_layout.mojom (right):

https://codereview.chromium.org/1835403002/diff/1/mash/wm/public/interfaces/s...
mash/wm/public/interfaces/shelf_layout.mojom:11: // TODO(msw): Why even bother
including? SHELF_ALIGNMENT_TOP,
On 2016/03/29 00:28:30, James Cook wrote:
> Don't bother, ALIGNMENT_TOP has never been supported. It was taking too long
to
> build and didn't work well with the tab strip right below it, so we scrapped
it
> in the middle of Ash development.

Done.

https://codereview.chromium.org/1835403002/diff/1/mash/wm/public/interfaces/s...
mash/wm/public/interfaces/shelf_layout.mojom:20: // Used to push desired shelf
state to the shelf container's layout manager.
On 2016/03/29 00:28:29, James Cook wrote:
> nit: Is this really "desired"? It seems like sysui is just telling the WM --
> hey, this is the current shelf layout, deal with it.  Also, what do you think
> for these sorts of mojom interfaces with including in the comment who the
> typical caller will be? Like, "Used by sysui to push the shelf state to...."

I took both suggested comment changes even though (1) the wm ultimately owns the
shelf bounds and (2) I'm not sure if mentioning the current client is helpful.

[msw, 2016-04-11 22:20:34]

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

[commit-bot: I haz the power, 2016-04-11 22:21:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1835403002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1835403002/120001

[commit-bot: I haz the power, 2016-04-11 22:25:45]

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

[commit-bot: I haz the power, 2016-04-11 22:25:46]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[James Cook, 2016-04-11 23:36:49]

LGTM with nits

https://codereview.chromium.org/1835403002/diff/120001/mash/wm/shelf_layout.cc
File mash/wm/shelf_layout.cc (right):

https://codereview.chromium.org/1835403002/diff/120001/mash/wm/shelf_layout.c...
mash/wm/shelf_layout.cc:52: // TODO(msw): Place the status area on the right for
RTL UIs.
on the right? or on the left?

https://codereview.chromium.org/1835403002/diff/120001/mash/wm/shelf_layout.c...
mash/wm/shelf_layout.cc:79: for (auto window : owner()->children())
I think for range-based for loops we prefer const auto& or auto& or plain-old
mus::Window*

https://codereview.chromium.org/1835403002/diff/120001/mash/wm/window_manager...
File mash/wm/window_manager_application.h (right):

https://codereview.chromium.org/1835403002/diff/120001/mash/wm/window_manager...
mash/wm/window_manager_application.h:126:
std::vector<scoped_ptr<mojo::InterfaceRequest<mash::wm::mojom::ShelfLayout>>>
scoped_ptr -> unique_ptr ... I think this file has already been through the
mega-conversion

[msw, 2016-04-12 00:13:09]

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

[msw, 2016-04-12 00:13:46]

Description was changed from

==========
Support additional mash shelf alignments.

Support left/bottom/right alignments in ash::wm::ShelfLayout.
Propagate alignment changes with a new ShelfLayout interface.
Add RootWindowController::GetLayoutManagerForContainer helper.

TODO: Update preferred sizes with alignment to avoid flicker.

BUG=595852
TEST=mash shelf supports left/bottom/right alignments (with some flicker).
R=jamescook@chromium.org
==========

to

==========
Support additional mash shelf alignments.

Support left/bottom/right alignments in ash::wm::ShelfLayout.
Propagate alignment changes with a new ShelfLayout interface.
Add RootWindowController::GetLayoutManagerForContainer helper.

TODO: Update preferred sizes with alignment to avoid flicker.

BUG=595852
TEST=mash shelf supports left/bottom/right alignments (with some flicker).
R=jamescook@chromium.org,sky@chromium.org
==========

[msw, 2016-04-12 00:13:47]

msw@chromium.org changed reviewers:
+ sky@chromium.org

[msw, 2016-04-12 00:14:00]

Hey Scott, please take a look; thanks!

https://codereview.chromium.org/1835403002/diff/120001/mash/wm/shelf_layout.cc
File mash/wm/shelf_layout.cc (right):

https://codereview.chromium.org/1835403002/diff/120001/mash/wm/shelf_layout.c...
mash/wm/shelf_layout.cc:52: // TODO(msw): Place the status area on the right for
RTL UIs.
On 2016/04/11 23:36:48, James Cook wrote:
> on the right? or on the left?

Done.

https://codereview.chromium.org/1835403002/diff/120001/mash/wm/shelf_layout.c...
mash/wm/shelf_layout.cc:79: for (auto window : owner()->children())
On 2016/04/11 23:36:48, James Cook wrote:
> I think for range-based for loops we prefer const auto& or auto& or plain-old
> mus::Window*

Done.

https://codereview.chromium.org/1835403002/diff/120001/mash/wm/window_manager...
File mash/wm/window_manager_application.h (right):

https://codereview.chromium.org/1835403002/diff/120001/mash/wm/window_manager...
mash/wm/window_manager_application.h:126:
std::vector<scoped_ptr<mojo::InterfaceRequest<mash::wm::mojom::ShelfLayout>>>
On 2016/04/11 23:36:48, James Cook wrote:
> scoped_ptr -> unique_ptr ... I think this file has already been through the
> mega-conversion

Done.

https://codereview.chromium.org/1835403002/diff/160001/mash/wm/public/interfa...
File mash/wm/public/interfaces/shelf_layout.mojom (right):

https://codereview.chromium.org/1835403002/diff/160001/mash/wm/public/interfa...
mash/wm/public/interfaces/shelf_layout.mojom:8: enum ShelfAlignment { BOTTOM,
LEFT, RIGHT, };
These enums already exist (in mash/shelf/public/interfaces/shelf.mojom and
ash/shelf/shelf_types.h), and shelf.mojom's Shelf interface is a more
feature-full version of the ShelfLayout interface below. Should I somehow share
these enum and/or interface definitions? Can we do so without having the
definitions live in mash/wm? (seems like they should live in mash/shelf, but
mash/shelf depends on mash/wm, not the inverse).

[commit-bot: I haz the power, 2016-04-12 00:14:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1835403002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1835403002/160001

[msw, 2016-04-12 00:33:17]

https://codereview.chromium.org/1835403002/diff/160001/mash/wm/window_manager...
File mash/wm/window_manager_application.h (right):

https://codereview.chromium.org/1835403002/diff/160001/mash/wm/window_manager...
mash/wm/window_manager_application.h:123: // The ShelfLayout object is created
once OnEmbed() is called. Until that
Also, it's unclear that we actually encounter this situation; can we just drop
any requests for Shelf or UserWindowController before they exist?

[commit-bot: I haz the power, 2016-04-12 01:05:12]

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

[commit-bot: I haz the power, 2016-04-12 01:05:13]

Dry run: This issue passed the CQ dry run.

[msw, 2016-04-12 23:24:06]

Scott, friendly ping! Please take a look, thanks!

[sky, 2016-04-13 23:44:11]

https://codereview.chromium.org/1835403002/diff/160001/mash/wm/public/interfa...
File mash/wm/public/interfaces/shelf_layout.mojom (right):

https://codereview.chromium.org/1835403002/diff/160001/mash/wm/public/interfa...
mash/wm/public/interfaces/shelf_layout.mojom:8: enum ShelfAlignment { BOTTOM,
LEFT, RIGHT, };
On 2016/04/12 00:14:00, msw wrote:
> These enums already exist (in mash/shelf/public/interfaces/shelf.mojom and
> ash/shelf/shelf_types.h), and shelf.mojom's Shelf interface is a more
> feature-full version of the ShelfLayout interface below. Should I somehow
share
> these enum and/or interface definitions? Can we do so without having the
> definitions live in mash/wm? (seems like they should live in mash/shelf, but
> mash/shelf depends on mash/wm, not the inverse).

The ash directory is likely to morph into the sysui directory. Seems like ash
should pull the enums from here. I recommend moving ShelfAlignment (and others
you want to sure) into a shelf_constants mojom that both places pull from.

https://codereview.chromium.org/1835403002/diff/160001/mash/wm/window_manager...
File mash/wm/window_manager_application.cc (right):

https://codereview.chromium.org/1835403002/diff/160001/mash/wm/window_manager...
mash/wm/window_manager_application.cc:77: ShelfLayout* shelf_layout =
These casts make me nervous. I prefer GetShelfLayoutManager().

https://codereview.chromium.org/1835403002/diff/160001/mash/wm/window_manager...
File mash/wm/window_manager_application.h (right):

https://codereview.chromium.org/1835403002/diff/160001/mash/wm/window_manager...
mash/wm/window_manager_application.h:123: // The ShelfLayout object is created
once OnEmbed() is called. Until that
On 2016/04/12 00:33:17, msw wrote:
> Also, it's unclear that we actually encounter this situation; can we just drop
> any requests for Shelf or UserWindowController before they exist?

What guarantees that? By that I mean how do you know that ShelfLayout and/or
UnserWindowController is not asked for before OnEmbed?

[msw, 2016-04-15 17:25:06]

Hey Scott, please take another look; thanks!

https://codereview.chromium.org/1835403002/diff/160001/mash/wm/public/interfa...
File mash/wm/public/interfaces/shelf_layout.mojom (right):

https://codereview.chromium.org/1835403002/diff/160001/mash/wm/public/interfa...
mash/wm/public/interfaces/shelf_layout.mojom:8: enum ShelfAlignment { BOTTOM,
LEFT, RIGHT, };
On 2016/04/13 23:44:11, sky wrote:
> On 2016/04/12 00:14:00, msw wrote:
> > These enums already exist (in mash/shelf/public/interfaces/shelf.mojom and
> > ash/shelf/shelf_types.h), and shelf.mojom's Shelf interface is a more
> > feature-full version of the ShelfLayout interface below. Should I somehow
> share
> > these enum and/or interface definitions? Can we do so without having the
> > definitions live in mash/wm? (seems like they should live in mash/shelf, but
> > mash/shelf depends on mash/wm, not the inverse).
> 
> The ash directory is likely to morph into the sysui directory. Seems like ash
> should pull the enums from here. I recommend moving ShelfAlignment (and others
> you want to sure) into a shelf_constants mojom that both places pull from.

Added mash/shelf/.../shelf_constants.mojom, lmk if you wanted mash/wm...

https://codereview.chromium.org/1835403002/diff/160001/mash/wm/window_manager...
File mash/wm/window_manager_application.cc (right):

https://codereview.chromium.org/1835403002/diff/160001/mash/wm/window_manager...
mash/wm/window_manager_application.cc:77: ShelfLayout* shelf_layout =
On 2016/04/13 23:44:11, sky wrote:
> These casts make me nervous. I prefer GetShelfLayoutManager().

Done (as a RootWindowController member, which still has a cast).

https://codereview.chromium.org/1835403002/diff/160001/mash/wm/window_manager...
File mash/wm/window_manager_application.h (right):

https://codereview.chromium.org/1835403002/diff/160001/mash/wm/window_manager...
mash/wm/window_manager_application.h:123: // The ShelfLayout object is created
once OnEmbed() is called. Until that
On 2016/04/13 23:44:11, sky wrote:
> On 2016/04/12 00:33:17, msw wrote:
> > Also, it's unclear that we actually encounter this situation; can we just
drop
> > any requests for Shelf or UserWindowController before they exist?
> 
> What guarantees that? By that I mean how do you know that ShelfLayout and/or
> UnserWindowController is not asked for before OnEmbed?

I guess nothing guarantees that, it just seems odd to support shelf interaction
before the window manager is initialized, and that doesn't happen in my testing.

[msw, 2016-04-15 17:25:18]

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

[commit-bot: I haz the power, 2016-04-15 17:25:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1835403002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1835403002/180001

[commit-bot: I haz the power, 2016-04-15 18:09:01]

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

[commit-bot: I haz the power, 2016-04-15 18:09:02]

Dry run: This issue passed the CQ dry run.

[sky, 2016-04-15 19:33:23]

LGTM

https://codereview.chromium.org/1835403002/diff/160001/mash/wm/window_manager...
File mash/wm/window_manager_application.h (right):

https://codereview.chromium.org/1835403002/diff/160001/mash/wm/window_manager...
mash/wm/window_manager_application.h:123: // The ShelfLayout object is created
once OnEmbed() is called. Until that
On 2016/04/15 17:25:06, msw wrote:
> On 2016/04/13 23:44:11, sky wrote:
> > On 2016/04/12 00:33:17, msw wrote:
> > > Also, it's unclear that we actually encounter this situation; can we just
> drop
> > > any requests for Shelf or UserWindowController before they exist?
> > 
> > What guarantees that? By that I mean how do you know that ShelfLayout and/or
> > UnserWindowController is not asked for before OnEmbed?
> 
> I guess nothing guarantees that, it just seems odd to support shelf
interaction
> before the window manager is initialized, and that doesn't happen in my
testing.

It shouldn't be hard to get this scenario in a test. Connect to the desktop_wm
directly and ask for the shelflayout. If mus isn't running that request will be
seen before OnEmbed.

[msw, 2016-04-15 19:37:42]

The CQ bit was checked by msw@chromium.org

[msw, 2016-04-15 19:37:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/1835403002/#ps180001
(title: "Sync and rebase; address comments (sorry).")

[commit-bot: I haz the power, 2016-04-15 19:38:10]

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

[commit-bot: I haz the power, 2016-04-15 19:43:48]

Description was changed from

==========
Support additional mash shelf alignments.

Support left/bottom/right alignments in ash::wm::ShelfLayout.
Propagate alignment changes with a new ShelfLayout interface.
Add RootWindowController::GetLayoutManagerForContainer helper.

TODO: Update preferred sizes with alignment to avoid flicker.

BUG=595852
TEST=mash shelf supports left/bottom/right alignments (with some flicker).
R=jamescook@chromium.org,sky@chromium.org
==========

to

==========
Support additional mash shelf alignments.

Support left/bottom/right alignments in ash::wm::ShelfLayout.
Propagate alignment changes with a new ShelfLayout interface.
Add RootWindowController::GetLayoutManagerForContainer helper.

TODO: Update preferred sizes with alignment to avoid flicker.

BUG=595852
TEST=mash shelf supports left/bottom/right alignments (with some flicker).
R=jamescook@chromium.org,sky@chromium.org
==========

[commit-bot: I haz the power, 2016-04-15 19:43:49]

Committed patchset #6 (id:180001)

[commit-bot: I haz the power, 2016-04-15 19:46:16]

Description was changed from

==========
Support additional mash shelf alignments.

Support left/bottom/right alignments in ash::wm::ShelfLayout.
Propagate alignment changes with a new ShelfLayout interface.
Add RootWindowController::GetLayoutManagerForContainer helper.

TODO: Update preferred sizes with alignment to avoid flicker.

BUG=595852
TEST=mash shelf supports left/bottom/right alignments (with some flicker).
R=jamescook@chromium.org,sky@chromium.org
==========

to

==========
Support additional mash shelf alignments.

Support left/bottom/right alignments in ash::wm::ShelfLayout.
Propagate alignment changes with a new ShelfLayout interface.
Add RootWindowController::GetLayoutManagerForContainer helper.

TODO: Update preferred sizes with alignment to avoid flicker.

BUG=595852
TEST=mash shelf supports left/bottom/right alignments (with some flicker).
R=jamescook@chromium.org,sky@chromium.org

Committed: https://crrev.com/68b18edb1ca481397ac2ccd95c40e201f657adfe
Cr-Commit-Position: refs/heads/master@{#387679}
==========

[commit-bot: I haz the power, 2016-04-15 19:46:18]

Patchset 6 (id:??) landed as
https://crrev.com/68b18edb1ca481397ac2ccd95c40e201f657adfe
Cr-Commit-Position: refs/heads/master@{#387679}