Bug 92698 - [chromium]Upstream WebViewImpl:StartPageScaleAnimation changes for Chrome for Android
Summary: [chromium]Upstream WebViewImpl:StartPageScaleAnimation changes for Chrome for...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: yusufo
URL:
Keywords:
Depends on:
Blocks: 66687
  Show dependency treegraph
 
Reported: 2012-07-30 18:11 PDT by yusufo
Modified: 2012-07-31 19:56 PDT (History)
3 users (show)

See Also:


Attachments
Patch (2.88 KB, patch)
2012-07-30 18:12 PDT, yusufo
no flags Details | Formatted Diff | Diff
Patch (2.08 KB, patch)
2012-07-31 16:13 PDT, yusufo
no flags Details | Formatted Diff | Diff
Patch (2.08 KB, patch)
2012-07-31 16:26 PDT, yusufo
no flags Details | Formatted Diff | Diff
Patch (2.09 KB, patch)
2012-07-31 17:01 PDT, yusufo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description yusufo 2012-07-30 18:11:23 PDT
[chromium]Upstream WebViewImpl:StartPageScaleAnimation changes for Chrome for Android
Comment 1 yusufo 2012-07-30 18:12:53 PDT
Created attachment 155411 [details]
Patch
Comment 2 Adam Barth 2012-07-30 20:29:41 PDT
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.
Comment 3 yusufo 2012-07-31 16:13:27 PDT
Created attachment 155663 [details]
Patch
Comment 4 yusufo 2012-07-31 16:26:47 PDT
Created attachment 155670 [details]
Patch
Comment 5 Adam Barth 2012-07-31 16:56:59 PDT
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.
Comment 6 yusufo 2012-07-31 17:01:55 PDT
Created attachment 155678 [details]
Patch
Comment 7 yusufo 2012-07-31 17:02:13 PDT
Thanks a lot!
Comment 8 WebKit Review Bot 2012-07-31 19:56:11 PDT
Comment on attachment 155678 [details]
Patch

Clearing flags on attachment: 155678

Committed r124288: <http://trac.webkit.org/changeset/124288>
Comment 9 WebKit Review Bot 2012-07-31 19:56:15 PDT
All reviewed patches have been landed.  Closing bug.