[webkit-reviews] review granted: [Bug 227559] SourceBuffer.abort() doesn't go back to state WAITING_FOR_SEGMENT properly : [Attachment 432770] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Jean-Yves Avenard [:jya]
<jean-yves.avenard at apple.com>'s request for review:
Bug 227559: SourceBuffer.abort() doesn't go back to state WAITING_FOR_SEGMENT
properly
https://bugs.webkit.org/show_bug.cgi?id=227559

Attachment 432770: Patch

https://bugs.webkit.org/attachment.cgi?id=432770&action=review




--- Comment #6 from Eric Carlson <eric.carlson at apple.com> ---
Comment on attachment 432770
  --> https://bugs.webkit.org/attachment.cgi?id=432770
Patch

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

> Source/WebCore/ChangeLog:34
> +	   - The SourceBufferParser handle two type of parser:
SourceBufferParser and the

s/two type/two types/

>
Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.m
m:417
> +	   callOnMainThread([this, weakThis = WTFMove(weakThis)] {

I believe we are trying to get away from using `callOnMainThread` as a way to
queue tasks in favor of using the event queue. SourceBuffer is an
ActiveDOMObject, so you could add a method that calls
`queueCancellableTaskKeepingObjectAlive` and use it instead. This would
presumably allow you to get rid of the WeakPtr, because SourceBuffer would
cancel the pending task when it clears the client pointer.

>
Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.m
m:575
> +    callOnMainThread([weakThis = makeWeakPtr(*this), data = WTFMove(data),
this]() mutable {
> +	   if (!weakThis)
> +	       return;

Ditto

>
Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.m
m:588
> +	       callOnMainThread([weakThis = WTFMove(weakThis), trackID,
respondedSemaphore]() {

Ditto.

>
Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.m
m:591
> +		   respondedSemaphore->signal();

Is it safe to signaling the semaphore if `weakThis` is null (the object has
been destroyed)?

>
Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.m
m:608
> +	       callOnMainThread([weakThis = WTFMove(weakThis), initData =
WTFMove(initData), trackID, hasSessionSemaphore]() mutable {

Ditto

>
Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.m
m:629
> +		   callOnMainThread([weakThis = WTFMove(weakThis)] {

Ditto

>
Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.m
m:672
> +    callOnMainThread([weakThis = makeWeakPtr(*this), start, end,
currentMediaTime, isEnded, completionHandler = WTFMove(completionHandler)]()
mutable {

Ditto

>
Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.m
m:696
> +    callOnMainThread([weakThis = makeWeakPtr(*this), this]() {

Ditto

>
Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.m
m:727
> +    callOnMainThread([weakThis = makeWeakPtr(*this), parser = m_parser,
this]() {

Ditto.

> Source/WebCore/platform/graphics/cocoa/SourceBufferParserWebM.cpp:812
> +    // partial init segment is encountered after a call to
sourceBuffer.abort().

s/segment is encountered/segment be encountered/