Bug 95217 – Make MediaSource event dispatch asynchronous.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Aaron Colwell]

Make MediaSource event dispatch asynchronous.

[Aaron Colwell]

Created attachment 161017 [details]
Patch

[Eric Carlson]

Comment on attachment 161017 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161017&action=review

r- because the crash is unrelated to this change and should be fixed in another patch.

> Source/WebCore/ChangeLog:9
> +        - Update MediaSource & SourceBufferList to use a GenericEventQueue to dispatch events 
> +          instead of using synchronous dispatch.

I questioned synchronous event dispatch in an earlier review. Did the spec change or was it a bug all along?

> Source/WebCore/ChangeLog:10
> +        - Fix a use-after-free bug in SourceBufferList::remove().

This fix should really be done in another patch. Why not use bug 94950?

> Source/WebCore/Modules/mediasource/MediaSource.cpp:57
> +    m_sourceBuffers = SourceBufferList::create(scriptExecutionContext(),
> +                                               m_asyncEventQueue.get());
> +    m_activeSourceBuffers = SourceBufferList::create(scriptExecutionContext(),
> +                                                     m_asyncEventQueue.get());

Nit: I don't think the line breaks make this more readable.

> Source/WebCore/Modules/mediasource/MediaSource.h:103
>      virtual void derefEventTarget() OVERRIDE { deref(); }
>  
> +
> +    void scheduleEvent(const AtomicString& eventName);

Nit: you have an extra blank line here.

> LayoutTests/http/tests/media/media-source/video-media-source-async-events.html:14
> +                inEventHandler = true;

Nit: it would probably be better for inEventHandler to be a counter rather than a bool.

> LayoutTests/http/tests/media/media-source/video-media-source-event-attributes.html:-22
> -               endTest();

Why did this test previously have to calls to endTest()?

[Aaron Colwell]

Comment on attachment 161017 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161017&action=review

>> Source/WebCore/ChangeLog:9
>> +          instead of using synchronous dispatch.
> 
> I questioned synchronous event dispatch in an earlier review. Did the spec change or was it a bug all along?

It was a bug all along. I think there was just some miscommunication when annacc responded to your comments.

>> Source/WebCore/ChangeLog:10
>> +        - Fix a use-after-free bug in SourceBufferList::remove().
> 
> This fix should really be done in another patch. Why not use bug 94950?

Ok. I'll split it back out into two patches. I'll have to find a new way to smoke out that bug. :/

>> Source/WebCore/Modules/mediasource/MediaSource.cpp:57
>> +                                                     m_asyncEventQueue.get());
> 
> Nit: I don't think the line breaks make this more readable.

Done

>> Source/WebCore/Modules/mediasource/MediaSource.h:103
>> +    void scheduleEvent(const AtomicString& eventName);
> 
> Nit: you have an extra blank line here.

Done

>> LayoutTests/http/tests/media/media-source/video-media-source-async-events.html:14
>> +                inEventHandler = true;
> 
> Nit: it would probably be better for inEventHandler to be a counter rather than a bool.

Done

>> LayoutTests/http/tests/media/media-source/video-media-source-event-attributes.html:-22
>> -               endTest();
> 
> Why did this test previously have to calls to endTest()?

I honestly don't know. The old code would have called onSourceClose within the 'video.src = "";' statement so I'm not sure why this one was here.

[Aaron Colwell]

Created attachment 161045 [details]
Patch

[Eric Carlson]

Comment on attachment 161045 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161045&action=review

> Source/WebCore/ChangeLog:14
> +        (WebCore::MediaSource::MediaSource): Create GenericEventQueue & pass a reference to SourceBufferList.

Nit: you actually pass a pointer to the GenericEventQueue, not a reference.

[Aaron Colwell]

Comment on attachment 161045 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161045&action=review

>> Source/WebCore/ChangeLog:14
>> +        (WebCore::MediaSource::MediaSource): Create GenericEventQueue & pass a reference to SourceBufferList.
> 
> Nit: you actually pass a pointer to the GenericEventQueue, not a reference.

Done

[Aaron Colwell]

Created attachment 161055 [details]
Patch

[WebKit Review Bot]

Comment on attachment 161055 [details]
Patch

Rejecting attachment 161055 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Review Bot]

Comment on attachment 161055 [details]
Patch

Clearing flags on attachment: 161055

Committed r126946: <http://trac.webkit.org/changeset/126946>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.