Don't create new UIWindow for video fullscreen.
<rdar://problem/16655188>
Created attachment 235108 [details] Patch
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?
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.
(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.
(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.
Created attachment 235266 [details] Patch for landing.
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.
Comment on attachment 235266 [details] Patch for landing. Clearing flags on attachment: 235266 Committed r171345: <http://trac.webkit.org/changeset/171345>