Removed duplicated JS hasVideoInputDeviceOnSystem() test helper

Removed duplicated JS hasVideoInputDeviceOnSystem() test helper

BUG=509735
Committed: https://crrev.com/8870e560931560b4dae5a1d2c87c5629b1e3c44e
Cr-Commit-Position: refs/heads/master@{#419008}

[lunalu1, 2016-09-03 02:43:02]

Description was changed from

==========
initial commit


save for now

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

to

==========
Removed duplicated JS hasVideoInputDeviceOnSystem() test helper

BUG=509735
==========

[lunalu1, 2016-09-06 14:21:48]

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

[commit-bot: I haz the power, 2016-09-06 14:22:03]

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-09-06 14:32:13]

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

[commit-bot: I haz the power, 2016-09-06 14:32:14]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lunalu1, 2016-09-06 15:04:05]

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

[commit-bot: I haz the power, 2016-09-06 15:04:25]

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-09-06 15:14:27]

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

[commit-bot: I haz the power, 2016-09-06 15:14:28]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mcasas, 2016-09-06 15:34:53]

mcasas@chromium.org changed reviewers:
+ mcasas@chromium.org

[mcasas, 2016-09-06 15:34:54]

https://codereview.chromium.org/2310683003/diff/80001/chrome/browser/media/we...
File chrome/browser/media/webrtc_webcam_browsertest.cc (right):

https://codereview.chromium.org/2310683003/diff/80001/chrome/browser/media/we...
chrome/browser/media/webrtc_webcam_browsertest.cc:82: LOG(INFO) << "No webcam
found on bot: skipping...";
nit: s/LOG/DLOG/ which is technically more
correct, and anyway we compile tests with NDEBUG
set to false.

https://codereview.chromium.org/2310683003/diff/80001/content/browser/webrtc/...
File content/browser/webrtc/webrtc_webcam_browsertest.cc (right):

https://codereview.chromium.org/2310683003/diff/80001/content/browser/webrtc/...
content/browser/webrtc/webrtc_webcam_browsertest.cc:78: shell(),
content::hasVideoInputDeviceFunction, &result));
No need to specify content::

Also, here you seem to be calling a function
hasVideoInputDeviceFunction which is not defined
anywhere. Instead, I suggest replacing l.76-81 
with sth like:

  if (!content::HasWebcamAvailableOnSystem(shell()->web_contents())) {
    DVLOG(INFO) << "No webcam available, skipping test.";
    return;
  }

https://codereview.chromium.org/2310683003/diff/80001/content/public/test/bro...
File content/public/test/browser_test_utils.h (right):

https://codereview.chromium.org/2310683003/diff/80001/content/public/test/bro...
content/public/test/browser_test_utils.h:178: static const std::string
hasVideoInputDeviceOnSystem =
Move this string to the anonymous namespace in the
implementation file.

https://codereview.chromium.org/2310683003/diff/80001/content/public/test/bro...
content/public/test/browser_test_utils.h:187: "})()";
Can we indent these lines to make them more readable?

[lunalu1, 2016-09-06 19:48:11]

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

[commit-bot: I haz the power, 2016-09-06 19:48:56]

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-09-06 19:59:58]

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

[commit-bot: I haz the power, 2016-09-06 19:59:59]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lunalu1, 2016-09-06 20:49:35]

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

[commit-bot: I haz the power, 2016-09-06 20:50:29]

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

[lunalu1, 2016-09-06 20:55:23]

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

[commit-bot: I haz the power, 2016-09-06 20:55:54]

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

[lunalu1, 2016-09-06 20:56:13]

Hi Miguel,

Thank you for the feedback.

I have made some change based on your comments. Please take a look at it.

Regards,
Luna

https://codereview.chromium.org/2310683003/diff/80001/chrome/browser/media/we...
File chrome/browser/media/webrtc_webcam_browsertest.cc (right):

https://codereview.chromium.org/2310683003/diff/80001/chrome/browser/media/we...
chrome/browser/media/webrtc_webcam_browsertest.cc:82: LOG(INFO) << "No webcam
found on bot: skipping...";
On 2016/09/06 15:34:54, mcasas wrote:
> nit: s/LOG/DLOG/ which is technically more
> correct, and anyway we compile tests with NDEBUG
> set to false.

Done.

https://codereview.chromium.org/2310683003/diff/80001/content/browser/webrtc/...
File content/browser/webrtc/webrtc_webcam_browsertest.cc (right):

https://codereview.chromium.org/2310683003/diff/80001/content/browser/webrtc/...
content/browser/webrtc/webrtc_webcam_browsertest.cc:78: shell(),
content::hasVideoInputDeviceFunction, &result));
On 2016/09/06 15:34:54, mcasas wrote:
> No need to specify content::
> 
> Also, here you seem to be calling a function
> hasVideoInputDeviceFunction which is not defined
> anywhere. Instead, I suggest replacing l.76-81 
> with sth like:
> 
>   if (!content::HasWebcamAvailableOnSystem(shell()->web_contents())) {
>     DVLOG(INFO) << "No webcam available, skipping test.";
>     return;
>   }
> 

Done.

https://codereview.chromium.org/2310683003/diff/80001/content/public/test/bro...
File content/public/test/browser_test_utils.h (right):

https://codereview.chromium.org/2310683003/diff/80001/content/public/test/bro...
content/public/test/browser_test_utils.h:178: static const std::string
hasVideoInputDeviceOnSystem =
On 2016/09/06 15:34:54, mcasas wrote:
> Move this string to the anonymous namespace in the
> implementation file.

Done.

https://codereview.chromium.org/2310683003/diff/80001/content/public/test/bro...
content/public/test/browser_test_utils.h:187: "})()";
On 2016/09/06 15:34:54, mcasas wrote:
> Can we indent these lines to make them more readable?

Done.

[lunalu1, 2016-09-06 21:30:55]

The CQ bit was checked by lunalu@chromium.org

[commit-bot: I haz the power, 2016-09-06 21:31:10]

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-09-06 21:31:11]

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

[commit-bot: I haz the power, 2016-09-06 21:31:13]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[mcasas, 2016-09-06 23:13:11]

mcasas@chromium.org changed reviewers:
+ sky@chromium.org

[mcasas, 2016-09-06 23:13:12]

el-gee-tee-em, deferring to sky@ : is content/public/test
the right place to put this helper method, or should 
we bury the implementation it one step further inside 
content/test/ ?

https://codereview.chromium.org/2310683003/diff/140001/content/public/test/br...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/2310683003/diff/140001/content/public/test/br...
content/public/test/browser_test_utils.cc:424:
"returnToTest('has-video-input-device');"
returnToTest(), confusingly enough, doesn't
seem to return. This code could returnToTest
a 'has-video-input-device' and then subsequently a
'no-video-input-devices', if e.g. the bot would 
have a webcam and another audio device. IOW you
need a return statement here.

Also please add a TODO and a bug saying that 
this needs to be migrated to Navigator.mediaDevices(),
since MediaStreamTrack.getSources() is deprecated
(https://www.chromestatus.com/feature/4906859072847872).

[sky, 2016-09-07 03:32:18]

sky@chromium.org changed reviewers:
+ jam@chromium.org

[sky, 2016-09-07 03:32:18]

I tend to think you want the function in content/public. +jam as I'm not sure.

https://codereview.chromium.org/2310683003/diff/140001/content/public/test/br...
File content/public/test/browser_test_utils.h (right):

https://codereview.chromium.org/2310683003/diff/140001/content/public/test/br...
content/public/test/browser_test_utils.h:170: bool
HasWebcamAvailableOnSystem(WebContents* tab_contents);
tab_contents -> web_contents

[lunalu1, 2016-09-07 14:30:34]

Hi, 

Thank you for reviewing my change. 

I have updated the CL based on your comments. Please take another look.

Thanks,
Luna

https://codereview.chromium.org/2310683003/diff/140001/content/public/test/br...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/2310683003/diff/140001/content/public/test/br...
content/public/test/browser_test_utils.cc:424:
"returnToTest('has-video-input-device');"
On 2016/09/06 23:13:12, mcasas wrote:
> returnToTest(), confusingly enough, doesn't
> seem to return. This code could returnToTest
> a 'has-video-input-device' and then subsequently a
> 'no-video-input-devices', if e.g. the bot would 
> have a webcam and another audio device. IOW you
> need a return statement here.
> 
> Also please add a TODO and a bug saying that 
> this needs to be migrated to Navigator.mediaDevices(),
> since MediaStreamTrack.getSources() is deprecated
> (https://www.chromestatus.com/feature/4906859072847872).

Done.

https://codereview.chromium.org/2310683003/diff/140001/content/public/test/br...
File content/public/test/browser_test_utils.h (right):

https://codereview.chromium.org/2310683003/diff/140001/content/public/test/br...
content/public/test/browser_test_utils.h:170: bool
HasWebcamAvailableOnSystem(WebContents* tab_contents);
On 2016/09/07 03:32:18, sky wrote:
> tab_contents -> web_contents

Done.

[lunalu1, 2016-09-07 16:09:28]

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

[commit-bot: I haz the power, 2016-09-07 16:09:56]

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

[lunalu1, 2016-09-07 16:25:53]

Patchset #1 (id:1) has been deleted

[lunalu1, 2016-09-07 16:25:58]

Patchset #1 (id:20001) has been deleted

[lunalu1, 2016-09-07 16:26:03]

Patchset #1 (id:40001) has been deleted

[lunalu1, 2016-09-07 16:26:11]

Patchset #1 (id:60001) has been deleted

[lunalu1, 2016-09-07 16:26:24]

Patchset #2 (id:100001) has been deleted

[lunalu1, 2016-09-07 16:26:30]

Patchset #2 (id:120001) has been deleted

[commit-bot: I haz the power, 2016-09-07 17:23:12]

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

[commit-bot: I haz the power, 2016-09-07 17:23:13]

Dry run: This issue passed the CQ dry run.

[jam, 2016-09-08 19:53:45]

On 2016/09/07 03:32:18, sky wrote:
> I tend to think you want the function in content/public. +jam as I'm not sure.
> 
>
https://codereview.chromium.org/2310683003/diff/140001/content/public/test/br...
> File content/public/test/browser_test_utils.h (right):
> 
>
https://codereview.chromium.org/2310683003/diff/140001/content/public/test/br...
> content/public/test/browser_test_utils.h:170: bool
> HasWebcamAvailableOnSystem(WebContents* tab_contents);
> tab_contents -> web_contents

right, if it's called by test code in src/chrome, it should be in
content/public/test

[lunalu1, 2016-09-09 15:11:19]

Done. Please take another look.

[mcasas, 2016-09-09 20:44:54]

On 2016/09/09 15:11:19, loonybear wrote:
> Done. Please take another look.

lgtm as-is

[sky, 2016-09-13 21:32:35]

https://codereview.chromium.org/2310683003/diff/160001/content/public/test/br...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/2310683003/diff/160001/content/public/test/br...
content/public/test/browser_test_utils.cc:419: static const std::string
hasVideoInputDeviceOnSystem =
There is no need for the static as this in an anonymous namespace. Style guide
says constants defined like this should be POD (plain old data, aka no objects).
Use a const char[] and you should be fine.
Style guide also says this should be named kFoo.

https://codereview.chromium.org/2310683003/diff/160001/content/public/test/br...
content/public/test/browser_test_utils.cc:771: return result ==
"has-video-input-device";
Can this be a constant?

https://codereview.chromium.org/2310683003/diff/160001/content/public/test/br...
File content/public/test/browser_test_utils.h (right):

https://codereview.chromium.org/2310683003/diff/160001/content/public/test/br...
content/public/test/browser_test_utils.h:170: bool
HasWebcamAvailableOnSystem(WebContents* web_contents);
IsWebcamAvailableOnSystem?

[lunalu1, 2016-09-13 21:53:47]

Patchset #4 (id:180001) has been deleted

[lunalu1, 2016-09-13 21:54:05]

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

[commit-bot: I haz the power, 2016-09-13 21:54:24]

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

[lunalu1, 2016-09-13 21:55:02]

Thanks for your feedback. 

I have made the change based on your comments. Please take another look.

Regards,
Luna

https://codereview.chromium.org/2310683003/diff/160001/content/public/test/br...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/2310683003/diff/160001/content/public/test/br...
content/public/test/browser_test_utils.cc:419: static const std::string
hasVideoInputDeviceOnSystem =
On 2016/09/13 21:32:34, sky wrote:
> There is no need for the static as this in an anonymous namespace. Style guide
> says constants defined like this should be POD (plain old data, aka no
objects).
> Use a const char[] and you should be fine.
> Style guide also says this should be named kFoo.

Done.

https://codereview.chromium.org/2310683003/diff/160001/content/public/test/br...
content/public/test/browser_test_utils.cc:771: return result ==
"has-video-input-device";
On 2016/09/13 21:32:34, sky wrote:
> Can this be a constant?

Done.

https://codereview.chromium.org/2310683003/diff/160001/content/public/test/br...
File content/public/test/browser_test_utils.h (right):

https://codereview.chromium.org/2310683003/diff/160001/content/public/test/br...
content/public/test/browser_test_utils.h:170: bool
HasWebcamAvailableOnSystem(WebContents* web_contents);
On 2016/09/13 21:32:34, sky wrote:
> IsWebcamAvailableOnSystem?

Done.

[commit-bot: I haz the power, 2016-09-13 23:09:56]

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

[commit-bot: I haz the power, 2016-09-13 23:09:57]

Dry run: This issue passed the CQ dry run.

[sky, 2016-09-14 02:55:51]

LGTM

[lunalu1, 2016-09-14 14:13:39]

The CQ bit was checked by lunalu@chromium.org

[lunalu1, 2016-09-14 14:13:40]

The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org
Link to the patchset: https://codereview.chromium.org/2310683003/#ps200001
(title: "Codereview update")

[commit-bot: I haz the power, 2016-09-14 14:13:59]

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-09-14 14:21:08]

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

[commit-bot: I haz the power, 2016-09-14 14:21:09]

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

[lunalu1, 2016-09-14 15:53:47]

lunalu@chromium.org changed reviewers:
+ tommi@chromium.org

[lunalu1, 2016-09-14 15:53:48]

Hi tommi@,

Could you please take a look at my CL before I commit? Mostly for the change I
made to chrome/test/data/webrtc/media_devices.js and
content/browser/webrtc/webrtc_webcam_browsertest.cc 

Thanks!

[tommi (sloooow) - chröme, 2016-09-15 16:29:12]

lgtm

[lunalu1, 2016-09-15 17:25:10]

The CQ bit was checked by lunalu@chromium.org

[commit-bot: I haz the power, 2016-09-15 17:25:34]

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-09-15 19:13:33]

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

[commit-bot: I haz the power, 2016-09-15 19:13:34]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[lunalu1, 2016-09-15 21:11:49]

The CQ bit was checked by lunalu@chromium.org

[commit-bot: I haz the power, 2016-09-15 21:12:36]

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-09-15 22:12:35]

Description was changed from

==========
Removed duplicated JS hasVideoInputDeviceOnSystem() test helper

BUG=509735
==========

to

==========
Removed duplicated JS hasVideoInputDeviceOnSystem() test helper

BUG=509735
==========

[commit-bot: I haz the power, 2016-09-15 22:12:36]

Committed patchset #4 (id:200001)

[commit-bot: I haz the power, 2016-09-15 22:17:40]

Description was changed from

==========
Removed duplicated JS hasVideoInputDeviceOnSystem() test helper

BUG=509735
==========

to

==========
Removed duplicated JS hasVideoInputDeviceOnSystem() test helper

BUG=509735

Committed: https://crrev.com/8870e560931560b4dae5a1d2c87c5629b1e3c44e
Cr-Commit-Position: refs/heads/master@{#419008}
==========

[commit-bot: I haz the power, 2016-09-15 22:17:42]

Patchset 4 (id:??) landed as
https://crrev.com/8870e560931560b4dae5a1d2c87c5629b1e3c44e
Cr-Commit-Position: refs/heads/master@{#419008}

[henrika (OOO until Aug 14), 2016-09-16 08:46:04]

A revert of this CL (patchset #4 id:200001) has been created in
https://codereview.chromium.org/2347473004/ by henrika@chromium.org.

The reason for reverting is: Speculative revert from WebRTC-in-Chrome sheriff
since it seems like
this CL breaks content_browser tests on Windows 7.

See
https://build.chromium.org/p/chromium.webrtc/builders/Win7%20Tester/builds/29...

for more details..