[Webkit-unassigned] [Bug 61878] Smooth scrolling for Chromium

Fri Jun 3 17:01:12 PDT 2011

https://bugs.webkit.org/show_bug.cgi?id=61878

--- Comment #25 from Adam Barth <abarth at webkit.org>  2011-06-03 17:01:11 PST ---
(From update of attachment 95985)
View in context: https://bugs.webkit.org/attachment.cgi?id=95985&action=review

(Again, I don't have any understanding of the actual content of this patch, the below are mostly just style nits.)

Three high-level points:

1) This code appears to be untested.  As a general rule, the project doesn't accept code without tests.  Sometimes it's difficult to have a testing plan in the initial stages of bringing up a feature.  You discuss this some in your ChangeLog.  Is there a plan for automated testing in the future?  How can folks work on this code without immediately breaking it?

2) None of this patch seems to be specific to Chromium.  Instead, this looks like a generic implementation that anyone could use if they're running on a platform without a platform-native implementation.  Rather than siloing this code away in a directory named "chromium," it's probably better to put it in the main platform directory.  In the past, we've used suffixes like "None" to indicate that a particular implementation is a generic version that you can use if your platform or library suite doesn't include this sort of functionality.

3) A bunch of the code around the settings is still really strange.  I don't fully understand what's going on there, but it doesn't look the way any other code in the project looks, which leads me to believe something isn't quite right there.  Rather than continuing to post on this bug, it might be helpful to find me or someone else on #webkit and we can try to understand what you're trying to accomplish there.

> Source/WebCore/platform/chromium/ScrollAnimatorChromium.cpp:54
> +    ScrollAnimatorSettings::Settings initial =
> +            { false, 0.25, Linear, 0.01, Linear, 0.01, false, false, Linear, 0, 0 };

Pls merge these lines.  There's no line length limit in WebKit.

> Source/WebCore/platform/chromium/ScrollAnimatorChromium.cpp:59
> +    ScrollAnimatorSettings::Settings keyboardLine =
> +            { true, 7 * kTickTime, Quadratic, 3 * kTickTime, Quadratic, 3 * kTickTime, false, false, Quadratic, 0, 0 };

Same for all these.

> Source/WebCore/platform/chromium/ScrollAnimatorChromium.cpp:121
> +double ScrollAnimatorChromium::PerAxisData::attackCurve(ScrollAnimatorSettings::Curve curve, double deltaT,
> +                                                        double curveT, double startPos, double attackPos)

These can all be one line too.

> Source/WebCore/platform/chromium/ScrollAnimatorChromium.cpp:137
> +    default:
> +        notImplemented();
> +        break;

You should list all the enum values explicitly.  That way the compiler will complain when this code evolves.

> Source/WebCore/platform/chromium/ScrollAnimatorChromium.cpp:218
> +                                                                 const ScrollAnimatorSettings::Settings* settings)

the const in "const ScrollAnimatorSettings::Settings*" is kind of meaningless.  That means the value of the pointer can't change, not that the object pointed to can't change.  I'd just drop he const.

> Source/WebCore/platform/chromium/ScrollAnimatorChromium.cpp:318
> +        double currentAttackPos = m_desiredVelocity ? attackCurve(m_attackCurve, deltaT, m_attackTime,
> +                                                              m_startPos, m_attackPos) : m_startPos;

I'd merge these lines.  They're already pretty tricky to read.

> Source/WebCore/platform/chromium/ScrollAnimatorChromium.cpp:321
> +        double newAttackPos = m_desiredVelocity ? attackCurve(m_attackCurve, deltaT, m_attackTime,
> +                                                          m_startPos, m_attackPos) : m_startPos;

ditto

> Source/WebCore/platform/chromium/ScrollAnimatorChromium.cpp:346
> +        // attack is based on adding velocity

I'd remove this comment.

> Source/WebCore/platform/chromium/ScrollAnimatorChromium.cpp:348
> +        double newPos = attackCurve(m_attackCurve, deltaT, m_attackTime,
> +                                    m_startPos, m_attackPos) + m_attackOffset;

merge

newPos => newPosition (WebKit doesn't like to abbreviate works in variable names.)  I guess really all instances of "pos" should be "position" in this class.

> Source/WebCore/platform/chromium/ScrollAnimatorChromium.cpp:394
> +    // TODO(scottbyer): get the type passed in. MouseWheel could also be by line, but should still have different

TODO(scottbyer) => FIXME
(This is just a stylistic difference between Chromium and WebKit.)

> Source/WebCore/platform/chromium/ScrollAnimatorChromium.cpp:396
> +    static ScrollAnimatorSettings settings;

If you need a non-trivial static, you need to use DECLARE_STATIC_LOCAL.  However, having this static here is very strange.

> Source/WebCore/platform/chromium/ScrollAnimatorChromium.h:64
> +    // Can be overspecified. animationTime takes priority over releaseTime takes priority over attackTime.
> +    // Missing: how to specify animation time accumulation (e.g., pure additive, asymptotic to a limit with a curve?)

"Can be overspecified." => Please use complete sentences in comments.  Should "Missing" be "FIXME" ?

> Source/WebCore/platform/chromium/ScrollAnimatorChromium.h:84
> +    Settings settings[5];

5 => No magic numbers please.  Presumably this is LastEntry ?

> Source/WebCore/platform/chromium/ScrollAnimatorChromium.h:86
> +    // Allow for per-platform initialization.

Please remove this comment.

> Source/WebCore/platform/chromium/ScrollAnimatorChromium.h:91
> +  public:

No indent for these qualifiers.

> Source/WebCore/platform/chromium/ScrollAnimatorChromium.h:99
> +    friend class ::ScrollAnimatorChromiumTest;

This class does not appear to exist.  Please remove.

> Source/WebCore/platform/chromium/ScrollAnimatorChromium.h:113
> +        static double attackCurve(ScrollAnimatorSettings::Curve, double deltaT, double curveT,
> +                                  double startPos, double attackPos);
> +        static double releaseCurve(ScrollAnimatorSettings::Curve, double deltaT, double curveT,
> +                                   double releasePos, double desiredPos);

Please merge these lines.

> Source/WebCore/platform/chromium/ScrollAnimatorChromium.h:149
> +    static const double animationTimerDelay;

Why static?  This is strange.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.