Makes DefaultCaptureClient work with a null root_window

Makes DefaultCaptureClient work with a null root_window

DefaultCaptureClient only uses the root to install itself as the
CaptureClient. I previously landed a change to make the root optional,
but didn't land this part of it that ignores a null root.

BUG=659155
TEST=none
R=msw@chromium.org

Committed: https://crrev.com/8adce9c941c6a2fdef33023a96b463b6f7cf06a3
Cr-Commit-Position: refs/heads/master@{#427458}

[sky, 2016-10-25 15:28:28]

Description was changed from

==========
Makes DefaultCaptureClient work with a null root_window

DefaultCaptureClient only uses the root to install itself as the
CaptureClient. I previously landed a change to make the root optional,
but didn't land this part of it that ignores a null root.

BUG=none
TEST=none
R=msw@chromium.org
==========

to

==========
Makes DefaultCaptureClient work with a null root_window

DefaultCaptureClient only uses the root to install itself as the
CaptureClient. I previously landed a change to make the root optional,
but didn't land this part of it that ignores a null root.

BUG=2449073002
TEST=none
R=msw@chromium.org
==========

[sky, 2016-10-25 15:28:35]

Description was changed from

==========
Makes DefaultCaptureClient work with a null root_window

DefaultCaptureClient only uses the root to install itself as the
CaptureClient. I previously landed a change to make the root optional,
but didn't land this part of it that ignores a null root.

BUG=2449073002
TEST=none
R=msw@chromium.org
==========

to

==========
Makes DefaultCaptureClient work with a null root_window

DefaultCaptureClient only uses the root to install itself as the
CaptureClient. I previously landed a change to make the root optional,
but didn't land this part of it that ignores a null root.

BUG=659155
TEST=none
R=msw@chromium.org
==========

[sky, 2016-10-25 15:33:21]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2016-10-25 15:33: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-10-25 15:33:52]

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

[commit-bot: I haz the power, 2016-10-25 15:33:52]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[sky, 2016-10-25 15:51:16]

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

[commit-bot: I haz the power, 2016-10-25 15:51:31]

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-10-25 16:39:14]

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

[commit-bot: I haz the power, 2016-10-25 16:39:15]

Dry run: This issue passed the CQ dry run.

[msw, 2016-10-25 17:57:58]

lgtm with a q.

https://codereview.chromium.org/2449073002/diff/1/ui/aura/client/default_capt...
File ui/aura/client/default_capture_client.cc (right):

https://codereview.chromium.org/2449073002/diff/1/ui/aura/client/default_capt...
ui/aura/client/default_capture_client.cc:46: capture_delegate =
capture_window_->GetHost()->dispatcher();
q: just checking that the change from |root_window_| to |capture_window_| and
|old_capture_window_| to get the WindowTreeHost and WindowEventDispatcher is
intentional; should we check that the root of the [old] capture window matches
|root_window_| if that value is non-null?

[sky, 2016-10-25 19:09:59]

I added the DCHECK, and this uncovered a test trying to use DefaultCaptureClient
with different hosts, even though that doesn't really work right. So, I changed
AuraTestBase to not supply a root. Take another look?

[sky, 2016-10-25 19:10:10]

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

[commit-bot: I haz the power, 2016-10-25 19:10: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-10-25 19:21:08]

lgtm with nit and aside.

https://codereview.chromium.org/2449073002/diff/20001/ui/aura/test/aura_test_...
File ui/aura/test/aura_test_helper.cc (right):

https://codereview.chromium.org/2449073002/diff/20001/ui/aura/test/aura_test_...
ui/aura/test/aura_test_helper.cc:82: SetCaptureClient(host_->window(),
capture_client_.get());
nit: client:: needed here like below?

https://codereview.chromium.org/2449073002/diff/20001/ui/aura/test/aura_test_...
ui/aura/test/aura_test_helper.cc:82: SetCaptureClient(host_->window(),
capture_client_.get());
aside: odd that host_->window()'s root is not root_window()...
(I haven't looked closely to actually figure this out...)

[sky, 2016-10-25 19:44:40]

https://codereview.chromium.org/2449073002/diff/20001/ui/aura/test/aura_test_...
File ui/aura/test/aura_test_helper.cc (right):

https://codereview.chromium.org/2449073002/diff/20001/ui/aura/test/aura_test_...
ui/aura/test/aura_test_helper.cc:82: SetCaptureClient(host_->window(),
capture_client_.get());
On 2016/10/25 19:21:08, msw wrote:
> nit: client:: needed here like below?

Surprisingly it isn't needed here, but is needed below. That is, this compiles,
but if I remove client:: from below I get:

../../ui/aura/test/aura_test_helper.cc:92:3: error: use of undeclared identifier
'SetCaptureClient'; did you mean 'client::SetCaptureClient'?
  SetCaptureClient(host_->window(), nullptr);
  ^~~~~~~~~~~~~~~~
  client::SetCaptureClient
../../ui/aura/client/capture_client.h:44:18: note: 'client

I stepped through this code to ensure it calls to aura::client::SetCaptureClient
and it does. I'm speculating, but I'm guessing having a type in capture_client_
makes this call work without the client:: where as the code below has no type,
so it fails.

[msw, 2016-10-25 19:47:59]

still lgtm

https://codereview.chromium.org/2449073002/diff/20001/ui/aura/test/aura_test_...
File ui/aura/test/aura_test_helper.cc (right):

https://codereview.chromium.org/2449073002/diff/20001/ui/aura/test/aura_test_...
ui/aura/test/aura_test_helper.cc:82: SetCaptureClient(host_->window(),
capture_client_.get());
On 2016/10/25 19:44:40, sky wrote:
> On 2016/10/25 19:21:08, msw wrote:
> > nit: client:: needed here like below?
> 
> Surprisingly it isn't needed here, but is needed below. That is, this
compiles,
> but if I remove client:: from below I get:
> 
> ../../ui/aura/test/aura_test_helper.cc:92:3: error: use of undeclared
identifier
> 'SetCaptureClient'; did you mean 'client::SetCaptureClient'?
>   SetCaptureClient(host_->window(), nullptr);
>   ^~~~~~~~~~~~~~~~
>   client::SetCaptureClient
> ../../ui/aura/client/capture_client.h:44:18: note: 'client
> 
> I stepped through this code to ensure it calls to
aura::client::SetCaptureClient
> and it does. I'm speculating, but I'm guessing having a type in
capture_client_
> makes this call work without the client:: where as the code below has no type,
> so it fails.

Odd... I might add it to be clear/consistent, but it's up to you.

[sky, 2016-10-25 19:51:14]

The CQ bit was checked by sky@chromium.org

[sky, 2016-10-25 19:51:14]

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/2449073002/#ps40001
(title: "client::")

[commit-bot: I haz the power, 2016-10-25 19:51:41]

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

[sky, 2016-10-25 19:56:45]

On 2016/10/25 19:47:59, msw wrote:
> still lgtm
> 
>
https://codereview.chromium.org/2449073002/diff/20001/ui/aura/test/aura_test_...
> File ui/aura/test/aura_test_helper.cc (right):
> 
>
https://codereview.chromium.org/2449073002/diff/20001/ui/aura/test/aura_test_...
> ui/aura/test/aura_test_helper.cc:82: SetCaptureClient(host_->window(),
> capture_client_.get());
> On 2016/10/25 19:44:40, sky wrote:
> > On 2016/10/25 19:21:08, msw wrote:
> > > nit: client:: needed here like below?
> > 
> > Surprisingly it isn't needed here, but is needed below. That is, this
> compiles,
> > but if I remove client:: from below I get:
> > 
> > ../../ui/aura/test/aura_test_helper.cc:92:3: error: use of undeclared
> identifier
> > 'SetCaptureClient'; did you mean 'client::SetCaptureClient'?
> >   SetCaptureClient(host_->window(), nullptr);
> >   ^~~~~~~~~~~~~~~~
> >   client::SetCaptureClient
> > ../../ui/aura/client/capture_client.h:44:18: note: 'client
> > 
> > I stepped through this code to ensure it calls to
> aura::client::SetCaptureClient
> > and it does. I'm speculating, but I'm guessing having a type in
> capture_client_
> > makes this call work without the client:: where as the code below has no
type,
> > so it fails.
> 
> Odd... I might add it to be clear/consistent, but it's up to you.

Good idea, done.

[commit-bot: I haz the power, 2016-10-25 20:47:19]

Description was changed from

==========
Makes DefaultCaptureClient work with a null root_window

DefaultCaptureClient only uses the root to install itself as the
CaptureClient. I previously landed a change to make the root optional,
but didn't land this part of it that ignores a null root.

BUG=659155
TEST=none
R=msw@chromium.org
==========

to

==========
Makes DefaultCaptureClient work with a null root_window

DefaultCaptureClient only uses the root to install itself as the
CaptureClient. I previously landed a change to make the root optional,
but didn't land this part of it that ignores a null root.

BUG=659155
TEST=none
R=msw@chromium.org
==========

[commit-bot: I haz the power, 2016-10-25 20:47:21]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-25 20:49:34]

Description was changed from

==========
Makes DefaultCaptureClient work with a null root_window

DefaultCaptureClient only uses the root to install itself as the
CaptureClient. I previously landed a change to make the root optional,
but didn't land this part of it that ignores a null root.

BUG=659155
TEST=none
R=msw@chromium.org
==========

to

==========
Makes DefaultCaptureClient work with a null root_window

DefaultCaptureClient only uses the root to install itself as the
CaptureClient. I previously landed a change to make the root optional,
but didn't land this part of it that ignores a null root.

BUG=659155
TEST=none
R=msw@chromium.org

Committed: https://crrev.com/8adce9c941c6a2fdef33023a96b463b6f7cf06a3
Cr-Commit-Position: refs/heads/master@{#427458}
==========

[commit-bot: I haz the power, 2016-10-25 20:49:35]

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