Ash:Shelf - Cleanup of Alternate Shelf (part 1)

Part 1) of cleaning up the code related to the shelf layout, specifically removing all instances of branching related to the 'alternate shelf layout' as this is now the only option.
Removed the about:flags entry relating to alternate shelf layout.

A patch will follow to simplify the remaining layout code as it is still a bit of a spaghetti like mess.

R=jamescook@chromium.org, skuhne@chromium.org
TBR=miket@chromium.org
BUG=338429
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=254301

[Harry McCleave, 2014-01-23 02:06:16]

ptal

https://codereview.chromium.org/140323010/diff/1/ash/system/tray/tray_backgro...
File ash/system/tray/tray_background_view.cc (right):

https://codereview.chromium.org/140323010/diff/1/ash/system/tray/tray_backgro...
ash/system/tray/tray_background_view.cc:379:
background_->set_alpha(hide_background_animator_.alpha() +
Should I remove this code now or in the later patch?

[James Cook, 2014-01-23 18:02:34]

It's so nice to see this getting cleaned up!  A few questions.

https://codereview.chromium.org/140323010/diff/30001/ash/shelf/app_list_button.h
File ash/shelf/app_list_button.h (right):

https://codereview.chromium.org/140323010/diff/30001/ash/shelf/app_list_butto...
ash/shelf/app_list_button.h:19: class AppListButton : public views::ImageButton
{
It looks like you deleted the old app_list_button.h and then renamed
alternate_app_list_button.h to app_list_button.h.  Unfortunately git and
reitveld don't understand that; they think you just modified app_list_button.h.
This will lose revision history.

It's up to you how to resolve this. I think there are two approaches:
* Just decide the revision history of alternate_app_list_button.h/cc isn't
important and do what you do here
* Break this CL into two patches. In the first one delete app_list_button.h/cc. 
In the second one rename alternate_app_list_button.h/cc to app_list_button.h/cc.

What do you think?

https://codereview.chromium.org/140323010/diff/30001/ash/shelf/shelf_button.cc
File ash/shelf/shelf_button.cc (left):

https://codereview.chromium.org/140323010/diff/30001/ash/shelf/shelf_button.c...
ash/shelf/shelf_button.cc:555: bar_id = IDR_AURA_LAUNCHER_UNDERLINE_ACTIVE;
Are these resources now unused? Can they be deleted?

https://codereview.chromium.org/140323010/diff/30001/ash/shelf/shelf_view.cc
File ash/shelf/shelf_view.cc (right):

https://codereview.chromium.org/140323010/diff/30001/ash/shelf/shelf_view.cc#...
ash/shelf/shelf_view.cc:700: int x =
layout_manager_->SelectValueForShelfAlignment(inset, 0, 0, inset);
Doesn't this always select 0?

https://codereview.chromium.org/140323010/diff/30001/ash/shelf/shelf_view.cc#...
ash/shelf/shelf_view.cc:780: x =
layout_manager_->SelectValueForShelfAlignment(inset, 0, 0, inset);
Isn't inset always 0 here?

https://codereview.chromium.org/140323010/diff/30001/ash/shelf/shelf_view.cc#...
ash/shelf/shelf_view.cc:807: }
Not for this CL, but man it would be awesome to break this function into smaller
pieces.

https://codereview.chromium.org/140323010/diff/30001/ash/shelf/shelf_view.cc#...
ash/shelf/shelf_view.cc:1549: }
optional nit: Maybe comment "// Nothing to do."  At first I thought you could
delete this method entirely but you can't.

https://codereview.chromium.org/140323010/diff/30001/ash/system/tray/system_t...
File ash/system/tray/system_tray.cc (right):

https://codereview.chromium.org/140323010/diff/30001/ash/system/tray/system_t...
ash/system/tray/system_tray.cc:489: init_params.first_item_has_no_margin = true;
Just to double-check, and not for this CL, is first_item_has_no_margin ever
false? Could it be deleted?

https://codereview.chromium.org/140323010/diff/30001/ash/system/tray/tray_con...
File ash/system/tray/tray_constants.cc (right):

https://codereview.chromium.org/140323010/diff/30001/ash/system/tray/tray_con...
ash/system/tray/tray_constants.cc:79: int GetTraySpacing() {
Maybe not for this CL, but references to this function could be replaced with
just using the constant.

https://codereview.chromium.org/140323010/diff/30001/ash/system/web_notificat...
File ash/system/web_notification/web_notification_tray.cc (right):

https://codereview.chromium.org/140323010/diff/30001/ash/system/web_notificat...
ash/system/web_notification/web_notification_tray.cc:340: true);
same question about first_item_has_no_margin

[Mr4D (OOO till 08-26), 2014-01-23 19:25:07]

Yeah! Less code! Great!

https://codereview.chromium.org/140323010/diff/30001/ash/shelf/app_list_butto...
File ash/shelf/app_list_button.cc (right):

https://codereview.chromium.org/140323010/diff/30001/ash/shelf/app_list_butto...
ash/shelf/app_list_button.cc:7: #include "ash/ash_constants.h"
Same here as in the .h file as descibed by jamescook

https://codereview.chromium.org/140323010/diff/30001/ash/shelf/shelf_layout_m...
File ash/shelf/shelf_layout_manager.cc (left):

https://codereview.chromium.org/140323010/diff/30001/ash/shelf/shelf_layout_m...
ash/shelf/shelf_layout_manager.cc:105: ShelfLayoutManager::kShelfSize :
kShelfPreferredSize;
Just checking - is kShelfPreferredSize still needed? I have seen some
references, but the question is if the use of that would even be still correct.
You use it in e.g. "ShelfView::GetButtonSize()" which suggests that you might
want to rename it into something like kShelfButtonSize?

https://codereview.chromium.org/140323010/diff/30001/ash/shelf/shelf_model.cc
File ash/shelf/shelf_model.cc (right):

https://codereview.chromium.org/140323010/diff/30001/ash/shelf/shelf_model.cc...
ash/shelf/shelf_model.cc:33: NOTREACHED() << "LauncherItemType must be set";
ShelfItemType - not LauncherItemType any more.

https://codereview.chromium.org/140323010/diff/30001/ash/shelf/shelf_view.cc
File ash/shelf/shelf_view.cc (right):

https://codereview.chromium.org/140323010/diff/30001/ash/shelf/shelf_view.cc#...
ash/shelf/shelf_view.cc:701: int y =
layout_manager_->SelectValueForShelfAlignment(0, inset, inset, 0);
as here.

https://codereview.chromium.org/140323010/diff/30001/ash/system/tray/tray_bac...
File ash/system/tray/tray_background_view.cc (right):

https://codereview.chromium.org/140323010/diff/30001/ash/system/tray/tray_bac...
ash/system/tray/tray_background_view.cc:215: PaintForAlternateShelf(canvas,
view);
I suppose you should rename that function as well since "AlternateShelf" doesn't
make much sense there anymore.

https://codereview.chromium.org/140323010/diff/30001/ash/system/tray/tray_bac...
ash/system/tray/tray_background_view.cc:270: int horizontal_padding =
kPaddingFromEdgeOfShelf;
Same as below.

https://codereview.chromium.org/140323010/diff/30001/ash/system/tray/tray_bac...
ash/system/tray/tray_background_view.cc:283: int horizontal_padding =
kPaddingFromEdgeOfShelf;
Since they are now fixed - why not using them directly below?

[Harry McCleave, 2014-01-24 22:48:47]

https://codereview.chromium.org/140323010/diff/30001/ash/shelf/app_list_butto...
File ash/shelf/app_list_button.cc (right):

https://codereview.chromium.org/140323010/diff/30001/ash/shelf/app_list_butto...
ash/shelf/app_list_button.cc:7: #include "ash/ash_constants.h"
On 2014/01/23 19:25:08, Mr4D wrote:
> Same here as in the .h file as descibed by jamescook

same as .h

https://codereview.chromium.org/140323010/diff/30001/ash/shelf/app_list_button.h
File ash/shelf/app_list_button.h (right):

https://codereview.chromium.org/140323010/diff/30001/ash/shelf/app_list_butto...
ash/shelf/app_list_button.h:19: class AppListButton : public views::ImageButton
{
On 2014/01/23 18:02:35, James Cook wrote:
> It looks like you deleted the old app_list_button.h and then renamed
> alternate_app_list_button.h to app_list_button.h.  Unfortunately git and
> reitveld don't understand that; they think you just modified
app_list_button.h.
> This will lose revision history.
> 
> It's up to you how to resolve this. I think there are two approaches:
> * Just decide the revision history of alternate_app_list_button.h/cc isn't
> important and do what you do here
> * Break this CL into two patches. In the first one delete
app_list_button.h/cc. 
> In the second one rename alternate_app_list_button.h/cc to
app_list_button.h/cc.
> 
> What do you think?

This is the case, I don't think there is any meaningful history in the
alternate_ files as they have only existed for 1 or 2 changes, so I would vote
for just letting those changes get compressed into this one in the history, if
you disagree I will split this into 2 changes (rm app_list_button, then mv).

https://codereview.chromium.org/140323010/diff/30001/ash/shelf/shelf_button.cc
File ash/shelf/shelf_button.cc (left):

https://codereview.chromium.org/140323010/diff/30001/ash/shelf/shelf_button.c...
ash/shelf/shelf_button.cc:555: bar_id = IDR_AURA_LAUNCHER_UNDERLINE_ACTIVE;
On 2014/01/23 18:02:35, James Cook wrote:
> Are these resources now unused? Can they be deleted?

Yes they are unused I have added their removal to this cl. To prevent other
exciting git oddities I will be moving the alt_launcher asset files into the
launcher folder in the next patch.

https://codereview.chromium.org/140323010/diff/30001/ash/shelf/shelf_layout_m...
File ash/shelf/shelf_layout_manager.cc (left):

https://codereview.chromium.org/140323010/diff/30001/ash/shelf/shelf_layout_m...
ash/shelf/shelf_layout_manager.cc:105: ShelfLayoutManager::kShelfSize :
kShelfPreferredSize;
On 2014/01/23 19:25:08, Mr4D wrote:
> Just checking - is kShelfPreferredSize still needed? I have seen some
> references, but the question is if the use of that would even be still
correct.
> You use it in e.g. "ShelfView::GetButtonSize()" which suggests that you might
> want to rename it into something like kShelfButtonSize?

I have removed this function but left GetButtonSize, which currently specifies
that buttons will be the same size as the shelf.

https://codereview.chromium.org/140323010/diff/30001/ash/shelf/shelf_model.cc
File ash/shelf/shelf_model.cc (right):

https://codereview.chromium.org/140323010/diff/30001/ash/shelf/shelf_model.cc...
ash/shelf/shelf_model.cc:33: NOTREACHED() << "LauncherItemType must be set";
On 2014/01/23 19:25:08, Mr4D wrote:
> ShelfItemType - not LauncherItemType any more.

Done, grumble grumble rebase...

https://codereview.chromium.org/140323010/diff/30001/ash/shelf/shelf_view.cc
File ash/shelf/shelf_view.cc (right):

https://codereview.chromium.org/140323010/diff/30001/ash/shelf/shelf_view.cc#...
ash/shelf/shelf_view.cc:700: int x =
layout_manager_->SelectValueForShelfAlignment(inset, 0, 0, inset);
On 2014/01/23 18:02:35, James Cook wrote:
> Doesn't this always select 0?

Done.

https://codereview.chromium.org/140323010/diff/30001/ash/shelf/shelf_view.cc#...
ash/shelf/shelf_view.cc:701: int y =
layout_manager_->SelectValueForShelfAlignment(0, inset, inset, 0);
On 2014/01/23 19:25:08, Mr4D wrote:
> as here.

Done.

https://codereview.chromium.org/140323010/diff/30001/ash/shelf/shelf_view.cc#...
ash/shelf/shelf_view.cc:780: x =
layout_manager_->SelectValueForShelfAlignment(inset, 0, 0, inset);
On 2014/01/23 18:02:35, James Cook wrote:
> Isn't inset always 0 here?

Done.

https://codereview.chromium.org/140323010/diff/30001/ash/shelf/shelf_view.cc#...
ash/shelf/shelf_view.cc:807: }
On 2014/01/23 18:02:35, James Cook wrote:
> Not for this CL, but man it would be awesome to break this function into
smaller
> pieces.

Part 2's intention is to do just this, the layout code is just too hard to
follow atm.

https://codereview.chromium.org/140323010/diff/30001/ash/shelf/shelf_view.cc#...
ash/shelf/shelf_view.cc:1549: }
On 2014/01/23 18:02:35, James Cook wrote:
> optional nit: Maybe comment "// Nothing to do."  At first I thought you could
> delete this method entirely but you can't.

Done. I thought it could be removed as well, the compiler slapped me.

https://codereview.chromium.org/140323010/diff/30001/ash/system/tray/system_t...
File ash/system/tray/system_tray.cc (right):

https://codereview.chromium.org/140323010/diff/30001/ash/system/tray/system_t...
ash/system/tray/system_tray.cc:489: init_params.first_item_has_no_margin = true;
On 2014/01/23 18:02:35, James Cook wrote:
> Just to double-check, and not for this CL, is first_item_has_no_margin ever
> false? Could it be deleted?

I thought it was the case it was false for some bubbles not associated with the
shelf Stefan do you remember? (my memory tells me Stefan added this fixing a
number of issues).

https://codereview.chromium.org/140323010/diff/30001/ash/system/tray/tray_bac...
File ash/system/tray/tray_background_view.cc (right):

https://codereview.chromium.org/140323010/diff/30001/ash/system/tray/tray_bac...
ash/system/tray/tray_background_view.cc:215: PaintForAlternateShelf(canvas,
view);
On 2014/01/23 19:25:08, Mr4D wrote:
> I suppose you should rename that function as well since "AlternateShelf"
doesn't
> make much sense there anymore.

Done.

https://codereview.chromium.org/140323010/diff/30001/ash/system/tray/tray_bac...
ash/system/tray/tray_background_view.cc:270: int horizontal_padding =
kPaddingFromEdgeOfShelf;
On 2014/01/23 19:25:08, Mr4D wrote:
> Same as below.

Done.

https://codereview.chromium.org/140323010/diff/30001/ash/system/tray/tray_bac...
ash/system/tray/tray_background_view.cc:283: int horizontal_padding =
kPaddingFromEdgeOfShelf;
On 2014/01/23 19:25:08, Mr4D wrote:
> Since they are now fixed - why not using them directly below?

Done.

https://codereview.chromium.org/140323010/diff/30001/ash/system/tray/tray_con...
File ash/system/tray/tray_constants.cc (right):

https://codereview.chromium.org/140323010/diff/30001/ash/system/tray/tray_con...
ash/system/tray/tray_constants.cc:79: int GetTraySpacing() {
On 2014/01/23 18:02:35, James Cook wrote:
> Maybe not for this CL, but references to this function could be replaced with
> just using the constant.

ShelfItemHeight as well, will do in part 2.

https://codereview.chromium.org/140323010/diff/30001/ash/system/web_notificat...
File ash/system/web_notification/web_notification_tray.cc (right):

https://codereview.chromium.org/140323010/diff/30001/ash/system/web_notificat...
ash/system/web_notification/web_notification_tray.cc:340: true);
On 2014/01/23 18:02:35, James Cook wrote:
> same question about first_item_has_no_margin

.

[Harry McCleave, 2014-01-24 23:14:58]

On 2014/01/24 22:48:47, Harry McCleave wrote:
>
https://codereview.chromium.org/140323010/diff/30001/ash/shelf/app_list_butto...
> File ash/shelf/app_list_button.cc (right):
> 
>
https://codereview.chromium.org/140323010/diff/30001/ash/shelf/app_list_butto...
> ash/shelf/app_list_button.cc:7: #include "ash/ash_constants.h"
> On 2014/01/23 19:25:08, Mr4D wrote:
> > Same here as in the .h file as descibed by jamescook
> 
> same as .h
> 
>
https://codereview.chromium.org/140323010/diff/30001/ash/shelf/app_list_button.h
> File ash/shelf/app_list_button.h (right):
> 
>
https://codereview.chromium.org/140323010/diff/30001/ash/shelf/app_list_butto...
> ash/shelf/app_list_button.h:19: class AppListButton : public
views::ImageButton
> {
> On 2014/01/23 18:02:35, James Cook wrote:
> > It looks like you deleted the old app_list_button.h and then renamed
> > alternate_app_list_button.h to app_list_button.h.  Unfortunately git and
> > reitveld don't understand that; they think you just modified
> app_list_button.h.
> > This will lose revision history.
> > 
> > It's up to you how to resolve this. I think there are two approaches:
> > * Just decide the revision history of alternate_app_list_button.h/cc isn't
> > important and do what you do here
> > * Break this CL into two patches. In the first one delete
> app_list_button.h/cc. 
> > In the second one rename alternate_app_list_button.h/cc to
> app_list_button.h/cc.
> > 
> > What do you think?
> 
> This is the case, I don't think there is any meaningful history in the
> alternate_ files as they have only existed for 1 or 2 changes, so I would vote
> for just letting those changes get compressed into this one in the history, if
> you disagree I will split this into 2 changes (rm app_list_button, then mv).
> 
> https://codereview.chromium.org/140323010/diff/30001/ash/shelf/shelf_button.cc
> File ash/shelf/shelf_button.cc (left):
> 
>
https://codereview.chromium.org/140323010/diff/30001/ash/shelf/shelf_button.c...
> ash/shelf/shelf_button.cc:555: bar_id = IDR_AURA_LAUNCHER_UNDERLINE_ACTIVE;
> On 2014/01/23 18:02:35, James Cook wrote:
> > Are these resources now unused? Can they be deleted?
> 
> Yes they are unused I have added their removal to this cl. To prevent other
> exciting git oddities I will be moving the alt_launcher asset files into the
> launcher folder in the next patch.
> 
>
https://codereview.chromium.org/140323010/diff/30001/ash/shelf/shelf_layout_m...
> File ash/shelf/shelf_layout_manager.cc (left):
> 
>
https://codereview.chromium.org/140323010/diff/30001/ash/shelf/shelf_layout_m...
> ash/shelf/shelf_layout_manager.cc:105: ShelfLayoutManager::kShelfSize :
> kShelfPreferredSize;
> On 2014/01/23 19:25:08, Mr4D wrote:
> > Just checking - is kShelfPreferredSize still needed? I have seen some
> > references, but the question is if the use of that would even be still
> correct.
> > You use it in e.g. "ShelfView::GetButtonSize()" which suggests that you
might
> > want to rename it into something like kShelfButtonSize?
> 
> I have removed this function but left GetButtonSize, which currently specifies
> that buttons will be the same size as the shelf.
> 
> https://codereview.chromium.org/140323010/diff/30001/ash/shelf/shelf_model.cc
> File ash/shelf/shelf_model.cc (right):
> 
>
https://codereview.chromium.org/140323010/diff/30001/ash/shelf/shelf_model.cc...
> ash/shelf/shelf_model.cc:33: NOTREACHED() << "LauncherItemType must be set";
> On 2014/01/23 19:25:08, Mr4D wrote:
> > ShelfItemType - not LauncherItemType any more.
> 
> Done, grumble grumble rebase...
> 
> https://codereview.chromium.org/140323010/diff/30001/ash/shelf/shelf_view.cc
> File ash/shelf/shelf_view.cc (right):
> 
>
https://codereview.chromium.org/140323010/diff/30001/ash/shelf/shelf_view.cc#...
> ash/shelf/shelf_view.cc:700: int x =
> layout_manager_->SelectValueForShelfAlignment(inset, 0, 0, inset);
> On 2014/01/23 18:02:35, James Cook wrote:
> > Doesn't this always select 0?
> 
> Done.
> 
>
https://codereview.chromium.org/140323010/diff/30001/ash/shelf/shelf_view.cc#...
> ash/shelf/shelf_view.cc:701: int y =
> layout_manager_->SelectValueForShelfAlignment(0, inset, inset, 0);
> On 2014/01/23 19:25:08, Mr4D wrote:
> > as here.
> 
> Done.
> 
>
https://codereview.chromium.org/140323010/diff/30001/ash/shelf/shelf_view.cc#...
> ash/shelf/shelf_view.cc:780: x =
> layout_manager_->SelectValueForShelfAlignment(inset, 0, 0, inset);
> On 2014/01/23 18:02:35, James Cook wrote:
> > Isn't inset always 0 here?
> 
> Done.
> 
>
https://codereview.chromium.org/140323010/diff/30001/ash/shelf/shelf_view.cc#...
> ash/shelf/shelf_view.cc:807: }
> On 2014/01/23 18:02:35, James Cook wrote:
> > Not for this CL, but man it would be awesome to break this function into
> smaller
> > pieces.
> 
> Part 2's intention is to do just this, the layout code is just too hard to
> follow atm.
> 
>
https://codereview.chromium.org/140323010/diff/30001/ash/shelf/shelf_view.cc#...
> ash/shelf/shelf_view.cc:1549: }
> On 2014/01/23 18:02:35, James Cook wrote:
> > optional nit: Maybe comment "// Nothing to do."  At first I thought you
could
> > delete this method entirely but you can't.
> 
> Done. I thought it could be removed as well, the compiler slapped me.
> 
>
https://codereview.chromium.org/140323010/diff/30001/ash/system/tray/system_t...
> File ash/system/tray/system_tray.cc (right):
> 
>
https://codereview.chromium.org/140323010/diff/30001/ash/system/tray/system_t...
> ash/system/tray/system_tray.cc:489: init_params.first_item_has_no_margin =
true;
> On 2014/01/23 18:02:35, James Cook wrote:
> > Just to double-check, and not for this CL, is first_item_has_no_margin ever
> > false? Could it be deleted?
> 
> I thought it was the case it was false for some bubbles not associated with
the
> shelf Stefan do you remember? (my memory tells me Stefan added this fixing a
> number of issues).
> 
>
https://codereview.chromium.org/140323010/diff/30001/ash/system/tray/tray_bac...
> File ash/system/tray/tray_background_view.cc (right):
> 
>
https://codereview.chromium.org/140323010/diff/30001/ash/system/tray/tray_bac...
> ash/system/tray/tray_background_view.cc:215: PaintForAlternateShelf(canvas,
> view);
> On 2014/01/23 19:25:08, Mr4D wrote:
> > I suppose you should rename that function as well since "AlternateShelf"
> doesn't
> > make much sense there anymore.
> 
> Done.
> 
>
https://codereview.chromium.org/140323010/diff/30001/ash/system/tray/tray_bac...
> ash/system/tray/tray_background_view.cc:270: int horizontal_padding =
> kPaddingFromEdgeOfShelf;
> On 2014/01/23 19:25:08, Mr4D wrote:
> > Same as below.
> 
> Done.
> 
>
https://codereview.chromium.org/140323010/diff/30001/ash/system/tray/tray_bac...
> ash/system/tray/tray_background_view.cc:283: int horizontal_padding =
> kPaddingFromEdgeOfShelf;
> On 2014/01/23 19:25:08, Mr4D wrote:
> > Since they are now fixed - why not using them directly below?
> 
> Done.
> 
>
https://codereview.chromium.org/140323010/diff/30001/ash/system/tray/tray_con...
> File ash/system/tray/tray_constants.cc (right):
> 
>
https://codereview.chromium.org/140323010/diff/30001/ash/system/tray/tray_con...
> ash/system/tray/tray_constants.cc:79: int GetTraySpacing() {
> On 2014/01/23 18:02:35, James Cook wrote:
> > Maybe not for this CL, but references to this function could be replaced
with
> > just using the constant.
> 
> ShelfItemHeight as well, will do in part 2.
> 
>
https://codereview.chromium.org/140323010/diff/30001/ash/system/web_notificat...
> File ash/system/web_notification/web_notification_tray.cc (right):
> 
>
https://codereview.chromium.org/140323010/diff/30001/ash/system/web_notificat...
> ash/system/web_notification/web_notification_tray.cc:340: true);
> On 2014/01/23 18:02:35, James Cook wrote:
> > same question about first_item_has_no_margin
> 
> .

Added ButtonSize and ButtonSpacing to the list of things in shelf_constants.

[James Cook, 2014-01-25 00:12:14]

LGTM with a couple optional comments

https://codereview.chromium.org/140323010/diff/30001/ash/shelf/app_list_button.h
File ash/shelf/app_list_button.h (right):

https://codereview.chromium.org/140323010/diff/30001/ash/shelf/app_list_butto...
ash/shelf/app_list_button.h:19: class AppListButton : public views::ImageButton
{
On 2014/01/24 22:48:47, Harry McCleave wrote:
> On 2014/01/23 18:02:35, James Cook wrote:
> > It looks like you deleted the old app_list_button.h and then renamed
> > alternate_app_list_button.h to app_list_button.h.  Unfortunately git and
> > reitveld don't understand that; they think you just modified
> app_list_button.h.
> > This will lose revision history.
> > 
> > It's up to you how to resolve this. I think there are two approaches:
> > * Just decide the revision history of alternate_app_list_button.h/cc isn't
> > important and do what you do here
> > * Break this CL into two patches. In the first one delete
> app_list_button.h/cc. 
> > In the second one rename alternate_app_list_button.h/cc to
> app_list_button.h/cc.
> > 
> > What do you think?
> 
> This is the case, I don't think there is any meaningful history in the
> alternate_ files as they have only existed for 1 or 2 changes, so I would vote
> for just letting those changes get compressed into this one in the history, if
> you disagree I will split this into 2 changes (rm app_list_button, then mv).

In that case, keep it in one change. I just wanted to check that we weren't
losing anything significant.

https://codereview.chromium.org/140323010/diff/30001/ash/system/tray/system_t...
File ash/system/tray/system_tray.cc (right):

https://codereview.chromium.org/140323010/diff/30001/ash/system/tray/system_t...
ash/system/tray/system_tray.cc:489: init_params.first_item_has_no_margin = true;
On 2014/01/24 22:48:47, Harry McCleave wrote:
> On 2014/01/23 18:02:35, James Cook wrote:
> > Just to double-check, and not for this CL, is first_item_has_no_margin ever
> > false? Could it be deleted?
> 
> I thought it was the case it was false for some bubbles not associated with
the
> shelf Stefan do you remember? (my memory tells me Stefan added this fixing a
> number of issues).

I didn't see any obvious cases where it was false, but I might have missed one.

https://codereview.chromium.org/140323010/diff/200001/ash/resources/ash_resou...
File ash/resources/ash_resources.grd (left):

https://codereview.chromium.org/140323010/diff/200001/ash/resources/ash_resou...
ash/resources/ash_resources.grd:32: <structure type="chrome_scaled_image"
name="IDR_AURA_LAUNCHER_UNDERLINE_HOVER"
file="common/launcher/launcher_underline_bottom_hover.png" />
You might want to delete the art assets in a different CL, otherwise you won't
be able to use the commit queue due to binary files changes in the patch. 
(Sorry if I didn't make it clear that it was OK to do that in another CL.)

[Mr4D (OOO till 08-26), 2014-01-25 01:37:37]

lgtm

https://codereview.chromium.org/140323010/diff/30001/ash/system/tray/system_t...
File ash/system/tray/system_tray.cc (right):

https://codereview.chromium.org/140323010/diff/30001/ash/system/tray/system_t...
ash/system/tray/system_tray.cc:489: init_params.first_item_has_no_margin = true;
Hmm. The Bubble code gets used in many places. e.g.
MessageCenterBubble::GetInitParam creates a views::TrayBubbleView::InitParams
and sets this member with a variable which got passed into the constructor.

I don't want to exclude the possibility that it might be obsolete (many things
have changed) but the default is false, we are setting oit to true and others
are also using it. More investigation would be necessary. Maybe another CL?

https://codereview.chromium.org/140323010/diff/200001/ash/shelf/shelf_view.cc
File ash/shelf/shelf_view.cc (right):

https://codereview.chromium.org/140323010/diff/200001/ash/shelf/shelf_view.cc...
ash/shelf/shelf_view.cc:748: last_hidden_index_ >= first_panel_index);
isn't here a space missing?

https://codereview.chromium.org/140323010/diff/200001/ash/shelf/shelf_view.cc...
ash/shelf/shelf_view.cc:1365: return gfx::Size(kShelfSize,
This might fit into one line.

[commit-bot: I haz the power, 2014-01-28 01:08:06]

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

[commit-bot: I haz the power, 2014-01-28 02:01:04]

Retried try job too often on win_rel for step(s) telemetry_perf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[James Cook, 2014-02-25 20:36:56]

On 2014/01/28 02:01:04, I haz the power (commit-bot) wrote:
> Retried try job too often on win_rel for step(s) telemetry_perf_unittests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Is this patch still active? Can it be landed, or if the change landed elsewhere,
closed?

[Harry McCleave, 2014-02-28 21:22:17]

The CQ bit was checked by harrym@chromium.org

[commit-bot: I haz the power, 2014-02-28 21:23:31]

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

[commit-bot: I haz the power, 2014-02-28 21:41:52]

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

[commit-bot: I haz the power, 2014-02-28 21:41:52]

Try jobs failed on following builders: linux_chromium_rel

[Harry McCleave, 2014-02-28 23:40:47]

The CQ bit was checked by harrym@chromium.org

[commit-bot: I haz the power, 2014-02-28 23:41:45]

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

[commit-bot: I haz the power, 2014-02-28 23:42:01]

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

[commit-bot: I haz the power, 2014-02-28 23:42:02]

Failed to apply patch for ash/system/web_notification/web_notification_tray.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file ash/system/web_notification/web_notification_tray.cc
  Hunk #4 succeeded at 345 (offset 10 lines).
  Hunk #5 FAILED at 406.
  1 out of 5 hunks FAILED -- saving rejects to file
ash/system/web_notification/web_notification_tray.cc.rej

Patch:       ash/system/web_notification/web_notification_tray.cc
Index: ash/system/web_notification/web_notification_tray.cc
diff --git a/ash/system/web_notification/web_notification_tray.cc
b/ash/system/web_notification/web_notification_tray.cc
index
e8218610bdf0dbd0e65c51e89af42cbcd1dc4088..d20e1dfe38ac0f78bc03128286a170bfc6bf3fe8
100644
--- a/ash/system/web_notification/web_notification_tray.cc
+++ b/ash/system/web_notification/web_notification_tray.cc
@@ -124,11 +124,10 @@ void WorkAreaObserver::SetSystemTrayHeight(int height) {
   // should be reduced by the height of shelf's shown height.
   if (shelf_ && shelf_->visibility_state() == SHELF_AUTO_HIDE &&
       shelf_->auto_hide_state() == SHELF_AUTO_HIDE_SHOWN) {
-    system_tray_height_ -= ShelfLayoutManager::GetPreferredShelfSize() -
-        ShelfLayoutManager::kAutoHideSize;
+    system_tray_height_ -= kShelfSize - ShelfLayoutManager::kAutoHideSize;
   }
 
-  if (system_tray_height_ > 0 && ash::switches::UseAlternateShelfLayout())
+  if (system_tray_height_ > 0)
     system_tray_height_ += message_center::kMarginBetweenItems;
 
   if (!shelf_)
@@ -174,8 +173,7 @@ void
WorkAreaObserver::OnAutoHideStateChanged(ShelfAutoHideState new_state) {
       new_state == SHELF_AUTO_HIDE_SHOWN) {
     // Since the work_area is already reduced by kAutoHideSize, the inset width
     // should be just the difference.
-    width = ShelfLayoutManager::GetPreferredShelfSize() -
-        ShelfLayoutManager::kAutoHideSize;
+    width = kShelfSize - ShelfLayoutManager::kAutoHideSize;
   }
   work_area.Inset(shelf_->SelectValueForShelfAlignment(
       gfx::Insets(0, 0, width, 0),
@@ -220,8 +218,7 @@ class WebNotificationBubbleWrapper {
     }
     views::TrayBubbleView* bubble_view = views::TrayBubbleView::Create(
         tray->GetBubbleWindowContainer(), anchor, tray, &init_params);
-    if (ash::switches::UseAlternateShelfLayout())
-      bubble_view->SetArrowPaintType(views::BubbleBorder::PAINT_NONE);
+    bubble_view->SetArrowPaintType(views::BubbleBorder::PAINT_NONE);
     bubble_wrapper_.reset(new TrayBubbleWrapper(tray, bubble_view));
     bubble->InitializeContents(bubble_view);
   }
@@ -338,7 +335,7 @@ bool WebNotificationTray::ShowMessageCenterInternal(bool
show_settings) {
       new message_center::MessageCenterBubble(
           message_center(),
           message_center_tray_.get(),
-          ash::switches::UseAlternateShelfLayout());
+          true);
 
   int max_height = 0;
   aura::Window* status_area_window = status_area_widget()->GetNativeView();
@@ -409,7 +406,7 @@ bool WebNotificationTray::ShowPopups() {
           internal::kShellWindowId_StatusContainer),
       message_center(),
       message_center_tray_.get(),
-      ash::switches::UseAlternateShelfLayout()));
+      true));
   work_area_observer_->StartObserving(
       popup_collection_.get(), GetWidget()->GetNativeView()->GetRootWindow());
   return true;

[Harry McCleave, 2014-03-01 00:10:45]

The CQ bit was checked by harrym@chromium.org

[commit-bot: I haz the power, 2014-03-01 00:13:34]

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

[Harry McCleave, 2014-03-01 00:46:43]

The CQ bit was unchecked by harrym@chromium.org

[Harry McCleave, 2014-03-01 00:47:36]

Committed patchset #9 manually as r254301 (presubmit successful).

[dgrogan, 2014-03-01 01:02:10]

A revert of this CL has been created in
https://codereview.chromium.org/185143002/ by dgrogan@chromium.org.

The reason for reverting is: Broke Linux ChromiumOS Builder (dbg):

http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2...

FAILED: /b/build/goma/gomacc c++ -MMD -MF
obj/ash/system/web_notification/ash.web_notification_tray.o.d ....
../../ash/system/web_notification/web_notification_tray.cc: In constructor
'ash::WebNotificationTray::WebNotificationTray(ash::internal::StatusAreaWidget*)':
../../ash/system/web_notification/web_notification_tray.cc:322:7:error:
'UseAlternateShelfLayout' is not a member of 'ash::switches'
../../ui/message_center/message_center_style.h: At global scope:
../../ui/message_center/message_center_style.h:103:15:error:
'message_center::kButtonSeparatorColor' defined but not used
[-Werror=unused-variable]
../../ui/message_center/message_center_style.h:104:15:error:
'message_center::kHoveredButtonBackgroundColor' defined but not used
[-Werror=unused-variable]
../../ui/message_center/message_center_style.h:111:15:error:
'message_center::kProgressBarBackgroundColor' defined but not used
[-Werror=unused-variable]
../../ui/message_center/message_center_style.h:112:15:error:
'message_center::kProgressBarSliceColor' defined but not used
[-Werror=unused-variable]
../../ui/message_center/message_center_style.h:43:15:error:
'message_center::settings::kEntrySeparatorColor' defined but not used
[-Werror=unused-variable]
.

[Peter Kasting, 2014-03-01 01:03:49]

This broke the Blink compile too, and looking at the try runs, they show Windows
compile failures.

Please don't proceed to commit without looking at your try runs...

[vmiura, 2014-03-01 01:38:23]

Please look at the Try results, and let CQ do it's work.
Both would prevent this build break.

[Harry McCleave, 2014-03-01 01:44:54]

On 2014/03/01 01:38:23, vmiura wrote:
> Please look at the Try results, and let CQ do it's work.
> Both would prevent this build break.

Very sorry everyone, local changes clearly did not make this patch and I was too
hasty to submit /w local build working and couple of quick try bots that passed,
I will split out the binary files to another patch to proceed with a direct CQ
submission and will be more patient in the future.