Clean up media gallery preferences.

Handle some todo's minor performance issues and some corner cases.

BUG=NONE


Review URL: https://chromiumcodereview.appspot.com/10836199

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@151390 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/media_gallery/media_galleries_dialog_controller.cc

@@ -148,10 +148,10 @@ void MediaGalleriesDialogController::LookUpPermissions() {
     known_galleries_[iter->first] = GalleryPermission(iter->second, false);
   }
 
-  std::set<MediaGalleryPrefId> permitted =
+  MediaGalleryPrefIdSet permitted =
       preferences_->GalleriesForExtension(extension_);
 
-  for (std::set<MediaGalleryPrefId>::iterator iter = permitted.begin();
+  for (MediaGalleryPrefIdSet::iterator iter = permitted.begin();
        iter != permitted.end(); ++iter) {
     known_galleries_[*iter].allowed = true;
   }

chrome/browser/media_gallery/media_galleries_preferences.cc

@@ -116,23 +116,6 @@ DictionaryValue* CreateGalleryPrefInfoDictionary(
   return dict;
 }
 
-bool FindPrefIdFromDeviceId(const MediaGalleriesPrefInfoMap& known_galleries,
-                            const std::string& device_id,
-                            MediaGalleryPrefId* pref_id)  {
-  // TODO(vandebo) Handle multiple galleries that use different paths.
-  // TODO(vandebo) Should we keep a second map device_id->pref_id?
-  for (MediaGalleriesPrefInfoMap::const_iterator it = known_galleries.begin();
-       it != known_galleries.end();
-       ++it) {
-    if (it->second.device_id == device_id) {
-      if (pref_id)
-        *pref_id = it->second.pref_id;
-      return true;
-    }
-  }
-  return false;
-}
-
 FilePath MakePathRelative(FilePath path) {
   if (!path.IsAbsolute())
     return path;
@@ -182,6 +165,7 @@ MediaGalleriesPreferences::~MediaGalleriesPreferences() {}
 
 void MediaGalleriesPreferences::InitFromPrefs() {
   known_galleries_.clear();
+  device_map_.clear();
 
   PrefService* prefs = profile_->GetPrefs();
   const ListValue* list = prefs->GetList(
@@ -197,6 +181,7 @@ void MediaGalleriesPreferences::InitFromPrefs() {
     MediaGalleryPrefInfo gallery_info;
     if (PopulateGalleryPrefInfoFromDictionary(*dict, &gallery_info))
       known_galleries_[gallery_info.pref_id] = gallery_info;
+      device_map_[gallery_info.device_id].insert(gallery_info.pref_id);
   }
 }
 
@@ -205,47 +190,70 @@ bool MediaGalleriesPreferences::LookUpGalleryByPath(
     MediaGalleryPrefInfo* gallery_info) const {
   std::string device_id =
       MediaFileSystemRegistry::GetInstance()->GetDeviceIdFromPath(path);
-  MediaGalleryPrefId pref_id;
-  if (!FindPrefIdFromDeviceId(known_galleries_, device_id, &pref_id)) {
-    if (gallery_info) {
-      gallery_info->pref_id = kInvalidMediaGalleryPrefId;
-      gallery_info->display_name = ComputeDisplayName(path);
-      gallery_info->device_id = device_id;
-      gallery_info->path = MakePathRelative(path);
-      gallery_info->type = MediaGalleryPrefInfo::kUserAdded;
-    }
-    return false;
+  FilePath relative_path = MakePathRelative(path);
+  MediaGalleryPrefIdSet galleries_on_device =
+    LookUpGalleriesByDeviceId(device_id);
+  for (MediaGalleryPrefIdSet::const_iterator it = galleries_on_device.begin();
+       it != galleries_on_device.end();
+       ++it) {
+    const MediaGalleryPrefInfo& gallery = known_galleries_.find(*it)->second;
+    if (gallery.path != relative_path)
+      continue;
+
+    if (gallery_info)
+      *gallery_info = gallery;
+    return true;
   }
 
   if (gallery_info) {
-    MediaGalleriesPrefInfoMap::const_iterator it =
-        known_galleries_.find(pref_id);
-    DCHECK(it != known_galleries_.end());
-    *gallery_info = it->second;
+    gallery_info->pref_id = kInvalidMediaGalleryPrefId;
+    gallery_info->display_name = ComputeDisplayName(path);
+    gallery_info->device_id = device_id;
+    gallery_info->path = relative_path;
+    gallery_info->type = MediaGalleryPrefInfo::kUserAdded;
   }
-  return true;
+  return false;
+}
+
+MediaGalleryPrefIdSet MediaGalleriesPreferences::LookUpGalleriesByDeviceId(
+    const std::string& device_id) const {
+  DeviceIdPrefIdsMap::const_iterator found = device_map_.find(device_id);
+  if (found == device_map_.end()) {
+    MediaGalleryPrefIdSet result;
+    return result;
+  }
+
+  return found->second;
 }
 
+
 MediaGalleryPrefId MediaGalleriesPreferences::AddGallery(
     const std::string& device_id, const string16& display_name,
     const FilePath& path, bool user_added) {
   DCHECK(display_name.length() > 0);
-  MediaGalleryPrefId existing_id;
-  if (FindPrefIdFromDeviceId(known_galleries_, device_id, &existing_id)) {
-    const MediaGalleryPrefInfo& existing = known_galleries_[existing_id];
+  FilePath relative_path = MakePathRelative(path);
+  MediaGalleryPrefIdSet galleries_on_device =
+    LookUpGalleriesByDeviceId(device_id);
+  for (MediaGalleryPrefIdSet::const_iterator it = galleries_on_device.begin();
+       it != galleries_on_device.end();
+       ++it) {
+    if (known_galleries_[*it].path != relative_path)
+      continue;
+
+    const MediaGalleryPrefInfo& existing = known_galleries_[*it];
     if (existing.type == MediaGalleryPrefInfo::kBlackListed) {
       PrefService* prefs = profile_->GetPrefs();
       ListPrefUpdate update(prefs, prefs::kMediaGalleriesRememberedGalleries);
       ListValue* list = update.Get();
 
-      for (ListValue::const_iterator it = list->begin();
-           it != list->end();
-           ++it) {
+      for (ListValue::const_iterator list_iter = list->begin();
+           list_iter != list->end();
+           ++list_iter) {
         DictionaryValue* dict;
         MediaGalleryPrefId iter_id;
-        if ((*it)->GetAsDictionary(&dict) &&
+        if ((*list_iter)->GetAsDictionary(&dict) &&
             GetPrefId(*dict, &iter_id) &&
-            existing_id == iter_id) {
+            *it == iter_id) {
           dict->SetString(kMediaGalleriesTypeKey,
                           kMediaGalleriesTypeAutoDetectedValue);
           InitFromPrefs();
@@ -253,10 +261,9 @@ MediaGalleryPrefId MediaGalleriesPreferences::AddGallery(
         }
       }
     }
-    return existing_id;
+    return *it;
   }
 
-  FilePath relative_path = MakePathRelative(path);
   PrefService* prefs = profile_->GetPrefs();
 
   MediaGalleryPrefInfo gallery_info;
@@ -290,6 +297,9 @@ void MediaGalleriesPreferences::ForgetGalleryById(MediaGalleryPrefId pref_id) {
   ListPrefUpdate update(prefs, prefs::kMediaGalleriesRememberedGalleries);
   ListValue* list = update.Get();
 
+  if (known_galleries_.find(pref_id) == known_galleries_.end())
+    return;
+
   for (ListValue::iterator iter = list->begin(); iter != list->end(); ++iter) {
     DictionaryValue* dict;
     MediaGalleryPrefId iter_id;
@@ -310,10 +320,9 @@ void MediaGalleriesPreferences::ForgetGalleryById(MediaGalleryPrefId pref_id) {
   }
 }
 
-std::set<MediaGalleryPrefId>
-MediaGalleriesPreferences::GalleriesForExtension(
+MediaGalleryPrefIdSet MediaGalleriesPreferences::GalleriesForExtension(
     const extensions::Extension& extension) const {
-  std::set<MediaGalleryPrefId> result;
+  MediaGalleryPrefIdSet result;
   if (extension.HasAPIPermission(
           extensions::APIPermission::kMediaGalleriesAllGalleries)) {
     for (MediaGalleriesPrefInfoMap::const_iterator it =
@@ -348,12 +357,16 @@ void MediaGalleriesPreferences::SetGalleryPermissionForExtension(
     const extensions::Extension& extension,
     MediaGalleryPrefId pref_id,
     bool has_permission) {
+  // The gallery may not exist anymore if the user opened a second config
+  // surface concurrently and removed it. Drop the permission update if so.
+  MediaGalleriesPrefInfoMap::iterator gallery_info =
+      known_galleries_.find(pref_id);
+  if (gallery_info == known_galleries_.end())
+    return;
+
   bool all_permission = extension.HasAPIPermission(
       extensions::APIPermission::kMediaGalleriesAllGalleries);
   if (has_permission && all_permission) {
-    MediaGalleriesPrefInfoMap::iterator gallery_info =
-        known_galleries_.find(pref_id);
-    DCHECK(gallery_info != known_galleries_.end());
     if (gallery_info->second.type == MediaGalleryPrefInfo::kAutoDetected) {
       GetExtensionPrefs()->UnsetMediaGalleryPermission(extension.id(), pref_id);
       return;

chrome/browser/media_gallery/media_galleries_preferences.h

@@ -65,6 +65,7 @@ struct MediaGalleryPrefInfo {
 
 typedef std::map<MediaGalleryPrefId, MediaGalleryPrefInfo>
     MediaGalleriesPrefInfoMap;
+typedef std::set<MediaGalleryPrefId> MediaGalleryPrefIdSet;
 
 // A class to manage the media gallery preferences.  There is one instance per
 // user profile.
@@ -85,6 +86,9 @@ class MediaGalleriesPreferences : public ProfileKeyedService {
   bool LookUpGalleryByPath(const FilePath& path,
                            MediaGalleryPrefInfo* gallery) const;
 
+  MediaGalleryPrefIdSet LookUpGalleriesByDeviceId(
+      const std::string& device_id) const;
+
   // Teaches the registry about a new gallery.
   MediaGalleryPrefId AddGallery(const std::string& device_id,
                                 const string16& display_name,
@@ -99,7 +103,7 @@ class MediaGalleriesPreferences : public ProfileKeyedService {
   // Removes the gallery identified by |id| from the store.
   void ForgetGalleryById(MediaGalleryPrefId id);
 
-  std::set<MediaGalleryPrefId> GalleriesForExtension(
+  MediaGalleryPrefIdSet GalleriesForExtension(
       const extensions::Extension& extension) const;
 
   void SetGalleryPermissionForExtension(const extensions::Extension& extension,
@@ -123,12 +127,18 @@ class MediaGalleriesPreferences : public ProfileKeyedService {
   static string16 ComputeDisplayName(const FilePath& path);
 
  private:
+  typedef std::map<std::string /*device id*/, MediaGalleryPrefIdSet>
+      DeviceIdPrefIdsMap;
+
   // The profile that owns |this|.
   Profile* profile_;
 
   // An in-memory cache of known galleries.
   MediaGalleriesPrefInfoMap known_galleries_;
 
+  // A mapping from device id to the set of gallery pref ids on that device.
+  DeviceIdPrefIdsMap device_map_;
+
   extensions::ExtensionPrefs* GetExtensionPrefs() const;
 
   DISALLOW_COPY_AND_ASSIGN(MediaGalleriesPreferences);

chrome/browser/media_gallery/media_galleries_preferences_unittest.cc

@@ -37,6 +37,9 @@ class TestMediaGalleriesPreferences : public MediaGalleriesPreferences {
 
 class MediaGalleriesPreferencesTest : public testing::Test {
  public:
+  typedef std::map<std::string /*device id*/, MediaGalleryPrefIdSet>
+      DeviceIdPrefIdsMap;
+
   MediaGalleriesPreferencesTest()
       : ui_thread_(content::BrowserThread::UI, &loop_),
         file_thread_(content::BrowserThread::FILE, &loop_),
@@ -105,6 +108,14 @@ class MediaGalleriesPreferencesTest : public testing::Test {
       VerifyGalleryInfo(&it->second, it->first);
     }
 
+    for (DeviceIdPrefIdsMap::const_iterator it = expected_device_map.begin();
+         it != expected_device_map.end();
+         ++it) {
+      MediaGalleryPrefIdSet actual_id_set =
+          gallery_prefs_->LookUpGalleriesByDeviceId(it->first);
+      EXPECT_EQ(it->second, actual_id_set);
+    }
+
     std::set<MediaGalleryPrefId> galleries_for_all =
         gallery_prefs_->GalleriesForExtension(*all_permission_extension.get());
     EXPECT_EQ(expected_galleries_for_all, galleries_for_all);
@@ -145,11 +156,13 @@ class MediaGalleriesPreferencesTest : public testing::Test {
     expected_galleries_[id].pref_id = id;
     expected_galleries_[id].display_name = ASCIIToUTF16(display_name);
     expected_galleries_[id].device_id = device_id;
-    expected_galleries_[id].path = FilePath(path);
+    expected_galleries_[id].path = FilePath(path).NormalizePathSeparators();
     expected_galleries_[id].type = type;
 
     if (type == MediaGalleryPrefInfo::kAutoDetected)
       expected_galleries_for_all.insert(id);
+
+    expected_device_map[device_id].insert(id);
   }
 
   scoped_refptr<extensions::Extension> all_permission_extension;
@@ -159,6 +172,8 @@ class MediaGalleriesPreferencesTest : public testing::Test {
   std::set<MediaGalleryPrefId> expected_galleries_for_all;
   std::set<MediaGalleryPrefId> expected_galleries_for_regular;
 
+  DeviceIdPrefIdsMap expected_device_map;
+
   MediaGalleriesPrefInfoMap expected_galleries_;
 
  private:
@@ -294,6 +309,7 @@ TEST_F(MediaGalleriesPreferencesTest, GalleryManagement) {
   // Remove a user added gallery and it should go away.
   gallery_prefs()->ForgetGalleryById(user_added_id);
   expected_galleries_.erase(user_added_id);
+  expected_device_map[device_id].erase(user_added_id);
   Verify();
 }
 
@@ -404,6 +420,84 @@ TEST_F(MediaGalleriesPreferencesTest, GalleryPermissions) {
       *regular_permission_extension.get(), user_added_id, false);
   expected_galleries_for_regular.erase(user_added_id);
   Verify();
+
+  // Add permission for an invalid gallery id.
+  gallery_prefs()->SetGalleryPermissionForExtension(
+      *regular_permission_extension.get(), 9999L, true);
+  Verify();
+}
+
+TEST_F(MediaGalleriesPreferencesTest, MultipleGalleriesPerDevices) {
+  FilePath path;
+  std::string device_id;
+  Verify();
+
+  // Add a regular gallery
+  path = MakePath("new_user");
+  device_id = MediaFileSystemRegistry::GetInstance()->GetDeviceIdFromPath(path);
+  MediaGalleryPrefId user_added_id = gallery_prefs()->AddGallery(
+      device_id, ASCIIToUTF16("NewUserGallery"), path, true /*user*/);
+  EXPECT_EQ(default_galleries_count() + 1UL, user_added_id);
+  AddGalleryExpectation(user_added_id, "NewUserGallery", device_id,
+                        FILE_PATH_LITERAL("new_user"),
+                        MediaGalleryPrefInfo::kUserAdded);
+  Verify();
+
+  // Find it by device id and fail to find something related.
+  MediaGalleryPrefIdSet pref_id_set;
+  pref_id_set = gallery_prefs()->LookUpGalleriesByDeviceId(device_id);
+  EXPECT_EQ(1U, pref_id_set.size());
+  EXPECT_TRUE(pref_id_set.find(user_added_id) != pref_id_set.end());
+
+  device_id = MediaFileSystemRegistry::GetInstance()->GetDeviceIdFromPath(
+      MakePath("new_user/foo"));
+  pref_id_set = gallery_prefs()->LookUpGalleriesByDeviceId(device_id);
+  EXPECT_EQ(0U, pref_id_set.size());
+
+  // Add some galleries on the same device.
+  path = MakePath("path1/on/device1");
+  device_id = "device1";
+  MediaGalleryPrefId dev1_path1_id = gallery_prefs()->AddGallery(
+      device_id, ASCIIToUTF16("Device1Path1"), path, true /*user*/);
+  EXPECT_EQ(default_galleries_count() + 2UL, dev1_path1_id);
+  AddGalleryExpectation(dev1_path1_id, "Device1Path1", device_id,
+                        FILE_PATH_LITERAL("path1/on/device1"),
+                        MediaGalleryPrefInfo::kUserAdded);
+  Verify();
+
+  path = MakePath("path2/on/device1");
+  MediaGalleryPrefId dev1_path2_id = gallery_prefs()->AddGallery(
+      device_id, ASCIIToUTF16("Device1Path2"), path, true /*user*/);
+  EXPECT_EQ(default_galleries_count() + 3UL, dev1_path2_id);
+  AddGalleryExpectation(dev1_path2_id, "Device1Path2", device_id,
+                        FILE_PATH_LITERAL("path2/on/device1"),
+                        MediaGalleryPrefInfo::kUserAdded);
+  Verify();
+
+  path = MakePath("path1/on/device2");
+  device_id = "device2";
+  MediaGalleryPrefId dev2_path1_id = gallery_prefs()->AddGallery(
+      device_id, ASCIIToUTF16("Device2Path1"), path, true /*user*/);
+  EXPECT_EQ(default_galleries_count() + 4UL, dev2_path1_id);
+  AddGalleryExpectation(dev2_path1_id, "Device2Path1", device_id,
+                        FILE_PATH_LITERAL("path1/on/device2"),
+                        MediaGalleryPrefInfo::kUserAdded);
+  Verify();
+
+  path = MakePath("path2/on/device2");
+  MediaGalleryPrefId dev2_path2_id = gallery_prefs()->AddGallery(
+      device_id, ASCIIToUTF16("Device2Path2"), path, true /*user*/);
+  EXPECT_EQ(default_galleries_count() + 5UL, dev2_path2_id);
+  AddGalleryExpectation(dev2_path2_id, "Device2Path2", device_id,
+                        FILE_PATH_LITERAL("path2/on/device2"),
+                        MediaGalleryPrefInfo::kUserAdded);
+  Verify();
+
+  // Check that adding one of them again works as expected.
+  MediaGalleryPrefId id = gallery_prefs()->AddGallery(
+      device_id, ASCIIToUTF16("Device2Path2"), path, true /*user*/);
+  EXPECT_EQ(dev2_path2_id, id);
+  Verify();
 }
 
 }  // namespace