[Chromecast] Add support for z-order and window focus.

[Chromecast] Add support for z-order and window focus.

BUG=internal b/33046596, internal b/33047358, internal b/34103918
TEST=keyboard input should work in a window with google.com

Change-Id: I01d9ae64d3175298d439577e4c83bea4ada6203f
Review-Url: https://codereview.chromium.org/2636303002
Cr-Commit-Position: refs/heads/master@{#447006}
Committed: https://chromium.googlesource.com/chromium/src/+/ff442c09047c888651b3b43a8e8d4955092d33ea

[Joshua LeVasseur, 2017-01-17 23:01:30]

jlevasseur@chromium.org changed reviewers:
+ derekjchow@chromium.org, halliwell@chromium.org

[Joshua LeVasseur, 2017-01-19 17:22:27]

This is ready for review.

[derekjchow1, 2017-01-19 18:12:13]

https://codereview.chromium.org/2636303002/diff/80001/chromecast/graphics/cas...
File chromecast/graphics/cast_window_manager.h (right):

https://codereview.chromium.org/2636303002/diff/80001/chromecast/graphics/cas...
chromecast/graphics/cast_window_manager.h:22: enum {
Where is this enum actually used?

https://codereview.chromium.org/2636303002/diff/80001/chromecast/graphics/cas...
chromecast/graphics/cast_window_manager.h:24: APP = BOTTOM,
To mimic line TOP = VOLUME, shouldn't BOTTOM = APP?

https://codereview.chromium.org/2636303002/diff/80001/chromecast/graphics/cas...
File chromecast/graphics/cast_window_manager_aura.cc (right):

https://codereview.chromium.org/2636303002/diff/80001/chromecast/graphics/cas...
chromecast/graphics/cast_window_manager_aura.cc:20: static bool
kFocusFollowsVisibility = true;
static keyword isn't needed here. const keyword should be.

https://codereview.chromium.org/2636303002/diff/80001/chromecast/graphics/cas...
chromecast/graphics/cast_window_manager_aura.cc:180: for (auto&& other :
windows) {
Could this be done with std::reduce?

http://en.cppreference.com/w/cpp/algorithm/reduce

Does it look cleaner?

https://codereview.chromium.org/2636303002/diff/80001/chromecast/graphics/cas...
chromecast/graphics/cast_window_manager_aura.cc:183: } else if ((other->id() <
child->id()) &&
No need for else case after a continue statement.

https://codereview.chromium.org/2636303002/diff/80001/chromecast/graphics/cas...
chromecast/graphics/cast_window_manager_aura.cc:208:
//////////////////////////////////////////////////////////////////////
I don't think this comment is needed in a .cc file.

https://codereview.chromium.org/2636303002/diff/80001/chromecast/graphics/cas...
chromecast/graphics/cast_window_manager_aura.cc:292: LOG(INFO) << "Requesting
focus for top-level: " << top_level->id() << ": "
This is duplicated a lot, could you turn this into a helper function or macro?

https://codereview.chromium.org/2636303002/diff/80001/chromecast/graphics/cas...
chromecast/graphics/cast_window_manager_aura.cc:337: if (focused_window_ !=
window) {
Why is this here? A few lines above make focused_window_ == window, so this
should always be true. Can OnWindowFocused function change focused_window_?

[Joshua LeVasseur, 2017-01-19 19:35:25]

https://codereview.chromium.org/2636303002/diff/80001/chromecast/graphics/cas...
File chromecast/graphics/cast_window_manager.h (right):

https://codereview.chromium.org/2636303002/diff/80001/chromecast/graphics/cas...
chromecast/graphics/cast_window_manager.h:22: enum {
On 2017/01/19 18:12:13, derekjchow1 wrote:
> Where is this enum actually used?

Pending CLs and internal code.

https://codereview.chromium.org/2636303002/diff/80001/chromecast/graphics/cas...
chromecast/graphics/cast_window_manager.h:24: APP = BOTTOM,
On 2017/01/19 18:12:13, derekjchow1 wrote:
> To mimic line TOP = VOLUME, shouldn't BOTTOM = APP?

That would require APP to be defined first at -1 (-1 is the default window ID
for Aura).  I'd rather have BOTTOM and TOP sandwich the others, for easier
reading.

https://codereview.chromium.org/2636303002/diff/80001/chromecast/graphics/cas...
File chromecast/graphics/cast_window_manager_aura.cc (right):

https://codereview.chromium.org/2636303002/diff/80001/chromecast/graphics/cas...
chromecast/graphics/cast_window_manager_aura.cc:20: static bool
kFocusFollowsVisibility = true;
On 2017/01/19 18:12:13, derekjchow1 wrote:
> static keyword isn't needed here. const keyword should be.

Done.

https://codereview.chromium.org/2636303002/diff/80001/chromecast/graphics/cas...
chromecast/graphics/cast_window_manager_aura.cc:180: for (auto&& other :
windows) {
On 2017/01/19 18:12:13, derekjchow1 wrote:
> Could this be done with std::reduce?
> 
> http://en.cppreference.com/w/cpp/algorithm/reduce
> 
> Does it look cleaner?

I don't see it ... I'd need an example, but I don't see std::reduce used
anywhere in the code base on cs.chromium.org.

https://codereview.chromium.org/2636303002/diff/80001/chromecast/graphics/cas...
chromecast/graphics/cast_window_manager_aura.cc:183: } else if ((other->id() <
child->id()) &&
On 2017/01/19 18:12:13, derekjchow1 wrote:
> No need for else case after a continue statement.

Done.

https://codereview.chromium.org/2636303002/diff/80001/chromecast/graphics/cas...
chromecast/graphics/cast_window_manager_aura.cc:208:
//////////////////////////////////////////////////////////////////////
On 2017/01/19 18:12:13, derekjchow1 wrote:
> I don't think this comment is needed in a .cc file.

Done.

https://codereview.chromium.org/2636303002/diff/80001/chromecast/graphics/cas...
chromecast/graphics/cast_window_manager_aura.cc:292: LOG(INFO) << "Requesting
focus for top-level: " << top_level->id() << ": "
On 2017/01/19 18:12:13, derekjchow1 wrote:
> This is duplicated a lot, could you turn this into a helper function or macro?

Done.

https://codereview.chromium.org/2636303002/diff/80001/chromecast/graphics/cas...
chromecast/graphics/cast_window_manager_aura.cc:337: if (focused_window_ !=
window) {
On 2017/01/19 18:12:13, derekjchow1 wrote:
> Why is this here? A few lines above make focused_window_ == window, so this
> should always be true. Can OnWindowFocused function change focused_window_?

Yes, OnWindowFocused can change focused_window_.  This needs a comment.

[halliwell, 2017-01-19 20:49:17]

https://codereview.chromium.org/2636303002/diff/120001/chromecast/graphics/ca...
File chromecast/graphics/cast_window_manager.h (right):

https://codereview.chromium.org/2636303002/diff/120001/chromecast/graphics/ca...
chromecast/graphics/cast_window_manager.h:30: } WindowId;
Maybe I'm being slow, why isn't this enum WindowId { ... }; ?

https://codereview.chromium.org/2636303002/diff/120001/chromecast/graphics/ca...
File chromecast/graphics/cast_window_manager_aura.cc (right):

https://codereview.chromium.org/2636303002/diff/120001/chromecast/graphics/ca...
chromecast/graphics/cast_window_manager_aura.cc:181: // Determine z-order
relative to existing windows.
This is ordering windows by ID?  Is the assumption that we use the WindowId enum
to set window IDs?

https://codereview.chromium.org/2636303002/diff/120001/chromecast/graphics/ca...
chromecast/graphics/cast_window_manager_aura.cc:263: }
All of these focus change logic functions are kind of hard to follow ... I mean
the logic is fine but I have no idea what the high level goals are or how to
tell if there are bugs.

It would be best to add some tests that set out the expected behaviour and allow
people to update this code with confidence.

Some high level comments on each function would be useful as well.

https://codereview.chromium.org/2636303002/diff/120001/chromecast/graphics/ca...
File chromecast/graphics/cast_window_manager_aura.h (right):

https://codereview.chromium.org/2636303002/diff/120001/chromecast/graphics/ca...
chromecast/graphics/cast_window_manager_aura.h:77: void
UpdateWindowFocus(aura::Window* skip);
what does 'skip' mean?

[Joshua LeVasseur, 2017-01-23 23:38:32]

https://codereview.chromium.org/2636303002/diff/120001/chromecast/graphics/ca...
File chromecast/graphics/cast_window_manager.h (right):

https://codereview.chromium.org/2636303002/diff/120001/chromecast/graphics/ca...
chromecast/graphics/cast_window_manager.h:30: } WindowId;
On 2017/01/19 20:49:17, halliwell wrote:
> Maybe I'm being slow, why isn't this enum WindowId { ... }; ?

Done.

https://codereview.chromium.org/2636303002/diff/120001/chromecast/graphics/ca...
File chromecast/graphics/cast_window_manager_aura.cc (right):

https://codereview.chromium.org/2636303002/diff/120001/chromecast/graphics/ca...
chromecast/graphics/cast_window_manager_aura.cc:181: // Determine z-order
relative to existing windows.
On 2017/01/19 20:49:17, halliwell wrote:
> This is ordering windows by ID?  Is the assumption that we use the WindowId
enum
> to set window IDs?

Yes, we use this enum for setting window IDs.  If you don't set the ID, it just
keeps the default of -1.

https://codereview.chromium.org/2636303002/diff/120001/chromecast/graphics/ca...
chromecast/graphics/cast_window_manager_aura.cc:263: }
On 2017/01/19 20:49:17, halliwell wrote:
> All of these focus change logic functions are kind of hard to follow ... I
mean
> the logic is fine but I have no idea what the high level goals are or how to
> tell if there are bugs.
> 
> It would be best to add some tests that set out the expected behaviour and
allow
> people to update this code with confidence.
> 
> Some high level comments on each function would be useful as well.

Done.

https://codereview.chromium.org/2636303002/diff/120001/chromecast/graphics/ca...
File chromecast/graphics/cast_window_manager_aura.h (right):

https://codereview.chromium.org/2636303002/diff/120001/chromecast/graphics/ca...
chromecast/graphics/cast_window_manager_aura.h:77: void
UpdateWindowFocus(aura::Window* skip);
On 2017/01/19 20:49:17, halliwell wrote:
> what does 'skip' mean?

It was redundant, so removed.

[halliwell, 2017-01-24 00:54:46]

Looking good.  Thanks for the tests and comments, much clearer to me now :D

https://codereview.chromium.org/2636303002/diff/160001/chromecast/graphics/BU...
File chromecast/graphics/BUILD.gn (right):

https://codereview.chromium.org/2636303002/diff/160001/chromecast/graphics/BU...
chromecast/graphics/BUILD.gn:74: "cast_window_manager_aura_test.cc",
Do these need guarding (e.g. for Android builds)

https://codereview.chromium.org/2636303002/diff/160001/chromecast/graphics/ca...
File chromecast/graphics/cast_focus_client_aura.cc (right):

https://codereview.chromium.org/2636303002/diff/160001/chromecast/graphics/ca...
chromecast/graphics/cast_focus_client_aura.cc:25: ++it) {
minor: can this be a range-based loop?

https://codereview.chromium.org/2636303002/diff/160001/chromecast/graphics/ca...
chromecast/graphics/cast_focus_client_aura.cc:201: ++iter) {
range-based?

https://codereview.chromium.org/2636303002/diff/160001/chromecast/graphics/ca...
File chromecast/graphics/cast_focus_client_aura.h (right):

https://codereview.chromium.org/2636303002/diff/160001/chromecast/graphics/ca...
chromecast/graphics/cast_focus_client_aura.h:28: void reset();
naming nit: Reset
And is it intended as a public API?

https://codereview.chromium.org/2636303002/diff/160001/chromecast/graphics/ca...
File chromecast/graphics/cast_window_manager_aura.cc (right):

https://codereview.chromium.org/2636303002/diff/160001/chromecast/graphics/ca...
chromecast/graphics/cast_window_manager_aura.cc:155: focus_client_.reset();
Can you explain why you needed this?

https://codereview.chromium.org/2636303002/diff/160001/chromecast/graphics/ca...
File chromecast/graphics/cast_window_manager_aura.h (right):

https://codereview.chromium.org/2636303002/diff/160001/chromecast/graphics/ca...
chromecast/graphics/cast_window_manager_aura.h:23: class CastFocusClientAura;
nit, unnecessary

https://codereview.chromium.org/2636303002/diff/160001/chromecast/graphics/ca...
File chromecast/graphics/cast_window_manager_aura_test.cc (right):

https://codereview.chromium.org/2636303002/diff/160001/chromecast/graphics/ca...
chromecast/graphics/cast_window_manager_aura_test.cc:29: window->Show();
What's this test checking? :)

[Joshua LeVasseur, 2017-01-24 03:39:12]

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

[commit-bot: I haz the power, 2017-01-24 03:39:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Joshua LeVasseur, 2017-01-24 03:39:50]

https://codereview.chromium.org/2636303002/diff/160001/chromecast/graphics/BU...
File chromecast/graphics/BUILD.gn (right):

https://codereview.chromium.org/2636303002/diff/160001/chromecast/graphics/BU...
chromecast/graphics/BUILD.gn:74: "cast_window_manager_aura_test.cc",
On 2017/01/24 00:54:46, halliwell wrote:
> Do these need guarding (e.g. for Android builds)

Done.

https://codereview.chromium.org/2636303002/diff/160001/chromecast/graphics/ca...
File chromecast/graphics/cast_focus_client_aura.cc (right):

https://codereview.chromium.org/2636303002/diff/160001/chromecast/graphics/ca...
chromecast/graphics/cast_focus_client_aura.cc:25: ++it) {
On 2017/01/24 00:54:46, halliwell wrote:
> minor: can this be a range-based loop?

Done.

https://codereview.chromium.org/2636303002/diff/160001/chromecast/graphics/ca...
chromecast/graphics/cast_focus_client_aura.cc:201: ++iter) {
On 2017/01/24 00:54:46, halliwell wrote:
> range-based?

Done.

https://codereview.chromium.org/2636303002/diff/160001/chromecast/graphics/ca...
File chromecast/graphics/cast_focus_client_aura.h (right):

https://codereview.chromium.org/2636303002/diff/160001/chromecast/graphics/ca...
chromecast/graphics/cast_focus_client_aura.h:28: void reset();
On 2017/01/24 00:54:46, halliwell wrote:
> naming nit: Reset
> And is it intended as a public API?

Fixed, and switched to private.

https://codereview.chromium.org/2636303002/diff/160001/chromecast/graphics/ca...
File chromecast/graphics/cast_window_manager_aura.cc (right):

https://codereview.chromium.org/2636303002/diff/160001/chromecast/graphics/ca...
chromecast/graphics/cast_window_manager_aura.cc:155: focus_client_.reset();
On 2017/01/24 00:54:46, halliwell wrote:
> Can you explain why you needed this?

I've switched to using a smart pointer, so that should make it clearer (ensure
that we discard state).

https://codereview.chromium.org/2636303002/diff/160001/chromecast/graphics/ca...
File chromecast/graphics/cast_window_manager_aura.h (right):

https://codereview.chromium.org/2636303002/diff/160001/chromecast/graphics/ca...
chromecast/graphics/cast_window_manager_aura.h:23: class CastFocusClientAura;
On 2017/01/24 00:54:46, halliwell wrote:
> nit, unnecessary

I keep this and toss the include file above.

https://codereview.chromium.org/2636303002/diff/160001/chromecast/graphics/ca...
File chromecast/graphics/cast_window_manager_aura_test.cc (right):

https://codereview.chromium.org/2636303002/diff/160001/chromecast/graphics/ca...
chromecast/graphics/cast_window_manager_aura_test.cc:29: window->Show();
On 2017/01/24 00:54:46, halliwell wrote:
> What's this test checking? :)

I'd like to check that it displays the window, but I don't know how to do that
yet.  Right now I just want to ensure that we can build and compile and run the
basics of our windowing system in the unit tests.

[halliwell, 2017-01-24 04:10:12]

lgtm :)

https://codereview.chromium.org/2636303002/diff/160001/chromecast/graphics/ca...
File chromecast/graphics/cast_window_manager_aura_test.cc (right):

https://codereview.chromium.org/2636303002/diff/160001/chromecast/graphics/ca...
chromecast/graphics/cast_window_manager_aura_test.cc:29: window->Show();
On 2017/01/24 03:39:50, Joshua LeVasseur wrote:
> On 2017/01/24 00:54:46, halliwell wrote:
> > What's this test checking? :)
> 
> I'd like to check that it displays the window, but I don't know how to do that
> yet.  Right now I just want to ensure that we can build and compile and run
the
> basics of our windowing system in the unit tests.

Fair enough.  Maybe can find some useful assertions to make in other aura tests?

[commit-bot: I haz the power, 2017-01-24 05:13:29]

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

[commit-bot: I haz the power, 2017-01-24 05:13:30]

Dry run: This issue passed the CQ dry run.

[Joshua LeVasseur, 2017-01-24 21:40:17]

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

[commit-bot: I haz the power, 2017-01-24 21:40:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Joshua LeVasseur, 2017-01-24 21:41:18]

On 2017/01/24 04:10:12, halliwell wrote:
> lgtm :)
> 
>
https://codereview.chromium.org/2636303002/diff/160001/chromecast/graphics/ca...
> File chromecast/graphics/cast_window_manager_aura_test.cc (right):
> 
>
https://codereview.chromium.org/2636303002/diff/160001/chromecast/graphics/ca...
> chromecast/graphics/cast_window_manager_aura_test.cc:29: window->Show();
> On 2017/01/24 03:39:50, Joshua LeVasseur wrote:
> > On 2017/01/24 00:54:46, halliwell wrote:
> > > What's this test checking? :)
> > 
> > I'd like to check that it displays the window, but I don't know how to do
that
> > yet.  Right now I just want to ensure that we can build and compile and run
> the
> > basics of our windowing system in the unit tests.
> 
> Fair enough.  Maybe can find some useful assertions to make in other aura
tests?

I've implemented better tests, and realized that I didn't completely ignore
input events on release builds, so added code to definitely drop events for
release builds (with a test for this too).

[commit-bot: I haz the power, 2017-01-24 23:06:17]

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

[commit-bot: I haz the power, 2017-01-24 23:06:18]

Dry run: This issue passed the CQ dry run.

[Joshua LeVasseur, 2017-01-25 18:37:52]

Description was changed from

==========
[Chromecast] Add support for z-order and window focus.

BUG=internal b/33046596, internal b/33047358, internal b/34103918
TEST=keyboard input should work in a window with google.com

Change-Id: I01d9ae64d3175298d439577e4c83bea4ada6203f
==========

to

==========
[Chromecast] Add support for z-order and window focus.

BUG=internal b/33046596, internal b/33047358, internal b/34103918
TEST=keyboard input should work in a window with google.com

Change-Id: I01d9ae64d3175298d439577e4c83bea4ada6203f
==========

[Joshua LeVasseur, 2017-01-25 18:37:52]

jlevasseur@chromium.org changed reviewers:
+ jbauman@chromium.org, sadrul@chromium.org

[Joshua LeVasseur, 2017-01-25 18:38:45]

This adds a dependency on ui/gl/test and ui/base

[sadrul, 2017-01-26 01:50:11]

https://codereview.chromium.org/2636303002/diff/200001/chromecast/graphics/ca...
File chromecast/graphics/cast_focus_client_aura.cc (right):

https://codereview.chromium.org/2636303002/diff/200001/chromecast/graphics/ca...
chromecast/graphics/cast_focus_client_aura.cc:58: LOG(INFO) << "Removing window,
" << LOG_WINDOW_INFO(top_level, window);
Should these be DLOG?

https://codereview.chromium.org/2636303002/diff/200001/chromecast/graphics/ca...
chromecast/graphics/cast_focus_client_aura.cc:215: return window;
Can you just use window->GetRootWindow(), instead of using GetTopLevelWindow()?
The only difference is that GetRootWindow() can return null if it's not attached
to a host, but from the uses of GetTopLevelWindow(), it doesn't look like that
would be a problem.

https://codereview.chromium.org/2636303002/diff/200001/chromecast/graphics/ca...
File chromecast/graphics/cast_window_manager_aura.cc (right):

https://codereview.chromium.org/2636303002/diff/200001/chromecast/graphics/ca...
chromecast/graphics/cast_window_manager_aura.cc:209: for (auto&& other :
windows) {
auto* other : windows

https://codereview.chromium.org/2636303002/diff/200001/chromecast/graphics/ca...
chromecast/graphics/cast_window_manager_aura.cc:216: (!above || other->id() <
above->id())) {
Where in the code do you assign the window ids? I assume you want to use
CastWindowManager::WindowId, but I couldn't find where it's set from a quick
look.

Note that aura only sets the default id to all windows during creation, and it
is upto the client to explicitly set a different id (if desired). So unless some
code in chromecast is setting the id, this wouldn't work very well.

https://codereview.chromium.org/2636303002/diff/200001/chromecast/graphics/ca...
File chromecast/graphics/cast_window_manager_aura_test.cc (right):

https://codereview.chromium.org/2636303002/diff/200001/chromecast/graphics/ca...
chromecast/graphics/cast_window_manager_aura_test.cc:66:
base::RunLoop().RunUntilIdle();
Do you need to spin the message-loop here?

https://codereview.chromium.org/2636303002/diff/200001/chromecast/graphics/ca...
chromecast/graphics/cast_window_manager_aura_test.cc:102:
base::RunLoop().RunUntilIdle();
ditto

[Joshua LeVasseur, 2017-01-26 02:51:01]

https://codereview.chromium.org/2636303002/diff/200001/chromecast/graphics/ca...
File chromecast/graphics/cast_focus_client_aura.cc (right):

https://codereview.chromium.org/2636303002/diff/200001/chromecast/graphics/ca...
chromecast/graphics/cast_focus_client_aura.cc:58: LOG(INFO) << "Removing window,
" << LOG_WINDOW_INFO(top_level, window);
On 2017/01/26 01:50:10, sadrul wrote:
> Should these be DLOG?

Done.

https://codereview.chromium.org/2636303002/diff/200001/chromecast/graphics/ca...
chromecast/graphics/cast_focus_client_aura.cc:215: return window;
On 2017/01/26 01:50:10, sadrul wrote:
> Can you just use window->GetRootWindow(), instead of using
GetTopLevelWindow()?
> The only difference is that GetRootWindow() can return null if it's not
attached
> to a host, but from the uses of GetTopLevelWindow(), it doesn't look like that
> would be a problem.

I want to get a child window of the root window (where the root window is the
one with the host).  The child windows will have different IDs specifying their
z-order.  I've updated the comment to try to separate this from the root window.

https://codereview.chromium.org/2636303002/diff/200001/chromecast/graphics/ca...
File chromecast/graphics/cast_window_manager_aura.cc (right):

https://codereview.chromium.org/2636303002/diff/200001/chromecast/graphics/ca...
chromecast/graphics/cast_window_manager_aura.cc:209: for (auto&& other :
windows) {
On 2017/01/26 01:50:10, sadrul wrote:
> auto* other : windows

Done.

https://codereview.chromium.org/2636303002/diff/200001/chromecast/graphics/ca...
chromecast/graphics/cast_window_manager_aura.cc:216: (!above || other->id() <
above->id())) {
On 2017/01/26 01:50:10, sadrul wrote:
> Where in the code do you assign the window ids? I assume you want to use
> CastWindowManager::WindowId, but I couldn't find where it's set from a quick
> look.
> 
> Note that aura only sets the default id to all windows during creation, and it
> is upto the client to explicitly set a different id (if desired). So unless
some
> code in chromecast is setting the id, this wouldn't work very well.

Some future code changes, which will bring in additional windows, will assign
the IDs.  I've tried to make the code handle the default ID gracefully, so that
it isn't necessary to assign an ID for the bottom-most (default position)
windows.  If an overlay is desired, then the ID will be necessary.  If the
aura::Window::kInitialId ever changes, then things will stop working, so I've
added another unit test that ensures that the BOTTOM window ID always matches
the default aura::Window::kInitialId.  Note that cast_window_manager.h, which
declares the window IDs, can't have a dependency on ui/aura/window.h, because
cast_window_manager.h can be used in code that doesn't use Aura (e.g., Android).
 But the Aura-based code is the only code concerned with z-order at the moment.

https://codereview.chromium.org/2636303002/diff/200001/chromecast/graphics/ca...
File chromecast/graphics/cast_window_manager_aura_test.cc (right):

https://codereview.chromium.org/2636303002/diff/200001/chromecast/graphics/ca...
chromecast/graphics/cast_window_manager_aura_test.cc:66:
base::RunLoop().RunUntilIdle();
On 2017/01/26 01:50:10, sadrul wrote:
> Do you need to spin the message-loop here?

The event-dispatch logic posts tasks, so this is necessary (as far as I can
tell).  If I remove it, the unit test will fail with tasks left over on the run
queue.

https://codereview.chromium.org/2636303002/diff/200001/chromecast/graphics/ca...
chromecast/graphics/cast_window_manager_aura_test.cc:102:
base::RunLoop().RunUntilIdle();
On 2017/01/26 01:50:10, sadrul wrote:
> ditto

Acknowledged.

[Joshua LeVasseur, 2017-01-26 18:42:44]

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

[commit-bot: I haz the power, 2017-01-26 18:43:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-26 20:39:45]

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

[commit-bot: I haz the power, 2017-01-26 20:39:45]

Dry run: This issue passed the CQ dry run.

[sadrul, 2017-01-27 00:30:57]

A couple of suggestions. lgtm

https://codereview.chromium.org/2636303002/diff/200001/chromecast/graphics/ca...
File chromecast/graphics/cast_focus_client_aura.cc (right):

https://codereview.chromium.org/2636303002/diff/200001/chromecast/graphics/ca...
chromecast/graphics/cast_focus_client_aura.cc:215: return window;
On 2017/01/26 02:51:00, Joshua LeVasseur wrote:
> On 2017/01/26 01:50:10, sadrul wrote:
> > Can you just use window->GetRootWindow(), instead of using
> GetTopLevelWindow()?
> > The only difference is that GetRootWindow() can return null if it's not
> attached
> > to a host, but from the uses of GetTopLevelWindow(), it doesn't look like
that
> > would be a problem.
> 
> I want to get a child window of the root window (where the root window is the
> one with the host).  The child windows will have different IDs specifying
their
> z-order.  I've updated the comment to try to separate this from the root
window.

OK. Note that there is also Window::GetToplevelWindow(), although not sure if
that will work for you (it returns the higher-most ancestor with a non-null
delegate).

https://codereview.chromium.org/2636303002/diff/200001/chromecast/graphics/ca...
File chromecast/graphics/cast_window_manager_aura_test.cc (right):

https://codereview.chromium.org/2636303002/diff/200001/chromecast/graphics/ca...
chromecast/graphics/cast_window_manager_aura_test.cc:66:
base::RunLoop().RunUntilIdle();
On 2017/01/26 02:51:01, Joshua LeVasseur wrote:
> On 2017/01/26 01:50:10, sadrul wrote:
> > Do you need to spin the message-loop here?
> 
> The event-dispatch logic posts tasks, so this is necessary (as far as I can
> tell).  If I remove it, the unit test will fail with tasks left over on the
run
> queue.

Ohh, you are using UIControlsAura. That's why.

Please switch to use ui::test::EventGenerator[1] instead. That is much simpler,
and does not need to cycle message-loop.

1:
https://cs.chromium.org/chromium/src/ui/events/test/event_generator.h?sq=pack...

[jbauman, 2017-01-27 00:43:32]

ui/gl/test DEPS lgtm.

[Joshua LeVasseur, 2017-01-27 04:50:28]

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

[commit-bot: I haz the power, 2017-01-27 04:50:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Joshua LeVasseur, 2017-01-27 04:53:44]

https://codereview.chromium.org/2636303002/diff/200001/chromecast/graphics/ca...
File chromecast/graphics/cast_focus_client_aura.cc (right):

https://codereview.chromium.org/2636303002/diff/200001/chromecast/graphics/ca...
chromecast/graphics/cast_focus_client_aura.cc:215: return window;
On 2017/01/27 00:30:56, sadrul wrote:
> On 2017/01/26 02:51:00, Joshua LeVasseur wrote:
> > On 2017/01/26 01:50:10, sadrul wrote:
> > > Can you just use window->GetRootWindow(), instead of using
> > GetTopLevelWindow()?
> > > The only difference is that GetRootWindow() can return null if it's not
> > attached
> > > to a host, but from the uses of GetTopLevelWindow(), it doesn't look like
> that
> > > would be a problem.
> > 
> > I want to get a child window of the root window (where the root window is
the
> > one with the host).  The child windows will have different IDs specifying
> their
> > z-order.  I've updated the comment to try to separate this from the root
> window.
> 
> OK. Note that there is also Window::GetToplevelWindow(), although not sure if
> that will work for you (it returns the higher-most ancestor with a non-null
> delegate).

To avoid conflict with existing names, I've renamed it to GetZOrderWindow()

https://codereview.chromium.org/2636303002/diff/200001/chromecast/graphics/ca...
File chromecast/graphics/cast_window_manager_aura_test.cc (right):

https://codereview.chromium.org/2636303002/diff/200001/chromecast/graphics/ca...
chromecast/graphics/cast_window_manager_aura_test.cc:66:
base::RunLoop().RunUntilIdle();
On 2017/01/27 00:30:56, sadrul wrote:
> On 2017/01/26 02:51:01, Joshua LeVasseur wrote:
> > On 2017/01/26 01:50:10, sadrul wrote:
> > > Do you need to spin the message-loop here?
> > 
> > The event-dispatch logic posts tasks, so this is necessary (as far as I can
> > tell).  If I remove it, the unit test will fail with tasks left over on the
> run
> > queue.
> 
> Ohh, you are using UIControlsAura. That's why.
> 
> Please switch to use ui::test::EventGenerator[1] instead. That is much
simpler,
> and does not need to cycle message-loop.
> 
> 1:
>
https://cs.chromium.org/chromium/src/ui/events/test/event_generator.h?sq=pack...

Thanks!  Done.

[commit-bot: I haz the power, 2017-01-27 05:56:41]

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

[commit-bot: I haz the power, 2017-01-27 05:56:42]

Dry run: This issue passed the CQ dry run.

[Joshua LeVasseur, 2017-01-30 16:19:43]

The CQ bit was checked by jlevasseur@chromium.org

[Joshua LeVasseur, 2017-01-30 16:19:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
halliwell@chromium.org, sadrul@chromium.org, jbauman@chromium.org
Link to the patchset: https://codereview.chromium.org/2636303002/#ps260001
(title: "reviewer feedback")

[commit-bot: I haz the power, 2017-01-30 16:20:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-30 17:19:48]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1485793183737790,
"parent_rev": "ceb64f73f7bf1e1ae1914970c9adc455f564a1e9", "commit_rev":
"ff442c09047c888651b3b43a8e8d4955092d33ea"}

[commit-bot: I haz the power, 2017-01-30 17:20:20]

Description was changed from

==========
[Chromecast] Add support for z-order and window focus.

BUG=internal b/33046596, internal b/33047358, internal b/34103918
TEST=keyboard input should work in a window with google.com

Change-Id: I01d9ae64d3175298d439577e4c83bea4ada6203f
==========

to

==========
[Chromecast] Add support for z-order and window focus.

BUG=internal b/33046596, internal b/33047358, internal b/34103918
TEST=keyboard input should work in a window with google.com

Change-Id: I01d9ae64d3175298d439577e4c83bea4ada6203f
Review-Url: https://codereview.chromium.org/2636303002
Cr-Commit-Position: refs/heads/master@{#447006}
Committed:
https://chromium.googlesource.com/chromium/src/+/ff442c09047c888651b3b43a8e8d...
==========

[commit-bot: I haz the power, 2017-01-30 17:20:22]

Committed patchset #14 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/ff442c09047c888651b3b43a8e8d...