[webkit-reviews] review granted: [Bug 208252] [Media in GPU process] Implement the video fullscreen and Picture-in-Picture support : [Attachment 392028] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Peng Liu
<peng.liu6 at apple.com>'s request for review:
Bug 208252: [Media in GPU process] Implement the video fullscreen and
Picture-in-Picture support
https://bugs.webkit.org/show_bug.cgi?id=208252

Attachment 392028: Patch

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




--- Comment #7 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 392028
  --> https://bugs.webkit.org/attachment.cgi?id=392028
Patch

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

> Source/WebCore/html/HTMLMediaElement.h:182
> +    RetainPtr<PlatformLayer> createVideoFullscreenLayer();

Should this really create a new one every time?

What is the "video fullscreen layer"? Is it the layer with the video in it, the
layer that goes fullscreen, or something else?

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:508
> +    m_inlineLayerHostingContext->setRootLayer(m_player->platformLayer());

LayerHostingContext takes a CALayer*. You should really be in a .mm file if
you're calling this code. RemoteMediaPlayerProxy.cpp doesn't sound
Cocoa-specific.

> Source/WebKit/GPUProcess/media/cocoa/RemoteMediaPlayerProxyCocoa.mm:51
> +    [videoFullscreenLayer setName:@"Web video fullscreen manager layer
(remote)"];

That's a lot of words.

Why "videoFullscreenLayer" if it's really "videoFullscreenRemoteManagerLayer"?

> Source/WebKit/GPUProcess/media/cocoa/RemoteMediaPlayerProxyCocoa.mm:52
> +    [videoFullscreenLayer setPosition:CGPointMake(0, 0)];

CGPointZero.

Maybe set the anchor point too because this won't be top-left aligned if it has
non-zero size.

> Source/WebKit/GPUProcess/media/cocoa/RemoteMediaPlayerProxyCocoa.mm:53
> +    [videoFullscreenLayer
setBackgroundColor:cachedCGColor(WebCore::Color::transparent)];

This shouldn't be necessary.

> Source/WebKit/WebProcess/GPU/media/cocoa/MediaPlayerPrivateRemoteCocoa.mm:37
> +#if PLATFORM(IOS_FAMILY) || (PLATFORM(MAC) &&
ENABLE(VIDEO_PRESENTATION_MODE))

We should have an ENABLE_ or something for this combination. Why doesn't iOS
just define ENABLE_VIDEO_PRESENTATION_MODE ?