Changes container names and adds a couple of more

Changes container names and adds a couple of more

I think these names help clarify containment a bit more than the old
names. The new ones are to mirror those of ash.

This also adds mapping between ash ids and mash ids and centralizes
containers that can have active children.

BUG=603369
TEST=none
R=jamescook@chromium.org

Committed: https://crrev.com/9a86a4fa7177958444cf81a0678859387c1ea738
Cr-Commit-Position: refs/heads/master@{#393656}

[sky, 2016-05-13 16:31:55]

https://codereview.chromium.org/1980593002/diff/1/mash/wm/public/interfaces/c...
File mash/wm/public/interfaces/container.mojom (right):

https://codereview.chromium.org/1980593002/diff/1/mash/wm/public/interfaces/c...
mash/wm/public/interfaces/container.mojom:12: // NOTE: containers ending with
'_CONTAINER' exist purely to hold other
I realize Container::FOO_CONTAINER is a bit redundant, but I need something to
differentiate them.

[James Cook, 2016-05-13 17:14:45]

https://codereview.chromium.org/1980593002/diff/20001/mash/wm/container_ids.cc
File mash/wm/container_ids.cc (right):

https://codereview.chromium.org/1980593002/diff/20001/mash/wm/container_ids.c...
mash/wm/container_ids.cc:17: };
Do login windows need to be able to activate?

https://codereview.chromium.org/1980593002/diff/20001/mash/wm/container_ids.c...
mash/wm/container_ids.cc:60: return mojom::Container::ALL_USER_BACKGROUND;
If there's a bug where a window defaults to this, will it be visible? If so,
maybe the default should be in USER_PRIVATE somewhere?

https://codereview.chromium.org/1980593002/diff/20001/mash/wm/container_ids.c...
mash/wm/container_ids.cc:90: 
Something should go here.

https://codereview.chromium.org/1980593002/diff/20001/mash/wm/container_ids.c...
mash/wm/container_ids.cc:116: return kUnknownAshId;
Do you want to move more IDs into ash/wm/common/wm_shell_window_ids.h, for
example to allow this to be kShellWindowId_StatusContainer?

Status at least has a 1:1 mapping between ash and mash.

https://codereview.chromium.org/1980593002/diff/20001/mash/wm/container_ids.c...
mash/wm/container_ids.cc:122: return kUnknownAshId;
kShellWindowId_SystemModalContainer? (It's already in wm_shell_window_ids.h)

https://codereview.chromium.org/1980593002/diff/20001/mash/wm/public/interfac...
File mash/wm/public/interfaces/container.mojom (right):

https://codereview.chromium.org/1980593002/diff/20001/mash/wm/public/interfac...
mash/wm/public/interfaces/container.mojom:22: USER_PRIVATE_CONTAINER,
I think it would be just as clear to use a short name for the parent container
and longer names for children, e.g.

USER_PRIVATE
USER_PRIVATE_WINDOWS
USER_PRIVATE_ALWAYS_ON_TOP_WINDOWS

Then you don't have the situation that they are all containers, but some have
_CONTAINER in the name.

If you really think it needs something to indicate that it's a container of
containers, maybe USER_PRIVATE_PARENT? USER_PRIVATE_ROOT?

https://codereview.chromium.org/1980593002/diff/20001/mash/wm/public/interfac...
mash/wm/public/interfaces/container.mojom:31: LOGIN_CONTAINER,
optional: This file might be clearer with a blank line between each "section" of
containers, like here.

https://codereview.chromium.org/1980593002/diff/20001/mash/wm/root_window_con...
File mash/wm/root_window_controller.cc (right):

https://codereview.chromium.org/1980593002/diff/20001/mash/wm/root_window_con...
mash/wm/root_window_controller.cc:244: 
Ah, this is much easier to read with newlines. Hooray!

[sky, 2016-05-13 19:14:06]

https://codereview.chromium.org/1980593002/diff/20001/mash/wm/container_ids.cc
File mash/wm/container_ids.cc (right):

https://codereview.chromium.org/1980593002/diff/20001/mash/wm/container_ids.c...
mash/wm/container_ids.cc:17: };
On 2016/05/13 17:14:44, James Cook wrote:
> Do login windows need to be able to activate?

I think this list is woefully wrong. I added a TODO.

https://codereview.chromium.org/1980593002/diff/20001/mash/wm/container_ids.c...
mash/wm/container_ids.cc:60: return mojom::Container::ALL_USER_BACKGROUND;
On 2016/05/13 17:14:44, James Cook wrote:
> If there's a bug where a window defaults to this, will it be visible? If so,
> maybe the default should be in USER_PRIVATE somewhere?

Made it return USER_PRIVATE_WINDOWS.

https://codereview.chromium.org/1980593002/diff/20001/mash/wm/container_ids.c...
mash/wm/container_ids.cc:90: 
On 2016/05/13 17:14:44, James Cook wrote:
> Something should go here.

Done.

https://codereview.chromium.org/1980593002/diff/20001/mash/wm/container_ids.c...
mash/wm/container_ids.cc:116: return kUnknownAshId;
On 2016/05/13 17:14:44, James Cook wrote:
> Do you want to move more IDs into ash/wm/common/wm_shell_window_ids.h, for
> example to allow this to be kShellWindowId_StatusContainer?
> 
> Status at least has a 1:1 mapping between ash and mash.

At this point wm_shell_window_ids is just the ids used by ash/wm/common. With
time that will likely change, but for now I would like to keep the list at only
what is needed by that code.

https://codereview.chromium.org/1980593002/diff/20001/mash/wm/container_ids.c...
mash/wm/container_ids.cc:122: return kUnknownAshId;
On 2016/05/13 17:14:44, James Cook wrote:
> kShellWindowId_SystemModalContainer? (It's already in wm_shell_window_ids.h)

SYSTEM_MODAL_WINDOWS is intended for non user specific system modals. It may be
I should map it to kShellWindowId_LockSystemModalContainer, but I'm not 100%
sure. I'm going to leave it as unknown for now and will come back to it.

https://codereview.chromium.org/1980593002/diff/20001/mash/wm/public/interfac...
File mash/wm/public/interfaces/container.mojom (right):

https://codereview.chromium.org/1980593002/diff/20001/mash/wm/public/interfac...
mash/wm/public/interfaces/container.mojom:22: USER_PRIVATE_CONTAINER,
On 2016/05/13 17:14:44, James Cook wrote:
> I think it would be just as clear to use a short name for the parent container
> and longer names for children, e.g.
> 
> USER_PRIVATE
> USER_PRIVATE_WINDOWS
> USER_PRIVATE_ALWAYS_ON_TOP_WINDOWS
> 
> Then you don't have the situation that they are all containers, but some have
> _CONTAINER in the name.
> 
> If you really think it needs something to indicate that it's a container of
> containers, maybe USER_PRIVATE_PARENT? USER_PRIVATE_ROOT?

Done.

https://codereview.chromium.org/1980593002/diff/20001/mash/wm/public/interfac...
mash/wm/public/interfaces/container.mojom:31: LOGIN_CONTAINER,
On 2016/05/13 17:14:44, James Cook wrote:
> optional: This file might be clearer with a blank line between each "section"
of
> containers, like here.

I couldn't come up with a sensical way to add whitespace to this file, so I'm
sticking with no whitespace.

[James Cook, 2016-05-13 19:49:12]

LGTM

https://codereview.chromium.org/1980593002/diff/40001/mash/wm/container_ids.h
File mash/wm/container_ids.h (right):

https://codereview.chromium.org/1980593002/diff/40001/mash/wm/container_ids.h...
mash/wm/container_ids.h:8: #include
"mash/wm/public/interfaces/container.mojom.h"
One thing I missed before... Do you need this include, given that you are
forward declaring Container below?

[sky, 2016-05-13 19:59:52]

https://codereview.chromium.org/1980593002/diff/40001/mash/wm/container_ids.h
File mash/wm/container_ids.h (right):

https://codereview.chromium.org/1980593002/diff/40001/mash/wm/container_ids.h...
mash/wm/container_ids.h:8: #include
"mash/wm/public/interfaces/container.mojom.h"
On 2016/05/13 19:49:12, James Cook wrote:
> One thing I missed before... Do you need this include, given that you are
> forward declaring Container below?

Done.

[sky, 2016-05-13 19:59:56]

The CQ bit was checked by sky@chromium.org

[sky, 2016-05-13 19:59:57]

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/1980593002/#ps60001
(title: "change include")

[commit-bot: I haz the power, 2016-05-13 20:00:24]

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

[commit-bot: I haz the power, 2016-05-13 22:03:41]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-13 22:04:49]

Description was changed from

==========
Changes container names and adds a couple of more

I think these names help clarify containment a bit more than the old
names. The new ones are to mirror those of ash.

This also adds mapping between ash ids and mash ids and centralizes
containers that can have active children.

BUG=603369
TEST=none
R=jamescook@chromium.org
==========

to

==========
Changes container names and adds a couple of more

I think these names help clarify containment a bit more than the old
names. The new ones are to mirror those of ash.

This also adds mapping between ash ids and mash ids and centralizes
containers that can have active children.

BUG=603369
TEST=none
R=jamescook@chromium.org

Committed: https://crrev.com/9a86a4fa7177958444cf81a0678859387c1ea738
Cr-Commit-Position: refs/heads/master@{#393656}
==========

[commit-bot: I haz the power, 2016-05-13 22:04:50]

Patchset 4 (id:??) landed as
https://crrev.com/9a86a4fa7177958444cf81a0678859387c1ea738
Cr-Commit-Position: refs/heads/master@{#393656}