Cocoa: Implement media gallery dialog

Cocoa: Implement media gallery dialog

This is the cocoa version of the media galleries permission dialog. The dialog is implemented as an NSAlert displayed as a constrained window. The gallery checkboxes are added inside a accessory view.

The views and GTK versions are here:
http://codereview.chromium.org/10828166/
https://chromiumcodereview.appspot.com/10826129

Screenshots: http://i.imgur.com/fedm0.png

BUG=134929
TBR=mihaip@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152128

[Nico, 2012-08-13 19:30:30]

I don't understand the dialog, and have no idea what any of the buttons would
do. Does that grant chrome access to my iphoto stuff (likely not, afaik that
doesn't require permissions, but that's how it sounds like)? Something else?

[sail, 2012-08-13 19:37:21]

On 2012/08/13 19:30:30, Nico wrote:
> I don't understand the dialog, and have no idea what any of the buttons would
> do. Does that grant chrome access to my iphoto stuff (likely not, afaik that
> doesn't require permissions, but that's how it sounds like)? Something else?

From what I understood this allows extensions to access media from these
folders. Normally they wouldn't have access to those folders. Evan would know
more.

[sail, 2012-08-13 19:37:22]

On 2012/08/13 19:30:30, Nico wrote:
> I don't understand the dialog, and have no idea what any of the buttons would
> do. Does that grant chrome access to my iphoto stuff (likely not, afaik that
> doesn't require permissions, but that's how it sounds like)? Something else?

From what I understood this allows extensions to access media from these
folders. Normally they wouldn't have access to those folders. Evan would know
more.

[Nico, 2012-08-13 19:40:24]

The UI seems pretty weird to me. The code is fine, except that it lacks a test.

http://codereview.chromium.org/10834242/diff/2002/chrome/browser/ui/cocoa/ext...
File chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm (right):

http://codereview.chromium.org/10834242/diff/2002/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm:13: namespace
{
"const has implicit internal linkage, no need for namespace"

http://codereview.chromium.org/10834242/diff/2002/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm:89:
has_permitted_galleries = has_permitted_galleries || permission.allowed;
The controller should have a function HasPermittedGalleries that does this loop
and computes has_permitted_galleries. As is, it looks like this logic is in
every view implementation.

(You still need the loop to build checkboxes; that's fine)

http://codereview.chromium.org/10834242/diff/2002/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm:121:
controller_->GalleryToggled(gallery, [checkbox state] == NSOnState);
DidToggleGallery() is chromier naming

http://codereview.chromium.org/10834242/diff/2002/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm:220:
[cocoa_bridge setDialog:dialog.get()];
Could the constructor do this?

[sail, 2012-08-13 20:02:25]

On 2012/08/13 19:40:24, Nico wrote:
> The UI seems pretty weird to me. The code is fine, except that it lacks a
test.

Thanks for the speedy review. Estade is working on the GTK/Views tests:
https://chromiumcodereview.appspot.com/10826129#msg5
I'm planning on porting it when he's done.

>
http://codereview.chromium.org/10834242/diff/2002/chrome/browser/ui/cocoa/ext...
> File chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm
(right):
> 
>
http://codereview.chromium.org/10834242/diff/2002/chrome/browser/ui/cocoa/ext...
> chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm:13:
namespace
> {
> "const has implicit internal linkage, no need for namespace"
> 
>
http://codereview.chromium.org/10834242/diff/2002/chrome/browser/ui/cocoa/ext...
> chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm:89:
> has_permitted_galleries = has_permitted_galleries || permission.allowed;
> The controller should have a function HasPermittedGalleries that does this
loop
> and computes has_permitted_galleries. As is, it looks like this logic is in
> every view implementation.
> 
> (You still need the loop to build checkboxes; that's fine)
> 
>
http://codereview.chromium.org/10834242/diff/2002/chrome/browser/ui/cocoa/ext...
> chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm:121:
> controller_->GalleryToggled(gallery, [checkbox state] == NSOnState);
> DidToggleGallery() is chromier naming
> 
>
http://codereview.chromium.org/10834242/diff/2002/chrome/browser/ui/cocoa/ext...
> chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm:220:
> [cocoa_bridge setDialog:dialog.get()];
> Could the constructor do this?

[sail, 2012-08-13 20:40:33]

http://codereview.chromium.org/10834242/diff/2002/chrome/browser/ui/cocoa/ext...
File chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm (right):

http://codereview.chromium.org/10834242/diff/2002/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm:13: namespace
{
On 2012/08/13 19:40:24, Nico wrote:
> "const has implicit internal linkage, no need for namespace"

Done.

http://codereview.chromium.org/10834242/diff/2002/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm:89:
has_permitted_galleries = has_permitted_galleries || permission.allowed;
On 2012/08/13 19:40:24, Nico wrote:
> The controller should have a function HasPermittedGalleries that does this
loop
> and computes has_permitted_galleries. As is, it looks like this logic is in
> every view implementation.
> 
> (You still need the loop to build checkboxes; that's fine)

Done.
Will updated the views implementation in this patch (once it lands).

http://codereview.chromium.org/10834242/diff/2002/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm:121:
controller_->GalleryToggled(gallery, [checkbox state] == NSOnState);
On 2012/08/13 19:40:24, Nico wrote:
> DidToggleGallery() is chromier naming

Done. 
Once the views code lands I'll update this patch to include the renaming.

http://codereview.chromium.org/10834242/diff/2002/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm:220:
[cocoa_bridge setDialog:dialog.get()];
On 2012/08/13 19:40:24, Nico wrote:
> Could the constructor do this?

Done.

[Nico, 2012-08-13 21:38:54]

Can you ping this CL once the ported tests are in, to make sure I don't forget
about it?

[sail, 2012-08-13 21:47:19]

On 2012/08/13 21:38:54, Nico wrote:
> Can you ping this CL once the ported tests are in, to make sure I don't forget
> about it?

Will do!

[sail, 2012-08-14 21:02:44]

On 2012/08/13 21:47:19, sail wrote:
> On 2012/08/13 21:38:54, Nico wrote:
> > Can you ping this CL once the ported tests are in, to make sure I don't
forget
> > about it?
> 
> Will do!

Ping! https://chromiumcodereview.appspot.com/10854150

[Robert Sesek, 2012-08-14 21:07:45]

Are there any Cocoa-specific tests, or do the cross-platform ones work for this,
too?

http://codereview.chromium.org/10834242/diff/8004/chrome/browser/ui/cocoa/ext...
File chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.h (right):

http://codereview.chromium.org/10834242/diff/8004/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.h:15: class
MediaGalleriesDialogCocoa :
Class-level comment.

http://codereview.chromium.org/10834242/diff/8004/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.h:24: void
OnAddFolderClicked();
Document these methods, too.

http://codereview.chromium.org/10834242/diff/8004/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.h:41: // Weak
pointer to the controller.
;  // weak should be sufficient

http://codereview.chromium.org/10834242/diff/8004/chrome/browser/ui/cocoa/ext...
File chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm (right):

http://codereview.chromium.org/10834242/diff/8004/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm:7: #import
<Cocoa/Cocoa.h>
Hm. You use NSAlert and NSView in the .h but do not forward declare them or
#import Cocoa

http://codereview.chromium.org/10834242/diff/8004/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm:16:
@interface MediaGalleriesCocoaBridge : NSObject {
@private

http://codereview.chromium.org/10834242/diff/8004/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm:24:
@implementation MediaGalleriesCocoaBridge
naming: bridges are usually thin C++ classes to bridge to ObjC. Call this the
CocoaController.

[sail, 2012-08-14 21:38:47]

https://chromiumcodereview.appspot.com/10834242/diff/8004/chrome/browser/ui/c...
File chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.h (right):

https://chromiumcodereview.appspot.com/10834242/diff/8004/chrome/browser/ui/c...
chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.h:15: class
MediaGalleriesDialogCocoa :
On 2012/08/14 21:07:45, rsesek wrote:
> Class-level comment.

Done.

https://chromiumcodereview.appspot.com/10834242/diff/8004/chrome/browser/ui/c...
chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.h:41: // Weak
pointer to the controller.
On 2012/08/14 21:07:45, rsesek wrote:
> ;  // weak should be sufficient

Done.

https://chromiumcodereview.appspot.com/10834242/diff/8004/chrome/browser/ui/c...
File chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm (right):

https://chromiumcodereview.appspot.com/10834242/diff/8004/chrome/browser/ui/c...
chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm:7: #import
<Cocoa/Cocoa.h>
On 2012/08/14 21:07:45, rsesek wrote:
> Hm. You use NSAlert and NSView in the .h but do not forward declare them or
> #import Cocoa

Done.

https://chromiumcodereview.appspot.com/10834242/diff/8004/chrome/browser/ui/c...
chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm:16:
@interface MediaGalleriesCocoaBridge : NSObject {
On 2012/08/14 21:07:45, rsesek wrote:
> @private

Done.

https://chromiumcodereview.appspot.com/10834242/diff/8004/chrome/browser/ui/c...
chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm:24:
@implementation MediaGalleriesCocoaBridge
On 2012/08/14 21:07:45, rsesek wrote:
> naming: bridges are usually thin C++ classes to bridge to ObjC. Call this the
> CocoaController.

Done.

[sail, 2012-08-14 21:39:29]

On 2012/08/14 21:07:45, rsesek wrote:
> Are there any Cocoa-specific tests, or do the cross-platform ones work for
this,
> too?

The Cocoa unit tests are here:
https://chromiumcodereview.appspot.com/10854150/

[Evan Stade, 2012-08-15 00:42:11]

On 2012/08/13 19:30:30, Nico wrote:
> I don't understand the dialog, and have no idea what any of the buttons would
> do. Does that grant chrome access to my iphoto stuff (likely not, afaik that
> doesn't require permissions, but that's how it sounds like)? Something else?

this is configuration UI for an experimental extension API which is currently
limited to platform apps called MediaGalleries. The UI pops up and allows users
to select from currently known galleries or add new galleries, then grant (or
deny) permission to the galleries for the calling app. This is a way for apps to
get at user media without having full file system access.

[Evan Stade, 2012-08-15 00:42:55]

> platform apps called MediaGalleries.

"called MediaGalleries" was intended to modify "an experimental extension API"

[Robert Sesek, 2012-08-16 15:43:54]

cocoa/ LGTM

https://chromiumcodereview.appspot.com/10834242/diff/2005/chrome/browser/ui/c...
File chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm (right):

https://chromiumcodereview.appspot.com/10834242/diff/2005/chrome/browser/ui/c...
chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm:19:
@property(nonatomic, readwrite) chrome::MediaGalleriesDialogCocoa* dialog;
readwrite -> assign

[sail, 2012-08-16 16:04:00]

https://chromiumcodereview.appspot.com/10834242/diff/2005/chrome/browser/ui/c...
File chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm (right):

https://chromiumcodereview.appspot.com/10834242/diff/2005/chrome/browser/ui/c...
chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm:19:
@property(nonatomic, readwrite) chrome::MediaGalleriesDialogCocoa* dialog;
On 2012/08/16 15:43:54, rsesek wrote:
> readwrite -> assign

Done.

[commit-bot: I haz the power, 2012-08-16 16:04:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/10834242/11004

[commit-bot: I haz the power, 2012-08-16 16:04:32]

Failed to apply patch for
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:
While running patch -p1 --forward --force;
patching file
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
Hunk #1 FAILED at 23.
Hunk #2 FAILED at 112.
2 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc.rej

[commit-bot: I haz the power, 2012-08-16 23:11:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/10834242/3027

[commit-bot: I haz the power, 2012-08-16 23:11:28]

Presubmit check for 10834242-3027 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome
    chrome/browser/media_gallery
    chrome/browser/extensions
    chrome/browser/ui/gtk

Presubmit checks took 1.6s to calculate.

[Evan Stade, 2012-08-16 23:24:20]

lgtm

[commit-bot: I haz the power, 2012-08-16 23:41:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/10834242/3027

[commit-bot: I haz the power, 2012-08-16 23:41:25]

Presubmit check for 10834242-3027 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome
    chrome/browser/extensions

Presubmit checks took 1.5s to calculate.

[Nico, 2012-08-16 23:48:02]

lgtm

[commit-bot: I haz the power, 2012-08-16 23:50:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/10834242/3027

[commit-bot: I haz the power, 2012-08-17 00:25:35]

Try job failure for 10834242-3027 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-08-17 00:39:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/10834242/9016

[commit-bot: I haz the power, 2012-08-17 01:18:31]

Try job failure for 10834242-9016 (retry) on linux_clang for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2012-08-17 03:29:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/10834242/15004

[commit-bot: I haz the power, 2012-08-17 04:03:30]

Try job failure for 10834242-15004 (retry) on mac for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...

[commit-bot: I haz the power, 2012-08-17 04:15:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/10834242/7007

[commit-bot: I haz the power, 2012-08-17 07:57:34]

Try job failure for 10834242-7007 (retry) on linux_chromeos for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-08-17 08:06:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/10834242/7007

[commit-bot: I haz the power, 2012-08-17 16:22:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/10834242/7007

[commit-bot: I haz the power, 2012-08-17 19:17:18]

Change committed as 152128