[webkit-reviews] review denied: [Bug 79827] [chromium] Implement scroll physics architecture for impl/main thread [not for review yet] : [Attachment 130692] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied W. James MacLean
<wjmaclean at chromium.org>'s request for review:
Bug 79827: [chromium] Implement scroll physics architecture for impl/main
thread [not for review yet]
https://bugs.webkit.org/show_bug.cgi?id=79827

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

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


> Source/WebCore/platform/ActivePlatformGestureAnimation.h:41
> +    virtual bool animate(double ts);

please don't use abbreviations or one or two letter variable names. spell it
out

i'm assuming all the timestamps/etc in this patch are in seconds - is that
correct? if anything is in millis please type-tag the variable/parameter to
reflect that.  webkit usually uses double seconds and we're converging on that
in the compositor as well, so seconds is preferred

> Source/WebCore/platform/PlatformGestureCurve.h:35
> +    virtual bool apply(double ts, PlatformGestureCurveTarget*) = 0;

bad parameter name. expand

it's very rare to have a pure virtual class without a declare d'tor. please add
a virtual d'tor in either the public section (if it's expected that classes
will retain ownership of objects via a PlatformGestureCurve*) or in the
protected section (if classes will only retain ownership by a pointer to a more
specific type)

> Source/WebCore/platform/PlatformGestureCurve.h:38
> +}; // namespace

no ; to close a namespace

> Source/WebCore/platform/PlatformGestureCurveTarget.h:35
> +    // FIXME: add interfaces for setScroll(), setPageScaleAndScroll(), etc.

same comment as above re: d'tor

> Source/WebCore/platform/PlatformGestureCurveTarget.h:38
> +}; // namespace

no ; to close a namespace

> Source/WebCore/platform/ScrollAnimatorNone.cpp:451
> +    if (!!m_gestureAnimation)

no !!. you can just call .clear(), it's fine to do that on an OwnPtr<> that's
already cleared

>> Source/WebCore/platform/ScrollAnimatorNone.cpp:491
>> +	if (!!m_gestureAnimation) {
> 
> did you really mean "!!"? Is that normative?

OwnPtr<> has an operator bool that does the right thing, just rely on that.
this isn't javascript - we have a (somewhat) real type system and don't need to
coorce to bool with these contortions

> Source/WebCore/platform/TouchFlingPlatformGestureCurve.h:37
> +class TouchFlingPlatformGestureCurve : public PlatformGestureCurve {

since this class has virtuals declare a d'tor explicitly in the header and
define it in the .cpp

> Source/WebCore/platform/TouchFlingPlatformGestureCurve.h:41
> +    virtual bool apply(double ts, PlatformGestureCurveTarget*);

expand parameter name

> Source/WebCore/platform/TouchFlingPlatformGestureCurve.h:45
> +    TouchFlingPlatformGestureCurve(const FloatPoint& velocity);

explicit

> Source/WebCore/platform/TouchFlingPlatformGestureCurve.h:48
> +    float m_timeScaler;

i think this should be named like a noun, but it feels more like a verb. this
isn't a float that scales time, it's some sort of scale factor

> Source/WebCore/platform/WheelFlingPlatformGestureCurve.cpp:42
> +    ASSERT(std::abs(velocity.x() || std::abs(velocity.y())));

using namespace std;, etc. or even better, why not just velocity !=
FloatPoint::zero() ?

> Source/WebCore/platform/WheelFlingPlatformGestureCurve.h:41
> +    virtual bool apply(double ts, PlatformGestureCurveTarget*);

need better parameter name

> Source/WebCore/platform/WheelFlingPlatformGestureCurve.h:45
> +    // FIXME: consider making the value of sigma settable in the
constructor.
> +    WheelFlingPlatformGestureCurve(const FloatPoint& velocity);

comment should probably move as nduca suggested above, c'tor needs explicit

> Source/WebCore/platform/WheelFlingPlatformGestureCurve.h:51
> +}; // namespace

no ; on namespace close. if you're going to comment that we're closing a
namespace at least say which namespace it is

> Source/WebCore/platform/graphics/chromium/cc/CCActiveGestureAnimation.h:42
> +    virtual bool animate(double ts);

guess what?

> Source/WebCore/platform/graphics/chromium/cc/CCGestureCurve.h:37
> +    virtual void setScrollIncrement(const IntPoint &) = 0;

put the & right next to the type, no space

please declare d'tor in appropriate visibility

> Source/WebCore/platform/graphics/chromium/cc/CCGestureCurve.h:43
> +    virtual bool apply(double ts, CCGestureCurveTarget*) = 0;

same as above (param name, d'tor)

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:753
> +    if (!!m_activeGestureAnimation) {

no !!