[webkit-reviews] review granted: [Bug 132978] [iOS WK2] When zoomed, fixed elements jump at the start of a scroll, and jump back at the end. : [Attachment 231545] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 132978: [iOS WK2] When zoomed, fixed elements jump at the start of a
scroll, and jump back at the end.
https://bugs.webkit.org/show_bug.cgi?id=132978

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=231545&action=review


Looks good to me but we'll have to look into the regression.

> Source/WebKit2/UIProcess/PageClient.h:260
> +    virtual float minimumZoomScale() const = 0;

Maybe displayedContentMinimumZoomScale()?

It would be nice to differentiate that scale from the real zoom scale we have
in the content.

displayedContentMinimumZoomScale() would make it more obvious when comparing
that value to displayedContentScale() in WebPageProxy.

> Source/WebKit2/UIProcess/PageClient.h:261
> +    virtual WebCore::FloatSize contentsSize() const = 0;

That make sense for our current architecture...but it is sooo weird the page
proxy has to ask the client for the content size.

Maybe we should store a copy of the contentSize on WebPageProxy and update it
on WebPageProxy::didCommitLayerTree()?

> Source/WebKit2/UIProcess/ios/PageClientImplIOS.mm:190
> +float PageClientImpl::minimumZoomScale() const

I would make this a double.

> Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:223
> +    float scale = displayedContentScale();

This should be a double.

> Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:224
> +    float minimumScale = m_pageClient.minimumZoomScale();

ditto

> Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:228
> +	   const CGFloat slope = 12;
> +	   CGFloat factor = std::max<CGFloat>(1 - slope * (minimumScale -
scale), 0);

Let's use double here instead of CGFloat now that this code is in WebPageProxy.