chromeos: converts ash devtools to aura::Window

chromeos: converts ash devtools to aura::Window

BUG=687656
TEST=covered by tests
R=sadrul@chromium.org

Review-Url: https://codereview.chromium.org/2821213002
Cr-Commit-Position: refs/heads/master@{#465299}
Committed: https://chromium.googlesource.com/chromium/src/+/3e82c5b9d88086ead8bbbd122eef8c1b5d45f5e3

[sky, 2017-04-18 04:04:16]

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

[commit-bot: I haz the power, 2017-04-18 04:04:37]

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-04-18 04:23:28]

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

[commit-bot: I haz the power, 2017-04-18 04:23:28]

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

[sky, 2017-04-18 15:04:33]

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

[commit-bot: I haz the power, 2017-04-18 15:04:58]

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

[sky, 2017-04-18 15:13:35]

Description was changed from

==========
chromeos: converts ash devtools to aura::Window

BUG=687656
TEST=covered by tests
R=sadrul@chromium.org
==========

to

==========
chromeos: converts ash devtools to aura::Window

BUG=687656
TEST=covered by tests
R=sadrul@chromium.org
==========

[sky, 2017-04-18 15:13:35]

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

[sky, 2017-04-18 15:13:42]

+alokp for chromecast

[commit-bot: I haz the power, 2017-04-18 16:00:17]

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

[commit-bot: I haz the power, 2017-04-18 16:00:17]

Dry run: This issue passed the CQ dry run.

[alokp, 2017-04-18 16:57:29]

rs lgtm

https://codereview.chromium.org/2821213002/diff/40001/ui/wm/public/activation...
File ui/wm/public/activation_client.h (right):

https://codereview.chromium.org/2821213002/diff/40001/ui/wm/public/activation...
ui/wm/public/activation_client.h:35: virtual const Window* GetActiveWindow()
const = 0;
non-owner nit: I personally find *const* interfaces undesirable. Base class
should not make assumptions about what derived classes would need to implement a
virtual interface.

I also find the const-cast above weird. I have not looked at the entire CL, so
am not sure why you need to do this.

[sky, 2017-04-18 17:16:05]

https://codereview.chromium.org/2821213002/diff/40001/ui/wm/public/activation...
File ui/wm/public/activation_client.h (right):

https://codereview.chromium.org/2821213002/diff/40001/ui/wm/public/activation...
ui/wm/public/activation_client.h:35: virtual const Window* GetActiveWindow()
const = 0;
On 2017/04/18 16:57:29, alokp wrote:
> non-owner nit: I personally find *const* interfaces undesirable. Base class
> should not make assumptions about what derived classes would need to implement
a
> virtual interface.

This function is intended to be a trivial getter, hence the const. If this
function isn't const, it means it is not possible (without const-casts) to
determine if a const Window is active. That seems wrong.

> I also find the const-cast above weird.

Indeed. It comes from Effective C++.

> I have not looked at the entire CL, so am not sure why you need to do this.

The const change is motivated by being able to determine if a const Window is
active, which should be possible.

[sadrul, 2017-04-18 17:36:11]

lgtm

[sky, 2017-04-18 18:13:49]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2017-04-18 18:14:32]

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-04-18 18:46:55]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492539229949000,
"parent_rev": "74500bd4d32d826311be745333f5b338b68bf63b", "commit_rev":
"3e82c5b9d88086ead8bbbd122eef8c1b5d45f5e3"}

[commit-bot: I haz the power, 2017-04-18 18:47:06]

Description was changed from

==========
chromeos: converts ash devtools to aura::Window

BUG=687656
TEST=covered by tests
R=sadrul@chromium.org
==========

to

==========
chromeos: converts ash devtools to aura::Window

BUG=687656
TEST=covered by tests
R=sadrul@chromium.org

Review-Url: https://codereview.chromium.org/2821213002
Cr-Commit-Position: refs/heads/master@{#465299}
Committed:
https://chromium.googlesource.com/chromium/src/+/3e82c5b9d88086ead8bbbd122eef...
==========

[commit-bot: I haz the power, 2017-04-18 18:47:07]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/3e82c5b9d88086ead8bbbd122eef...

[alokp, 2017-04-18 18:48:33]

https://codereview.chromium.org/2821213002/diff/40001/ui/wm/public/activation...
File ui/wm/public/activation_client.h (right):

https://codereview.chromium.org/2821213002/diff/40001/ui/wm/public/activation...
ui/wm/public/activation_client.h:35: virtual const Window* GetActiveWindow()
const = 0;
> This function is intended to be a trivial getter, hence the const.

Which is what I meant by making assumptions about the implementation (trivial
getter) of derived classes :)

> If this
> function isn't const, it means it is not possible (without const-casts) to
> determine if a const Window is active. That seems wrong.

May be I am missing something, but I do not see any problem in
wm::IsActiveWindow taking a const Window and ActivationClient::GetActiveWindow
returning a const or non-const window. I do not see why
ActivationClient::GetActiveWindow needs to be a const function.

This is all super-nit bike-shedding though :)

[sky, 2017-04-18 19:08:13]

https://codereview.chromium.org/2821213002/diff/40001/ui/wm/public/activation...
File ui/wm/public/activation_client.h (right):

https://codereview.chromium.org/2821213002/diff/40001/ui/wm/public/activation...
ui/wm/public/activation_client.h:35: virtual const Window* GetActiveWindow()
const = 0;
On 2017/04/18 18:48:33, alokp wrote:
> > This function is intended to be a trivial getter, hence the const.
> 
> Which is what I meant by making assumptions about the implementation (trivial
> getter) of derived classes :)
> 
> > If this
> > function isn't const, it means it is not possible (without const-casts) to
> > determine if a const Window is active. That seems wrong.
> 
> May be I am missing something, but I do not see any problem in
> wm::IsActiveWindow taking a const Window and ActivationClient::GetActiveWindow
> returning a const or non-const window. I do not see why
> ActivationClient::GetActiveWindow needs to be a const function.

IsActiveWindow() requires the ActivationClient, which is found from the root
window, e.g.:

const aura::Window* root = window->GetRootWindow();

If |window| is const, then root must also be const. If it wasn't, meaning you
could get the non-const root given a const, it would mean you could effectively
circumvent the const'ness of the window.

Similarly, when getting the ActivationClient for the root, if the root is const
then so should the ActivationClient be:

const ActivationClient* activation_client = root->GetProperty(...);

Why should the ActivationClient be const? For the same reason as above, if it
didn't return a const ActivationClient it would mean you can circumvent the
const'ness of the underlying window.

As this point we have a const ActivationClient. Again, const functions should
return const objects, otherwise you can circumvent the const'ness. That means we
need GetActiveWindow() to be const and return a const object.

> This is all super-nit bike-shedding though :)

Indeed.