Bug 92698 – [chromium]Upstream WebViewImpl:StartPageScaleAnimation changes for Chrome for Android

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

[chromium]Upstream WebViewImpl:StartPageScaleAnimation changes for Chrome for Android

Created attachment 155411 [details]
Patch

[Adam Barth]

Comment on attachment 155411 [details]
Patch

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.

Created attachment 155663 [details]
Patch

Created attachment 155670 [details]
Patch

[Adam Barth]

Comment on attachment 155670 [details]
Patch

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

> Source/WebKit/chromium/src/WebViewImpl.cpp:778
> +      clampedPoint = clampOffsetAtScale(targetPosition, newScale);

Four-space indent.

Created attachment 155678 [details]
Patch

Thanks a lot!

[WebKit Review Bot]

Comment on attachment 155678 [details]
Patch

Clearing flags on attachment: 155678

Committed r124288: <http://trac.webkit.org/changeset/124288>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.