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 424 matching lines @@
 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);
   } else {

[oshima, 2013/09/19 21:42:11]

can you expland if else like
if (... == TYPE_APP) {
} else if (controller->locked()) {
  UnpinRunningAppInternal(model_->ItemIndexByID(id));
} else {
  LauncherItemClosed(id);
}

[Mr4D (OOO till 08-26), 2013/09/19 22:31:50]

Ahhh! Good one! case combined!

     // Prevent the removal of items upon unpin if it is locked by a running
     // windowed V1 app.
-    if (!controller->locked()) {
+    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);
-    }
+    else
+      UnpinRunningAppInternal(model_->ItemIndexByID(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).
+  int app_list_found = 0;

[oshima, 2013/09/19 21:42:11]

maybe app_list_index_found, or found_app_list_index? also size_t for index.

[Mr4D (OOO till 08-26), 2013/09/19 22:31:50]

Done.

+
   // 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_found);
+      } 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_found = 1;

[oshima, 2013/09/19 21:42:11]

why this is 1 not i?

[Mr4D (OOO till 08-26), 2013/09/19 22:31:50]

Because all we do is to subtract 1 if the app list item is before the chrome
item. This has to change once Harry's CL lands. It's on my list, will prepare
that once it has landed.

       }
     }
   }
   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)
+    MoveItemWithoutPinnedStateChangeNotification(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.
+  // Note: This function has to make sure that it never changes a state which
+  // calls |PersistPinnedState()| - otherwise all kinds of problems can arise.

[oshima, 2013/09/19 21:42:11]

add reference to bug? (assuming one of bugs listed is for this)

[Mr4D (OOO till 08-26), 2013/09/19 22:31:50]

No it is not. This was meant to be a helper comment for the next poor guy
debugging the code. I am a bit worried what happens if someone changes a
callback handler in an edge case. An "observer" suddenly call Pin/Unpin/Move/
which will cause the sync function to fire. Finding that will take a while. 
As an example:
  There is a special MoveItemWithoutPinnedStateChangeNotification function which
takes care of this for move.
I thought already to add a special member variable in the sync code and a DCHECK
at the end of the function to see if the sync function was called. But if I do
that - it would possibly be better to add the suggested ignore flag (then we are
100% sure).

+  // If it happens (due to some complicated observer interactions) it would be
+  // best to either add here
+  // base::AutoReset<bool> auto_reset(&ignore_persist_pinned_state_change_,
+  //                                  true);
+  // or call |PersistPinnedState()| at the end. For now this is not necessary.
   std::vector<std::string> pinned_apps;
-  int chrome_icon_index = GetChromeIconIndexFromPref();
+  GetListOfPinnedAppsAndBrowser(&pinned_apps);
+
   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).
+
+  // 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()) {
     ++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 12 matching lines @@
              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);
           } 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);
+        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.
+      MoveItemWithoutPinnedStateChangeNotification(chrome_index,
+                                                  model_->item_count() - 1);

[oshima, 2013/09/19 21:42:11]

indent

[Mr4D (OOO till 08-26), 2013/09/19 22:31:50]

Done.

+    }
   }
-
 }
 
 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(&pinned_apps);
+
+  std::vector<std::string>::iterator it =
+      std::find(pinned_apps.begin(),
+                pinned_apps.end(),
+                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())
+    ++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);
+}
+
+void 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;
+  int 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 (base::ListValue::const_iterator it(pinned_apps_pref->begin());
+       it != pinned_apps_pref->end(); ++it) {

[oshima, 2013/09/19 21:42:11]

since you need index anyway, using size_t index would be better than using
iterator.

[Mr4D (OOO till 08-26), 2013/09/19 22:31:50]

Done.

+    // 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 (it - pinned_apps_pref->begin() == chrome_icon_index &&
+        model_->item_count() > 1) {
+      pinned_apps->push_back(extension_misc::kChromeAppId);
+      chrome_icon_added = true;
+    }
+
+    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);
+  }
+
+  // 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);
+}
+
 bool ChromeLauncherController::IsIncognito(
     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
@@ skipping 75 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();
 }