Converts WindowTreeClientTest to be in terms of aura

Converts WindowTreeClientTest to be in terms of aura

Only real interesting change is
WindowTreeClient::OnWindowDeleted(). If a root is deleted this now
calls to the delegate to handle deletion. This is necessary as
generally the Window shouldn't be deleted directly, rather the
WindowTreeHostMus which owns the window.

I also changed Embed() to fail immediately if the window has children. The
server has logic to force removal, but I think we should treat it as
an error in client code if they try to embed and there are children.

BUG=664621
TEST=covered by tests
R=msw@chromium.org

Committed: https://crrev.com/247a606263c48168e56020f845df0b3c41857e14
Cr-Commit-Position: refs/heads/master@{#437264}

[sky, 2016-12-08 00:24:08]

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

[commit-bot: I haz the power, 2016-12-08 00:24:40]

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

[msw, 2016-12-08 01:20:12]

As with other services/ui patches, there are some details that escape me here,
but this mostly seems alright. lgtm with minor nits/qs.

https://codereview.chromium.org/2559703003/diff/20001/services/ui/ws/window_m...
File services/ui/ws/window_manager_client_unittest.cc (right):

https://codereview.chromium.org/2559703003/diff/20001/services/ui/ws/window_m...
services/ui/ws/window_manager_client_unittest.cc:136: for (auto it =
window->children().begin(); it != window->children().end();
optional nit: range-based for loop?

https://codereview.chromium.org/2559703003/diff/20001/services/ui/ws/window_m...
services/ui/ws/window_manager_client_unittest.cc:246: aura::Window* window;
nit: = nullptr

https://codereview.chromium.org/2559703003/diff/20001/services/ui/ws/window_m...
services/ui/ws/window_manager_client_unittest.cc:265: if
(embed_details_->callback_run) {
nit: maybe explain why checking if the callback has been run matters here?
I'm not sure that I follow the pattern used here, or these checks.

https://codereview.chromium.org/2559703003/diff/20001/services/ui/ws/window_m...
services/ui/ws/window_manager_client_unittest.cc:287:
std::unique_ptr<ClientAreaChange> WaitForClientAreaToChange() {
nit: maybe comment on why the pattern is different here from
WaitForBoundsToChange, etc.?

https://codereview.chromium.org/2559703003/diff/20001/services/ui/ws/window_m...
services/ui/ws/window_manager_client_unittest.cc:363: aura::Window*
window_in_embedded =
nit: explain why this is different than the |window_in_embedded| on line 377
(and others), maybe rename this or others to clarify the distinction? (or check
that the two values are the same here?)

https://codereview.chromium.org/2559703003/diff/20001/services/ui/ws/window_m...
services/ui/ws/window_manager_client_unittest.cc:470: aura::Window*
root_in_embedded = embed_result->window_tree_host->window();
optional nit: name window_in_embedded like above?

https://codereview.chromium.org/2559703003/diff/20001/services/ui/ws/window_m...
services/ui/ws/window_manager_client_unittest.cc:547: aura::Window*
embedded_root = embed_result->window_tree_host->window();
ditto optional nit: name window_in_embedded like above? (I'd say that's not the
clearest name, so if you settle on another clearer name for all/most instances,
that'd be good too)

https://codereview.chromium.org/2559703003/diff/20001/services/ui/ws/window_m...
services/ui/ws/window_manager_client_unittest.cc:584: NewVisibleWindow(window1,
window_manager());
nit: check the return value? name child or similar?

https://codereview.chromium.org/2559703003/diff/20001/services/ui/ws/window_m...
services/ui/ws/window_manager_client_unittest.cc:620: aura::Window* embed_window
=
nit: try to use consistent naming throughout

https://codereview.chromium.org/2559703003/diff/20001/services/ui/ws/window_s...
File services/ui/ws/window_server_test_base.cc (right):

https://codereview.chromium.org/2559703003/diff/20001/services/ui/ws/window_s...
services/ui/ws/window_server_test_base.cc:41:
display::Screen::SetScreenInstance(nullptr);
Is this needed here and in TearDown? Ditto for window_tree_clients_.clear();

https://codereview.chromium.org/2559703003/diff/20001/services/ui/ws/window_s...
services/ui/ws/window_server_test_base.cc:110:
ASSERT_TRUE(DoRunLoopWithTimeout());  // RunLoop should be quit by OnEmbed().
nit: maybe explain why OnEmbed is called? (eg. it's embedding
|window_manager_window_tree_client|?)

https://codereview.chromium.org/2559703003/diff/20001/services/ui/ws/window_s...
services/ui/ws/window_server_test_base.cc:142: void
WindowServerTestBase::OnEmbedRootDestroyed(
optional nit: share code with WindowServerTestBase::OnWmDisplayRemoved?

https://codereview.chromium.org/2559703003/diff/20001/services/ui/ws/window_s...
services/ui/ws/window_server_test_base.cc:270: const std::vector<gfx::Rect>&
additional_client_areas) {}
Should this also call through to the delegate?

https://codereview.chromium.org/2559703003/diff/20001/services/ui/ws/window_s...
File services/ui/ws/window_server_test_base.h (right):

https://codereview.chromium.org/2559703003/diff/20001/services/ui/ws/window_s...
services/ui/ws/window_server_test_base.h:69:
std::unique_ptr<aura::WindowTreeClient> ReleaseMostRecentClient();
nit: comment

https://codereview.chromium.org/2559703003/diff/20001/ui/aura/mus/window_tree...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2559703003/diff/20001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:267: if (!window->children().empty()) {
nit: add a comment, explaining why only one child is allowed?

https://codereview.chromium.org/2559703003/diff/20001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:1010: if (roots_.count(window)) {
nit: add a comment explaining the two paths here

https://codereview.chromium.org/2559703003/diff/20001/ui/views/mus/desktop_wi...
File ui/views/mus/desktop_window_tree_host_mus.h (right):

https://codereview.chromium.org/2559703003/diff/20001/ui/views/mus/desktop_wi...
ui/views/mus/desktop_window_tree_host_mus.h:37: void ServerDestroyedWindow() {
CloseNow(); }
nit: move definition to cc file? Add a comment?

[commit-bot: I haz the power, 2016-12-08 01:51:21]

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

[commit-bot: I haz the power, 2016-12-08 01:51:22]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[sky, 2016-12-08 04:59:45]

Also, I nuked all the focus related tests as focus is much different in this
aura-mus case. The only way to get the focus tests to work would be to change
WindowTreeclient to have a single FocusClient, which is problematic. If this
becomes important in the future I will revive the tests.

https://codereview.chromium.org/2559703003/diff/20001/services/ui/ws/window_m...
File services/ui/ws/window_manager_client_unittest.cc (right):

https://codereview.chromium.org/2559703003/diff/20001/services/ui/ws/window_m...
services/ui/ws/window_manager_client_unittest.cc:136: for (auto it =
window->children().begin(); it != window->children().end();
On 2016/12/08 01:20:11, msw wrote:
> optional nit: range-based for loop?

Done.

https://codereview.chromium.org/2559703003/diff/20001/services/ui/ws/window_m...
services/ui/ws/window_manager_client_unittest.cc:246: aura::Window* window;
On 2016/12/08 01:20:11, msw wrote:
> nit: = nullptr

Done.

https://codereview.chromium.org/2559703003/diff/20001/services/ui/ws/window_m...
services/ui/ws/window_manager_client_unittest.cc:265: if
(embed_details_->callback_run) {
On 2016/12/08 01:20:11, msw wrote:
> nit: maybe explain why checking if the callback has been run matters here?
> I'm not sure that I follow the pattern used here, or these checks.

I added more docs here.

https://codereview.chromium.org/2559703003/diff/20001/services/ui/ws/window_m...
services/ui/ws/window_manager_client_unittest.cc:287:
std::unique_ptr<ClientAreaChange> WaitForClientAreaToChange() {
On 2016/12/08 01:20:11, msw wrote:
> nit: maybe comment on why the pattern is different here from
> WaitForBoundsToChange, etc.?

Done.

https://codereview.chromium.org/2559703003/diff/20001/services/ui/ws/window_m...
services/ui/ws/window_manager_client_unittest.cc:363: aura::Window*
window_in_embedded =
On 2016/12/08 01:20:11, msw wrote:
> nit: explain why this is different than the |window_in_embedded| on line 377
> (and others), maybe rename this or others to clarify the distinction? (or
check
> that the two values are the same here?)

They are the same. I added a couple more assertions.

https://codereview.chromium.org/2559703003/diff/20001/services/ui/ws/window_m...
services/ui/ws/window_manager_client_unittest.cc:470: aura::Window*
root_in_embedded = embed_result->window_tree_host->window();
On 2016/12/08 01:20:11, msw wrote:
> optional nit: name window_in_embedded like above?

Done.

https://codereview.chromium.org/2559703003/diff/20001/services/ui/ws/window_m...
services/ui/ws/window_manager_client_unittest.cc:547: aura::Window*
embedded_root = embed_result->window_tree_host->window();
On 2016/12/08 01:20:11, msw wrote:
> ditto optional nit: name window_in_embedded like above? (I'd say that's not
the
> clearest name, so if you settle on another clearer name for all/most
instances,
> that'd be good too)

I went with embed_root every where, but I'm open to a different name.

https://codereview.chromium.org/2559703003/diff/20001/services/ui/ws/window_m...
services/ui/ws/window_manager_client_unittest.cc:584: NewVisibleWindow(window1,
window_manager());
On 2016/12/08 01:20:11, msw wrote:
> nit: check the return value? name child or similar?

I had it named, but the compiler complains as it isn't used. I've changed to
ASSERT.

https://codereview.chromium.org/2559703003/diff/20001/services/ui/ws/window_m...
services/ui/ws/window_manager_client_unittest.cc:620: aura::Window* embed_window
=
On 2016/12/08 01:20:11, msw wrote:
> nit: try to use consistent naming throughout

Done (hopefully).

https://codereview.chromium.org/2559703003/diff/20001/services/ui/ws/window_s...
File services/ui/ws/window_server_test_base.cc (right):

https://codereview.chromium.org/2559703003/diff/20001/services/ui/ws/window_s...
services/ui/ws/window_server_test_base.cc:41:
display::Screen::SetScreenInstance(nullptr);
On 2016/12/08 01:20:11, msw wrote:
> Is this needed here and in TearDown? Ditto for window_tree_clients_.clear();

Nope. Done.

https://codereview.chromium.org/2559703003/diff/20001/services/ui/ws/window_s...
services/ui/ws/window_server_test_base.cc:110:
ASSERT_TRUE(DoRunLoopWithTimeout());  // RunLoop should be quit by OnEmbed().
On 2016/12/08 01:20:11, msw wrote:
> nit: maybe explain why OnEmbed is called? (eg. it's embedding
> |window_manager_window_tree_client|?)

Done.

https://codereview.chromium.org/2559703003/diff/20001/services/ui/ws/window_s...
services/ui/ws/window_server_test_base.cc:142: void
WindowServerTestBase::OnEmbedRootDestroyed(
On 2016/12/08 01:20:11, msw wrote:
> optional nit: share code with WindowServerTestBase::OnWmDisplayRemoved?

Done.

https://codereview.chromium.org/2559703003/diff/20001/services/ui/ws/window_s...
services/ui/ws/window_server_test_base.cc:270: const std::vector<gfx::Rect>&
additional_client_areas) {}
On 2016/12/08 01:20:11, msw wrote:
> Should this also call through to the delegate?

Done.

https://codereview.chromium.org/2559703003/diff/20001/services/ui/ws/window_s...
File services/ui/ws/window_server_test_base.h (right):

https://codereview.chromium.org/2559703003/diff/20001/services/ui/ws/window_s...
services/ui/ws/window_server_test_base.h:69:
std::unique_ptr<aura::WindowTreeClient> ReleaseMostRecentClient();
On 2016/12/08 01:20:12, msw wrote:
> nit: comment

Done.

https://codereview.chromium.org/2559703003/diff/20001/ui/aura/mus/window_tree...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2559703003/diff/20001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:267: if (!window->children().empty()) {
On 2016/12/08 01:20:12, msw wrote:
> nit: add a comment, explaining why only one child is allowed?

You mean no children I believe. I added a comment.

https://codereview.chromium.org/2559703003/diff/20001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:1010: if (roots_.count(window)) {
On 2016/12/08 01:20:12, msw wrote:
> nit: add a comment explaining the two paths here

Done.

https://codereview.chromium.org/2559703003/diff/20001/ui/views/mus/desktop_wi...
File ui/views/mus/desktop_window_tree_host_mus.h (right):

https://codereview.chromium.org/2559703003/diff/20001/ui/views/mus/desktop_wi...
ui/views/mus/desktop_window_tree_host_mus.h:37: void ServerDestroyedWindow() {
CloseNow(); }
On 2016/12/08 01:20:12, msw wrote:
> nit: move definition to cc file? Add a comment?

Added comment. As this function is nothing more than calling another function my
take is the style guide allows it to be inlined.

[sky, 2016-12-08 04:59:50]

The CQ bit was checked by sky@chromium.org

[sky, 2016-12-08 04:59:51]

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

[commit-bot: I haz the power, 2016-12-08 05:00:12]

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, 2016-12-08 06:19:13]

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

[commit-bot: I haz the power, 2016-12-08 06:19:16]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[sky, 2016-12-08 16:00:30]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2016-12-08 16:00:51]

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, 2016-12-08 16:45:17]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481212830060200,
"parent_rev": "e31075c33d70951918764ae2509a57bf9fcbe28c", "commit_rev":
"75320d24a3464e6559564945b7e9b9eb14c31c14"}

[commit-bot: I haz the power, 2016-12-08 16:45:50]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-08 16:49:32]

Description was changed from

==========
Converts WindowTreeClientTest to be in terms of aura

Only real interesting change is
WindowTreeClient::OnWindowDeleted(). If a root is deleted this now
calls to the delegate to handle deletion. This is necessary as
generally the Window shouldn't be deleted directly, rather the
WindowTreeHostMus which owns the window.

I also changed Embed() to fail immediately if the window has children. The
server has logic to force removal, but I think we should treat it as
an error in client code if they try to embed and there are children.

BUG=664621
TEST=covered by tests
R=msw@chromium.org
==========

to

==========
Converts WindowTreeClientTest to be in terms of aura

Only real interesting change is
WindowTreeClient::OnWindowDeleted(). If a root is deleted this now
calls to the delegate to handle deletion. This is necessary as
generally the Window shouldn't be deleted directly, rather the
WindowTreeHostMus which owns the window.

I also changed Embed() to fail immediately if the window has children. The
server has logic to force removal, but I think we should treat it as
an error in client code if they try to embed and there are children.

BUG=664621
TEST=covered by tests
R=msw@chromium.org

Committed: https://crrev.com/247a606263c48168e56020f845df0b3c41857e14
Cr-Commit-Position: refs/heads/master@{#437264}
==========

[commit-bot: I haz the power, 2016-12-08 16:49:33]

Patchset 3 (id:??) landed as
https://crrev.com/247a606263c48168e56020f845df0b3c41857e14
Cr-Commit-Position: refs/heads/master@{#437264}

[msw, 2016-12-08 19:07:41]

still lgtm with minor nits for potential followup

https://codereview.chromium.org/2559703003/diff/20001/services/ui/ws/window_m...
File services/ui/ws/window_manager_client_unittest.cc (right):

https://codereview.chromium.org/2559703003/diff/20001/services/ui/ws/window_m...
services/ui/ws/window_manager_client_unittest.cc:265: if
(embed_details_->callback_run) {
On 2016/12/08 04:59:44, sky wrote:
> On 2016/12/08 01:20:11, msw wrote:
> > nit: maybe explain why checking if the callback has been run matters here?
> > I'm not sure that I follow the pattern used here, or these checks.
> 
> I added more docs here.

Much appreciated; now it makes sense.

https://codereview.chromium.org/2559703003/diff/40001/services/ui/ws/window_s...
File services/ui/ws/window_server_test_base.h (right):

https://codereview.chromium.org/2559703003/diff/40001/services/ui/ws/window_s...
services/ui/ws/window_server_test_base.h:69: // Returns the most
WindowTreeClient that was created as the result of
wow, which one is the *most WindowTreeClient*? lol, such doge grammar :)
http://weknowmemes.com/generator/uploads/generated/g1389215111816185047.jpg

https://codereview.chromium.org/2559703003/diff/40001/services/ui/ws/window_s...
services/ui/ws/window_server_test_base.h:71: // recent WindowTreeClient created
for an embed.
nit: "created as the result of a client embedding" or similar?

[sky, 2016-12-12 18:35:59]

This patch landed, but I forgot to send comments. Here they are.

https://codereview.chromium.org/2559703003/diff/40001/services/ui/ws/window_s...
File services/ui/ws/window_server_test_base.h (right):

https://codereview.chromium.org/2559703003/diff/40001/services/ui/ws/window_s...
services/ui/ws/window_server_test_base.h:69: // Returns the most
WindowTreeClient that was created as the result of
On 2016/12/08 19:07:41, msw wrote:
> wow, which one is the *most WindowTreeClient*? lol, such doge grammar :)
> http://weknowmemes.com/generator/uploads/generated/g1389215111816185047.jpg

Ha!

https://codereview.chromium.org/2559703003/diff/40001/services/ui/ws/window_s...
services/ui/ws/window_server_test_base.h:71: // recent WindowTreeClient created
for an embed.
On 2016/12/08 19:07:41, msw wrote:
> nit: "created as the result of a client embedding" or similar?

Done.