Media Galleries: Keep media gallery permission dialogs in sync with the gallery

chrome/browser/media_gallery/media_galleries_dialog_controller.cc

Index: chrome/browser/media_gallery/media_galleries_dialog_controller.cc
===================================================================
--- chrome/browser/media_gallery/media_galleries_dialog_controller.cc	(revision 179687)
+++ chrome/browser/media_gallery/media_galleries_dialog_controller.cc	(working copy)
@@ -5,6 +5,7 @@
 #include "chrome/browser/media_gallery/media_galleries_dialog_controller.h"
 
 #include "base/path_service.h"
+#include "base/stl_util.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/media_gallery/media_file_system_registry.h"
@@ -59,6 +60,8 @@
       RemovableStorageNotifications::GetInstance();
   if (notifications)
     notifications->AddObserver(this);
+
+  preferences_->AddGalleryChangeObserver(this);
 }
 
 MediaGalleriesDialogController::MediaGalleriesDialogController()
@@ -153,6 +156,12 @@
 }
 
 void MediaGalleriesDialogController::DialogFinished(bool accepted) {
+  // The dialog has finished, so there is no need to watch for more updates
+  // from |preferences_|. Do this here and not in the dtor since this is the
+  // only non-test code path that deletes |this|. The test ctor never adds
+  // this observer in the first place.
+  preferences_->RemoveGalleryChangeObserver(this);
+
   if (accepted)
     SavePermissions();
 
@@ -172,20 +181,16 @@
 void MediaGalleriesDialogController::FileSelected(const FilePath& path,
                                                   int /*index*/,
                                                   void* /*params*/) {
-  // Try to find it in |known_galleries_|.
+  // Try to find it in the prefs.
   MediaGalleryPrefInfo gallery;
   bool gallery_exists = preferences_->LookUpGalleryByPath(path, &gallery);
   if (gallery_exists && gallery.type != MediaGalleryPrefInfo::kBlackListed) {
+    // The prefs are in sync with |known_galleries_|, so it should exist in
+    // |known_galleries_| as well. User selecting a known gallery effectively
+    // just sets the gallery to permitted.
     KnownGalleryPermissions::const_iterator iter =
         known_galleries_.find(gallery.pref_id);
-
-    if (iter == known_galleries_.end()) {
-      // This is rare, but could happen if a gallery was not "known"
-      // when the controller first initialized, but has since been added.
-      known_galleries_[gallery.pref_id] = GalleryPermission(gallery, true);
-      iter = known_galleries_.find(gallery.pref_id);
-    }
-
+    DCHECK(iter != known_galleries_.end());
     dialog_->UpdateGallery(&iter->second.pref_info, true);
     return;
   }
@@ -201,7 +206,7 @@
     }
   }
 
-  // Lastly, add it to |new_galleries_|.
+  // Lastly, add a new gallery to |new_galleries_|.
   new_galleries_.push_back(GalleryPermission(gallery, true));
   dialog_->UpdateGallery(&new_galleries_.back().pref_info, true);
 }
@@ -216,6 +221,12 @@
   UpdateGalleriesOnDeviceEvent(info.device_id);
 }
 
+void MediaGalleriesDialogController::OnGalleryChanged(
+    MediaGalleriesPreferences* pref) {
+  DCHECK_EQ(preferences_, pref);
+  UpdateGalleriesOnPreferencesEvent();
+}
+
 void MediaGalleriesDialogController::InitializePermissions() {
   const MediaGalleriesPrefInfoMap& galleries = preferences_->known_galleries();
   for (MediaGalleriesPrefInfoMap::const_iterator iter = galleries.begin();
@@ -233,6 +244,7 @@
 
   for (MediaGalleryPrefIdSet::iterator iter = permitted.begin();
        iter != permitted.end(); ++iter) {
+    DCHECK(ContainsKey(known_galleries_, *iter));
     known_galleries_[*iter].allowed = true;
   }
 }
@@ -253,11 +265,97 @@
     const MediaGalleryPrefInfo& gallery = iter->pref_info;
     MediaGalleryPrefId id = preferences_->AddGallery(
         gallery.device_id, gallery.display_name, gallery.path, true);
-    preferences_->SetGalleryPermissionForExtension(
-        *extension_, id, true);
+    preferences_->SetGalleryPermissionForExtension(*extension_, id, true);
   }
 }
 
+void MediaGalleriesDialogController::UpdateGalleriesOnPreferencesEvent() {
+  // First, update |known_galleries_| from |pref_galleries|.

[vandebo (ex-Chrome), 2013/01/31 01:54:07]

Seems that you could just put this all in OnGalleryChanged

[Lei Zhang, 2013/01/31 06:59:21]

Yes, this is not strictly necessary. I hope it's more readable and for it to
mirror the storage attached event handlers.

[vandebo (ex-Chrome), 2013/01/31 19:38:50]

Didn't notice that part.  The storage attached/detach event handlers are done
that way to remove code duplication, but mirroring that seems fine.

+  const MediaGalleriesPrefInfoMap& pref_galleries =
+      preferences_->known_galleries();
+  for (MediaGalleriesPrefInfoMap::const_iterator it = pref_galleries.begin();
+       it != pref_galleries.end();
+       ++it) {
+    const MediaGalleryPrefInfo& gallery = it->second;
+    if (gallery.type == MediaGalleryPrefInfo::kBlackListed)

[vandebo (ex-Chrome), 2013/01/31 01:54:07]

Seems that this shares some code with InitializePermissions

[Lei Zhang, 2013/02/01 22:04:11]

Done.

+      continue;
+
+    if (ContainsKey(known_galleries_, it->first)) {
+      // Reset all existing galleries to unpermitted.
+      const MediaGalleryPrefInfo& existing_gallery =
+          known_galleries_[it->first].pref_info;
+      DCHECK_EQ(existing_gallery.display_name, gallery.display_name);

[vandebo (ex-Chrome), 2013/01/31 01:54:07]

We might get a gallery change event because the name of a gallery was updated.

[Lei Zhang, 2013/01/31 06:59:21]

I don't think that can happen. Do you have an example in mind? Maybe I shouldn't
worry about this and just accept that the prefs is the more authoritative
information source.

[vandebo (ex-Chrome), 2013/01/31 19:38:50]

See chrome/browser/media_gallery/media_galleries_preferences.cc:268
(update_gallery_name).  We added that so that if the drive letter or volume name
changed on a device, the display name would update as well.

You can treat prefs as the source of truth.

[Lei Zhang, 2013/02/01 22:04:11]

Ya, I'm doing that since I just call InitializePermissions now.

+      DCHECK_EQ(existing_gallery.device_id, gallery.device_id);
+      DCHECK_EQ(existing_gallery.path.value(), gallery.path.value());
+      known_galleries_[it->first].allowed = false;
+    } else {
+      // Add new galleries as unpermitted.
+      known_galleries_[it->first] = GalleryPermission(gallery, false);
+      continue;
+    }
+  }
+
+  // Next, find all the galleries in |known_galleries_| that no longer exist

[vandebo (ex-Chrome), 2013/01/31 01:54:07]

Would it be easier to sort the new and old collections and walk them together?
Same gallery -> update, only in new list -> add, only in old list -> remove

Actually, maps are already sorted.

[Lei Zhang, 2013/01/31 06:59:21]

This seems to go against the suggestion above to re-use InitializePermissions().
It's not fun to walk an iterator and erase from it at the same time.

[vandebo (ex-Chrome), 2013/01/31 19:38:50]

It may not be practical to do both sure, but trying to do one or the other seems
like it could reduce the code and make it more maintainable.

You can certainly play the queue deletes in another list trick.  Even then you'd
still only have two loops.  Right now there are like six loops.

[Lei Zhang, 2013/02/01 22:04:11]

Got it down to InitializePermissions + 3 loops.

+  // in |pref_galleries|.
+  MediaGalleryPrefIdSet galleries_to_forget;
+  for (KnownGalleryPermissions::const_iterator it = known_galleries_.begin();
+       it != known_galleries_.end();
+       ++it) {
+    MediaGalleriesPrefInfoMap::const_iterator gallery_it =
+        pref_galleries.find(it->first);
+    if (gallery_it == pref_galleries.end() ||
+        gallery_it->second.type == MediaGalleryPrefInfo::kBlackListed) {
+      galleries_to_forget.insert(it->first);
+    }
+  }
+
+  // And forget them both in |known_galleries_| and in the view.
+  for (MediaGalleryPrefIdSet::const_iterator it = galleries_to_forget.begin();
+       it != galleries_to_forget.end();
+       ++it) {
+    dialog_->ForgetGallery(&known_galleries_[*it].pref_info);
+    known_galleries_.erase(*it);
+  }
+
+  // Set the permitted bits for the remaining valid galleries.
+  MediaGalleryPrefIdSet permitted =
+      preferences_->GalleriesForExtension(*extension_);
+  for (MediaGalleryPrefIdSet::iterator it = permitted.begin();
+       it != permitted.end();
+       ++it) {
+    DCHECK(ContainsKey(known_galleries_, *it));
+    known_galleries_[*it].allowed = true;
+  }
+
+  // Update the view for |known_galleries_|.
+  for (KnownGalleryPermissions::const_iterator it = known_galleries_.begin();
+       it != known_galleries_.end();
+       ++it) {
+    dialog_->UpdateGallery(&it->second.pref_info, it->second.allowed);
+  }
+
+  // Finally, resolve |new_galleries_| again the updated |known_galleries_|.
+  NewGalleryPermissions::iterator new_it = new_galleries_.begin();
+  while (new_it != new_galleries_.end()) {
+    bool new_gallery_in_known_galleries = false;
+    for (KnownGalleryPermissions::const_iterator it = known_galleries_.begin();
+         it != known_galleries_.end(); ++it) {
+      const MediaGalleryPrefInfo& gallery = it->second.pref_info;
+      if (new_it->pref_info.path == gallery.path &&
+          new_it->pref_info.device_id == gallery.device_id) {
+        new_gallery_in_known_galleries = true;
+        break;
+      }
+    }
+    if (new_gallery_in_known_galleries) {
+      dialog_->ForgetGallery(&new_it->pref_info);
+      new_it = new_galleries_.erase(new_it);
+    } else {
+      ++new_it;
+    }
+  }
+}
+
 void MediaGalleriesDialogController::UpdateGalleriesOnDeviceEvent(
     const std::string& device_id) {
   for (KnownGalleryPermissions::iterator iter = known_galleries_.begin();