Bug 133706 – Improve enter and exit fullscreen transition by using two step transition.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Jeremy Jones]

Improve enter and exit fullscreen transition by using two step transition.

[Jeremy Jones]

radar://16878025

[Jeremy Jones]

Created attachment 232830 [details]
Patch

[Eric Carlson]

Comment on attachment 232830 [details]
Patch

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

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:742
> +    __block RefPtr<WebVideoFullscreenInterfaceAVKit> protect2(this);

Nit: You don't need the "2" on the variable name now that isn't in setExternalPlayback.

> Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.mm:195
> +    
> +    dispatch_async(dispatch_get_main_queue(), ^{

No need to protect "this"?

[Jeremy Jones]

(In reply to comment #3)
> (From update of attachment 232830 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=232830&action=review
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:742
> > +    __block RefPtr<WebVideoFullscreenInterfaceAVKit> protect2(this);
> 
> Nit: You don't need the "2" on the variable name now that isn't in setExternalPlayback.

Removed the 2.

> 
> > Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.mm:195
> > +    
> > +    dispatch_async(dispatch_get_main_queue(), ^{
> 
> No need to protect "this"?

Added a protect.

[Jeremy Jones]

Created attachment 232971 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 232971 [details]
Patch

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

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:156
> +        _model->setVideoFullscreenLayer(nullptr);

nil if that's a CALayer*

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:792
> +        [m_window setHidden:YES];
> +        [m_window setRootViewController:nil];
> +        [m_playerViewController setDelegate:nil];
> +        [m_playerViewController setPlayerController:nil];
> +        m_playerViewController = nil;
> +        m_viewController = nil;
> +        m_window = nil;
> +        [m_videoLayer removeFromSuperlayer];
> +        m_videoLayer = nil;
> +        [m_videoLayerContainer removeFromSuperlayer];
> +        [m_videoLayerContainer setPlayerViewController:nil];
> +        m_videoLayerContainer = nil;

So much stuff.

> Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.messages.in:33
> +    SetupFullscreenWithID(uint32_t videoLayerID, WebCore::IntRect initialRect)

Is this a GraphicsLayer::PlatformLayerID? If so, it needs to be 64-bit.

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

Blank line.

[Jeremy Jones]

(In reply to comment #6)
> (From update of attachment 232971 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=232971&action=review
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:156
> > +        _model->setVideoFullscreenLayer(nullptr);
> 
> nil if that's a CALayer*

It is a PlatformLayer, which I guess is close enough in an Obj-c file. Change.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:792
> > +        [m_window setHidden:YES];
> > +        [m_window setRootViewController:nil];
> > +        [m_playerViewController setDelegate:nil];
> > +        [m_playerViewController setPlayerController:nil];
> > +        m_playerViewController = nil;
> > +        m_viewController = nil;
> > +        m_window = nil;
> > +        [m_videoLayer removeFromSuperlayer];
> > +        m_videoLayer = nil;
> > +        [m_videoLayerContainer removeFromSuperlayer];
> > +        [m_videoLayerContainer setPlayerViewController:nil];
> > +        m_videoLayerContainer = nil;
> 
> So much stuff.

Sigh. Maybe a session object that cleans up all this implicitly in its destructor.

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

> 
> > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.messages.in:33
> > +    SetupFullscreenWithID(uint32_t videoLayerID, WebCore::IntRect initialRect)
> 
> Is this a GraphicsLayer::PlatformLayerID? If so, it needs to be 64-bit.

It is one of these:
uint32_t LayerHostingContext::contextID()
so it stays 32-bit.

> 
> > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:121
> > +    
> 
> Blank line.

Deleted.

[Jeremy Jones]

Created attachment 233203 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 233203 [details]
Patch for landing

Rejecting attachment 233203 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 233203, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Full output: http://webkit-queues.appspot.com/results/5873666126512128

[Jeremy Jones]

Created attachment 233263 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 233263 [details]
Patch for landing.

Clearing flags on attachment: 233263

Committed r170083: <http://trac.webkit.org/changeset/170083>