[webkit-reviews] review granted: [Bug 216995] [Media in GPU Process] Use VideoLayerManager to manage layers of MediaPlayerPrivateRemote : [Attachment 409925] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Peng Liu
<peng.liu6 at apple.com>'s request for review:
Bug 216995: [Media in GPU Process] Use VideoLayerManager to manage layers of
MediaPlayerPrivateRemote
https://bugs.webkit.org/show_bug.cgi?id=216995

Attachment 409925: Patch

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




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

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

> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:137
> +#if PLATFORM(COCOA)

`setVideoLayer` is pure-virtual and is not Cocoa-only, so would get rid of the
compile flag and let other ports just ignore the call.

> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:138
> +	   IntSize defaultSize =
snappedIntRect(m_player->playerContentBoxRect()).size();

Nit: this variable isn't necessary

> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:159
> +    RefPtr<const Logger> m_logger;

Can you make this be a Ref <> since it looks like it can never be NULL?

> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:363
>      RefPtr<WebCore::PlatformMediaResourceLoader> m_mediaResourceLoader;

Ditto.

> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:365
> +    std::unique_ptr<WebCore::VideoLayerManager> m_videoLayerManager;

Can you make this be a UniqueRef<> since it looks like it can never be NULL?