Summary: | [iOS][WK2] Add SPI to do a dynamic viewport update without showing any content | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Benjamin Poulain <benjamin> | ||||||
Component: | New Bugs | Assignee: | Benjamin Poulain <benjamin> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | ian, thorton | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Benjamin Poulain
2014-07-16 22:59:10 PDT
Created attachment 235053 [details]
Patch
I welcome any better name for the SPI :) (In reply to comment #2) > I welcome any better name for the SPI :) Maybe _resizeImmediatelyWithUpdates:? Or just _resizeWithUpdates:? Comment on attachment 235053 [details] Patch 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". Created attachment 235091 [details]
Patch
Comment on attachment 235091 [details] Patch 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 on attachment 235091 [details] Patch Clearing flags on attachment: 235091 Committed r171201: <http://trac.webkit.org/changeset/171201> All reviewed patches have been landed. Closing bug. |