Add simple test for VideoTrack.stop().

Add simple test for VideoTrack.stop().

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

[phoglund_chromium, 2013-11-14 09:01:18]

https://codereview.chromium.org/68263016/diff/30001/content/test/data/media/g...
File content/test/data/media/getusermedia.html (right):

https://codereview.chromium.org/68263016/diff/30001/content/test/data/media/g...
content/test/data/media/getusermedia.html:44: detectVideoIn('local-view',
stopVideoTrack);
This doesn't read very intuitively since IMO it looks like you're detecting and
stopping at the same time. You'd want it to read detectVideoIn(...,
andThenStopVideoTrack). Perhaps that is a bit silly so I'd write a comment 'Wait
for video to play, then stop it and wait for it to stop playing', or something
like that.

https://codereview.chromium.org/68263016/diff/30001/content/test/data/media/g...
content/test/data/media/getusermedia.html:74: waitForVideoToStop('local-view');
You should be able to swap this statement and the next, right? I think that's
read better.

https://codereview.chromium.org/68263016/diff/30001/content/test/data/media/w...
File content/test/data/media/webrtc_test_utilities.js (right):

https://codereview.chromium.org/68263016/diff/30001/content/test/data/media/w...
content/test/data/media/webrtc_test_utilities.js:37: if (old_pixels.length ==
pixels.length &&
Why is this length check necessary?

https://codereview.chromium.org/68263016/diff/30001/content/test/data/media/w...
content/test/data/media/webrtc_test_utilities.js:46: function
detectVideoStopped(videoElementName, callback) {
You should be able to re-use the old function without adding too much
complexity. Imagine you extract a helper function which looks like the two
detection functions but which takes an additional function 'predicate' as an
argument. The predicate should accept two args old_pixels and new_pixels and
return a boolean. Then you can pass in isVideoPlaying in the first one and the
inverse in the second one.

https://codereview.chromium.org/68263016/diff/30001/content/test/data/media/w...
content/test/data/media/webrtc_test_utilities.js:62: }, 300);
Don't see why this should be different from the other detector.

https://codereview.chromium.org/68263016/diff/30001/content/test/data/media/w...
content/test/data/media/webrtc_test_utilities.js:98: //This very basic video
verification algorithm will be satisfied if any
Nit: space between // and letters

https://codereview.chromium.org/68263016/diff/30001/content/test/data/media/w...
content/test/data/media/webrtc_test_utilities.js:100: function
isVideoPlaying(pixels, previouse_pixels) {
Nit: previous_pixels

[perkj_chrome, 2013-11-14 13:29:07]

PTAL

https://codereview.chromium.org/68263016/diff/30001/content/test/data/media/g...
File content/test/data/media/getusermedia.html (right):

https://codereview.chromium.org/68263016/diff/30001/content/test/data/media/g...
content/test/data/media/getusermedia.html:44: detectVideoIn('local-view',
stopVideoTrack);
On 2013/11/14 09:01:18, phoglund wrote:
> This doesn't read very intuitively since IMO it looks like you're detecting
and
> stopping at the same time. You'd want it to read detectVideoIn(...,
> andThenStopVideoTrack). Perhaps that is a bit silly so I'd write a comment
'Wait
> for video to play, then stop it and wait for it to stop playing', or something
> like that.

Done.

https://codereview.chromium.org/68263016/diff/30001/content/test/data/media/g...
content/test/data/media/getusermedia.html:74: waitForVideoToStop('local-view');
On 2013/11/14 09:01:18, phoglund wrote:
> You should be able to swap this statement and the next, right? I think that's
> read better.

Done.

https://codereview.chromium.org/68263016/diff/30001/content/test/data/media/w...
File content/test/data/media/webrtc_test_utilities.js (right):

https://codereview.chromium.org/68263016/diff/30001/content/test/data/media/w...
content/test/data/media/webrtc_test_utilities.js:37: if (old_pixels.length ==
pixels.length &&
On 2013/11/14 09:01:18, phoglund wrote:
> Why is this length check necessary?
Its a prerequisite for isVideoPlaying that they are the same lenght.
First time old_pixel.length()= 0;

https://codereview.chromium.org/68263016/diff/30001/content/test/data/media/w...
content/test/data/media/webrtc_test_utilities.js:46: function
detectVideoStopped(videoElementName, callback) {
On 2013/11/14 09:01:18, phoglund wrote:
> You should be able to re-use the old function without adding too much
> complexity. Imagine you extract a helper function which looks like the two
> detection functions but which takes an additional function 'predicate' as an
> argument. The predicate should accept two args old_pixels and new_pixels and
> return a boolean. Then you can pass in isVideoPlaying in the first one and the
> inverse in the second one.

Done.

https://codereview.chromium.org/68263016/diff/30001/content/test/data/media/w...
content/test/data/media/webrtc_test_utilities.js:62: }, 300);
On 2013/11/14 09:01:18, phoglund wrote:
> Don't see why this should be different from the other detector.

We can increase it a bit. But it it is lower that the frame rate of the fake
video capture device we will detect false video track stop.

https://codereview.chromium.org/68263016/diff/30001/content/test/data/media/w...
content/test/data/media/webrtc_test_utilities.js:98: //This very basic video
verification algorithm will be satisfied if any
On 2013/11/14 09:01:18, phoglund wrote:
> Nit: space between // and letters

Done.

https://codereview.chromium.org/68263016/diff/30001/content/test/data/media/w...
content/test/data/media/webrtc_test_utilities.js:100: function
isVideoPlaying(pixels, previouse_pixels) {
On 2013/11/14 09:01:18, phoglund wrote:
> Nit: previous_pixels

Done.

[phoglund_chromium, 2013-11-14 13:41:05]

One last comment.

https://codereview.chromium.org/68263016/diff/30001/content/test/data/media/w...
File content/test/data/media/webrtc_test_utilities.js (right):

https://codereview.chromium.org/68263016/diff/30001/content/test/data/media/w...
content/test/data/media/webrtc_test_utilities.js:37: if (old_pixels.length ==
pixels.length &&
On 2013/11/14 13:29:07, perkj wrote:
> On 2013/11/14 09:01:18, phoglund wrote:
> > Why is this length check necessary?
> Its a prerequisite for isVideoPlaying that they are the same lenght.
> First time old_pixel.length()= 0;
Ok. Perhaps we should bake that into isVideoPlaying. Just add a check in that
function that if pixels.length != old_pixels.length, return false. I think that
makes sense for the function since it cannot tell that video is playing given
those circumstances. Moving that check into isVideoPlaying simplifies this
fairly complicated algorithm.

[perkj_chrome, 2013-11-14 14:58:43]

ptal

[phoglund_chromium, 2013-11-14 15:33:12]

lgtm

[commit-bot: I haz the power, 2013-11-14 16:41:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/perkj@chromium.org/68263016/170001

[commit-bot: I haz the power, 2013-11-14 22:09:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/perkj@chromium.org/68263016/170001

[commit-bot: I haz the power, 2013-11-15 01:57:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/perkj@chromium.org/68263016/170001

[commit-bot: I haz the power, 2013-11-15 04:06:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/perkj@chromium.org/68263016/170001

[commit-bot: I haz the power, 2013-11-15 06:29:00]

Retried try job too often on win_rel for step(s) nacl_integration
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-15 08:21:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/perkj@chromium.org/68263016/170001

[commit-bot: I haz the power, 2013-11-15 10:05:17]

Retried try job too often on win_rel for step(s) nacl_integration
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-18 10:02:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/perkj@chromium.org/68263016/170001

[commit-bot: I haz the power, 2013-11-18 11:16:27]

Change committed as 235698