[Blimp Engine] Adds a WindowTreeClient so that popup will have non-null paren…

[Blimp Engine] Adds a WindowTreeClient so that popup such as select dropdown will have non-null parent.

After this CL, Engine no longer crashes, but tapping select box does not show dropdown because of crbug/598764.

BUG=597992
Committed: https://crrev.com/0539fc4bc55c129a632bc9b10dc36601b9fac8ad
Cr-Commit-Position: refs/heads/master@{#384027}

[haibinlu, 2016-03-29 00:26:40]

Description was changed from

==========
[Blimp Engine] Adds a WindowTreeClient so that popup such as selection box will
have non-null parent.


BUG=597992
==========

to

==========
[Blimp Engine] Adds a WindowTreeClient so that popup such as selection drop down
will have non-null parent.


BUG=597992
==========

[haibinlu, 2016-03-29 00:28:25]

Description was changed from

==========
[Blimp Engine] Adds a WindowTreeClient so that popup such as selection drop down
will have non-null parent.


BUG=597992
==========

to

==========
[Blimp Engine] Adds a WindowTreeClient so that popup such as select dropdown
will have non-null parent.


BUG=597992
==========

[haibinlu, 2016-03-29 17:46:40]

Description was changed from

==========
[Blimp Engine] Adds a WindowTreeClient so that popup such as select dropdown
will have non-null parent.


BUG=597992
==========

to

==========
[Blimp Engine] Adds a WindowTreeClient so that popup such as select dropdown
will have non-null parent.

After this CL, Engine no longer crashes, but tapping select box does not show
dropdown because of crbug/598764.

BUG=597992
==========

[haibinlu, 2016-03-29 17:46:41]

haibinlu@chromium.org changed reviewers:
+ wez@chromium.org

[Wez, 2016-03-29 21:20:06]

https://codereview.chromium.org/1842593002/diff/20001/blimp/engine/app/ui/bli...
File blimp/engine/app/ui/blimp_window_tree_client.cc (right):

https://codereview.chromium.org/1842593002/diff/20001/blimp/engine/app/ui/bli...
blimp/engine/app/ui/blimp_window_tree_client.cc:18:
aura::client::SetWindowTreeClient(root_window_, NULL);
NULL->nullptr?

https://codereview.chromium.org/1842593002/diff/20001/blimp/engine/app/ui/bli...
blimp/engine/app/ui/blimp_window_tree_client.cc:24: return root_window_;
Add a brief comment explaining why we use |root_window_| and ignore |context|
and |window| - should we be checking anything about the parameters?

https://codereview.chromium.org/1842593002/diff/20001/blimp/engine/app/ui/bli...
File blimp/engine/app/ui/blimp_window_tree_client.h (right):

https://codereview.chromium.org/1842593002/diff/20001/blimp/engine/app/ui/bli...
blimp/engine/app/ui/blimp_window_tree_client.h:16: explicit
BlimpWindowTreeClient(aura::Window* root_window);
Add a brief comment explaining the lifetime expectation for |root_window|, since
it is a bare pointer.

https://codereview.chromium.org/1842593002/diff/20001/blimp/engine/app/ui/bli...
blimp/engine/app/ui/blimp_window_tree_client.h:19: // Overridden from
aura::client::WindowTreeClient.
Conventionally we use the form

// aura::client::WindowTreeClient interface.

https://codereview.chromium.org/1842593002/diff/20001/blimp/engine/session/bl...
File blimp/engine/session/blimp_engine_session.h (right):

https://codereview.chromium.org/1842593002/diff/20001/blimp/engine/session/bl...
blimp/engine/session/blimp_engine_session.h:185: // Used to locate null-parented
windows, such as a popup.
"locate" is ambiguous; I think you mean "locate in the window hierarchy"?

Suggest: "Used to attach null-parented windows (e.g. popups) to the root
window."

[haibinlu, 2016-03-29 21:57:16]

PTAL

https://codereview.chromium.org/1842593002/diff/20001/blimp/engine/app/ui/bli...
File blimp/engine/app/ui/blimp_window_tree_client.cc (right):

https://codereview.chromium.org/1842593002/diff/20001/blimp/engine/app/ui/bli...
blimp/engine/app/ui/blimp_window_tree_client.cc:18:
aura::client::SetWindowTreeClient(root_window_, NULL);
On 2016/03/29 21:20:06, Wez wrote:
> NULL->nullptr?

Done.

https://codereview.chromium.org/1842593002/diff/20001/blimp/engine/app/ui/bli...
blimp/engine/app/ui/blimp_window_tree_client.cc:24: return root_window_;
On 2016/03/29 21:20:06, Wez wrote:
> Add a brief comment explaining why we use |root_window_| and ignore |context|
> and |window| - should we be checking anything about the parameters?

Added comment. 

Per window_tree_client comments, context and bounds are planed to be removed. //
TODO(erg): Remove |context|, and maybe after oshima's patch lands, |bounds|.

https://codereview.chromium.org/1842593002/diff/20001/blimp/engine/app/ui/bli...
File blimp/engine/app/ui/blimp_window_tree_client.h (right):

https://codereview.chromium.org/1842593002/diff/20001/blimp/engine/app/ui/bli...
blimp/engine/app/ui/blimp_window_tree_client.h:16: explicit
BlimpWindowTreeClient(aura::Window* root_window);
On 2016/03/29 21:20:06, Wez wrote:
> Add a brief comment explaining the lifetime expectation for |root_window|,
since
> it is a bare pointer.

Done.

https://codereview.chromium.org/1842593002/diff/20001/blimp/engine/app/ui/bli...
blimp/engine/app/ui/blimp_window_tree_client.h:19: // Overridden from
aura::client::WindowTreeClient.
On 2016/03/29 21:20:06, Wez wrote:
> Conventionally we use the form
> 
> // aura::client::WindowTreeClient interface.

Done.

https://codereview.chromium.org/1842593002/diff/20001/blimp/engine/session/bl...
File blimp/engine/session/blimp_engine_session.h (right):

https://codereview.chromium.org/1842593002/diff/20001/blimp/engine/session/bl...
blimp/engine/session/blimp_engine_session.h:185: // Used to locate null-parented
windows, such as a popup.
On 2016/03/29 21:20:06, Wez wrote:
> "locate" is ambiguous; I think you mean "locate in the window hierarchy"?
> 
> Suggest: "Used to attach null-parented windows (e.g. popups) to the root
> window."

Done.

[Wez, 2016-03-30 00:07:56]

lgtm

https://codereview.chromium.org/1842593002/diff/40001/blimp/engine/app/ui/bli...
File blimp/engine/app/ui/blimp_window_tree_client.cc (right):

https://codereview.chromium.org/1842593002/diff/40001/blimp/engine/app/ui/bli...
blimp/engine/app/ui/blimp_window_tree_client.cc:24: // Blimp has one root
window. a null-parent window always attaches to it.
nit: Second sentence should start with a capital, or alternatively:

Suggest: "Always attach null-parented windows to the root window."

[haibinlu, 2016-03-30 01:40:07]

https://codereview.chromium.org/1842593002/diff/40001/blimp/engine/app/ui/bli...
File blimp/engine/app/ui/blimp_window_tree_client.cc (right):

https://codereview.chromium.org/1842593002/diff/40001/blimp/engine/app/ui/bli...
blimp/engine/app/ui/blimp_window_tree_client.cc:24: // Blimp has one root
window. a null-parent window always attaches to it.
On 2016/03/30 00:07:55, Wez wrote:
> nit: Second sentence should start with a capital, or alternatively:
> 
> Suggest: "Always attach null-parented windows to the root window."

Done.

[haibinlu, 2016-03-30 01:40:15]

The CQ bit was checked by haibinlu@chromium.org

[haibinlu, 2016-03-30 01:40:15]

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

[commit-bot: I haz the power, 2016-03-30 01:40:30]

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

[commit-bot: I haz the power, 2016-03-30 07:49:55]

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

[commit-bot: I haz the power, 2016-03-30 07:49:56]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[haibinlu, 2016-03-30 17:16:56]

The CQ bit was checked by haibinlu@chromium.org

[commit-bot: I haz the power, 2016-03-30 17:17:07]

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

[commit-bot: I haz the power, 2016-03-30 18:25:41]

Description was changed from

==========
[Blimp Engine] Adds a WindowTreeClient so that popup such as select dropdown
will have non-null parent.

After this CL, Engine no longer crashes, but tapping select box does not show
dropdown because of crbug/598764.

BUG=597992
==========

to

==========
[Blimp Engine] Adds a WindowTreeClient so that popup such as select dropdown
will have non-null parent.

After this CL, Engine no longer crashes, but tapping select box does not show
dropdown because of crbug/598764.

BUG=597992
==========

[commit-bot: I haz the power, 2016-03-30 18:25:42]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-30 18:26:44]

Description was changed from

==========
[Blimp Engine] Adds a WindowTreeClient so that popup such as select dropdown
will have non-null parent.

After this CL, Engine no longer crashes, but tapping select box does not show
dropdown because of crbug/598764.

BUG=597992
==========

to

==========
[Blimp Engine] Adds a WindowTreeClient so that popup such as select dropdown
will have non-null parent.

After this CL, Engine no longer crashes, but tapping select box does not show
dropdown because of crbug/598764.

BUG=597992

Committed: https://crrev.com/0539fc4bc55c129a632bc9b10dc36601b9fac8ad
Cr-Commit-Position: refs/heads/master@{#384027}
==========

[commit-bot: I haz the power, 2016-03-30 18:26:45]

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