Make MediaSource event dispatch asynchronous.
Created attachment 161017 [details] Patch
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()?
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.
Created attachment 161045 [details] Patch
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.
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
Created attachment 161055 [details] Patch
Comment on attachment 161055 [details] Patch Rejecting attachment 161055 [details] from commit-queue. acolwell@chromium.org 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.
Comment on attachment 161055 [details] Patch Clearing flags on attachment: 161055 Committed r126946: <http://trac.webkit.org/changeset/126946>
All reviewed patches have been landed. Closing bug.