[webkit-reviews] review denied: [Bug 80607] [chromium] Wire up PlatformGestureCurves for CC, ScrollAnimatorNone. : [Attachment 131199] 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 80607: [chromium] Wire up PlatformGestureCurves for CC, ScrollAnimatorNone.
https://bugs.webkit.org/show_bug.cgi?id=80607

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

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


Getting there, down to mostly small issues.

> Source/WebCore/platform/ActivePlatformGestureAnimation.cpp:32
> +#include <wtf/CurrentTime.h>

don't need. in general code in the animation system outside the controller
shouldn't need to lock at any clocks, it should just used the controller's
clock for everything

> Source/WebCore/platform/ActivePlatformGestureAnimation.cpp:56
> +	   m_startTime = monotonicallyIncreasingTime();

you should use the passed-in time and not look at the clock again. this is
wasteful and means that you might start off at some point inside the curve
instead of at the very start

> Source/WebCore/platform/graphics/chromium/cc/CCActiveGestureAnimation.cpp:30
> +#include <wtf/CurrentTime.h>

do not need

> Source/WebCore/platform/graphics/chromium/cc/CCActiveGestureAnimation.cpp:54
> +	   m_startTime = monotonicallyIncreasingTime();

this should use the pass-in time, it shouldn't look at the clock again

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:76
> +    virtual CCActiveGestureAnimation* activeGestureAnimation() { return
m_activeGestureAnimation.get(	 ); }

why all the spaces in get(     ) ?

> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:50
> +using namespace WebCore;

this code is in namespace WebCore, no need for a using declaration

> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:71
> +    class TargetAdapter : public PlatformGestureCurveTarget {

why do we need two layers of adapter here? why can't the
PlatformGestureCurveToCCGestureCurveAdapter simply implement
PlatformGestureCurveTarget and do the right thing in the scrollBy
implementation?

> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:249
> +	   m_client->didHandleInputEvent();
> +	   return;

are you sure we want to eat the tap even if they didn't have an active scroll
gesture?

> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:259
> +    ASSERT(event.type == WebInputEvent::GestureScrollEnd);
> +
> +    const WebGestureEvent& gestureEvent = *static_cast<const
WebGestureEvent*>(&event);

i think it would be better to do the type conversion in the caller since that's
inside a switch on type already

> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:271
> +	   if (m_inputHandlerClient->activeGestureAnimation())
> +	       m_inputHandlerClient->setActiveGestureAnimation(nullptr);

this seems unnecessary - we're about to overwrite the animation

> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:275
> +		  
CCActiveGestureAnimation::create(PlatformGestureCurveToCCGestureCurveAdapter::c
reate(flingCurve.release()), this);

i personally wouldn't line wrap this. if you do want to wrap it, though, it
should have a 4-space indentation

> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:304
> +	   m_inputHandlerClient->scrollBy(IntSize(increment.x(),
increment.y()));

increment.toSize()

> Source/WebKit/chromium/tests/PlatformGestureCurveTest.cpp:37
> +#include <wtf/CurrentTime.h>

it's weird for a test to depend on an actual wall clock time - if there is a
bug exposed by the value, you'll have a hell of a time trying to reproduce it.
much better to use a mock clock

> Source/WebKit/chromium/tests/PlatformGestureCurveTest.cpp:65
> +    double startTime = monotonicallyIncreasingTime();

i think you need to mock this clock out

> Source/WebKit/chromium/tests/PlatformGestureCurveTest.cpp:80
> +    startTime = monotonicallyIncreasingTime();

same here

> Source/WebKit/chromium/tests/PlatformGestureCurveTest.cpp:98
> +    double startTime = monotonicallyIncreasingTime();

and here

> Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:73
> +    virtual void clearActiveGestureAnimation() {
m_activeGestureAnimation.clear(); }

there's no caller to this - remove?

> Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:299
> +#ifndef NDEBUG
> +    // WebCompositorInputHandler APIs can only be called from the compositor
thread.
> +    WebCore::DebugScopedSetImplThread alwaysImplThread;
> +#endif

the internals of DebugScopedSetImplThread are #ifndef NDEBUG guarded, you don't
need to guard them yourself

> Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:326
> +    // Verify that a GestureTapDown during an animation cancels it.

you should also test the behavior of a tap with no active scroll