Bug 135010

Summary: [iOS][WK2] Add SPI to do a dynamic viewport update without showing any content
Product: WebKit Reporter: Benjamin Poulain <benjamin>
Component: New BugsAssignee: Benjamin Poulain <benjamin>
Severity: Normal CC: ian, thorton
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
Patch none

Description Benjamin Poulain 2014-07-16 22:59:10 PDT
[iOS][WK2] Add SPI to do a dynamic viewport update without showing any content
Comment 1 Benjamin Poulain 2014-07-16 23:16:23 PDT
Created attachment 235053 [details]
Comment 2 Benjamin Poulain 2014-07-16 23:17:37 PDT
I welcome any better name for the SPI :)
Comment 3 Ian Henderson 2014-07-17 04:16:07 PDT
(In reply to comment #2)
> I welcome any better name for the SPI :)

Maybe _resizeImmediatelyWithUpdates:?  Or just _resizeWithUpdates:?
Comment 4 Darin Adler 2014-07-17 08:59:32 PDT
Comment on attachment 235053 [details]

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

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:169
> +    BOOL _hasCommitedAnyPageLoad;

Spelling error in this. The word committed has two "t"s.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:173
> +    BOOL _isResizingWithDynamicViewportUpdate;
> +    BOOL _isDocumentHiddenDuringResize;

This would be better as a single field with three values. NotResizing, ResizingWithAnimation, or ResizingWithDocumentHidden.

The flag _isDocumentHiddenDuringResize means nothing unless _isResizingWithDynamicViewportUpdate is also YES, but the _beginAnimatedResizeWithUpdates method relies on _isDocumentHiddenDuringResize being NO already. 

Another possibility is to not have the _isDocumentHiddenDuringResize flag at all, since we can tell if the content view is hidden by calling [_contentView hidden]. Can't think of a good reason to keep our own separate copy of that state.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:776
> +    _hasCommitedAnyPageLoad = YES;

I think _hasCommittedLoadForMainFrame would be a better name since it matches the method name. No reason to use similar but different terminology for the same concept.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:2287
> +    [self _beginAnimatedResizeWithUpdates:updateBlock];

I think it’s strange that you went to the trouble to rename _isAnimatingResize but not to rename these methods, but the methods are used in both cases; presumably because the methods are directly exposed as SPI. I’m not sure the renaming is necessary. Treating the hidden document version as a special case of animating resizing seems OK to me.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebViewPrivate.h:140
> +- (void)_resizeWhileHiddingContentWithUpdates:(void (^)(void))updateBlock;

Spelling mistake here. The word hiding has only one "d".
Comment 5 Benjamin Poulain 2014-07-17 14:08:14 PDT
Created attachment 235091 [details]
Comment 6 Tim Horton 2014-07-17 14:18:50 PDT
Comment on attachment 235091 [details]

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

Seems fine to me. Too bad the mechanism I added in r171191 to hide the content until the next commit didn't also work to hide the content until an arbitrary commit ID too. I suppose you might have non-Web-layer-tree WKContentView children that you want to (and do) hide as well.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:122
> +enum class DynamicViewportUpdateMode {

is this a State or a Mode?
Comment 7 Benjamin Poulain 2014-07-17 15:09:07 PDT
Comment on attachment 235091 [details]

Clearing flags on attachment: 235091

Committed r171201: <http://trac.webkit.org/changeset/171201>
Comment 8 Benjamin Poulain 2014-07-17 15:09:10 PDT
All reviewed patches have been landed.  Closing bug.