Add layout tests for the audio component of MediaStream Recording.

Add layout tests for the audio component of MediaStream Recording.

Additionally, xtends components/test_runner's generated
WebMediaStream to have a ChromeMock AudioTrack and Source

components/test_runner is used by Blink LayoutTest to
generate a MediaStream. But this MS has only Blink-side
objects, whereas some tests (e.g. MediaRecorder) need
to have Chrome counterparts of those objects. This CL
adds those, for audio.

Since components/ has a strict dependency requirement,
the added code goes into a subfolder of
components/test_runner.

BUG=528519, 548290
Committed: https://crrev.com/dbebd0d23dd4ef4582af0ddbdd55e0727abecb06
Cr-Commit-Position: refs/heads/master@{#360965}

[ajose, 2015-11-17 04:53:42]

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

[ajose, 2015-11-17 04:53:43]

Moved here from https://codereview.chromium.org/1407083006/

[mcasas, 2015-11-17 21:56:01]

Looking good, just a few comments and
please update the description, e.g. indicate
it touches test_runner etc..

https://codereview.chromium.org/1448203002/diff/1/content/public/renderer/med...
File content/public/renderer/media_stream_api.cc (right):

https://codereview.chromium.org/1448203002/diff/1/content/public/renderer/med...
content/public/renderer/media_stream_api.cc:97: 48000,  //
audio_config.rtp_timebase,  // sampling rate
Please clean up these comments.

https://codereview.chromium.org/1448203002/diff/1/content/shell/renderer/layo...
File content/shell/renderer/layout_test/blink_test_runner.h (right):

https://codereview.chromium.org/1448203002/diff/1/content/shell/renderer/layo...
content/shell/renderer/layout_test/blink_test_runner.h:20: 
remove empty line

https://codereview.chromium.org/1448203002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/1448203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/TestExpectations:262: crbug.com/552456 [ Linux ]
fast/mediarecorder/MediaRecorder-audio-video.html [ Pass Failure ]
Why do you need to touch this file? It should only
be touched when you land tests that Fail, and only 
until they are enabled, but the new tests you are 
landing are supposed to work with the additions
to the test_runner. Moreover, you are marking some
other tests as Failure? :?

https://codereview.chromium.org/1448203002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-audio-video.html
(right):

https://codereview.chromium.org/1448203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-audio-video.html:2:
<script src=../../resources/testharness.js></script>
Hmm these three html files are quite similar, I wonder if we
could parameterise them in some fancy way. Check this
testharness.js for ideas, e.g. generate_tests() [1] ?

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[ajose, 2015-11-18 00:21:51]

Description was changed from

==========
Add layout tests for the audio component of MediaStream Recording.

BUG=528519
==========

to

==========
Add layout tests for the audio component of MediaStream Recording.

Additionally, xtends components/test_runner's generated 
WebMediaStream to have a ChromeMock AudioTrack and Source

components/test_runner is used by Blink LayoutTest to
generate a MediaStream. But this MS has only Blink-side
objects, whereas some tests (e.g. MediaRecorder) need 
to have Chrome counterparts of those objects. This CL
adds those, for audio.

Since components/ has a strict dependency requirement,
the added code goes into a subfolder of 
components/test_runner.

BUG=528519
==========

[ajose, 2015-11-18 03:22:19]

Description was changed from

==========
Add layout tests for the audio component of MediaStream Recording.

Additionally, xtends components/test_runner's generated 
WebMediaStream to have a ChromeMock AudioTrack and Source

components/test_runner is used by Blink LayoutTest to
generate a MediaStream. But this MS has only Blink-side
objects, whereas some tests (e.g. MediaRecorder) need 
to have Chrome counterparts of those objects. This CL
adds those, for audio.

Since components/ has a strict dependency requirement,
the added code goes into a subfolder of 
components/test_runner.

BUG=528519
==========

to

==========
Add layout tests for the audio component of MediaStream Recording.

Additionally, xtends components/test_runner's generated 
WebMediaStream to have a ChromeMock AudioTrack and Source

components/test_runner is used by Blink LayoutTest to
generate a MediaStream. But this MS has only Blink-side
objects, whereas some tests (e.g. MediaRecorder) need 
to have Chrome counterparts of those objects. This CL
adds those, for audio.

Since components/ has a strict dependency requirement,
the added code goes into a subfolder of 
components/test_runner.

BUG=528519,548290
==========

[ajose, 2015-11-18 03:35:27]

Thanks for the comments, sorry about the mess

https://codereview.chromium.org/1448203002/diff/1/content/public/renderer/med...
File content/public/renderer/media_stream_api.cc (right):

https://codereview.chromium.org/1448203002/diff/1/content/public/renderer/med...
content/public/renderer/media_stream_api.cc:97: 48000,  //
audio_config.rtp_timebase,  // sampling rate
On 2015/11/17 21:56:00, mcasas wrote:
> Please clean up these comments.

Done.

https://codereview.chromium.org/1448203002/diff/1/content/shell/renderer/layo...
File content/shell/renderer/layout_test/blink_test_runner.h (right):

https://codereview.chromium.org/1448203002/diff/1/content/shell/renderer/layo...
content/shell/renderer/layout_test/blink_test_runner.h:20: 
On 2015/11/17 21:56:00, mcasas wrote:
> remove empty line

Done.

https://codereview.chromium.org/1448203002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/1448203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/TestExpectations:262: crbug.com/552456 [ Linux ]
fast/mediarecorder/MediaRecorder-audio-video.html [ Pass Failure ]
On 2015/11/17 21:56:00, mcasas wrote:
> Why do you need to touch this file? It should only
> be touched when you land tests that Fail, and only 
> until they are enabled, but the new tests you are 
> landing are supposed to work with the additions
> to the test_runner. Moreover, you are marking some
> other tests as Failure? :?

I think I've been screwing up merge conflicts with this file over many, many
rebases...

https://codereview.chromium.org/1448203002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-audio-video.html
(right):

https://codereview.chromium.org/1448203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-audio-video.html:2:
<script src=../../resources/testharness.js></script>
On 2015/11/17 21:56:00, mcasas wrote:
> Hmm these three html files are quite similar, I wonder if we
> could parameterise them in some fancy way. Check this
> testharness.js for ideas, e.g. generate_tests() [1] ?
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Mind if I make this a comment on http://crbug.com/548290 for now?

[ajose, 2015-11-18 03:56:38]

Patchset #2 (id:20001) has been deleted

[ajose, 2015-11-19 18:33:26]

Patchset #3 (id:60001) has been deleted

[ajose, 2015-11-19 18:36:57]

ajose@chromium.org changed reviewers:
+ jochen@chromium.org, peter@chromium.org

[ajose, 2015-11-19 18:36:58]

Hi peter@ and jochen@: PTAL at changes in components/ and content/. Mostly minor
changes to test_runner.

[ajose, 2015-11-19 18:38:51]

Patchset #3 (id:80001) has been deleted

[jochen (gone - plz use gerrit), 2015-11-20 13:41:44]

lgtm

[Peter Beverloo, 2015-11-20 14:05:02]

lgtm

A few nits now that you're hoisting things in narrower scopes :)

https://codereview.chromium.org/1448203002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-audio-video.html
(right):

https://codereview.chromium.org/1448203002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-audio-video.html:5:

nit: I realize you're mostly copying this, but could you please add a comment
briefly describing (one or two sentences) what this test does?

https://codereview.chromium.org/1448203002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-audio-video.html:28:
recorderOnDataAvailable = this.step_func(function(event) {
nit: const recorderOnDataAvailable = ...

(Same for lines 44, 46, 50, 54 and 73.)

Using "const" here (or "let"/"var") scopes them to the function, now you're
making them global. In ES6, "let" scopes a variable to the nearest block,
whereas "var" scopes them to the nearest function (or global). "const" does the
same, but also makes sure that the reference won't change - mind that the value
itself won't be immutable.

https://codereview.chromium.org/1448203002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-audio-video.html:60:
assert_unreached('Exception while creating MediaRecorder: '
nit: your preference, but there's no line length limits in Blink yet.. :-)

https://codereview.chromium.org/1448203002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-audio-video.html:86:
[["video-only",  {video:true}],
nit: I'd prefer these to be explicit, rather than relying on undefined.

{audio: false, video: true}

That allows you to remove the locals on lines 24 and 25 and just use |value|
directly, and makes the generator values clearer.

https://codereview.chromium.org/1448203002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-requestData.html
(right):

https://codereview.chromium.org/1448203002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-requestData.html:5:

dito re: comment

https://codereview.chromium.org/1448203002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-requestData.html:28:
recorderOnDataAvailable = this.step_func_done(function(event) {
dito re: constness

https://codereview.chromium.org/1448203002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-requestData.html:80:
[["video-only",  {video:true}],
dito re: values

[ajose, 2015-11-20 18:24:40]

Patchset #5 (id:140001) has been deleted

[ajose, 2015-11-20 21:37:07]

Thanks for the comments!

mcasas@: WDYT?

https://codereview.chromium.org/1448203002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-audio-video.html
(right):

https://codereview.chromium.org/1448203002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-audio-video.html:5:

On 2015/11/20 14:05:02, Peter Beverloo wrote:
> nit: I realize you're mostly copying this, but could you please add a comment
> briefly describing (one or two sentences) what this test does?

Done.

https://codereview.chromium.org/1448203002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-audio-video.html:28:
recorderOnDataAvailable = this.step_func(function(event) {
On 2015/11/20 14:05:02, Peter Beverloo wrote:
> nit: const recorderOnDataAvailable = ...
> 
> (Same for lines 44, 46, 50, 54 and 73.)
> 
> Using "const" here (or "let"/"var") scopes them to the function, now you're
> making them global. In ES6, "let" scopes a variable to the nearest block,
> whereas "var" scopes them to the nearest function (or global). "const" does
the
> same, but also makes sure that the reference won't change - mind that the
value
> itself won't be immutable.

Neat! I'll have to read more about 'const' and 'let'.

https://codereview.chromium.org/1448203002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-audio-video.html:60:
assert_unreached('Exception while creating MediaRecorder: '
On 2015/11/20 14:05:02, Peter Beverloo wrote:
> nit: your preference, but there's no line length limits in Blink yet.. :-)

Awesome.

https://codereview.chromium.org/1448203002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-audio-video.html:86:
[["video-only",  {video:true}],
On 2015/11/20 14:05:01, Peter Beverloo wrote:
> nit: I'd prefer these to be explicit, rather than relying on undefined.
> 
> {audio: false, video: true}
> 
> That allows you to remove the locals on lines 24 and 25 and just use |value|
> directly, and makes the generator values clearer.

Done.

https://codereview.chromium.org/1448203002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-requestData.html
(right):

https://codereview.chromium.org/1448203002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-requestData.html:5:

On 2015/11/20 14:05:02, Peter Beverloo wrote:
> dito re: comment

Done.

https://codereview.chromium.org/1448203002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-requestData.html:28:
recorderOnDataAvailable = this.step_func_done(function(event) {
On 2015/11/20 14:05:02, Peter Beverloo wrote:
> dito re: constness

Done.

https://codereview.chromium.org/1448203002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-requestData.html:80:
[["video-only",  {video:true}],
On 2015/11/20 14:05:02, Peter Beverloo wrote:
> dito re: values

Done.

[ajose, 2015-11-20 22:28:27]

The CQ bit was checked by ajose@chromium.org

[ajose, 2015-11-20 22:28:27]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1448203002/#ps160001
(title: "peter@'s comments")

[commit-bot: I haz the power, 2015-11-20 22:29:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1448203002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1448203002/160001

[commit-bot: I haz the power, 2015-11-20 22:51:21]

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

[commit-bot: I haz the power, 2015-11-20 22:51:22]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, no build URL)

[ajose, 2015-11-20 23:12:03]

The CQ bit was checked by ajose@chromium.org

[commit-bot: I haz the power, 2015-11-20 23:12:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1448203002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1448203002/160001

[commit-bot: I haz the power, 2015-11-21 00:39:29]

Committed patchset #5 (id:160001)

[commit-bot: I haz the power, 2015-11-21 00:40:54]

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