Fullscreen in WebKit2 does not restore topContentInset upon exiting; leaves top of page not visible

[Jer Noble]

Fullscreen in WebKit2 does not restore topContentInset upon exiting; leaves top of page not visible

[Jer Noble]

Created attachment 296729 [details]
Patch

[Darin Adler]

Comment on attachment 296729 [details]
Patch

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

No way to regression-test?

> Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:421
> +    _page->setTopContentInset(_savedTopContentInset);

Can we make this share some code? At least three lines of this code are identical to part of finishedEnterFullScreenAnimation: and all five lines are the same although for some reason they are not in the same order here and there.

We should make the two methods share as much as possible, to help us avoid bugs like this one.

[Jer Noble]

Created attachment 296935 [details]
Patch

[Jer Noble]

Created attachment 296952 [details]
Patch

[Tim Horton]

Comment on attachment 296952 [details]
Patch

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

> Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:521
> +    if ([_webView isKindOfClass:[WKWebView class]]) {

This should go through PageClient and be implemented in WebViewImpl for Mac.

[Jer Noble]

(In reply to comment #5)
> Comment on attachment 296952 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=296952&action=review
> 
> > Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.mm:521
> > +    if ([_webView isKindOfClass:[WKWebView class]]) {
> 
> This should go through PageClient and be implemented in WebViewImpl for Mac.

Tim told me (in person) to ignore the fact he said this.

[Jer Noble]

Created attachment 296970 [details]
Patch

[Tim Horton]

Comment on attachment 296970 [details]
Patch

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

> Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.h:63
> +    bool _savedAutomaticallyAdjustsContentInsets;

Don't think you use this anymore?

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/FullscreenTopContentInset.mm:61
> +    [webView _setAutomaticallyAdjustsContentInsets:NO];

Do you still need this? I don't think it's on by default, is it?

[Jer Noble]

(In reply to comment #8)
> Comment on attachment 296970 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=296970&action=review
> 
> > Source/WebKit2/UIProcess/mac/WKFullScreenWindowController.h:63
> > +    bool _savedAutomaticallyAdjustsContentInsets;
> 
> Don't think you use this anymore?

Good point.

> > Tools/TestWebKitAPI/Tests/WebKit2Cocoa/FullscreenTopContentInset.mm:61
> > +    [webView _setAutomaticallyAdjustsContentInsets:NO];
> 
> Do you still need this? I don't think it's on by default, is it?

It is on Mac.

Thanks!

[Jer Noble]

Created attachment 297020 [details]
Patch for landing

[Jer Noble]

Created attachment 297022 [details]
Patch for landing

[Jon Lee]

rdar://problem/27954473

[WebKit Commit Bot]

Comment on attachment 297022 [details]
Patch for landing

Clearing flags on attachment: 297022

Committed r209782: <http://trac.webkit.org/changeset/209782>