Bug 135038 – Don't create new UIWindow for video fullscreen.

• Component: WebKit2
• Status: RESOLVED FIXED

[Jeremy Jones]

Don't create new UIWindow for video fullscreen.

[Jeremy Jones]

<rdar://problem/16655188>

[Jeremy Jones]

Created attachment 235108 [details]
Patch

[Eric Carlson]

Comment on attachment 235108 [details]
Patch

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

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:743
> +    assert([parentLayer.delegate isKindOfClass:[getUIViewClass() class]]);

Won't this assert in a release build? Do we really want to crash, wouldn't logging and returning be sufficient?

[Darin Adler]

Comment on attachment 235108 [details]
Patch

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

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.h:38
> +- (void)enterFullscreen:(WAKWindow *)window;

Instead of a WAKWindow, I suggest passing a UIView here. Or at least the layer object. It’d be nice to reduce the use of WAKWindow rather than increasing it over time. I’d like to remove the WAK stuff eventually.

>> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:743
>> +    assert([parentLayer.delegate isKindOfClass:[getUIViewClass() class]]);
> 
> Won't this assert in a release build? Do we really want to crash, wouldn't logging and returning be sufficient?

If we want a WebKit assert, for debug builds only, then we should use ASSERT rather than assert.

But I agree that just doing an early return might be a better alternative.

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:751
> +        UIView* parentView = (UIView *)parentLayer.delegate;

Formatting mistake here; it should be UIView *parentView.

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

This idiom seems a bit peculiar. I think a brief “why” comment explaining why it’s helpful to protect until we are done exiting would be worthwhile.

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:867
> +        [m_playerViewController exitFullScreenWithCompletionHandler:^(BOOL, NSError*)
> +        {

The { should be on the line above with the ^ to follow our standard formatting for blocks, as in the outer block.

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:868
> +            protect.clear();

The idiom is now:

    protect = nullptr;

I think we are going to deprecate the clear function, in fact.

> Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.mm:57
> +    return toElement(node)->clientRect();

Please rename this function since we’re changing what it does. Just doing the mechanical thing and renaming it to clientRectForNode would be sufficient for now.

[Jeremy Jones]

(In reply to comment #3)
> (From update of attachment 235108 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235108&action=review
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:743
> > +    assert([parentLayer.delegate isKindOfClass:[getUIViewClass() class]]);
> 
> Won't this assert in a release build? Do we really want to crash, wouldn't logging and returning be sufficient?

Changed the function to take a UIView, so this is no longer necessary.

[Jeremy Jones]

(In reply to comment #4)
> (From update of attachment 235108 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235108&action=review
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.h:38
> > +- (void)enterFullscreen:(WAKWindow *)window;
> 
> Instead of a WAKWindow, I suggest passing a UIView here. Or at least the layer object. It’d be nice to reduce the use of WAKWindow rather than increasing it over time. I’d like to remove the WAK stuff eventually.

UIView it is. This simplifies things a bit.

> 
> >> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:743
> >> +    assert([parentLayer.delegate isKindOfClass:[getUIViewClass() class]]);
> > 
> > Won't this assert in a release build? Do we really want to crash, wouldn't logging and returning be sufficient?
> 
> If we want a WebKit assert, for debug builds only, then we should use ASSERT rather than assert.
> 
> But I agree that just doing an early return might be a better alternative.

I changed the function to take a UIView, so this isn't necessary.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:751
> > +        UIView* parentView = (UIView *)parentLayer.delegate;
> 
> Formatting mistake here; it should be UIView *parentView.

Fixed.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:863
> > +    __block RefPtr<WebVideoFullscreenInterfaceAVKit> protect(this);
> 
> This idiom seems a bit peculiar. I think a brief “why” comment explaining why it’s helpful to protect until we are done exiting would be worthwhile.

It is used throughout the file. I'll comment this case.
// Retain this to extend object life until async block completes.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:867
> > +        [m_playerViewController exitFullScreenWithCompletionHandler:^(BOOL, NSError*)
> > +        {
> 
> The { should be on the line above with the ^ to follow our standard formatting for blocks, as in the outer block.

I'll make the change. For some reason style check has been getting confused and insisting I put in on the following line. I'll also fix the other occurrence of this.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:868
> > +            protect.clear();
> 
> The idiom is now:
> 
>     protect = nullptr;
> 
> I think we are going to deprecate the clear function, in fact.
> 

Updated all instances of .clear(); in the file.

> > Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.mm:57
> > +    return toElement(node)->clientRect();
> 
> Please rename this function since we’re changing what it does. Just doing the mechanical thing and renaming it to clientRectForNode would be sufficient for now.

Renamed.

[Jeremy Jones]

Created attachment 235266 [details]
Patch for landing.

[WebKit Commit Bot]

Attachment 235266 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:810:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:866:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 235266 [details]
Patch for landing.

Clearing flags on attachment: 235266

Committed r171345: <http://trac.webkit.org/changeset/171345>