[MediaGalleries] Context menu for Mac media galleries dialog.

[MediaGalleries] Context menu for Mac media galleries dialog.

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

[Greg Billock, 2013-10-26 00:59:32]

(Need to merge with https://codereview.chromium.org/37553002/)

This moves the menu model to the controller. Need to get that into the views
code and then base this on that.

[Greg Billock, 2013-10-28 22:45:23]

On 2013/10/26 00:59:32, Greg Billock wrote:
> (Need to merge with https://codereview.chromium.org/37553002/)
> 
> This moves the menu model to the controller. Need to get that into the views
> code and then base this on that.

(done -- now restricted to mac changes and based on 37553002)

[Greg Billock, 2013-10-28 22:46:20]

On 2013/10/28 22:45:23, Greg Billock wrote:
> On 2013/10/26 00:59:32, Greg Billock wrote:
> > (Need to merge with https://codereview.chromium.org/37553002/)
> > 
> > This moves the menu model to the controller. Need to get that into the views
> > code and then base this on that.
> 
> (done -- now restricted to mac changes and based on 37553002)

+vandebo for review
+thakis for cocoa owners. Please look carefully at this context menu change. It
looks simpler to me than other context menus. It works, but I'm not sure I'm
doing everything I need to be doing to make it right.

[vandebo (ex-Chrome), 2013-10-29 15:18:27]

On 2013/10/28 22:46:20, Greg Billock wrote:
> On 2013/10/28 22:45:23, Greg Billock wrote:
> > On 2013/10/26 00:59:32, Greg Billock wrote:
> > > (Need to merge with https://codereview.chromium.org/37553002/)
> > > 
> > > This moves the menu model to the controller. Need to get that into the
views
> > > code and then base this on that.
> > 
> > (done -- now restricted to mac changes and based on 37553002)
> 
> +vandebo for review
> +thakis for cocoa owners. Please look carefully at this context menu change.
It
> looks simpler to me than other context menus. It works, but I'm not sure I'm
> doing everything I need to be doing to make it right.

cocoa plus UI code, I have no idea.

[Greg Billock, 2013-11-04 18:49:02]

On 2013/10/29 15:18:27, vandebo wrote:
> On 2013/10/28 22:46:20, Greg Billock wrote:
> > On 2013/10/28 22:45:23, Greg Billock wrote:
> > > On 2013/10/26 00:59:32, Greg Billock wrote:
> > > > (Need to merge with https://codereview.chromium.org/37553002/)
> > > > 
> > > > This moves the menu model to the controller. Need to get that into the
> views
> > > > code and then base this on that.
> > > 
> > > (done -- now restricted to mac changes and based on 37553002)
> > 
> > +vandebo for review
> > +thakis for cocoa owners. Please look carefully at this context menu change.
> It
> > looks simpler to me than other context menus. It works, but I'm not sure I'm
> > doing everything I need to be doing to make it right.
> 
> cocoa plus UI code, I have no idea.

ping. thakis, can you have a look?

[Greg Billock, 2013-11-05 18:35:55]

avi, do you have time to take a look at this?

[Avi (use Gerrit), 2013-11-05 19:17:33]

https://codereview.chromium.org/46503002/diff/90001/chrome/browser/ui/cocoa/e...
File chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm (right):

https://codereview.chromium.org/46503002/diff/90001/chrome/browser/ui/cocoa/e...
chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm:65:
@property(nonatomic, assign) MediaGalleryPrefId prefid;
prefid? I'd prefer prefID, or at least prefId.

And if these are never set except during construction, then just make your
intializer:

-initWithFrame:dialog:prefID:

and drop the properties.

[Greg Billock, 2013-11-06 18:28:25]

https://codereview.chromium.org/46503002/diff/90001/chrome/browser/ui/cocoa/e...
File chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm (right):

https://codereview.chromium.org/46503002/diff/90001/chrome/browser/ui/cocoa/e...
chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm:65:
@property(nonatomic, assign) MediaGalleryPrefId prefid;
On 2013/11/05 19:17:33, Avi wrote:
> prefid? I'd prefer prefID, or at least prefId.
> 
> And if these are never set except during construction, then just make your
> intializer:
> 
> -initWithFrame:dialog:prefID:
> 
> and drop the properties.

That sounds good. Will do.

[Avi (use Gerrit), 2013-11-06 18:49:25]

https://codereview.chromium.org/46503002/diff/150001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm (right):

https://codereview.chromium.org/46503002/diff/150001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm:76: prefId_ =
prefId;
These assignments go in the braces (otherwise you're accessing a nil |self|).

[Greg Billock, 2013-11-06 21:59:23]

https://codereview.chromium.org/46503002/diff/150001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm (right):

https://codereview.chromium.org/46503002/diff/150001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm:76: prefId_ =
prefId;
On 2013/11/06 18:49:26, Avi wrote:
> These assignments go in the braces (otherwise you're accessing a nil |self|).

Done.

[Avi (use Gerrit), 2013-11-06 22:01:51]

LGTM with the noted change.

https://codereview.chromium.org/46503002/diff/200001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm (right):

https://codereview.chromium.org/46503002/diff/200001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm:64: -
(NSMenu*)menuForEvent:(NSEvent*)theEvent;
Now that you have a custom initializer you need to declare it up here.

[Greg Billock, 2013-11-06 22:30:17]

Thanks!

https://codereview.chromium.org/46503002/diff/200001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm (right):

https://codereview.chromium.org/46503002/diff/200001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/media_galleries_dialog_cocoa.mm:64: -
(NSMenu*)menuForEvent:(NSEvent*)theEvent;
On 2013/11/06 22:01:51, Avi wrote:
> Now that you have a custom initializer you need to declare it up here.

Done.

[commit-bot: I haz the power, 2013-11-06 22:46:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/46503002/270001

[commit-bot: I haz the power, 2013-11-07 06:05:57]

Change committed as 233496