Fixed problems with pin/unpin preferences changing for not running applications & icon order chang…

chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/ash/launcher/chrome_launcher_controller.h"
 
 #include <vector>
 
 #include "ash/ash_switches.h"
 #include "ash/desktop_background/desktop_background_controller.h"
@@ skipping 315 matching lines @@
     Profile* profile,
     ash::LauncherModel* model) {
   // We do not check here for re-creation of the ChromeLauncherController since
   // it appears that it might be intentional that the ChromeLauncherController
   // can be re-created.
   instance_ = new ChromeLauncherController(profile, model);
   return instance_;
 }
 
 void ChromeLauncherController::Init() {
+  CreateBrowserShortcutLauncherItem();
   UpdateAppLaunchersFromPref();
-  CreateBrowserShortcutLauncherItem();
 
   // TODO(sky): update unit test so that this test isn't necessary.
   if (ash::Shell::HasInstance()) {
     SetShelfAutoHideBehaviorFromPrefs();
     SetShelfAlignmentFromPrefs();
     PrefServiceSyncable* prefs = PrefServiceSyncable::FromProfile(profile_);
     if (!prefs->FindPreference(prefs::kShelfAlignmentLocal)->HasUserSetting() ||
         !prefs->FindPreference(prefs::kShelfAutoHideBehaviorLocal)->
             HasUserSetting()) {
       // This causes OnIsSyncingChanged to be called when the value of
@@ skipping 81 matching lines @@
   }
 
   if (CanPin())
     PersistPinnedState();
 }
 
 void ChromeLauncherController::Unpin(ash::LauncherID id) {
   DCHECK(HasItemController(id));
 
   LauncherItemController* controller = id_to_item_controller_map_[id];
-  if (controller->type() == LauncherItemController::TYPE_APP) {
-    int index = model_->ItemIndexByID(id);
-    DCHECK_GE(index, 0);
-    ash::LauncherItem item = model_->items()[index];
-    item.type = ash::TYPE_PLATFORM_APP;
-    model_->Set(index, item);
+  if (controller->type() == LauncherItemController::TYPE_APP ||
+      controller->locked()) {
+    UnpinRunningAppInternal(model_->ItemIndexByID(id));
   } else {
-    // Prevent the removal of items upon unpin if it is locked by a running
-    // windowed V1 app.
-    if (!controller->locked()) {
-      LauncherItemClosed(id);
-    } else {
-      int index = model_->ItemIndexByID(id);
-      DCHECK_GE(index, 0);
-      ash::LauncherItem item = model_->items()[index];
-      item.type = ash::TYPE_WINDOWED_APP;
-      model_->Set(index, item);
-    }
+    LauncherItemClosed(id);
   }
   if (CanPin())
     PersistPinnedState();
 }
 
 bool ChromeLauncherController::IsPinned(ash::LauncherID id) {
   int index = model_->ItemIndexByID(id);
   if (index < 0)
     return false;
   ash::LauncherItemType type = model_->items()[index].type;
@@ skipping 266 matching lines @@
   if (ignore_persist_pinned_state_change_)
     return;
   // It is a coding error to call PersistPinnedState() if the pinned apps are
   // not user-editable. The code should check earlier and not perform any
   // modification actions that trigger persisting the state.
   if (!CanPin()) {
     NOTREACHED() << "Can't pin but pinned state being updated";
     return;
   }
 
+  // With the alternate shelf layout, the app list is locked in position #0.
+  // Since the app list does not have any impact on the rest of the order,
+  // we need to keep track of its appearance so that the chrome icon position
+  // is at the proper location (backwards compatible).
+  size_t app_list_index_offset = 0;
+
   // Mutating kPinnedLauncherApps is going to notify us and trigger us to
   // process the change. We don't want that to happen so remove ourselves as a
   // listener.
   pref_change_registrar_.Remove(prefs::kPinnedLauncherApps);
   {
     ListPrefUpdate updater(profile_->GetPrefs(), prefs::kPinnedLauncherApps);
     updater->Clear();
     for (size_t i = 0; i < model_->items().size(); ++i) {
       if (model_->items()[i].type == ash::TYPE_APP_SHORTCUT) {
         ash::LauncherID id = model_->items()[i].id;
         if (HasItemController(id) && IsPinned(id)) {
           base::DictionaryValue* app_value = ash::CreateAppDict(
               id_to_item_controller_map_[id]->app_id());
           if (app_value)
             updater->Append(app_value);
         }
       } else if (model_->items()[i].type == ash::TYPE_BROWSER_SHORTCUT) {
-        PersistChromeItemIndex(i);
+        PersistChromeItemIndex(i - app_list_index_offset);
+      } else if (model_->items()[i].type == ash::TYPE_APP_LIST &&
+                 ash::switches::UseAlternateShelfLayout()) {
+        // With the new alternate shelf layout the app list can be pinned to the
+        // position 0 and should not be taken into account.
+        app_list_index_offset = 1;
       }
     }
   }
   pref_change_registrar_.Add(
       prefs::kPinnedLauncherApps,
       base::Bind(&ChromeLauncherController::UpdateAppLaunchersFromPref,
                  base::Unretained(this)));
 }
 
 ash::LauncherModel* ChromeLauncherController::model() {
@@ skipping 531 matching lines @@
       PersistPinnedState();
   }
 }
 
 void ChromeLauncherController::DoUnpinAppWithID(const std::string& app_id) {
   ash::LauncherID launcher_id = GetLauncherIDForAppID(app_id);
   if (launcher_id && IsPinned(launcher_id))
     Unpin(launcher_id);
 }
 
+int ChromeLauncherController::PinRunningAppInternal(
+    int index,
+    ash::LauncherID launcher_id) {
+  int running_index = model_->ItemIndexByID(launcher_id);
+  ash::LauncherItem item = model_->items()[running_index];
+  DCHECK(item.type == ash::TYPE_WINDOWED_APP ||
+         item.type == ash::TYPE_PLATFORM_APP);
+  item.type = ash::TYPE_APP_SHORTCUT;
+  model_->Set(running_index, item);
+  // The |LauncherModel|'s weight system might reposition the item to a
+  // new index, so we get the index again.
+  running_index = model_->ItemIndexByID(launcher_id);
+  if (running_index < index)
+    --index;
+  if (running_index != index)
+    model_->Move(running_index, index);
+  return index;
+}
+
+void ChromeLauncherController::UnpinRunningAppInternal(int index) {
+  DCHECK_GE(index, 0);
+  ash::LauncherItem item = model_->items()[index];
+  DCHECK_EQ(item.type, ash::TYPE_APP_SHORTCUT);
+  item.type = ash::TYPE_WINDOWED_APP;
+  // A platform app and a windowed app are sharing TYPE_APP_SHORTCUT. As such
+  // we have to check here what this was before it got a shortcut.
+  if (HasItemController(item.id) &&
+      id_to_item_controller_map_[item.id]->type() ==
+          LauncherItemController::TYPE_APP)
+    item.type = ash::TYPE_PLATFORM_APP;
+  model_->Set(index, item);
+}
+
 void ChromeLauncherController::UpdateAppLaunchersFromPref() {
-  // Construct a vector representation of to-be-pinned apps from the pref.
-  std::vector<std::string> pinned_apps;
-  int chrome_icon_index = GetChromeIconIndexFromPref();
+  // There are various functions which will trigger a |PersistPinnedState| call
+  // like a direct call to |DoPinAppWithID|, or an indirect call to the menu
+  // model which will use weights to re-arrange the icons to new positions.
+  // Since this function is meant to synchronize the "is state" with the
+  // "sync state", it makes no sense to store any changes by this function back
+  // into the pref state. Therefore we tell |persistPinnedState| to ignore any
+  // invocations while we are running.
+  base::AutoReset<bool> auto_reset(&ignore_persist_pinned_state_change_, true);
+  std::vector<std::string> pinned_apps = GetListOfPinnedAppsAndBrowser();
+
   int index = 0;
   int max_index = model_->item_count();
-  // Using the alternate shelf layout the App Icon should be the first item in
-  // the list thus start adding items at slot 1 (instead of slot 0).
-  if (ash::switches::UseAlternateShelfLayout()) {
+
+  // Using the alternate shelf layout the app list Icon should be the first item
+  // in the list thus start adding items at slot 1 (instead of slot 0).
+  if (ash::switches::UseAlternateShelfLayout() &&
+      model_->item_count() && model_->items()[0].type == ash::TYPE_APP_LIST) {
     ++index;
     ++max_index;
-    // The alternate shelf layout's icon position will always include the
-    // AppLauncher which needs to be subtracted here.
-    if (chrome_icon_index > 0)
-      --chrome_icon_index;
-  }
-  const base::ListValue* pinned_apps_pref =
-      profile_->GetPrefs()->GetList(prefs::kPinnedLauncherApps);
-  for (base::ListValue::const_iterator it(pinned_apps_pref->begin());
-       it != pinned_apps_pref->end(); ++it) {
-    // To preserve the Chrome icon position, we insert a dummy slot for it - if
-    // the model has a Chrome item. While initializing we can come here with no
-    // item in which case the count would be 1 or below.
-    if (it - pinned_apps_pref->begin() == chrome_icon_index &&
-        model_->item_count() > 1) {
-      pinned_apps.push_back(extension_misc::kChromeAppId);
-    }
-
-    DictionaryValue* app = NULL;
-    std::string app_id;
-    if ((*it)->GetAsDictionary(&app) &&
-        app->GetString(ash::kPinnedAppsPrefAppIDPath, &app_id) &&
-        std::find(pinned_apps.begin(), pinned_apps.end(), app_id) ==
-            pinned_apps.end() &&
-        app_tab_helper_->IsValidID(app_id)) {
-      pinned_apps.push_back(app_id);
-    }
   }
 
   // Walk the model and |pinned_apps| from the pref lockstep, adding and
   // removing items as necessary. NB: This code uses plain old indexing instead
   // of iterators because of model mutations as part of the loop.
   std::vector<std::string>::const_iterator pref_app_id(pinned_apps.begin());
   for (; index < max_index && pref_app_id != pinned_apps.end(); ++index) {
     // If the next app launcher according to the pref is present in the model,
     // delete all app launcher entries in between.
     if (*pref_app_id == extension_misc::kChromeAppId ||
@@ skipping 10 matching lines @@
              item.type == ash::TYPE_BROWSER_SHORTCUT) ||
             (entry != id_to_item_controller_map_.end() &&
              entry->second->app_id() == *pref_app_id)) {
           ++pref_app_id;
           break;
         } else {
           if (item.type == ash::TYPE_BROWSER_SHORTCUT) {
             // We cannot delete the browser shortcut. As such we move it up by
             // one. To avoid any side effects from our pinned state observer, we
             // do not call the model directly.
-            MoveItemWithoutPinnedStateChangeNotification(index, index + 1);
+            model_->Move(index, index + 1);
           } else {
-            LauncherItemClosed(item.id);
-            --max_index;
+            // Check if this is a platform or a windowed app.
+            if (item.type == ash::TYPE_APP_SHORTCUT &&
+                (id_to_item_controller_map_[item.id]->locked() ||
+                 id_to_item_controller_map_[item.id]->type() ==
+                     LauncherItemController::TYPE_APP)) {
+              // Note: This will not change the amount of items (|max_index|).
+              // Even changes to the actual |index| due to item weighting
+              // changes should be fine.
+              UnpinRunningAppInternal(index);
+            } else {
+              LauncherItemClosed(item.id);
+              --max_index;
+            }
           }
           --index;
         }
       }
       // If the item wasn't found, that means id_to_item_controller_map_
       // is out of sync.
       DCHECK(index <= max_index);
     } else {
-      // This app wasn't pinned before, insert a new entry.
-      ash::LauncherID id = CreateAppShortcutLauncherItem(*pref_app_id, index);
-      index = model_->ItemIndexByID(id);
+      // Check if the item was already running but not yet pinned.
+      ash::LauncherID launcher_id = GetLauncherIDForAppID(*pref_app_id);
+      if (launcher_id) {
+        // This app is running but not yet pinned. So pin and move it.
+        index = PinRunningAppInternal(index, launcher_id);
+      } else {
+        // This app wasn't pinned before, insert a new entry.
+        launcher_id = CreateAppShortcutLauncherItem(*pref_app_id, index);
+        ++max_index;
+        index = model_->ItemIndexByID(launcher_id);
+      }
       ++pref_app_id;
     }
   }
 
+  // Since we are removing the currently existing shortcuts, but the chrome item
+  // might be in the middle of that area, we need to keep track of it, so that
+  // it can be shifted to the correct location afterwards.
+  // Note: This might still produce location shifts with windowed V1
+  // applications since they will be grouped at the moment between shortcuts.
+  // To address that problem we could either group them separately again - or
+  // we will have to remember all shelf item locations.
+  int chrome_index = -1;
+
   // Remove any trailing existing items.
   while (index < model_->item_count()) {
     const ash::LauncherItem& item(model_->items()[index]);
-    if (item.type == ash::TYPE_APP_SHORTCUT)
-      LauncherItemClosed(item.id);
-    else
+    if (item.type == ash::TYPE_APP_SHORTCUT) {
+      if (id_to_item_controller_map_[item.id]->locked() ||
+          id_to_item_controller_map_[item.id]->type() ==
+              LauncherItemController::TYPE_APP)
+        UnpinRunningAppInternal(index);
+      else
+        LauncherItemClosed(item.id);
+    } else {
+      if (item.type == ash::TYPE_BROWSER_SHORTCUT)
+        chrome_index = index;
       ++index;
+    }
   }
 
   // Append unprocessed items from the pref to the end of the model.
   for (; pref_app_id != pinned_apps.end(); ++pref_app_id) {
-    // Ignore the chrome icon.
-    if (*pref_app_id != extension_misc::kChromeAppId)
+    // All items but the chrome shortcut needs to be added.
+    if (*pref_app_id != extension_misc::kChromeAppId) {
       DoPinAppWithID(*pref_app_id);
+    } else if (chrome_index != -1) {
+      // The chrome item was in the middle of the to be removed items and needs
+      // now to be moved to the last possible location.
+      model_->Move(chrome_index, model_->item_count() - 1);
+    }
   }
-
 }
 
 void ChromeLauncherController::SetShelfAutoHideBehaviorPrefs(
     ash::ShelfAutoHideBehavior behavior,
     aura::RootWindow* root_window) {
   const char* value = NULL;
   switch (behavior) {
     case ash::SHELF_AUTO_HIDE_BEHAVIOR_ALWAYS:
       value = ash::kShelfAutoHideBehaviorAlways;
       break;
@@ skipping 131 matching lines @@
   return static_cast<BrowserShortcutLauncherItemController*>(
       id_to_item_controller_map_[id]);
 }
 
 ash::LauncherID ChromeLauncherController::CreateBrowserShortcutLauncherItem() {
   ash::LauncherItem browser_shortcut;
   browser_shortcut.type = ash::TYPE_BROWSER_SHORTCUT;
   ResourceBundle& rb = ResourceBundle::GetSharedInstance();
   browser_shortcut.image = *rb.GetImageSkiaNamed(IDR_PRODUCT_LOGO_32);
   ash::LauncherID id = model_->next_id();
-  size_t index = GetChromeIconIndexFromPref();
+  size_t index = GetChromeIconIndexForCreation();
   model_->AddAt(index, browser_shortcut);
   browser_item_controller_.reset(
       new BrowserShortcutLauncherItemController(this, profile_));
   id_to_item_controller_map_[id] = browser_item_controller_.get();
   id_to_item_controller_map_[id]->set_launcher_id(id);
   return id;
 }
 
 void ChromeLauncherController::PersistChromeItemIndex(int index) {
   profile_->GetPrefs()->SetInteger(prefs::kShelfChromeIconIndex, index);
 }
 
 int ChromeLauncherController::GetChromeIconIndexFromPref() const {
   size_t index = profile_->GetPrefs()->GetInteger(prefs::kShelfChromeIconIndex);
   const base::ListValue* pinned_apps_pref =
       profile_->GetPrefs()->GetList(prefs::kPinnedLauncherApps);
-  if (ash::switches::UseAlternateShelfLayout())
-    return std::max(static_cast<size_t>(1),
-                    std::min(pinned_apps_pref->GetSize() + 1, index));
   return std::max(static_cast<size_t>(0),
                   std::min(pinned_apps_pref->GetSize(), index));
 }
 
+int ChromeLauncherController::GetChromeIconIndexForCreation() {
+  // We get the list of pinned apps as they currently would get pinned.
+  // Within this list the chrome icon will be the correct location.
+  std::vector<std::string> pinned_apps = GetListOfPinnedAppsAndBrowser();
+
+  std::vector<std::string>::iterator it =
+      std::find(pinned_apps.begin(),
+                pinned_apps.end(),
+                std::string(extension_misc::kChromeAppId));
+  DCHECK(it != pinned_apps.end());
+  int index = it - pinned_apps.begin();
+
+  // Since this function returns the index of the item within the shelf, the
+  // app list item must be taken into account and for now the app list is always
+  // the left most item in the list.
+  if (ash::switches::UseAlternateShelfLayout() &&
+      model_->item_count() && model_->items()[0].type == ash::TYPE_APP_LIST)
+    ++index;
+
+  // We should do here a comparison between the is state and the "want to be"
+  // state since some apps might be able to pin but are not yet. Instead - for
+  // the time being we clamp against the amount of known items and wait for the
+  // next |UpdateAppLaunchersFromPref()| call to correct it - it will come since
+  // the pinning will be done then.
+  return std::min(model_->item_count(), index);
+}
+
+std::vector<std::string>
+ChromeLauncherController::GetListOfPinnedAppsAndBrowser() {
+  std::vector<std::string> pinned_apps;
+  const base::ListValue* pinned_apps_pref =
+      profile_->GetPrefs()->GetList(prefs::kPinnedLauncherApps);
+
+  // Keep track of the addition of the chrome icon.
+  bool chrome_icon_added = false;
+  size_t chrome_icon_index = GetChromeIconIndexFromPref();
+  // Note: We do not need special handling for the app List here since it is
+  // either always at the start or the end of the list and it is not part of
+  // the pinned 'dynamic' items.
+  for (size_t index = 0; index < pinned_apps_pref->GetSize(); ++index) {
+    // We need to position the chrome icon relative to it's place in the pinned
+    // preference list - even if an item of that list isn't shown yet.
+    if (index == chrome_icon_index) {
+      pinned_apps.push_back(extension_misc::kChromeAppId);
+      chrome_icon_added = true;
+    }
+
+    const DictionaryValue* app = NULL;
+    std::string app_id;
+    if (pinned_apps_pref->GetDictionary(index, &app) &&
+        app->GetString(ash::kPinnedAppsPrefAppIDPath, &app_id) &&
+        (std::find(pinned_apps.begin(), pinned_apps.end(), app_id) ==
+             pinned_apps.end()) && app_tab_helper_->IsValidID(app_id))
+      pinned_apps.push_back(app_id);
+  }
+
+  // If not added yet, the chrome item will be the final item in the list.
+  if (!chrome_icon_added)
+    pinned_apps.push_back(extension_misc::kChromeAppId);
+  return pinned_apps;
+}
+
 bool ChromeLauncherController::IsIncognito(
-    content::WebContents* web_contents) const {
+    const content::WebContents* web_contents) const {
   const Profile* profile =
       Profile::FromBrowserContext(web_contents->GetBrowserContext());
   return profile->IsOffTheRecord() && !profile->IsGuestSession();
 }
 
 void ChromeLauncherController::CloseWindowedAppsFromRemovedExtension(
     const std::string& app_id) {
   // This function cannot rely on the controller's enumeration functionality
   // since the extension has already be unloaded.
   const BrowserList* ash_browser_list =
@@ skipping 11 matching lines @@
       browser_to_close.push_back(browser);
     }
   }
   while (!browser_to_close.empty()) {
     TabStripModel* tab_strip = browser_to_close.back()->tab_strip_model();
     tab_strip->CloseWebContentsAt(0, TabStripModel::CLOSE_NONE);
     browser_to_close.pop_back();
   }
 }
 
-void
-ChromeLauncherController::MoveItemWithoutPinnedStateChangeNotification(
-    int source_index, int target_index) {
-  base::AutoReset<bool> auto_reset(&ignore_persist_pinned_state_change_, true);
-  model_->Move(source_index, target_index);
-}
-
 void ChromeLauncherController::RegisterLauncherItemDelegate() {
   // TODO(simon.hong81): Register LauncherItemDelegate when LauncherItemDelegate
   // is created.
   ash::LauncherItemDelegateManager* manager =
       ash::Shell::GetInstance()->launcher_item_delegate_manager();
   manager->RegisterLauncherItemDelegate(ash::TYPE_APP_PANEL, this);
   manager->RegisterLauncherItemDelegate(ash::TYPE_APP_SHORTCUT, this);
   manager->RegisterLauncherItemDelegate(ash::TYPE_BROWSER_SHORTCUT, this);
   manager->RegisterLauncherItemDelegate(ash::TYPE_PLATFORM_APP, this);
   manager->RegisterLauncherItemDelegate(ash::TYPE_WINDOWED_APP, this);
@@ skipping 35 matching lines @@
 }
 
 void ChromeLauncherController::ReleaseProfile() {
   if (app_sync_ui_state_)
     app_sync_ui_state_->RemoveObserver(this);
 
   PrefServiceSyncable::FromProfile(profile_)->RemoveObserver(this);
 
   pref_change_registrar_.RemoveAll();
 }