[webkit-reviews] review granted: [Bug 135038] Don't create new UIWindow for video fullscreen. : [Attachment 235108] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Jeremy Jones <jeremyj-wk at apple.com>'s
request for review:
Bug 135038: Don't create new UIWindow for video fullscreen.
https://bugs.webkit.org/show_bug.cgi?id=135038

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

------- Additional Comments from Darin Adler <darin at apple.com>
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.