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 5d804d908a9b28bf586e494e8cef548af9d6a95a..e0cca290e8328873c8b5978a6af21f554863ee26 100644
--- a/chrome/browser/ui/ash/chrome_launcher_prefs.cc
+++ b/chrome/browser/ui/ash/chrome_launcher_prefs.cc
@@ -41,8 +41,10 @@ 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]));
+  for (size_t i = 0; i < arraysize(kDefaultPinnedApps); ++i) {
+    AppLauncherId* app_launcher_id = new AppLauncherId(kDefaultPinnedApps[i]);
+    apps->Append(CreateAppDict(app_launcher_id));

[stevenjb, 2016/09/21 16:06:40]

This is leaky!

Just do:
apps->Append(CreateAppDict(AppLauncherId(kDefaultPinnedApps[i])))

(And use const& in CreateAppDict).

[Andra Paraschiv, 2016/09/22 09:23:41]

Done.

+  }
 
   return apps.release();
 }
@@ -206,10 +208,12 @@ void PropagatePrefToLocalIfNotSet(
 }
 
 struct PinInfo {
-  PinInfo(const std::string& app_id, const syncer::StringOrdinal& item_ordinal)
-      : app_id(app_id), item_ordinal(item_ordinal) {}
+  PinInfo(AppLauncherId* app_launcher_id,
+          const syncer::StringOrdinal& item_ordinal)
+      : app_launcher_id(app_launcher_id->GetAppLauncherID()),
+        item_ordinal(item_ordinal) {}
 
-  std::string app_id;
+  std::string app_launcher_id;

[stevenjb, 2016/09/21 16:06:40]

This should be an AppLauncherId. Only use the string version when necessary,
e.g. when storing the pref.

[Andra Paraschiv, 2016/09/22 09:23:41]

If we change it to AppLauncherId, we should add an assignment operator overload
function for AppLauncherId class and make app_launcher_id string attribute (from
the AppLauncherId class) non-const. Should we include these changes too? 

[stevenjb, 2016/09/27 16:42:36]

I'm not exactly sure what you are asking, but we should be able to store and
copy AppLauncherId.

   syncer::StringOrdinal item_ordinal;
 };
 
@@ -223,23 +227,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(AppLauncherId* app_launcher_id) const {
+    return app_set_.find(app_launcher_id->GetAppLauncherID()) != app_set_.end();
   }
 
-  void AddApp(const std::string& app_id) {
-    if (HasApp(app_id))
+  void AddApp(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->GetAppLauncherID());
+    app_set_.insert(app_launcher_id->GetAppLauncherID());
   }
 
-  void MaybeAddApp(const std::string& app_id,
+  void MaybeAddApp(AppLauncherId* app_launcher_id,
                    const LauncherControllerHelper* helper) {
-    DCHECK_NE(kPinnedAppsPlaceholder, app_id);
-    if (!helper->IsValidIDForCurrentUser(app_id))
+    DCHECK_NE(kPinnedAppsPlaceholder, app_launcher_id->GetAppLauncherID());
+    if (!helper->IsValidIDForCurrentUser(app_launcher_id->GetAppLauncherID()))
       return;
-    AddApp(app_id);
+    AddApp(app_launcher_id);
   }
 
   void MaybeAddAppFromPref(const base::DictionaryValue* app_pref,
@@ -260,7 +264,8 @@ class AppTracker {
       return;
     }
 
-    MaybeAddApp(app_id, helper);
+    AppLauncherId* app_launcher_id = new AppLauncherId(app_id);
+    MaybeAddApp(app_launcher_id, helper);
   }
 
   const std::vector<std::string>& app_list() const { return app_list_; }
@@ -309,9 +314,10 @@ void RegisterChromeLauncherUserPrefs(
   registry->RegisterBooleanPref(prefs::kShowLogoutButtonInTray, false);
 }
 
-base::DictionaryValue* CreateAppDict(const std::string& app_id) {
+base::DictionaryValue* CreateAppDict(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->GetAppLauncherID());
   return app_value.release();
 }
 
@@ -408,10 +414,12 @@ 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);
+        AppLauncherId* app_launcher_id = new AppLauncherId(arc_app_id);
+        apps->MaybeAddApp(app_launcher_id, helper);
       }
     } else {
-      apps->MaybeAddApp(app_id, helper);
+      AppLauncherId* app_launcher_id = new AppLauncherId(app_id);
+      apps->MaybeAddApp(app_launcher_id, helper);
     }
   }
 }
@@ -432,8 +440,10 @@ std::vector<std::string> GetPinnedAppsFromPrefsLegacy(
                           prefs->GetInteger(prefs::kShelfChromeIconIndex)));
 
   // Check if Chrome is in either of the the preferences lists.
+  AppLauncherId* chrome_app_launcher_id =
+      new AppLauncherId(extension_misc::kChromeAppId);
   std::unique_ptr<base::Value> chrome_app(
-      CreateAppDict(extension_misc::kChromeAppId));
+      CreateAppDict(chrome_app_launcher_id));
 
   AppTracker apps;
   GetAppsPinnedByPolicy(prefs, helper, &apps);
@@ -442,8 +452,11 @@ std::vector<std::string> GetPinnedAppsFromPrefsLegacy(
   for (size_t i = 0; i < pinned_apps->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)
-      apps.AddApp(extension_misc::kChromeAppId);
+    if (i == chrome_icon_index) {
+      AppLauncherId* chrome_app_launcher_id =
+          new AppLauncherId(extension_misc::kChromeAppId);
+      apps.AddApp(chrome_app_launcher_id);
+    }
     const base::DictionaryValue* app_pref = nullptr;
     if (!pinned_apps->GetDictionary(i, &app_pref)) {
       LOG(ERROR) << "There is no dictionary for app entry.";
@@ -453,7 +466,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(chrome_app_launcher_id);
   return apps.app_list();
 }
 
@@ -597,8 +610,9 @@ std::vector<std::string> GetPinnedAppsFromPrefs(
       continue;
     }
 
+    AppLauncherId* app_launcher_id = new AppLauncherId(sync_peer.first);
     pin_infos.push_back(
-        PinInfo(sync_peer.first, sync_peer.second->item_pin_ordinal));
+        PinInfo(app_launcher_id, sync_peer.second->item_pin_ordinal));
   }
 
   if (first_run) {
@@ -624,15 +638,19 @@ std::vector<std::string> GetPinnedAppsFromPrefs(
       continue;
 
     // Now move it to the front.
-    pin_infos.insert(pin_infos.begin(), PinInfo(app_id, front_position));
+    AppLauncherId* app_launcher_id = new AppLauncherId(app_id);
+    pin_infos.insert(pin_infos.begin(),
+                     PinInfo(app_launcher_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()) {
+    AppLauncherId* chrome_app_launcher_id =
+        new AppLauncherId(extension_misc::kChromeAppId);
     pin_infos.insert(pin_infos.begin(),
-                     PinInfo(extension_misc::kChromeAppId, front_position));
+                     PinInfo(chrome_app_launcher_id, front_position));
     app_service->SetPinPosition(extension_misc::kChromeAppId, front_position);
   }
 
@@ -640,8 +658,10 @@ std::vector<std::string> GetPinnedAppsFromPrefs(
     if (!app_service->GetSyncItem(ArcSupportHost::kHostAppId)) {
       const syncer::StringOrdinal arc_host_position =
           GetLastPinPosition(helper->profile());
+      AppLauncherId* app_launcher_id =
+          new AppLauncherId(ArcSupportHost::kHostAppId);
       pin_infos.insert(pin_infos.begin(),
-                       PinInfo(ArcSupportHost::kHostAppId, arc_host_position));
+                       PinInfo(app_launcher_id, arc_host_position));
       app_service->SetPinPosition(ArcSupportHost::kHostAppId,
                                   arc_host_position);
     }
@@ -650,28 +670,34 @@ std::vector<std::string> GetPinnedAppsFromPrefs(
   // 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;
+    pins[i] = pin_infos[i].app_launcher_id;
 
   return pins;
 }
 
-void RemovePinPosition(Profile* profile, const std::string& app_id) {
+void RemovePinPosition(Profile* profile, AppLauncherId* app_launcher_id) {
   DCHECK(profile);
-  DCHECK(!app_id.empty());
+  const std::string launcher_id = app_launcher_id->GetAppLauncherID();
+  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::string& app_id_after) {
+                    AppLauncherId* app_launcher_id,
+                    AppLauncherId* app_launcher_id_before,
+                    AppLauncherId* app_launcher_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);
+  const std::string launcher_id = app_launcher_id->GetAppLauncherID();
+  DCHECK(!launcher_id.empty());
+  const std::string launcher_id_before =
+      app_launcher_id_before->GetAppLauncherID();
+  DCHECK_NE(launcher_id, launcher_id_before);
+  const std::string launcher_id_after =
+      app_launcher_id_after->GetAppLauncherID();
+  DCHECK_NE(launcher_id, launcher_id_after);
+  DCHECK(launcher_id_before.empty() || launcher_id_before != launcher_id_after);
 
   app_list::AppListSyncableService* app_service =
       app_list::AppListSyncableServiceFactory::GetForProfile(profile);
@@ -680,11 +706,13 @@ 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 =
-      app_id_after.empty() ? syncer::StringOrdinal()
-                           : app_service->GetPinPosition(app_id_after);
+      launcher_id_after.empty()
+          ? syncer::StringOrdinal()
+          : app_service->GetPinPosition(launcher_id_after);
 
   syncer::StringOrdinal pin_position;
   if (position_before.IsValid() && position_after.IsValid())
@@ -695,8 +723,13 @@ 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);
 }
 
+AppLauncherId::AppLauncherId(const std::string& app_id)
+    : app_launcher_id_(app_id) {}
+
+AppLauncherId::~AppLauncherId() {}
+
 }  // namespace launcher
 }  // namespace ash