[Webkit-unassigned] [Bug 126818] Add AVKit fullscreen video interface.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=126818


Jer Noble <jer.noble at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #220941|review?                     |review+
               Flag|                            |




--- Comment #3 from Jer Noble <jer.noble at apple.com>  2014-01-11 21:01:18 PST ---
(From update of attachment 220941)
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);

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.