Make LogoutButtonTray a regular View

Make LogoutButtonTray a regular View

Currently, LogoutButtonTray is a TrayBackgroundView, however, it does
not use most of its functionality (e.g., drawing and animating
background, handling bubbles, drawing separators and focus rings). This
CL makes LogoutButtonTray directly inherit from View and just adds the
padding using TrayContainer class.

This CL also removes shelf alignment caching in TrayBackgroundView,
TrayContainer, and StatusAreaWidgetDelegate and cleaned up some related
shelf alignment code.

BUG=698134, 635963
TEST=StatusAreaWidgetTest.Basic in ash_unittests

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

[mohsen, 2017-04-07 18:19:55]

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

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

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-07 20:54:16]

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

[commit-bot: I haz the power, 2017-04-07 20:54:16]

Dry run: This issue passed the CQ dry run.

[mohsen, 2017-04-10 18:11:51]

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

[commit-bot: I haz the power, 2017-04-10 18:12:57]

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-10 18:17:29]

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

[commit-bot: I haz the power, 2017-04-10 18:17:30]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mohsen, 2017-04-11 05:14:30]

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

[commit-bot: I haz the power, 2017-04-11 05:14:49]

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

[mohsen, 2017-04-11 05:15:39]

Description was changed from

==========
Make LogoutButtonTray a regular View

Currently, LogoutButtonTray is a TrayBackgroundView, however, it does
not use most of its functionality; e.g., drawing and animating
background, handling bubbles, drawing separators and focus rings). This
CL makes LogoutButtonTray directly inherit from View and just adds the
padding using TrayContainer class.

BUG=698134
TEST=StatusAreaWidgetTest.Basic in ash_unittests
==========

to

==========
Make LogoutButtonTray a regular View

Currently, LogoutButtonTray is a TrayBackgroundView, however, it does
not use most of its functionality (e.g., drawing and animating
background, handling bubbles, drawing separators and focus rings). This
CL makes LogoutButtonTray directly inherit from View and just adds the
padding using TrayContainer class.

BUG=698134
TEST=StatusAreaWidgetTest.Basic in ash_unittests
==========

[commit-bot: I haz the power, 2017-04-11 06:14:19]

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

[commit-bot: I haz the power, 2017-04-11 06:14:20]

Dry run: This issue passed the CQ dry run.

[mohsen, 2017-04-13 18:14:04]

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

[mohsen, 2017-04-13 18:14:05]

PTAL (Let's try this instead of crrev/2795363002)

[James Cook, 2017-04-17 16:54:35]

LGTM assuming shelf alignment still works OK if you log in with a non-bottom
alignment.

Also, please mention issue 635963 in the BUG line, since it looks like you fixed
most (all?) of it.

Nice CL!

https://codereview.chromium.org/2807693002/diff/40001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager_unittest.cc (right):

https://codereview.chromium.org/2807693002/diff/40001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager_unittest.cc:395: else if (SHELF_ALIGNMENT_LEFT ==
shelf->alignment())
Thanks for fixing these.

https://codereview.chromium.org/2807693002/diff/40001/ash/system/palette/pale...
File ash/system/palette/palette_tray.cc (right):

https://codereview.chromium.org/2807693002/diff/40001/ash/system/palette/pale...
ash/system/palette/palette_tray.cc:25: #include
"ash/system/tray/tray_container.h"
Is this needed?

https://codereview.chromium.org/2807693002/diff/40001/ash/system/tray/system_...
File ash/system/tray/system_tray.cc (left):

https://codereview.chromium.org/2807693002/diff/40001/ash/system/tray/system_...
ash/system/tray/system_tray.cc:388: // Items default to SHELF_ALIGNMENT_BOTTOM.
Update them if the initial
Do things still work OK if you log in with a shelf alignment that isn't bottom?

https://codereview.chromium.org/2807693002/diff/40001/ash/system/tray/tray_ba...
File ash/system/tray/tray_background_view.h (left):

https://codereview.chromium.org/2807693002/diff/40001/ash/system/tray/tray_ba...
ash/system/tray/tray_background_view.h:115: ShelfAlignment shelf_alignment()
const { return shelf_alignment_; }
Please update CL description to mention changes to shelf alignment caching. It
will make it easier to identify this CL if you cause a regression.

(Also, I was surprised when I first started reviewing this CL to see the
alignment changes.)

https://codereview.chromium.org/2807693002/diff/40001/ash/system/tray/tray_ba...
ash/system/tray/tray_background_view.h:170: ShelfAlignment shelf_alignment_;
Thanks for taking care of removing this.

https://codereview.chromium.org/2807693002/diff/40001/ash/system/tray/tray_co...
File ash/system/tray/tray_container.cc (right):

https://codereview.chromium.org/2807693002/diff/40001/ash/system/tray/tray_co...
ash/system/tray/tray_container.cc:18: UpdateLayout();
nit: DCHECK(wm_shelf_)

I like formal preconditions. :-)

https://codereview.chromium.org/2807693002/diff/40001/ash/system/tray/tray_co...
ash/system/tray/tray_container.cc:46: bool is_horizontal =
wm_shelf_->IsHorizontalAlignment();
nit: const

https://codereview.chromium.org/2807693002/diff/40001/ash/system/tray/tray_co...
File ash/system/tray/tray_container.h (right):

https://codereview.chromium.org/2807693002/diff/40001/ash/system/tray/tray_co...
ash/system/tray/tray_container.h:19: ~TrayContainer() override {}
nit: prefer to define {} in the .cc file

https://codereview.chromium.org/2807693002/diff/40001/ash/system/web_notifica...
File ash/system/web_notification/web_notification_tray.cc (right):

https://codereview.chromium.org/2807693002/diff/40001/ash/system/web_notifica...
ash/system/web_notification/web_notification_tray.cc:21: #include
"ash/system/tray/tray_container.h"
Is this needed?

[mohsen, 2017-04-19 22:37:52]

Description was changed from

==========
Make LogoutButtonTray a regular View

Currently, LogoutButtonTray is a TrayBackgroundView, however, it does
not use most of its functionality (e.g., drawing and animating
background, handling bubbles, drawing separators and focus rings). This
CL makes LogoutButtonTray directly inherit from View and just adds the
padding using TrayContainer class.

BUG=698134
TEST=StatusAreaWidgetTest.Basic in ash_unittests
==========

to

==========
Make LogoutButtonTray a regular View

Currently, LogoutButtonTray is a TrayBackgroundView, however, it does
not use most of its functionality (e.g., drawing and animating
background, handling bubbles, drawing separators and focus rings). This
CL makes LogoutButtonTray directly inherit from View and just adds the
padding using TrayContainer class.

This CL also removes shelf alignment caching in TrayBackgroundView,
TrayContainer, and StatusAreaWidgetDelegate and cleaned up some related
shelf alignment code.

BUG=698134
TEST=StatusAreaWidgetTest.Basic in ash_unittests
==========

[mohsen, 2017-04-19 22:42:30]

Description was changed from

==========
Make LogoutButtonTray a regular View

Currently, LogoutButtonTray is a TrayBackgroundView, however, it does
not use most of its functionality (e.g., drawing and animating
background, handling bubbles, drawing separators and focus rings). This
CL makes LogoutButtonTray directly inherit from View and just adds the
padding using TrayContainer class.

This CL also removes shelf alignment caching in TrayBackgroundView,
TrayContainer, and StatusAreaWidgetDelegate and cleaned up some related
shelf alignment code.

BUG=698134
TEST=StatusAreaWidgetTest.Basic in ash_unittests
==========

to

==========
Make LogoutButtonTray a regular View

Currently, LogoutButtonTray is a TrayBackgroundView, however, it does
not use most of its functionality (e.g., drawing and animating
background, handling bubbles, drawing separators and focus rings). This
CL makes LogoutButtonTray directly inherit from View and just adds the
padding using TrayContainer class.

This CL also removes shelf alignment caching in TrayBackgroundView,
TrayContainer, and StatusAreaWidgetDelegate and cleaned up some related
shelf alignment code.

BUG=698134,635963
TEST=StatusAreaWidgetTest.Basic in ash_unittests
==========

[mohsen, 2017-04-19 22:56:13]

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

[commit-bot: I haz the power, 2017-04-19 22:57:02]

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

[mohsen, 2017-04-19 22:59:08]

https://codereview.chromium.org/2807693002/diff/40001/ash/system/palette/pale...
File ash/system/palette/palette_tray.cc (right):

https://codereview.chromium.org/2807693002/diff/40001/ash/system/palette/pale...
ash/system/palette/palette_tray.cc:25: #include
"ash/system/tray/tray_container.h"
On 2017/04/17 at 16:54:35, James Cook wrote:
> Is this needed?

Yes, this file works with TrayBackgroundView::tray_container() which is
forward-declared in tray_background_view.h. Previously, the full declaration was
in tray_background_view.h.

https://codereview.chromium.org/2807693002/diff/40001/ash/system/tray/system_...
File ash/system/tray/system_tray.cc (left):

https://codereview.chromium.org/2807693002/diff/40001/ash/system/tray/system_...
ash/system/tray/system_tray.cc:388: // Items default to SHELF_ALIGNMENT_BOTTOM.
Update them if the initial
On 2017/04/17 at 16:54:35, James Cook wrote:
> Do things still work OK if you log in with a shelf alignment that isn't
bottom?

As far as I can tell, UpdateAfterShelfAlignmentChange (before this CL,
SetShelfAlignment) is called if shelf alignment is different after login.

I tested both old and new code with the following code removed and both worked
correctly for non-bottom shelf alignments.

https://codereview.chromium.org/2807693002/diff/40001/ash/system/tray/tray_ba...
File ash/system/tray/tray_background_view.h (left):

https://codereview.chromium.org/2807693002/diff/40001/ash/system/tray/tray_ba...
ash/system/tray/tray_background_view.h:115: ShelfAlignment shelf_alignment()
const { return shelf_alignment_; }
On 2017/04/17 at 16:54:35, James Cook wrote:
> Please update CL description to mention changes to shelf alignment caching. It
will make it easier to identify this CL if you cause a regression.
> 
> (Also, I was surprised when I first started reviewing this CL to see the
alignment changes.)

Done.

Because, the updated LogoutButtonTray did not cache alignment, I thought of
removing caching from TrayBackgroundView, too, to make them more consistent.

https://codereview.chromium.org/2807693002/diff/40001/ash/system/tray/tray_co...
File ash/system/tray/tray_container.cc (right):

https://codereview.chromium.org/2807693002/diff/40001/ash/system/tray/tray_co...
ash/system/tray/tray_container.cc:18: UpdateLayout();
On 2017/04/17 at 16:54:35, James Cook wrote:
> nit: DCHECK(wm_shelf_)
> 
> I like formal preconditions. :-)

Done.

https://codereview.chromium.org/2807693002/diff/40001/ash/system/tray/tray_co...
ash/system/tray/tray_container.cc:46: bool is_horizontal =
wm_shelf_->IsHorizontalAlignment();
On 2017/04/17 at 16:54:35, James Cook wrote:
> nit: const

Done.

https://codereview.chromium.org/2807693002/diff/40001/ash/system/tray/tray_co...
File ash/system/tray/tray_container.h (right):

https://codereview.chromium.org/2807693002/diff/40001/ash/system/tray/tray_co...
ash/system/tray/tray_container.h:19: ~TrayContainer() override {}
On 2017/04/17 at 16:54:35, James Cook wrote:
> nit: prefer to define {} in the .cc file

Done.

https://codereview.chromium.org/2807693002/diff/40001/ash/system/web_notifica...
File ash/system/web_notification/web_notification_tray.cc (right):

https://codereview.chromium.org/2807693002/diff/40001/ash/system/web_notifica...
ash/system/web_notification/web_notification_tray.cc:21: #include
"ash/system/tray/tray_container.h"
On 2017/04/17 at 16:54:35, James Cook wrote:
> Is this needed?

Yes, this file works with TrayBackgroundView::tray_container() which is
forward-declared in tray_background_view.h. Previously, the full declaration was
in tray_background_view.h.

[commit-bot: I haz the power, 2017-04-19 23:01:27]

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

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

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[James Cook, 2017-04-19 23:17:23]

LGTM! Thanks for taking care of all the shelf alignment caching.

[mohsen, 2017-04-20 03:52:10]

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

[commit-bot: I haz the power, 2017-04-20 03:52:23]

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-20 04:52:07]

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

[commit-bot: I haz the power, 2017-04-20 04:52:08]

Dry run: This issue passed the CQ dry run.

[mohsen, 2017-04-20 04:56:44]

The CQ bit was checked by mohsen@chromium.org

[mohsen, 2017-04-20 04:56:44]

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/2807693002/#ps80001
(title: "Rebased")

[commit-bot: I haz the power, 2017-04-20 04:57:20]

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-20 05:02:56]

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

[commit-bot: I haz the power, 2017-04-20 05:02:58]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[mohsen, 2017-04-20 05:30:19]

The CQ bit was checked by mohsen@chromium.org

[mohsen, 2017-04-20 05:30:19]

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/2807693002/#ps100001
(title: "Rebased")

[commit-bot: I haz the power, 2017-04-20 05:30:36]

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-20 06:30:23]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1492666219090830,
"parent_rev": "b72ac686eb305a9201b6410c8918637415d204fc", "commit_rev":
"e6704a0c2b29585fab489399ba0396104cff7d85"}

[commit-bot: I haz the power, 2017-04-20 06:30:36]

Description was changed from

==========
Make LogoutButtonTray a regular View

Currently, LogoutButtonTray is a TrayBackgroundView, however, it does
not use most of its functionality (e.g., drawing and animating
background, handling bubbles, drawing separators and focus rings). This
CL makes LogoutButtonTray directly inherit from View and just adds the
padding using TrayContainer class.

This CL also removes shelf alignment caching in TrayBackgroundView,
TrayContainer, and StatusAreaWidgetDelegate and cleaned up some related
shelf alignment code.

BUG=698134,635963
TEST=StatusAreaWidgetTest.Basic in ash_unittests
==========

to

==========
Make LogoutButtonTray a regular View

Currently, LogoutButtonTray is a TrayBackgroundView, however, it does
not use most of its functionality (e.g., drawing and animating
background, handling bubbles, drawing separators and focus rings). This
CL makes LogoutButtonTray directly inherit from View and just adds the
padding using TrayContainer class.

This CL also removes shelf alignment caching in TrayBackgroundView,
TrayContainer, and StatusAreaWidgetDelegate and cleaned up some related
shelf alignment code.

BUG=698134,635963
TEST=StatusAreaWidgetTest.Basic in ash_unittests

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

[commit-bot: I haz the power, 2017-04-20 06:30:37]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/e6704a0c2b29585fab489399ba03...