[webkit-reviews] review granted: [Bug 212293] VideoFullscreenInterfaceAVKit is leaking when a video element enters and exits fullscreen/picture-in-picture : [Attachment 400100] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Peng Liu <peng.liu6 at apple.com>'s
request for review:
Bug 212293: VideoFullscreenInterfaceAVKit is leaking when a video element
enters and exits fullscreen/picture-in-picture
https://bugs.webkit.org/show_bug.cgi?id=212293

Attachment 400100: Patch

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




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

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

> Source/WebKit/ChangeLog:16
> +	   VideoFullscreenInterface[AVKit|Mac] is not ready yet.

s/not ready yet/ready/

> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.h:64
> +    , public ThreadSafeRefCounted<VideoFullscreenInterfaceAVKit>

This might be left to another patch but do we even need this one to be ref
counted?
Can we make it a unique_ptr and replace all the
Ref<VideoFullscreenInterfaceAVKit> by WeakPtr<VideoFullscreenInterfaceAVKit>
except for the owner of the VideoFullscreenInterfaceAVKit?

If we need it to be refcounted, is there a possibility
VideoFullscreenInterfaceAVKit get destroyed in a background thread?
Given it has string, we probably do not want that. We could make it
ThreadSafeRefCounted<VideoFullscreenInterfaceAVKit, DestructionThread::Main (or
MainRunLoop)>

> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:48
> +#import <wtf/RefPtr.h>

Is this one really needed, I would expect WTFString.h to include it somehow.

> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:98
> +    WeakPtr<VideoFullscreenInterfaceAVKit> _fullscreenInterface;

Are we sure _fullscreenInterface will only be used in the main thread?

> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:112
> +    _fullscreenInterface = makeWeakPtr(*fullscreenInterface);

Let's add an ASSERT(IsMainThread()) here.

> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:540
> +    if (!m_contextMap.contains(contextId))

It would be nice to use an ObjectIdentifier<> instead of a uint64_t, as a
follow-up patch.
That would make the code more type-aware and allow removing the
MESSAGE_CHECK_CONTEXTID(contextId).

> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:543
>      ensureInterface(contextId).hasVideoChanged(hasVideo);

We are searching m_contextMap twice probably.
Can we add a routine so that we can write :
if (auto* interface = interfaceIfFromIdentifier(contextId))
    interface->hasVideoChanged(hasVideo);

> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:553
> +	   return;

Ditto