Preliminary work for unprefixed MSE SourceBuffer.mode manipulation

Preliminary work for unprefixed MSE SourceBuffer.mode manipulation

Adds the SourceBuffer.mode attribute and basic layout tests.
Later changes will plumb this through to correctly process 'segments' and
'sequence' append modes.

R=acolwell@chromium.org
TEST=On Linux, new layout tests pass with no other mediasource layout test regression. SourceBuffer.mode attribute's implementation is correctly hidden: new tests fail in Chromium without --enable-experimental-web-platform-features.
BUG=249422
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=164919

[wolenetz, 2014-01-09 02:12:55]

Additional verification of parsing state and actual support for the distinct
sequence mode are currently left for a later CL. Please review this CL. Thanks!

[acolwell GONE FROM CHROMIUM, 2014-01-09 02:53:57]

This looks good so far. Just add the necessary changes to WebSourceBuffer and I
think this will be ready to land.

https://codereview.chromium.org/130523002/diff/1/Source/modules/mediasource/S...
File Source/modules/mediasource/SourceBuffer.cpp (right):

https://codereview.chromium.org/130523002/diff/1/Source/modules/mediasource/S...
Source/modules/mediasource/SourceBuffer.cpp:130: // TODO(wolenetz): Figure out
how to enforce this step. See http://crbug.com/249422.
I'd recommmend you just have your m_source->setMode() method return a bool much
like the setTimestampOffset() method does below.

https://codereview.chromium.org/130523002/diff/1/Source/modules/mediasource/S...
File Source/modules/mediasource/SourceBuffer.idl (right):

https://codereview.chromium.org/130523002/diff/1/Source/modules/mediasource/S...
Source/modules/mediasource/SourceBuffer.idl:38: [RaisesException=Setter]
attribute DOMString mode;
Please add ", RuntimeEnabled=MediaSourceExperimental" so this isn't exposed
without enabling the experimental web platform features flag.

[jamesr, 2014-01-09 21:31:25]

https://codereview.chromium.org/130523002/diff/90001/public/platform/WebSourc...
File public/platform/WebSourceBuffer.h (right):

https://codereview.chromium.org/130523002/diff/90001/public/platform/WebSourc...
public/platform/WebSourceBuffer.h:40: enum AppendMode {
please read http://www.chromium.org/blink/public-c-api - this enum needs to
follow the public API style

[wolenetz, 2014-01-09 21:37:29]

On 2014/01/09 21:31:25, jamesr wrote:
>
https://codereview.chromium.org/130523002/diff/90001/public/platform/WebSourc...
> File public/platform/WebSourceBuffer.h (right):
> 
>
https://codereview.chromium.org/130523002/diff/90001/public/platform/WebSourc...
> public/platform/WebSourceBuffer.h:40: enum AppendMode {
> please read http://www.chromium.org/blink/public-c-api - this enum needs to
> follow the public API style

Thanks for pointing this out.
Looking at similar enums in public/platform/WebMediaSource.h, the enum names
themselves are not prefixed with "Web". Has that portion of the public API style
changed recently? Are those WebMediaSource enums not compliant?

[wolenetz, 2014-01-09 21:43:10]

PTAL @ patch set 3

https://codereview.chromium.org/130523002/diff/1/Source/modules/mediasource/S...
File Source/modules/mediasource/SourceBuffer.cpp (right):

https://codereview.chromium.org/130523002/diff/1/Source/modules/mediasource/S...
Source/modules/mediasource/SourceBuffer.cpp:130: // TODO(wolenetz): Figure out
how to enforce this step. See http://crbug.com/249422.
On 2014/01/09 02:53:58, acolwell wrote:
> I'd recommmend you just have your m_source->setMode() method return a bool
much
> like the setTimestampOffset() method does below.

Done.

https://codereview.chromium.org/130523002/diff/1/Source/modules/mediasource/S...
File Source/modules/mediasource/SourceBuffer.idl (right):

https://codereview.chromium.org/130523002/diff/1/Source/modules/mediasource/S...
Source/modules/mediasource/SourceBuffer.idl:38: [RaisesException=Setter]
attribute DOMString mode;
On 2014/01/09 02:53:58, acolwell wrote:
> Please add ", RuntimeEnabled=MediaSourceExperimental" so this isn't exposed
> without enabling the experimental web platform features flag.

Done.

https://codereview.chromium.org/130523002/diff/90001/public/platform/WebSourc...
File public/platform/WebSourceBuffer.h (right):

https://codereview.chromium.org/130523002/diff/90001/public/platform/WebSourc...
public/platform/WebSourceBuffer.h:40: enum AppendMode {
On 2014/01/09 21:31:26, jamesr wrote:
> please read http://www.chromium.org/blink/public-c-api - this enum needs to
> follow the public API style

Done, though does this enum (and enums in WebMediaSource.h) need to be prefixed
also with "Web"?

[wolenetz, 2014-01-09 21:47:45]

On 2014/01/09 21:43:10, wolenetz wrote:
> PTAL @ patch set 3
> 
>
https://codereview.chromium.org/130523002/diff/1/Source/modules/mediasource/S...
> File Source/modules/mediasource/SourceBuffer.cpp (right):
> 
>
https://codereview.chromium.org/130523002/diff/1/Source/modules/mediasource/S...
> Source/modules/mediasource/SourceBuffer.cpp:130: // TODO(wolenetz): Figure out
> how to enforce this step. See http://crbug.com/249422.
> On 2014/01/09 02:53:58, acolwell wrote:
> > I'd recommmend you just have your m_source->setMode() method return a bool
> much
> > like the setTimestampOffset() method does below.
> 
> Done.
> 
>
https://codereview.chromium.org/130523002/diff/1/Source/modules/mediasource/S...
> File Source/modules/mediasource/SourceBuffer.idl (right):
> 
>
https://codereview.chromium.org/130523002/diff/1/Source/modules/mediasource/S...
> Source/modules/mediasource/SourceBuffer.idl:38: [RaisesException=Setter]
> attribute DOMString mode;
> On 2014/01/09 02:53:58, acolwell wrote:
> > Please add ", RuntimeEnabled=MediaSourceExperimental" so this isn't exposed
> > without enabling the experimental web platform features flag.
> 
> Done.
> 
>
https://codereview.chromium.org/130523002/diff/90001/public/platform/WebSourc...
> File public/platform/WebSourceBuffer.h (right):
> 
>
https://codereview.chromium.org/130523002/diff/90001/public/platform/WebSourc...
> public/platform/WebSourceBuffer.h:40: enum AppendMode {
> On 2014/01/09 21:31:26, jamesr wrote:
> > please read http://www.chromium.org/blink/public-c-api - this enum needs to
> > follow the public API style
> 
> Done, though does this enum (and enums in WebMediaSource.h) need to be
prefixed
> also with "Web"?

After looking further around public API enums, I think I have it right in patch
set 3: only top-level enums require 'Web' prefix. Those within Web-prefixed
classes don't.

[jamesr, 2014-01-09 22:15:36]

https://codereview.chromium.org/130523002/diff/200001/public/platform/WebSour...
File public/platform/WebSourceBuffer.h (right):

https://codereview.chromium.org/130523002/diff/200001/public/platform/WebSour...
public/platform/WebSourceBuffer.h:47: // FIXME: Remove default implementation
once Chromium setMode() implementation has landed. See http://crbug.com/249422.
why don't you just land the chromium-side implementation of this interface first
and skip the extra step / FIXME?

[wolenetz, 2014-01-09 22:21:14]

On 2014/01/09 22:15:36, jamesr wrote:
>
https://codereview.chromium.org/130523002/diff/200001/public/platform/WebSour...
> File public/platform/WebSourceBuffer.h (right):
> 
>
https://codereview.chromium.org/130523002/diff/200001/public/platform/WebSour...
> public/platform/WebSourceBuffer.h:47: // FIXME: Remove default implementation
> once Chromium setMode() implementation has landed. See
http://crbug.com/249422.
> why don't you just land the chromium-side implementation of this interface
first
> and skip the extra step / FIXME?

That would work too, though the chromium-side implementation isn't ready yet :)

[jamesr, 2014-01-09 22:21:50]

It could be at least as real as this stub.

[wolenetz, 2014-01-09 22:39:02]

On 2014/01/09 22:21:50, jamesr wrote:
> It could be at least as real as this stub.

Regarding landing something in Chromium first, it would need to (eventually)
reference the WebSourceBuffer::AppendMode enum as setMode() parameter type.
Since that enum doesn't previously exist, I'm not clear that compilers will
understand that some other enum/int param type would match and override the
eventual blink interface. Seems this would still need to be a 3-sided commit.
WDYT?

[jamesr, 2014-01-09 22:45:31]

Ah, good point.  Tricksy.  lgtm

[wolenetz, 2014-01-10 20:41:22]

On 2014/01/09 22:45:31, jamesr wrote:
> Ah, good point.  Tricksy.  lgtm

Thank you. acolwell: As I mentioned offline, I'll upload another patch set (#4)
shortly with a nit I noticed fixed (use assert_equals(...) instead of
assert_true( ... == ... ).

[wolenetz, 2014-01-10 20:59:40]

acolwell, please review patch set 4 now that I fixed those nits I noticed and we
chatted about. Thank you.

[acolwell GONE FROM CHROMIUM, 2014-01-10 22:09:34]

lgtm % nits

https://codereview.chromium.org/130523002/diff/330001/Source/modules/mediasou...
File Source/modules/mediasource/SourceBuffer.cpp (right):

https://codereview.chromium.org/130523002/diff/330001/Source/modules/mediasou...
Source/modules/mediasource/SourceBuffer.cpp:131: // FIXME: Remove default no-op
implementation once these steps are landed in Chromium. See
http://crbug.com/249422.
nit: Remove this comment since nothing needs to change here when the Chromium
changes actually land.

https://codereview.chromium.org/130523002/diff/330001/Source/modules/mediasou...
Source/modules/mediasource/SourceBuffer.cpp:183: // FIXME: Add step 6 text when
mode attribute is implemented. See http://crbug.com/249422.
nit: Might as well just add the step 6 text now since this will happen at the
Chromium level. I think you should file a bug for implementing this step in
WebSourceBufferImpl or SourceState and then put a comment referencing the bug in
the relevant place.

[wolenetz, 2014-01-10 23:06:50]

Thank you acolwell@ and jamesr@. I fixed the nits and am sending patch set 5 to
CQ shortly.
(This change is not pending your review.)

https://codereview.chromium.org/130523002/diff/330001/Source/modules/mediasou...
File Source/modules/mediasource/SourceBuffer.cpp (right):

https://codereview.chromium.org/130523002/diff/330001/Source/modules/mediasou...
Source/modules/mediasource/SourceBuffer.cpp:131: // FIXME: Remove default no-op
implementation once these steps are landed in Chromium. See
http://crbug.com/249422.
On 2014/01/10 22:09:34, acolwell wrote:
> nit: Remove this comment since nothing needs to change here when the Chromium
> changes actually land.

Done.

https://codereview.chromium.org/130523002/diff/330001/Source/modules/mediasou...
Source/modules/mediasource/SourceBuffer.cpp:183: // FIXME: Add step 6 text when
mode attribute is implemented. See http://crbug.com/249422.
On 2014/01/10 22:09:34, acolwell wrote:
> nit: Might as well just add the step 6 text now since this will happen at the
> Chromium level. I think you should file a bug for implementing this step in
> WebSourceBufferImpl or SourceState and then put a comment referencing the bug
in
> the relevant place.

Done, including updating step 5's text to current spec. I filed
http://crbug.com/333437 and added a reference to it from a SourceState comment
introduced in patch set 2 of the initial Chromium side plumbing for SourceBuffer
mode setting (https://codereview.chromium.org/134883002/).

[commit-bot: I haz the power, 2014-01-10 23:11:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wolenetz@chromium.org/130523002/380002

[commit-bot: I haz the power, 2014-01-11 00:06:14]

Retried try job too often on linux_blink for step(s) webkit_unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2014-01-11 00:13:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wolenetz@chromium.org/130523002/380002

[commit-bot: I haz the power, 2014-01-11 01:45:19]

Change committed as 164919

[Mike West, 2014-01-13 11:56:20]

https://codereview.chromium.org/130523002/diff/380002/Source/modules/mediasou...
File Source/modules/mediasource/SourceBuffer.cpp (right):

https://codereview.chromium.org/130523002/diff/380002/Source/modules/mediasou...
Source/modules/mediasource/SourceBuffer.cpp:112:
exceptionState.throwUninformativeAndGenericDOMException(InvalidAccessError);
Hello, friendly engineer!

Please don't add new exceptions which are uninformative and generic. I'm working
on driving these to zero so I can remove the method entirely. Adding more makes
that more difficult. :)

Thanks!

-mike

[wolenetz, 2014-01-13 18:42:37]

On 2014/01/13 11:56:20, Mike West wrote:
>
https://codereview.chromium.org/130523002/diff/380002/Source/modules/mediasou...
> File Source/modules/mediasource/SourceBuffer.cpp (right):
> 
>
https://codereview.chromium.org/130523002/diff/380002/Source/modules/mediasou...
> Source/modules/mediasource/SourceBuffer.cpp:112:
> exceptionState.throwUninformativeAndGenericDOMException(InvalidAccessError);
> Hello, friendly engineer!
> 
> Please don't add new exceptions which are uninformative and generic. I'm
working
> on driving these to zero so I can remove the method entirely. Adding more
makes
> that more difficult. :)
> 
> Thanks!
> 
> -mike

Hi Mike -
Sorry, I was unaware of your effort and followed the example of other existing
pieces of the MediaSource API spec implementation in Blink that similarly throw
exceptions this way. For my future reference, do you have an example where
InvalidStateError or InvalidAccessError are thrown the way you recommend?

Thanks,
Matt

[Mike West, 2014-01-13 19:19:26]

On 2014/01/13 18:42:37, wolenetz wrote:
> On 2014/01/13 11:56:20, Mike West wrote:
> >
>
https://codereview.chromium.org/130523002/diff/380002/Source/modules/mediasou...
> > File Source/modules/mediasource/SourceBuffer.cpp (right):
> > 
> >
>
https://codereview.chromium.org/130523002/diff/380002/Source/modules/mediasou...
> > Source/modules/mediasource/SourceBuffer.cpp:112:
> > exceptionState.throwUninformativeAndGenericDOMException(InvalidAccessError);
> > Hello, friendly engineer!
> > 
> > Please don't add new exceptions which are uninformative and generic. I'm
> working
> > on driving these to zero so I can remove the method entirely. Adding more
> makes
> > that more difficult. :)
> > 
> > Thanks!
> > 
> > -mike
> 
> Hi Mike -
> Sorry, I was unaware of your effort and followed the example of other existing
> pieces of the MediaSource API spec implementation in Blink that similarly
throw
> exceptions this way. For my future reference, do you have an example where
> InvalidStateError or InvalidAccessError are thrown the way you recommend?
> 
> Thanks,
> Matt

Thanks Matt!

You'd throw the error exactly the same way, you'd simply use
`throwDOMException([type], [reason])` instead of
`throwUninformativeAndGenericDOMException([type])`. The first of these
exceptions might be something like:

    exceptionState.throwDOMException(InvalidAccessError, "The mode provided ('"
+ newMode + "') is neither 'segments' nor 'sequence'.");

Something like that. I'd love to review CLs to that effect!

-mike