mash/wm: Fix detecting container windows.

mash/wm: Fix detecting container windows.

Now that not all container windows are direct children of the root window, it is
necessary to consult the RootWindowController to find the container windows.
Also, instead of checking whether a direct child of the root, RootWindowController
needs to look at the local-id of a window to determine whether it's a container
window or not.

BUG=605668
Committed: https://crrev.com/0a064b94c18ee6d1d251c8d381aa8c22372e6248
Cr-Commit-Position: refs/heads/master@{#389152}

[sadrul, 2016-04-22 00:36:04]

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

[sky, 2016-04-22 15:23:42]

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

https://codereview.chromium.org/1907153002/diff/20001/mash/wm/root_window_con...
mash/wm/root_window_controller.cc:77: window->local_id() <
ContainerToLocalId(mojom::Container::COUNT);
The default local_id is -1, should you check for local_id >= 0?

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

https://codereview.chromium.org/1907153002/diff/20001/mash/wm/window_manager....
mash/wm/window_manager.cc:48: // Observe all the containers and their child
windows.
Why do you need to observer the child windows?

https://codereview.chromium.org/1907153002/diff/20001/mash/wm/window_manager....
mash/wm/window_manager.cc:50: for (int id = 1; id < count; ++id) {
1 -> ROOT + 1 ?

[sky, 2016-04-22 15:25:40]

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

https://codereview.chromium.org/1907153002/diff/20001/mash/wm/window_manager....
mash/wm/window_manager.cc:48: // Observe all the containers and their child
windows.
On 2016/04/22 15:23:42, sky wrote:
> Why do you need to observer the child windows?

Never mind this comment, I just saw your comments in your other patch and get
why you want to observe children. I think it's error prone to have this class
observe two types of windows for different purposes. I would rather you add an
observer specifically for children.

[sadrul, 2016-04-22 16:06:06]

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

https://codereview.chromium.org/1907153002/diff/20001/mash/wm/root_window_con...
mash/wm/root_window_controller.cc:77: window->local_id() <
ContainerToLocalId(mojom::Container::COUNT);
On 2016/04/22 15:23:41, sky wrote:
> The default local_id is -1, should you check for local_id >= 0?

Changed to > 0 (to exclude root)

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

https://codereview.chromium.org/1907153002/diff/20001/mash/wm/window_manager....
mash/wm/window_manager.cc:48: // Observe all the containers and their child
windows.
On 2016/04/22 15:25:40, sky wrote:
> On 2016/04/22 15:23:42, sky wrote:
> > Why do you need to observer the child windows?
> 
> Never mind this comment, I just saw your comments in your other patch and get
> why you want to observe children. I think it's error prone to have this class
> observe two types of windows for different purposes. I would rather you add an
> observer specifically for children.

Done (introduced a DisconnectedAppHandler). Change both WindowManager and
DisconnectedAppHandler to become WindowTrackers too, to make it easier to stop
observing during teardown.

https://codereview.chromium.org/1907153002/diff/20001/mash/wm/window_manager....
mash/wm/window_manager.cc:50: for (int id = 1; id < count; ++id) {
On 2016/04/22 15:23:42, sky wrote:
> 1 -> ROOT + 1 ?

Done.

[sky, 2016-04-22 16:34:27]

LGTM

https://codereview.chromium.org/1907153002/diff/60001/mash/wm/disconnected_ap...
File mash/wm/disconnected_app_handler.h (right):

https://codereview.chromium.org/1907153002/diff/60001/mash/wm/disconnected_ap...
mash/wm/disconnected_app_handler.h:14: class DisconnectedAppHandler : public
mus::WindowTracker {
Add a description.

[sadrul, 2016-04-22 17:08:47]

https://codereview.chromium.org/1907153002/diff/60001/mash/wm/disconnected_ap...
File mash/wm/disconnected_app_handler.h (right):

https://codereview.chromium.org/1907153002/diff/60001/mash/wm/disconnected_ap...
mash/wm/disconnected_app_handler.h:14: class DisconnectedAppHandler : public
mus::WindowTracker {
On 2016/04/22 16:34:27, sky wrote:
> Add a description.

Done.

[sadrul, 2016-04-22 17:09:10]

The CQ bit was checked by sadrul@chromium.org

[sadrul, 2016-04-22 17:09:11]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1907153002/#ps80001
(title: ".")

[commit-bot: I haz the power, 2016-04-22 17:09:33]

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

[commit-bot: I haz the power, 2016-04-22 17:37:01]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-22 19:49:16]

Description was changed from

==========
mash/wm: Fix detecting container windows.

Now that not all container windows are direct children of the root window, it is
necessary to consult the RootWindowController to find the container windows.
Also, instead of checking whether a direct child of the root,
RootWindowController
needs to look at the local-id of a window to determine whether it's a container
window or not.

BUG=605668
==========

to

==========
mash/wm: Fix detecting container windows.

Now that not all container windows are direct children of the root window, it is
necessary to consult the RootWindowController to find the container windows.
Also, instead of checking whether a direct child of the root,
RootWindowController
needs to look at the local-id of a window to determine whether it's a container
window or not.

BUG=605668

Committed: https://crrev.com/0a064b94c18ee6d1d251c8d381aa8c22372e6248
Cr-Commit-Position: refs/heads/master@{#389152}
==========

[commit-bot: I haz the power, 2016-04-22 19:49:17]

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