[webkit-reviews] review denied: [Bug 73489] [chromium] Add fling animation support to ScrollAnimatorNone : [Attachment 118272] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied vollick at chromium.org's request
for review:
Bug 73489: [chromium] Add fling animation support to ScrollAnimatorNone
https://bugs.webkit.org/show_bug.cgi?id=73489

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

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=118272&action=review


handleGestureEvent() is dead and has been for some time:
http://trac.webkit.org/changeset/107823, so this patch needs some updating.

Are you sure you want to put the fling animation curves right into
ScrollAnimatorNone?  On android, for instance, we'll be pulling the fling
animation curves by calling into a system library, so it might be nice if the
fling animation curves were easily extractable.

>>> Source/WebCore/platform/ScrollAnimator.cpp:133
>>> +void ScrollAnimator::handleGestureEvent(const PlatformGestureEvent& pge)
>> 
>> i've been dinged on reviews with use of abbreviations like this. I'd be good
with it staying this way but the reviewer might still be sad.
> 
> Hopefully it's ok -- I was following the lead of other code in the file!

No, it's definitely not OK.  We pretty much never use abbreviations or acronyms
in WebKit. http://www.webkit.org/coding/coding-style.html#names-full-words

> Source/WebCore/platform/ScrollAnimator.cpp:139
> +	   float deltaX = (pge.deltaX() < 0.f) ? max(pge.deltaX(), 
-float(maxForwardScrollDelta.width())) : min(pge.deltaX(),
float(maxBackwardScrollDelta.width()));

don't append ".f" -
http://www.webkit.org/coding/coding-style.html#float-suffixes

> Source/WebCore/platform/ScrollAnimatorNone.cpp:59
> +const double kDefaultFlingAcceleration = -2500.0; // pixels/second^2

the other constants are misnamed here too, but FYI we don't typically use the k
prefix for constants, we just name them like normal variables.

> Source/WebCore/platform/ScrollAnimatorNone.cpp:61
> +static double getFlingPosition(double initialPosition, double
initialVelocity, double acceleration, double t)

we very rarely use one-letter variable or parameter names. i think this would
be better named "time"

> Source/WebCore/platform/ScrollAnimatorNone.cpp:63
> +    double maxT = -initialVelocity / acceleration;

again, maxTime would be a better name - although it's not incredibly clear why
this is a cap here. a comment might help

> Source/WebCore/platform/ScrollAnimatorNone.cpp:66
> +    return initialPosition + t * (initialVelocity + 0.5f * acceleration *
t);

no trailing "f"

> Source/WebCore/platform/ScrollAnimatorNone.cpp:346
> +    m_startTime = m_lastAnimationTime = WTF::monotonicallyIncreasingTime();

don't put the "WTF::" here. <wtf/CurrentTime.h> puts this function into the
global namespace with a using statement. this is the general pattern WTF uses,
it's very rare to see an explicit WTF:: unless there's an ambiguity

> Source/WebCore/platform/ScrollAnimatorNone.cpp:528
> +void ScrollAnimatorNone::fling(const PlatformGestureEvent& pge)

pge needs to be renamed

> Source/WebKit/chromium/tests/ScrollAnimatorNoneTest.cpp:994
> +    double const maxT = 10.0; // a scroll better not take longer than 10
seconds.

no trailing ".0"

this variable name isn't ideal

> Source/WebKit/chromium/tests/ScrollAnimatorNoneTest.cpp:997
> +    double t;

don't use one-letter variable names