Bug 66258 - ScrollAnimatorNone coasting implementation
Summary: ScrollAnimatorNone coasting implementation
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: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-15 15:20 PDT by Scott Byer
Modified: 2011-08-23 18:37 PDT (History)
6 users (show)

See Also:

Attachments
Patch (50.93 KB, patch)
2011-08-15 16:06 PDT, Scott Byer 		no flags Details | Formatted Diff | Diff
Patch (49.35 KB, patch)
2011-08-18 11:34 PDT, Scott Byer 		no flags Details | Formatted Diff | Diff
Patch (49.23 KB, patch)
2011-08-18 12:02 PDT, Scott Byer 		no flags Details | Formatted Diff | Diff
Address review comments (51.42 KB, patch)
2011-08-18 16:55 PDT, Scott Byer 		no flags Details | Formatted Diff | Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Scott Byer 2011-08-15 15:20:37 PDT 
ScrollAnimatorNone coasting implementation
Comment 1 Scott Byer 2011-08-15 16:06:41 PDT 
Created attachment 103972 [details]
Patch
Comment 2 Scott Byer 2011-08-18 11:34:17 PDT 
Created attachment 104371 [details]
Patch
Comment 3 Scott Byer 2011-08-18 11:36:49 PDT 
Rebased. James, if you have a chance, could you review this? It's slightly big, but is mostly tests. As always, feel free to point me at someone else.
Comment 4 Scott Byer 2011-08-18 12:02:11 PDT 
Created attachment 104378 [details]
Patch
Comment 5 James Robinson 2011-08-18 12:26:25 PDT 
Comment on attachment 104378 [details]
Patch

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

> Source/WebCore/platform/ScrollAnimatorNone.cpp:141
> +        double t1 = std::min(t, 1 / 2.75);

In WebKit, we prefer to put a 'using namespace std' at the top of the file and use the name unqualified.

Where are all of these magical numbers coming from?  Is it possible to give them useful symbolic names? They seem a little precise to be completely arbitrary.

> Source/WebKit/chromium/tests/ScrollAnimatorNoneTest.cpp:416
> +    for (double t = .25 ; t < 1 ; t += .25) {

nit: odd spacing here, i wouldn't expect a space before the ;
Comment 6 Scott Byer 2011-08-18 16:55:07 PDT 
Created attachment 104425 [details]
Address review comments
Comment 7 Scott Byer 2011-08-23 14:33:50 PDT 
PTAL, I've cleaned up the style issues and rebased. Thanks.
Comment 8 James Robinson 2011-08-23 17:38:43 PDT 
Comment on attachment 104425 [details]
Address review comments

Looks great, thanks for explaining some of the math in comments.
Comment 9 WebKit Review Bot 2011-08-23 18:37:03 PDT 
Comment on attachment 104425 [details]
Address review comments

Clearing flags on attachment: 104425

Committed r93682: <http://trac.webkit.org/changeset/93682>
Comment 10 WebKit Review Bot 2011-08-23 18:37:08 PDT 
All reviewed patches have been landed.  Closing bug.