Summary: | [chromium]Upstream WebViewImpl:StartPageScaleAnimation changes for Chrome for Android | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | yusufo | ||||||||||
Component: | New Bugs | Assignee: | yusufo | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, aelias, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 66687 | ||||||||||||
Attachments: |
|
Description
yusufo
2012-07-30 18:11:23 PDT
Created attachment 155411 [details]
Patch
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
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! Comment on attachment 155678 [details] Patch Clearing flags on attachment: 155678 Committed r124288: <http://trac.webkit.org/changeset/124288> All reviewed patches have been landed. Closing bug. |