Media galleries: configuration dialog controller and GTK impl.

Media galleries: configuration dialog controller and GTK impl.

BUG=134929
TEST=manual

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

[Evan Stade, 2012-08-02 23:18:13]

this change depends on http://codereview.chromium.org/10821077/

erg: gtk
vandebo: the rest
sail: fyi

http://codereview.chromium.org/10826129/diff/1/chrome/browser/media_gallery/m...
File chrome/browser/media_gallery/media_galleries_preferences.cc (right):

http://codereview.chromium.org/10826129/diff/1/chrome/browser/media_gallery/m...
chrome/browser/media_gallery/media_galleries_preferences.cc:50: if
(!dict->GetString(kMediaGalleriesTypeKey, &string_type))
ignore changes in this file.

[Evan Stade, 2012-08-02 23:31:05]

and there's a screenshot on the bug now.

[Elliot Glaysher, 2012-08-02 23:33:14]

gtk lgtm

[sail, 2012-08-03 23:29:13]

Would it be possible to add a test for MediaGalleriesDialogGtk? (That way I can
copy paste it for the Cocoa version).

[Evan Stade, 2012-08-04 00:40:26]

yes, I had thought about adding a browser test, but would rather do so in a
separate CL.

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

http://codereview.chromium.org/10826129/diff/5001/chrome/browser/media_galler...
File chrome/browser/media_gallery/media_galleries_dialog_controller.cc (right):

http://codereview.chromium.org/10826129/diff/5001/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_dialog_controller.cc:79: // Check
new galleries.
You could locally assign ids to new galleries, counting down from the top of the
number space and therefore unify some of the management.

http://codereview.chromium.org/10826129/diff/5001/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_dialog_controller.cc:137:
preferences_ =
Move this bit to the constructor?  Here it's not obvious that this will be
called before the other consumers of preferences_.

http://codereview.chromium.org/10826129/diff/5001/chrome/browser/media_galler...
File chrome/browser/media_gallery/media_galleries_dialog_controller.h (right):

http://codereview.chromium.org/10826129/diff/5001/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_dialog_controller.h:33: virtual
void UpdateGallery(const MediaGalleryPrefInfo* gallery,
const ref?

http://codereview.chromium.org/10826129/diff/5001/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_dialog_controller.h:56: const
extensions::Extension* extension,
const ref?

http://codereview.chromium.org/10826129/diff/5001/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_dialog_controller.h:72: typedef
std::map<MediaGalleryPrefId, std::pair<MediaGalleryPrefInfo, bool> >
Any advantage to using a pair instead of an explicit struct?  Especially with
map iterators, explicit structs tend to be much easier to read.

http://codereview.chromium.org/10826129/diff/5001/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_dialog_controller.h:95: typedef
std::list<std::pair<MediaGalleryPrefInfo, bool> >
The struct mentioned above can be reused here.
Any reason to use list vs vector?

http://codereview.chromium.org/10826129/diff/5001/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_dialog_controller.h:103: const
extensions::Extension* extension_;
Why not actually make it const ref?

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

so that I don't have to keep syncing and fixing conflicts I'm going to stop
working on this until Steve's change has landed. These comments apply to a
version I haven't uploaded yet.

http://codereview.chromium.org/10826129/diff/5001/chrome/browser/media_galler...
File chrome/browser/media_gallery/media_galleries_dialog_controller.cc (right):

http://codereview.chromium.org/10826129/diff/5001/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_dialog_controller.cc:79: // Check
new galleries.
On 2012/08/06 20:39:30, vandebo wrote:
> You could locally assign ids to new galleries, counting down from the top of
the
> number space and therefore unify some of the management.

I don't think that would help very much, especially when it comes to
SavePermissions. It might save LOC but at the expense of understandability.

http://codereview.chromium.org/10826129/diff/5001/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_dialog_controller.cc:137:
preferences_ =
On 2012/08/06 20:39:30, vandebo wrote:
> Move this bit to the constructor?  Here it's not obvious that this will be
> called before the other consumers of preferences_.

Done.

http://codereview.chromium.org/10826129/diff/5001/chrome/browser/media_galler...
File chrome/browser/media_gallery/media_galleries_dialog_controller.h (right):

http://codereview.chromium.org/10826129/diff/5001/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_dialog_controller.h:33: virtual
void UpdateGallery(const MediaGalleryPrefInfo* gallery,
On 2012/08/06 20:39:30, vandebo wrote:
> const ref?

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

http://codereview.chromium.org/10826129/diff/5001/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_dialog_controller.h:56: const
extensions::Extension* extension,
On 2012/08/06 20:39:30, vandebo wrote:
> const ref?

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.

http://codereview.chromium.org/10826129/diff/5001/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_dialog_controller.h:72: typedef
std::map<MediaGalleryPrefId, std::pair<MediaGalleryPrefInfo, bool> >
On 2012/08/06 20:39:30, vandebo wrote:
> Any advantage to using a pair instead of an explicit struct?  Especially with
> map iterators, explicit structs tend to be much easier to read.

Done.

http://codereview.chromium.org/10826129/diff/5001/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_dialog_controller.h:95: typedef
std::list<std::pair<MediaGalleryPrefInfo, bool> >
On 2012/08/06 20:39:30, vandebo wrote:
> The struct mentioned above can be reused here.
> Any reason to use list vs vector?

don't need random access; desire cheap append

http://codereview.chromium.org/10826129/diff/5001/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_dialog_controller.h:103: const
extensions::Extension* extension_;
On 2012/08/06 20:39:30, vandebo wrote:
> Why not actually make it const ref?

see above

[Evan Stade, 2012-08-07 04:29:04]

patch updated

[vandebo (ex-Chrome), 2012-08-08 19:24:21]

LGTM

http://codereview.chromium.org/10826129/diff/5001/chrome/browser/media_galler...
File chrome/browser/media_gallery/media_galleries_dialog_controller.cc (right):

http://codereview.chromium.org/10826129/diff/5001/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_dialog_controller.cc:79: // Check
new galleries.
On 2012/08/06 23:43:02, Evan Stade wrote:
> On 2012/08/06 20:39:30, vandebo wrote:
> > You could locally assign ids to new galleries, counting down from the top of
> the
> > number space and therefore unify some of the management.
> 
> I don't think that would help very much, especially when it comes to
> SavePermissions. It might save LOC but at the expense of understandability.

On second thought, fragmenting the id space is probably too fragile....

You are free to ignore the comment, but I don't think it would hurt readability:

bool GalleryExists(const MediaGalleryPrefInfo& gallery) {
  return gallery.pref_id > 0;  // Use signed id.
}

void MediaGalleriesDialogController::SavePermissions() {
  for (KnownGalleryPermissions::iterator iter = known_galleries_.begin();
       iter != known_galleries_.end(); ++iter) {
    MediaGalleriesPrefInfo gallery = iter->second.gallery;
    bool has_access = iter->second.permission;
    if (!GalleryExists(gallery)) {
      // If the user added a gallery then unchecked it, forget about it.
      if (!has_access)
        continue;
      gallery.pref_id = preferences_->AddGallery(
         gallery->device_id, gallery->display_name, gallery->path, true);
    }
    preferences_->SetGalleryPermissionForExtension(
        *extension_, gallery.pref_id, has_access);
  }
}

http://codereview.chromium.org/10826129/diff/2005/chrome/browser/extensions/a...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(right):

http://codereview.chromium.org/10826129/diff/2005/chrome/browser/extensions/a...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:56: } else
if (interactive == "if_needed") {
nit: fall through to no for now.

http://codereview.chromium.org/10826129/diff/2005/chrome/browser/media_galler...
File chrome/browser/media_gallery/media_galleries_dialog_controller.h (right):

http://codereview.chromium.org/10826129/diff/2005/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_dialog_controller.h:43: class
MediaGalleriesDialogDelegate {
Is there a reason to use a delegate instead of a callback?

http://codereview.chromium.org/10826129/diff/2005/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_dialog_controller.h:63: void
GalleryToggled(const MediaGalleryPrefInfo*, bool enabled);
This can be const ref

http://codereview.chromium.org/10826129/diff/2005/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_dialog_controller.h:71: struct
GalleryPermission {
Should go above the constructor (implied by style guide).

http://codereview.chromium.org/10826129/diff/2005/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_dialog_controller.h:81: typedef
std::map<MediaGalleryPrefId, GalleryPermission>
typedefs should be above constructors

http://codereview.chromium.org/10826129/diff/2005/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_dialog_controller.h:104: typedef
std::list<GalleryPermission> NewGalleryPermissions;
above destructor

http://codereview.chromium.org/10826129/diff/2005/chrome/browser/ui/gtk/exten...
File chrome/browser/ui/gtk/extensions/media_galleries_dialog_gtk.h (right):

http://codereview.chromium.org/10826129/diff/2005/chrome/browser/ui/gtk/exten...
chrome/browser/ui/gtk/extensions/media_galleries_dialog_gtk.h:63: typedef
std::map<const MediaGalleryPrefInfo*, GtkWidget*> CheckboxMap;
typedef should be just after private

http://codereview.chromium.org/10826129/diff/2005/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/cross_origin_xhr/file_access/test.html
(left):

http://codereview.chromium.org/10826129/diff/2005/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/cross_origin_xhr/file_access/test.html:3: *
source code is governed by a BSD-style license that can be found in the
Did you mean to delete this file?

[Evan Stade, 2012-08-08 21:48:57]

http://codereview.chromium.org/10826129/diff/5001/chrome/browser/media_galler...
File chrome/browser/media_gallery/media_galleries_dialog_controller.cc (right):

http://codereview.chromium.org/10826129/diff/5001/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_dialog_controller.cc:79: // Check
new galleries.
On 2012/08/08 19:24:21, vandebo wrote:
> On 2012/08/06 23:43:02, Evan Stade wrote:
> > On 2012/08/06 20:39:30, vandebo wrote:
> > > You could locally assign ids to new galleries, counting down from the top
of
> > the
> > > number space and therefore unify some of the management.
> > 
> > I don't think that would help very much, especially when it comes to
> > SavePermissions. It might save LOC but at the expense of understandability.
> 
> On second thought, fragmenting the id space is probably too fragile....
> 
> You are free to ignore the comment, but I don't think it would hurt
readability:
> 
> bool GalleryExists(const MediaGalleryPrefInfo& gallery) {
>   return gallery.pref_id > 0;  // Use signed id.
> }
> 
> void MediaGalleriesDialogController::SavePermissions() {
>   for (KnownGalleryPermissions::iterator iter = known_galleries_.begin();
>        iter != known_galleries_.end(); ++iter) {
>     MediaGalleriesPrefInfo gallery = iter->second.gallery;
>     bool has_access = iter->second.permission;
>     if (!GalleryExists(gallery)) {
>       // If the user added a gallery then unchecked it, forget about it.
>       if (!has_access)
>         continue;
>       gallery.pref_id = preferences_->AddGallery(
>          gallery->device_id, gallery->display_name, gallery->path, true);
>     }
>     preferences_->SetGalleryPermissionForExtension(
>         *extension_, gallery.pref_id, has_access);
>   }
> }

I'm not sure how that would work. known_gallery_ is a map from ID to
GalleryPermission, but all new galleries have the same (invalid) ID.

http://codereview.chromium.org/10826129/diff/2005/chrome/browser/media_galler...
File chrome/browser/media_gallery/media_galleries_dialog_controller.h (right):

http://codereview.chromium.org/10826129/diff/2005/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_dialog_controller.h:43: class
MediaGalleriesDialogDelegate {
On 2012/08/08 19:24:21, vandebo wrote:
> Is there a reason to use a delegate instead of a callback?

Done.

http://codereview.chromium.org/10826129/diff/2005/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_dialog_controller.h:63: void
GalleryToggled(const MediaGalleryPrefInfo*, bool enabled);
On 2012/08/08 19:24:21, vandebo wrote:
> This can be const ref

same case as UpdateGallery

http://codereview.chromium.org/10826129/diff/2005/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_dialog_controller.h:71: struct
GalleryPermission {
On 2012/08/08 19:24:21, vandebo wrote:
> Should go above the constructor (implied by style guide).

Done.

http://codereview.chromium.org/10826129/diff/2005/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_dialog_controller.h:81: typedef
std::map<MediaGalleryPrefId, GalleryPermission>
On 2012/08/08 19:24:21, vandebo wrote:
> typedefs should be above constructors

Done.

http://codereview.chromium.org/10826129/diff/2005/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_dialog_controller.h:104: typedef
std::list<GalleryPermission> NewGalleryPermissions;
On 2012/08/08 19:24:21, vandebo wrote:
> above destructor

Done.

http://codereview.chromium.org/10826129/diff/2005/chrome/browser/ui/gtk/exten...
File chrome/browser/ui/gtk/extensions/media_galleries_dialog_gtk.h (right):

http://codereview.chromium.org/10826129/diff/2005/chrome/browser/ui/gtk/exten...
chrome/browser/ui/gtk/extensions/media_galleries_dialog_gtk.h:63: typedef
std::map<const MediaGalleryPrefInfo*, GtkWidget*> CheckboxMap;
On 2012/08/08 19:24:21, vandebo wrote:
> typedef should be just after private

Done.

http://codereview.chromium.org/10826129/diff/2005/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/cross_origin_xhr/file_access/test.html
(left):

http://codereview.chromium.org/10826129/diff/2005/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/cross_origin_xhr/file_access/test.html:3: *
source code is governed by a BSD-style license that can be found in the
On 2012/08/08 19:24:21, vandebo wrote:
> Did you mean to delete this file?

not sure why this happened.

[vandebo (ex-Chrome), 2012-08-09 01:07:43]

modulo clang errors, LGTM