MSE: Throw TypeError instead of InvalidAccessError per spec update

MSE: Throw TypeError instead of InvalidAccessError per spec update

Conforms to https://github.com/w3c/media-source/pull/47 spec change for
certain exceptional conditions, leaving a couple pieces for later changes:
1) bug 607372 "Comply with forcing appendMode to remain 'sequence' for MSE mp3 and adts stream parsers"
2) bug 644863 "Update TrackDefault and TrackDefaultList to use TypeError correctly"

BUG=623781
TEST=updated http/tests/media/media-source tests, and current related upstream w3c MSE tests

Committed: https://crrev.com/bf5ee5415899e8e8c9e3ac2385d7092bbd11a3d8
Cr-Commit-Position: refs/heads/master@{#417107}

[wolenetz, 2016-09-07 02:03:56]

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

[commit-bot: I haz the power, 2016-09-07 02:04:19]

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

[wolenetz, 2016-09-07 02:09:07]

wolenetz@chromium.org changed reviewers:
+ chcunningham@chromium.org

[wolenetz, 2016-09-07 02:09:08]

Please take a look.

cc+=foolip@ as FYI around the addSourceBuffer(null) test case.

With this CL, Chromium also passes the TypeError-related upstream w3c
web-platform-tests MSE tests (except, at the moment, the upstream
addSourceBuffer(null) test is still expecting TypeError, not NotSupportedError).

[commit-bot: I haz the power, 2016-09-07 03:24:31]

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

[commit-bot: I haz the power, 2016-09-07 03:24:31]

Dry run: This issue passed the CQ dry run.

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

On 2016/09/07 02:09:08, wolenetz wrote:
> ...(except, at the moment, the upstream
> addSourceBuffer(null) test is still expecting TypeError, not
NotSupportedError).

fyi - I have a PR in review upstream to fix that addSourceBuffer(null) test to
expect NotSupportedError: https://github.com/w3c/web-platform-tests/pull/3653

[wolenetz, 2016-09-07 18:33:41]

Description was changed from

==========
MSE: Throw TypeError instead of InvalidAccessError per spec update

Conforms to https://github.com/w3c/media-source/pull/47 spec change for
certain exceptional conditions.

BUG=623781
TEST=updated http/tests/media/media-source tests
==========

to

==========
MSE: Throw TypeError instead of InvalidAccessError per spec update

Conforms to https://github.com/w3c/media-source/pull/47 spec change for
certain exceptional conditions.

BUG=623781
TEST=updated http/tests/media/media-source tests, and current related upstream
w3c MSE tests % addSourceBuffer(null)
==========

[chcunningham, 2016-09-07 18:36:06]

Looks pretty good. I think you're missing 3 cases in that pull request to emit
TypeError.

1. setting mode: generate-timestamps-flag = true and new mode = sgements
2. TrackDefault - language is not an empty string and language is not a BCP 47
tag
3. TrackDefaultList - trackDefaults contains 2+ objects with the same type and
same bytestreamTrackID

https://codereview.chromium.org/2315113002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-remove.html
(right):

https://codereview.chromium.org/2315113002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-remove.html:16:
mediaSource.duration = 10;
Remind me again, why were these added?

Edit: answered offline - you have to set duration to test the case you're
looking for. Failing to set duration will also cause a TypeError, so its a false
positive.

https://codereview.chromium.org/2315113002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediasource/MediaSource.cpp (right):

https://codereview.chromium.org/2315113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasource/MediaSource.cpp:579: if (start < 0
|| start > end) {
Note that we let start == end here. That allolwed by the spec, just calling it
to check that we're ok with this

https://codereview.chromium.org/2315113002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right):

https://codereview.chromium.org/2315113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:167: void
SourceBuffer::setMode(const AtomicString& newMode, ExceptionState&
exceptionState)
Not covered: If generate timestamps flag equals true and new mode equals
"segments", then throw a TypeError exception and abort these steps.

Also, not tested?

https://codereview.chromium.org/2315113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:274: if (start <
0 || start >= m_appendWindowEnd) {
Weird that we don't check NaN for start like we do end? Spec doesn't seem to
mention it.

[foolip, 2016-09-07 19:42:49]

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

[foolip, 2016-09-07 19:42:49]

Thanks for the context, left a comment you can ignore :)

https://codereview.chromium.org/2315113002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-addsourcebuffer.html
(right):

https://codereview.chromium.org/2315113002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-addsourcebuffer.html:29:
assert_throws({name: "TypeError"},
This does what the spec asks, but I do wonder why the spec handled the empty
string separately instead of just letting NotSupportedError be thrown. If the
check is already implemented in other engines then I wouldn't go about changes
the spec, though.

[wolenetz, 2016-09-07 21:05:53]

Description was changed from

==========
MSE: Throw TypeError instead of InvalidAccessError per spec update

Conforms to https://github.com/w3c/media-source/pull/47 spec change for
certain exceptional conditions.

BUG=623781
TEST=updated http/tests/media/media-source tests, and current related upstream
w3c MSE tests % addSourceBuffer(null)
==========

to

==========
MSE: Throw TypeError instead of InvalidAccessError per spec update

Conforms to https://github.com/w3c/media-source/pull/47 spec change for
certain exceptional conditions, leaving a couple pieces for later changes:
1) bug 607372 "Comply with forcing appendMode to remain 'sequence' for MSE mp3
and adts stream parsers"
2) bug 644863 "Update TrackDefault and TrackDefaultList to use TypeError
correctly"

BUG=623781
TEST=updated http/tests/media/media-source tests, and current related upstream
w3c MSE tests
==========

[wolenetz, 2016-09-07 21:09:27]

Thanks for reviews.
chcunningham@, here are inline replies to your previous comment:
> 1. setting mode: generate-timestamps-flag = true and new mode = sgements

Expected to not be fixed by this CL. CL description updated. Bug 607372 tracks
fixing this.

> 2. TrackDefault - language is not an empty string and language is not a BCP 47
tag
> 3. TrackDefaultList - trackDefaults contains 2+ objects with the same type and
same bytestreamTrackID

Expected to not be fixed by this CL. CL description updated. Bug 644863 tracks
fixing TrackDefault{List} w.r.t. TypeError.

https://codereview.chromium.org/2315113002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-addsourcebuffer.html
(right):

https://codereview.chromium.org/2315113002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-addsourcebuffer.html:29:
assert_throws({name: "TypeError"},
On 2016/09/07 19:42:49, foolip wrote:
> This does what the spec asks, but I do wonder why the spec handled the empty
> string separately instead of just letting NotSupportedError be thrown. If the
> check is already implemented in other engines then I wouldn't go about changes
> the spec, though.

Good point. I believe this is implemented in other UAs now (we'll see if that's
so in the upcoming implementation report). I also believe this was to enforce a
bare minimum bar that *something* must be in the string passed to
addSourceBuffer(). As we've seen, other UA's have been particularly lenient in
allowing underspecified mimetype/codec strings in both isTypeSupported() and
addSourceBuffer() vs Chrome's more strict take on it ala EME, so at least we
have this check :)

https://codereview.chromium.org/2315113002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-remove.html
(right):

https://codereview.chromium.org/2315113002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-remove.html:16:
mediaSource.duration = 10;
On 2016/09/07 18:36:05, chcunningham wrote:
> Remind me again, why were these added?
> 
> Edit: answered offline - you have to set duration to test the case you're
> looking for. Failing to set duration will also cause a TypeError, so its a
false
> positive.

Acknowledged.

https://codereview.chromium.org/2315113002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediasource/MediaSource.cpp (right):

https://codereview.chromium.org/2315113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasource/MediaSource.cpp:579: if (start < 0
|| start > end) {
On 2016/09/07 18:36:05, chcunningham wrote:
> Note that we let start == end here. That allolwed by the spec, just calling it
> to check that we're ok with this

This is intended. It allows an app to direct the UA to restrict seeks to exactly
such a time (of course, extended earlier and/or later if the current buffered
ranges start and end are less than this time or greater than this time,
respectively.
Note also that
https://www.w3.org/TR/html51/semantics-embedded-content.html#normalized-timer...
allows a range to exist with start == end.

https://codereview.chromium.org/2315113002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right):

https://codereview.chromium.org/2315113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:167: void
SourceBuffer::setMode(const AtomicString& newMode, ExceptionState&
exceptionState)
On 2016/09/07 18:36:06, chcunningham wrote:
> Not covered: If generate timestamps flag equals true and new mode equals
> "segments", then throw a TypeError exception and abort these steps.
> 
> Also, not tested?

This is expected. Fixing this in Chrome is tracked separately by
https://bugs.chromium.org/p/chromium/issues/detail?id=607372

Test of this is already in upstream w3c mse tests: 

Base cases:
*
https://github.com/w3c/web-platform-tests/blob/master/media-source/mediasourc...
(Chrome, even with this CL, fails 1/2 of the subtests: "addSourceBuffer() sets
SourceBuffer.mode to 'sequence' when the generate timestamps flag is true")
*
https://github.com/w3c/web-platform-tests/blob/master/media-source/mediasourc...
(Chrome, with this CL (and I think without it) passes all these subtests)

Specific coverage for the spec item you referenced:
*
https://github.com/w3c/web-platform-tests/blob/master/media-source/mediasourc...
(Chrome, even with this CL, fails 2/2 of the subtests).

https://codereview.chromium.org/2315113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:274: if (start <
0 || start >= m_appendWindowEnd) {
On 2016/09/07 18:36:05, chcunningham wrote:
> Weird that we don't check NaN for start like we do end? Spec doesn't seem to
> mention it.

We do and spec does. That's enforced by IDL bindings (it's a double, not an
unrestricted double like end.) IDL is:
 [RaisesException=Setter] attribute double appendWindowStart;

vs end:
 [RaisesException=Setter] attribute unrestricted double appendWindowEnd;

And we have test coverage for this in
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test...

[wolenetz, 2016-09-07 21:27:42]

On 2016/09/07 21:09:27, wolenetz wrote:
> Thanks for reviews.
> chcunningham@, here are inline replies to your previous comment:
> > 1. setting mode: generate-timestamps-flag = true and new mode = sgements
> 
> Expected to not be fixed by this CL. CL description updated. Bug 607372 tracks
> fixing this.
> 
> > 2. TrackDefault - language is not an empty string and language is not a BCP
47
> tag
> > 3. TrackDefaultList - trackDefaults contains 2+ objects with the same type
and
> same bytestreamTrackID
> 
> Expected to not be fixed by this CL. CL description updated. Bug 644863 tracks
> fixing TrackDefault{List} w.r.t. TypeError.
> 
>
https://codereview.chromium.org/2315113002/diff/20001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-addsourcebuffer.html
> (right):
> 
>
https://codereview.chromium.org/2315113002/diff/20001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-addsourcebuffer.html:29:
> assert_throws({name: "TypeError"},
> On 2016/09/07 19:42:49, foolip wrote:
> > This does what the spec asks, but I do wonder why the spec handled the empty
> > string separately instead of just letting NotSupportedError be thrown. If
the
> > check is already implemented in other engines then I wouldn't go about
changes
> > the spec, though.
> 
> Good point. I believe this is implemented in other UAs now (we'll see if
that's
> so in the upcoming implementation report). I also believe this was to enforce
a
> bare minimum bar that *something* must be in the string passed to
> addSourceBuffer(). As we've seen, other UA's have been particularly lenient in
> allowing underspecified mimetype/codec strings in both isTypeSupported() and
> addSourceBuffer() vs Chrome's more strict take on it ala EME, so at least we
> have this check :)
> 
>
https://codereview.chromium.org/2315113002/diff/20001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-remove.html
> (right):
> 
>
https://codereview.chromium.org/2315113002/diff/20001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-remove.html:16:
> mediaSource.duration = 10;
> On 2016/09/07 18:36:05, chcunningham wrote:
> > Remind me again, why were these added?
> > 
> > Edit: answered offline - you have to set duration to test the case you're
> > looking for. Failing to set duration will also cause a TypeError, so its a
> false
> > positive.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/2315113002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/mediasource/MediaSource.cpp (right):
> 
>
https://codereview.chromium.org/2315113002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/mediasource/MediaSource.cpp:579: if (start <
0
> || start > end) {
> On 2016/09/07 18:36:05, chcunningham wrote:
> > Note that we let start == end here. That allolwed by the spec, just calling
it
> > to check that we're ok with this
> 
> This is intended. It allows an app to direct the UA to restrict seeks to
exactly
> such a time (of course, extended earlier and/or later if the current buffered
> ranges start and end are less than this time or greater than this time,
> respectively.
> Note also that
>
https://www.w3.org/TR/html51/semantics-embedded-content.html#normalized-timer...
> allows a range to exist with start == end.
> 
>
https://codereview.chromium.org/2315113002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right):
> 
>
https://codereview.chromium.org/2315113002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:167: void
> SourceBuffer::setMode(const AtomicString& newMode, ExceptionState&
> exceptionState)
> On 2016/09/07 18:36:06, chcunningham wrote:
> > Not covered: If generate timestamps flag equals true and new mode equals
> > "segments", then throw a TypeError exception and abort these steps.
> > 
> > Also, not tested?
> 
> This is expected. Fixing this in Chrome is tracked separately by
> https://bugs.chromium.org/p/chromium/issues/detail?id=607372
> 
> Test of this is already in upstream w3c mse tests: 
> 
> Base cases:
> *
>
https://github.com/w3c/web-platform-tests/blob/master/media-source/mediasourc...
> (Chrome, even with this CL, fails 1/2 of the subtests: "addSourceBuffer() sets
> SourceBuffer.mode to 'sequence' when the generate timestamps flag is true")
> *
>
https://github.com/w3c/web-platform-tests/blob/master/media-source/mediasourc...
> (Chrome, with this CL (and I think without it) passes all these subtests)
> 
> Specific coverage for the spec item you referenced:
> *
>
https://github.com/w3c/web-platform-tests/blob/master/media-source/mediasourc...
> (Chrome, even with this CL, fails 2/2 of the subtests).
> 
>
https://codereview.chromium.org/2315113002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:274: if (start
<
> 0 || start >= m_appendWindowEnd) {
> On 2016/09/07 18:36:05, chcunningham wrote:
> > Weird that we don't check NaN for start like we do end? Spec doesn't seem to
> > mention it.
> 
> We do and spec does. That's enforced by IDL bindings (it's a double, not an
> unrestricted double like end.) IDL is:
>  [RaisesException=Setter] attribute double appendWindowStart;
> 
> vs end:
>  [RaisesException=Setter] attribute unrestricted double appendWindowEnd;
> 
> And we have test coverage for this in
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test...

chcunningham@, since I need to leave a bit early today and this CL will fix a
bunch of glaring compliance failures, would you please CQ this ASAP if it LGTY %
no other comments/fixes/nits?

[chcunningham, 2016-09-07 22:53:23]

lgtm

[chcunningham, 2016-09-07 22:53:28]

The CQ bit was checked by chcunningham@chromium.org

[commit-bot: I haz the power, 2016-09-07 22:54:01]

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-07 22:58:32]

Description was changed from

==========
MSE: Throw TypeError instead of InvalidAccessError per spec update

Conforms to https://github.com/w3c/media-source/pull/47 spec change for
certain exceptional conditions, leaving a couple pieces for later changes:
1) bug 607372 "Comply with forcing appendMode to remain 'sequence' for MSE mp3
and adts stream parsers"
2) bug 644863 "Update TrackDefault and TrackDefaultList to use TypeError
correctly"

BUG=623781
TEST=updated http/tests/media/media-source tests, and current related upstream
w3c MSE tests
==========

to

==========
MSE: Throw TypeError instead of InvalidAccessError per spec update

Conforms to https://github.com/w3c/media-source/pull/47 spec change for
certain exceptional conditions, leaving a couple pieces for later changes:
1) bug 607372 "Comply with forcing appendMode to remain 'sequence' for MSE mp3
and adts stream parsers"
2) bug 644863 "Update TrackDefault and TrackDefaultList to use TypeError
correctly"

BUG=623781
TEST=updated http/tests/media/media-source tests, and current related upstream
w3c MSE tests
==========

[commit-bot: I haz the power, 2016-09-07 22:58:33]

Committed patchset #2 (id:20001)

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

Description was changed from

==========
MSE: Throw TypeError instead of InvalidAccessError per spec update

Conforms to https://github.com/w3c/media-source/pull/47 spec change for
certain exceptional conditions, leaving a couple pieces for later changes:
1) bug 607372 "Comply with forcing appendMode to remain 'sequence' for MSE mp3
and adts stream parsers"
2) bug 644863 "Update TrackDefault and TrackDefaultList to use TypeError
correctly"

BUG=623781
TEST=updated http/tests/media/media-source tests, and current related upstream
w3c MSE tests
==========

to

==========
MSE: Throw TypeError instead of InvalidAccessError per spec update

Conforms to https://github.com/w3c/media-source/pull/47 spec change for
certain exceptional conditions, leaving a couple pieces for later changes:
1) bug 607372 "Comply with forcing appendMode to remain 'sequence' for MSE mp3
and adts stream parsers"
2) bug 644863 "Update TrackDefault and TrackDefaultList to use TypeError
correctly"

BUG=623781
TEST=updated http/tests/media/media-source tests, and current related upstream
w3c MSE tests

Committed: https://crrev.com/bf5ee5415899e8e8c9e3ac2385d7092bbd11a3d8
Cr-Commit-Position: refs/heads/master@{#417107}
==========

[commit-bot: I haz the power, 2016-09-07 23:00:14]

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