[webkit-reviews] review denied: [Bug 92698] [chromium]Upstream WebViewImpl:StartPageScaleAnimation changes for Chrome for Android : [Attachment 155411] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied yusufo at chromium.org's request for
review:
Bug 92698: [chromium]Upstream WebViewImpl:StartPageScaleAnimation changes for
Chrome for Android
https://bugs.webkit.org/show_bug.cgi?id=92698

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=155411&action=review


Thanks for the patch.  A number of minor comments below.

> Source/WebKit/chromium/ChangeLog:8
> +	   Additional information of the change such as approach, rationale.
Please add per-function descriptions below (OOPS!).

Please replace this line of the ChangeLog with a description of why you;re
making this change.

> Source/WebKit/chromium/src/WebViewImpl.cpp:771
> -void WebViewImpl::startPageScaleAnimation(const IntPoint& scroll, bool
useAnchor, float newScale, double durationInSeconds)
> +void WebViewImpl::startPageScaleAnimation(const WebPoint& targetPosition,
bool useAnchor, float newScale, double durationSec)

Typically we use WebCore types (e.g., IntPoint) in the implementation and
reserve API types (e.g., WebPoint) for the API.

We shouldn't rename durationInSeconds to durationSec.  WebKit prefers variable
names that use complete works and (ideally) English grammar.

> Source/WebKit/chromium/src/WebViewImpl.cpp:781
> +	 setPageScaleFactor(newScale, clampedPoint);

Please use four-space indent.