[webkit-reviews] review denied: [Bug 95217] Make MediaSource event dispatch asynchronous. : [Attachment 161017] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has denied Aaron Colwell
<acolwell at chromium.org>'s request for review:
Bug 95217: Make MediaSource event dispatch asynchronous.
https://bugs.webkit.org/show_bug.cgi?id=95217

Attachment 161017: Patch
https://bugs.webkit.org/attachment.cgi?id=161017&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
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.h
tml:-22
> -		  endTest();

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