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

chrome/browser/media_gallery/media_galleries_dialog_controller.h

Index: chrome/browser/media_gallery/media_galleries_dialog_controller.h
===================================================================
--- chrome/browser/media_gallery/media_galleries_dialog_controller.h	(revision 179687)
+++ chrome/browser/media_gallery/media_galleries_dialog_controller.h	(working copy)
@@ -39,6 +39,9 @@
   virtual void UpdateGallery(const MediaGalleryPrefInfo* gallery,

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

It seems to me that there should be three methods, or one, but not two:
UpdateGallery(PrefInfo, enum {unchecked, checked, removed}), or
AddGallery(PrefInfo), RemoveGallery(PrefInfo), UpdateGalleryPermission(PrefInfo,
bool permitted). What do you think?
Also, in case you missed it, as an abstract method, you'll have to update the
implementations before committing.

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

You should know that Update does more than just update the checkbox. It's also
used to refresh the tooltip and gallery name. The comment is slightly out of
date.

Really, what I have is AddOrUpdate and Forget. We can do Add/Remove/Update, but
that's a lot less forgiving on the dialog controller, which has to be more
careful about when to call Add vs Update.
I mentioned the lack of UI specific implementations in my original message.

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

What about UpdateGallery(PrefInfo, enum {unchecked, checked, removed})? 
Removing is a way of updating, and it would remove a bool in favor of an enum. 
If the implementation with Update and Forget is easier, that's fine.
Oops, missed that, sorry.

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

I really would like to keep Remove separate from Update and not overload the
|permitted| parameter to become a tri-state variable.

                              bool permitted) = 0;
 
+  // If there exists a checkbox for |gallery|, it should be removed.
+  virtual void ForgetGallery(const MediaGalleryPrefInfo* gallery) = 0;
+
   // Constructs a platform-specific dialog owned and controlled by |controller|.
   static MediaGalleriesDialog* Create(
       MediaGalleriesDialogController* controller);
@@ -49,7 +52,8 @@
 // the dialog and owns itself.
 class MediaGalleriesDialogController
     : public ui::SelectFileDialog::Listener,
-      public RemovableStorageObserver {
+      public RemovableStorageObserver,
+      public MediaGalleriesPreferences::GalleryChangeObserver {
  public:
   // A fancy pair.
   struct GalleryPermission {
@@ -107,13 +111,21 @@
   virtual void OnRemovableStorageDetached(
       const RemovableStorageNotifications::StorageInfo& info) OVERRIDE;
 
+  // MediaGalleriesPreferences::GalleryChangeObserver implementation:
+  virtual void OnGalleryChanged(MediaGalleriesPreferences* pref) OVERRIDE;
+
   // Populates |known_galleries_|.
   void InitializePermissions();
 
   // Saves state of |known_galleries_| and |new_galleries_| to model.
   void SavePermissions();
 
-  // Update the model and view when a device is attached or detached.
+  // Updates the model and view when |preferences_| changes. Some of the
+  // possible changes includes a gallery getting blacklisted, or a new
+  // auto detected gallery coming available.

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

nit: coming->becoming

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

Done.

+  void UpdateGalleriesOnPreferencesEvent();
+
+  // Updates the model and view when a device is attached or detached.
   void UpdateGalleriesOnDeviceEvent(const std::string& device_id);

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

Do you still need this when you have the OnPreferencesEvent ?

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

There's no preference event when a device gets detached. We can probably get rid
of OnRemovableStorageAttached(), but I need to test that.

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

Oh, you still need these to update the attached/detached status.  Got it.

 
   // The web contents from which the request originated.
@@ -126,9 +138,12 @@
   // This map excludes those galleries which have been blacklisted; it only
   // counts active known galleries.
   KnownGalleryPermissions known_galleries_;
+
+  // Map of new galleries the user added, but have not saved. This list should
+  // never overlap with |known_galleries_|.
   NewGalleryPermissions new_galleries_;
 
-  // We run this callback when done.
+  // Callback to run when the dialog closes.
   base::Closure on_finish_;
 
   // The model that tracks galleries and extensions' permissions.