Media galleries: configuration dialog controller and GTK impl.

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>
+
+#include "base/memory/scoped_ptr.h"
+#include "base/string16.h"
+#include "chrome/browser/media_gallery/media_galleries_preferences.h"
+#include "ui/base/dialogs/select_file_dialog.h"
+#include "ui/gfx/native_widget_types.h"
+
+namespace extensions {
+struct Extension;
+}
+
+class TabContents;
+
+namespace chrome {
+
+class MediaGalleriesDialogController;
+
+// The view.
+class MediaGalleriesDialog {
+ public:
+  // 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), 2012/08/06 20:39:30]

const ref?

[Evan Stade, 2012/08/06 23:43:02]

the memory address is being used to identify the gallery so a pointer makes more
sense.

+                             bool permitted) = 0;
+
+  // Constructs a platform-specific dialog owned and controlled by |controller|.
+  static MediaGalleriesDialog* Create(
+      MediaGalleriesDialogController* controller);
+};
+
+// An interface that notifies the caller when the user has finished interacting
+// with the dialog.
+class MediaGalleriesDialogDelegate {
+ public:
+  // Called when the dialog has been dismissed.
+  virtual void MediaGalleriesDialogFinished() = 0;
+};
+
+// 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:
+  // The constructor creates a dialog controller which owns itself.
+  MediaGalleriesDialogController(TabContents* tab_contents,
+                                 const extensions::Extension* extension,

[vandebo (ex-Chrome), 2012/08/06 20:39:30]

const ref?

[Evan Stade, 2012/08/06 23:43:02]

extensions are traditionally passed around by pointer, not ref. I'm not sure why
except I think a const ref tends to suggest "this is a thing you can copy if you
want" whereas a pointer implies the opposite.

I noticed that you used const Extension& in the preferences CL and I didn't say
anything but that breaks from the pervasive convention. 

+                                 MediaGalleriesDialogDelegate* delegate);
+
+  // Called by the view.
+  string16 GetHeader();
+  string16 GetSubtext();
+  void OnAddFolderClicked();
+  void GalleryToggled(const MediaGalleryPrefInfo*, bool enabled);
+  void DialogFinished(bool accepted);
+
+  // SelectFileDialog::Listener implementation:
+  virtual void FileSelected(const FilePath& path,
+                            int index,
+                            void* params) OVERRIDE;
+
+  // This type keeps track of media galleries already known to the prefs system.
+  typedef std::map<MediaGalleryPrefId, std::pair<MediaGalleryPrefInfo, bool> >

[vandebo (ex-Chrome), 2012/08/06 20:39:30]

Any advantage to using a pair instead of an explicit struct?  Especially with
map iterators, explicit structs tend to be much easier to read.

[Evan Stade, 2012/08/06 23:43:02]

Done.

+      KnownGalleryPermissions;
+  const KnownGalleryPermissions& permissions() const {
+    return known_galleries_;
+  }
+
+  TabContents* tab_contents() const {
+    return tab_contents_;
+  }
+
+ private:
+  virtual ~MediaGalleriesDialogController();
+
+  // Populates |known_galleries_|.
+  void LookUpPermissions();
+
+  // Saves state of |known_galleries_| and |new_galleries_| to model.
+  void SavePermissions();
+
+  // This type is for media galleries that have been added via "add gallery"
+  // button, but have not yet been committed to the prefs system and will be
+  // forgotten if the user Cancels. Since they don't have IDs assigned yet, it's
+  // just a list and not a map.
+  typedef std::list<std::pair<MediaGalleryPrefInfo, bool> >

[vandebo (ex-Chrome), 2012/08/06 20:39:30]

The struct mentioned above can be reused here.
Any reason to use list vs vector?

[Evan Stade, 2012/08/06 23:43:02]

don't need random access; desire cheap append

+      NewGalleryPermissions;
+
+  // The tab contents from which the request originated.
+  TabContents* tab_contents_;
+
+  // This is just a reference, but it's assumed that it won't become invalid
+  // while the dialog is showing.
+  const extensions::Extension* extension_;

[vandebo (ex-Chrome), 2012/08/06 20:39:30]

Why not actually make it const ref?

[Evan Stade, 2012/08/06 23:43:02]

see above

+
+  KnownGalleryPermissions known_galleries_;
+  NewGalleryPermissions new_galleries_;
+
+  // We notify this delegate when done.
+  MediaGalleriesDialogDelegate* delegate_;
+
+  // 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_