Shelf Cleanup AlternateShelfLayout P1 Attempt 3

R=skuhne@chromium.org
TBR=jamescook@chromium.org, miket@chromium.org, skuhne@chromium.org
BUG=338429
re-landing of https://codereview.chromium.org/176883022/ with fix for the inset of app panel as discussed offline /w jennyz@

Review URL: https://codereview.chromium.org/229453005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@263451 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 11 insertions, 232 deletions

diff --git a/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc b/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc
index bdfb818..656f47a 100644
--- a/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc
+++ b/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc
@@ -1054,8 +1054,7 @@ void ChromeLauncherController::ShelfItemAdded(int index) {
   // The app list launcher can get added to the shelf after we applied the
   // preferences. In that case the item might be at the wrong spot. As such we
   // call the function again.
-  if (model_->items()[index].type == ash::TYPE_APP_LIST &&
-      ash::switches::UseAlternateShelfLayout())
+  if (model_->items()[index].type == ash::TYPE_APP_LIST)
     UpdateAppLaunchersFromPref();
 }
 
@@ -1068,8 +1067,7 @@ void ChromeLauncherController::ShelfItemMoved(int start_index,
   // We remember the moved item position if it is either pinnable or
   // it is the app list with the alternate shelf layout.
   if ((HasItemController(item.id) && IsPinned(item.id)) ||
-      (ash::switches::UseAlternateShelfLayout() &&
-       item.type == ash::TYPE_APP_LIST))
+       item.type == ash::TYPE_APP_LIST)
     PersistPinnedState();
 }
 
@@ -1817,18 +1815,17 @@ void ChromeLauncherController::MoveChromeOrApplistToFinalPosition(
 }
 
 int ChromeLauncherController::FindInsertionPoint(bool is_app_list) {
-  bool alternate = ash::switches::UseAlternateShelfLayout();
   // Keeping this change small to backport to M33&32 (see crbug.com/329597).
   // TODO(skuhne): With the removal of the legacy shelf layout we should remove
   // the ability to move the app list item since this was never used. We should
   // instead ask the ShelfModel::ValidateInsertionIndex or similir for an index.
-  if (is_app_list && alternate)
+  if (is_app_list)
     return 0;
 
   for (int i = model_->item_count() - 1; i > 0; --i) {
     ash::ShelfItemType type = model_->items()[i].type;
     if (type == ash::TYPE_APP_SHORTCUT ||
-        ((is_app_list || alternate) && type == ash::TYPE_APP_LIST) ||
+        (is_app_list && type == ash::TYPE_APP_LIST) ||
         type == ash::TYPE_BROWSER_SHORTCUT) {
       return i;
     }
@@ -1918,10 +1915,7 @@ ChromeLauncherController::GetListOfPinnedAppsAndBrowser() {
   // If not added yet, add the app list item either at the end or at the
   // beginning - depending on the shelf layout.
   if (!app_list_icon_added) {
-    if (ash::switches::UseAlternateShelfLayout())
-      pinned_apps.insert(pinned_apps.begin(), kAppShelfIdPlaceholder);
-    else
-      pinned_apps.push_back(kAppShelfIdPlaceholder);
+    pinned_apps.insert(pinned_apps.begin(), kAppShelfIdPlaceholder);
   }
   return pinned_apps;
 }
diff --git a/chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc b/chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc
index 34576c8..800eecb 100644
--- a/chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc
+++ b/chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc
@@ -11,6 +11,7 @@
 #include "ash/display/display_controller.h"
 #include "ash/shelf/shelf.h"
 #include "ash/shelf/shelf_button.h"
+#include "ash/shelf/shelf_constants.h"
 #include "ash/shelf/shelf_model.h"
 #include "ash/shelf/shelf_util.h"
 #include "ash/shelf/shelf_view.h"
@@ -820,7 +821,7 @@ IN_PROC_BROWSER_TEST_F(LauncherPlatformAppBrowserTest, SetIcon) {
   EXPECT_TRUE(app_item_controller->image_set_by_controller());
   EXPECT_TRUE(panel_item_controller->image_set_by_controller());
   // Ensure icon heights are correct (see test.js in app_icon/ test directory)
-  EXPECT_EQ(48, app_item.image.height());
+  EXPECT_EQ(ash::kShelfSize, app_item.image.height());
   EXPECT_EQ(64, panel_item.image.height());
 }
 
diff --git a/chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc b/chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc
index 7dd122d..aee63e8 100644
--- a/chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc
+++ b/chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc
@@ -618,27 +618,6 @@ class ChromeLauncherControllerTest : public BrowserWithTestWindowTest {
   DISALLOW_COPY_AND_ASSIGN(ChromeLauncherControllerTest);
 };
 
-// The testing framework to test the legacy shelf layout.
-class LegacyShelfLayoutChromeLauncherControllerTest
-    : public ChromeLauncherControllerTest {
- protected:
-  LegacyShelfLayoutChromeLauncherControllerTest() {
-  }
-
-  virtual ~LegacyShelfLayoutChromeLauncherControllerTest() {
-  }
-
-  // Overwrite the Setup function to use the legacy shelf layout option.
-  virtual void SetUp() OVERRIDE {
-    CommandLine::ForCurrentProcess()->AppendSwitch(
-        ash::switches::kAshDisableAlternateShelfLayout);
-    ChromeLauncherControllerTest::SetUp();
-  }
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(LegacyShelfLayoutChromeLauncherControllerTest);
-};
-
 #if defined(OS_CHROMEOS)
 // A browser window proxy which is able to associate an aura native window with
 // it.
@@ -967,178 +946,6 @@ class MultiProfileMultiBrowserShelfLayoutChromeLauncherControllerTest
 };
 #endif  // defined(OS_CHROMEOS)
 
-TEST_F(LegacyShelfLayoutChromeLauncherControllerTest, DefaultApps) {
-  InitLauncherController();
-  // Model should only contain the browser shortcut and app list items.
-  EXPECT_EQ(2, model_->item_count());
-  EXPECT_FALSE(launcher_controller_->IsAppPinned(extension1_->id()));
-  EXPECT_FALSE(launcher_controller_->IsAppPinned(extension2_->id()));
-  EXPECT_FALSE(launcher_controller_->IsAppPinned(extension3_->id()));
-
-  // Installing |extension3_| should add it to the launcher - behind the
-  // chrome icon.
-  extension_service_->AddExtension(extension3_.get());
-  EXPECT_EQ("Chrome, App3, AppList", GetPinnedAppStatus());
-  EXPECT_FALSE(launcher_controller_->IsAppPinned(extension1_->id()));
-  EXPECT_FALSE(launcher_controller_->IsAppPinned(extension2_->id()));
-}
-
-// Check that the restauration of launcher items is happening in the same order
-// as the user has pinned them (on another system) when they are synced reverse
-// order.
-TEST_F(LegacyShelfLayoutChromeLauncherControllerTest,
-    RestoreDefaultAppsReverseOrder) {
-  InitLauncherController();
-
-  base::ListValue policy_value;
-  InsertPrefValue(&policy_value, 0, extension1_->id());
-  InsertPrefValue(&policy_value, 1, extension2_->id());
-  InsertPrefValue(&policy_value, 2, extension3_->id());
-  profile()->GetTestingPrefService()->SetUserPref(prefs::kPinnedLauncherApps,
-                                                  policy_value.DeepCopy());
-  EXPECT_EQ(0, profile()->GetPrefs()->GetInteger(prefs::kShelfChromeIconIndex));
-  // Model should only contain the browser shortcut and app list items.
-  EXPECT_FALSE(launcher_controller_->IsAppPinned(extension1_->id()));
-  EXPECT_FALSE(launcher_controller_->IsAppPinned(extension2_->id()));
-  EXPECT_FALSE(launcher_controller_->IsAppPinned(extension3_->id()));
-  EXPECT_EQ("Chrome, AppList", GetPinnedAppStatus());
-
-  // Installing |extension3_| should add it to the shelf - behind the
-  // chrome icon.
-  ash::ShelfItem item;
-  extension_service_->AddExtension(extension3_.get());
-  EXPECT_FALSE(launcher_controller_->IsAppPinned(extension1_->id()));
-  EXPECT_FALSE(launcher_controller_->IsAppPinned(extension2_->id()));
-  EXPECT_EQ("Chrome, App3, AppList", GetPinnedAppStatus());
-
-  // Installing |extension2_| should add it to the launcher - behind the
-  // chrome icon, but in first location.
-  extension_service_->AddExtension(extension2_.get());
-  EXPECT_FALSE(launcher_controller_->IsAppPinned(extension1_->id()));
-  EXPECT_EQ("Chrome, App2, App3, AppList", GetPinnedAppStatus());
-
-  // Installing |extension1_| should add it to the launcher - behind the
-  // chrome icon, but in first location.
-  extension_service_->AddExtension(extension1_.get());
-  EXPECT_EQ("Chrome, App1, App2, App3, AppList", GetPinnedAppStatus());
-}
-
-// Check that the restauration of launcher items is happening in the same order
-// as the user has pinned them (on another system) when they are synced random
-// order.
-TEST_F(LegacyShelfLayoutChromeLauncherControllerTest,
-    RestoreDefaultAppsRandomOrder) {
-  InitLauncherController();
-
-  base::ListValue policy_value;
-  InsertPrefValue(&policy_value, 0, extension1_->id());
-  InsertPrefValue(&policy_value, 1, extension2_->id());
-  InsertPrefValue(&policy_value, 2, extension3_->id());
-  profile()->GetTestingPrefService()->SetUserPref(prefs::kPinnedLauncherApps,
-                                                  policy_value.DeepCopy());
-  EXPECT_EQ(0, profile()->GetPrefs()->GetInteger(prefs::kShelfChromeIconIndex));
-  // Model should only contain the browser shortcut and app list items.
-  EXPECT_FALSE(launcher_controller_->IsAppPinned(extension1_->id()));
-  EXPECT_FALSE(launcher_controller_->IsAppPinned(extension2_->id()));
-  EXPECT_FALSE(launcher_controller_->IsAppPinned(extension3_->id()));
-  EXPECT_EQ("Chrome, AppList", GetPinnedAppStatus());
-
-  // Installing |extension2_| should add it to the launcher - behind the
-  // chrome icon.
-  extension_service_->AddExtension(extension2_.get());
-  EXPECT_FALSE(launcher_controller_->IsAppPinned(extension1_->id()));
-  EXPECT_FALSE(launcher_controller_->IsAppPinned(extension3_->id()));
-  EXPECT_EQ("Chrome, App2, AppList", GetPinnedAppStatus());
-
-  // Installing |extension1_| should add it to the launcher - behind the
-  // chrome icon, but in first location.
-  extension_service_->AddExtension(extension1_.get());
-  EXPECT_FALSE(launcher_controller_->IsAppPinned(extension3_->id()));
-  EXPECT_EQ("Chrome, App1, App2, AppList", GetPinnedAppStatus());
-
-  // Installing |extension3_| should add it to the launcher - behind the
-  // chrome icon, but in first location.
-  extension_service_->AddExtension(extension3_.get());
-  EXPECT_EQ("Chrome, App1, App2, App3, AppList", GetPinnedAppStatus());
-}
-
-// Check that the restauration of launcher items is happening in the same order
-// as the user has pinned / moved them (on another system) when they are synced
-// random order - including the chrome icon.
-TEST_F(LegacyShelfLayoutChromeLauncherControllerTest,
-    RestoreDefaultAppsRandomOrderChromeMoved) {
-  InitLauncherController();
-
-  base::ListValue policy_value;
-  InsertPrefValue(&policy_value, 0, extension1_->id());
-  InsertPrefValue(&policy_value, 1, extension2_->id());
-  InsertPrefValue(&policy_value, 2, extension3_->id());
-  profile()->GetTestingPrefService()->SetUserPref(prefs::kPinnedLauncherApps,
-                                                  policy_value.DeepCopy());
-  profile()->GetTestingPrefService()->SetInteger(prefs::kShelfChromeIconIndex,
-                                                 1);
-  // Model should only contain the browser shortcut and app list items.
-  EXPECT_FALSE(launcher_controller_->IsAppPinned(extension1_->id()));
-  EXPECT_FALSE(launcher_controller_->IsAppPinned(extension2_->id()));
-  EXPECT_FALSE(launcher_controller_->IsAppPinned(extension3_->id()));
-  EXPECT_EQ("Chrome, AppList", GetPinnedAppStatus());
-
-  // Installing |extension2_| should add it to the shelf - behind the
-  // chrome icon.
-  ash::ShelfItem item;
-  extension_service_->AddExtension(extension2_.get());
-  EXPECT_FALSE(launcher_controller_->IsAppPinned(extension1_->id()));
-  EXPECT_FALSE(launcher_controller_->IsAppPinned(extension3_->id()));
-  EXPECT_EQ("Chrome, App2, AppList", GetPinnedAppStatus());
-
-  // Installing |extension1_| should add it to the launcher - behind the
-  // chrome icon, but in first location.
-  extension_service_->AddExtension(extension1_.get());
-  EXPECT_FALSE(launcher_controller_->IsAppPinned(extension3_->id()));
-  EXPECT_EQ("App1, Chrome, App2, AppList", GetPinnedAppStatus());
-
-  // Installing |extension3_| should add it to the launcher - behind the
-  // chrome icon, but in first location.
-  extension_service_->AddExtension(extension3_.get());
-  EXPECT_EQ("App1, Chrome, App2, App3, AppList", GetPinnedAppStatus());
-}
-
-// Check that syncing to a different state does the correct thing.
-TEST_F(LegacyShelfLayoutChromeLauncherControllerTest,
-    RestoreDefaultAppsResyncOrder) {
-  InitLauncherController();
-  base::ListValue policy_value;
-  InsertPrefValue(&policy_value, 0, extension1_->id());
-  InsertPrefValue(&policy_value, 1, extension2_->id());
-  InsertPrefValue(&policy_value, 2, extension3_->id());
-  profile()->GetTestingPrefService()->SetUserPref(prefs::kPinnedLauncherApps,
-                                                  policy_value.DeepCopy());
-  EXPECT_EQ(0, profile()->GetPrefs()->GetInteger(prefs::kShelfChromeIconIndex));
-  extension_service_->AddExtension(extension2_.get());
-  extension_service_->AddExtension(extension1_.get());
-  extension_service_->AddExtension(extension3_.get());
-  EXPECT_EQ("Chrome, App1, App2, App3, AppList", GetPinnedAppStatus());
-
-  // Change the order with increasing chrome position and decreasing position.
-  base::ListValue policy_value1;
-  InsertPrefValue(&policy_value1, 0, extension3_->id());
-  InsertPrefValue(&policy_value1, 1, extension1_->id());
-  InsertPrefValue(&policy_value1, 2, extension2_->id());
-  profile()->GetTestingPrefService()->SetInteger(prefs::kShelfChromeIconIndex,
-                                                 2);
-  profile()->GetTestingPrefService()->SetUserPref(prefs::kPinnedLauncherApps,
-                                                  policy_value1.DeepCopy());
-  EXPECT_EQ("App3, App1, Chrome, App2, AppList", GetPinnedAppStatus());
-  base::ListValue policy_value2;
-  InsertPrefValue(&policy_value2, 0, extension2_->id());
-  InsertPrefValue(&policy_value2, 1, extension3_->id());
-  InsertPrefValue(&policy_value2, 2, extension1_->id());
-  profile()->GetTestingPrefService()->SetInteger(prefs::kShelfChromeIconIndex,
-                                                 1);
-  profile()->GetTestingPrefService()->SetUserPref(prefs::kPinnedLauncherApps,
-                                                  policy_value2.DeepCopy());
-  EXPECT_EQ("App2, Chrome, App3, App1, AppList", GetPinnedAppStatus());
-}
 
 TEST_F(ChromeLauncherControllerTest, DefaultApps) {
   InitLauncherController();
@@ -1156,29 +963,6 @@ TEST_F(ChromeLauncherControllerTest, DefaultApps) {
   EXPECT_FALSE(launcher_controller_->IsAppPinned(extension2_->id()));
 }
 
-// Check that changing from the alternate shelf layout to the old shelflayout
-// and back does keep the app launcher at location #0.
-TEST_F(ChromeLauncherControllerTest,
-       SwitchingFromAlternateShelfLayoutToLegacyAndBack) {
-  InitLauncherController();
-
-  // We simulate this problem by intentionally placing the app list item in
-  // the middle of several apps which caused a crash (see crbug.com/329597).
-  const char kAppShelfIdPlaceholder[] = "AppShelfIDPlaceholder--------";
-
-  base::ListValue policy_value;
-  InsertPrefValue(&policy_value, 0, extension1_->id());
-  InsertPrefValue(&policy_value, 1, kAppShelfIdPlaceholder);
-  InsertPrefValue(&policy_value, 2, extension2_->id());
-  profile()->GetTestingPrefService()->SetUserPref(prefs::kPinnedLauncherApps,
-                                                  policy_value.DeepCopy());
-  EXPECT_EQ(0, profile()->GetPrefs()->GetInteger(prefs::kShelfChromeIconIndex));
-  // Model should only contain the browser shortcut and app list items.
-  extension_service_->AddExtension(extension1_.get());
-  extension_service_->AddExtension(extension2_.get());
-  EXPECT_EQ("AppList, Chrome, App1, App2", GetPinnedAppStatus());
-}
-
 // Check that the restauration of launcher items is happening in the same order
 // as the user has pinned them (on another system) when they are synced reverse
 // order.
diff --git a/chrome/browser/ui/ash/launcher/launcher_favicon_loader.cc b/chrome/browser/ui/ash/launcher/launcher_favicon_loader.cc
index 895441e..d1fc953 100644
--- a/chrome/browser/ui/ash/launcher/launcher_favicon_loader.cc
+++ b/chrome/browser/ui/ash/launcher/launcher_favicon_loader.cc
@@ -19,7 +19,7 @@ const int kMaxBitmapSize = 256;
 
 ////////////////////////////////////////////////////////////////////////////////
 // FaviconBitmapHandler fetchs all bitmaps with the 'icon' (or 'shortcut icon')
-// link tag, storing the one that best matches ash::kShelfPreferredSize.
+// link tag, storing the one that best matches ash::kShelfSize.
 // These icon bitmaps are not resized and are not cached beyond the lifetime
 // of the class. Bitmaps larger than kMaxBitmapSize are ignored.
 
@@ -147,7 +147,7 @@ void FaviconBitmapHandler::AddFavicon(const GURL& image_url,
   if (new_bitmap.height() > kMaxBitmapSize ||
       new_bitmap.width() > kMaxBitmapSize)
     return;
-  if (new_bitmap.height() < ash::kShelfPreferredSize)
+  if (new_bitmap.height() < ash::kShelfSize)
     return;
   if (!bitmap_.isNull()) {
     // We want the smallest icon that is large enough.
diff --git a/chrome/browser/ui/ash/launcher/launcher_favicon_loader_browsertest.cc b/chrome/browser/ui/ash/launcher/launcher_favicon_loader_browsertest.cc
index 546489f..e408851 100644
--- a/chrome/browser/ui/ash/launcher/launcher_favicon_loader_browsertest.cc
+++ b/chrome/browser/ui/ash/launcher/launcher_favicon_loader_browsertest.cc
@@ -158,8 +158,8 @@ IN_PROC_BROWSER_TEST_F(LauncherFaviconLoaderBrowsertest, ManyLauncherIcons) {
 
   EXPECT_FALSE(favicon_loader_->GetFavicon().empty());
   // When multiple favicons are present, the correctly sized icon should be
-  // chosen. The icons are sized assuming ash::kShelfPreferredSize < 128.
-  EXPECT_GT(128, ash::kShelfPreferredSize);
+  // chosen. The icons are sized assuming ash::kShelfSize < 128.
+  EXPECT_GT(128, ash::kShelfSize);
   EXPECT_EQ(48, favicon_loader_->GetFavicon().height());
 }