MediaRecorder: fire onError if the amount of tracks of the MediaStream varies

MediaRecorder: fire onError if the amount of tracks of the MediaStream varies

MediaRecorder is supposed to fire an Error event if the amount
of tracks of the associated MediaStream varies. This CL adds
monitoring of said amount and Error firing.

There's no need to stop the MediaRecorder: adding a Track
would not start automatically recording it, and removing it
guarantees it would not be recorded not recording. Moreover,
the spec does not say if MRecorder should stop if the
#Tracks varies, and also instructs to throw an Exception,
which is not possible since the error is asynchronous.

Also this CL removes superfluous test variables.

BUG=545156
Committed: https://crrev.com/69b654089f6315378b508330044f39dd56bb85e1
Cr-Commit-Position: refs/heads/master@{#369333}

[mcasas, 2016-01-11 22:36:07]

Description was changed from

==========
MediaRecorder: fire onError if the amount of tracks of the MediaStream varies

MediaRecorder is supposed to fire an Error event if the amount of
tracks of the associated MediaStream varies. This CL adds monitoring
of said amount and Error throwing.

Note that the spec does not say if MRecorder should stop if the
says to throw an Exception, but that's not possible since the
error is asynchronous.

BUG=545156
==========

to

==========
MediaRecorder: fire onError if the amount of tracks of the MediaStream varies

MediaRecorder is supposed to fire an Error event if the amount 
of tracks of the associated MediaStream varies. This CL adds 
monitoring of said amount and Error throwing.

There's no need to stop the MediaRecorder: adding a Track
would not start automatically recording it, and removing it
guarantees its not recording, so I think best to leave to the
JS app. 

Note that the spec does not say if MRecorder should stop if the
says to throw an Exception, but that's not possible since the
error is asynchronous.

Also this CL removes superfluous test variables.

BUG=545156
==========

[mcasas, 2016-01-11 22:41:29]

Description was changed from

==========
MediaRecorder: fire onError if the amount of tracks of the MediaStream varies

MediaRecorder is supposed to fire an Error event if the amount 
of tracks of the associated MediaStream varies. This CL adds 
monitoring of said amount and Error throwing.

There's no need to stop the MediaRecorder: adding a Track
would not start automatically recording it, and removing it
guarantees its not recording, so I think best to leave to the
JS app. 

Note that the spec does not say if MRecorder should stop if the
says to throw an Exception, but that's not possible since the
error is asynchronous.

Also this CL removes superfluous test variables.

BUG=545156
==========

to

==========
MediaRecorder: fire onError if the amount of tracks of the MediaStream varies

MediaRecorder is supposed to fire an Error event if the amount 
of tracks of the associated MediaStream varies. This CL adds 
monitoring of said amount and Error firing.

There's no need to stop the MediaRecorder: adding a Track
would not start automatically recording it, and removing it
guarantees its not recording, so I think best to leave to the
JS app. 

Note that the spec does not say if MRecorder should stop if the
says to throw an Exception, but that's not possible since the
error is asynchronous.

Also this CL removes superfluous test variables.

BUG=545156
==========

[mcasas, 2016-01-11 23:08:59]

Patchset #1 (id:1) has been deleted

[mcasas, 2016-01-11 23:09:14]

Patchset #1 (id:20001) has been deleted

[mcasas, 2016-01-12 01:22:42]

Description was changed from

==========
MediaRecorder: fire onError if the amount of tracks of the MediaStream varies

MediaRecorder is supposed to fire an Error event if the amount 
of tracks of the associated MediaStream varies. This CL adds 
monitoring of said amount and Error firing.

There's no need to stop the MediaRecorder: adding a Track
would not start automatically recording it, and removing it
guarantees its not recording, so I think best to leave to the
JS app. 

Note that the spec does not say if MRecorder should stop if the
says to throw an Exception, but that's not possible since the
error is asynchronous.

Also this CL removes superfluous test variables.

BUG=545156
==========

to

==========
MediaRecorder: fire onError if the amount of tracks of the MediaStream varies

MediaRecorder is supposed to fire an Error event if the amount 
of tracks of the associated MediaStream varies. This CL adds 
monitoring of said amount and Error firing.

There's no need to stop the MediaRecorder: adding a Track
would not start automatically recording it, and removing it
guarantees its not recording, so I think best to leave to the
JS app. 

Note that the spec does not say if MRecorder should stop if 
the #Tracks varies, and also instructs to throw an Exception,
which is not possible since the error is asynchronous.

Also this CL removes superfluous test variables.

BUG=545156
==========

[mcasas, 2016-01-12 03:14:11]

Description was changed from

==========
MediaRecorder: fire onError if the amount of tracks of the MediaStream varies

MediaRecorder is supposed to fire an Error event if the amount 
of tracks of the associated MediaStream varies. This CL adds 
monitoring of said amount and Error firing.

There's no need to stop the MediaRecorder: adding a Track
would not start automatically recording it, and removing it
guarantees its not recording, so I think best to leave to the
JS app. 

Note that the spec does not say if MRecorder should stop if 
the #Tracks varies, and also instructs to throw an Exception,
which is not possible since the error is asynchronous.

Also this CL removes superfluous test variables.

BUG=545156
==========

to

==========
MediaRecorder: fire onError if the amount of tracks of the MediaStream varies

MediaRecorder is supposed to fire an Error event if the amount 
of tracks of the associated MediaStream varies. This CL adds 
monitoring of said amount and Error firing.

There's no need to stop the MediaRecorder: adding a Track
would not start automatically recording it, and removing it
guarantees it would not be recorded not recording. Moreover,
the spec does not say if MRecorder should stop if the 
#Tracks varies, and also instructs to throw an Exception,
which is not possible since the error is asynchronous.

Also this CL removes superfluous test variables.

BUG=545156
==========

[mcasas, 2016-01-12 03:14:30]

Patchset #1 (id:40001) has been deleted

[mcasas, 2016-01-12 16:58:09]

mcasas@chromium.org changed reviewers:
+ cpaulin@chromium.org, peter@chromium.org, phoglund@chromium.org

[mcasas, 2016-01-12 16:58:10]

peter@: WebKit/.../mediarecorder/ files
phoglund@: test files
 (cpaulin@: FYI/PTAL).

[phoglund_chromium, 2016-01-13 08:10:31]

lgtm, but someone should look into whether testStartStopAndRecorderState is
actually doing anything.

https://codereview.chromium.org/1578973002/diff/60001/content/test/data/media...
File content/test/data/media/mediarecorder_test.html (left):

https://codereview.chromium.org/1578973002/diff/60001/content/test/data/media...
content/test/data/media/mediarecorder_test.html:154: assertEquals('inactive',
theRecorder.state);
This could not possibly have worked - theRecorder was never assigned! Is this
test even running? When I look the .cc I see this test is called at least, so it
should be running.

[cpaulin (no longer in chrome), 2016-01-13 17:59:35]

On 2016/01/13 08:10:31, phoglund_chrome wrote:
> lgtm, but someone should look into whether testStartStopAndRecorderState is
> actually doing anything.
> 
>
https://codereview.chromium.org/1578973002/diff/60001/content/test/data/media...
> File content/test/data/media/mediarecorder_test.html (left):
> 
>
https://codereview.chromium.org/1578973002/diff/60001/content/test/data/media...
> content/test/data/media/mediarecorder_test.html:154: assertEquals('inactive',
> theRecorder.state);
> This could not possibly have worked - theRecorder was never assigned! Is this
> test even running? When I look the .cc I see this test is called at least, so
it
> should be running.

Wow, it was right in CL 1436043002 and it was garbled when merged with event
checks in CL 1524363002
Will fix it

[cpaulin (no longer in chrome), 2016-01-13 18:05:02]

On 2016/01/13 17:59:35, cpaulin wrote:
> On 2016/01/13 08:10:31, phoglund_chrome wrote:
> > lgtm, but someone should look into whether testStartStopAndRecorderState is
> > actually doing anything.
> > 
> >
>
https://codereview.chromium.org/1578973002/diff/60001/content/test/data/media...
> > File content/test/data/media/mediarecorder_test.html (left):
> > 
> >
>
https://codereview.chromium.org/1578973002/diff/60001/content/test/data/media...
> > content/test/data/media/mediarecorder_test.html:154:
assertEquals('inactive',
> > theRecorder.state);
> > This could not possibly have worked - theRecorder was never assigned! Is
this
> > test even running? When I look the .cc I see this test is called at least,
so
> it
> > should be running.
> 
> Wow, it was right in CL 1436043002 and it was garbled when merged with event
> checks in CL 1524363002
> Will fix it

Ok, I see you fixed it already thanks

[cpaulin (no longer in chrome), 2016-01-13 18:08:53]

lgtm

[cpaulin (no longer in chrome), 2016-01-13 18:08:54]

lgtm

[Peter Beverloo, 2016-01-13 18:38:01]

https://codereview.chromium.org/1578973002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp (right):

https://codereview.chromium.org/1578973002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:210:
onError("Amount of tracks in MediaStream has changed.");
This fires an `onerror` event each time writeData() is called after the number
of tracks has changed. There's also the case where a track was removed, and
another track was added - today that would mute the error again.

How useful are the repeated `onerror` events if we don't stop the recording? In
most APIs, receiving an `onerror` event means that the functionality stops (i.e.
notifications or XHR).

https://codereview.chromium.org/1578973002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:210:
onError("Amount of tracks in MediaStream has changed.");
Will you address the specification bug?

[mcasas, 2016-01-13 21:50:41]

peter@ PTAL

https://codereview.chromium.org/1578973002/diff/60001/content/test/data/media...
File content/test/data/media/mediarecorder_test.html (left):

https://codereview.chromium.org/1578973002/diff/60001/content/test/data/media...
content/test/data/media/mediarecorder_test.html:154: assertEquals('inactive',
theRecorder.state);
On 2016/01/13 08:10:31, phoglund_chrome wrote:
> This could not possibly have worked - theRecorder was never assigned! Is this
> test even running? When I look the .cc I see this test is called at least, so
it
> should be running.

Acknowledged.

https://codereview.chromium.org/1578973002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp (right):

https://codereview.chromium.org/1578973002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:210:
onError("Amount of tracks in MediaStream has changed.");
On 2016/01/13 18:38:01, Peter Beverloo wrote:
> This fires an `onerror` event each time writeData() is called after the number
> of tracks has changed. There's also the case where a track was removed, and
> another track was added - today that would mute the error again.

Yes, I should update m_streamAmountOfTracks after the
first error is fired. Done. 

> 
> How useful are the repeated `onerror` events if we don't stop the recording?
In
> most APIs, receiving an `onerror` event means that the functionality stops
(i.e.
> notifications or XHR).

So I was thinking about it, and wrote in the CL description,
that what would be the best way to proceed towards the
user  when a Track is added or Removed, since anyway 
the content would not be recorded/ sotp being recorded,
respectively, automatically? I think the MR should keep
recording whatever it has and let the JS decide what's the
appropriate course of action. The Spec is quite fuzzy 
about it, and it only mentions a DOMException in case
Tracks are added or removed... which can't be since is
an asynchronous condition. Like you say below, the Spec
should be updated (which falls on me again!)

So the point of this CL is to, at least, fire an Error if 
something like this happen...

https://codereview.chromium.org/1578973002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:210:
onError("Amount of tracks in MediaStream has changed.");
On 2016/01/13 18:38:01, Peter Beverloo wrote:
> Will you address the specification bug?

Acknowledged.

[Peter Beverloo, 2016-01-13 22:05:20]

lgtm

In the long run we probably want a more stable mechanism, e.g. maintaining the
exact set of tracks in //content or //media and firing the error event when a
modification occurs (not just the # of tracks when writing data, as the
add/remove situation is still possible now)

[mcasas, 2016-01-13 22:15:50]

The CQ bit was checked by mcasas@chromium.org

[mcasas, 2016-01-13 22:15:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
phoglund@chromium.org, cpaulin@chromium.org
Link to the patchset: https://codereview.chromium.org/1578973002/#ps80001
(title: "peter@ comments")

[commit-bot: I haz the power, 2016-01-13 22:18:03]

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

[commit-bot: I haz the power, 2016-01-13 23:39:14]

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

[commit-bot: I haz the power, 2016-01-13 23:39:15]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_TIMED_OUT, no
build URL)
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_TIMED_OUT,
no build URL)
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_TIMED_OUT,
no build URL)
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_TIMED_OUT, no
build URL)
  android_compile_dbg on tryserver.chromium.android (JOB_TIMED_OUT, no build
URL)
  cast_shell_android on tryserver.chromium.android (JOB_TIMED_OUT, no build URL)
  linux_android_rel_ng on tryserver.chromium.android (JOB_TIMED_OUT, no build
URL)
  cast_shell_linux on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
  win8_chromium_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)

[mcasas, 2016-01-14 01:08:39]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2016-01-14 01:10:47]

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

[commit-bot: I haz the power, 2016-01-14 02:17:44]

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

[commit-bot: I haz the power, 2016-01-14 02:17:45]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mcasas, 2016-01-14 03:56:03]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2016-01-14 03:58:42]

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

[commit-bot: I haz the power, 2016-01-14 05:09:52]

Description was changed from

==========
MediaRecorder: fire onError if the amount of tracks of the MediaStream varies

MediaRecorder is supposed to fire an Error event if the amount 
of tracks of the associated MediaStream varies. This CL adds 
monitoring of said amount and Error firing.

There's no need to stop the MediaRecorder: adding a Track
would not start automatically recording it, and removing it
guarantees it would not be recorded not recording. Moreover,
the spec does not say if MRecorder should stop if the 
#Tracks varies, and also instructs to throw an Exception,
which is not possible since the error is asynchronous.

Also this CL removes superfluous test variables.

BUG=545156
==========

to

==========
MediaRecorder: fire onError if the amount of tracks of the MediaStream varies

MediaRecorder is supposed to fire an Error event if the amount 
of tracks of the associated MediaStream varies. This CL adds 
monitoring of said amount and Error firing.

There's no need to stop the MediaRecorder: adding a Track
would not start automatically recording it, and removing it
guarantees it would not be recorded not recording. Moreover,
the spec does not say if MRecorder should stop if the 
#Tracks varies, and also instructs to throw an Exception,
which is not possible since the error is asynchronous.

Also this CL removes superfluous test variables.

BUG=545156
==========

[commit-bot: I haz the power, 2016-01-14 05:09:53]

Committed patchset #2 (id:80001)

[commit-bot: I haz the power, 2016-01-14 05:11:34]

Description was changed from

==========
MediaRecorder: fire onError if the amount of tracks of the MediaStream varies

MediaRecorder is supposed to fire an Error event if the amount 
of tracks of the associated MediaStream varies. This CL adds 
monitoring of said amount and Error firing.

There's no need to stop the MediaRecorder: adding a Track
would not start automatically recording it, and removing it
guarantees it would not be recorded not recording. Moreover,
the spec does not say if MRecorder should stop if the 
#Tracks varies, and also instructs to throw an Exception,
which is not possible since the error is asynchronous.

Also this CL removes superfluous test variables.

BUG=545156
==========

to

==========
MediaRecorder: fire onError if the amount of tracks of the MediaStream varies

MediaRecorder is supposed to fire an Error event if the amount
of tracks of the associated MediaStream varies. This CL adds
monitoring of said amount and Error firing.

There's no need to stop the MediaRecorder: adding a Track
would not start automatically recording it, and removing it
guarantees it would not be recorded not recording. Moreover,
the spec does not say if MRecorder should stop if the
#Tracks varies, and also instructs to throw an Exception,
which is not possible since the error is asynchronous.

Also this CL removes superfluous test variables.

BUG=545156

Committed: https://crrev.com/69b654089f6315378b508330044f39dd56bb85e1
Cr-Commit-Position: refs/heads/master@{#369333}
==========

[commit-bot: I haz the power, 2016-01-14 05:11:35]

Patchset 2 (id:??) landed as
https://crrev.com/69b654089f6315378b508330044f39dd56bb85e1
Cr-Commit-Position: refs/heads/master@{#369333}