[webkit-reviews] review granted: [Bug 179413] Make WKFullScreenWindowController more robust against modification by the embedding app : [Attachment 326310] 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 179413: Make WKFullScreenWindowController more robust against modification
by the embedding app
https://bugs.webkit.org/show_bug.cgi?id=179413

Attachment 326310: Patch

https://bugs.webkit.org/attachment.cgi?id=326310&action=review




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 326310
  --> https://bugs.webkit.org/attachment.cgi?id=326310
Patch

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

r=me if you fix the storage leak, but also please consider my other comments
and make sure you do substantial testing

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:6224
> + at implementation WKWebView (FullscreenMode)

Why put this in a separate category rather than in the main WKWebView
implementation? Seems like it would be easy to accidentally write two different
overrides of this method, but if in the main implementation we would get a
compile time error if we made that mistake. I suppose there might be a benefit
to using a category if we wanted it in a separate source file, but here it’s in
the main source file with an #if.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:6226
> +- (void)removeFromSuperview {

Brace goes on a separate line in WebKit coding style.

> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:186
> -    [self setView:adoptNS([[UIView alloc] initWithFrame:[[UIScreen
mainScreen] bounds]]).get()];
> +    [self setView:adoptNS([[UIView alloc] init]).get()];

Code like this has been dangerous in the past; objects with zero frames ended
up in strange state later after resizing. How much have you tested with this
change?

> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:417
> +    [[_window rootViewController] setView:[[UIView alloc]
initWithFrame:[_window bounds]]];

Missing adoptNS here. I think that means we will have a storage leak.

> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:420
> +    [_window setWindowLevel:UIWindowLevelNormal-1];

Missing spaces around the "-" here.

> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:460
> +	       [protectedSelf _manager]->willEnterFullScreen();

Is there a guarantee that _manager will be non-null later when this is called
back? By then the page could be null.

Cleaner way to write protectedSelf without stating the long type:

    protectedSelf = retainPtr(self)

> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:569
>	   [_webView _page]->setSuppressVisibilityUpdates(false);

Is there a guarantee that _page will be non-null later when this is called
back?

> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:596
> +- (void)webViewDidRemoveFromSuperviewWhileInFullscreen {

Brace goes on next line.