Implementing screenshare bypass flag on Mac.

Implementing screenshare bypass flag on Mac.

The --auto-select-desktop-capture-source flag is quite useful for
testing since tools like Selenium generally can't interact with dialogs
we pop up in the browser. It is implemented for aura-using platforms
like Linux, CrOS and Windows and this patch adds it for Mac too.

BUG=475372
Committed: https://crrev.com/c6a64af09117b48524074e9a07a98f45de503ff8
Cr-Commit-Position: refs/heads/master@{#327013}

[phoglund_chromium, 2015-04-14 12:41:21]

phoglund@chromium.org changed reviewers:
+ groby@chromium.org

[phoglund_chromium, 2015-04-14 12:41:22]

Please review! See bug and CL description for context. Thanks.

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

https://codereview.chromium.org/1084913002/diff/1/chrome/browser/ui/cocoa/med...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:294:
[self reportResult:[item sourceID]];
I assume here it's OK to do this on this thread and that I don't have to
PostTask it anywhere. It seemed to work fine when I tested it, but I'm a total
objc newbie. Review with care :)

[groby-ooo-7-16, 2015-04-14 15:01:21]

groby@chromium.org changed reviewers:
+ dcaiafa@chromium.org

[groby-ooo-7-16, 2015-04-14 15:01:21]

Stuck on an implementation issue in the surrounding code. Pinging dcaiafa to
clarify

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

https://codereview.chromium.org/1084913002/diff/1/chrome/browser/ui/cocoa/med...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:294:
[self reportResult:[item sourceID]];
On 2015/04/14 12:41:22, phoglund wrote:
> I assume here it's OK to do this on this thread and that I don't have to
> PostTask it anywhere. It seemed to work fine when I tested it, but I'm a total
> objc newbie. Review with care :)

It *should* be OK. sourceAddedAtIndex definitely has to be called on the UI
thread, or strange things happen. Same goes for closing.

I'm worried about the |bridge_| not being reset, though. Technically,
|media_list_| could be reporting more items after we close here. AIUI,
DesktopMediaList doesn't have a StopUpdating() call - that _probably_ means you
need to reset |media_list_| when closing. 

dcaiafa@, any thoughts?

[phoglund_chromium, 2015-04-16 11:57:34]

> It *should* be OK. sourceAddedAtIndex definitely has to be called on the UI
> thread, or strange things happen. Same goes for closing.
> 
> I'm worried about the |bridge_| not being reset, though. Technically,
> |media_list_| could be reporting more items after we close here. AIUI,
> DesktopMediaList doesn't have a StopUpdating() call - that _probably_ means
you
> need to reset |media_list_| when closing. 
> 
> dcaiafa@, any thoughts?

Hmm. Yeah, we did have a similar problem in the Aura picker. In that case,
closing would lead the invalidation the list of targets, but the list of targets
was being enumerated upon by the caller of OnSourceAdded. That led to a crash.
I'm not seeing anything like that here (it doesn't crash), but I'm not sure what
happens with sourceAddedAtIndex calls done after I step in and close the window.

[phoglund_chromium, 2015-04-20 13:14:08]

Friendly ping :)

[groby-ooo-7-16, 2015-04-22 01:17:46]

Can you try this with a) a source list that has more than one update and b) a
hack that will automatically call [self close] after the first one is updated?

The idea that something closes itself while still being an active recipient of
notifications makes me somewhat uneasy.

[phoglund_chromium, 2015-04-22 14:40:10]

On 2015/04/22 01:17:46, groby wrote:
> Can you try this with a) a source list that has more than one update and b) a
> hack that will automatically call [self close] after the first one is updated?
> 
> The idea that something closes itself while still being an active recipient of
> notifications makes me somewhat uneasy.

I tried hacking this into line 288:
if (index == 0) {
    [self close];
    return;
}

which should leave the other 7 sources being sent to a closed window. Obviously
my new functionality doesn't work with the above hacked in. The window appears
and disappears again and Chrome stays up and nothing crashes as far I can see.
Any idea how I trigger a)? Just pop up a new window while running? Note that the
dialog disappears very quickly if we find the targeted window in the list.

[dcaiafa, 2015-04-22 20:32:42]

lgtm

Apologies for the late response. I was OOO in the past few days.

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

https://codereview.chromium.org/1084913002/diff/1/chrome/browser/ui/cocoa/med...
chrome/browser/ui/cocoa/media_picker/desktop_media_picker_controller.mm:294:
[self reportResult:[item sourceID]];
On 2015/04/14 15:01:21, groby wrote:
> On 2015/04/14 12:41:22, phoglund wrote:
> > I assume here it's OK to do this on this thread and that I don't have to
> > PostTask it anywhere. It seemed to work fine when I tested it, but I'm a
total
> > objc newbie. Review with care :)
> 
> It *should* be OK. sourceAddedAtIndex definitely has to be called on the UI
> thread, or strange things happen. Same goes for closing.
> 
> I'm worried about the |bridge_| not being reset, though. Technically,
> |media_list_| could be reporting more items after we close here. AIUI,
> DesktopMediaList doesn't have a StopUpdating() call - that _probably_ means
you
> need to reset |media_list_| when closing. 
> 
> dcaiafa@, any thoughts?

It's been a while since I looked at this code, but I looked around and I think
this is fine. No need to reset |media_list_|. It's by design that the window
could be closed while new sources are being reported.

[phoglund_chromium, 2015-04-23 11:31:56]

> It's been a while since I looked at this code, but I looked around and I think
> this is fine. No need to reset |media_list_|. It's by design that the window
> could be closed while new sources are being reported.

Ok, thanks! groby, you fine with landing this then?

[phoglund_chromium, 2015-04-24 09:24:55]

On 2015/04/23 11:31:56, phoglund wrote:
> > It's been a while since I looked at this code, but I looked around and I
think
> > this is fine. No need to reset |media_list_|. It's by design that the window
> > could be closed while new sources are being reported.
> 
> Ok, thanks! groby, you fine with landing this then?

I'll assume this is alright to land since it seems to work and dcaiafa seems
happy with it.

[phoglund_chromium, 2015-04-24 09:25:15]

The CQ bit was checked by phoglund@chromium.org

[commit-bot: I haz the power, 2015-04-24 09:25:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1084913002/1

[commit-bot: I haz the power, 2015-04-24 09:31:59]

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

[commit-bot: I haz the power, 2015-04-24 09:32:00]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[phoglund_chromium, 2015-04-24 12:55:00]

Ah, dcaiafa wasn't OWNER. Still need LGTM from groby@.

[groby-ooo-7-16, 2015-04-25 00:20:36]

lgtm

[phoglund_chromium, 2015-04-27 07:41:12]

The CQ bit was checked by phoglund@chromium.org

[commit-bot: I haz the power, 2015-04-27 07:41:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1084913002/1

[commit-bot: I haz the power, 2015-04-27 08:25:06]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-04-27 08:26:09]

Patchset 1 (id:??) landed as
https://crrev.com/c6a64af09117b48524074e9a07a98f45de503ff8
Cr-Commit-Position: refs/heads/master@{#327013}