Bug 127886 – [iOS] Synchronize the WKContentView and UIScrollView updates with the tiles being commited

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Benjamin Poulain]

[iOS] Synchronize the WKContentView and UIScrollView updates with the tiles being commited

[Benjamin Poulain]

Created attachment 222625 [details]
Patch

[Benjamin Poulain]

Created attachment 222738 [details]
Patch

[Benjamin Poulain]

Comment on attachment 222738 [details]
Patch

Let's put this one ice. I'll look into changing the viewport handling first.

[Tim Horton]

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.

[Benjamin Poulain]

Created attachment 223274 [details]
Patch

[Simon Fraser (smfr)]

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!

[Benjamin Poulain]

Created attachment 223295 [details]
Patch

[Benjamin Poulain]

Created attachment 223304 [details]
Patch

[Benjamin Poulain]

Committed r163515: <http://trac.webkit.org/changeset/163515>