ash:Shelf Update Alternate Layout

ash:Shelf Update Alternate Layout

For Alternate Layout:
- Overflow Button Positioned correctly
- Spacing between buttons increased
- Position of App List button adjusted
- Position of Status Area adjusted
- App List no longer can be re positioned.

TEST=Visual
TBR=sadrul@chromium.org
BUG=264232, 263884, 265661, 268500
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216624

[Harry McCleave, 2013-08-06 03:41:29]

ptal

[Mr4D (OOO till 08-26), 2013-08-06 21:10:33]

A few nits.

https://codereview.chromium.org/22291003/diff/1/ash/launcher/launcher_view.cc
File ash/launcher/launcher_view.cc (right):

https://codereview.chromium.org/22291003/diff/1/ash/launcher/launcher_view.cc...
ash/launcher/launcher_view.cc:787: last_visible_index_ > 0)
Shouldn't "last_visible_index_" always be > 0?

https://codereview.chromium.org/22291003/diff/1/ash/launcher/launcher_view.cc...
ash/launcher/launcher_view.cc:1104: return;
Yeah! code which will go away soon!

https://codereview.chromium.org/22291003/diff/1/ash/shelf/shelf_layout_manage...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/22291003/diff/1/ash/shelf/shelf_layout_manage...
ash/shelf/shelf_layout_manager.cc:874: available_bounds.right() - shelf_width +
kStatusAreaInset);
Sorry for this nit but: since the term spans two lines you should use {}'s.

https://codereview.chromium.org/22291003/diff/1/ash/system/status_area_widget...
File ash/system/status_area_widget_delegate.cc (right):

https://codereview.chromium.org/22291003/diff/1/ash/system/status_area_widget...
ash/system/status_area_widget_delegate.cc:103: kAlternateTraySpacing :
kTraySpacing);
You might want to populate a variable before the first if and use it here and
down there?

https://codereview.chromium.org/22291003/diff/1/ash/system/tray/tray_constant...
File ash/system/tray/tray_constants.cc (right):

https://codereview.chromium.org/22291003/diff/1/ash/system/tray/tray_constant...
ash/system/tray/tray_constants.cc:68: const int kAlternateTraySpacing = 4;
This feels rather odd. If in all cases where kTraySpacing is used,
kAlternateTraySpacing should get used you should possibly create a function to
access it and not offer the constant this way.

That said - I know that this is (hopefully) temporary code and will get removed
soon. As such it might also simply stay as it is.

[Harry McCleave, 2013-08-06 22:41:06]

https://codereview.chromium.org/22291003/diff/1/ash/launcher/launcher_view.cc
File ash/launcher/launcher_view.cc (right):

https://codereview.chromium.org/22291003/diff/1/ash/launcher/launcher_view.cc...
ash/launcher/launcher_view.cc:787: last_visible_index_ > 0)
On 2013/08/06 21:10:34, Mr4D wrote:
> Shouldn't "last_visible_index_" always be > 0?

This was to maintain the code stability in all paths, iirc it can be -1 if 0
icons can be visible, but this seems to be an invalid state should I add an
assert instead?

https://codereview.chromium.org/22291003/diff/1/ash/launcher/launcher_view.cc...
ash/launcher/launcher_view.cc:1104: return;
On 2013/08/06 21:10:34, Mr4D wrote:
> Yeah! code which will go away soon!

Indeed

https://codereview.chromium.org/22291003/diff/1/ash/shelf/shelf_layout_manage...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/22291003/diff/1/ash/shelf/shelf_layout_manage...
ash/shelf/shelf_layout_manager.cc:874: available_bounds.right() - shelf_width +
kStatusAreaInset);
On 2013/08/06 21:10:34, Mr4D wrote:
> Sorry for this nit but: since the term spans two lines you should use {}'s.

Done.

https://codereview.chromium.org/22291003/diff/1/ash/system/status_area_widget...
File ash/system/status_area_widget_delegate.cc (right):

https://codereview.chromium.org/22291003/diff/1/ash/system/status_area_widget...
ash/system/status_area_widget_delegate.cc:103: kAlternateTraySpacing :
kTraySpacing);
On 2013/08/06 21:10:34, Mr4D wrote:
> You might want to populate a variable before the first if and use it here and
> down there?

Done.

[Mr4D (OOO till 08-26), 2013-08-06 23:32:19]

Two nits. Beside that lgtm!

https://codereview.chromium.org/22291003/diff/1/ash/launcher/launcher_view.cc
File ash/launcher/launcher_view.cc (right):

https://codereview.chromium.org/22291003/diff/1/ash/launcher/launcher_view.cc...
ash/launcher/launcher_view.cc:787: last_visible_index_ > 0)
If the *overflow* gets shown - there should be a whole slew of icons (shouldn't
it?).

Beside that - I think that a DCHECK for <0 would be a good idea since there
should always be at least one icon.

https://codereview.chromium.org/22291003/diff/9001/ash/system/tray/tray_const...
File ash/system/tray/tray_constants.h (right):

https://codereview.chromium.org/22291003/diff/9001/ash/system/tray/tray_const...
ash/system/tray/tray_constants.h:70: int GetTraySpacing();
Thanks for doing this! To make sure that the constants are not used - would it
be possible to remove the two exports above (that way people have to use the
function).

[Harry McCleave, 2013-08-06 23:52:56]

James: ptal for owner when possible.

[sadrul, 2013-08-07 01:23:08]

ui/views/ lgtm

[James Cook, 2013-08-07 16:48:45]

https://codereview.chromium.org/22291003/diff/19001/ash/launcher/launcher_vie...
File ash/launcher/launcher_view.cc (right):

https://codereview.chromium.org/22291003/diff/19001/ash/launcher/launcher_vie...
ash/launcher/launcher_view.cc:684: void
LauncherView::CalculateIdealBounds(IdealBounds* bounds) {
Maybe not in this CL, but it would be good for this very complex function to be
tested somehow, especially the parts that change item visibility.  That might be
easier if it were broken into smaller parts.

https://codereview.chromium.org/22291003/diff/19001/ash/launcher/launcher_vie...
ash/launcher/launcher_view.cc:1046: bool
LauncherView::SameDragType(LauncherItemType typea,
Please add a simple test for this function.

[Harry McCleave, 2013-08-08 00:44:38]

https://codereview.chromium.org/22291003/diff/19001/ash/launcher/launcher_vie...
File ash/launcher/launcher_view.cc (right):

https://codereview.chromium.org/22291003/diff/19001/ash/launcher/launcher_vie...
ash/launcher/launcher_view.cc:684: void
LauncherView::CalculateIdealBounds(IdealBounds* bounds) {
On 2013/08/07 16:48:45, James Cook wrote:
> Maybe not in this CL, but it would be good for this very complex function to
be
> tested somehow, especially the parts that change item visibility.  That might
be
> easier if it were broken into smaller parts.

Ack, would prefer a separate cl for simplification and test of visibility.

https://codereview.chromium.org/22291003/diff/19001/ash/launcher/launcher_vie...
ash/launcher/launcher_view.cc:1046: bool
LauncherView::SameDragType(LauncherItemType typea,
On 2013/08/07 16:48:45, James Cook wrote:
> Please add a simple test for this function.

Done.

[James Cook, 2013-08-08 16:34:50]

LGTM with nit

https://codereview.chromium.org/22291003/diff/25001/ash/test/launcher_view_te...
File ash/test/launcher_view_test_api.cc (right):

https://codereview.chromium.org/22291003/diff/25001/ash/test/launcher_view_te...
ash/test/launcher_view_test_api.cc:101: }
nit: blank line after }

[commit-bot: I haz the power, 2013-08-09 01:10:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/harrym@chromium.org/22291003/37001

[commit-bot: I haz the power, 2013-08-09 07:12:00]

Change committed as 216624