[webkit-reviews] review granted: [Bug 130555] [iOS][WK2] Reduce the tiling coverage to the current rect and 1 tile size ahead : [Attachment 227379] 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 130555: [iOS][WK2] Reduce the tiling coverage to the current rect and 1
tile size ahead
https://bugs.webkit.org/show_bug.cgi?id=130555

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=227379&action=review


> Source/WebCore/platform/ScrollView.h:188
> +    void setScrollVelocityInformation(double horizontalVelocity, double
verticalVelocity, double timestamp);

FloatSize?
"information" seems redundant.

> Source/WebCore/platform/ScrollView.h:189
> +    FloatRect computeCoverageRect(double horizontalMargin, double
verticalMargin);

Are these margins on both sides?
Function should be const.

> Source/WebCore/platform/ScrollView.h:418
> +    double m_horizontalVelocity;
> +    double m_verticalVelocity;

FloatSize?

> Source/WebCore/platform/ScrollView.h:419
> +    double m_velocityUpdateTimestamp;

I'd call this m_lastVelocityUpdateTime

> Source/WebCore/platform/ios/ScrollViewIOS.mm:149
> +	   futureRect.setWidth(futureRect.width() + horizontalMargin);
> +	   if (m_horizontalVelocity < 0)
> +	       futureRect.setX(std::max(futureRect.x() - horizontalMargin,
0.));
> +    }
> +
> +    if (m_verticalVelocity) {
> +	   futureRect.setHeight(futureRect.height() + verticalMargin);
> +	   if (m_verticalVelocity < 0)
> +	       futureRect.setY(std::max(futureRect.y() - verticalMargin, 0.));
> +    }

I think we need to clamp when velocity is really high, to avoid making too many
tiles.

> Source/WebKit2/Shared/VisibleContentRectUpdateInfo.h:80
> +    double m_horizontalVelocity;
> +    double m_verticalVelocity;

FloatSize?

> Source/WebKit2/UIProcess/ios/WKContentView.mm:66
> +class HistoricalKinematicData {

VelocityData maybe? Kinematic sounds like we're doing multi-body simulation.

> Source/WebKit2/UIProcess/ios/WKContentView.mm:75
> +    CGPoint velocityForNewData(CGPoint newPosition, double timestamp)

I think a CGSize would make more sense as a return value.