Add minimal implementation of unprefixed MediaSource API that has feature parity with prefixed API

Add minimal implementation of unprefixed MediaSource API that has feature parity with the prefixed API.

BUG=239506
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=152379

[acolwell GONE FROM CHROMIUM, 2013-06-10 22:10:54]

LayoutTests are coming soon, but I wanted to get the review started. 

This change introduces the minimal subset of the existing Media Source Extension
spec that provides equivalent functionality to the prefixed API. The main
difference is that the prefixed API has a synchronous append() method and the
unprefixed API has asynchronous appendBuffer() methods. There are also minor
differences in error checking and event firing order, but as you will see from
the diffs most of the code is the same. 

Additional features in the current spec will be added in followup CLs once this
lands.

https://codereview.chromium.org/16625011/diff/1/Source/modules/mediasource/Me...
File Source/modules/mediasource/MediaSourceBase.cpp (right):

https://codereview.chromium.org/16625011/diff/1/Source/modules/mediasource/Me...
Source/modules/mediasource/MediaSourceBase.cpp:179: m_asyncEventQueue->close();
Moved here to prevent events from being queued when setReadyState() is called
below.

https://codereview.chromium.org/16625011/diff/1/Source/modules/mediasource/Me...
Source/modules/mediasource/MediaSourceBase.cpp:181:
setReadyState(closedKeyword());
Forcing close when ActiveDOMObject's are being stopped so that all references to
other objects can be cleaned up before we get to the destructor.

[adamk, 2013-06-10 23:05:40]

Just a first run through. I suspect I'll see more the next time around, but I'm
just starting to grok the code.

https://codereview.chromium.org/16625011/diff/3001/Source/modules/mediasource...
File Source/modules/mediasource/DOMWindowMediaSource.idl (right):

https://codereview.chromium.org/16625011/diff/3001/Source/modules/mediasource...
Source/modules/mediasource/DOMWindowMediaSource.idl:31:
[EnabledAtRuntime=mediaSource] attribute MediaSourceConstructor MediaSource;
You shouldn't need this anymore, see below...

https://codereview.chromium.org/16625011/diff/3001/Source/modules/mediasource...
File Source/modules/mediasource/MediaSource.cpp (right):

https://codereview.chromium.org/16625011/diff/3001/Source/modules/mediasource...
Source/modules/mediasource/MediaSource.cpp:67: SourceBufferList*
MediaSource::sourceBuffers()
Not a big deal, but any reason not to inline this and activeSourceBuffers()?

https://codereview.chromium.org/16625011/diff/3001/Source/modules/mediasource...
Source/modules/mediasource/MediaSource.cpp:154: m_sourceBuffers->remove(buffer);
Won't this fire two events?

https://codereview.chromium.org/16625011/diff/3001/Source/modules/mediasource...
Source/modules/mediasource/MediaSource.cpp:186: for (int i = 0; i <
m_sourceBuffers->length(); ++i)
I expect you'll need to make this some sort of unsigned value to get this to
compile.

https://codereview.chromium.org/16625011/diff/3001/Source/modules/mediasource...
File Source/modules/mediasource/MediaSource.idl (right):

https://codereview.chromium.org/16625011/diff/3001/Source/modules/mediasource...
Source/modules/mediasource/MediaSource.idl:32: NoInterfaceObject,
If you just remove this NoInterfaceObject line, your constructor will
automatically be exposed on the Window object thanks to codegen magic.

https://codereview.chromium.org/16625011/diff/3001/Source/modules/mediasource...
File Source/modules/mediasource/SourceBufferList.cpp (right):

https://codereview.chromium.org/16625011/diff/3001/Source/modules/mediasource...
Source/modules/mediasource/SourceBufferList.cpp:54: return m_list.size();
Any reason not to inline? Similar questions about most small functions in this
patch. I guess that can easily wait till later, if only to make the diff for
this patch small.

[acolwell GONE FROM CHROMIUM, 2013-06-10 23:36:15]

https://codereview.chromium.org/16625011/diff/3001/Source/modules/mediasource...
File Source/modules/mediasource/DOMWindowMediaSource.idl (right):

https://codereview.chromium.org/16625011/diff/3001/Source/modules/mediasource...
Source/modules/mediasource/DOMWindowMediaSource.idl:31:
[EnabledAtRuntime=mediaSource] attribute MediaSourceConstructor MediaSource;
On 2013/06/10 23:05:40, adamk wrote:
> You shouldn't need this anymore, see below...

Done.

https://codereview.chromium.org/16625011/diff/3001/Source/modules/mediasource...
File Source/modules/mediasource/MediaSource.cpp (right):

https://codereview.chromium.org/16625011/diff/3001/Source/modules/mediasource...
Source/modules/mediasource/MediaSource.cpp:67: SourceBufferList*
MediaSource::sourceBuffers()
On 2013/06/10 23:05:40, adamk wrote:
> Not a big deal, but any reason not to inline this and activeSourceBuffers()?

Done.

https://codereview.chromium.org/16625011/diff/3001/Source/modules/mediasource...
Source/modules/mediasource/MediaSource.cpp:154: m_sourceBuffers->remove(buffer);
On 2013/06/10 23:05:40, adamk wrote:
> Won't this fire two events?
Yes. One for the m_activeSourceBuffers list and one for the m_sourceBuffers
list. Right now these 2 lists always contain the same objects, but when multiple
track support is added, these lists may differ based on what tracks are
selected/enabled.

https://codereview.chromium.org/16625011/diff/3001/Source/modules/mediasource...
Source/modules/mediasource/MediaSource.cpp:186: for (int i = 0; i <
m_sourceBuffers->length(); ++i)
On 2013/06/10 23:05:40, adamk wrote:
> I expect you'll need to make this some sort of unsigned value to get this to
> compile.

Done. {Shakes fist at clang for not detecting this.}

https://codereview.chromium.org/16625011/diff/3001/Source/modules/mediasource...
File Source/modules/mediasource/MediaSource.idl (right):

https://codereview.chromium.org/16625011/diff/3001/Source/modules/mediasource...
Source/modules/mediasource/MediaSource.idl:32: NoInterfaceObject,
On 2013/06/10 23:05:40, adamk wrote:
> If you just remove this NoInterfaceObject line, your constructor will
> automatically be exposed on the Window object thanks to codegen magic.

Done.

https://codereview.chromium.org/16625011/diff/3001/Source/modules/mediasource...
File Source/modules/mediasource/SourceBufferList.cpp (right):

https://codereview.chromium.org/16625011/diff/3001/Source/modules/mediasource...
Source/modules/mediasource/SourceBufferList.cpp:54: return m_list.size();
On 2013/06/10 23:05:40, adamk wrote:
> Any reason not to inline? Similar questions about most small functions in this
> patch. I guess that can easily wait till later, if only to make the diff for
> this patch small.
Done for all non-virtual methods. It isn't clear from the style guide whether
Chromium's ban on inline virtual functions applies to Blink.

[adamk, 2013-06-11 18:19:26]

A few more nits, but things look mostly good. Are you still planning on adding
layout tests to this patch?

https://codereview.chromium.org/16625011/diff/10001/Source/modules/mediasourc...
File Source/modules/mediasource/SourceBuffer.cpp (right):

https://codereview.chromium.org/16625011/diff/10001/Source/modules/mediasourc...
Source/modules/mediasource/SourceBuffer.cpp:66: SourceBuffer::~SourceBuffer()
Would it be useful to ASSERT(isRemoved()) here?

https://codereview.chromium.org/16625011/diff/10001/Source/modules/mediasourc...
Source/modules/mediasource/SourceBuffer.cpp:125:
appendBufferInternal(reinterpret_cast<unsigned char*>(data->data()),
data->byteLength(), ec);
This can just be a static_cast, can't it (since data is void*)?

https://codereview.chromium.org/16625011/diff/10001/Source/modules/mediasourc...
Source/modules/mediasource/SourceBuffer.cpp:138:
appendBufferInternal(reinterpret_cast<unsigned char*>(data->baseAddress()),
data->byteLength(), ec);
Same here, prefer static_cast when possible.

https://codereview.chromium.org/16625011/diff/10001/Source/modules/mediasourc...
Source/modules/mediasource/SourceBuffer.cpp:216: 
Nit: extra blank line.

https://codereview.chromium.org/16625011/diff/10001/Source/modules/mediasourc...
File Source/modules/mediasource/SourceBufferList.cpp (right):

https://codereview.chromium.org/16625011/diff/10001/Source/modules/mediasourc...
Source/modules/mediasource/SourceBufferList.cpp:73: void
SourceBufferList::createAndFireEvent(const AtomicString& eventName)
I think scheduleEvent would be a better name here; when I first saw this I
assumed it was synchronously firing an event.

[acolwell GONE FROM CHROMIUM, 2013-06-11 22:25:24]

On 2013/06/11 18:19:26, adamk wrote:
> A few more nits, but things look mostly good. Are you still planning on adding
> layout tests to this patch?

Thanks for the review. Yes. LayoutTests will be included in my next patch.

[acolwell GONE FROM CHROMIUM, 2013-06-12 01:14:04]

Addressed comments and uploaded a few LayoutTests. I'm still working on a few
more to exercise the async appendBuffer() logic. 

I'm integrating with the W3C testharness.js framework because the plan is for
these tests to eventually become part of the W3C test suite. I also wanted to
make them easy to send to other browser vendors so I can facilitate interop.

https://codereview.chromium.org/16625011/diff/10001/Source/modules/mediasourc...
File Source/modules/mediasource/SourceBuffer.cpp (right):

https://codereview.chromium.org/16625011/diff/10001/Source/modules/mediasourc...
Source/modules/mediasource/SourceBuffer.cpp:66: SourceBuffer::~SourceBuffer()
On 2013/06/11 18:19:26, adamk wrote:
> Would it be useful to ASSERT(isRemoved()) here?

Done.

https://codereview.chromium.org/16625011/diff/10001/Source/modules/mediasourc...
Source/modules/mediasource/SourceBuffer.cpp:125:
appendBufferInternal(reinterpret_cast<unsigned char*>(data->data()),
data->byteLength(), ec);
On 2013/06/11 18:19:26, adamk wrote:
> This can just be a static_cast, can't it (since data is void*)?

Done.

https://codereview.chromium.org/16625011/diff/10001/Source/modules/mediasourc...
Source/modules/mediasource/SourceBuffer.cpp:138:
appendBufferInternal(reinterpret_cast<unsigned char*>(data->baseAddress()),
data->byteLength(), ec);
On 2013/06/11 18:19:26, adamk wrote:
> Same here, prefer static_cast when possible.

Done.

https://codereview.chromium.org/16625011/diff/10001/Source/modules/mediasourc...
Source/modules/mediasource/SourceBuffer.cpp:216: 
On 2013/06/11 18:19:26, adamk wrote:
> Nit: extra blank line.

Done.

https://codereview.chromium.org/16625011/diff/10001/Source/modules/mediasourc...
File Source/modules/mediasource/SourceBufferList.cpp (right):

https://codereview.chromium.org/16625011/diff/10001/Source/modules/mediasourc...
Source/modules/mediasource/SourceBufferList.cpp:73: void
SourceBufferList::createAndFireEvent(const AtomicString& eventName)
On 2013/06/11 18:19:26, adamk wrote:
> I think scheduleEvent would be a better name here; when I first saw this I
> assumed it was synchronously firing an event.

Done.

[adamk, 2013-06-12 15:32:42]

Tests look good so far; would like to see more tests exercising events (e.g.,
{add,remove}sourcebuffer)

[acolwell GONE FROM CHROMIUM, 2013-06-12 23:04:46]

I've added more LayoutTest. I think there is sufficient coverage now to consider
this ready to land.

[adamk, 2013-06-12 23:43:41]

lgtm

[commit-bot: I haz the power, 2013-06-13 00:00:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acolwell@chromium.org/16625011/45001

[commit-bot: I haz the power, 2013-06-13 07:50:13]

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

[commit-bot: I haz the power, 2013-06-13 17:22:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acolwell@chromium.org/16625011/71001

[commit-bot: I haz the power, 2013-06-13 18:18:13]

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

[adamk, 2013-06-13 18:39:35]

I think you'll actually need to update global-constructors-listing.html as
you're adding new constructors to the Window object.

[adamk, 2013-06-13 18:39:57]

On 2013/06/13 18:39:35, adamk wrote:
> I think you'll actually need to update global-constructors-listing.html as
> you're adding new constructors to the Window object.

That is to say, the CQ is correct in complaining about your patch and a test
needs updating.

[acolwell GONE FROM CHROMIUM, 2013-06-13 19:13:00]

On 2013/06/13 18:39:57, adamk wrote:
> On 2013/06/13 18:39:35, adamk wrote:
> > I think you'll actually need to update global-constructors-listing.html as
> > you're adding new constructors to the Window object.
> 
> That is to say, the CQ is correct in complaining about your patch and a test
> needs updating.

Thanks. I was about to look into that.

[commit-bot: I haz the power, 2013-06-13 19:47:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acolwell@chromium.org/16625011/83001

[commit-bot: I haz the power, 2013-06-13 21:15:39]

Change committed as 152379