Changes ownership of WindowTreeClient

Changes ownership of WindowTreeClient

Previously WindowTreeClient had two types of ownership. It would some
times delete itself, but clients could also delete it. This is
confusing and leads to problems. This patch makes it so that
WindowTreeClient is always owned by whoever created it and never
deletes itself.

The only wrinkle is what to do when WindowTreeClient loses its
connection to mus. I made it so the delegate is told this happens, but
the internal state is not changed in anyway. I went with this in
so far as it lets the consumer of WindowTreeClient take whatever
action is appropriate. To do otherwise can often conflict with
what the client wants to do. This does mean the client can continue
creating windows and other things, but effectively nothing is
happening on the mus side (it's gone).

BUG=642182
TEST=none
R=sadrul@chromium.org
TBR=ben@chromium.org

Committed: https://crrev.com/9636beeb7afa6167b07e6afe2b463d920ebe61bc
Cr-Commit-Position: refs/heads/master@{#416670}

[sky, 2016-09-02 15:52:27]

Description was changed from

==========
Changes ownership of WindowTreeClient

Previously WindowTreeClient had two types of ownership. It would some
times delete itself, but clients could also delete it. This is
confusing and leads to problems. This patch makes it so that
WindowTreeClient is always owned by whoever created it and never
deletes itself.

The only wrinkle is what to do when WindowTreeClient loses its
connection to mus. I made it so the delegate is told this happens, but
the internal state is not changed in anyway. I went with this in
so far as it lets the consumer of WindowTreeClient take whatever
action is appropriate. To do otherwise can often conflict with
what the client wants to do. This does mean the client can continue
creating windows and other things, but effectively nothing is
happening on the mus side (it's gone).

BUG=642182
TEST=none
R=sadrul@chromium.org
==========

to

==========
Changes ownership of WindowTreeClient

Previously WindowTreeClient had two types of ownership. It would some
times delete itself, but clients could also delete it. This is
confusing and leads to problems. This patch makes it so that
WindowTreeClient is always owned by whoever created it and never
deletes itself.

The only wrinkle is what to do when WindowTreeClient loses its
connection to mus. I made it so the delegate is told this happens, but
the internal state is not changed in anyway. I went with this in
so far as it lets the consumer of WindowTreeClient take whatever
action is appropriate. To do otherwise can often conflict with
what the client wants to do. This does mean the client can continue
creating windows and other things, but effectively nothing is
happening on the mus side (it's gone).

BUG=642182
TEST=none
R=sadrul@chromium.org
==========

[sky, 2016-09-02 15:52:28]

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

[sky, 2016-09-02 15:52:47]

jam: content
sadrul: all

[sky, 2016-09-02 15:53:51]

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

[commit-bot: I haz the power, 2016-09-02 15:54:06]

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, 2016-09-02 16:07:29]

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

[commit-bot: I haz the power, 2016-09-02 16:07:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sadrul, 2016-09-02 17:04:16]

Cool! lgtm

(sorry, my comments are in patchset 1. It looks like you have addressed at least
one of them already in patchset 3)

https://codereview.chromium.org/2301353003/diff/1/services/ui/public/cpp/test...
File services/ui/public/cpp/tests/test_window_tree_client_setup.cc (right):

https://codereview.chromium.org/2301353003/diff/1/services/ui/public/cpp/test...
services/ui/public/cpp/tests/test_window_tree_client_setup.cc:36: return
std::move(window_tree_client_);
Would it make sense to DCHECK(window_tree_client_) here?

https://codereview.chromium.org/2301353003/diff/1/services/ui/public/cpp/test...
File services/ui/public/cpp/tests/window_server_test_base.h (right):

https://codereview.chromium.org/2301353003/diff/1/services/ui/public/cpp/test...
services/ui/public/cpp/tests/window_server_test_base.h:64: // its.
incomplete?

https://codereview.chromium.org/2301353003/diff/1/services/ui/public/cpp/wind...
File services/ui/public/cpp/window_tree_client.h (right):

https://codereview.chromium.org/2301353003/diff/1/services/ui/public/cpp/wind...
services/ui/public/cpp/window_tree_client.h:398: bool in_destructor_;
Sidenote: should we switch to default initializing these in the header when
possible?

https://codereview.chromium.org/2301353003/diff/1/services/ui/public/cpp/wind...
File services/ui/public/cpp/window_tree_client_delegate.h (right):

https://codereview.chromium.org/2301353003/diff/1/services/ui/public/cpp/wind...
services/ui/public/cpp/window_tree_client_delegate.h:39: // client was embedded
in the same window). This is only called if the
unnecessary line-break

https://codereview.chromium.org/2301353003/diff/1/services/ui/public/cpp/wind...
File services/ui/public/cpp/window_tree_host_factory.h (right):

https://codereview.chromium.org/2301353003/diff/1/services/ui/public/cpp/wind...
services/ui/public/cpp/window_tree_host_factory.h:28: WARN_UNUSED_RESULT
std::unique_ptr<WindowTreeClient> CreateWindowTreeHost(
WARN_UNUSED_RESULT seem to be normally used at the end, although it looks like
we have both styles in our code.

[sky, 2016-09-02 18:29:31]

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

[commit-bot: I haz the power, 2016-09-02 18:29:51]

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, 2016-09-02 18:45:33]

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

[commit-bot: I haz the power, 2016-09-02 18:45:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jam, 2016-09-03 03:30:21]

lgtm

[sky, 2016-09-06 17:14:39]

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

[commit-bot: I haz the power, 2016-09-06 17:15:06]

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

[sky, 2016-09-06 17:15:22]

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

[sky, 2016-09-06 17:15:42]

+ben for services/navigation

https://codereview.chromium.org/2301353003/diff/1/services/ui/public/cpp/test...
File services/ui/public/cpp/tests/test_window_tree_client_setup.cc (right):

https://codereview.chromium.org/2301353003/diff/1/services/ui/public/cpp/test...
services/ui/public/cpp/tests/test_window_tree_client_setup.cc:36: return
std::move(window_tree_client_);
On 2016/09/02 17:04:15, sadrul wrote:
> Would it make sense to DCHECK(window_tree_client_) here?

Done.

https://codereview.chromium.org/2301353003/diff/1/services/ui/public/cpp/test...
File services/ui/public/cpp/tests/window_server_test_base.h (right):

https://codereview.chromium.org/2301353003/diff/1/services/ui/public/cpp/test...
services/ui/public/cpp/tests/window_server_test_base.h:64: // its.
On 2016/09/02 17:04:15, sadrul wrote:
> incomplete?

Done.

https://codereview.chromium.org/2301353003/diff/1/services/ui/public/cpp/wind...
File services/ui/public/cpp/window_tree_client.h (right):

https://codereview.chromium.org/2301353003/diff/1/services/ui/public/cpp/wind...
services/ui/public/cpp/window_tree_client.h:398: bool in_destructor_;
On 2016/09/02 17:04:15, sadrul wrote:
> Sidenote: should we switch to default initializing these in the header when
> possible?

Yes, but I'll keep that to a separate patch.

https://codereview.chromium.org/2301353003/diff/1/services/ui/public/cpp/wind...
File services/ui/public/cpp/window_tree_client_delegate.h (right):

https://codereview.chromium.org/2301353003/diff/1/services/ui/public/cpp/wind...
services/ui/public/cpp/window_tree_client_delegate.h:39: // client was embedded
in the same window). This is only called if the
On 2016/09/02 17:04:15, sadrul wrote:
> unnecessary line-break

Done.

https://codereview.chromium.org/2301353003/diff/1/services/ui/public/cpp/wind...
File services/ui/public/cpp/window_tree_host_factory.h (right):

https://codereview.chromium.org/2301353003/diff/1/services/ui/public/cpp/wind...
services/ui/public/cpp/window_tree_host_factory.h:28: WARN_UNUSED_RESULT
std::unique_ptr<WindowTreeClient> CreateWindowTreeHost(
On 2016/09/02 17:04:15, sadrul wrote:
> WARN_UNUSED_RESULT seem to be normally used at the end, although it looks like
> we have both styles in our code.

I'm going to remove it. I mostly did it to ensure I updated all sites.

[sky, 2016-09-06 18:01:47]

Description was changed from

==========
Changes ownership of WindowTreeClient

Previously WindowTreeClient had two types of ownership. It would some
times delete itself, but clients could also delete it. This is
confusing and leads to problems. This patch makes it so that
WindowTreeClient is always owned by whoever created it and never
deletes itself.

The only wrinkle is what to do when WindowTreeClient loses its
connection to mus. I made it so the delegate is told this happens, but
the internal state is not changed in anyway. I went with this in
so far as it lets the consumer of WindowTreeClient take whatever
action is appropriate. To do otherwise can often conflict with
what the client wants to do. This does mean the client can continue
creating windows and other things, but effectively nothing is
happening on the mus side (it's gone).

BUG=642182
TEST=none
R=sadrul@chromium.org
==========

to

==========
Changes ownership of WindowTreeClient

Previously WindowTreeClient had two types of ownership. It would some
times delete itself, but clients could also delete it. This is
confusing and leads to problems. This patch makes it so that
WindowTreeClient is always owned by whoever created it and never
deletes itself.

The only wrinkle is what to do when WindowTreeClient loses its
connection to mus. I made it so the delegate is told this happens, but
the internal state is not changed in anyway. I went with this in
so far as it lets the consumer of WindowTreeClient take whatever
action is appropriate. To do otherwise can often conflict with
what the client wants to do. This does mean the client can continue
creating windows and other things, but effectively nothing is
happening on the mus side (it's gone).

BUG=642182
TEST=none
R=sadrul@chromium.org
TBR=ben@chromium.org
==========

[sky, 2016-09-06 18:01:59]

TBR ben for services/navigation change

[sky, 2016-09-06 18:02:06]

The CQ bit was unchecked by sky@chromium.org

[sky, 2016-09-06 18:02:11]

The CQ bit was checked by sky@chromium.org

[sky, 2016-09-06 18:02:12]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2301353003/#ps100001
(title: "fix navigation")

[commit-bot: I haz the power, 2016-09-06 18:03: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, 2016-09-06 18:10:09]

Description was changed from

==========
Changes ownership of WindowTreeClient

Previously WindowTreeClient had two types of ownership. It would some
times delete itself, but clients could also delete it. This is
confusing and leads to problems. This patch makes it so that
WindowTreeClient is always owned by whoever created it and never
deletes itself.

The only wrinkle is what to do when WindowTreeClient loses its
connection to mus. I made it so the delegate is told this happens, but
the internal state is not changed in anyway. I went with this in
so far as it lets the consumer of WindowTreeClient take whatever
action is appropriate. To do otherwise can often conflict with
what the client wants to do. This does mean the client can continue
creating windows and other things, but effectively nothing is
happening on the mus side (it's gone).

BUG=642182
TEST=none
R=sadrul@chromium.org
TBR=ben@chromium.org
==========

to

==========
Changes ownership of WindowTreeClient

Previously WindowTreeClient had two types of ownership. It would some
times delete itself, but clients could also delete it. This is
confusing and leads to problems. This patch makes it so that
WindowTreeClient is always owned by whoever created it and never
deletes itself.

The only wrinkle is what to do when WindowTreeClient loses its
connection to mus. I made it so the delegate is told this happens, but
the internal state is not changed in anyway. I went with this in
so far as it lets the consumer of WindowTreeClient take whatever
action is appropriate. To do otherwise can often conflict with
what the client wants to do. This does mean the client can continue
creating windows and other things, but effectively nothing is
happening on the mus side (it's gone).

BUG=642182
TEST=none
R=sadrul@chromium.org
TBR=ben@chromium.org
==========

[commit-bot: I haz the power, 2016-09-06 18:10:10]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-06 18:14:27]

Description was changed from

==========
Changes ownership of WindowTreeClient

Previously WindowTreeClient had two types of ownership. It would some
times delete itself, but clients could also delete it. This is
confusing and leads to problems. This patch makes it so that
WindowTreeClient is always owned by whoever created it and never
deletes itself.

The only wrinkle is what to do when WindowTreeClient loses its
connection to mus. I made it so the delegate is told this happens, but
the internal state is not changed in anyway. I went with this in
so far as it lets the consumer of WindowTreeClient take whatever
action is appropriate. To do otherwise can often conflict with
what the client wants to do. This does mean the client can continue
creating windows and other things, but effectively nothing is
happening on the mus side (it's gone).

BUG=642182
TEST=none
R=sadrul@chromium.org
TBR=ben@chromium.org
==========

to

==========
Changes ownership of WindowTreeClient

Previously WindowTreeClient had two types of ownership. It would some
times delete itself, but clients could also delete it. This is
confusing and leads to problems. This patch makes it so that
WindowTreeClient is always owned by whoever created it and never
deletes itself.

The only wrinkle is what to do when WindowTreeClient loses its
connection to mus. I made it so the delegate is told this happens, but
the internal state is not changed in anyway. I went with this in
so far as it lets the consumer of WindowTreeClient take whatever
action is appropriate. To do otherwise can often conflict with
what the client wants to do. This does mean the client can continue
creating windows and other things, but effectively nothing is
happening on the mus side (it's gone).

BUG=642182
TEST=none
R=sadrul@chromium.org
TBR=ben@chromium.org

Committed: https://crrev.com/9636beeb7afa6167b07e6afe2b463d920ebe61bc
Cr-Commit-Position: refs/heads/master@{#416670}
==========

[commit-bot: I haz the power, 2016-09-06 18:14:28]

Patchset 6 (id:??) landed as
https://crrev.com/9636beeb7afa6167b07e6afe2b463d920ebe61bc
Cr-Commit-Position: refs/heads/master@{#416670}