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

chrome/browser/media_gallery/media_galleries_dialog_controller.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/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"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/system_monitor/media_storage_util.h"
 #include "chrome/browser/system_monitor/removable_storage_notifications.h"
 #include "chrome/browser/ui/chrome_select_file_policy.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/extensions/extension.h"
 #include "content/public/browser/web_contents.h"
@@ skipping 34 matching lines @@
   preferences_ = registry->GetPreferences(
       Profile::FromBrowserContext(web_contents->GetBrowserContext()));
   InitializePermissions();
 
   dialog_.reset(MediaGalleriesDialog::Create(this));
 
   RemovableStorageNotifications* notifications =
       RemovableStorageNotifications::GetInstance();
   if (notifications)
     notifications->AddObserver(this);
+
+  preferences_->AddGalleryChangeObserver(this);
 }
 
 MediaGalleriesDialogController::MediaGalleriesDialogController()
     : web_contents_(NULL),
       extension_(NULL),
       preferences_(NULL) {}
 
 MediaGalleriesDialogController::~MediaGalleriesDialogController() {
   RemovableStorageNotifications* notifications =
       RemovableStorageNotifications::GetInstance();
@@ skipping 57 matching lines @@
       NULL);
 }
 
 void MediaGalleriesDialogController::DidToggleGallery(
     const MediaGalleryPrefInfo* gallery,
     bool enabled) {
   // Check known galleries.
   KnownGalleryPermissions::iterator iter =
       known_galleries_.find(gallery->pref_id);
   if (iter != known_galleries_.end()) {
+    DCHECK_NE(iter->second.allowed, enabled);
     iter->second.allowed = enabled;
+    if (ContainsKey(toggled_galleries_, gallery->pref_id))
+      toggled_galleries_.erase(gallery->pref_id);
+    else
+      toggled_galleries_.insert(gallery->pref_id);
     return;
   }
 
   // Check new galleries.
   for (NewGalleryPermissions::iterator iter = new_galleries_.begin();
        iter != new_galleries_.end(); ++iter) {
     if (&iter->pref_info == gallery) {
       iter->allowed = enabled;
       return;
     }
   }
 
   NOTREACHED();
 }
 
 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();
 
   on_finish_.Run();
   delete this;
 }
 
 const MediaGalleriesDialogController::KnownGalleryPermissions&
 MediaGalleriesDialogController::permissions() const {
   return known_galleries_;
 }
 
 content::WebContents* MediaGalleriesDialogController::web_contents() {
   return web_contents_;
 }
 
 void MediaGalleriesDialogController::FileSelected(const base::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;
   }
 
   // Try to find it in |new_galleries_| (user added same folder twice).
   for (NewGalleryPermissions::iterator iter = new_galleries_.begin();
        iter != new_galleries_.end(); ++iter) {
     if (iter->pref_info.path == gallery.path &&
         iter->pref_info.device_id == gallery.device_id) {
       iter->allowed = true;
       dialog_->UpdateGallery(&iter->pref_info, true);
       return;
     }
   }
 
-  // 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);
 }
 
 void MediaGalleriesDialogController::OnRemovableStorageAttached(
     const RemovableStorageNotifications::StorageInfo& info) {
   UpdateGalleriesOnDeviceEvent(info.device_id);
 }
 
 void MediaGalleriesDialogController::OnRemovableStorageDetached(
     const RemovableStorageNotifications::StorageInfo& info) {
   UpdateGalleriesOnDeviceEvent(info.device_id);
 }
 
+void MediaGalleriesDialogController::OnGalleryChanged(
+    MediaGalleriesPreferences* pref, const std::string& extension_id) {
+  DCHECK_EQ(preferences_, pref);
+  if (extension_id.empty() || extension_id == extension_->id())

[Greg Billock, 2013/02/08 20:09:25]

Will this check end up being useful? That is, it seems likely that simultaneous
actions by the user will be on the same extension, and a rescan is pretty cheap
anyway if it isn't (we don't need the extension_id to do it).

+    UpdateGalleriesOnPreferencesEvent();
+}
+
 void MediaGalleriesDialogController::InitializePermissions() {
   const MediaGalleriesPrefInfoMap& galleries = preferences_->known_galleries();
   for (MediaGalleriesPrefInfoMap::const_iterator iter = galleries.begin();
        iter != galleries.end();
        ++iter) {
     const MediaGalleryPrefInfo& gallery = iter->second;
     if (gallery.type == MediaGalleryPrefInfo::kBlackListed)
       continue;
 
     known_galleries_[iter->first] = GalleryPermission(gallery, false);
   }
 
   MediaGalleryPrefIdSet permitted =
       preferences_->GalleriesForExtension(*extension_);
 
   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;
   }
 }
 
 void MediaGalleriesDialogController::SavePermissions() {
   for (KnownGalleryPermissions::const_iterator iter = known_galleries_.begin();
        iter != known_galleries_.end(); ++iter) {
     preferences_->SetGalleryPermissionForExtension(
         *extension_, iter->first, iter->second.allowed);
   }
 
   for (NewGalleryPermissions::const_iterator iter = new_galleries_.begin();
        iter != new_galleries_.end(); ++iter) {
     // If the user added a gallery then unchecked it, forget about it.
     if (!iter->allowed)
       continue;
 
     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() {
+  // Merge in the permissions from |preferences_|. Afterwards,
+  // |known_galleries_| may contain galleries that no longer belong there,
+  // but the code below will put |known_galleries_| back in a consistent state.
+  InitializePermissions();

[Greg Billock, 2013/02/08 20:09:25]

Could we basically just re-initialize here, clearing everything? That'd be
simpler to follow and maintain. Or does that make the dialog flash in a way
that's annoying?

[Lei Zhang, 2013/02/08 21:02:12]

The dialog UI code has raw pointers to the GalleryPermissions in
|known_galleries_|. Thus if we just clear and repopulate it, the UI would be
pointing to invalid memory.

We could rewrite all the UI to avoid using GalleryPermission pointers, but that
seems like a lot of work.

+
+  // If a gallery no longer belongs in |known_galleries_|, forget it in the
+  // model/view.
+  // If a gallery still belong in |known_galleries_|, check for a duplicate
+  // entry in |new_galleries_|, merge its permission and remove it. Then update
+  // the view.
+  const MediaGalleriesPrefInfoMap& pref_galleries =
+      preferences_->known_galleries();
+  MediaGalleryPrefIdSet galleries_to_forget;
+  for (KnownGalleryPermissions::iterator it = known_galleries_.begin();
+       it != known_galleries_.end();
+       ++it) {
+    const MediaGalleryPrefId& gallery_id = it->first;
+    GalleryPermission& gallery = it->second;
+    MediaGalleriesPrefInfoMap::const_iterator pref_it =
+        pref_galleries.find(gallery_id);
+    // Check for lingering entry that should be removed.
+    if (pref_it == pref_galleries.end() ||
+        pref_it->second.type == MediaGalleryPrefInfo::kBlackListed) {
+      galleries_to_forget.insert(gallery_id);
+      dialog_->ForgetGallery(&gallery.pref_info);
+      continue;
+    }
+
+    // Look for duplicate entries in |new_galleries_|.
+    for (NewGalleryPermissions::iterator new_it = new_galleries_.begin();
+         new_it != new_galleries_.end();
+         ++new_it) {
+      if (new_it->pref_info.path == gallery.pref_info.path &&
+          new_it->pref_info.device_id == gallery.pref_info.device_id) {
+        // Found duplicate entry. Get the existing permission from it and then
+        // remove it.
+        gallery.allowed = new_it->allowed;
+        dialog_->ForgetGallery(&new_it->pref_info);
+        new_galleries_.erase(new_it);
+        break;
+      }
+    }
+    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);
   }
 }
 
 void MediaGalleriesDialogController::UpdateGalleriesOnDeviceEvent(
     const std::string& device_id) {
   for (KnownGalleryPermissions::iterator iter = known_galleries_.begin();
        iter != known_galleries_.end(); ++iter) {
     if (iter->second.pref_info.device_id == device_id)
       dialog_->UpdateGallery(&iter->second.pref_info, iter->second.allowed);
   }
 
   for (NewGalleryPermissions::iterator iter = new_galleries_.begin();
        iter != new_galleries_.end(); ++iter) {
     if (iter->pref_info.device_id == device_id)
       dialog_->UpdateGallery(&iter->pref_info, iter->allowed);
   }
 }
 
 // MediaGalleries dialog -------------------------------------------------------
 
 MediaGalleriesDialog::~MediaGalleriesDialog() {}
 
 }  // namespace chrome