ash:Shelf - Enable Alternate Shelf and Side Shelf by default.

ash:Shelf - Enable Alternate Shelf and Side Shelf by default.

Updated and/or duplicated (updated with copy renamed *ForLegacyShelfLayout) unit tests as required.

BUG=246279
TBR=jamescook@chromium.org, skuhne@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221325

[Harry McCleave, 2013-08-21 21:34:42]

ptal

https://codereview.chromium.org/22793011/diff/1/chrome/browser/ui/ash/launche...
File
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_browser_unittest.cc
(right):

https://codereview.chromium.org/22793011/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_browser_unittest.cc:44:
CommandLine::ForCurrentProcess()->AppendSwitch(
ChromeLaun...PerBrowser is being removed (cl in the cq atm iirc) thus these
tests are not being updated [duplicated in ChromeLaun...PerApp]

[Mr4D (OOO till 08-26), 2013-08-21 23:56:56]

Let's wait until the PerBrowser removal has been done and you sync'ed your code?
There will be several changes then...

[Harry McCleave, 2013-08-22 00:00:46]

On 2013/08/21 23:56:56, Mr4D wrote:
> Let's wait until the PerBrowser removal has been done and you sync'ed your
code?
> There will be several changes then...

Will do, though "There will be several changes then..." sounds very ominous :P

[James Cook, 2013-08-22 00:19:12]

Oops, I just reviewed this.

In general I'm OK with duplicating tests that are doing a lot of checking of
pixel widths/heights/bounds.

It would be nice if you could take a look at the tests that are being duplicated
because the browser icon moved and see if you could introduce a variable that
represents where the icon is expected to be and remove the test duplication.

Something like:
EXPECT_EQ(browser_icon_index() + 1, foo_index);

Instead of duplicating tests for values 0 and 1

https://codereview.chromium.org/22793011/diff/1/ash/launcher/launcher_navigat...
File ash/launcher/launcher_navigator_unittest.cc (right):

https://codereview.chromium.org/22793011/diff/1/ash/launcher/launcher_navigat...
ash/launcher/launcher_navigator_unittest.cc:95: // Fifth one.  It will skip the
APP_SHORTCUT and APP_LIST  at the beginning
nit: one space after APP_LIST

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

https://codereview.chromium.org/22793011/diff/1/ash/launcher/launcher_view.cc...
ash/launcher/launcher_view.cc:795: last_visible_index_ > 0 &&
last_visible_index_ < last_button_index)
Should this be part of a separate CL, or is it needed to get tests to pass?

https://codereview.chromium.org/22793011/diff/1/ash/launcher/launcher_view_un...
File ash/launcher/launcher_view_unittest.cc (right):

https://codereview.chromium.org/22793011/diff/1/ash/launcher/launcher_view_un...
ash/launcher/launcher_view_unittest.cc:413: LauncherViewLegacyShelfLayoutTest()
: LauncherViewTest() {}
see below, but this could do something like:

{
  set_browser_shortcut_index(0);
  set_app_shortcut_index(1);
}

https://codereview.chromium.org/22793011/diff/1/ash/launcher/launcher_view_un...
ash/launcher/launcher_view_unittest.cc:433: virtual void SetupForDragTest(
This feels to me like it doesn't need to be duplicated, especially since you're
just changing i+1 to i+2.  Maybe put this functionality in the base class and
provide a variable that sets the initial index of app shortcuts.

https://codereview.chromium.org/22793011/diff/1/ash/root_window_controller_un...
File ash/root_window_controller_unittest.cc (right):

https://codereview.chromium.org/22793011/diff/1/ash/root_window_controller_un...
ash/root_window_controller_unittest.cc:154: // Disabled here as the work_area
insets here are hard coded they are
nit: period after "hard coded"

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

https://codereview.chromium.org/22793011/diff/1/ash/shelf/shelf_layout_manage...
ash/shelf/shelf_layout_manager.cc:895:
target_bounds->status_bounds_in_shelf.set_x(kStatusAreaInset);
I don't understand why you are reversing this.  Why does SHELF_ALIGNMENT_LEFT
need to do the math with available_bounds.right()?

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

https://codereview.chromium.org/22793011/diff/1/ash/shelf/shelf_layout_manage...
ash/shelf/shelf_layout_manager_unittest.cc:713:
screen->GetPrimaryDisplay().bounds().bottom());
nit: align indent

https://codereview.chromium.org/22793011/diff/1/ash/shelf/shelf_layout_manage...
ash/shelf/shelf_layout_manager_unittest.cc:716:
screen->GetPrimaryDisplay().bounds().bottom());
nit: align indent

https://codereview.chromium.org/22793011/diff/1/ash/shelf/shelf_widget_unitte...
File ash/shelf/shelf_widget_unittest.cc (right):

https://codereview.chromium.org/22793011/diff/1/ash/shelf/shelf_widget_unitte...
ash/shelf/shelf_widget_unittest.cc:7: #include "ash/ash_switches.h"
Is this needed?

https://codereview.chromium.org/22793011/diff/1/ash/shelf/shelf_widget_unitte...
ash/shelf/shelf_widget_unittest.cc:18: #include "base/command_line.h"
Is this needed?

https://codereview.chromium.org/22793011/diff/1/ash/wm/custom_frame_view_ash_...
File ash/wm/custom_frame_view_ash_unittest.cc (right):

https://codereview.chromium.org/22793011/diff/1/ash/wm/custom_frame_view_ash_...
ash/wm/custom_frame_view_ash_unittest.cc:7: #include "ash/ash_switches.h"
Is this needed?

https://codereview.chromium.org/22793011/diff/1/ash/wm/workspace/workspace_la...
File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/22793011/diff/1/ash/wm/workspace/workspace_la...
ash/wm/workspace/workspace_layout_manager_unittest.cc:16: #include
"base/command_line.h"
Is this needed?

https://codereview.chromium.org/22793011/diff/1/chrome/browser/ui/ash/launche...
File
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_browser_unittest.cc
(right):

https://codereview.chromium.org/22793011/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_browser_unittest.cc:44:
CommandLine::ForCurrentProcess()->AppendSwitch(
On 2013/08/21 21:34:43, Harry McCleave wrote:
> ChromeLaun...PerBrowser is being removed (cl in the cq atm iirc) thus these
> tests are not being updated [duplicated in ChromeLaun...PerApp]

Just FYI: The original removal got reverted, but I'm landing a new removal in
https://codereview.chromium.org/23068021/

[simonhong_, 2013-08-22 21:56:16]

On 2013/08/22 00:00:46, Harry McCleave wrote:
> On 2013/08/21 23:56:56, Mr4D wrote:
> > Let's wait until the PerBrowser removal has been done and you sync'ed your
> code?
> > There will be several changes then...
> 
> Will do, though "There will be several changes then..." sounds very ominous :P

Finally, PerBrowser is removed.
FYI, PersistLauncherItemPositions is moved into
chrome_launcher_controller_unittest.cc
And PersistLauncherItemPositionsPerBrowser is removed.

[Harry McCleave, 2013-08-27 00:36:59]

https://codereview.chromium.org/22793011/diff/1/ash/launcher/launcher_navigat...
File ash/launcher/launcher_navigator_unittest.cc (right):

https://codereview.chromium.org/22793011/diff/1/ash/launcher/launcher_navigat...
ash/launcher/launcher_navigator_unittest.cc:95: // Fifth one.  It will skip the
APP_SHORTCUT and APP_LIST  at the beginning
On 2013/08/22 00:19:12, James Cook wrote:
> nit: one space after APP_LIST

Done.

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

https://codereview.chromium.org/22793011/diff/1/ash/launcher/launcher_view.cc...
ash/launcher/launcher_view.cc:795: last_visible_index_ > 0 &&
last_visible_index_ < last_button_index)
On 2013/08/22 00:19:12, James Cook wrote:
> Should this be part of a separate CL, or is it needed to get tests to pass?

This fixes an issue asserted by a unit test, if there is enough space to show
all the buttons but overflow state is triggered by panels or notifications
(stuff from the right) without this a button would be incorrectly hidden to
create space for the overflow (rather than correctly hiding an element from the
right).

https://codereview.chromium.org/22793011/diff/1/ash/launcher/launcher_view_un...
File ash/launcher/launcher_view_unittest.cc (right):

https://codereview.chromium.org/22793011/diff/1/ash/launcher/launcher_view_un...
ash/launcher/launcher_view_unittest.cc:413: LauncherViewLegacyShelfLayoutTest()
: LauncherViewTest() {}
On 2013/08/22 00:19:12, James Cook wrote:
> see below, but this could do something like:
> 
> {
>   set_browser_shortcut_index(0);
>   set_app_shortcut_index(1);
> }

Done.

https://codereview.chromium.org/22793011/diff/1/ash/launcher/launcher_view_un...
ash/launcher/launcher_view_unittest.cc:433: virtual void SetupForDragTest(
On 2013/08/22 00:19:12, James Cook wrote:
> This feels to me like it doesn't need to be duplicated, especially since
you're
> just changing i+1 to i+2.  Maybe put this functionality in the base class and
> provide a variable that sets the initial index of app shortcuts.

Done.

https://codereview.chromium.org/22793011/diff/1/ash/root_window_controller_un...
File ash/root_window_controller_unittest.cc (right):

https://codereview.chromium.org/22793011/diff/1/ash/root_window_controller_un...
ash/root_window_controller_unittest.cc:154: // Disabled here as the work_area
insets here are hard coded they are
On 2013/08/22 00:19:12, James Cook wrote:
> nit: period after "hard coded"

Done.

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

https://codereview.chromium.org/22793011/diff/1/ash/shelf/shelf_layout_manage...
ash/shelf/shelf_layout_manager.cc:895:
target_bounds->status_bounds_in_shelf.set_x(kStatusAreaInset);
On 2013/08/22 00:19:12, James Cook wrote:
> I don't understand why you are reversing this.  Why does SHELF_ALIGNMENT_LEFT
> need to do the math with available_bounds.right()?

The code here was wrong (caught by GestureDrag unit tests), the inset needs to
be kept constant on they inside edge of the shelf in the alternate shelf layout
so when the shelf is on the left the right side of the status area is what is
being positioned.

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

https://codereview.chromium.org/22793011/diff/1/ash/shelf/shelf_layout_manage...
ash/shelf/shelf_layout_manager_unittest.cc:713:
screen->GetPrimaryDisplay().bounds().bottom());
On 2013/08/22 00:19:12, James Cook wrote:
> nit: align indent

Done.

https://codereview.chromium.org/22793011/diff/1/ash/shelf/shelf_layout_manage...
ash/shelf/shelf_layout_manager_unittest.cc:716:
screen->GetPrimaryDisplay().bounds().bottom());
On 2013/08/22 00:19:12, James Cook wrote:
> nit: align indent

Done.

https://codereview.chromium.org/22793011/diff/1/ash/shelf/shelf_widget_unitte...
File ash/shelf/shelf_widget_unittest.cc (right):

https://codereview.chromium.org/22793011/diff/1/ash/shelf/shelf_widget_unitte...
ash/shelf/shelf_widget_unittest.cc:7: #include "ash/ash_switches.h"
On 2013/08/22 00:19:12, James Cook wrote:
> Is this needed?

Done.

https://codereview.chromium.org/22793011/diff/1/ash/shelf/shelf_widget_unitte...
ash/shelf/shelf_widget_unittest.cc:18: #include "base/command_line.h"
On 2013/08/22 00:19:12, James Cook wrote:
> Is this needed?

Done.

https://codereview.chromium.org/22793011/diff/1/ash/wm/custom_frame_view_ash_...
File ash/wm/custom_frame_view_ash_unittest.cc (right):

https://codereview.chromium.org/22793011/diff/1/ash/wm/custom_frame_view_ash_...
ash/wm/custom_frame_view_ash_unittest.cc:7: #include "ash/ash_switches.h"
On 2013/08/22 00:19:12, James Cook wrote:
> Is this needed?

Done.

https://codereview.chromium.org/22793011/diff/1/ash/wm/workspace/workspace_la...
File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/22793011/diff/1/ash/wm/workspace/workspace_la...
ash/wm/workspace/workspace_layout_manager_unittest.cc:16: #include
"base/command_line.h"
On 2013/08/22 00:19:12, James Cook wrote:
> Is this needed?

Done.

[Mr4D (OOO till 08-26), 2013-08-27 02:35:14]

Only nits. Nearly done. 

Wow - that looked like a big chunk of work adding all the unit tests for the
transition! Many thanks for doing that!

https://codereview.chromium.org/22793011/diff/16001/ash/launcher/launcher_mod...
File ash/launcher/launcher_model_unittest.cc (right):

https://codereview.chromium.org/22793011/diff/16001/ash/launcher/launcher_mod...
ash/launcher/launcher_model_unittest.cc:157: // APP_SHORTCUT's priority is
higher than TABBED but same as
Usually we use |APP_SHORTCUT| and so in comments.

https://codereview.chromium.org/22793011/diff/16001/ash/launcher/launcher_vie...
File ash/launcher/launcher_view_unittest.cc (right):

https://codereview.chromium.org/22793011/diff/16001/ash/launcher/launcher_vie...
ash/launcher/launcher_view_unittest.cc:622: // Add tabbed browser until
overflow, remember last visible tabbed browser.
Somehow this isn't really a sentence.

https://codereview.chromium.org/22793011/diff/16001/ash/launcher/launcher_vie...
ash/launcher/launcher_view_unittest.cc:863: // Adding a launcher item at the end
(i.e. a panel)  canels drag and respects
cancels I suppose?

https://codereview.chromium.org/22793011/diff/16001/ash/launcher/launcher_vie...
ash/launcher/launcher_view_unittest.cc:959:
(void)button_host;(void)dragged_button_mouse;(void)dragged_button_touch;
?

https://codereview.chromium.org/22793011/diff/16001/ash/root_window_controlle...
File ash/root_window_controller_unittest.cc (right):

https://codereview.chromium.org/22793011/diff/16001/ash/root_window_controlle...
ash/root_window_controller_unittest.cc:174: EXPECT_EQ("600,0 500x453",
maximized->GetWindowBoundsInScreen().ToString());
Odd. Is the +1 because of the new shelf layout?

https://codereview.chromium.org/22793011/diff/16001/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc
(right):

https://codereview.chromium.org/22793011/diff/16001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:1465:
EXPECT_EQ(ash::TYPE_BROWSER_SHORTCUT, model_->items()[3].type);
Would it make sense to test if you can move the AppList (which you can't)? I
just run across a problem with that today.

[Harry McCleave, 2013-08-27 03:25:45]

https://codereview.chromium.org/22793011/diff/16001/ash/launcher/launcher_mod...
File ash/launcher/launcher_model_unittest.cc (right):

https://codereview.chromium.org/22793011/diff/16001/ash/launcher/launcher_mod...
ash/launcher/launcher_model_unittest.cc:157: // APP_SHORTCUT's priority is
higher than TABBED but same as
On 2013/08/27 02:35:14, Mr4D wrote:
> Usually we use |APP_SHORTCUT| and so in comments.

Done.

https://codereview.chromium.org/22793011/diff/16001/ash/launcher/launcher_vie...
File ash/launcher/launcher_view_unittest.cc (right):

https://codereview.chromium.org/22793011/diff/16001/ash/launcher/launcher_vie...
ash/launcher/launcher_view_unittest.cc:622: // Add tabbed browser until
overflow, remember last visible tabbed browser.
On 2013/08/27 02:35:14, Mr4D wrote:
> Somehow this isn't really a sentence.

Done.

https://codereview.chromium.org/22793011/diff/16001/ash/launcher/launcher_vie...
ash/launcher/launcher_view_unittest.cc:959:
(void)button_host;(void)dragged_button_mouse;(void)dragged_button_touch;
On 2013/08/27 02:35:14, Mr4D wrote:
> ?

Done.

https://codereview.chromium.org/22793011/diff/16001/ash/root_window_controlle...
File ash/root_window_controller_unittest.cc (right):

https://codereview.chromium.org/22793011/diff/16001/ash/root_window_controlle...
ash/root_window_controller_unittest.cc:174: EXPECT_EQ("600,0 500x453",
maximized->GetWindowBoundsInScreen().ToString());
On 2013/08/27 02:35:14, Mr4D wrote:
> Odd. Is the +1 because of the new shelf layout?

Yes, shelf is 1 px smaller (oh yes, soo much fun for so small a change).

https://codereview.chromium.org/22793011/diff/16001/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc
(right):

https://codereview.chromium.org/22793011/diff/16001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:1465:
EXPECT_EQ(ash::TYPE_BROWSER_SHORTCUT, model_->items()[3].type);
On 2013/08/27 02:35:14, Mr4D wrote:
> Would it make sense to test if you can move the AppList (which you can't)? I
> just run across a problem with that today.

Done.

[Mr4D (OOO till 08-26), 2013-08-27 14:13:36]

lgtm

https://codereview.chromium.org/22793011/diff/16001/ash/launcher/launcher_mod...
File ash/launcher/launcher_model_unittest.cc (right):

https://codereview.chromium.org/22793011/diff/16001/ash/launcher/launcher_mod...
ash/launcher/launcher_model_unittest.cc:157: // APP_SHORTCUT's priority is
higher than TABBED but same as
Hmmm. Where? I still see the // APP_SHORTCUT priority is ... and not //
|APP_SHORTCUT| priority ..

https://codereview.chromium.org/22793011/diff/16001/ash/root_window_controlle...
File ash/root_window_controller_unittest.cc (right):

https://codereview.chromium.org/22793011/diff/16001/ash/root_window_controlle...
ash/root_window_controller_unittest.cc:174: EXPECT_EQ("600,0 500x453",
maximized->GetWindowBoundsInScreen().ToString());
Since this was such a small change with such a huge fallout + our designers like
to change things around by one pixel I would have possibly created a global
factor and name it like "kHorizontalShelfSize" and calculated the size in these
commands. That way a change to a new size would have been much less work. So
maybe not this time, but next time. :)

[James Cook, 2013-08-27 17:08:52]

lgtm with nits

https://codereview.chromium.org/22793011/diff/26001/ash/launcher/launcher_vie...
File ash/launcher/launcher_view_unittest.cc (right):

https://codereview.chromium.org/22793011/diff/26001/ash/launcher/launcher_vie...
ash/launcher/launcher_view_unittest.cc:864: // Adding a launcher item at the end
(i.e. a panel)  cancels drag and respects
nit: one space before "cancels"

https://codereview.chromium.org/22793011/diff/26001/ash/launcher/launcher_vie...
ash/launcher/launcher_view_unittest.cc:931: // Adding a launcher item at the end
(i.e. a panel)  cancels drag and respects
nit: one space before "cancels"

[commit-bot: I haz the power, 2013-08-27 20:07:34]

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

[commit-bot: I haz the power, 2013-08-28 00:34:35]

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

[commit-bot: I haz the power, 2013-08-28 04:02:21]

Retried try job too often on linux_chromeos for step(s) interactive_ui_tests,
unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-09-04 22:24:54]

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

[commit-bot: I haz the power, 2013-09-04 23:14:14]

Retried try job too often on ios_dbg_simulator for step(s) ui_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2013-09-04 23:16:14]

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

[commit-bot: I haz the power, 2013-09-04 23:27:32]

Sorry for I got bad news for ya.
Compile failed with a clobber build on ios_dbg_simulator.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-04 23:35:31]

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

[commit-bot: I haz the power, 2013-09-05 00:31:15]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[Harry McCleave, 2013-09-05 01:09:06]

Committed patchset #8 manually as r221325 (presubmit successful).