[iOS] Synchronize the WKContentView and UIScrollView updates with the tiles being commited
Created attachment 222625 [details] Patch
Created attachment 222738 [details] Patch
Comment on attachment 222738 [details] Patch Let's put this one ice. I'll look into changing the viewport handling first.
Comment on attachment 222738 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222738&action=review r+ from smfr and I if you do choose to go this route > Source/WebKit2/ChangeLog:24 > + 1) A source (the page or the user) change parameters of the geometry. s/change/changes/ > Source/WebKit2/Shared/mac/RemoteLayerTreeTransaction.h:158 > + const WebCore::IntSize& renderedPageSize() const { return m_renderedPageSize; } anders would tell you not to return a const& because otherwise it will fit in a register. or something. > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:70 > + BOOL _hasPendingPageReset; I don't think reset is the word you want (we're not sure what it means). viewportUpdatePending? smfr says "the terminology should match whatever's called when whatever's pending is no longer pending" :D > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:341 > + // Do not mess with UIScrollView during using interaction, that causes undefined behaviors. s/using/user/? > Source/WebKit2/UIProcess/API/ios/WKContentView.mm:315 > if ([_delegate respondsToSelector:@selector(contentView:contentsSizeDidChange:)]) > [_delegate contentView:self contentsSizeDidChange:contentsSize]; we're a bit curious what this is about, and why setBounds is conditional > Source/WebKit2/UIProcess/API/ios/WKViewIOS.mm:278 > + // It is common to have the view initialized with a null frame. The resulting WebKit viewport configuration This is really weird. > Source/WebKit2/UIProcess/API/ios/WKViewIOS.mm:286 > + size = [[UIScreen mainScreen] bounds].size; Why the screen size? Why is this common? Why does it matter if it does happen? Shouldn't we just adjust the viewport when we get a valid frame? > Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm:325 > + layerTransaction.setRenderedPageSize(m_webPage->corePage()->mainFrame().view()->contentsSize()); > + layerTransaction.setRenderedPageScale(m_webPage->corePage()->pageScaleFactor()); Can we call renderedPageSize mainFrameContentsSize or something? and use the whole name for pageScaleFactor. and I don't think we need the "rendered" part. it's inherently the size and scale that go along with the transaction.
Created attachment 223274 [details] Patch
Comment on attachment 223274 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=223274&action=review > Source/WebCore/page/VirtualViewport.cpp:51 > + m_defaultConfiguration.width = 980; > + m_defaultConfiguration.widthIsSet = true; > + m_defaultConfiguration.allowsUserScaling = true; > + m_defaultConfiguration.minimumScale = 0.25; > + m_defaultConfiguration.maximumScale = 5; The nicer way to do this would be to give VirtualViewportConfiguration a member function that returns a pre-populated VirtualViewportConfiguration with these values. > Source/WebCore/page/VirtualViewport.cpp:60 > + ASSERT(virtualViewportConfiguration.maximumScale >= virtualViewportConfiguration.minimumScale); Why isn't this assertion inside of VirtualViewportConfiguration? > Source/WebCore/page/VirtualViewport.cpp:151 > +double VirtualViewport::maximumScale() const > +{ > + return m_configuration.maximumScale; > +} > + > +bool VirtualViewport::allowsUserScaling() const > +{ > + return m_configuration.allowsUserScaling; > +} Could be inline. > Source/WebCore/page/VirtualViewport.h:38 > + bzero(this, sizeof(VirtualViewportConfiguration)); We normally initialize the members explicitly. > Source/WebKit2/Shared/mac/RemoteLayerTreeTransaction.h:159 > + WebCore::IntSize mainFrameContentsSize() const { return m_mainFrameContentsSize; } > + void setMainFrameContentsSize(const WebCore::IntSize& size) { m_mainFrameContentsSize = size; }; Not sure you need "main frame" in the name. Just contentsSize would be fine. > Source/WebKit2/UIProcess/API/ios/WKContentView.h:49 > +- (void)contentView:(WKContentView *)contentView didCommitLayerTree:(const WebKit::RemoteLayerTreeTransaction&)layerTreeTransaction; It's a bit gross to see an internal detail of layer tree committing (RemoteLayerTreeTransaction) here. Can you just pass the values you care about? > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3994 > + defaultConfiguration.width = 980; > + defaultConfiguration.widthIsSet = true; > + defaultConfiguration.allowsUserScaling = true; > + defaultConfiguration.minimumScale = 0.25; > + defaultConfiguration.maximumScale = 5; This is the same as the VirtualViewport::VirtualViewport() one!
Created attachment 223295 [details] Patch
Created attachment 223304 [details] Patch
Committed r163515: <http://trac.webkit.org/changeset/163515>