Bug 133417 – [iOS][WK2] Round the UIScrollView content size to device pixel

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

[Benjamin Poulain]

[iOS][WK2] Round the UIScrollView content size to device pixel

[Benjamin Poulain]

Created attachment 232314 [details]
Patch

[Benjamin Poulain]

<rdar://problem/15922440>

[zalan]

Comment on attachment 232314 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=232314&action=review

> Source/WebKit2/ChangeLog:11
> +        This patch ensure the content size as seen by the API is rounded to device pixels.

rounded or floored?

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:455
> +static inline CGFloat floorFloatToPixels(CGFloat input, float deviceScaleFactor)

We've been using roundToDevicePixel, floorToDevicePixel terms in WebCore. Having 'float' in the function name doesn't add any useful information, while it's missing what the rounding unit is.
Any explanation why we floor instead of round or ceil? (I guess, it's because you don't want garbage at the edges, but it'd be helpful to explain it in the changelog)

[Ian Henderson]

Comment on attachment 232314 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=232314&action=review

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:460
> +static CGSize roundScrollViewCountentSize(const WebKit::WebPageProxy& page, const CGSize& contentSize)

"Countent" -> "Content"

Is there a reason to pass contentSize by const reference instead of by value here?  It seems likely it'll all be inlined anyway.

Everything else looks good to me, thanks!

[Benjamin Poulain]

> Is there a reason to pass contentSize by const reference instead of by value here?  It seems likely it'll all be inlined anyway.

It is an habit. We pass stuff by const reference when they cannot be passed as registers. The "default" is to use const reference for non POD.

Here the CGSize can be passed by registers, so I should not have used const references.

[Benjamin Poulain]

Created attachment 232401 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 232401 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=232401&action=review

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:460
> +static CGSize roundScrollViewContentSize(const WebKit::WebPageProxy& page, CGSize contentSize)

Why not just pass in page.deviceScaleFactor()?

[Benjamin Poulain]

(In reply to comment #7)
> (From update of attachment 232401 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=232401&action=review
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:460
> > +static CGSize roundScrollViewContentSize(const WebKit::WebPageProxy& page, CGSize contentSize)
> 
> Why not just pass in page.deviceScaleFactor()?

Too many call sites, the repetition is not worth it.

[Benjamin Poulain]

Committed r169549: <http://trac.webkit.org/changeset/169549>