[webkit-reviews] review granted: [Bug 184188] [Extra zoom mode] Replace video with a placeholder image during fullscreen transition : [Attachment 336885] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 184188: [Extra zoom mode] Replace video with a placeholder image during
fullscreen transition
https://bugs.webkit.org/show_bug.cgi?id=184188

Attachment 336885: Patch

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




--- Comment #8 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 336885
  --> https://bugs.webkit.org/attachment.cgi?id=336885
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:6028
> +    if (m_player)

Is there a case where m_player is nullified before exiting full screen?

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.h:265
> +    enum UpdateType { UpdateSynchronously, UpdateAsynchronously };

Can it be an enum class?

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:1175
>  void
MediaPlayerPrivateAVFoundationObjC::setVideoFullscreenLayer(PlatformLayer*
videoFullscreenLayer, WTF::Function<void()>&& completionHandler)

WTF:: is no longer needed.

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourc
eAVFObjC.mm:1069
>  void
MediaPlayerPrivateMediaSourceAVFObjC::setVideoFullscreenLayer(PlatformLayer
*videoFullscreenLayer, WTF::Function<void()>&& completionHandler)

Ditto here.

>
Source/WebCore/platform/graphics/avfoundation/objc/VideoFullscreenLayerManagerO
bjC.mm:77
> +    [m_videoInlineLayer setContentsGravity:kCAGravityResizeAspect];

Not sure why this is needed now, maybe a comment in changelog?

>
Source/WebCore/platform/graphics/avfoundation/objc/VideoFullscreenLayerManagerO
bjC.mm:91
> +	   [m_videoInlineLayer setContents:(id)image.get()];

This is ObjC so if m_videoInlinLayer is null, would this line be a no op, in
which case we can remove if(m_videoInlineLayer) ?

> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1112
> +    if (m_waitingForPreparedToExit) {

early return?

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:287
> +    WebThreadRun([protectedThis = makeRefPtr(this), this] () mutable {

makeRef(*this) is usually what we use.
Is it supposed to work in WK1?

> Source/WebKit/WebProcess/cocoa/VideoFullscreenManager.mm:443
> +    dispatch_async(dispatch_get_main_queue(), [protectedThis =
makeRefPtr(this), videoElement, contextId] {

makeRef(*this) maybe and WTFMove(videoElement) to reduce count churn.