[webkit-reviews] review granted: [Bug 228560] [Cocoa] Remove support for AVAssetImageGenerator : [Attachment 434926] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Jer Noble
<jer.noble at apple.com>'s request for review:
Bug 228560: [Cocoa] Remove support for AVAssetImageGenerator
https://bugs.webkit.org/show_bug.cgi?id=228560

Attachment 434926: Patch

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




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

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

>
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cp
p:-132
> -	   tearDownVideoRendering();

Last changed in 2011!

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:2429
>      m_videoOutputDelegate = adoptNS([[WebCoreAVFPullDelegate alloc] init]);
> +    [m_videoOutputDelegate setParent:*this];

Nit: since you pass a reference, and so never need to clear the parent, you
could add an initializer and pass it there.

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:2513
> +    updateLastImage(UpdateType::UpdateSynchronously);

Is it OK to ignore the old warning about not doing this synchronously?

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:2566
> +    bool satisfied = timeoutTimer.isActive();

Isn't there is a (very) small chance that this will be wrong because you check
the timer after the logging above?

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:2568
> +	   INFO_LOG(LOGIDENTIFIER, "timed out");

Why not log this as an error so it always shows up in logs?