Mac Picker UI: Screen Auto Selection

Mac Picker UI: Screen Auto Selection

This CL improves usability of Desktop Capture Picker on Mac
a little.

1. we auto select the first screen source,
2. we omit the label "Entire Screen" in the case there is only one screen, as this is redundant.

BUG=580116, 630172
Committed: https://crrev.com/8ddd74f352bda12bbad52aebb20fe9b6126eae1f
Cr-Commit-Position: refs/heads/master@{#407490}

[qiangchen, 2016-07-21 21:17:23]

Description was changed from

==========
Mac Picker UI: Screen Auto Selection

BUG=
==========

to

==========
Mac Picker UI: Screen Auto Selection

This CL improves usability of Desktop Capture Picker on Mac 
a little.

1. we auto select the first screen source, 
2. we omit the label "Entire Screen" in the case there is only one screen, as
this is redundant.

BUG=580116,630172
==========

[qiangchen, 2016-07-21 21:17:24]

qiangchen@chromium.org changed reviewers:
+ tapted@chromium.org

[qiangchen, 2016-07-21 21:28:52]

Can you take a look? 

Thanks.

[qiangchen, 2016-07-21 22:48:22]

The CQ bit was checked by qiangchen@chromium.org

[qiangchen, 2016-07-21 22:48:25]

The CQ bit was unchecked by qiangchen@chromium.org

[qiangchen, 2016-07-21 22:48:31]

The CQ bit was checked by qiangchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-21 22:49:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-21 23:36:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-21 23:36:21]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tapted, 2016-07-22 03:43:37]

looks like there's a failing test too

https://codereview.chromium.org/2168853003/diff/1/chrome/browser/ui/cocoa/med...
File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm
(right):

https://codereview.chromium.org/2168853003/diff/1/chrome/browser/ui/cocoa/med...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:600: id
item = [items objectAtIndex:index];
id -> DesktopMediaPickerItem* (it's usually best to avoid duck-typing with `id`
when possible)

https://codereview.chromium.org/2168853003/diff/1/chrome/browser/ui/cocoa/med...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:601:
[item setTitleHidden:browser == screenBrowser_ && [items count] == 1];
this should have a comment - it's pretty subtle

https://codereview.chromium.org/2168853003/diff/1/chrome/browser/ui/cocoa/med...
File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_item.h (right):

https://codereview.chromium.org/2168853003/diff/1/chrome/browser/ui/cocoa/med...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_item.h:23: bool
hiddenTitle_;
bool -> BOOL  (and hiddenTitle_ -> titleHidden_)

https://codereview.chromium.org/2168853003/diff/1/chrome/browser/ui/cocoa/med...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_item.h:46: -
(void)setTitleHidden:(bool)hidden;
for simply types we usually just use properties. In the header (before the
initializer above)

@property(assign, nonatomic) BOOL titleHidden;


then in the .mm (also just before the initializer)

@synthesize titleHidden = titleHidden_;

you don't need an implementaiton of - (void)setTitleHidden:

https://codereview.chromium.org/2168853003/diff/1/chrome/browser/ui/cocoa/med...
File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_item.mm (right):

https://codereview.chromium.org/2168853003/diff/1/chrome/browser/ui/cocoa/med...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_item.mm:21:
hiddenTitle_ = false;
ObjC objects are memset to 0s, so this isn't needed (see go/objcguide)

https://codereview.chromium.org/2168853003/diff/1/chrome/browser/ui/cocoa/med...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_item.mm:53: if
(hiddenTitle_)
nit: return titleHidden_ ? nil : imageTitle_;

(the .get shouldn't be needed)

[qiangchen, 2016-07-22 16:35:55]

Patchset #2 (id:20001) has been deleted

[qiangchen, 2016-07-22 18:33:10]

The CQ bit was checked by qiangchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 18:33:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[qiangchen, 2016-07-22 18:33:57]

Fixed the test, and added a test case.

Fixed by comments

Thanks

https://codereview.chromium.org/2168853003/diff/1/chrome/browser/ui/cocoa/med...
File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm
(right):

https://codereview.chromium.org/2168853003/diff/1/chrome/browser/ui/cocoa/med...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:600: id
item = [items objectAtIndex:index];
On 2016/07/22 03:43:36, tapted wrote:
> id -> DesktopMediaPickerItem* (it's usually best to avoid duck-typing with
`id`
> when possible)

Done.

https://codereview.chromium.org/2168853003/diff/1/chrome/browser/ui/cocoa/med...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:601:
[item setTitleHidden:browser == screenBrowser_ && [items count] == 1];
On 2016/07/22 03:43:36, tapted wrote:
> this should have a comment - it's pretty subtle

Done.

https://codereview.chromium.org/2168853003/diff/1/chrome/browser/ui/cocoa/med...
File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_item.h (right):

https://codereview.chromium.org/2168853003/diff/1/chrome/browser/ui/cocoa/med...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_item.h:23: bool
hiddenTitle_;
On 2016/07/22 03:43:36, tapted wrote:
> bool -> BOOL  (and hiddenTitle_ -> titleHidden_)

Done.

https://codereview.chromium.org/2168853003/diff/1/chrome/browser/ui/cocoa/med...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_item.h:46: -
(void)setTitleHidden:(bool)hidden;
On 2016/07/22 03:43:36, tapted wrote:
> for simply types we usually just use properties. In the header (before the
> initializer above)
> 
> @property(assign, nonatomic) BOOL titleHidden;
> 
> 
> then in the .mm (also just before the initializer)
> 
> @synthesize titleHidden = titleHidden_;
> 
> you don't need an implementaiton of - (void)setTitleHidden:

Done.

https://codereview.chromium.org/2168853003/diff/1/chrome/browser/ui/cocoa/med...
File chrome/browser/ui/cocoa/media_picker/desktop_media_picker_item.mm (right):

https://codereview.chromium.org/2168853003/diff/1/chrome/browser/ui/cocoa/med...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_item.mm:21:
hiddenTitle_ = false;
On 2016/07/22 03:43:37, tapted wrote:
> ObjC objects are memset to 0s, so this isn't needed (see go/objcguide)

Done.

https://codereview.chromium.org/2168853003/diff/1/chrome/browser/ui/cocoa/med...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_item.mm:53: if
(hiddenTitle_)
On 2016/07/22 03:43:37, tapted wrote:
> nit: return titleHidden_ ? nil : imageTitle_;
> 
> (the .get shouldn't be needed)

Done.

[commit-bot: I haz the power, 2016-07-22 19:29:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-22 19:29:36]

Dry run: This issue passed the CQ dry run.

[tapted, 2016-07-24 23:46:10]

lgtm and thanks for the extra test case!

[qiangchen, 2016-07-25 15:55:22]

The CQ bit was checked by qiangchen@chromium.org

[commit-bot: I haz the power, 2016-07-25 15:55:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-25 16:34:47]

Description was changed from

==========
Mac Picker UI: Screen Auto Selection

This CL improves usability of Desktop Capture Picker on Mac 
a little.

1. we auto select the first screen source, 
2. we omit the label "Entire Screen" in the case there is only one screen, as
this is redundant.

BUG=580116,630172
==========

to

==========
Mac Picker UI: Screen Auto Selection

This CL improves usability of Desktop Capture Picker on Mac 
a little.

1. we auto select the first screen source, 
2. we omit the label "Entire Screen" in the case there is only one screen, as
this is redundant.

BUG=580116,630172
==========

[commit-bot: I haz the power, 2016-07-25 16:34:48]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-07-25 16:36:46]

Description was changed from

==========
Mac Picker UI: Screen Auto Selection

This CL improves usability of Desktop Capture Picker on Mac 
a little.

1. we auto select the first screen source, 
2. we omit the label "Entire Screen" in the case there is only one screen, as
this is redundant.

BUG=580116,630172
==========

to

==========
Mac Picker UI: Screen Auto Selection

This CL improves usability of Desktop Capture Picker on Mac
a little.

1. we auto select the first screen source,
2. we omit the label "Entire Screen" in the case there is only one screen, as
this is redundant.

BUG=580116,630172

Committed: https://crrev.com/8ddd74f352bda12bbad52aebb20fe9b6126eae1f
Cr-Commit-Position: refs/heads/master@{#407490}
==========

[commit-bot: I haz the power, 2016-07-25 16:36:47]

Patchset 3 (id:??) landed as
https://crrev.com/8ddd74f352bda12bbad52aebb20fe9b6126eae1f
Cr-Commit-Position: refs/heads/master@{#407490}