Add AppLauncherId wrapper for items shown in shelf

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 98160a91d2b8e55c7545aee580c0d6805dad1c2f..c25adfee0ed6f8b4927537ff186c0b2b1b4ba515 100644
--- a/chrome/browser/ui/ash/chrome_launcher_prefs.cc
+++ b/chrome/browser/ui/ash/chrome_launcher_prefs.cc
@@ -42,7 +42,7 @@ const char* kDefaultPinnedApps[] = {
 base::ListValue* CreateDefaultPinnedAppsList() {
   std::unique_ptr<base::ListValue> apps(new base::ListValue);
   for (size_t i = 0; i < arraysize(kDefaultPinnedApps); ++i)
-    apps->Append(CreateAppDict(kDefaultPinnedApps[i]));
+    apps->Append(CreateAppDict(AppLauncherId(kDefaultPinnedApps[i])));
 
   return apps.release();
 }
@@ -206,10 +206,11 @@ void PropagatePrefToLocalIfNotSet(
 }
 
 struct PinInfo {
-  PinInfo(const std::string& app_id, const syncer::StringOrdinal& item_ordinal)
-      : app_id(app_id), item_ordinal(item_ordinal) {}
+  PinInfo(const AppLauncherId& app_launcher_id,
+          const syncer::StringOrdinal& item_ordinal)
+      : app_launcher_id(app_launcher_id), item_ordinal(item_ordinal) {}
 
-  std::string app_id;
+  AppLauncherId app_launcher_id;
   syncer::StringOrdinal item_ordinal;
 };
 
@@ -223,23 +224,23 @@ struct ComparePinInfo {
 // to check if app exists in the list.
 class AppTracker {
  public:
-  bool HasApp(const std::string& app_id) const {
-    return app_set_.find(app_id) != app_set_.end();
+  bool HasApp(const AppLauncherId& app_launcher_id) const {
+    return app_set_.find(app_launcher_id) != app_set_.end();
   }
 
-  void AddApp(const std::string& app_id) {
-    if (HasApp(app_id))
+  void AddApp(const AppLauncherId& app_launcher_id) {
+    if (HasApp(app_launcher_id))
       return;
-    app_list_.push_back(app_id);
-    app_set_.insert(app_id);
+    app_list_.push_back(app_launcher_id);
+    app_set_.insert(app_launcher_id);
   }
 
-  void MaybeAddApp(const std::string& app_id,
+  void MaybeAddApp(const AppLauncherId& app_launcher_id,
                    const LauncherControllerHelper* helper) {
-    DCHECK_NE(kPinnedAppsPlaceholder, app_id);
-    if (!helper->IsValidIDForCurrentUser(app_id))
+    DCHECK_NE(kPinnedAppsPlaceholder, app_launcher_id.GetAsString());
+    if (!helper->IsValidIDForCurrentUser(app_launcher_id.GetAsString()))
       return;
-    AddApp(app_id);
+    AddApp(app_launcher_id);
   }
 
   void MaybeAddAppFromPref(const base::DictionaryValue* app_pref,
@@ -260,14 +261,14 @@ class AppTracker {
       return;
     }
 
-    MaybeAddApp(app_id, helper);
+    MaybeAddApp(AppLauncherId(app_id), helper);
   }
 
-  const std::vector<std::string>& app_list() const { return app_list_; }
+  const std::vector<AppLauncherId>& app_list() const { return app_list_; }
 
  private:
-  std::vector<std::string> app_list_;
-  std::set<std::string> app_set_;
+  std::vector<AppLauncherId> app_list_;
+  std::set<AppLauncherId> app_set_;
 };
 
 }  // namespace
@@ -283,6 +284,39 @@ const char kShelfAlignmentBottom[] = "Bottom";
 const char kShelfAlignmentLeft[] = "Left";
 const char kShelfAlignmentRight[] = "Right";
 
+AppLauncherId::AppLauncherId(const std::string& app_id)
+    : app_launcher_id_(app_id) {}
+
+AppLauncherId::AppLauncherId(const std::string& app_id,
+                             const std::string& launch_id)
+    : app_launcher_id_(app_id + launch_id) {}

[msw, 2016/10/04 17:09:24]

Why concatenate these internally? Then there's no way to reconstitute the
|app_id| and |launcher_id| elements from an AppLauncherId object. It would be
more useful if this single id could replace various uses of the other two, but
losing the constituent id information means that we'll need to keep using the
other ids in more places.

[Andra Paraschiv, 2016/10/05 11:21:58]

I added app_id and launch_id as class members.

[James Cook, 2016/10/05 15:33:10]

I suspect msw meant to replace app_launcher_id_ with just app_id_ and launch_id_
and concatenate them in GetAsString, but I'll leave that up to him.

[msw, 2016/10/05 17:14:28]

Indeed, |app_launcher_id_| isn't needed anymore; just return a concatenated
value in GetAsString, and define the comparison operator as "return
GetAsString() < other.GetAsString();". It trades performance for memory, but
that seems like the right tradeoff here.

[Andra Paraschiv, 2016/10/06 07:49:47]

Done.

+
+AppLauncherId::~AppLauncherId() {}
+
+bool AppLauncherId::operator==(const AppLauncherId& other) const {

[msw, 2016/10/04 17:09:24]

I see that some places DCHECK_NE(kPinnedAppsPlaceholder,
app_launcher_id.GetAsString());, but where do we ever actually compare two
AppLauncherId instances?

[Andra Paraschiv, 2016/10/05 11:21:58]

I included in the last patch set only the operators that are currently
necessary.

+  return app_launcher_id_ == other.app_launcher_id_;
+}
+
+bool AppLauncherId::operator!=(const AppLauncherId& other) const {
+  return app_launcher_id_ != other.app_launcher_id_;
+}
+
+bool AppLauncherId::operator<(const AppLauncherId& other) const {

[msw, 2016/10/04 17:09:24]

Why are all these comparison functions needed? Could callers just compare the
GetAsString values?

[James Cook, 2016/10/04 17:18:44]

I think this one is needed to store AppLauncherId in a std::set<>. Most of the
others are probably not needed.

+  return app_launcher_id_ < other.app_launcher_id_;
+}
+
+bool AppLauncherId::operator<=(const AppLauncherId& other) const {
+  return app_launcher_id_ <= other.app_launcher_id_;
+}
+
+bool AppLauncherId::operator>(const AppLauncherId& other) const {
+  return app_launcher_id_ > other.app_launcher_id_;
+}
+
+bool AppLauncherId::operator>=(const AppLauncherId& other) const {
+  return app_launcher_id_ >= other.app_launcher_id_;
+}
+
 void RegisterChromeLauncherUserPrefs(
     user_prefs::PrefRegistrySyncable* registry) {
   // TODO: If we want to support multiple profiles this will likely need to be
@@ -309,9 +343,9 @@ void RegisterChromeLauncherUserPrefs(
   registry->RegisterBooleanPref(prefs::kShowLogoutButtonInTray, false);
 }
 
-base::DictionaryValue* CreateAppDict(const std::string& app_id) {
+base::DictionaryValue* CreateAppDict(const AppLauncherId& app_launcher_id) {
   std::unique_ptr<base::DictionaryValue> app_value(new base::DictionaryValue);
-  app_value->SetString(kPinnedAppsPrefAppIDPath, app_id);
+  app_value->SetString(kPinnedAppsPrefAppIDPath, app_launcher_id.GetAsString());
   return app_value.release();
 }
 
@@ -408,15 +442,15 @@ void GetAppsPinnedByPolicy(const PrefService* prefs,
       for (const auto& activity : activities) {
         const std::string arc_app_id =
             ArcAppListPrefs::GetAppId(arc_package, activity);
-        apps->MaybeAddApp(arc_app_id, helper);
+        apps->MaybeAddApp(AppLauncherId(arc_app_id), helper);
       }
     } else {
-      apps->MaybeAddApp(app_id, helper);
+      apps->MaybeAddApp(AppLauncherId(app_id), helper);
     }
   }
 }
 
-std::vector<std::string> GetPinnedAppsFromPrefsLegacy(
+std::vector<AppLauncherId> GetPinnedAppsFromPrefsLegacy(
     const PrefService* prefs,
     const LauncherControllerHelper* helper) {
   // Adding the app list item to the list of items requires that the ID is not
@@ -433,7 +467,7 @@ std::vector<std::string> GetPinnedAppsFromPrefsLegacy(
 
   // Check if Chrome is in either of the the preferences lists.
   std::unique_ptr<base::Value> chrome_app(
-      CreateAppDict(extension_misc::kChromeAppId));
+      CreateAppDict(AppLauncherId(extension_misc::kChromeAppId)));
 
   AppTracker apps;
   GetAppsPinnedByPolicy(prefs, helper, &apps);
@@ -443,7 +477,7 @@ std::vector<std::string> GetPinnedAppsFromPrefsLegacy(
     // 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)
-      apps.AddApp(extension_misc::kChromeAppId);
+      apps.AddApp(AppLauncherId(extension_misc::kChromeAppId));
     const base::DictionaryValue* app_pref = nullptr;
     if (!pinned_apps->GetDictionary(i, &app_pref)) {
       LOG(ERROR) << "There is no dictionary for app entry.";
@@ -453,7 +487,7 @@ std::vector<std::string> GetPinnedAppsFromPrefsLegacy(
   }
 
   // If not added yet, the chrome item will be the last item in the list.
-  apps.AddApp(extension_misc::kChromeAppId);
+  apps.AddApp(AppLauncherId(extension_misc::kChromeAppId));
   return apps.app_list();
 }
 
@@ -532,11 +566,11 @@ syncer::StringOrdinal GetLastPinPosition(Profile* profile) {
                             : syncer::StringOrdinal::CreateInitialOrdinal();
 }
 
-std::vector<std::string> ImportLegacyPinnedApps(
+std::vector<AppLauncherId> ImportLegacyPinnedApps(
     const PrefService* prefs,
     LauncherControllerHelper* helper,
     const AppTracker& policy_apps) {
-  std::vector<std::string> legacy_pins =
+  std::vector<AppLauncherId> legacy_pins =
       GetPinnedAppsFromPrefsLegacy(prefs, helper);
   DCHECK(!legacy_pins.empty());
 
@@ -546,9 +580,9 @@ std::vector<std::string> ImportLegacyPinnedApps(
   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);
+  for (const auto& app_launcher_id : legacy_pins) {
+    DCHECK_NE(kPinnedAppsPlaceholder, app_launcher_id.GetAsString());
+    app_service->SetPinPosition(app_launcher_id.GetAsString(), last_position);
     last_position = last_position.CreateAfter();
   }
 
@@ -568,14 +602,14 @@ std::vector<std::string> ImportLegacyPinnedApps(
   return legacy_pins;
 }
 
-std::vector<std::string> GetPinnedAppsFromPrefs(
+std::vector<AppLauncherId> 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>();
+    return std::vector<AppLauncherId>();
 
   std::vector<PinInfo> pin_infos;
 
@@ -597,8 +631,8 @@ std::vector<std::string> GetPinnedAppsFromPrefs(
       continue;
     }
 
-    pin_infos.push_back(
-        PinInfo(sync_peer.first, sync_peer.second->item_pin_ordinal));
+    pin_infos.push_back(PinInfo(AppLauncherId(sync_peer.first),
+                                sync_peer.second->item_pin_ordinal));
   }
 
   if (first_run) {
@@ -612,10 +646,10 @@ std::vector<std::string> GetPinnedAppsFromPrefs(
 
   // Pinned by policy apps appear first, if they were not shown before.
   syncer::StringOrdinal front_position = GetFirstPinPosition(helper->profile());
-  std::vector<std::string>::const_reverse_iterator it;
+  std::vector<AppLauncherId>::const_reverse_iterator it;
   for (it = policy_apps.app_list().rbegin();
        it != policy_apps.app_list().rend(); ++it) {
-    const std::string& app_id = *it;
+    const std::string& app_id = (*it).GetAsString();
     if (app_id == kPinnedAppsPlaceholder)
       continue;
 
@@ -624,15 +658,17 @@ std::vector<std::string> GetPinnedAppsFromPrefs(
       continue;
 
     // Now move it to the front.
-    pin_infos.insert(pin_infos.begin(), PinInfo(app_id, front_position));
+    pin_infos.insert(pin_infos.begin(),
+                     PinInfo(AppLauncherId(app_id), front_position));
     app_service->SetPinPosition(app_id, front_position);
     front_position = front_position.CreateBefore();
   }
 
   // Now insert Chrome browser app if needed.
   if (!app_service->GetPinPosition(extension_misc::kChromeAppId).IsValid()) {
-    pin_infos.insert(pin_infos.begin(),
-                     PinInfo(extension_misc::kChromeAppId, front_position));
+    pin_infos.insert(
+        pin_infos.begin(),
+        PinInfo(AppLauncherId(extension_misc::kChromeAppId), front_position));
     app_service->SetPinPosition(extension_misc::kChromeAppId, front_position);
   }
 
@@ -641,35 +677,39 @@ std::vector<std::string> GetPinnedAppsFromPrefs(
       const syncer::StringOrdinal arc_host_position =
           GetLastPinPosition(helper->profile());
       pin_infos.insert(pin_infos.begin(),
-                       PinInfo(ArcSupportHost::kHostAppId, arc_host_position));
+                       PinInfo(AppLauncherId(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());
+  // Convert to AppLauncherId array.
+  std::vector<AppLauncherId> pins;
   for (size_t i = 0; i < pin_infos.size(); ++i)
-    pins[i] = pin_infos[i].app_id;
+    pins.push_back(pin_infos[i].app_launcher_id);
 
   return pins;
 }
 
-void RemovePinPosition(Profile* profile, const std::string& app_id) {
+void RemovePinPosition(Profile* profile, const AppLauncherId& app_launcher_id) {
   DCHECK(profile);
-  DCHECK(!app_id.empty());
+  const std::string launcher_id = app_launcher_id.GetAsString();
+  DCHECK(!launcher_id.empty());
   app_list::AppListSyncableService* app_service =
       app_list::AppListSyncableServiceFactory::GetForProfile(profile);
-  app_service->SetPinPosition(app_id, syncer::StringOrdinal());
+  app_service->SetPinPosition(launcher_id, syncer::StringOrdinal());
 }
 
 void SetPinPosition(Profile* profile,
-                    const std::string& app_id,
-                    const std::string& app_id_before,
-                    const std::vector<std::string>& app_ids_after) {
+                    const AppLauncherId& app_launcher_id,
+                    const AppLauncherId& app_launcher_id_before,
+                    const std::vector<AppLauncherId>& app_launcher_ids_after) {
   DCHECK(profile);
-  DCHECK(!app_id.empty());
-  DCHECK_NE(app_id, app_id_before);
+  const std::string launcher_id = app_launcher_id.GetAsString();
+  DCHECK(!launcher_id.empty());
+  const std::string launcher_id_before = app_launcher_id_before.GetAsString();
+  DCHECK_NE(launcher_id, launcher_id_before);
 
   app_list::AppListSyncableService* app_service =
       app_list::AppListSyncableServiceFactory::GetForProfile(profile);
@@ -678,16 +718,19 @@ void SetPinPosition(Profile* profile,
     return;
 
   syncer::StringOrdinal position_before =
-      app_id_before.empty() ? syncer::StringOrdinal()
-                            : app_service->GetPinPosition(app_id_before);
+      launcher_id_before.empty()
+          ? syncer::StringOrdinal()
+          : app_service->GetPinPosition(launcher_id_before);
   syncer::StringOrdinal position_after;
-  for (const auto& app_id_after : app_ids_after) {
-    DCHECK_NE(app_id_after, app_id);
-    DCHECK_NE(app_id_after, app_id_before);
-    syncer::StringOrdinal position = app_service->GetPinPosition(app_id_after);
+  for (const auto& app_launcher_id_after : app_launcher_ids_after) {
+    const std::string launcher_id_after = app_launcher_id_after.GetAsString();
+    DCHECK_NE(launcher_id_after, launcher_id);
+    DCHECK_NE(launcher_id_after, launcher_id_before);
+    syncer::StringOrdinal position =
+        app_service->GetPinPosition(launcher_id_after);
     DCHECK(position.IsValid());
     if (!position.IsValid()) {
-      LOG(ERROR) << "Sync pin position was not found for " << app_id_after;
+      LOG(ERROR) << "Sync pin position was not found for " << launcher_id_after;
       continue;
     }
     if (!position_before.IsValid() || !position.Equals(position_before)) {
@@ -705,7 +748,7 @@ void SetPinPosition(Profile* profile,
     pin_position = position_after.CreateBefore();
   else
     pin_position = syncer::StringOrdinal::CreateInitialOrdinal();
-  app_service->SetPinPosition(app_id, pin_position);
+  app_service->SetPinPosition(launcher_id, pin_position);
 }
 
 }  // namespace launcher