Implement Desktop Media Picker (Mac version) for Desktop Capture API

Implement Desktop Media Picker (Mac version) for the Desktop Capture API

This CL implements DesktopMediaPicker for the Mac used by the Desktop Capture API to present the user with a list of sources (including live thumbnails) to use.

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

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

[dcaiafa, 2013-09-04 09:47:49]

rsesek@: could you please take a look at chrome/browser/ui/cocoa/media_picker/*?
I fished your name from OWNERS and the dir's recent history. Please let me know
if you feel you're not the right person to make this review.

sergeyu@: please look at chrome/browser/media/... and
chrome/browser/extensions/.... I'm not happy with the DesktopMediaPickerModel
changes. This CL is currently failing presubmit checks because the unittest is
including internal webrtc headers. It would be so much cleaner if
DesktopMediaPickerModel was an interface :-p

[Robert Sesek, 2013-09-04 20:23:13]

This looks pretty good. Can you please attach a screen shot of the new UI? Also,
have you reviewed this document, specifically the section regarding adding a new
XIB?

http://www.chromium.org/developers/design-documents/mac-xib-files

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_bridge.h (right):

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_bridge.h:40: 
nit: extra blank line

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_cocoa.h (right):

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_cocoa.h:8: #include
"base/mac/scoped_nsobject.h"
Use #import for ObjC headers

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_cocoa.h:27:
base::scoped_nsobject<DesktopMediaPickerController> controller_;
We usually have controllers owning themselves. How does this get torn down?

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_cocoa.mm (right):

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_cocoa.mm:24:
[controller_ showWindow:NULL];
NULL -> nil (though they are the same thing effectively)

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.h
(right):

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.h:12:
#include "base/mac/scoped_nsobject.h"
#import

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.h:42:
DesktopMediaPicker::DoneCallback done_callback_;
naming: doneCallback_

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm
(right):

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:70: if
(!done_callback_.is_null()) {
I'd flip this to an early return.

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:95:
[self reportResult:content::DesktopMediaID()];
This is being done unconditionally, even after -okPressed: calls -reportResult:
?

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:121:
[okButton_ setEnabled:YES];
You can just setEnabled: with the result of the above boolean expression.

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:132:
DesktopMediaPickerItem* item =
Put this in a scoped_nsobject<>

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:163:
NSImage* image = gfx::SkBitmapToNSImageWithColorSpace(
Use gfx::NSImageFromImageSkia()

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
File
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm
(right):

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:16:
@interface DesktopMediaPickerController(ExposedForTesting)
nit: space before (

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:17:
- (IKImageBrowserView*) sourceBrowser;
nit: no space after )

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:22:
@implementation DesktopMediaPickerController(ExposedForTesting)
nit: space before (

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:96:
virtual void SetUp() {
OVERRIDE

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:112:
virtual void TearDown() {
OVERRIDE

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:118:
if (!callback_called_) {
Is it possible for the loop to be idle with the callback yet to be complete?

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:120:
run_loop.RunUntilIdle();
You can fold this into a one-liner:

base::RunLoop().RunUntilIdle()

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_item.h (right):

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_item.h:10: #include
"base/mac/scoped_nsobject.h"
#import

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_item.h:18:
content::DesktopMediaID sourceId_;
I'd call this sourceID_ since it's DesktopMediaID and you use UID_ below.

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_item.h:36: -
(content::DesktopMediaID)sourceId;
same: sourceID

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_item.mm (right):

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_item.mm:7: #import
<Quartz/Quartz.h>
What's Quartzy in this file?

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_item.mm:44: return
@"IKImageBrowserNSImageRepresentationType";
Is there not a constant for this?

[dcaiafa, 2013-09-04 21:39:35]

Thanks! PTAL.

I did review the document a while ago. I felt that the new UI made the bar when
compared to the existing xibs.

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_bridge.h (right):

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_bridge.h:40: 
On 2013/09/04 20:23:19, rsesek wrote:
> nit: extra blank line

Done.

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_cocoa.h (right):

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_cocoa.h:8: #include
"base/mac/scoped_nsobject.h"
On 2013/09/04 20:23:19, rsesek wrote:
> Use #import for ObjC headers

Done.

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_cocoa.h:27:
base::scoped_nsobject<DesktopMediaPickerController> controller_;
The Desktop Media Picker API will destroy the DesktopMediaPicker when the
callback is received or when the operation is cancelled (e.g. the app/extension
is closed). The latter is the main reason for ownership.

On 2013/09/04 20:23:19, rsesek wrote:
> We usually have controllers owning themselves. How does this get torn down?

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_cocoa.mm (right):

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_cocoa.mm:24:
[controller_ showWindow:NULL];
On 2013/09/04 20:23:19, rsesek wrote:
> NULL -> nil (though they are the same thing effectively)

Done.

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.h
(right):

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.h:12:
#include "base/mac/scoped_nsobject.h"
On 2013/09/04 20:23:19, rsesek wrote:
> #import

Done.

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.h:42:
DesktopMediaPicker::DoneCallback done_callback_;
On 2013/09/04 20:23:19, rsesek wrote:
> naming: doneCallback_

Done.

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm
(right):

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:70: if
(!done_callback_.is_null()) {
On 2013/09/04 20:23:19, rsesek wrote:
> I'd flip this to an early return.

Done.

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:95:
[self reportResult:content::DesktopMediaID()];
|reportResult:| will only report the result once. I added a comment for
clarification.

On 2013/09/04 20:23:19, rsesek wrote:
> This is being done unconditionally, even after -okPressed: calls
-reportResult:
> ?

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:121:
[okButton_ setEnabled:YES];
On 2013/09/04 20:23:19, rsesek wrote:
> You can just setEnabled: with the result of the above boolean expression.

Done.

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:132:
DesktopMediaPickerItem* item =
On 2013/09/04 20:23:19, rsesek wrote:
> Put this in a scoped_nsobject<>

Done.

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:163:
NSImage* image = gfx::SkBitmapToNSImageWithColorSpace(
Awesome! Done.

On 2013/09/04 20:23:19, rsesek wrote:
> Use gfx::NSImageFromImageSkia()

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
File
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm
(right):

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:16:
@interface DesktopMediaPickerController(ExposedForTesting)
On 2013/09/04 20:23:19, rsesek wrote:
> nit: space before (

Done.

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:17:
- (IKImageBrowserView*) sourceBrowser;
On 2013/09/04 20:23:19, rsesek wrote:
> nit: no space after )

Done.

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:22:
@implementation DesktopMediaPickerController(ExposedForTesting)
On 2013/09/04 20:23:19, rsesek wrote:
> nit: space before (

Done.

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:96:
virtual void SetUp() {
On 2013/09/04 20:23:19, rsesek wrote:
> OVERRIDE

Done.

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:112:
virtual void TearDown() {
On 2013/09/04 20:23:19, rsesek wrote:
> OVERRIDE

Done.

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:118:
if (!callback_called_) {
The callback is always posted on the local message loop to be executed
immediately. RunUntilIdle will process all pending/non-delayed tasks.

On 2013/09/04 20:23:19, rsesek wrote:
> Is it possible for the loop to be idle with the callback yet to be complete?

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller_unittest.mm:120:
run_loop.RunUntilIdle();
On 2013/09/04 20:23:19, rsesek wrote:
> You can fold this into a one-liner:
> 
> base::RunLoop().RunUntilIdle()

Done.

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_item.h (right):

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_item.h:10: #include
"base/mac/scoped_nsobject.h"
On 2013/09/04 20:23:19, rsesek wrote:
> #import

Done.

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_item.h:18:
content::DesktopMediaID sourceId_;
On 2013/09/04 20:23:19, rsesek wrote:
> I'd call this sourceID_ since it's DesktopMediaID and you use UID_ below.

Done.

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_item.h:36: -
(content::DesktopMediaID)sourceId;
On 2013/09/04 20:23:19, rsesek wrote:
> same: sourceID

Done.

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_item.mm (right):

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_item.mm:7: #import
<Quartz/Quartz.h>
The IKImageBrowserNSImageRepresentationType constant, no longer used. See the
next comment.

On 2013/09/04 20:23:19, rsesek wrote:
> What's Quartzy in this file?

https://codereview.chromium.org/23944003/diff/3001/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_item.mm:44: return
@"IKImageBrowserNSImageRepresentationType";
Yes. But unfortunately, it requires the Quartz framework while chrome pulls in
only QuartzCore. I didn't want to add Quartz framework dependency just for this.
What do you think?

On 2013/09/04 20:23:19, rsesek wrote:
> Is there not a constant for this?

[dcaiafa, 2013-09-04 22:07:32]

BTW, I attached a screenshot to the tracking bug.

[Robert Sesek, 2013-09-05 15:33:19]

Just some nits.

I'm not convinced that the UI is complex enough to warrant a new XIB -- it's a
total of four subviews, it looks like. I don't feel strongly enough to require
you to change it, though bear in mind that compiled NIBs still add a few K to
the overall binary size and will require a disk access to open. Doing layout in
code will only add a few hundred bytes probably and the code segment will
already be in memory when your UI needs to be shown. (Personally I prefer the
cleanness of XIBs, but on a project this size, we have other concerns that
outweigh that).

LGTM

https://codereview.chromium.org/23944003/diff/17001/chrome/browser/ui/cocoa/m...
File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_bridge.h (right):

https://codereview.chromium.org/23944003/diff/17001/chrome/browser/ui/cocoa/m...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_bridge.h:34:
id<DesktopMediaPickerObserver> observer_;  // weak; owns me
nit: me -> this

https://codereview.chromium.org/23944003/diff/17001/chrome/browser/ui/cocoa/m...
File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_cocoa.mm (right):

https://codereview.chromium.org/23944003/diff/17001/chrome/browser/ui/cocoa/m...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_cocoa.mm:16:
gfx::NativeWindow parent,
nit: alignment

https://codereview.chromium.org/23944003/diff/17001/chrome/browser/ui/cocoa/m...
File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm
(right):

https://codereview.chromium.org/23944003/diff/17001/chrome/browser/ui/cocoa/m...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:10:
#include "chrome/browser/ui/cocoa/media_picker/desktop_media_picker_item.h"
#import

[dcaiafa, 2013-09-06 19:15:44]

I removed the xib, and the contents are now created programatically. PTAL.

https://codereview.chromium.org/23944003/diff/17001/chrome/browser/ui/cocoa/m...
File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_bridge.h (right):

https://codereview.chromium.org/23944003/diff/17001/chrome/browser/ui/cocoa/m...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_bridge.h:34:
id<DesktopMediaPickerObserver> observer_;  // weak; owns me
On 2013/09/05 15:33:19, rsesek wrote:
> nit: me -> this

Done.

https://codereview.chromium.org/23944003/diff/17001/chrome/browser/ui/cocoa/m...
File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_cocoa.mm (right):

https://codereview.chromium.org/23944003/diff/17001/chrome/browser/ui/cocoa/m...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_cocoa.mm:16:
gfx::NativeWindow parent,
On 2013/09/05 15:33:19, rsesek wrote:
> nit: alignment

Done.

https://codereview.chromium.org/23944003/diff/17001/chrome/browser/ui/cocoa/m...
File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm
(right):

https://codereview.chromium.org/23944003/diff/17001/chrome/browser/ui/cocoa/m...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:10:
#include "chrome/browser/ui/cocoa/media_picker/desktop_media_picker_item.h"
On 2013/09/05 15:33:19, rsesek wrote:
> #import

Done.

[Robert Sesek, 2013-09-06 20:08:21]

Nice! Thanks.

https://codereview.chromium.org/23944003/diff/4017/chrome/browser/ui/cocoa/me...
File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.h
(right):

https://codereview.chromium.org/23944003/diff/4017/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.h:26:
IKImageBrowserView* sourceBrowser_;
These need ownership comments. They can either be placed in scoped_nsobject and
hold ownership, or they can just be weak and annotated to be owned by the view
hierarchy.

https://codereview.chromium.org/23944003/diff/4017/chrome/browser/ui/cocoa/me...
File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm
(right):

https://codereview.chromium.org/23944003/diff/4017/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:47: //
Action handlers
nit: full-stop

https://codereview.chromium.org/23944003/diff/4017/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:63:
[[NSWindow alloc] initWithContentRect:NSMakeRect(0, 0, 1, 1)
Use ui::kWindowSizeDeterminedLater

https://codereview.chromium.org/23944003/diff/4017/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:83:
[[FlippedView alloc] initWithFrame:NSMakeRect(0, 0, 1, 1)]);
You can just use NSZeroRect for this, and below.

https://codereview.chromium.org/23944003/diff/4017/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:103:
[[NSClassFromString(@"IKImageBrowserView") alloc]
At this point, it may make sense to pull in Quartz.

https://codereview.chromium.org/23944003/diff/4017/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:126:
NSString* cancelTitle = l10n_util::GetNSString(IDS_CANCEL);
This and |okTitle| can just be inlined into the method expression below.

https://codereview.chromium.org/23944003/diff/4017/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:204:
[GTMUILocalizerAndLayoutTweaker sizeToFitFixedWidthTextField: textField];
nit: no space after :

[dcaiafa, 2013-09-06 20:50:50]

PTAL. Thanks!

https://codereview.chromium.org/23944003/diff/4017/chrome/browser/ui/cocoa/me...
File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.h
(right):

https://codereview.chromium.org/23944003/diff/4017/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.h:26:
IKImageBrowserView* sourceBrowser_;
On 2013/09/06 20:08:21, rsesek wrote:
> These need ownership comments. They can either be placed in scoped_nsobject
and
> hold ownership, or they can just be weak and annotated to be owned by the view
> hierarchy.

Done.

https://codereview.chromium.org/23944003/diff/4017/chrome/browser/ui/cocoa/me...
File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm
(right):

https://codereview.chromium.org/23944003/diff/4017/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:47: //
Action handlers
On 2013/09/06 20:08:21, rsesek wrote:
> nit: full-stop

Done.

https://codereview.chromium.org/23944003/diff/4017/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:63:
[[NSWindow alloc] initWithContentRect:NSMakeRect(0, 0, 1, 1)
On 2013/09/06 20:08:21, rsesek wrote:
> Use ui::kWindowSizeDeterminedLater

Done.

https://codereview.chromium.org/23944003/diff/4017/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:83:
[[FlippedView alloc] initWithFrame:NSMakeRect(0, 0, 1, 1)]);
On 2013/09/06 20:08:21, rsesek wrote:
> You can just use NSZeroRect for this, and below.

Done.

https://codereview.chromium.org/23944003/diff/4017/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:126:
NSString* cancelTitle = l10n_util::GetNSString(IDS_CANCEL);
On 2013/09/06 20:08:21, rsesek wrote:
> This and |okTitle| can just be inlined into the method expression below.

Done.

https://codereview.chromium.org/23944003/diff/4017/chrome/browser/ui/cocoa/me...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:204:
[GTMUILocalizerAndLayoutTweaker sizeToFitFixedWidthTextField: textField];
On 2013/09/06 20:08:21, rsesek wrote:
> nit: no space after :

Done.

[Robert Sesek, 2013-09-06 21:10:12]

https://codereview.chromium.org/23944003/diff/37001/chrome/browser/ui/cocoa/m...
File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_item.mm (right):

https://codereview.chromium.org/23944003/diff/37001/chrome/browser/ui/cocoa/m...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_item.mm:30:
imageVersion_++;
nit: ++imageVersion_ is preferred

https://codereview.chromium.org/23944003/diff/37001/ui/ui.gyp
File ui/ui.gyp (right):

https://codereview.chromium.org/23944003/diff/37001/ui/ui.gyp#newcode1047
ui/ui.gyp:1047: '$(SDKROOT)/System/Library/Frameworks/Quartz.framework',
Why pull this in here instead of in chrome_browser.gypi?

[dcaiafa, 2013-09-06 21:39:17]

PTAL

https://codereview.chromium.org/23944003/diff/37001/chrome/browser/ui/cocoa/m...
File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_item.mm (right):

https://codereview.chromium.org/23944003/diff/37001/chrome/browser/ui/cocoa/m...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_item.mm:30:
imageVersion_++;
On 2013/09/06 21:10:13, rsesek wrote:
> nit: ++imageVersion_ is preferred

Done.

https://codereview.chromium.org/23944003/diff/37001/ui/ui.gyp
File ui/ui.gyp (right):

https://codereview.chromium.org/23944003/diff/37001/ui/ui.gyp#newcode1047
ui/ui.gyp:1047: '$(SDKROOT)/System/Library/Frameworks/Quartz.framework',
On 2013/09/06 21:10:13, rsesek wrote:
> Why pull this in here instead of in chrome_browser.gypi?

Done.

[Robert Sesek, 2013-09-06 21:41:55]

LGTM

[Sergey Ulanov, 2013-09-07 00:26:59]

LGTM. Thanks!

[commit-bot: I haz the power, 2013-09-07 00:28:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcaiafa@chromium.org/23944003/40001

[commit-bot: I haz the power, 2013-09-07 03:26:11]

Change committed as 221874

[commit-bot: I haz the power, 2013-09-09 21:46:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcaiafa@chromium.org/23944003/58001

[commit-bot: I haz the power, 2013-09-10 00:40:46]

Change committed as 222156