Add AppLauncherId wrapper for items shown in shelf

chrome/browser/ui/ash/chrome_launcher_prefs.cc

 // Copyright (c) 2012 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/chrome_launcher_prefs.h"
 
 #include <stddef.h>
 
 #include <set>
 
@@ skipping 24 matching lines @@
 namespace {
 
 // App ID of default pinned apps.
 const char* kDefaultPinnedApps[] = {
     extension_misc::kGmailAppId, extension_misc::kGoogleDocAppId,
     extension_misc::kYoutubeAppId, ArcSupportHost::kHostAppId};
 
 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();
 }
 
 // Returns the preference value for the display with the given |display_id|.
 // The pref value is stored in |local_path| and |path|, but the pref service may
 // have per-display preferences and the value can be specified by policy.
 // Here is the priority:
 //  * A value managed by policy. This is a single value that applies to all
 //    displays.
@@ skipping 143 matching lines @@
 // copied to |local_path|.
 void PropagatePrefToLocalIfNotSet(
     syncable_prefs::PrefServiceSyncable* pref_service,
     const char* local_path,
     const char* synced_path) {
   if (!pref_service->FindPreference(local_path)->HasUserSetting())
     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) {}
+  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;
 };
 
 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 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,
                            const LauncherControllerHelper* helper) {
     std::string app_id;
     if (!app_pref->GetString(kPinnedAppsPrefAppIDPath, &app_id)) {
       LOG(ERROR) << "Cannot get app id from app pref entry.";
       return;
     }
 
     if (app_id == kPinnedAppsPlaceholder)
       return;
 
     bool pinned_by_policy = false;
     if (app_pref->GetBoolean(kPinnedAppsPrefPinnedByPolicy,
                              &pinned_by_policy) &&
         pinned_by_policy) {
       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
 
 const char kPinnedAppsPrefAppIDPath[] = "id";
 const char kPinnedAppsPrefPinnedByPolicy[] = "pinned_by_policy";
 const char kPinnedAppsPlaceholder[] = "AppShelfIDPlaceholder--------";
 
 const char kShelfAutoHideBehaviorAlways[] = "Always";
 const char kShelfAutoHideBehaviorNever[] = "Never";
 
 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
   // pushed to local state and we'll need to track profile per item.
   registry->RegisterIntegerPref(
       prefs::kShelfChromeIconIndex,
       0,
       user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
   registry->RegisterListPref(prefs::kPinnedLauncherApps,
                              CreateDefaultPinnedAppsList(),
                              user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
   registry->RegisterListPref(prefs::kPolicyPinnedLauncherApps);
   registry->RegisterStringPref(prefs::kShelfAutoHideBehavior,
                                kShelfAutoHideBehaviorNever,
                                user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
   registry->RegisterStringPref(prefs::kShelfAutoHideBehaviorLocal,
                                std::string());
   registry->RegisterStringPref(prefs::kShelfAlignment,
                                kShelfAlignmentBottom,
                                user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
   registry->RegisterStringPref(prefs::kShelfAlignmentLocal, std::string());
   registry->RegisterDictionaryPref(prefs::kShelfPreferences);
   registry->RegisterIntegerPref(prefs::kLogoutDialogDurationMs, 20000);
   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();
 }
 
 ShelfAutoHideBehavior GetShelfAutoHideBehaviorPref(PrefService* prefs,
                                                    int64_t display_id) {
   DCHECK_NE(display_id, display::Display::kInvalidDisplayID);
 
   // Don't show the shelf in app mode.
   if (chrome::IsRunningInAppMode())
     return SHELF_AUTO_HIDE_ALWAYS_HIDDEN;
@@ skipping 76 matching lines @@
       if (!arc_app_list_pref)
         continue;
 
       // We are dealing with package name, not with 32 characters ID.
       const std::string& arc_package = app_id;
       const std::vector<std::string> activities = GetActivitiesForPackage(
           arc_package, all_arc_app_ids, *arc_app_list_pref);
       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
   // 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_apps = prefs->GetList(prefs::kPinnedLauncherApps);
 
   // 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_apps->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));
+      CreateAppDict(AppLauncherId(extension_misc::kChromeAppId)));
 
   AppTracker apps;
   GetAppsPinnedByPolicy(prefs, helper, &apps);
 
   std::string app_id;
   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);
+      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.";
       continue;
     }
     apps.MaybeAddAppFromPref(app_pref, helper);
   }
 
   // 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();
 }
 
 // static
 std::unique_ptr<ChromeLauncherPrefsObserver>
 ChromeLauncherPrefsObserver::CreateIfNecessary(Profile* profile) {
   syncable_prefs::PrefServiceSyncable* prefs =
       PrefServiceSyncableFromProfile(profile);
   if (!prefs->FindPreference(prefs::kShelfAlignmentLocal)->HasUserSetting() ||
       !prefs->FindPreference(prefs::kShelfAutoHideBehaviorLocal)
@@ skipping 58 matching lines @@
     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(
+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());
 
   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);
+  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();
   }
 
   // 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(
+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;
 
   AppTracker 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));
+    pin_infos.push_back(PinInfo(AppLauncherId(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());
 
   // 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;
 
     // Check if we already processed current app.
     if (app_service->GetPinPosition(app_id).IsValid())
       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);
   }
 
   if (helper->IsValidIDForCurrentUser(ArcSupportHost::kHostAppId)) {
     if (!app_service->GetSyncItem(ArcSupportHost::kHostAppId)) {
       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);
   // 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);
+      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)) {
       position_after = position;
       break;
     }
   }
 
   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);
+  app_service->SetPinPosition(launcher_id, pin_position);
 }
 
 }  // namespace launcher
 }  // namespace ash