arc: Support pinned apps across Arc-enabled and Arc-disabled platforms.

chrome/browser/ui/ash/chrome_launcher_prefs.cc

Index: chrome/browser/ui/ash/chrome_launcher_prefs.cc
diff --git a/chrome/browser/ui/ash/chrome_launcher_prefs.cc b/chrome/browser/ui/ash/chrome_launcher_prefs.cc
index a75cd55fb012608e30100176ae468df7b14923d5..bcf96ec704eae101fb8f12f0dddfab85f61f3e60 100644
--- a/chrome/browser/ui/ash/chrome_launcher_prefs.cc
+++ b/chrome/browser/ui/ash/chrome_launcher_prefs.cc
@@ -6,7 +6,7 @@
 
 #include <stddef.h>
 
-#include <memory>
+#include <set>
 
 #include "base/macros.h"
 #include "base/strings/string_number_conversions.h"
@@ -15,6 +15,8 @@
 #include "chrome/browser/chromeos/arc/arc_auth_service.h"
 #include "chrome/browser/chromeos/arc/arc_support_host.h"
 #include "chrome/browser/prefs/pref_service_syncable_util.h"
+#include "chrome/browser/ui/app_list/app_list_syncable_service.h"
+#include "chrome/browser/ui/app_list/app_list_syncable_service_factory.h"
 #include "chrome/browser/ui/app_list/arc/arc_app_list_prefs.h"
 #include "chrome/browser/ui/ash/launcher/launcher_controller_helper.h"
 #include "chrome/common/extensions/extension_constants.h"
@@ -23,10 +25,12 @@
 #include "components/prefs/pref_service.h"
 #include "components/prefs/scoped_user_pref_update.h"
 #include "components/syncable_prefs/pref_service_syncable.h"
+#include "sync/api/string_ordinal.h"
 #include "ui/display/display.h"
 #include "ui/display/screen.h"
 
 namespace ash {
+namespace launcher {
 
 namespace {
 
@@ -203,6 +207,47 @@ void PropagatePrefToLocalIfNotSet(
     pref_service->SetString(local_path, pref_service->GetString(synced_path));
 }
 
+struct PinInfo {
+  PinInfo(const std::string& app_id, const syncer::StringOrdinal& item_ordinal)
+      : app_id(app_id), item_ordinal(item_ordinal) {}
+
+  std::string app_id;
+  syncer::StringOrdinal item_ordinal;
+};
+
+struct ComparePinInfo {
+  bool operator()(const PinInfo& pin1, const PinInfo& pin2) {
+    return pin1.item_ordinal.LessThan(pin2.item_ordinal);
+  }
+};
+
+// Helper class to keep apps in order of appearance and to provide fast way
+// to check if app exists in the list.
+class AppList {

[xiyuan, 2016/06/14 21:42:50]

nit: Can we get a better name? AppList collides with, errr AppList. How about
call it AppTracker?

[khmel, 2016/06/15 17:01:20]

Agree, quite confusing :)

+ public:
+  AppList() {}
+  ~AppList() {}

[stevenjb, 2016/06/14 22:44:09]

nit: In a file local helper class, we can omit the trivial
constructor/destructor and DISALLOW_...

[khmel, 2016/06/15 17:01:20]

Done.

+
+  bool HasApp(const std::string& app_id) const {
+    return app_set_.find(app_id) != app_set_.end();
+  }
+
+  void AddApp(const std::string& app_id) {
+    if (HasApp(app_id))
+      return;
+    app_list_.push_back(app_id);
+    app_set_.insert(app_id);
+  }
+
+  const std::vector<std::string>& app_list() const { return app_list_; }
+
+ private:
+  std::vector<std::string> app_list_;
+  std::set<std::string> app_set_;
+
+  DISALLOW_COPY_AND_ASSIGN(AppList);

[xiyuan, 2016/06/15 17:26:25]

Why removing this?

[khmel, 2016/06/15 17:29:14]

As Steven recommended in comment above. I would personally leave DISALLOW, this
class is not suppose to be copied.

[xiyuan, 2016/06/15 17:30:22]

I see. Never mind then. I am fine either way.

[stevenjb, 2016/06/15 17:53:52]

I forget whether we can use DISALLOW without an explicit constructor/destructor.
Maybe we can if it is not virtual? If so, that's fine.

[khmel, 2016/06/15 18:26:22]

I tested, we still need CTOR for DISALLOW, so skipping DISALLOW  makes sense.

+};
+
 }  // namespace
 
 const char kPinnedAppsPrefAppIDPath[] = "id";
@@ -304,29 +349,16 @@ void SetShelfAlignmentPref(PrefService* prefs,
   }
 }
 
-std::vector<std::string> GetPinnedAppsFromPrefs(
-    const PrefService* prefs,
-    const LauncherControllerHelper* helper) {
-  // Adding the app list item to the list of items requires that the ID is not
-  // a valid and known ID for the extension system. The ID was constructed that
-  // way - but just to make sure...
-  DCHECK(!helper->IsValidIDForCurrentUser(kPinnedAppsPlaceholder));
+// Helper that extracts app list from policy preferences.
+void GetAppsPinnedByPolicy(const PrefService* prefs,
+                           const LauncherControllerHelper* helper,
+                           AppList* apps) {
+  DCHECK(apps);
+  DCHECK(apps->app_list().empty());
 
-  std::vector<std::string> apps;
-  const auto* pinned = prefs->GetList(prefs::kPinnedLauncherApps);
   const auto* policy = prefs->GetList(prefs::kPolicyPinnedLauncherApps);
-
-  // Get the sanitized preference value for the index of the Chrome app icon.
-  const size_t chrome_icon_index = std::max<size_t>(
-      0, std::min<size_t>(pinned->GetSize(),
-                          prefs->GetInteger(prefs::kShelfChromeIconIndex)));
-
-  // Check if Chrome is in either of the the preferences lists.
-  std::unique_ptr<base::Value> chrome_app(
-      ash::CreateAppDict(extension_misc::kChromeAppId));
-  bool chrome_listed =
-      (pinned->Find(*chrome_app.get()) != pinned->end() ||
-       (policy && policy->Find(*chrome_app.get()) != policy->end()));
+  if (!policy)
+    return;
 
   // Obtain here all ids of ARC apps because it takes linear time, and getting
   // them in the loop bellow would lead to quadratic complexity.
@@ -336,11 +368,10 @@ std::vector<std::string> GetPinnedAppsFromPrefs(
                         : std::vector<std::string>());
 
   std::string app_id;
-  for (size_t i = 0; policy && (i < policy->GetSize()); ++i) {
+  for (size_t i = 0; i < policy->GetSize(); ++i) {
     const base::DictionaryValue* dictionary = nullptr;
     if (policy->GetDictionary(i, &dictionary) &&
-        dictionary->GetString(kPinnedAppsPrefAppIDPath, &app_id) &&
-        std::find(apps.begin(), apps.end(), app_id) == apps.end()) {
+        dictionary->GetString(kPinnedAppsPrefAppIDPath, &app_id)) {
       if (IsAppIdArcPackage(app_id)) {
         if (!arc_app_list_pref)
           continue;
@@ -353,48 +384,59 @@ std::vector<std::string> GetPinnedAppsFromPrefs(
           const std::string arc_app_id =
               ArcAppListPrefs::GetAppId(arc_package, activity);
           if (helper->IsValidIDForCurrentUser(arc_app_id))
-            apps.push_back(arc_app_id);
+            apps->AddApp(arc_app_id);
         }
       } else if (helper->IsValidIDForCurrentUser(app_id)) {
-        apps.push_back(app_id);
+        apps->AddApp(app_id);
       }
     }
   }
+}

[stevenjb, 2016/06/14 22:44:08]

nit: This could be a member of AppList.

[khmel, 2016/06/15 17:01:19]

Done.

+
+std::vector<std::string> GetPinnedAppsFromPrefsLegacy(
+    const PrefService* prefs,
+    const LauncherControllerHelper* helper) {
+  // Adding the app list item to the list of items requires that the ID is not
+  // a valid and known ID for the extension system. The ID was constructed that
+  // way - but just to make sure...
+  DCHECK(!helper->IsValidIDForCurrentUser(kPinnedAppsPlaceholder));
 
+  const auto* pinned = prefs->GetList(prefs::kPinnedLauncherApps);

[stevenjb, 2016/06/14 22:44:09]

nit: pinned_apps

[khmel, 2016/06/15 17:01:20]

Done.

+
+  // Get the sanitized preference value for the index of the Chrome app icon.
+  const size_t chrome_icon_index = std::max<size_t>(
+      0, std::min<size_t>(pinned->GetSize(),
+                          prefs->GetInteger(prefs::kShelfChromeIconIndex)));
+
+  // Check if Chrome is in either of the the preferences lists.
+  std::unique_ptr<base::Value> chrome_app(
+      CreateAppDict(extension_misc::kChromeAppId));
+
+  AppList apps;
+  GetAppsPinnedByPolicy(prefs, helper, &apps);
+
+  std::string app_id;
   for (size_t i = 0; i < pinned->GetSize(); ++i) {
     // We need to position the chrome icon relative to its place in the pinned
     // preference list - even if an item of that list isn't shown yet.
-    if (i == chrome_icon_index && !chrome_listed) {
-      apps.push_back(extension_misc::kChromeAppId);
-      chrome_listed = true;
-    }
+    if (i == chrome_icon_index)
+      apps.AddApp(extension_misc::kChromeAppId);
     bool pinned_by_policy = false;
     const base::DictionaryValue* dictionary = nullptr;
     if (pinned->GetDictionary(i, &dictionary) &&
         dictionary->GetString(kPinnedAppsPrefAppIDPath, &app_id) &&
+        app_id != kPinnedAppsPlaceholder &&
         helper->IsValidIDForCurrentUser(app_id) &&
-        std::find(apps.begin(), apps.end(), app_id) == apps.end() &&
         (!dictionary->GetBoolean(kPinnedAppsPrefPinnedByPolicy,
                                  &pinned_by_policy) ||
          !pinned_by_policy)) {
-      apps.push_back(app_id);
+      apps.AddApp(app_id);
     }

[stevenjb, 2016/06/14 22:44:09]

nit: This would be a bit clearer as:

const base::DictionaryValue* app_pref = nullptr;
if (!pinned->GetDictionary(i, &app_pref)) {
  LOG(ERROR) ...
  continue;
}
apps.MaybeAddAppFromPref(app_pref, helper)

[khmel, 2016/06/15 17:01:20]

Done.

   }
 
-  if (arc::ArcAuthService::IsAllowedForProfile(helper->profile()) &&
-      helper->IsValidIDForCurrentUser(ArcSupportHost::kHostAppId)) {
-    apps.push_back(ArcSupportHost::kHostAppId);
-  }
-
   // If not added yet, the chrome item will be the last item in the list.
-  if (!chrome_listed)
-    apps.push_back(extension_misc::kChromeAppId);
-
-  // If not added yet, place the app list item at the beginning of the list.
-  if (std::find(apps.begin(), apps.end(), kPinnedAppsPlaceholder) == apps.end())
-    apps.insert(apps.begin(), kPinnedAppsPlaceholder);
-
-  return apps;
+  apps.AddApp(extension_misc::kChromeAppId);
+  return apps.app_list();
 }
 
 // static
@@ -434,4 +476,242 @@ void ChromeLauncherPrefsObserver::OnIsSyncingChanged() {
   }
 }
 
+// Helper to create pin position that stays before any synced app, even if
+// app is not currently visbile on a device.
+syncer::StringOrdinal CreateFirstPinPosition(Profile* profile) {

[stevenjb, 2016/06/14 22:44:09]

nit: GetFirstPinPosition
s/create/get/ in the comment
(create sounds like it constructs something more complicated than just an
ordinal)

[khmel, 2016/06/15 17:01:20]

Done.

+  syncer::StringOrdinal position;
+  app_list::AppListSyncableService* app_service =
+      app_list::AppListSyncableServiceFactory::GetForProfile(profile);
+  for (const auto& sync_peer : app_service->sync_items()) {
+    if (!sync_peer.second->item_pin_ordinal.IsValid())
+      continue;
+    if (!position.IsValid() ||
+        sync_peer.second->item_pin_ordinal.LessThan(position)) {
+      position = sync_peer.second->item_pin_ordinal;
+    }
+  }
+
+  return position.IsValid() ? position.CreateBefore()
+                            : syncer::StringOrdinal::CreateInitialOrdinal();
+}
+
+// Helper to creates pin position that stays before any synced app, even if
+// app is not currently visbile on a device.
+syncer::StringOrdinal CreateLastPinPosition(Profile* profile) {

[stevenjb, 2016/06/14 22:44:09]

Ditto

[khmel, 2016/06/15 17:01:20]

Done.

+  syncer::StringOrdinal position;
+  app_list::AppListSyncableService* app_service =
+      app_list::AppListSyncableServiceFactory::GetForProfile(profile);
+  for (const auto& sync_peer : app_service->sync_items()) {
+    if (!sync_peer.second->item_pin_ordinal.IsValid())
+      continue;
+    if (!position.IsValid() ||
+        sync_peer.second->item_pin_ordinal.GreaterThan(position)) {
+      position = sync_peer.second->item_pin_ordinal;
+    }
+  }
+
+  return position.IsValid() ? position.CreateAfter()
+                            : syncer::StringOrdinal::CreateInitialOrdinal();
+}
+
+std::vector<std::string> ImportLegacyPinnedApps(
+    const PrefService* prefs,
+    LauncherControllerHelper* helper,
+    const AppList& policy_apps) {
+  std::vector<std::string> legacy_pins =
+      GetPinnedAppsFromPrefsLegacy(prefs, helper);
+  DCHECK(!legacy_pins.empty());
+
+  app_list::AppListSyncableService* app_service =
+      app_list::AppListSyncableServiceFactory::GetForProfile(helper->profile());
+
+  syncer::StringOrdinal last_position =
+      syncer::StringOrdinal::CreateInitialOrdinal();
+  // Convert to sync item record.
+  for (const auto& app_id : legacy_pins) {
+    DCHECK_NE(kPinnedAppsPlaceholder, app_id);
+    app_service->SetPinPosition(app_id, last_position);
+    last_position = last_position.CreateAfter();
+  }
+
+  // Now process default apps.
+  for (size_t i = 0; i < arraysize(kDefaultPinnedApps); ++i) {
+    const std::string& app_id = kDefaultPinnedApps[i];
+    // Check if it is already imported.
+    if (app_service->GetPinPosition(app_id).IsValid())
+      continue;
+    // Check if it is present but not in legacy pin.
+    if (helper->IsValidIDForCurrentUser(app_id))
+      continue;
+    app_service->SetPinPosition(app_id, last_position);
+    last_position = last_position.CreateAfter();
+  }
+
+  return legacy_pins;
+}
+
+std::vector<std::string> GetPinnedAppsFromPrefs(
+    const PrefService* prefs,
+    LauncherControllerHelper* helper) {
+  app_list::AppListSyncableService* app_service =
+      app_list::AppListSyncableServiceFactory::GetForProfile(helper->profile());
+  // Some unit tests may not have it.
+  if (!app_service)
+    return std::vector<std::string>();
+
+  std::vector<PinInfo> pin_infos;
+
+  AppList policy_apps;
+  GetAppsPinnedByPolicy(prefs, helper, &policy_apps);
+
+  // Empty pins indicates that sync based pin model is used for the first
+  // time. In normal workflow we have at least Chrome browser pin info.
+  bool first_run = true;
+
+  for (const auto& sync_peer : app_service->sync_items()) {
+    if (!sync_peer.second->item_pin_ordinal.IsValid())
+      continue;
+
+    first_run = false;
+    // Don't include apps that currently do not exist on device.
+    if (sync_peer.first != extension_misc::kChromeAppId &&
+        !helper->IsValidIDForCurrentUser(sync_peer.first)) {
+      continue;
+    }
+
+    pin_infos.push_back(
+        PinInfo(sync_peer.first, sync_peer.second->item_pin_ordinal));
+  }
+
+  if (first_run) {
+    // We need to import legacy pins model and convert it to sync based
+    // model.
+    return ImportLegacyPinnedApps(prefs, helper, policy_apps);
+  }
+
+  // Sort pins according their ordinals.
+  std::sort(pin_infos.begin(), pin_infos.end(), ComparePinInfo());
+
+  // Now insert Chrome browser app if needed.
+  syncer::StringOrdinal chrome_position =
+      app_service->GetPinPosition(extension_misc::kChromeAppId);
+  if (!chrome_position.IsValid()) {
+    chrome_position = CreateFirstPinPosition(helper->profile());
+    pin_infos.insert(pin_infos.begin(),
+                     PinInfo(extension_misc::kChromeAppId, chrome_position));
+    app_service->SetPinPosition(extension_misc::kChromeAppId, chrome_position);
+  }
+
+  // Policy apps must preserve order of appearance and be first app in the list.
+  // Validate that there have a correct position and fix if needed.

[stevenjb, 2016/06/14 22:44:08]

s/there/they/

[khmel, 2016/06/15 17:01:20]

Done.

+  size_t shelf_index = 0;
+  for (const auto& app_id : policy_apps.app_list()) {

[xiyuan, 2016/06/14 21:42:50]

Would this be enforced every time? If a user moves a policy pinned app, would
the new position be kept after this code runs again (e.g. on the next login)?

[stevenjb, 2016/06/14 22:44:09]

If we put the policy pinned apps in front every time without preventing them
from being re-ordered in the UI, that would be confusing I think.

In the issue description, it was suggested: "+1 to keeping things super simple".

My suggestion would be to simplify the logic below as follows:

* If the app is already in pin_infos (because its pinned position was synced),
continue (i.e. leave it).
* Otherwise put it before the first item.
* Move the chrome browser app insertion below so that it will be added in front
of policy apps if it doesn't exist.

Does that work / make sense?

[khmel, 2016/06/15 17:01:19]

This is was done for unmovable pinned by policy apps, as legacy. I changed this
version to movable pinned by policy apps. I think we want to change legacy
design. Based on this assumption your comments make sense.

+    if (app_id == kPinnedAppsPlaceholder)
+      continue;
+    if (app_id == extension_misc::kChromeAppId)
+      continue;
+
+    // Chrome browser app has right to appear between pinned by policy apps.
+    if (shelf_index < pin_infos.size() &&
+        pin_infos[shelf_index].app_id == extension_misc::kChromeAppId) {
+      ++shelf_index;
+    }
+
+    const bool need_position_item = shelf_index >= pin_infos.size() ||
+                                    app_id != pin_infos[shelf_index].app_id;
+    if (need_position_item) {
+      // Remove existing app that breaks the order from the list.
+      if (shelf_index < pin_infos.size()) {
+        pin_infos.erase(
+            std::remove_if(pin_infos.begin() + shelf_index + 1, pin_infos.end(),
+                           [app_id](const PinInfo& pin_info) {
+                             return (pin_info.app_id == app_id);
+                           }),
+            pin_infos.end());
+      }
+
+      syncer::StringOrdinal new_shelf_ordinal;
+      if (shelf_index == 0) {
+        // First app.
+        new_shelf_ordinal = pin_infos.empty()
+                                ? syncer::StringOrdinal::CreateInitialOrdinal()
+                                : pin_infos.front().item_ordinal.CreateBefore();
+      } else if (shelf_index == pin_infos.size()) {
+        new_shelf_ordinal = pin_infos.back().item_ordinal.CreateAfter();
+      } else {
+        new_shelf_ordinal =
+            pin_infos[shelf_index - 1].item_ordinal.CreateBetween(
+                pin_infos[shelf_index].item_ordinal);
+      }
+      pin_infos.insert(pin_infos.begin() + shelf_index,
+                       PinInfo(app_id, new_shelf_ordinal));
+      app_service->SetPinPosition(app_id, new_shelf_ordinal);
+    }
+    ++shelf_index;
+  }
+
+  if (arc::ArcAuthService::IsAllowedForProfile(helper->profile()) &&
+      helper->IsValidIDForCurrentUser(ArcSupportHost::kHostAppId)) {
+    if (!app_service->GetSyncItem(ArcSupportHost::kHostAppId)) {
+      const syncer::StringOrdinal arc_host_position =
+          CreateLastPinPosition(helper->profile());
+      pin_infos.insert(pin_infos.begin(),
+                       PinInfo(ArcSupportHost::kHostAppId, arc_host_position));
+      app_service->SetPinPosition(ArcSupportHost::kHostAppId,
+                                  arc_host_position);
+    }
+  }
+
+  // Convert to string array.
+  std::vector<std::string> pins(pin_infos.size());
+  for (size_t i = 0; i < pin_infos.size(); ++i)
+    pins[i] = pin_infos[i].app_id;
+
+  return pins;
+}
+
+void RemovePinPosition(Profile* profile, const std::string& app_id) {
+  DCHECK(profile);
+  DCHECK(!app_id.empty());
+  app_list::AppListSyncableService* app_service =
+      app_list::AppListSyncableServiceFactory::GetForProfile(profile);
+  app_service->SetPinPosition(app_id, syncer::StringOrdinal());
+}
+
+void SetPinPosition(Profile* profile,
+                    const std::string& app_id,
+                    const std::string& app_id_before,
+                    const std::string& app_id_after) {
+  DCHECK(profile);
+  DCHECK(!app_id.empty());
+  DCHECK_NE(app_id, app_id_before);
+  DCHECK_NE(app_id, app_id_after);
+  DCHECK(app_id_before.empty() || app_id_before != app_id_after);
+
+  app_list::AppListSyncableService* app_service =
+      app_list::AppListSyncableServiceFactory::GetForProfile(profile);
+  // Some unit tests may not have this service.
+  if (!app_service)
+    return;
+
+  syncer::StringOrdinal position_before =
+      app_id_before.empty() ? syncer::StringOrdinal()
+                            : app_service->GetPinPosition(app_id_before);
+  syncer::StringOrdinal position_after =
+      app_id_after.empty() ? syncer::StringOrdinal()
+                           : app_service->GetPinPosition(app_id_after);
+
+  syncer::StringOrdinal pin_position;
+  if (position_before.IsValid() && position_after.IsValid())
+    pin_position = position_before.CreateBetween(position_after);
+  else if (position_before.IsValid())
+    pin_position = position_before.CreateAfter();
+  else if (position_after.IsValid())
+    pin_position = position_after.CreateBefore();
+  else
+    pin_position = syncer::StringOrdinal::CreateInitialOrdinal();
+  app_service->SetPinPosition(app_id, pin_position);
+}
+
+}  // namespace launcher
 }  // namespace ash