Simplify layout of tray items and fix spacing for MD.

Simplify layout of tray items and fix spacing for MD.

BUG=653292
Committed: https://crrev.com/2449770edb6a482bf442bcdb669cd15d5a2f2632
Cr-Commit-Position: refs/heads/master@{#428454}

[Evan Stade, 2016-10-27 19:05:29]

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

[Evan Stade, 2016-10-27 19:05:30]

net negative lines of code 8)

https://codereview.chromium.org/2455183002/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/network/tray_network.cc (left):

https://codereview.chromium.org/2455183002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/tray_network.cc:52: new
views::BoxLayout(views::BoxLayout::kHorizontal, 0, 0, 0));
use the TrayItemView layout (FillLayout)

https://codereview.chromium.org/2455183002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/tray_network.cc:92: SetLayoutManager(new
views::BoxLayout(IsHorizontalAlignment(alignment)
the image already centers itself

https://codereview.chromium.org/2455183002/diff/1/ash/common/system/tray/fixe...
File ash/common/system/tray/fixed_sized_image_view.cc (left):

https://codereview.chromium.org/2455183002/diff/1/ash/common/system/tray/fixe...
ash/common/system/tray/fixed_sized_image_view.cc:12:
SetVerticalAlignment(views::ImageView::CENTER);
this is already the default

https://codereview.chromium.org/2455183002/diff/1/ash/common/system/tray/tray...
File ash/common/system/tray/tray_image_item.cc (left):

https://codereview.chromium.org/2455183002/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_image_item.cc:116: ? views::BoxLayout::kHorizontal
use the TrayItemView's fill layout. ImageView already centers the actual Image.

https://codereview.chromium.org/2455183002/diff/1/ash/common/system/tray/tray...
File ash/common/system/tray/tray_utils.cc (right):

https://codereview.chromium.org/2455183002/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_utils.cc:41: if
(MaterialDesignController::IsShelfMaterial())
TrayItemView takes care of sizing, don't need this border as well

(yay compartmentalization)

[tdanderson, 2016-10-27 19:28:01]

Evan, please see my comments below:

https://codereview.chromium.org/2455183002/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/network/tray_network.cc (left):

https://codereview.chromium.org/2455183002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/tray_network.cc:52: new
views::BoxLayout(views::BoxLayout::kHorizontal, 0, 0, 0));
On 2016/10/27 19:05:30, Evan Stade wrote:
> use the TrayItemView layout (FillLayout)

Acknowledged.

https://codereview.chromium.org/2455183002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/tray_network.cc:92: SetLayoutManager(new
views::BoxLayout(IsHorizontalAlignment(alignment)
On 2016/10/27 19:05:30, Evan Stade wrote:
> the image already centers itself

Acknowledged.

https://codereview.chromium.org/2455183002/diff/1/ash/common/system/tray/fixe...
File ash/common/system/tray/fixed_sized_image_view.cc (left):

https://codereview.chromium.org/2455183002/diff/1/ash/common/system/tray/fixe...
ash/common/system/tray/fixed_sized_image_view.cc:12:
SetVerticalAlignment(views::ImageView::CENTER);
On 2016/10/27 19:05:30, Evan Stade wrote:
> this is already the default

Acknowledged.

https://codereview.chromium.org/2455183002/diff/1/ash/common/system/tray/tray...
File ash/common/system/tray/tray_image_item.cc (left):

https://codereview.chromium.org/2455183002/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_image_item.cc:116: ? views::BoxLayout::kHorizontal
On 2016/10/27 19:05:30, Evan Stade wrote:
> use the TrayItemView's fill layout. ImageView already centers the actual
Image.

Acknowledged.

https://codereview.chromium.org/2455183002/diff/1/ash/common/system/tray/tray...
File ash/common/system/tray/tray_item_view.cc (right):

https://codereview.chromium.org/2455183002/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_item_view.cc:55:
views::Background::CreateSolidBackground(SK_ColorGREEN));
Is this for debugging?

https://codereview.chromium.org/2455183002/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_item_view.cc:91: inner_size =
gfx::Size(kTrayIconSize, kTrayIconSize);
I don't really see the purpose of |inner_size|. Why do you mutate |inner_size|
here but then never use it before it goes out of scope?

https://codereview.chromium.org/2455183002/diff/1/ash/common/system/tray/tray...
File ash/common/system/tray/tray_item_view.h (left):

https://codereview.chromium.org/2455183002/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_item_view.h:60: virtual gfx::Size DesiredSize()
const;
Nice.

https://codereview.chromium.org/2455183002/diff/1/ash/common/system/tray/tray...
File ash/common/system/tray/tray_utils.cc (right):

https://codereview.chromium.org/2455183002/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_utils.cc:41: if
(MaterialDesignController::IsShelfMaterial())
On 2016/10/27 19:05:30, Evan Stade wrote:
> TrayItemView takes care of sizing, don't need this border as well
> 
> (yay compartmentalization)

Won't this still be needed by TrayPower::UpdateAfterShelfAlignmentChange()
(which isn't a TrayItemView)?

[Evan Stade, 2016-10-27 21:35:10]

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

[commit-bot: I haz the power, 2016-10-27 21:35:45]

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-27 21:36:34]

latest patch also improves TrayIME and TrayDate

https://codereview.chromium.org/2455183002/diff/1/ash/common/system/tray/tray...
File ash/common/system/tray/tray_item_view.cc (right):

https://codereview.chromium.org/2455183002/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_item_view.cc:55:
views::Background::CreateSolidBackground(SK_ColorGREEN));
On 2016/10/27 19:28:01, tdanderson wrote:
> Is this for debugging?

oops.

https://codereview.chromium.org/2455183002/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_item_view.cc:91: inner_size =
gfx::Size(kTrayIconSize, kTrayIconSize);
On 2016/10/27 19:28:01, tdanderson wrote:
> I don't really see the purpose of |inner_size|. Why do you mutate |inner_size|
> here but then never use it before it goes out of scope?

another oops. I'm not sure how it worked like this. I must have got it working,
refactored and forgotten to test my refactor. Anyway, fixed now.

https://codereview.chromium.org/2455183002/diff/1/ash/common/system/tray/tray...
File ash/common/system/tray/tray_utils.cc (right):

https://codereview.chromium.org/2455183002/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_utils.cc:41: if
(MaterialDesignController::IsShelfMaterial())
On 2016/10/27 19:28:01, tdanderson wrote:
> On 2016/10/27 19:05:30, Evan Stade wrote:
> > TrayItemView takes care of sizing, don't need this border as well
> > 
> > (yay compartmentalization)
> 
> Won't this still be needed by TrayPower::UpdateAfterShelfAlignmentChange()
> (which isn't a TrayItemView)?

yes. I saw that class, but figured it was working somehow because the
measurements between icons seemed correct. But that was because I was using
Sebastien's measurement from the visible edge of the wifi icon to the battery,
while comparing to the visible edge of a cell icon to the battery, which
magically compensated.

TrayPower should be a TrayItemView. Now it is.

[commit-bot: I haz the power, 2016-10-27 22:11:10]

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

[commit-bot: I haz the power, 2016-10-27 22:11:11]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[tdanderson, 2016-10-27 23:58:41]

Looks like the trybot failures are relevant, but lgtm with those addressed.

https://codereview.chromium.org/2455183002/diff/20001/ash/common/system/date/...
File ash/common/system/date/date_view.cc (right):

https://codereview.chromium.org/2455183002/diff/20001/ash/common/system/date/...
ash/common/system/date/date_view.cc:50: // when the shelf is horizontally
aligned, and on the top when the shelf is
Looks like you deleted some of this comment by mistake

https://codereview.chromium.org/2455183002/diff/20001/ash/common/system/date/...
ash/common/system/date/date_view.cc:329: SetBorder(views::Border::NullBorder());
Double-checking: this will set a null border if MD is enabled and the clock is
vertical. Do you want to do that?

[Evan Stade, 2016-10-28 18:07:49]

On 2016/10/27 23:58:41, tdanderson wrote:
> Looks like the trybot failures are relevant, but lgtm with those addressed.

yea, seems gridlayout doesn't like it when you add a child view without its say
so. Luckily easy to fix.

https://codereview.chromium.org/2455183002/diff/20001/ash/common/system/date/...
File ash/common/system/date/date_view.cc (right):

https://codereview.chromium.org/2455183002/diff/20001/ash/common/system/date/...
ash/common/system/date/date_view.cc:50: // when the shelf is horizontally
aligned, and on the top when the shelf is
On 2016/10/27 23:58:41, tdanderson wrote:
> Looks like you deleted some of this comment by mistake

Done.

https://codereview.chromium.org/2455183002/diff/20001/ash/common/system/date/...
ash/common/system/date/date_view.cc:329: SetBorder(views::Border::NullBorder());
On 2016/10/27 23:58:41, tdanderson wrote:
> Double-checking: this will set a null border if MD is enabled and the clock is
> vertical. Do you want to do that?

It's not harmful, but I guess it's not really necessary. Added early return for
md.

[Evan Stade, 2016-10-28 18:08:03]

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

[commit-bot: I haz the power, 2016-10-28 18:08:32]

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 18:11:39]

The CQ bit was unchecked by estade@chromium.org

[Evan Stade, 2016-10-28 18:11:49]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-10-28 18:11:50]

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/2455183002/#ps40001
(title: "tda review")

[commit-bot: I haz the power, 2016-10-28 18:12: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, 2016-10-28 19:10:28]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-28 19:24:38]

Description was changed from

==========
Simplify layout of tray items and fix spacing for MD.

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

to

==========
Simplify layout of tray items and fix spacing for MD.

BUG=653292

Committed: https://crrev.com/2449770edb6a482bf442bcdb669cd15d5a2f2632
Cr-Commit-Position: refs/heads/master@{#428454}
==========

[commit-bot: I haz the power, 2016-10-28 19:24:39]

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