RESOLVED FIXED 135038
Don't create new UIWindow for video fullscreen.
https://bugs.webkit.org/show_bug.cgi?id=135038
Summary Don't create new UIWindow for video fullscreen.
Jeremy Jones
Reported 2014-07-17 18:42:23 PDT
Don't create new UIWindow for video fullscreen.
Attachments
Patch (14.91 KB, patch)
2014-07-17 18:55 PDT, Jeremy Jones
darin: review+
Patch for landing. (20.00 KB, patch)
2014-07-21 20:17 PDT, Jeremy Jones
no flags
Jeremy Jones
Comment 1 2014-07-17 18:43:08 PDT
Jeremy Jones
Comment 2 2014-07-17 18:55:26 PDT
Eric Carlson
Comment 3 2014-07-17 22:50:04 PDT
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 4 2014-07-20 20:49:12 PDT
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
Comment 5 2014-07-21 19:54:28 PDT
(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
Comment 6 2014-07-21 19:58:13 PDT
(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
Comment 7 2014-07-21 20:17:01 PDT
Created attachment 235266 [details] Patch for landing.
WebKit Commit Bot
Comment 8 2014-07-21 20:20:15 PDT
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 9 2014-07-22 10:19:41 PDT
Comment on attachment 235266 [details] Patch for landing. Clearing flags on attachment: 235266 Committed r171345: <http://trac.webkit.org/changeset/171345>
Note You need to log in before you can comment on or make changes to this bug.