media galleries config dialog on views

media galleries config dialog on views

BUG=134929
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=151758

[Evan Stade, 2012-08-09 23:52:16]

pkasting: views code
vandebo: rest

[vandebo (ex-Chrome), 2012-08-10 00:35:28]

!views code LGTM

http://codereview.chromium.org/10828166/diff/8001/chrome/browser/media_galler...
File chrome/browser/media_gallery/media_galleries_preferences.cc (left):

http://codereview.chromium.org/10828166/diff/8001/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_preferences.cc:389: if
(!UserInteractionIsEnabled())
Please remove this from media_galleries_pereferences_unittests.cc too

[Evan Stade, 2012-08-10 21:58:13]

http://codereview.chromium.org/10828166/diff/8001/chrome/browser/media_galler...
File chrome/browser/media_gallery/media_galleries_preferences.cc (left):

http://codereview.chromium.org/10828166/diff/8001/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_preferences.cc:389: if
(!UserInteractionIsEnabled())
On 2012/08/10 00:35:28, vandebo wrote:
> Please remove this from media_galleries_pereferences_unittests.cc too

Done.

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc (right):

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:130:
checkbox->SetTooltipText(gallery->path.LossyDisplayName());
Sailesh: note this (I haven't yet done this for Linux but we decided it was a
good idea)

[sail, 2012-08-10 22:09:04]

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc (right):

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:130:
checkbox->SetTooltipText(gallery->path.LossyDisplayName());
On 2012/08/10 21:58:13, Evan Stade wrote:
> Sailesh: note this (I haven't yet done this for Linux but we decided it was a
> good idea)

Sounds good, will add to Cocoa.

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

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc (right):

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:25: //
Heading font size correction.
Nit: Can you say more here?  I have no idea what this is about.

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:37: using
views::GridLayout;
Nit: We normally avoid using statements unless they significantly shorten code.

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:43:
contents_(new views::View()),
Who owns this?

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:48: window_ =
new ConstrainedWindowViews(controller->tab_contents(), this);
Who owns this?

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:97: iter !=
permissions.end(); iter++) {
Nit: Preincrement

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:112: const
MediaGalleryPrefInfo* gallery, bool permitted) {
Nit: One arg per line (2 places)

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:122: bool
new_checkbox = false;
Nit: How about:

  CheckboxMap::iterator iter = checkbox_map_.find(gallery);
  if (iter != checkbox_map_.end()) {
    iter->second->SetChecked(permitted);
    return false;
  }

  views::Checkbox* checkbox = new views::Checkbox(gallery->display_name);
  ...
  return true;

This reads a little more clearly and avoids temps when possible.

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:141: if
(button == ui::DIALOG_BUTTON_OK)
Nit: Most places we just do:

  return l10n_util::GetStringUTF16((button == ui::DIALOG_BUTTON_OK) ?
      IDS_MEDIA_GALLERIES_DIALOG_CONFIRM : IDS_MEDIA_GALLERIES_DIALOG_CANCEL;

...and don't worry about the hypothetical "other button" case.

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:152: return
!(button == ui::DIALOG_BUTTON_OK && !confirm_available_);
Nit: Please factor ! through, as nested !s make my brain hurt

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:191: if
(iter->second == sender) {
Tiny nit: For consistency with conditional above, (sender == iter->second)

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:199: return;
Nit: No need for return here

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:202: void
MediaGalleriesDialogViews::EnableConfirm() {
Nit: Any particular reason to factor this out into its own function?  It's only
called once and is incredibly simple.

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/media_galleries_dialog_views.h (right):

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.h:22: // The
media galleries configuration view for Gtk. It will immediately show
Gtk?

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.h:42: //
views::WidgetDelegate implementation:
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.

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.h:50:
views::Button* sender, const ui::Event& event) OVERRIDE;
Nit: First arg on previous line

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.h:66: 
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]

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc (right):

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:25: //
Heading font size correction.
On 2012/08/11 23:09:14, Peter Kasting wrote:
> Nit: Can you say more here?  I have no idea what this is about.

There are a couple other places which use this pattern and I just copied it. The
documentation for the define states:

// Single define to trigger whether CrOS fonts have BCI on.
// In that case font sizes/deltas should be adjusted.

that's all I know about it.

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:37: using
views::GridLayout;
On 2012/08/11 23:09:14, Peter Kasting wrote:
> Nit: We normally avoid using statements unless they significantly shorten
code.

Done.

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:43:
contents_(new views::View()),
On 2012/08/11 23:09:14, Peter Kasting wrote:
> Who owns this?

its parent View (a RootView)

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:48: window_ =
new ConstrainedWindowViews(controller->tab_contents(), this);
On 2012/08/11 23:09:14, Peter Kasting wrote:
> Who owns this?

the TabContents (first arg)

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:97: iter !=
permissions.end(); iter++) {
On 2012/08/11 23:09:14, Peter Kasting wrote:
> Nit: Preincrement

Done.

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:112: const
MediaGalleryPrefInfo* gallery, bool permitted) {
On 2012/08/11 23:09:14, Peter Kasting wrote:
> Nit: One arg per line (2 places)

Done.

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:122: bool
new_checkbox = false;
On 2012/08/11 23:09:14, Peter Kasting wrote:
> Nit: How about:
> 
>   CheckboxMap::iterator iter = checkbox_map_.find(gallery);
>   if (iter != checkbox_map_.end()) {
>     iter->second->SetChecked(permitted);
>     return false;
>   }
> 
>   views::Checkbox* checkbox = new views::Checkbox(gallery->display_name);
>   ...
>   return true;
> 
> This reads a little more clearly and avoids temps when possible.

Done.

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:141: if
(button == ui::DIALOG_BUTTON_OK)
On 2012/08/11 23:09:14, Peter Kasting wrote:
> Nit: Most places we just do:
> 
>   return l10n_util::GetStringUTF16((button == ui::DIALOG_BUTTON_OK) ?
>       IDS_MEDIA_GALLERIES_DIALOG_CONFIRM : IDS_MEDIA_GALLERIES_DIALOG_CANCEL;
> 
> ...and don't worry about the hypothetical "other button" case.

Done.

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:152: return
!(button == ui::DIALOG_BUTTON_OK && !confirm_available_);
On 2012/08/11 23:09:14, Peter Kasting wrote:
> Nit: Please factor ! through, as nested !s make my brain hurt

Done.

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:191: if
(iter->second == sender) {
On 2012/08/11 23:09:14, Peter Kasting wrote:
> Tiny nit: For consistency with conditional above, (sender == iter->second)

Done.

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:199: return;
On 2012/08/11 23:09:14, Peter Kasting wrote:
> Nit: No need for return here

Done.

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:202: void
MediaGalleriesDialogViews::EnableConfirm() {
On 2012/08/11 23:09:14, Peter Kasting wrote:
> Nit: Any particular reason to factor this out into its own function?  It's
only
> called once and is incredibly simple.

Done. (I originally had it called twice.)

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/media_galleries_dialog_views.h (right):

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.h:22: // The
media galleries configuration view for Gtk. It will immediately show
On 2012/08/11 23:09:14, Peter Kasting wrote:
> Gtk?

Done.

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.h:42: //
views::WidgetDelegate implementation:
On 2012/08/11 23:09:14, Peter Kasting wrote:
> 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.

Done.

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.h:50:
views::Button* sender, const ui::Event& event) OVERRIDE;
On 2012/08/11 23:09:14, Peter Kasting wrote:
> Nit: First arg on previous line

Done.

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.h:66: 
On 2012/08/11 23:09:14, Peter Kasting wrote:
> 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?

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]

LGTM

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc (right):

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:25: //
Heading font size correction.
On 2012/08/13 23:10:30, Evan Stade wrote:
> On 2012/08/11 23:09:14, Peter Kasting wrote:
> > Nit: Can you say more here?  I have no idea what this is about.
> 
> There are a couple other places which use this pattern and I just copied it.
The
> documentation for the define states:
> 
> // Single define to trigger whether CrOS fonts have BCI on.
> // In that case font sizes/deltas should be adjusted.
> 
> that's all I know about it.

I'm following up on this separately with the CrOS guys to see what this is
about.

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/media_galleries_dialog_views.h (right):

http://codereview.chromium.org/10828166/diff/12007/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.h:66: 
On 2012/08/13 23:10:30, Evan Stade wrote:
> On 2012/08/11 23:09:14, Peter Kasting wrote:
> > 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?
> 
> 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.

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.

http://codereview.chromium.org/10828166/diff/9007/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc (right):

http://codereview.chromium.org/10828166/diff/9007/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc:47: window_ =
new ConstrainedWindowViews(controller->tab_contents(), this);
Nit: Perhaps add "// After this call, |window_| and |contents_| are both owned."

http://codereview.chromium.org/10828166/diff/9007/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/media_galleries_dialog_views.h (right):

http://codereview.chromium.org/10828166/diff/9007/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.h:40: virtual
views::View* GetContentsView() OVERRIDE;
Nit: Now that you've moved these declarations, also reorder the definitions to
match.

http://codereview.chromium.org/10828166/diff/9007/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.h:66: // its
parent view.
Nit: its its

It's not actually obvious what the parent of this is.  I think it's owned by
|window_|'s RootView.  Maybe the comment here can say "The contents of the
dialog.  Owned by |window_|'s RootView."?

[Evan Stade, 2012-08-14 21:07:28]

I've made all the changes you suggested including documenting in
ConstrainedWindowViews the ownership model.

http://codereview.chromium.org/10828166/diff/9007/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/media_galleries_dialog_views.h (right):

http://codereview.chromium.org/10828166/diff/9007/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/media_galleries_dialog_views.h:66: // its
parent view.
On 2012/08/14 18:44:01, Peter Kasting wrote:
> Nit: its its

you passed the test!

> 
> It's not actually obvious what the parent of this is.  I think it's owned by
> |window_|'s RootView.  Maybe the comment here can say "The contents of the
> dialog.  Owned by |window_|'s RootView."?

[Evan Stade, 2012-08-15 21:41:49]

gyp change TBR=sky, extensions TBR=asargent