Improve fullscreen video rotation animation.
Created attachment 238213 [details] Patch
rdar://problem/17950149
Comment on attachment 238213 [details] Patch 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.
Created attachment 238263 [details] Patch
(In reply to comment #3) > (From update of attachment 238213 [details]) > 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. Not sure what to say here. This just formalized the relationship that already existed. See the change from addSublayer: to setVideoSublayer: > > > 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. > Comment added. > > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:539 > > + [CATransaction setAnimationTimingFunction:[CAMediaTimingFunction functionWithName:kCAMediaTimingFunctionEaseInEaseOut]]; > > Do you want to explicitly end the transaction after setting the properties? Added transaction begin/commit. > > > 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? I removed this and moved LayerHostingContext off of WKCAContext per AVSystemInterface guidelines. > > > 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. Removed.
Comment on attachment 238263 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238263&action=review > Source/WebKit2/Platform/mac/LayerHostingContext.mm:51 > + layerHostingContext->m_context = (CAContext*)WKCAContextMakeRemoteWithServerPort(serverPort); CAContext * > Source/WebKit2/Platform/mac/LayerHostingContext.mm:86 > + return [m_context setLayer:rootLayer]; no return. > Source/WebKit2/Platform/mac/LayerHostingContext.mm:102 > + return [m_context invalidate]; no return > Source/WebKit2/Platform/mac/LayerHostingContext.mm:107 > + return [m_context setColorSpace:colorSpace]; no return > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:37 > +#include <UIKit/UIWindow_Private.h> This will annoy dbates who is trying to make us build with the public SDK. You should do the platform/spi thing.
Created attachment 238283 [details] Patch
Comment on attachment 238283 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238283&action=review > Source/WebKit2/Platform/mac/LayerHostingContext.mm:41 > +@interface CAContext : NSObject (Details) > +- (void)invalidate; > +@property (readonly) uint32_t contextId; > +@property (strong) CALayer *layer; > +@property CGColorSpaceRef colorSpace; > +- (void)setFencePort:(mach_port_t)port; > + (CAContext *)remoteContextWithOptions:(NSDictionary *)dict; > @end This should really go into its own spi header, but that can happen in a later commit. > Source/WebKit2/Platform/mac/LayerHostingContext.mm:88 > + return [m_context setLayer:rootLayer]; No return. > Source/WebKit2/Platform/mac/LayerHostingContext.mm:104 > + return [m_context invalidate]; No return. > Source/WebKit2/Platform/mac/LayerHostingContext.mm:109 > + return [m_context setColorSpace:colorSpace]; No return.
Created attachment 238284 [details] Patch
Comment on attachment 238284 [details] Patch Patch doesn't apply to TOT!
(In reply to comment #10) > (From update of attachment 238284 [details]) > Patch doesn't apply to TOT! dbates backed out his changes for platform/spi. So I'm removing mine also.
Created attachment 238285 [details] Patch
Created attachment 238318 [details] Patch
Comment on attachment 238318 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238318&action=review > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:540 > + if (animationDuration == 0.0) // a duration of 0 for CA means 0.25. This is a way to approximate 0. We normally write this as if (!animationDuration)
Created attachment 238330 [details] Patch for landing.
(In reply to comment #14) > (From update of attachment 238318 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=238318&action=review > > > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:540 > > + if (animationDuration == 0.0) // a duration of 0 for CA means 0.25. This is a way to approximate 0. > > We normally write this as if (!animationDuration) Fixed.
Comment on attachment 238330 [details] Patch for landing. Clearing flags on attachment: 238330 Committed r173741: <http://trac.webkit.org/changeset/173741>
I get the following build error after this change (but the bots seem fine): Source/WebKit2/Platform/mac/LayerHostingContext.mm:121:16: error: instance method '-setFencePort:' not found (return type defaults to 'id') [-Werror,-Wobjc-method-access] [m_context setFencePort:fencePort]; ^~~~~~~~~~~~ Anyone knows what I am missing?
Mac build fixed in https://trac.webkit.org/r173762.
(In reply to comment #19) > Mac build fixed in https://trac.webkit.org/r173762. You rock Simon :) Thanks.