Renames WmWindowUserData and converts to using aura

Renames WmWindowUserData and converts to using aura

As part of this I converted a handful of places from WmWindow to aura::Window. For example, GetContainersFromAllRootWindows was converted from WmWindow to aura::Window.

BUG=699678
TEST=covered by tests
R=jamescook@chromium.org

Review-Url: https://codereview.chromium.org/2735983006
Cr-Commit-Position: refs/heads/master@{#455891}
Committed: https://chromium.googlesource.com/chromium/src/+/e02e09e5f6b809e3af3c8d2707929c1bfcb43781

[sky, 2017-03-09 00:57:38]

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

[commit-bot: I haz the power, 2017-03-09 00:58:08]

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

[sky, 2017-03-09 01:02:26]

I think this patch makes the ownership of WindowDimmer mildly confusing and
error prone. I'm thinking of keeping WmWindowUserData but making it only work
with aura::Window. Thoughts?

https://codereview.chromium.org/2735983006/diff/20001/ash/common/wm/window_di...
File ash/common/wm/window_dimmer.cc (right):

https://codereview.chromium.org/2735983006/diff/20001/ash/common/wm/window_di...
ash/common/wm/window_dimmer.cc:29: ->NewWindow(ui::wm::WINDOW_TYPE_NORMAL,
ui::LAYER_SOLID_COLOR)
NewWindow is being nuked in another patch cycling through the cq. I'll need to
merge with it before landing.

[commit-bot: I haz the power, 2017-03-09 02:10:45]

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

[commit-bot: I haz the power, 2017-03-09 02:10:46]

Dry run: This issue passed the CQ dry run.

[James Cook, 2017-03-09 18:39:27]

I had not yet looked at ScreenPinningController before we talked. I now think it
would be clearer to keep WmWindowUserData and convert it to aura, as you
mentioned. It should probably be renamed as part of the aura conversion.

https://codereview.chromium.org/2735983006/diff/40001/ash/common/wm/container...
File ash/common/wm/container_finder.h (right):

https://codereview.chromium.org/2735983006/diff/40001/ash/common/wm/container...
ash/common/wm/container_finder.h:39: ASH_EXPORT std::vector<aura::Window*>
GetContainersFromAllRootWindows(
It wasn't immediately obvious to me why this change was part of this CL. Can you
mention it in the CL description?

https://codereview.chromium.org/2735983006/diff/40001/ash/common/wm/screen_di...
File ash/common/wm/screen_dimmer.cc (right):

https://codereview.chromium.org/2735983006/diff/40001/ash/common/wm/screen_di...
ash/common/wm/screen_dimmer.cc:74: container->SetProperty(kWindowDimmerKey,
window_dimmer);
Aside: It's kind of a bummer we don't have a SetOwnedProperty() method that
takes a unique_ptr<> to make call sites like this more clear. I have no idea if
there's a way to use template magic to actually enforce that it's an
owned-property, though.

https://codereview.chromium.org/2735983006/diff/40001/ash/common/wm/screen_di...
File ash/common/wm/screen_dimmer.h (right):

https://codereview.chromium.org/2735983006/diff/40001/ash/common/wm/screen_di...
ash/common/wm/screen_dimmer.h:58: // Returns the WindowDimmer for |window|. This
used in tests.
ForTesting, then?

https://codereview.chromium.org/2735983006/diff/40001/ash/common/wm/window_di...
File ash/common/wm/window_dimmer.h (right):

https://codereview.chromium.org/2735983006/diff/40001/ash/common/wm/window_di...
ash/common/wm/window_dimmer.h:48: std::unique_ptr<aura::Window> window_;
The code might be a little clearer if this was dimmer_window_.

https://codereview.chromium.org/2735983006/diff/40001/ash/wm/window_propertie...
File ash/wm/window_properties.cc (right):

https://codereview.chromium.org/2735983006/diff/40001/ash/wm/window_propertie...
ash/wm/window_properties.cc:10:
DECLARE_UI_CLASS_PROPERTY_TYPE(ash::WindowDimmer*);
Question for my knowledge: Why can't the DECLARE go in the same file as the
DEFINE?

[sky, 2017-03-09 20:29:04]

Description was changed from

==========
Removes WmWindowUserData

It's replaced with a combination of an aura::WindowProperty and
WindowTracker.

BUG=699678
TEST=covered by tests
R=jamescook@chromium.org
==========

to

==========
Reanmes WmWindowUserData and converts to using aura

As part of this I converted a handful of places from WmWindow to aura::Window.
For example, GetContainersFromAllRootWindows was converted from WmWindow to
aura::Window.

BUG=699678
TEST=covered by tests
R=jamescook@chromium.org
==========

[sky, 2017-03-09 20:39:04]

https://codereview.chromium.org/2735983006/diff/40001/ash/common/wm/container...
File ash/common/wm/container_finder.h (right):

https://codereview.chromium.org/2735983006/diff/40001/ash/common/wm/container...
ash/common/wm/container_finder.h:39: ASH_EXPORT std::vector<aura::Window*>
GetContainersFromAllRootWindows(
On 2017/03/09 18:39:27, James Cook wrote:
> It wasn't immediately obvious to me why this change was part of this CL. Can
you
> mention it in the CL description?

Done.

https://codereview.chromium.org/2735983006/diff/40001/ash/common/wm/screen_di...
File ash/common/wm/screen_dimmer.cc (right):

https://codereview.chromium.org/2735983006/diff/40001/ash/common/wm/screen_di...
ash/common/wm/screen_dimmer.cc:74: container->SetProperty(kWindowDimmerKey,
window_dimmer);
On 2017/03/09 18:39:27, James Cook wrote:
> Aside: It's kind of a bummer we don't have a SetOwnedProperty() method that
> takes a unique_ptr<> to make call sites like this more clear. I have no idea
if
> there's a way to use template magic to actually enforce that it's an
> owned-property, though.

Agreed. I filed 687408 on that a while back.

https://codereview.chromium.org/2735983006/diff/40001/ash/common/wm/screen_di...
File ash/common/wm/screen_dimmer.h (right):

https://codereview.chromium.org/2735983006/diff/40001/ash/common/wm/screen_di...
ash/common/wm/screen_dimmer.h:58: // Returns the WindowDimmer for |window|. This
used in tests.
On 2017/03/09 18:39:27, James Cook wrote:
> ForTesting, then?

This has been nuked.

https://codereview.chromium.org/2735983006/diff/40001/ash/common/wm/window_di...
File ash/common/wm/window_dimmer.h (right):

https://codereview.chromium.org/2735983006/diff/40001/ash/common/wm/window_di...
ash/common/wm/window_dimmer.h:48: std::unique_ptr<aura::Window> window_;
On 2017/03/09 18:39:27, James Cook wrote:
> The code might be a little clearer if this was dimmer_window_.

This has been removed.

https://codereview.chromium.org/2735983006/diff/40001/ash/wm/window_propertie...
File ash/wm/window_properties.cc (right):

https://codereview.chromium.org/2735983006/diff/40001/ash/wm/window_propertie...
ash/wm/window_properties.cc:10:
DECLARE_UI_CLASS_PROPERTY_TYPE(ash::WindowDimmer*);
On 2017/03/09 18:39:27, James Cook wrote:
> Question for my knowledge: Why can't the DECLARE go in the same file as the
> DEFINE?

Two places used this type, so rather than have it in one and not the other I
moved it here. But now that I'm using WindowUserData again this has been
removed.

[sky, 2017-03-09 20:39:09]

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

[commit-bot: I haz the power, 2017-03-09 20:39:31]

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

[sky, 2017-03-09 20:39:35]

Description was changed from

==========
Reanmes WmWindowUserData and converts to using aura

As part of this I converted a handful of places from WmWindow to aura::Window.
For example, GetContainersFromAllRootWindows was converted from WmWindow to
aura::Window.

BUG=699678
TEST=covered by tests
R=jamescook@chromium.org
==========

to

==========
Renames WmWindowUserData and converts to using aura

As part of this I converted a handful of places from WmWindow to aura::Window.
For example, GetContainersFromAllRootWindows was converted from WmWindow to
aura::Window.

BUG=699678
TEST=covered by tests
R=jamescook@chromium.org
==========

[James Cook, 2017-03-09 21:12:00]

https://codereview.chromium.org/2735983006/diff/80001/ash/common/wm/window_di...
File ash/common/wm/window_dimmer.h (right):

https://codereview.chromium.org/2735983006/diff/80001/ash/common/wm/window_di...
ash/common/wm/window_dimmer.h:49: std::unique_ptr<aura::Window> window_;
Wait, why does this have to become a unique_ptr<>? Is this an attempt to clarify
ownership? If so, I think it might be clearer to keep it a raw aura::Window*,
since the common case seems to be that the appropriate controller clears the
owned property which deletes the window. Then there could be a "delete window_"
with a comment explaining that this is the special case for WindowDimmer being
killed early. WDYT?

The ownership here is still hard to understand.

[sky, 2017-03-09 21:18:42]

https://codereview.chromium.org/2735983006/diff/80001/ash/common/wm/window_di...
File ash/common/wm/window_dimmer.h (right):

https://codereview.chromium.org/2735983006/diff/80001/ash/common/wm/window_di...
ash/common/wm/window_dimmer.h:49: std::unique_ptr<aura::Window> window_;
On 2017/03/09 21:12:00, James Cook wrote:
> Wait, why does this have to become a unique_ptr<>? Is this an attempt to
clarify
> ownership? If so, I think it might be clearer to keep it a raw aura::Window*,
> since the common case seems to be that the appropriate controller clears the
> owned property which deletes the window. Then there could be a "delete
window_"
> with a comment explaining that this is the special case for WindowDimmer being
> killed early. WDYT?
> 
> The ownership here is still hard to understand.

I thought using the unique_ptr would better clarify ownership, but I agree the
mixed ownership is tricky. Ok, no unique_ptr.

[sky, 2017-03-09 21:18:50]

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

[commit-bot: I haz the power, 2017-03-09 21:19:16]

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

[James Cook, 2017-03-09 21:24:57]

LGTM but can you see if you can improve the ownership comments?

https://codereview.chromium.org/2735983006/diff/120001/ash/common/wm/window_d...
File ash/common/wm/window_dimmer.cc (right):

https://codereview.chromium.org/2735983006/diff/120001/ash/common/wm/window_d...
ash/common/wm/window_dimmer.cc:50: delete window_;
Please add a comment here that this is the uncommon case.

Frankly, I think the class comment could be better also. It seems like there are
potentially three deletion paths:

* window_ gets destroyed
* The parent of window_ gets destroyed
* The controller holding the WindowUserData clears its list

[sky, 2017-03-09 21:30:58]

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

[commit-bot: I haz the power, 2017-03-09 21:31:18]

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

[sky, 2017-03-09 21:32:28]

https://codereview.chromium.org/2735983006/diff/120001/ash/common/wm/window_d...
File ash/common/wm/window_dimmer.cc (right):

https://codereview.chromium.org/2735983006/diff/120001/ash/common/wm/window_d...
ash/common/wm/window_dimmer.cc:50: delete window_;
On 2017/03/09 21:24:57, James Cook wrote:
> Please add a comment here that this is the uncommon case.
> 
> Frankly, I think the class comment could be better also. It seems like there
are
> potentially three deletion paths:
> 
> * window_ gets destroyed
> * The parent of window_ gets destroyed
> * The controller holding the WindowUserData clears its list

1 and 2 are typically the same, and from the perspective of WindowDimmer they
are exactly the same. Generally window_ is only destroyed if the parent_ is also
destroyed.

Case 3 is when WindowDimmer is deleted. I've expanded the class description to
better describe this. Any better?

[James Cook, 2017-03-09 21:40:39]

still lgtm

https://codereview.chromium.org/2735983006/diff/140001/ash/common/wm/window_d...
File ash/common/wm/window_dimmer.h (right):

https://codereview.chromium.org/2735983006/diff/140001/ash/common/wm/window_d...
ash/common/wm/window_dimmer.h:24: // still valid, then WindowDimmer deletes the
window.
This is much better, thanks.

[sky, 2017-03-09 21:41:04]

The CQ bit was unchecked by sky@chromium.org

[sky, 2017-03-09 21:41:08]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2017-03-09 21:41:18]

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-03-09 22:54:34]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1489095668566580,
"parent_rev": "77f6721b3f38f935a2fb473507dbba81bca6fea1", "commit_rev":
"e02e09e5f6b809e3af3c8d2707929c1bfcb43781"}

[commit-bot: I haz the power, 2017-03-09 22:55:12]

Description was changed from

==========
Renames WmWindowUserData and converts to using aura

As part of this I converted a handful of places from WmWindow to aura::Window.
For example, GetContainersFromAllRootWindows was converted from WmWindow to
aura::Window.

BUG=699678
TEST=covered by tests
R=jamescook@chromium.org
==========

to

==========
Renames WmWindowUserData and converts to using aura

As part of this I converted a handful of places from WmWindow to aura::Window.
For example, GetContainersFromAllRootWindows was converted from WmWindow to
aura::Window.

BUG=699678
TEST=covered by tests
R=jamescook@chromium.org

Review-Url: https://codereview.chromium.org/2735983006
Cr-Commit-Position: refs/heads/master@{#455891}
Committed:
https://chromium.googlesource.com/chromium/src/+/e02e09e5f6b809e3af3c8d270792...
==========

[commit-bot: I haz the power, 2017-03-09 22:55:18]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/e02e09e5f6b809e3af3c8d270792...