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

chrome/browser/media_gallery/media_galleries_dialog_controller.h

 // 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.
 
 #ifndef CHROME_BROWSER_MEDIA_GALLERY_MEDIA_GALLERIES_DIALOG_CONTROLLER_H_
 #define CHROME_BROWSER_MEDIA_GALLERY_MEDIA_GALLERIES_DIALOG_CONTROLLER_H_
 
 #include <list>
 #include <map>
 
@@ skipping 18 matching lines @@
 class MediaGalleriesDialogController;
 
 // The view.
 class MediaGalleriesDialog {
  public:
   virtual ~MediaGalleriesDialog();
 
   // Updates the checkbox state for |gallery|. |gallery| is owned by the
   // controller and is guaranteed to live longer than the dialog. If the
   // checkbox doesn't already exist, it should be created.
   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);
 };
 
 // The controller is responsible for handling the logic of the dialog and
 // interfacing with the model (i.e., MediaGalleriesPreferences). It shows
 // the dialog and owns itself.
 class MediaGalleriesDialogController
     : public ui::SelectFileDialog::Listener,
-      public RemovableStorageObserver {
+      public RemovableStorageObserver,
+      public MediaGalleriesPreferences::GalleryChangeObserver {
  public:
   // A fancy pair.
   struct GalleryPermission {
     GalleryPermission(const MediaGalleryPrefInfo& pref_info, bool allowed)
         : pref_info(pref_info), allowed(allowed) {}
     GalleryPermission() {}
 
     MediaGalleryPrefInfo pref_info;
     bool allowed;
   };
@@ skipping 37 matching lines @@
   virtual void FileSelected(const FilePath& path,
                             int index,
                             void* params) OVERRIDE;
 
   // RemovableStorageObserver implementation:
   virtual void OnRemovableStorageAttached(
       const RemovableStorageNotifications::StorageInfo& info) OVERRIDE;
   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.
   content::WebContents* web_contents_;
 
   // This is just a reference, but it's assumed that it won't become invalid
   // while the dialog is showing. Will be NULL only during tests.
   const extensions::Extension* extension_;
 
   // 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.
   MediaGalleriesPreferences* preferences_;
 
   // The view that's showing.
   scoped_ptr<MediaGalleriesDialog> dialog_;
 
   scoped_refptr<ui::SelectFileDialog> select_folder_dialog_;
 
   DISALLOW_COPY_AND_ASSIGN(MediaGalleriesDialogController);
 };
 
 }  // namespace chrome
 
 #endif  // CHROME_BROWSER_MEDIA_GALLERY_MEDIA_GALLERIES_DIALOG_CONTROLLER_H_