[webkit-reviews] review granted: [Bug 126818] Add AVKit fullscreen video interface. : [Attachment 220941] Patch

[bugzilla-daemon at webkit.org]

Jer Noble <jer.noble at apple.com> has granted Jeremy Jones
<jeremyj-wk at apple.com>'s request for review:
Bug 126818: Add AVKit fullscreen video interface.
https://bugs.webkit.org/show_bug.cgi?id=126818

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

------- Additional Comments from Jer Noble <jer.noble at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=220941&action=review


r=me, with some nits. Go ahead and upload a new patch with a cq? flag and
someone will cq+ it.

> Source/WebCore/html/HTMLMediaElement.cpp:4916
> +    if (document().settings() &&
document().settings()->mediaPlaybackFullscreenAVKit()) {
> +	   if (m_platformLayerBorrowed)

I think the if (m_platformLayerBorrowed) check here is sufficient.

>> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.h:42
>> + at property (assign) WebView *webView;
> 
> WebCore is not allowed to know about WebView's existence. If you need the
host view, you should use some abstraction.

It looks like this is dead code.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:131
> +	   _mediaElement.get()->exitFullscreen();

RefPtr has an operator->, so no need to call .get() here.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:140
> +	   _mediaElement.get()->play();

Ditto.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:148
> +	   _mediaElement.get()->pause();

Ditto.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:156
> +	   _mediaElement.get()->togglePlayState();

Ditto.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:171
> +	       _mediaElement.get()->play();
> +	   else
> +	       _mediaElement.get()->pause();

Double ditto.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:183
> +	   ExceptionCode exceptionCode {};

ExceptionCode is an integer, so it can be initialized with `= 0`;

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:184
> +	   _mediaElement.get()->setCurrentTime(time, exceptionCode);

Ditto.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:190
> +    NSTimeInterval anchorTimeStamp = ([self rate] == 0.f) ? NAN :
[classAVValueTiming currentTimeStamp];

No need for the `.f`.  In fact, that conditional should be `![self rate]`.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:192
> +	 anchorTimeStamp:anchorTimeStamp rate:0];

Weird indenting here.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:251
> +    self.playerLayer = (CALayer<AVPlayerLayer>
*)_mediaElement.get()->borrowPlatformLayer();

Again, no need for .get().  This should have something like:

ASSERT([self.playerLayer isKindOfClass:classAVPlayerLayer]);

To ensure that we catch if anyone starts returning an unexpected layer type
here.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:254
> +    [self updateTimingWithCurrentTime:_mediaElement.get()->currentTime()];

No need for .get().

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:257
> +	   HTMLVideoElement *ve = toHTMLVideoElement(_mediaElement.get());

Please use a more descriptive variable name here, like "videoElement".

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:286
> +	   [self
updateTimingWithCurrentTime:_mediaElement.get()->currentTime()];

No need for .get().

> Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm:979
> +#if PLATFORM(IOS)
> +    if (Settings::mediaPlaybackFullscreenAVKit())
> +	   return isHTMLVideoElement(node);
> +    else
> +	   return false;
> +#else

Just to clean things up a little, this could be:

#if PLATFORM(IOS)
    if (!Settings::mediaPlaybackFullscreenAVKit())
	return false;
#endif
    return isHTMLVideoElement(node);