Refactors DimWindow and moves to ash/common

Refactors DimWindow and moves to ash/common

DimWindow was both an aura::Window and had some logic. As that won't
work at all in mash (and we generally don't subclass aura::Windows)
I've converted it to have a WmWindow. I also renamed it to
WindowDimmer.

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

Committed: https://crrev.com/b733d4184e6a91eebd951cae60c871118d4cc2a2
Cr-Commit-Position: refs/heads/master@{#417755}

[sky, 2016-09-08 21:04:31]

https://codereview.chromium.org/2320273002/diff/40001/ash/common/wm_window.h
File ash/common/wm_window.h (right):

https://codereview.chromium.org/2320273002/diff/40001/ash/common/wm_window.h#...
ash/common/wm_window.h:134: base::SupportsUserData* GetUserData();
I went with this approach rather than having WmWindow implement
base::SupportsUserData so that destruction of the userdata can happen from the
subclass. This is important, otherwise if the destruction tried to access any
pure virtual methods it would get garbage.

[sky, 2016-09-08 21:04:37]

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

[commit-bot: I haz the power, 2016-09-08 21:05:11]

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-08 21:09:57]

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

[commit-bot: I haz the power, 2016-09-08 21:09:57]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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_...)

[sky, 2016-09-08 21:23:41]

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

[commit-bot: I haz the power, 2016-09-08 21:24:48]

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-08 22:15:38]

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

[commit-bot: I haz the power, 2016-09-08 22:15:39]

Dry run: This issue passed the CQ dry run.

[James Cook, 2016-09-09 00:03:36]

I like WindowDimmer, both the name and the fact that it isn't a window subclass.
DimWindow wasn't the smartest window. :-)

Now that WindowDimmer isn't a window, I think it might be clearer to have
explicit ownership of the objects where they are created, rather than storing
them as userdata/properties. See below.

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

https://codereview.chromium.org/2320273002/diff/60001/ash/common/wm/window_di...
ash/common/wm/window_dimmer.cc:27: ~Data() override { delete window_dimmer; }
So is the point of all this userdata to ensure that the dimmer window is deleted
when the container is deleted? Could you just explicitly observe the container
window for destruction?  Get() could be implemented with a set of containers
being observed. That might be easier to understand than the side effects and
destruction order of userdata.

https://codereview.chromium.org/2320273002/diff/60001/ash/common/wm/window_di...
ash/common/wm/window_dimmer.cc:65: // RemoveUserData() riggers deleting this.
nit: triggers

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

https://codereview.chromium.org/2320273002/diff/60001/ash/common/wm/window_di...
ash/common/wm/window_dimmer.h:5: #ifndef ASH_COMMON_WINDOW_DIMMER_H_
Aside: I find it amusing how often we (I) forget header guards.

https://codereview.chromium.org/2320273002/diff/60001/ash/common/wm/window_di...
ash/common/wm/window_dimmer.h:14: // WindowDimmer creates a window that is used
to dim siblings of window.
nit: of a window

https://codereview.chromium.org/2320273002/diff/60001/ash/common/wm_window.cc
File ash/common/wm_window.cc (right):

https://codereview.chromium.org/2320273002/diff/60001/ash/common/wm_window.cc...
ash/common/wm_window.cc:12: class WmWindow::SupportsUserDataImpl : public
base::SupportsUserData {
Is the whole point of this to get around the protected virtual destructor of
base::SupportsUserData? WDYT of making that destructor public so you can just
allocate one?

https://codereview.chromium.org/2320273002/diff/60001/ash/common/wm_window.cc...
ash/common/wm_window.cc:29: // pure virtual methods are still valid while
destroying |user_data_|.
Thanks for documenting this.

https://codereview.chromium.org/2320273002/diff/60001/ash/common/wm_window.h
File ash/common/wm_window.h (right):

https://codereview.chromium.org/2320273002/diff/60001/ash/common/wm_window.h#...
ash/common/wm_window.h:13: #include "base/supports_user_data.h"
nit: either skip this or skip the forward declare below

https://codereview.chromium.org/2320273002/diff/60001/ash/common/wm_window.h#...
ash/common/wm_window.h:176: virtual void SetVisibilityChangesAnimated() = 0;
nit: move above comment.

Also, a comment for this function would be nice. I guess visibility changes
aren't animated by default?

https://codereview.chromium.org/2320273002/diff/60001/ash/mus/bridge/wm_windo...
File ash/mus/bridge/wm_window_mus.cc (right):

https://codereview.chromium.org/2320273002/diff/60001/ash/mus/bridge/wm_windo...
ash/mus/bridge/wm_window_mus.cc:208: return is_wm_window_type_set_ ?
wm_window_type_ : GetWmWindowType(window_);
Is it not sufficient to return wm_window_type_ if it's not equal to
WINDOW_TYPE_UNKNOWN? Or rather, you want to return WINDOW_TYPE_UNKNOWN if
someone explicitly sets it?

https://codereview.chromium.org/2320273002/diff/60001/ash/wm/screen_dimmer.cc
File ash/wm/screen_dimmer.cc (right):

https://codereview.chromium.org/2320273002/diff/60001/ash/wm/screen_dimmer.cc...
ash/wm/screen_dimmer.cc:98: dim = new
WindowDimmer(WmWindowAura::Get(container));
Or maybe ScreenDimmer could store a set of WindowDimmers. The ownership here is
tricky -- this looks leaky, but isn't.

[sky, 2016-09-09 03:50:04]

I'm not opposed to nuking UserData. See motivation below for keeping it. Not
going with UserData will be more code, but you are right that ownership is
better understood without it.

One advantage UserData has is that different parts of the code can look up data
without necessarily have created it. I don't think that's the case with
WindowDimmer, but is worth mentioning.

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

https://codereview.chromium.org/2320273002/diff/60001/ash/common/wm/window_di...
ash/common/wm/window_dimmer.cc:27: ~Data() override { delete window_dimmer; }
On 2016/09/09 00:03:35, James Cook wrote:
> So is the point of all this userdata to ensure that the dimmer window is
deleted
> when the container is deleted? Could you just explicitly observe the container
> window for destruction?  Get() could be implemented with a set of containers
> being observed. That might be easier to understand than the side effects and
> destruction order of userdata.

The main motivation for user data is the same as that of properties on
aura::Window or ui::Window. Arbitrary data associated with a window that you can
find when you have a handle to the window. See the Get() function for what I
mean. ScreenDimmer has a similar use case.

https://codereview.chromium.org/2320273002/diff/60001/ash/common/wm/window_di...
ash/common/wm/window_dimmer.cc:65: // RemoveUserData() riggers deleting this.
On 2016/09/09 00:03:35, James Cook wrote:
> nit: triggers

Done.

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

https://codereview.chromium.org/2320273002/diff/60001/ash/common/wm/window_di...
ash/common/wm/window_dimmer.h:14: // WindowDimmer creates a window that is used
to dim siblings of window.
On 2016/09/09 00:03:35, James Cook wrote:
> nit: of a window

Done.

https://codereview.chromium.org/2320273002/diff/60001/ash/common/wm_window.cc
File ash/common/wm_window.cc (right):

https://codereview.chromium.org/2320273002/diff/60001/ash/common/wm_window.cc...
ash/common/wm_window.cc:12: class WmWindow::SupportsUserDataImpl : public
base::SupportsUserData {
On 2016/09/09 00:03:35, James Cook wrote:
> Is the whole point of this to get around the protected virtual destructor of
> base::SupportsUserData? WDYT of making that destructor public so you can just
> allocate one?

I think it's better to keep SUDs destructor protected so that it's up to the
implementation to decide lifetime control. For example, some subclasses may not
want a public destructor either.

https://codereview.chromium.org/2320273002/diff/60001/ash/common/wm_window.h
File ash/common/wm_window.h (right):

https://codereview.chromium.org/2320273002/diff/60001/ash/common/wm_window.h#...
ash/common/wm_window.h:13: #include "base/supports_user_data.h"
On 2016/09/09 00:03:36, James Cook wrote:
> nit: either skip this or skip the forward declare below

Done.

https://codereview.chromium.org/2320273002/diff/60001/ash/common/wm_window.h#...
ash/common/wm_window.h:176: virtual void SetVisibilityChangesAnimated() = 0;
On 2016/09/09 00:03:36, James Cook wrote:
> nit: move above comment.
> 
> Also, a comment for this function would be nice. I guess visibility changes
> aren't animated by default?

I added a pointer to the function it mirrors.

https://codereview.chromium.org/2320273002/diff/60001/ash/mus/bridge/wm_windo...
File ash/mus/bridge/wm_window_mus.cc (right):

https://codereview.chromium.org/2320273002/diff/60001/ash/mus/bridge/wm_windo...
ash/mus/bridge/wm_window_mus.cc:208: return is_wm_window_type_set_ ?
wm_window_type_ : GetWmWindowType(window_);
On 2016/09/09 00:03:36, James Cook wrote:
> Is it not sufficient to return wm_window_type_ if it's not equal to
> WINDOW_TYPE_UNKNOWN? Or rather, you want to return WINDOW_TYPE_UNKNOWN if
> someone explicitly sets it?

Ya, sorry, this is mildly confusing. I added a comment that hopefully clarifies
things. I could have also go with setting the windowtype on the window, but that
requires a reverse mapping type, which isn't very satisfying either. See
GetWmWindowType in property_util for what I mean.

https://codereview.chromium.org/2320273002/diff/60001/ash/wm/screen_dimmer.cc
File ash/wm/screen_dimmer.cc (right):

https://codereview.chromium.org/2320273002/diff/60001/ash/wm/screen_dimmer.cc...
ash/wm/screen_dimmer.cc:98: dim = new
WindowDimmer(WmWindowAura::Get(container));
On 2016/09/09 00:03:36, James Cook wrote:
> Or maybe ScreenDimmer could store a set of WindowDimmers. The ownership here
is
> tricky -- this looks leaky, but isn't.

You're right that I could be more explicit and maintain a
map<WmWindow*,WindowDimmer> directly and get rid of the UserData. But that would
mean that logic is required for all consumers. (ScreenPinningController also
uses WindowDimmer). Going with UserData seemed less code, a better match for
what we have now, at the expense of dual ownership.

[James Cook, 2016-09-09 04:13:08]

I agree that userdata is useful for WmWindow and we should  have something like
it eventually. I like window properties.

However, I find it hard in this case to reason about lifetimes and ownership. It
also bothers me that this code needs comments explaining that clearing a
property deletes |this| and that the steps being taken to avoid calling pure
virtual methods during destruction. It would be nice if one or both of these
issues could be avoided, even at the expense of more code.

So I would prefer to avoid userdata to own dimmer windows. I'm happy to chat
about it more in the morning if you like.

[sky, 2016-09-09 20:28:32]

Ok, I refactored things so that ownership is (hopefully) clearer. Take another
look?

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

https://codereview.chromium.org/2320273002/diff/140001/ash/common/wm/window_d...
ash/common/wm/window_dimmer.h:20: class ASH_EXPORT WindowDimmer : public
WmWindowObserver {
I changed this code around so that it owns window_. Previously it didn't and
assumed window_ was owned by it's parent and deleted with it. That seems error
prone in the case of someone deleting WindowDimmer directly. I also made the
code deal with window_ being destroyed as well. That said, I made SetDimOpacity
DCHECK that window_ is valid. I went with that as I viewed the case of window_
being null as an error on the callers part, and not a case this code should deal
with.

https://codereview.chromium.org/2320273002/diff/140001/ash/wm/screen_pinning_...
File ash/wm/screen_pinning_controller.cc (left):

https://codereview.chromium.org/2320273002/diff/140001/ash/wm/screen_pinning_...
ash/wm/screen_pinning_controller.cc:157: // Tracks DimWindow's life.
This class appears to be doing two things:

If WindowDimmer's window moves to a different parent it is hidden. I moved this
logic to WindowDimmer. I actually tend to think it isn't needed, but I was a bit
nervous about nuking it entirely, so I kept it.

If WindowDimmer's window is destroyed then it called back to
ScreenPinningController::OnDimWindowDestroying so that it
ScreenPinningController could cleanup dim_windows_. This was a roundabout way of
cleanup. Now WindowDimmer owns the window, so manual management isn't necessary.
WindowDimmers are now tracked by WmWindowUserData, so that if the container is
deleted the WindowDimmer is deleted as well. In other words, this code should no
longer be necessary.

https://codereview.chromium.org/2320273002/diff/140001/ash/wm/screen_pinning_...
ash/wm/screen_pinning_controller.cc:268: // Delete the dim windows. This works,
but do not use RAII, because
The call to clear on 173 does does this.

https://codereview.chromium.org/2320273002/diff/140001/ash/wm/system_modal_co...
File ash/wm/system_modal_container_layout_manager.cc (left):

https://codereview.chromium.org/2320273002/diff/140001/ash/wm/system_modal_co...
ash/wm/system_modal_container_layout_manager.cc:56: DCHECK(child ==
modal_background_ ||
I don't think this was ever true as what was modal_background_ was added
before modal_background_ was assigned (WindowDimmer's constructor adds the
window to the parent).

[sky, 2016-09-09 20:28:39]

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

[commit-bot: I haz the power, 2016-09-09 20:29:07]

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-09 20:49:32]

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

[commit-bot: I haz the power, 2016-09-09 20:49:33]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[James Cook, 2016-09-09 21:33:37]

LGTM with nits.

https://codereview.chromium.org/2320273002/diff/140001/ash/aura/wm_window_aur...
File ash/aura/wm_window_aura.cc (right):

https://codereview.chromium.org/2320273002/diff/140001/ash/aura/wm_window_aur...
ash/aura/wm_window_aura.cc:140: }
set window_ = nullptr here?

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

https://codereview.chromium.org/2320273002/diff/140001/ash/common/wm/window_d...
ash/common/wm/window_dimmer.h:19: // obscured.
Maybe comment on the usual lifetime, specifically that it can be destroyed when
a controller like ScreenDimmer deletes it, or when the container window it is
blocking is destroyed? I know that's implied by storage in WmWindowUserData, but
I think it's unusual enough that it's worth a note.

https://codereview.chromium.org/2320273002/diff/140001/ash/common/wm/window_d...
ash/common/wm/window_dimmer.h:20: class ASH_EXPORT WindowDimmer : public
WmWindowObserver {
On 2016/09/09 20:28:32, sky wrote:
> I changed this code around so that it owns window_. Previously it didn't and
> assumed window_ was owned by it's parent and deleted with it. That seems error
> prone in the case of someone deleting WindowDimmer directly. I also made the
> code deal with window_ being destroyed as well. That said, I made
SetDimOpacity
> DCHECK that window_ is valid. I went with that as I viewed the case of window_
> being null as an error on the callers part, and not a case this code should
deal
> with.

Acknowledged.

https://codereview.chromium.org/2320273002/diff/140001/ash/common/wm/window_d...
ash/common/wm/window_dimmer.h:32: // NOTE: WindowDimmer is an observer for both
|parent_| and |window_|.
Thanks for documenting this.

https://codereview.chromium.org/2320273002/diff/140001/ash/common/wm_window_u...
File ash/common/wm_window_user_data_unittest.cc (right):

https://codereview.chromium.org/2320273002/diff/140001/ash/common/wm_window_u...
ash/common/wm_window_user_data_unittest.cc:79: }
Nice test suite. Complete and easy to read.

https://codereview.chromium.org/2320273002/diff/140001/ash/wm/screen_dimmer.cc
File ash/wm/screen_dimmer.cc (right):

https://codereview.chromium.org/2320273002/diff/140001/ash/wm/screen_dimmer.c...
ash/wm/screen_dimmer.cc:98: WindowDimmer* window_dimmer =
window_dimmers_->Get(container);
WDYT of WmWindowUserData::Get() being named either GetForWindow() or GetData()?

https://codereview.chromium.org/2320273002/diff/140001/ash/wm/screen_pinning_...
File ash/wm/screen_pinning_controller.cc (left):

https://codereview.chromium.org/2320273002/diff/140001/ash/wm/screen_pinning_...
ash/wm/screen_pinning_controller.cc:157: // Tracks DimWindow's life.
On 2016/09/09 20:28:32, sky wrote:
> This class appears to be doing two things:
> 
> If WindowDimmer's window moves to a different parent it is hidden. I moved
this
> logic to WindowDimmer. I actually tend to think it isn't needed, but I was a
bit
> nervous about nuking it entirely, so I kept it.
> 
> If WindowDimmer's window is destroyed then it called back to
> ScreenPinningController::OnDimWindowDestroying so that it
> ScreenPinningController could cleanup dim_windows_. This was a roundabout way
of
> cleanup. Now WindowDimmer owns the window, so manual management isn't
necessary.
> WindowDimmers are now tracked by WmWindowUserData, so that if the container is
> deleted the WindowDimmer is deleted as well. In other words, this code should
no
> longer be necessary.

Acknowledged.

[sky, 2016-09-09 21:50:36]

The CQ bit was checked by sky@chromium.org

[sky, 2016-09-09 21:50:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2320273002/#ps160001
(title: "feedback and member initializer ordering")

[sky, 2016-09-09 21:50:45]

https://codereview.chromium.org/2320273002/diff/140001/ash/aura/wm_window_aur...
File ash/aura/wm_window_aura.cc (right):

https://codereview.chromium.org/2320273002/diff/140001/ash/aura/wm_window_aur...
ash/aura/wm_window_aura.cc:140: }
On 2016/09/09 21:33:36, James Cook wrote:
> set window_ = nullptr here?

'this' should be deleted as a result of delete window_. I'll add a comment.

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

https://codereview.chromium.org/2320273002/diff/140001/ash/common/wm/window_d...
ash/common/wm/window_dimmer.h:19: // obscured.
On 2016/09/09 21:33:36, James Cook wrote:
> Maybe comment on the usual lifetime, specifically that it can be destroyed
when
> a controller like ScreenDimmer deletes it, or when the container window it is
> blocking is destroyed? I know that's implied by storage in WmWindowUserData,
but
> I think it's unusual enough that it's worth a note.

Done.

https://codereview.chromium.org/2320273002/diff/140001/ash/wm/screen_dimmer.cc
File ash/wm/screen_dimmer.cc (right):

https://codereview.chromium.org/2320273002/diff/140001/ash/wm/screen_dimmer.c...
ash/wm/screen_dimmer.cc:98: WindowDimmer* window_dimmer =
window_dimmers_->Get(container);
On 2016/09/09 21:33:37, James Cook wrote:
> WDYT of WmWindowUserData::Get() being named either GetForWindow() or
GetData()?

I view WmWindowUserData close to a map, in which case set/get seems preferred.

[commit-bot: I haz the power, 2016-09-09 21:51:14]

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-09 23:18:59]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-09-09 23:20:56]

Description was changed from

==========
Refactors DimWindow and moves to ash/common

DimWindow was both an aura::Window and had some logic. As that won't
work at all in mash (and we generally don't subclass aura::Windows)
I've converted it to have a WmWindow. I also renamed it to
WindowDimmer.

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

to

==========
Refactors DimWindow and moves to ash/common

DimWindow was both an aura::Window and had some logic. As that won't
work at all in mash (and we generally don't subclass aura::Windows)
I've converted it to have a WmWindow. I also renamed it to
WindowDimmer.

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

Committed: https://crrev.com/b733d4184e6a91eebd951cae60c871118d4cc2a2
Cr-Commit-Position: refs/heads/master@{#417755}
==========

[commit-bot: I haz the power, 2016-09-09 23:20:57]

Patchset 9 (id:??) landed as
https://crrev.com/b733d4184e6a91eebd951cae60c871118d4cc2a2
Cr-Commit-Position: refs/heads/master@{#417755}