[webkit-reviews] review granted: [Bug 135010] [iOS][WK2] Add SPI to do a dynamic viewport update without showing any content : [Attachment 235053] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Jul 17 08:59:16 PDT 2014
Darin Adler <darin at apple.com> has granted Benjamin Poulain
<benjamin at webkit.org>'s request for review:
Bug 135010: [iOS][WK2] Add SPI to do a dynamic viewport update without showing
any content
https://bugs.webkit.org/show_bug.cgi?id=135010
Attachment 235053: Patch
https://bugs.webkit.org/attachment.cgi?id=235053&action=review
------- Additional Comments from Darin Adler <darin at apple.com>
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".
More information about the webkit-reviews
mailing list