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 180515)
+++ 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()
@@ -137,6 +140,10 @@
       known_galleries_.find(gallery->pref_id);
   if (iter != known_galleries_.end()) {
     iter->second.allowed = enabled;

[vandebo (ex-Chrome), 2013/02/06 00:42:23]

DCHECK_NE(iter->second.allowed, enabled) ?

[Lei Zhang, 2013/02/07 01:44:56]

Done.

+    if (ContainsKey(toggled_galleries_, gallery->pref_id))
+      toggled_galleries_.erase(gallery->pref_id);
+    else
+      toggled_galleries_.insert(gallery->pref_id);
     return;
   }
 
@@ -153,6 +160,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

[vandebo (ex-Chrome), 2013/02/06 00:42:23]

How can the test ctor never add the observer? The test ctor has to call the base
class's ctor...

[Lei Zhang, 2013/02/07 01:44:56]

The tests go through MediaGalleriesDialogControllerMock which calls the second
version of the base ctor which does not add an observer.

+  // this observer in the first place.
+  preferences_->RemoveGalleryChangeObserver(this);
+
   if (accepted)
     SavePermissions();
 
@@ -172,20 +185,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 +210,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 +225,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 +248,9 @@
 
   for (MediaGalleryPrefIdSet::iterator iter = permitted.begin();
        iter != permitted.end(); ++iter) {
+    if (ContainsKey(toggled_galleries_, *iter))
+      continue;
+    DCHECK(ContainsKey(known_galleries_, *iter));
     known_galleries_[*iter].allowed = true;
   }
 }
@@ -253,11 +271,66 @@
     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() {

[vandebo (ex-Chrome), 2013/02/06 00:42:23]

I've previously suggested walking both lists concurrently.  I wanted to see if
it was easier to reed, so I wrote it.  I'm not sure it's easier to read, but it
is more explicit about information flow.  What do you think?

void MediaGalleriesDialogController::UpdateGalleriesOnPreferencesEvent() {
  MediaGalleryPrefIdSet galleries_to_forget;
  const MediaGalleriesPrefInfoMap& pref_galleries =
      preferences_->known_galleries();
  KnownGalleryPermissions::const_iterator existing = known_galleries_.begin();

  // Walk the existing set of galleries along with the new set retrieved from
  // preferences.
  for (MediaGalleriesPrefInfoMap::const_iterator it = pref_galleries.begin();
       it != pref_galleries.end();
       ++it) {
    // Anything that's in the existing set and not in the new set should be
    // removed.
    while (existing->first < it->first) {
      galleries_to_forget.insert(existing->first);
      dialog_->ForgetGallery(&existing->second.pref_info);
      ++existing;
    }
    // Entries that are in both sets need to be updated.
    if (existing->first == it->first)
      if (it->second.type == MediaGalleryPrefInfo::kBlackListed)) {
        galleries_to_forget.insert(existing->first);
        dialog_->ForgetGallery(&existing->second.pref_info);
      } else {
        bool allowed = it->second.allowed;
        if (ContainsKey(toggled_galleries_, it->first))
          allowed = existing->second.allowed;
        dialog_->UpdateGallery(&existing->second.pref_info, allowed);
      }
      ++existing;
    } else {
      // Add entries that are just in preferences.
      DCHECK_GT(existing->first > it->first);
      bool allowed = it->second.allowed;
      // If it was already added in this dialog, remove the added one first.
      for (NewGalleryPermissions::iterator added_it = new_galleries_.begin();
           added_it != new_galleries_.end();
           ++added_it) {
        if (added_it->pref_info.path == it->second.path &&
            added_it->pref_info.device_id == it->second.device_id) {
          allowed = new_if->allowed;
          dialog_->ForgetGallery(&new_it->pref_info);
          new_galleries_.erase(added_it);
          break;
        }
      }

      existing = known_galleries_.insert(
          std::pair<MediaGalleryPrefId, GalleryPermission>(
              it->first,
              GalleryPermission(it->second.pref_info, allowed)))->first;
      dialog_->UpdateGallery(&existing->second.pref_info, allowed);
      ++existing;
    }
  }

  // Remove the galleries to forget from |known_galleries_|. Doing it in the
  // above loop would invalidate the iterator there.
  for (MediaGalleryPrefIdSet::const_iterator it = galleries_to_forget.begin();
       it != galleries_to_forget.end();
       ++it) {
    known_galleries_.erase(*it);
  }
}

+  // Merge in the permissions from |preferences_|. Afterwards,
+  // |known_galleries_| may contain galleries that no longer belong there.
+  InitializePermissions();
+
+  // If a gallery still belong in |known_galleries_|, update its view.
+  // If it no longer belongs in |known_galleries_|, forget it in the model/view.
+  const MediaGalleriesPrefInfoMap& pref_galleries =
+      preferences_->known_galleries();
+  MediaGalleryPrefIdSet galleries_to_forget;
+  for (KnownGalleryPermissions::const_iterator it = known_galleries_.begin();
+       it != known_galleries_.end();
+       ++it) {
+    const MediaGalleryPrefId& gallery_id = it->first;
+    const GalleryPermission& gallery = it->second;
+    MediaGalleriesPrefInfoMap::const_iterator pref_it =
+        pref_galleries.find(gallery_id);
+    if (pref_it == pref_galleries.end() ||
+        pref_it->second.type == MediaGalleryPrefInfo::kBlackListed) {
+      galleries_to_forget.insert(gallery_id);
+      dialog_->ForgetGallery(&gallery.pref_info);
+    } else {
+      dialog_->UpdateGallery(&gallery.pref_info, gallery.allowed);
+    }
+  }
+
+  // Remove the galleries to forget from |known_galleries_|. Doing it in the
+  // above loop would invalidate the iterator there.
+  for (MediaGalleryPrefIdSet::const_iterator it = galleries_to_forget.begin();
+       it != galleries_to_forget.end();
+       ++it) {
+    known_galleries_.erase(*it);
+  }
+
+  // Resolve |new_galleries_| against the updated |known_galleries_|.

[vandebo (ex-Chrome), 2013/02/06 00:42:23]

Does this need to go first?  We will have already called UpdateGallery for the
replacement view of the gallery.

[Lei Zhang, 2013/02/07 01:44:56]

I think it's fine to do it here. There's no way to reuse the existing entry
because it's pointing to memory in |new_galleries_| but now the entry's info is
in |known_galleries_|.

For the case this loop is trying to solve, no matter how you do it, there's
going to be a moment when there's two entries for a gallery, one in
|known_galleries_| and one in |new_galleries_|. The update happens fast enough
that users are not going to see any visual glitches, if that's what you are
worried about.

[vandebo (ex-Chrome), 2013/02/07 02:08:05]

I just don't know how the dialog actually works so I didn't know if it was ok to
have two entries for the same gallery.

+  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();