[webkit-reviews] review granted: [Bug 127886] [iOS] Synchronize the WKContentView and UIScrollView updates with the tiles being commited : [Attachment 223274] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Benjamin Poulain
<benjamin at webkit.org>'s request for review:
Bug 127886: [iOS] Synchronize the WKContentView and UIScrollView updates with
the tiles being commited
https://bugs.webkit.org/show_bug.cgi?id=127886

Attachment 223274: Patch
https://bugs.webkit.org/attachment.cgi?id=223274&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
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!