MD cros - Fix padding in system tray

MD cros - Fix padding in system tray

BUG=653292
Committed: https://crrev.com/78cffd731645283982ec8821a054b67317ee3b8a
Cr-Commit-Position: refs/heads/master@{#429011}

[Evan Stade, 2016-10-28 23:41:38]

estade@chromium.org changed reviewers:
+ tdanderson@chromium.org

[Evan Stade, 2016-10-28 23:41:38]

It took me way longer than it should have to come up with this solution.

[Evan Stade, 2016-10-28 23:41:42]

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

[commit-bot: I haz the power, 2016-10-28 23:42:10]

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

[Evan Stade, 2016-10-28 23:43:11]

https://screenshot.googleplex.com/fkSno3eDj0h.png

[commit-bot: I haz the power, 2016-10-29 00:19:05]

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

[commit-bot: I haz the power, 2016-10-29 00:19:06]

Dry run: This issue passed the CQ dry run.

[tdanderson, 2016-10-31 21:29:09]

A couple of questions below:

https://codereview.chromium.org/2461693003/diff/20001/ash/common/system/tray/...
File ash/common/system/tray/system_tray.cc (right):

https://codereview.chromium.org/2461693003/diff/20001/ash/common/system/tray/...
ash/common/system/tray/system_tray.cc:85: // Overridden from SystemTrayItem.
nit: just "SystemTrayItem:"

https://codereview.chromium.org/2461693003/diff/20001/ash/common/system/user/...
File ash/common/system/user/tray_user.cc (left):

https://codereview.chromium.org/2461693003/diff/20001/ash/common/system/user/...
ash/common/system/user/tray_user.cc:178: avatar_->SetCornerRadii(0,
kTrayRoundedBorderRadius,
Won't removing this sacrifice the rounded corners of the avatar in the tray
(which I think we want to keep)?

https://codereview.chromium.org/2461693003/diff/20001/ash/common/system/user/...
File ash/common/system/user/tray_user.cc (right):

https://codereview.chromium.org/2461693003/diff/20001/ash/common/system/user/...
ash/common/system/user/tray_user.cc:153: delete avatar_;
Why the change to using delete here? Wouldn't it be safer to stick with
RemoveAllChildViews()?

[Evan Stade, 2016-10-31 22:11:57]

https://codereview.chromium.org/2461693003/diff/20001/ash/common/system/tray/...
File ash/common/system/tray/system_tray.cc (right):

https://codereview.chromium.org/2461693003/diff/20001/ash/common/system/tray/...
ash/common/system/tray/system_tray.cc:85: // Overridden from SystemTrayItem.
On 2016/10/31 21:29:09, tdanderson wrote:
> nit: just "SystemTrayItem:"

I will make this change, but don't want to switch to this patch's branch at the
moment

https://codereview.chromium.org/2461693003/diff/20001/ash/common/system/user/...
File ash/common/system/user/tray_user.cc (left):

https://codereview.chromium.org/2461693003/diff/20001/ash/common/system/user/...
ash/common/system/user/tray_user.cc:178: avatar_->SetCornerRadii(0,
kTrayRoundedBorderRadius,
On 2016/10/31 21:29:09, tdanderson wrote:
> Won't removing this sacrifice the rounded corners of the avatar in the tray
> (which I think we want to keep)?

this whole block is redundant with UpdateAfterShelfAlignmentChange

https://codereview.chromium.org/2461693003/diff/20001/ash/common/system/user/...
File ash/common/system/user/tray_user.cc (right):

https://codereview.chromium.org/2461693003/diff/20001/ash/common/system/user/...
ash/common/system/user/tray_user.cc:153: delete avatar_;
On 2016/10/31 21:29:09, tdanderson wrote:
> Why the change to using delete here? Wouldn't it be safer to stick with
> RemoveAllChildViews()?

This block of code (re)generates these two views as needed. It doesn't make
sense to remove any more children than those. I originally changed this because
I was adding another child to |layout_view_|. That's no longer the case, but
it's still more intuitive IMO --- the next time someone comes along and adds a
child to layout_view_ somewhere else in this file, they won't have to hunt down
why that view is failing to appear.

[tdanderson, 2016-11-01 00:00:35]

LGTM

https://codereview.chromium.org/2461693003/diff/20001/ash/common/system/tray/...
File ash/common/system/tray/system_tray.cc (right):

https://codereview.chromium.org/2461693003/diff/20001/ash/common/system/tray/...
ash/common/system/tray/system_tray.cc:85: // Overridden from SystemTrayItem.
On 2016/10/31 22:11:56, Evan Stade wrote:
> On 2016/10/31 21:29:09, tdanderson wrote:
> > nit: just "SystemTrayItem:"
> 
> I will make this change, but don't want to switch to this patch's branch at
the
> moment

Acknowledged.

https://codereview.chromium.org/2461693003/diff/20001/ash/common/system/user/...
File ash/common/system/user/tray_user.cc (left):

https://codereview.chromium.org/2461693003/diff/20001/ash/common/system/user/...
ash/common/system/user/tray_user.cc:178: avatar_->SetCornerRadii(0,
kTrayRoundedBorderRadius,
On 2016/10/31 22:11:56, Evan Stade wrote:
> On 2016/10/31 21:29:09, tdanderson wrote:
> > Won't removing this sacrifice the rounded corners of the avatar in the tray
> > (which I think we want to keep)?
> 
> this whole block is redundant with UpdateAfterShelfAlignmentChange

Ah I see now, thanks for clarifying.

https://codereview.chromium.org/2461693003/diff/20001/ash/common/system/user/...
File ash/common/system/user/tray_user.cc (right):

https://codereview.chromium.org/2461693003/diff/20001/ash/common/system/user/...
ash/common/system/user/tray_user.cc:153: delete avatar_;
On 2016/10/31 22:11:56, Evan Stade wrote:
> On 2016/10/31 21:29:09, tdanderson wrote:
> > Why the change to using delete here? Wouldn't it be safer to stick with
> > RemoveAllChildViews()?
> 
> This block of code (re)generates these two views as needed. It doesn't make
> sense to remove any more children than those. I originally changed this
because
> I was adding another child to |layout_view_|. That's no longer the case, but
> it's still more intuitive IMO --- the next time someone comes along and adds a
> child to layout_view_ somewhere else in this file, they won't have to hunt
down
> why that view is failing to appear.

Acknowledged.

[Evan Stade, 2016-11-01 15:23:20]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-11-01 15:23:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdanderson@chromium.org
Link to the patchset: https://codereview.chromium.org/2461693003/#ps40001
(title: "nit + rebase")

[commit-bot: I haz the power, 2016-11-01 15:23:34]

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-11-01 15:55:56]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-01 16:07:41]

Description was changed from

==========
MD cros - Fix padding in system tray

BUG=653292
==========

to

==========
MD cros - Fix padding in system tray

BUG=653292

Committed: https://crrev.com/78cffd731645283982ec8821a054b67317ee3b8a
Cr-Commit-Position: refs/heads/master@{#429011}
==========

[commit-bot: I haz the power, 2016-11-01 16:07:42]

Patchset 3 (id:??) landed as
https://crrev.com/78cffd731645283982ec8821a054b67317ee3b8a
Cr-Commit-Position: refs/heads/master@{#429011}