[webkit-reviews] review denied: [Bug 128564] WK2 AVKit fullscreen doesn't display video. : [Attachment 224244] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Jeremy Jones
<jeremyj-wk at apple.com>'s request for review:
Bug 128564: WK2 AVKit fullscreen doesn't display video.
https://bugs.webkit.org/show_bug.cgi?id=128564

Attachment 224244: Patch
https://bugs.webkit.org/attachment.cgi?id=224244&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=224244&action=review


> Source/WebCore/ChangeLog:8
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   * WebCore.exp.in:

A short summary of the change should go here.

> Source/WebKit2/ChangeLog:8
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   * Shared/mac/RemoteLayerTreeTransaction.h:

Ditto

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:287
> +    if ([videoLayer isKindOfClass:[classAVPlayerLayer class]])
> +	   avPlayerLayer = (CALayer<AVPlayerLayer>*)videoLayer;
> +    else

Why do we need to handle both cases here? Do they occur in different scenarios?
A comment would be nice.

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:290
> +	   avPlayerLayer = [[[WebAVPlayerLayer alloc] init] autorelease];

[WebAVPlayerLayer layer] ?

> Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:97
> +   
m_videoFullscreenInterface->setVideoLayer(m_mediaElement->platformLayer());

I hate the name "interface". Is it human interface, or a an interface class?

> Source/WebKit2/Shared/mac/RemoteLayerTreeTransaction.h:176
> +    bool videoLayerUnparented() const { return m_videoLayerUnparented; }

videoLayerIsUnparented

> Source/WebKit2/Shared/mac/RemoteLayerTreeTransaction.h:190
> +    bool m_videoLayerUnparented;

This doesn't seem to expand to > 1 video layer (yeah, I know you can only take
one fullscreen at a time, but even so...)

I think this data should be the layerID for a going-fullscreen video layer.

> Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.cpp:73
> +    m_expectVideoLayerUnparentedTransaction = true;

This is confusing. Didn't you just get it? Or are you waiting for the next
unparenting?

Might be clearer with a state machine with enum states.

> Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.h:49
> +    void enterFullscreen() override;

virtual

> Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.h:55
> +    virtual void setVideoLayerID(WebCore::GraphicsLayer::PlatformLayerID);

override?

> Source/WebKit2/WebProcess/WebPage/WebPage.h:208
> +    void buildTransaction(RemoteLayerTreeTransaction&);

willCommitLayerTree

> Source/WebKit2/WebProcess/WebPage/mac/PlatformCALayerRemoteCustom.mm:68
> +    [m_platformLayer setValue:[NSValue valueWithPointer:nullptr]
forKey:platformCALayerPointer];

setValue:nil

> Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.h:60
> +    void buildTransaction(RemoteLayerTreeTransaction&);

willCommitLayerTree