Add command-line flag for tests to bypass the screenshare window picker.

Add command-line flag for tests to bypass the screenshare window picker.

This flag will make it possible for external tests to bypass the dialog
where you select what to screen share. When this flag is passed on
startup and an extension asks to use the desktop capture API, it will
auto-bypass the dialogue if the target shows up in window picker.

This is assumed to be safe because the flag would have to be passed
each time Chrome is started up, rather than being an about:flags flag.

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

[jiayl, 2014-07-09 17:02:34]

https://codereview.chromium.org/378273003/diff/1/chrome/browser/ui/views/desk...
File chrome/browser/ui/views/desktop_media_picker_views.cc (right):

https://codereview.chromium.org/378273003/diff/1/chrome/browser/ui/views/desk...
chrome/browser/ui/views/desktop_media_picker_views.cc:498: if
(ShouldAutoSelectSource(source)) {
It's not safe to do these in this callback, as it will cause the window and the
backing objects (including the media_list_) destroyed while the caller
(media_list_) is looping to add more sources. You may PostTask instead.

[msw, 2014-07-09 17:03:10]

Do this with a test-specific API instead of a command-line flag; flags are
generally not meant to last beyond short experimental periods.

[phoglund_chromium, 2014-08-04 12:37:25]

https://codereview.chromium.org/378273003/diff/1/chrome/browser/ui/views/desk...
File chrome/browser/ui/views/desktop_media_picker_views.cc (right):

https://codereview.chromium.org/378273003/diff/1/chrome/browser/ui/views/desk...
chrome/browser/ui/views/desktop_media_picker_views.cc:498: if
(ShouldAutoSelectSource(source)) {
On 2014/07/09 17:02:34, jiayl wrote:
> It's not safe to do these in this callback, as it will cause the window and
the
> backing objects (including the media_list_) destroyed while the caller
> (media_list_) is looping to add more sources. You may PostTask instead.

Oh, right. Umm, I'm not very familiar with Chrome threading so please help me
out. So I would like to post a task to run OnSelectionChanged and OnDoubleClick,
but only when the caller is done adding sources. I can't post to this object
though because it is not reference counted, and therefore I can't base::Bind it
(right?)

I looked at the calling code in for instance
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/med...,
and I considered moving the bypass code there. Like, when done adding all
sources, check if we should forcibly select one of them, to avoid the problem
you outline above. However that method doesn't have a way to forcibly select an
entry since it talks to the view through an observer interface where we can't
call OnDoubleClick and the like. Which is nice design, since it prevents such a
horrible hack :)

So the fundamental problem is to wait to some suitable safe time when the dialog
has been populated, and then go through the media_list and see if the source
we're waiting for has appeared. Is there some method that gets called when we
can safely call OnSelectionChanged etc? If so I could just hook up this
find-and-doubleclick code there. Otherwise I guess I need to set up some kind of
periodical task (if flag is specified, set up a task which periodically polls
the source list for the desired source).

[jiayl, 2014-08-07 15:18:10]

On 2014/08/04 12:37:25, phoglund wrote:
>
https://codereview.chromium.org/378273003/diff/1/chrome/browser/ui/views/desk...
> File chrome/browser/ui/views/desktop_media_picker_views.cc (right):
> 
>
https://codereview.chromium.org/378273003/diff/1/chrome/browser/ui/views/desk...
> chrome/browser/ui/views/desktop_media_picker_views.cc:498: if
> (ShouldAutoSelectSource(source)) {
> On 2014/07/09 17:02:34, jiayl wrote:
> > It's not safe to do these in this callback, as it will cause the window and
> the
> > backing objects (including the media_list_) destroyed while the caller
> > (media_list_) is looping to add more sources. You may PostTask instead.
> 
> Oh, right. Umm, I'm not very familiar with Chrome threading so please help me
> out. So I would like to post a task to run OnSelectionChanged and
OnDoubleClick,
> but only when the caller is done adding sources. I can't post to this object
> though because it is not reference counted, and therefore I can't base::Bind
it
> (right?)
> 
> I looked at the calling code in for instance
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/med...,
> and I considered moving the bypass code there. Like, when done adding all
> sources, check if we should forcibly select one of them, to avoid the problem
> you outline above. However that method doesn't have a way to forcibly select
an
> entry since it talks to the view through an observer interface where we can't
> call OnDoubleClick and the like. Which is nice design, since it prevents such
a
> horrible hack :)
> 
> So the fundamental problem is to wait to some suitable safe time when the
dialog
> has been populated, and then go through the media_list and see if the source
> we're waiting for has appeared. Is there some method that gets called when we
> can safely call OnSelectionChanged etc? If so I could just hook up this
> find-and-doubleclick code there. Otherwise I guess I need to set up some kind
of
> periodical task (if flag is specified, set up a task which periodically polls
> the source list for the desired source).

I would use a WeakPtr in the PostTask so the task is auto cancelled if the
dialog is destroyed.
See
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...

[phoglund_chromium, 2014-08-12 13:11:33]

Ok jiayl, I tried rewriting this using PostTask instead. Please review, and bear
in mind it's the first time I'm writing PostTask/Bind code :)

[phoglund_chromium, 2014-08-12 13:26:51]

pkasting: review/owner stamp for the UI parts.

Note that there's a fair amount of discussion in the bug about how to introduce
the new flag; I'd argue that the new flag is necessary for this particular case.

[Peter Kasting, 2014-08-12 17:48:11]

On 2014/08/12 13:26:51, phoglund wrote:
> Note that there's a fair amount of discussion in the bug about how to
introduce
> the new flag; I'd argue that the new flag is necessary for this particular
case.

Let's finish hashing things out on the bug first.

[Peter Kasting, 2014-08-13 17:17:51]

LGTM

https://codereview.chromium.org/378273003/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/desktop_media_picker_views.cc (right):

https://codereview.chromium.org/378273003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/desktop_media_picker_views.cc:66: static bool
ShouldAutoSelectSource(const DesktopMediaList::Source& source) {
Nit: "static" unnecessary (you're already in an anonymous namespace)

Especially with the shortening suggestion below I'd probably just inline this
into the lone caller, but up to you.

https://codereview.chromium.org/378273003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/desktop_media_picker_views.cc:73: return desired_source
== base::UTF16ToASCII(source.name);
Nit: Shorter:

  return !desired_source.empty() &&
      (base::ASCIIToUTF16(desired_source) == source.name);

(I chose to upconvert to UTF-16 instead of downconverting to ASCII in order to
avoid implying anything about whether source.name is actually ASCII.)

https://codereview.chromium.org/378273003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/desktop_media_picker_views.cc:76: class AutoSelectTask:
public base::RefCounted<AutoSelectTask> {
Does this really need to be refcounted?  Seems like the posted task could just
own this object.

Nit: Space before single colon

https://codereview.chromium.org/378273003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/desktop_media_picker_views.cc:93: };
Nit: DISALLOW_COPY_AND_ASSIGN

[jiayl, 2014-08-13 17:32:43]

https://codereview.chromium.org/378273003/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/desktop_media_picker_views.cc (right):

https://codereview.chromium.org/378273003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/desktop_media_picker_views.cc:361:
base::Bind(&AutoSelectTask::AcceptSelectedSource, task));
AutoSelectTask is not needed.
You can just do

PostTask(UI, FROM_HERE,    
    base::Bind(&DesktopMediaListView::AcceptSelection,     
               weak_factory_.GetWeakPtr());
 
DesktopMediaListView::AcceptSelection() {
  OnSelectionChanged();
  OnDoubleClick();
}

[Peter Kasting, 2014-08-13 17:34:31]

On 2014/08/13 17:32:43, jiayl wrote:
>
https://codereview.chromium.org/378273003/diff/20001/chrome/browser/ui/views/...
> File chrome/browser/ui/views/desktop_media_picker_views.cc (right):
> 
>
https://codereview.chromium.org/378273003/diff/20001/chrome/browser/ui/views/...
> chrome/browser/ui/views/desktop_media_picker_views.cc:361:
> base::Bind(&AutoSelectTask::AcceptSelectedSource, task));
> AutoSelectTask is not needed.
> You can just do
> 
> PostTask(UI, FROM_HERE,    
>     base::Bind(&DesktopMediaListView::AcceptSelection,     
>                weak_factory_.GetWeakPtr());
>  
> DesktopMediaListView::AcceptSelection() {
>   OnSelectionChanged();
>   OnDoubleClick();
> }

Ooh, that's even better than making the object non-refcounted.

[phoglund_chromium, 2014-08-14 13:16:24]

PTAL

https://codereview.chromium.org/378273003/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/desktop_media_picker_views.cc (right):

https://codereview.chromium.org/378273003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/desktop_media_picker_views.cc:66: static bool
ShouldAutoSelectSource(const DesktopMediaList::Source& source) {
On 2014/08/13 17:17:50, pkasting (away thru Aug 20) wrote:
> Nit: "static" unnecessary (you're already in an anonymous namespace)
> 
> Especially with the shortening suggestion below I'd probably just inline this
> into the lone caller, but up to you.
Done. Inlining worked out pretty nicely with your shorter version.

https://codereview.chromium.org/378273003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/desktop_media_picker_views.cc:73: return desired_source
== base::UTF16ToASCII(source.name);
On 2014/08/13 17:17:50, pkasting (away thru Aug 20) wrote:
> Nit: Shorter:
> 
>   return !desired_source.empty() &&
>       (base::ASCIIToUTF16(desired_source) == source.name);
> 
> (I chose to upconvert to UTF-16 instead of downconverting to ASCII in order to
> avoid implying anything about whether source.name is actually ASCII.)

Done.

https://codereview.chromium.org/378273003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/desktop_media_picker_views.cc:76: class AutoSelectTask:
public base::RefCounted<AutoSelectTask> {
On 2014/08/13 17:17:50, pkasting (away thru Aug 20) wrote:
> Does this really need to be refcounted?  Seems like the posted task could just
> own this object.
> 
> Nit: Space before single colon
Nope, went with jiayl's solution instead.

https://codereview.chromium.org/378273003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/desktop_media_picker_views.cc:93: };
On 2014/08/13 17:17:50, pkasting (away thru Aug 20) wrote:
> Nit: DISALLOW_COPY_AND_ASSIGN

Acknowledged.

https://codereview.chromium.org/378273003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/desktop_media_picker_views.cc:361:
base::Bind(&AutoSelectTask::AcceptSelectedSource, task));
On 2014/08/13 17:32:42, jiayl wrote:
> AutoSelectTask is not needed.
> You can just do
> 
> PostTask(UI, FROM_HERE,    
>     base::Bind(&DesktopMediaListView::AcceptSelection,     
>                weak_factory_.GetWeakPtr());
>  
> DesktopMediaListView::AcceptSelection() {
>   OnSelectionChanged();
>   OnDoubleClick();
> }
So PostTask is smart enough to check the weak pointer? Nice.

[jiayl, 2014-08-14 17:02:57]

lgtm

[phoglund_chromium, 2014-08-15 11:34:03]

pkasting, will assume here you're happy with the changes. If not, please accept
my apologies and let me know what you want fixed :)

[phoglund_chromium, 2014-08-15 11:34:10]

The CQ bit was checked by phoglund@chromium.org

[commit-bot: I haz the power, 2014-08-15 11:35:35]

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

[commit-bot: I haz the power, 2014-08-15 11:44:57]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/42244)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/47448)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-15 11:46:51]

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

[commit-bot: I haz the power, 2014-08-15 11:46:53]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/42246)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[phoglund_chromium, 2014-08-15 13:50:53]

The CQ bit was checked by phoglund@chromium.org

[commit-bot: I haz the power, 2014-08-15 13:52:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/phoglund@chromium.org/378273003/60001

[commit-bot: I haz the power, 2014-08-15 15:49:38]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/47479)
  android_dbg_triggered_tests on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-15 16:03:18]

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

[commit-bot: I haz the power, 2014-08-15 16:03:19]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[phoglund_chromium, 2014-08-18 12:09:43]

The CQ bit was checked by phoglund@chromium.org

[commit-bot: I haz the power, 2014-08-18 12:10:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/phoglund@chromium.org/378273003/80001

[phoglund_chromium, 2014-08-18 12:25:14]

The CQ bit was unchecked by phoglund@chromium.org

[phoglund_chromium, 2014-08-18 12:25:57]

The CQ bit was checked by phoglund@chromium.org

[commit-bot: I haz the power, 2014-08-18 12:26:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/phoglund@chromium.org/378273003/100001

[commit-bot: I haz the power, 2014-08-18 13:43:10]

Committed patchset #6 (100001) as 290249