media galleries config dialog on views

chrome/browser/ui/views/extensions/media_galleries_dialog_views.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_UI_VIEWS_EXTENSIONS_MEDIA_GALLERIES_DIALOG_VIEWS_H_
+#define CHROME_BROWSER_UI_VIEWS_EXTENSIONS_MEDIA_GALLERIES_DIALOG_VIEWS_H_
+
+#include <map>
+
+#include "base/compiler_specific.h"
+#include "chrome/browser/media_gallery/media_galleries_dialog_controller.h"
+#include "ui/views/window/dialog_delegate.h"
+
+class ConstrainedWindowViews;
+
+namespace views {
+class Checkbox;
+}
+
+namespace chrome {
+
+// The media galleries configuration view for Gtk. It will immediately show

[Peter Kasting, 2012/08/11 23:09:14]

Gtk?

[Evan Stade, 2012/08/13 23:10:30]

Done.

+// upon construction.
+class MediaGalleriesDialogViews : public MediaGalleriesDialog,
+                                  public views::DialogDelegate,
+                                  public views::ButtonListener {
+ public:
+  explicit MediaGalleriesDialogViews(
+      MediaGalleriesDialogController* controller);
+  virtual ~MediaGalleriesDialogViews();
+
+  // MediaGalleriesDialog implementation:
+  virtual void UpdateGallery(const MediaGalleryPrefInfo* gallery,
+                             bool permitted) OVERRIDE;
+
+  // views::DialogDelegate implementation:
+  virtual string16 GetDialogButtonLabel(ui::DialogButton button) const OVERRIDE;
+  virtual bool IsDialogButtonEnabled(ui::DialogButton button) const OVERRIDE;
+  virtual bool Cancel() OVERRIDE;
+  virtual bool Accept() OVERRIDE;
+
+  // views::WidgetDelegate implementation:

[Peter Kasting, 2012/08/11 23:09:14]

Nit: This class doesn't directly inherit from WidgetDelegate, so group these
with the other functions from the relevant direct override, in the appropriate
order.

[Evan Stade, 2012/08/13 23:10:30]

Done.

+  virtual views::View* GetContentsView() OVERRIDE;
+  virtual views::Widget* GetWidget() OVERRIDE;
+  virtual const views::Widget* GetWidget() const OVERRIDE;
+  virtual void DeleteDelegate() OVERRIDE;
+
+  // views::ButtonListener implementation:
+  virtual void ButtonPressed(
+      views::Button* sender, const ui::Event& event) OVERRIDE;

[Peter Kasting, 2012/08/11 23:09:14]

Nit: First arg on previous line

[Evan Stade, 2012/08/13 23:10:30]

Done.

+
+ private:
+  typedef std::map<const MediaGalleryPrefInfo*, views::Checkbox*> CheckboxMap;
+
+  void InitChildViews();
+
+  // Adds a checkbox or updates an existing checkbox. Returns true if a new one
+  // was added.
+  bool AddOrUpdateGallery(const MediaGalleryPrefInfo* gallery,
+                          bool permitted);
+
+  // Enables the confirm button (see |confirm_available_|).
+  void EnableConfirm();
+
+  MediaGalleriesDialogController* controller_;
+

[Peter Kasting, 2012/08/11 23:09:14]

Nit: No need for blank lines for members without comments.

Though you might want to add comments about ownership.  For example, |window_|
is a raw pointer here but you allocate it on the heap in the constructor.  Who
owns it?

[Evan Stade, 2012/08/13 23:10:30]

I originally did not document that because it is documented in
constrained_window_views.h. It seems like that is the correct place to document
how a class is used. But I have added a note here as well.

[Peter Kasting, 2012/08/14 18:44:01]

I read constrained_window_views.h but it doesn't seem terribly clear.  The
closest it gets to documenting ownership is calling the TabContents* getter
"owner()", but even that doesn't make it clear whether that actually means
ownership in the memory management sense.

If the TabContents really does own the constrained window pointer, perhaps the
comment here can say "The constrained window.  Owned by |controller_|'s
TabContents."  It would be nice if we could drop a note in
constrained_window_views.h to make this clearer too.

+  ConstrainedWindowViews* window_;
+
+  views::View* contents_;
+
+  views::View* checkbox_container_;
+
+  // A map from media gallery to views::Checkbox view.
+  CheckboxMap checkbox_map_;
+
+  views::Button* add_gallery_;
+
+  // This tracks whether the confirm button can be clicked. It starts as false
+  // if no checkboxes are ticked. After there is any interaction, or some
+  // checkboxes start checked, this will be true.
+  bool confirm_available_;
+
+  // True if the user has pressed accept.
+  bool accepted_;
+
+  DISALLOW_COPY_AND_ASSIGN(MediaGalleriesDialogViews);
+};
+
+}  // namespace chrome
+
+#endif  // CHROME_BROWSER_UI_VIEWS_EXTENSIONS_MEDIA_GALLERIES_DIALOG_VIEWS_H_