mash: Move the shelf DimmerView out of shelf_widget.cc.

mash: Move the shelf DimmerView out of shelf_widget.cc.

Move DimmerView to ash/shelf/dimmer_view.[h|cc].
Add a static Create helper for View&Widget creation.

Convert aura::WindowOberserver->WmWindowObserver.
(and move observation from ShelfWidget to DimmerView)

Nix ShelfWidget's pointer to the DimmerView's widget.
Make the dimmer widget be owned by its native widget.

Pass WmShelf not ShelfWidget to DimmerView.
Undim on ShelfWidget::Shutdown.

BUG=615155
TEST=Automated tests; no Chrome OS shelf dimmer changes.
R=jamescook@chromium.org

Committed: https://crrev.com/36af7b5e3aa2816d67e720bfd633029b82a724f1
Cr-Commit-Position: refs/heads/master@{#409558}

[msw, 2016-08-02 23:02:03]

Description was changed from

==========
mash: Move the shelf DimmerView out of shelf_widget.cc.

BUG=615155
==========

to

==========
mash: Move the shelf DimmerView out of shelf_widget.cc.

Move DimmerView to ash/shelf/dimmer_view.[h|cc].
Add a static Create helper for View&Widget creation.

Convert aura::WindowOberserver->WmWindowObserver.
(and move observation from ShelfWidget to DimmerView)

Nix ShelfWidget's pointer to the DimmerView's widget.
Make the dimmer widget be owned by its native widget.

Pass WmShelf not ShelfWidget to DimmerView.
Undim on ShelfWidget::Shutdown.

BUG=615155
TEST=Automated tests; no Chrome OS shelf dimmer changes.
R=jamescook@chromium.org
==========

[msw, 2016-08-02 23:02:03]

msw@chromium.org changed reviewers:
+ jamescook@chromium.org

[msw, 2016-08-02 23:02:16]

Hey James, please take a look; thanks!

[msw, 2016-08-02 23:02:22]

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

[commit-bot: I haz the power, 2016-08-02 23:03:10]

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

[msw, 2016-08-02 23:05:15]

+CC bruthig@ for heads up.

[msw, 2016-08-02 23:06:31]

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

[commit-bot: I haz the power, 2016-08-02 23:07:03]

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-08-02 23:53:03]

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

[commit-bot: I haz the power, 2016-08-02 23:53:04]

Dry run: This issue passed the CQ dry run.

[James Cook, 2016-08-03 03:09:11]

LGTM with nits.

https://codereview.chromium.org/2204843003/diff/60001/ash/shelf/dimmer_view.cc
File ash/shelf/dimmer_view.cc (right):

https://codereview.chromium.org/2204843003/diff/60001/ash/shelf/dimmer_view.c...
ash/shelf/dimmer_view.cc:43: dimmer->GetNativeWindow()->SetName("ShelfDimmer");
nit: use params.name = "ShelfDimmer";

I'm trying to push new code in that direction.

https://codereview.chromium.org/2204843003/diff/60001/ash/shelf/dimmer_view.c...
ash/shelf/dimmer_view.cc:79: // Undimm also if we were forced to by e.g. an open
menu.
nit: Undimm -> Undim (not your fault, I know)

https://codereview.chromium.org/2204843003/diff/60001/ash/shelf/dimmer_view.h
File ash/shelf/dimmer_view.h (right):

https://codereview.chromium.org/2204843003/diff/60001/ash/shelf/dimmer_view.h...
ash/shelf/dimmer_view.h:4: 
header guards please

https://codereview.chromium.org/2204843003/diff/60001/ash/shelf/dimmer_view.h...
ash/shelf/dimmer_view.h:8: #include "ui/views/widget/widget_delegate.h"
base/macros.h too

https://codereview.chromium.org/2204843003/diff/60001/ash/shelf/dimmer_view.h...
ash/shelf/dimmer_view.h:9: 
event_handler.h too

https://codereview.chromium.org/2204843003/diff/60001/ash/shelf/dimmer_view.h...
ash/shelf/dimmer_view.h:14: // Class used to slightly dim shelf items when
maximized and visible.
nit: "when a window is maximized and visible"

https://codereview.chromium.org/2204843003/diff/60001/ash/shelf/dimmer_view.h...
ash/shelf/dimmer_view.h:24: DimmerView(WmShelf* shelf, bool
disable_animations_for_test);
Could these be private, or are they accessed from outside?

https://codereview.chromium.org/2204843003/diff/60001/ash/shelf/dimmer_view.h...
ash/shelf/dimmer_view.h:41: // views::View overrides:
nit: make the overridden function declaration order match the order of the
subclasses in the "class DimmerView" declaration.

https://codereview.chromium.org/2204843003/diff/60001/ash/shelf/dimmer_view.h...
ash/shelf/dimmer_view.h:71: // TRUE if the mouse is inside the shelf.
nit: TRUE -> True, and below. Some previous code author apparently liked to
shout. :-)

https://codereview.chromium.org/2204843003/diff/60001/ash/shelf/shelf_widget.cc
File ash/shelf/shelf_widget.cc (right):

https://codereview.chromium.org/2204843003/diff/60001/ash/shelf/shelf_widget....
ash/shelf/shelf_widget.cc:209: void ShelfWidget::DelegateView::SetDimmed(bool
value) {
optional nit: Rename "value" to "dimmed" or similar.

https://codereview.chromium.org/2204843003/diff/60001/ash/shelf/shelf_widget....
ash/shelf/shelf_widget.cc:217: dimmer_view_->GetWidget()->CloseNow();
I like how you reversed the ownership of the widget. I'm thinking of doing
something similar with DragImageView.

https://codereview.chromium.org/2204843003/diff/60001/ash/shelf/shelf_widget....
ash/shelf/shelf_widget.cc:533: // Tear down the dimmer before
|shelf_layout_manager_| (used to get WmWindow).
nit: I don't understand the "(used to get WmWindow)" part. Clarify?

https://codereview.chromium.org/2204843003/diff/60001/ash/shelf/shelf_widget.h
File ash/shelf/shelf_widget.h (right):

https://codereview.chromium.org/2204843003/diff/60001/ash/shelf/shelf_widget....
ash/shelf/shelf_widget.h:107: friend class DelegateView;
Q: Which access needs the friend?

[msw, 2016-08-03 16:48:24]

Thanks for all the good comments!

https://codereview.chromium.org/2204843003/diff/60001/ash/shelf/dimmer_view.cc
File ash/shelf/dimmer_view.cc (right):

https://codereview.chromium.org/2204843003/diff/60001/ash/shelf/dimmer_view.c...
ash/shelf/dimmer_view.cc:43: dimmer->GetNativeWindow()->SetName("ShelfDimmer");
On 2016/08/03 03:09:10, James Cook (OOO until 8-5) wrote:
> nit: use params.name = "ShelfDimmer";
> 
> I'm trying to push new code in that direction.

Done.

https://codereview.chromium.org/2204843003/diff/60001/ash/shelf/dimmer_view.c...
ash/shelf/dimmer_view.cc:79: // Undimm also if we were forced to by e.g. an open
menu.
On 2016/08/03 03:09:10, James Cook (OOO until 8-5) wrote:
> nit: Undimm -> Undim (not your fault, I know)

Done.

https://codereview.chromium.org/2204843003/diff/60001/ash/shelf/dimmer_view.h
File ash/shelf/dimmer_view.h (right):

https://codereview.chromium.org/2204843003/diff/60001/ash/shelf/dimmer_view.h...
ash/shelf/dimmer_view.h:4: 
On 2016/08/03 03:09:11, James Cook (OOO until 8-5) wrote:
> header guards please

Done.

https://codereview.chromium.org/2204843003/diff/60001/ash/shelf/dimmer_view.h...
ash/shelf/dimmer_view.h:8: #include "ui/views/widget/widget_delegate.h"
On 2016/08/03 03:09:11, James Cook (OOO until 8-5) wrote:
> base/macros.h too

Done.

https://codereview.chromium.org/2204843003/diff/60001/ash/shelf/dimmer_view.h...
ash/shelf/dimmer_view.h:9: 
On 2016/08/03 03:09:10, James Cook (OOO until 8-5) wrote:
> event_handler.h too

Done.

https://codereview.chromium.org/2204843003/diff/60001/ash/shelf/dimmer_view.h...
ash/shelf/dimmer_view.h:14: // Class used to slightly dim shelf items when
maximized and visible.
On 2016/08/03 03:09:11, James Cook (OOO until 8-5) wrote:
> nit: "when a window is maximized and visible"

Done.

https://codereview.chromium.org/2204843003/diff/60001/ash/shelf/dimmer_view.h...
ash/shelf/dimmer_view.h:24: DimmerView(WmShelf* shelf, bool
disable_animations_for_test);
On 2016/08/03 03:09:11, James Cook (OOO until 8-5) wrote:
> Could these be private, or are they accessed from outside?

Done.

https://codereview.chromium.org/2204843003/diff/60001/ash/shelf/dimmer_view.h...
ash/shelf/dimmer_view.h:41: // views::View overrides:
On 2016/08/03 03:09:10, James Cook (OOO until 8-5) wrote:
> nit: make the overridden function declaration order match the order of the
> subclasses in the "class DimmerView" declaration.

Done.

https://codereview.chromium.org/2204843003/diff/60001/ash/shelf/dimmer_view.h...
ash/shelf/dimmer_view.h:71: // TRUE if the mouse is inside the shelf.
On 2016/08/03 03:09:11, James Cook (OOO until 8-5) wrote:
> nit: TRUE -> True, and below. Some previous code author apparently liked to
> shout. :-)

Done.

https://codereview.chromium.org/2204843003/diff/60001/ash/shelf/shelf_widget.cc
File ash/shelf/shelf_widget.cc (right):

https://codereview.chromium.org/2204843003/diff/60001/ash/shelf/shelf_widget....
ash/shelf/shelf_widget.cc:209: void ShelfWidget::DelegateView::SetDimmed(bool
value) {
On 2016/08/03 03:09:11, James Cook (OOO until 8-5) wrote:
> optional nit: Rename "value" to "dimmed" or similar.

Done.

https://codereview.chromium.org/2204843003/diff/60001/ash/shelf/shelf_widget....
ash/shelf/shelf_widget.cc:217: dimmer_view_->GetWidget()->CloseNow();
On 2016/08/03 03:09:11, James Cook (OOO until 8-5) wrote:
> I like how you reversed the ownership of the widget. I'm thinking of doing
> something similar with DragImageView.

Acknowledged.

https://codereview.chromium.org/2204843003/diff/60001/ash/shelf/shelf_widget....
ash/shelf/shelf_widget.cc:533: // Tear down the dimmer before
|shelf_layout_manager_| (used to get WmWindow).
On 2016/08/03 03:09:11, James Cook (OOO until 8-5) wrote:
> nit: I don't understand the "(used to get WmWindow)" part. Clarify?

Done.

https://codereview.chromium.org/2204843003/diff/60001/ash/shelf/shelf_widget.h
File ash/shelf/shelf_widget.h (right):

https://codereview.chromium.org/2204843003/diff/60001/ash/shelf/shelf_widget....
ash/shelf/shelf_widget.h:107: friend class DelegateView;
On 2016/08/03 03:09:11, James Cook (OOO until 8-5) wrote:
> Q: Which access needs the friend?

It passes |wm_shelf_aura_| to the DimmerView.

[msw, 2016-08-03 16:48:31]

The CQ bit was checked by msw@chromium.org

[msw, 2016-08-03 16:48:31]

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/2204843003/#ps80001
(title: "Address comments.")

[commit-bot: I haz the power, 2016-08-03 16:49:10]

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-08-03 17:31:28]

Description was changed from

==========
mash: Move the shelf DimmerView out of shelf_widget.cc.

Move DimmerView to ash/shelf/dimmer_view.[h|cc].
Add a static Create helper for View&Widget creation.

Convert aura::WindowOberserver->WmWindowObserver.
(and move observation from ShelfWidget to DimmerView)

Nix ShelfWidget's pointer to the DimmerView's widget.
Make the dimmer widget be owned by its native widget.

Pass WmShelf not ShelfWidget to DimmerView.
Undim on ShelfWidget::Shutdown.

BUG=615155
TEST=Automated tests; no Chrome OS shelf dimmer changes.
R=jamescook@chromium.org
==========

to

==========
mash: Move the shelf DimmerView out of shelf_widget.cc.

Move DimmerView to ash/shelf/dimmer_view.[h|cc].
Add a static Create helper for View&Widget creation.

Convert aura::WindowOberserver->WmWindowObserver.
(and move observation from ShelfWidget to DimmerView)

Nix ShelfWidget's pointer to the DimmerView's widget.
Make the dimmer widget be owned by its native widget.

Pass WmShelf not ShelfWidget to DimmerView.
Undim on ShelfWidget::Shutdown.

BUG=615155
TEST=Automated tests; no Chrome OS shelf dimmer changes.
R=jamescook@chromium.org
==========

[commit-bot: I haz the power, 2016-08-03 17:31:29]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-03 17:34:17]

Description was changed from

==========
mash: Move the shelf DimmerView out of shelf_widget.cc.

Move DimmerView to ash/shelf/dimmer_view.[h|cc].
Add a static Create helper for View&Widget creation.

Convert aura::WindowOberserver->WmWindowObserver.
(and move observation from ShelfWidget to DimmerView)

Nix ShelfWidget's pointer to the DimmerView's widget.
Make the dimmer widget be owned by its native widget.

Pass WmShelf not ShelfWidget to DimmerView.
Undim on ShelfWidget::Shutdown.

BUG=615155
TEST=Automated tests; no Chrome OS shelf dimmer changes.
R=jamescook@chromium.org
==========

to

==========
mash: Move the shelf DimmerView out of shelf_widget.cc.

Move DimmerView to ash/shelf/dimmer_view.[h|cc].
Add a static Create helper for View&Widget creation.

Convert aura::WindowOberserver->WmWindowObserver.
(and move observation from ShelfWidget to DimmerView)

Nix ShelfWidget's pointer to the DimmerView's widget.
Make the dimmer widget be owned by its native widget.

Pass WmShelf not ShelfWidget to DimmerView.
Undim on ShelfWidget::Shutdown.

BUG=615155
TEST=Automated tests; no Chrome OS shelf dimmer changes.
R=jamescook@chromium.org

Committed: https://crrev.com/36af7b5e3aa2816d67e720bfd633029b82a724f1
Cr-Commit-Position: refs/heads/master@{#409558}
==========

[commit-bot: I haz the power, 2016-08-03 17:34:18]

Patchset 5 (id:??) landed as
https://crrev.com/36af7b5e3aa2816d67e720bfd633029b82a724f1
Cr-Commit-Position: refs/heads/master@{#409558}