[webkit-reviews] review denied: [Bug 79827] [chromium] Implement scroll physics architecture for impl/main thread : [Attachment 130838] 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
https://bugs.webkit.org/show_bug.cgi?id=79827

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

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


Seems pretty good. I think it's worth taking another pass to tighten up
interface, etc, then we're good to land.

> Source/WebCore/platform/ActivePlatformGestureAnimation.h:42
> +class ActivePlatformGestureAnimation {
> +public:

WTF_MAKE_NONCOPYABLE please

> Source/WebCore/platform/ActivePlatformGestureAnimation.h:43
> +    static PassOwnPtr<ActivePlatformGestureAnimation> create(double
startTime, PassOwnPtr<PlatformGestureCurve>, PlatformGestureCurveTarget*);

can you document the requirements of the target here? looks like the target has
to outlive the animation, right?

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

why is this virtual?

> Source/WebCore/platform/PlatformGestureCurve.h:38
> +    virtual ~PlatformGestureCurve() { }

why is this d'tor public (as opposed to protected)? it's important to think
carefully about this distinction for all of your interface classes - do you
expect that users who see only this interface should be able to take ownership
of the object (and delete it) or should owners have access to a pointer of a
more specific type?

> Source/WebCore/platform/PlatformGestureCurveTarget.h:34
> +    virtual ~PlatformGestureCurveTarget() { }

same comment as above re: visibility of d'tor

> Source/WebCore/platform/TouchFlingPlatformGestureCurve.cpp:43
> +    , m_timeScaleFactor(1000 / max(1.f, max(abs(velocity.x()),
abs(velocity.y()))))

if this is supposed to take a floating point velocity, did you mean to use
fabs()?

> Source/WebCore/platform/TouchFlingPlatformGestureCurve.cpp:45
> +    ASSERT(abs(velocity.x()) || abs(velocity.y()));

the "abs()" calls here are a bit surprising. they'll coerce to int. are you
trying to ASSERT that velocity is not too close to zero or that it's not
actually zero? i think either way this assertion could be written in a much
clearer fashion.

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

again - why virtual? if you expect subclasses then the d'tor should be declared
virtual

> Source/WebCore/platform/graphics/chromium/cc/CCGestureCurve.h:29
> +#include <wtf/Noncopyable.h>
> +#include <wtf/PassOwnPtr.h>

you don't appear to need either of these #includes in this header

> Source/WebCore/platform/graphics/chromium/cc/CCGestureCurve.h:37
> +    virtual ~CCGestureCurveTarget() { }

feels like it should be protected, is that right?

> Source/WebCore/platform/graphics/chromium/cc/CCGestureCurve.h:45
> +    virtual ~CCGestureCurve() { }

protected?

> Source/WebKit/chromium/tests/PlatformGestureCurveTest.cpp:2
> + * Copyright (C) 2011 Google Inc. All rights reserved.

2012