[webkit-reviews] review granted: [Bug 136870] Improve fullscreen video rotation animation. : [Attachment 238213] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Jeremy Jones
<jeremyj-wk at apple.com>'s request for review:
Bug 136870: Improve fullscreen video rotation animation.
https://bugs.webkit.org/show_bug.cgi?id=136870

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=238213&action=review


> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:486
> +    RetainPtr<CALayer> _videoSublayer;

WebAVVideoLayer has a videoSublayer? Needs some comments so we know which
layers are which.

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:538
> +    [CATransaction setAnimationDuration:[self
animationForKey:@"bounds"].duration ?:0.0001];

?:0.0001 is weird, both the magic number and the ?: This needs an explanatory
comment.

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:539
> +    [CATransaction setAnimationTimingFunction:[CAMediaTimingFunction
functionWithName:kCAMediaTimingFunctionEaseInEaseOut]];

Do you want to explicitly end the transaction after setting the properties?

> Source/WebKit2/Platform/mac/LayerHostingContext.mm:116
> +    WKCAContextSetFencePort(m_context.get(), fencePort);

I don't see WKCAContextSetFencePort anywhere. I assume this depends on other
changes?

> Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:43
> +SOFT_LINK_FRAMEWORK(UIKit)
> +SOFT_LINK_CLASS(UIKit, UIWindow)

WebKit2 doesn't need to soft link with UIKit.