Bug 129870 – Allow media element to supply the video layer after fullscreen transition has already begun.

• Component: New Bugs
• Status: RESOLVED FIXED

[Jeremy Jones]

Allow media element to supply the video layer after fullscreen transition has already begun.

[Jeremy Jones]

Created attachment 226110 [details]
Patch

[Eric Carlson]

Comment on attachment 226110 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:758
> +    void MediaPlayerPrivateAVFoundationObjC::setVideoFullscreenGravity(MediaPlayer::VideoGravity gravity)

Nit: you don't need this indentation.

> Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.mm:135
> +        exitFullscreenForNode(m_node.get());

"exitFullscreenForNode" called from didEnterFullscreen?

> Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.mm:156
> +        enterFullscreenForNode(m_node.get());

"enterFullscreenForNode" called from didExitFullscreen?

> Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.mm:166
> +

Nit: extra blank line.

[Jon Lee]

Comment on attachment 226110 [details]
Patch

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

> Source/WebCore/ChangeLog:35
> +        Stash the gravity propety, pass it along to the media player if possible.

property*

> Source/WebCore/html/HTMLMediaElement.h:800
> +    RetainPtr<PlatformLayer> m_videoFullscreenLayer;

Should these variables be moved after the flags?

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:501
> +        {

Brace on same line as "if"

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:758
>> +    void MediaPlayerPrivateAVFoundationObjC::setVideoFullscreenGravity(MediaPlayer::VideoGravity gravity)
> 
> Nit: you don't need this indentation.

Remove excess indentation

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:764
> +    NSString* videoGravity = AVLayerVideoGravityResizeAspect;

NSString *videoGravity

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:768
> +        videoGravity = AVLayerVideoGravityResize;

It seems this might be slightly better:

switch (gravity) {
case VideoGravityResizeAspectFill:
    videoGravity = AVLayerVideoGravityResizeAspectFill;
    break;
…
default:
    ASSERT_NOT_REACHED();
}

only in that any additional modes would trigger an assert.

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:201
> +    _avPlayerController = (WebAVPlayerController*)playerController;

missing space: (WebAVPlayerController *)playerController

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:215
> +    {

There's enough of the consequent clause here to return early to avoid the indentation.

> Source/WebCore/platform/ios/WebVideoFullscreenModel.h:45
> +    enum VideoGravity { VideoGravityReize, VideoGravityResizeAspect, VideoGravityResizeAspectFill};

Missing space separating last enum from brace

> Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:94
> +    else

Our style puts the concluding brace and "else" on the same line.

> Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:176
> +    [m_videoFullscreenLayer setFrame:CGRectMake(rect.x(), rect.y(), rect.width(), rect.height())];

You can use CGRect(rect) here. (See FloatRectCG.cpp)

> Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:186
> +        videoGravity = MediaPlayer::VideoGravityResizeAspectFill;

same thing here re: switch statement

> Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:108
> +    

Remove excess space.

> Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:113
> +    

Ditto

> Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.mm:70
> +    return isHTMLVideoElement(node);

Would be clearer to write this simply as

return Settings::avKitEnabled() && isHTMLVideoElement(node);

[Jon Lee]

<rdar://problem/16207968>

[Jeremy Jones]

(In reply to comment #2)
> (From update of attachment 226110 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226110&action=review
> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:758
> > +    void MediaPlayerPrivateAVFoundationObjC::setVideoFullscreenGravity(MediaPlayer::VideoGravity gravity)
> 
> Nit: you don't need this indentation.

Done.

> 
> > Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.mm:135
> > +        exitFullscreenForNode(m_node.get());
> 
> "exitFullscreenForNode" called from didEnterFullscreen?

Surprising but intended. This handles the case the exitFullScreen was called while animating into fullscreen.

> 
> > Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.mm:156
> > +        enterFullscreenForNode(m_node.get());
> 
> "enterFullscreenForNode" called from didExitFullscreen?

Ditto.

> 
> > Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.mm:166
> > +
> 
> Nit: extra blank line.

Done.

[Jeremy Jones]

(In reply to comment #3)
> (From update of attachment 226110 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226110&action=review
> 
> > Source/WebCore/ChangeLog:35
> > +        Stash the gravity propety, pass it along to the media player if possible.
> 
> property*

Done.

> 
> > Source/WebCore/html/HTMLMediaElement.h:800
> > +    RetainPtr<PlatformLayer> m_videoFullscreenLayer;
> 
> Should these variables be moved after the flags?

I moved this and moved the initializer to match the order.

> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:501
> > +        {
> 
> Brace on same line as "if"

Done.

> 
> >> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:758
> >> +    void MediaPlayerPrivateAVFoundationObjC::setVideoFullscreenGravity(MediaPlayer::VideoGravity gravity)
> > 
> > Nit: you don't need this indentation.
> 
> Remove excess indentation

Done.

> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:764
> > +    NSString* videoGravity = AVLayerVideoGravityResizeAspect;
> 
> NSString *videoGravity

Done.

> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:768
> > +        videoGravity = AVLayerVideoGravityResize;
> 
> It seems this might be slightly better:
> 
> switch (gravity) {
> case VideoGravityResizeAspectFill:
>     videoGravity = AVLayerVideoGravityResizeAspectFill;
>     break;
> …
> default:
>     ASSERT_NOT_REACHED();
> }

Done, using an else clause.

> 
> only in that any additional modes would trigger an assert.
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:201
> > +    _avPlayerController = (WebAVPlayerController*)playerController;
> 
> missing space: (WebAVPlayerController *)playerController

Done.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:215
> > +    {
> 
> There's enough of the consequent clause here to return early to avoid the indentation.

Done.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenModel.h:45
> > +    enum VideoGravity { VideoGravityReize, VideoGravityResizeAspect, VideoGravityResizeAspectFill};
> 
> Missing space separating last enum from brace

Done. And fixed another occurrence of this in MediaPlayer. And fixed typo VideoGravityResize

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:94
> > +    else
> 
> Our style puts the concluding brace and "else" on the same line.

Done.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:176
> > +    [m_videoFullscreenLayer setFrame:CGRectMake(rect.x(), rect.y(), rect.width(), rect.height())];
> 
> You can use CGRect(rect) here. (See FloatRectCG.cpp)

Done.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:186
> > +        videoGravity = MediaPlayer::VideoGravityResizeAspectFill;
> 
> same thing here re: switch statement

Done, using an else.

Also fixed same pattern in WebAVVideoLayer.

> 
> > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:108
> > +    
> 
> Remove excess space.

Done.

> 
> > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:113
> > +    
> 
> Ditto

Ditto.

> 
> > Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.mm:70
> > +    return isHTMLVideoElement(node);
> 
> Would be clearer to write this simply as
> 
> return Settings::avKitEnabled() && isHTMLVideoElement(node);

Done.

[Eric Carlson]

Comment on attachment 226110 [details]
Patch

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

>>> Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.mm:135
>>> +        exitFullscreenForNode(m_node.get());
>> 
>> "exitFullscreenForNode" called from didEnterFullscreen?
> 
> Surprising but intended. This handles the case the exitFullScreen was called while animating into fullscreen.

It probably deserves an explanatory comment, others will probably misunderstand this as well.

>>> Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.mm:156
>>> +        enterFullscreenForNode(m_node.get());
>> 
>> "enterFullscreenForNode" called from didExitFullscreen?
> 
> Ditto.

Ditto.

[Jeremy Jones]

Created attachment 226144 [details]
Patch

[Simon Fraser (smfr)]

r+ (bugzilla is broken)

[Simon Fraser (smfr)]

Test

[Jeremy Jones]

(In reply to comment #7)
> (From update of attachment 226110 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226110&action=review
> 
> >>> Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.mm:135
> >>> +        exitFullscreenForNode(m_node.get());
> >> 
> >> "exitFullscreenForNode" called from didEnterFullscreen?
> > 
> > Surprising but intended. This handles the case the exitFullScreen was called while animating into fullscreen.
> 
> It probably deserves an explanatory comment, others will probably misunderstand this as well.
> 
> >>> Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.mm:156
> >>> +        enterFullscreenForNode(m_node.get());
> >> 
> >> "enterFullscreenForNode" called from didExitFullscreen?
> > 
> > Ditto.
> 
> Ditto.

Added comments.

[Jeremy Jones]

(In reply to comment #9)
> r+ (bugzilla is broken)

I'm submitting a rebased version to prevent a merge conflict.

[Jeremy Jones]

Created attachment 226178 [details]
Patch

[Eric Carlson]

Comment on attachment 226178 [details]
Patch

Marking r+ based on Simon's earlier review.

[WebKit Commit Bot]

Comment on attachment 226178 [details]
Patch

Clearing flags on attachment: 226178

Committed r165344: <http://trac.webkit.org/changeset/165344>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.