Bug 143131 – Synchronize fullscreen animation between processes.

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

[Jeremy Jones]

Synchronize fullscreen animation between processes.

[Jeremy Jones]

Created attachment 249562 [details]
Patch

[Jeremy Jones]

rdar://problem/19948096

[Simon Fraser (smfr)]

Comment on attachment 249562 [details]
Patch

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

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:623
> +        id<CAAction> action = nil; // [self actionForKey:@"bounds"];

Please don't commit commented-out code. Reference a bug in a comment if you need to.

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:624
> +        if (action && action != [NSNull null]) {

Should this use -isEqual:?

> Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:38
> +#import <BackBoardServices/BKSAnimationFence.h>
> +#import <BackBoardServices/BKSAnimationFence_Private.h>

This will break the public iOS build.

> Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:40
> +#import <UIKit/UIWindow_Private.h>

Ditto.

> Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:56
> +    : m_page(&page), m_lastFencePort(0)

Each initialization on its own line please.

[Jer Noble]

Comment on attachment 249562 [details]
Patch

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

>> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:623
>> +        id<CAAction> action = nil; // [self actionForKey:@"bounds"];
> 
> Please don't commit commented-out code. Reference a bug in a comment if you need to.

This must have been inadvertent. It makes the following section meaningless.

>> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:624
>> +        if (action && action != [NSNull null]) {
> 
> Should this use -isEqual:?

It should probably just get reversed to:

if (!action || [action isKindOfClass:[NSNull class]])
    ...disableActions...
else {
    ...other stuff...
}

I'm curious, however, why the `action` doesn't seem to be used inside the following block:

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

Shouldn't we be matching the timing function and duration with that of the action?

[Jeremy Jones]

(In reply to comment #3)
> Comment on attachment 249562 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=249562&action=review
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:623
> > +        id<CAAction> action = nil; // [self actionForKey:@"bounds"];
> 
> Please don't commit commented-out code. Reference a bug in a comment if you
> need to.

Removed. Code, referenced radar.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:624
> > +        if (action && action != [NSNull null]) {
> 
> Should this use -isEqual:?

NSNull is a singleton, so this is valid, but I'll change it to -isEqual:

> 
> > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:38
> > +#import <BackBoardServices/BKSAnimationFence.h>
> > +#import <BackBoardServices/BKSAnimationFence_Private.h>
> 
> This will break the public iOS build.

Removed imports. Created BackBoardServiesSPI.h

> 
> > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:40
> > +#import <UIKit/UIWindow_Private.h>
> 
> Ditto.

Removed import. Added interfaces to UIKitSPI.h

> 
> > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:56
> > +    : m_page(&page), m_lastFencePort(0)
> 
> Each initialization on its own line please.

Moved to separate line.

[Jeremy Jones]

(In reply to comment #4)
> Comment on attachment 249562 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=249562&action=review
> 
> >> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:623
> >> +        id<CAAction> action = nil; // [self actionForKey:@"bounds"];
> > 
> > Please don't commit commented-out code. Reference a bug in a comment if you need to.

Removed commented out code. Referenced a radar.

> 
> This must have been inadvertent. It makes the following section meaningless.

Intentional. I'll remove that code also and put it in the radar. The commented out code causes big UI problems. Still need to find a better way to do that.

> 
> >> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:624
> >> +        if (action && action != [NSNull null]) {
> > 
> > Should this use -isEqual:?

NSNull is a singleton, so it is valid, but I've changed it to -isEqual:

> 
> It should probably just get reversed to:
> 
> if (!action || [action isKindOfClass:[NSNull class]])
>     ...disableActions...
> else {
>     ...other stuff...
> }

Reversed.

> 
> I'm curious, however, why the `action` doesn't seem to be used inside the
> following block:
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:626
> > +            [CATransaction setAnimationTimingFunction:[CAMediaTimingFunction functionWithName:kCAMediaTimingFunctionEaseInEaseOut]];
> > +            [CATransaction setAnimationDuration:0.4];
> 
> Shouldn't we be matching the timing function and duration with that of the
> action?

That action is a private class from UIKit and doesn't lend itself to proper inspection. I'm still looking for a better way to do that.

I'll remove this code and move it to the radar.

[Jeremy Jones]

Created attachment 249594 [details]
Patch

[Jeremy Jones]

Created attachment 249610 [details]
Patch

[Jer Noble]

Created attachment 250028 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 250028 [details]
Patch

Clearing flags on attachment: 250028

Committed r182439: <http://trac.webkit.org/changeset/182439>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jer Noble]

Follow up build fix committed.

Committed r182477: <http://trac.webkit.org/r182477>.