Bug 80607 – [chromium] Wire up PlatformGestureCurves for CC, ScrollAnimatorNone.

• Component: New Bugs
• Status: RESOLVED FIXED

[W. James MacLean]

[chromium] Wire up PlatformGestureCurves for CC, ScrollAnimatorNone.

[W. James MacLean]

Created attachment 130840 [details]
Patch

[W. James MacLean]

This will fail on the bots, as it depends on 79827.

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Gustavo Noronha (kov)]

Comment on attachment 130840 [details]
Patch

Attachment 130840 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11865351

[WebKit Review Bot]

Comment on attachment 130840 [details]
Patch

Attachment 130840 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11861379

[W. James MacLean]

Created attachment 130876 [details]
Patch

[W. James MacLean]

Revised patch to remove icky cast.

[Nat Duca]

Comment on attachment 130876 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130876&action=review

> Source/WebCore/platform/ScrollAnimatorNone.cpp:517
> +// Implementation of PlatformGestureCurveTarget interface functions.

dont need this comment

> Source/WebCore/platform/ScrollAnimatorNone.h:91
> +    // Declaration of PlatformGestureCurveTarget interface function.

I think we typically say something like
// PlatformGestureCurveTarget implementation.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:747
> +        m_client->setNeedsRedrawOnImplThread();

I think you need this conditional on isContinuing?

isContinuning -> animationStillRunning?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:30
> +#include "cc/CCGestureCurve.h"

can we get away with predeclaring? I dont see a use of the curve here... but Im probably blind.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:77
> +    virtual void clearActiveGestureAnimation();

Can we just make setActiveGestureANimation(nullptr) work as you'd expect it? If so, put comments on the CCInputHandlerClient class explaining to pass nullptr to clear.

> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:30
> +#include "PlatformGestureCurve.h"

needed?

> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:32
> +#include "TouchFlingPlatformGestureCurve.h"

Dont the TouchFlingPlatformGestureCurve subclasses bring in the platformgesturecurve base class?

> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:39
> +#include "cc/CCLayerTreeHostImpl.h"

Shouldn't be including this... what's causing us to include this? Lets kill it with fire.

> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:42
> +#include <wtf/CurrentTime.h>

whats causing us to bring this in? ideally, we dont.

> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:58
> +class CCToPlatformGestureCurveTargetAdapter : public PlatformGestureCurveTarget {

Since this is instantiated just inside the apply function of the curve adapter, then it should just be an inner class of that class.

> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:63
> +    virtual void setScrollIncrement(const IntPoint&);

You can implement inline when you're in cpp files. For simple stuff like this, for certain.

> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:103
> +    CCToPlatformGestureCurveTargetAdapter adapter(target);

This should just be an inner class called TargetAdapter

> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:107
> +class GestureCurveTarget : public CCGestureCurveTarget {

instead of having this, we can make WebCompositorInputHandlerImpl implement CCGestureCurveTarget? Then you'd just pass "this" into the CCActiveGestureAnimation::create function's second argument.

> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:272
> +                                                                    PlatformGestureCurveToCCGestureCurveAdapter::create(flingCurve.release()),

indent is funky here. one more temporary variable maybe?

> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:273
> +                                                                    m_gestureCurveTarget.get()));

Indewhy not just make a class

GestureCurveTarget : CCGestureCurve, PlatformGestureCCurve

[W. James MacLean]

Comment on attachment 130876 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130876&action=review

>> Source/WebCore/platform/ScrollAnimatorNone.cpp:517
>> +// Implementation of PlatformGestureCurveTarget interface functions.
> 
> dont need this comment

Removed.

>> Source/WebCore/platform/ScrollAnimatorNone.h:91
>> +    // Declaration of PlatformGestureCurveTarget interface function.
> 
> I think we typically say something like
> // PlatformGestureCurveTarget implementation.

Fixed.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:747
>> +        m_client->setNeedsRedrawOnImplThread();
> 
> I think you need this conditional on isContinuing?
> 
> isContinuning -> animationStillRunning?

I wasn't clear on this ... fixed.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:30
>> +#include "cc/CCGestureCurve.h"
> 
> can we get away with predeclaring? I dont see a use of the curve here... but Im probably blind.

We can just remove it ... it's a remnant from an earlier iteration.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:77
>> +    virtual void clearActiveGestureAnimation();
> 
> Can we just make setActiveGestureANimation(nullptr) work as you'd expect it? If so, put comments on the CCInputHandlerClient class explaining to pass nullptr to clear.

Hmm, not sure how that works with PassOwnPtr<> but will investigate.

>> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:30
>> +#include "PlatformGestureCurve.h"
> 
> needed?

Nope, based on your reasoning below. Removed.

>> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:32
>> +#include "TouchFlingPlatformGestureCurve.h"
> 
> Dont the TouchFlingPlatformGestureCurve subclasses bring in the platformgesturecurve base class?

Yes.

>> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:39
>> +#include "cc/CCLayerTreeHostImpl.h"
> 
> Shouldn't be including this... what's causing us to include this? Lets kill it with fire.

Ooops, remnant from the icky cast we got rid of (this *was* necessary for that, part of why it was icky).

Removed.

>> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:42
>> +#include <wtf/CurrentTime.h>
> 
> whats causing us to bring this in? ideally, we dont.

We need to set a start time for the animation, and therefore need monotonicallyIncreasingTime. I'm not sure how we can avoid this (save maybe have *all* gesture animation types set this internally in their constructors, but that seems limiting).

>> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:58
>> +class CCToPlatformGestureCurveTargetAdapter : public PlatformGestureCurveTarget {
> 
> Since this is instantiated just inside the apply function of the curve adapter, then it should just be an inner class of that class.

Done.

I assume it's OK to drop the WTF_MAKE_NONCOPYABLE since no-one outside the class can make one anyways?

>> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:63
>> +    virtual void setScrollIncrement(const IntPoint&);
> 
> You can implement inline when you're in cpp files. For simple stuff like this, for certain.

Done.

>> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:103
>> +    CCToPlatformGestureCurveTargetAdapter adapter(target);
> 
> This should just be an inner class called TargetAdapter

Done.

>> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:107
>> +class GestureCurveTarget : public CCGestureCurveTarget {
> 
> instead of having this, we can make WebCompositorInputHandlerImpl implement CCGestureCurveTarget? Then you'd just pass "this" into the CCActiveGestureAnimation::create function's second argument.

OK, that works - done.

>> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:272
>> +                                                                    PlatformGestureCurveToCCGestureCurveAdapter::create(flingCurve.release()),
> 
> indent is funky here. one more temporary variable maybe?

Done.

>> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:273
>> +                                                                    m_gestureCurveTarget.get()));
> 
> Indewhy not just make a class
> 
> GestureCurveTarget : CCGestureCurve, PlatformGestureCCurve

Not quite sure what you have in mind here. It's late, so I'll submit this patch shortly and we'll pick up on the next iteration.

[W. James MacLean]

Created attachment 130978 [details]
Patch

Sorry about the lack of Changelog entries, but it's late and I can't get "webkit-patch upload -g" to work for me.

This is not the final version I suspect, so we'll get the changelogs in the next round.

As before, the bots won't be able to process this until 79827 lands.

[Nat Duca]

Comment on attachment 130978 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130978&action=review

Ima happy. LGTM from my PoV. But, I'd wait for jamesr feedback before applying my lingering ints. He might disagree on some of the style points, and he's TeH Boss around these parts. :)

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

can probably rewrite this
  if(!m_active) return

then unindent the remainder

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:75
> +    // To clear an active animation, pass nullptr.

I'd put this comment on the client .h instead of on the implementation

> Source/WebKit/chromium/public/WebInputEvent.h:200
> +            || type == GestureTapDown // Include GestureTapDown, as it is used to cancel in-flight scroll animations.

This comment baffles me but maybe its because I dont have context?

> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:65
> +        : m_target(target)

indentation

> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:82
> +PassOwnPtr<CCGestureCurve> PlatformGestureCurveToCCGestureCurveAdapter::create(PassOwnPtr<PlatformGestureCurve> platformCurve)

put these inline

> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:87
> +PlatformGestureCurveToCCGestureCurveAdapter::PlatformGestureCurveToCCGestureCurveAdapter(PassOwnPtr<PlatformGestureCurve> curve)

inline

> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:92
> +bool PlatformGestureCurveToCCGestureCurveAdapter::apply(double time, CCGestureCurveTarget* target)

inline

> Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:320
> +    ASSERT_TRUE(mockInputHandler.activeGestureAnimation());

assert or expect, here?

[W. James MacLean]

Comment on attachment 130978 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130978&action=review

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:741
>> +    if (m_activeGestureAnimation) {
> 
> can probably rewrite this
>   if(!m_active) return
> 
> then unindent the remainder

Done.

>> Source/WebKit/chromium/public/WebInputEvent.h:200
>> +            || type == GestureTapDown // Include GestureTapDown, as it is used to cancel in-flight scroll animations.
> 
> This comment baffles me but maybe its because I dont have context?

Removed.

It was motivated by the comment on the line below, as it was obviously puzzling to someone as to why GestureTap was on the list of ScrollGestureEvents ... since GestureTapDown will cancel in-flight gesture animations, I though I should forestall anyone being curious about why it was on the list.

Howver, if the comment out-of-context is confusing, better to remove it.

>> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:65
>> +        : m_target(target)
> 
> indentation

Fixed.

>> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:82
>> +PassOwnPtr<CCGestureCurve> PlatformGestureCurveToCCGestureCurveAdapter::create(PassOwnPtr<PlatformGestureCurve> platformCurve)
> 
> put these inline

Done.

>> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:87
>> +PlatformGestureCurveToCCGestureCurveAdapter::PlatformGestureCurveToCCGestureCurveAdapter(PassOwnPtr<PlatformGestureCurve> curve)
> 
> inline

Done.

>> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:92
>> +bool PlatformGestureCurveToCCGestureCurveAdapter::apply(double time, CCGestureCurveTarget* target)
> 
> inline

Done.

>> Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:320
>> +    ASSERT_TRUE(mockInputHandler.activeGestureAnimation());
> 
> assert or expect, here?

So far as I know it should be assert, as I want the test to abort if mockInputHandler.activeGestureAnimation() returns null.

[W. James MacLean]

Created attachment 131113 [details]
Patch

Addresses Nat's comments.

[James Robinson]

Comment on attachment 131113 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=131113&action=review

This needs ChangeLogs, as I'm sure you know.

> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:221
> +            // GestureScrollEnd with non-zero velocity (deltaX/Y) signals start of Fling.

this is getting to be way too much logic in handleInputEvent - please split this out into its own function

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

i'm not sure this is really the right model - do we want to grab a timestamp when we handle the event, or put the animation in a "waiting for start" state and let the animation subsystem decide when the animation starts? this will change whether or not you 'miss' the start of the curve if the compositor doesn't tick for a short period of time after the input is handled.

> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:296
> +void WebCompositorInputHandlerImpl::setScrollIncrement(const IntPoint& increment)
> +{
> +    if (m_inputHandlerClient)
> +        m_inputHandlerClient->scrollBy(IntSize(increment.x(), increment.y()));
> +}

now that i see the implementation this function name really confuses me. I thought that setScrollIncrement sets a value that changes the granularity of scrolls, but it looks like it just scrolls you.  What's the meaning of the "incremeent" in the name?

[James Robinson]

+cc Ian - can you let us know if this is starting the gesture animation in the right way?

(In reply to comment #15)
> +cc Ian - can you let us know if this is starting the gesture animation in the right way?

I do think there is a chance of a slight hiccup if there's a pause between the scheduling of the animation and the first tick. I like your idea of putting the animation in a waiting-to-start state and then setting the start time to be the time of the first tick.

[Nat Duca]

(In reply to comment #16)
> (In reply to comment #15)
> > +cc Ian - can you let us know if this is starting the gesture animation in the right way?
> 
> I do think there is a chance of a slight hiccup if there's a pause between the scheduling of the animation and the first tick. I like your idea of putting the animation in a waiting-to-start state and then setting the start time to be the time of the first tick.


I dont think this is particularly likely. We're on the impl thread remember? There's so much noise in the input handling system getting to this point that by comparison, the impl thread runs like clockwork. Besdies which, if we had a delay on the impl thread, you'd want the exact opposite: lests say you do a fling and then the thread gets descheduled for 500ms. You want the fling to be mostly over by the time the thread comes back alive, not for the fling to start at that point!

I really think that the existing system --- creating a gesture animation in a started state with the input event time is sufficient and correct.

[W. James MacLean]

Comment on attachment 131113 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=131113&action=review

>> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:221
>> +            // GestureScrollEnd with non-zero velocity (deltaX/Y) signals start of Fling.
> 
> this is getting to be way too much logic in handleInputEvent - please split this out into its own function

Will do.

>> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:233
>> +                            CCActiveGestureAnimation::create(monotonicallyIncreasingTime(), PlatformGestureCurveToCCGestureCurveAdapter::create(flingCurve.release()), this);
> 
> i'm not sure this is really the right model - do we want to grab a timestamp when we handle the event, or put the animation in a "waiting for start" state and let the animation subsystem decide when the animation starts? this will change whether or not you 'miss' the start of the curve if the compositor doesn't tick for a short period of time after the input is handled.

I can see merit for this for some gestures, e.g. GestureDoubleTap.

But, for something like GestureFling, which is really a continuation of GestureScroll, I'm concerned pushing the start time back to the start of the next tick might cause a hiccup in the transition from GestureScroll to GestureFling. Rob, can you weigh in on this?

Perhaps provide a means of using both mechanisms? E.g. a start-time of 0 means wait for the next tick, but a non-zero start time works as per the current implementation.

>> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:296
>> +}
> 
> now that i see the implementation this function name really confuses me. I thought that setScrollIncrement sets a value that changes the granularity of scrolls, but it looks like it just scrolls you.  What's the meaning of the "incremeent" in the name?

It's scrolling by a displacement, not scrolling to an absolute position. I'll rename it to setScrollDelta()? Or would that be confusing with how the same name is used in CCLayerImpl.

[James Robinson]

I think you just want scroll() then (or scrollBy() perhaps). You aren't setting a field, so don't name it like a setter.  It should be named something verb-y.

[W. James MacLean]

(In reply to comment #19)
> I think you just want scroll() then (or scrollBy() perhaps). You aren't setting a field, so don't name it like a setter.  It should be named something verb-y.

Sure, that sounds good - thanks.

[Robert Kroeger]

Comment on attachment 131113 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=131113&action=review

>>> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:233
>>> +                            CCActiveGestureAnimation::create(monotonicallyIncreasingTime(), PlatformGestureCurveToCCGestureCurveAdapter::create(flingCurve.release()), this);
>> 
>> i'm not sure this is really the right model - do we want to grab a timestamp when we handle the event, or put the animation in a "waiting for start" state and let the animation subsystem decide when the animation starts? this will change whether or not you 'miss' the start of the curve if the compositor doesn't tick for a short period of time after the input is handled.
> 
> I can see merit for this for some gestures, e.g. GestureDoubleTap.
> 
> But, for something like GestureFling, which is really a continuation of GestureScroll, I'm concerned pushing the start time back to the start of the next tick might cause a hiccup in the transition from GestureScroll to GestureFling. Rob, can you weigh in on this?
> 
> Perhaps provide a means of using both mechanisms? E.g. a start-time of 0 means wait for the next tick, but a non-zero start time works as per the current implementation.

I'm mostly in agreement with jamesr -- assuming that my understanding is correct that we redraw on (roughly) monotonic ticks that should be aligning with the vsync.

If so, then deferring the animation to start at the next tick after receipt of the ScrollEnd event ought to proceed something like this:

tick_i:  receive event ScrollUpdate j
tick_i+1: draw page with displacements for ScrollUpdate j, receive event ScrollUpdate j+1
tick_i+2: SwapBuffers, show tick_i+1's frame, draw page with displacement ScrollUpdate j+1, receive ScrollEnd j+2, post request to start animation on next tick.
tick_i+3: SwapBuffers to show tick_i+2's frame, draw page displaced by velocity * (time(tick_i+4) - time(ScrollEnd)).  (1 tick length + time from ScrollEnd to next tick,)
tick_i+4: Swap, to i+3, draw page displaced by velocity * tick length.

and would therefore be find.

A much larger disconnect is probably a user's sense that the finger was still be accelerating the surface after the finger is no longer in contact.

[W. James MacLean]

Created attachment 131198 [details]
Patch

[W. James MacLean]

(In reply to comment #22)
> Created an attachment (id=131198) [details]
> Patch

Covers all suggestions, converts to "start on next tick" animation model.

[W. James MacLean]

Comment on attachment 131198 [details]
Patch

Ugh, needs rebase.

[W. James MacLean]

Created attachment 131199 [details]
Patch

[Nat Duca]

Comment on attachment 131199 [details]
Patch

In an ideal world, this would be 4 patches [add input event enum, change animation start model, hook curves into scroll animator none, hook curves into wcih].

I appreciate that we are under immense time and schedule pressure.

This LGTM. James, this is ready for your review. If the patch is too large, let me know and I'll be happy to break it up into smaller patches for easier consumption.

[James Robinson]

Comment on attachment 131199 [details]
Patch

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::create(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

[W. James MacLean]

Comment on attachment 131199 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=131199&action=review

>> 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

Removed.

>> 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

D'oh - you're absolutely right - not sure what was crawling through my head when I did this.

Fixed.

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

Removed.

>> 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

Ditto above.

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

That's bizarre - even my clumsy typing can't account for that!

Removed.

>> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:50
>> +using namespace WebCore;
> 
> this code is in namespace WebCore, no need for a using declaration

Removed.

>> 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?

OK. I've renamed to reflect the fact the adapter is now doing double-duty.

>> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:249
>> +        return;
> 
> are you sure we want to eat the tap even if they didn't have an active scroll gesture?

Hmm, probably not, but perhaps Rob K can confirm?

>> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:259
>> +    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

OK. I put it here trying to keep things simpler in the caller (and the .h file), but I'm happy enough to move it back.

>> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:271
>> +            m_inputHandlerClient->setActiveGestureAnimation(nullptr);
> 
> this seems unnecessary - we're about to overwrite the animation

Removed.

>> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:275
>> +                CCActiveGestureAnimation::create(PlatformGestureCurveToCCGestureCurveAdapter::create(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

Unwrapped.

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

Done.

>> 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

No longer necessary, so removed.

>> Source/WebKit/chromium/tests/PlatformGestureCurveTest.cpp:65
>> +    double startTime = monotonicallyIncreasingTime();
> 
> i think you need to mock this clock out

No longer an issue.

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

Ditto.

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

Ditto.

>> Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:73
>> +    virtual void clearActiveGestureAnimation() { m_activeGestureAnimation.clear(); }
> 
> there's no caller to this - remove?

Yup ... originally included to allow CCInputHandler to be instantiated, but now obsolete.

>> Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:299
>> +#endif
> 
> the internals of DebugScopedSetImplThread are #ifndef NDEBUG guarded, you don't need to guard them yourself

I borrowed this from the gesturePinch test - have removed.

>> 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

Done.

[W. James MacLean]

Created attachment 131943 [details]
Patch

[James Robinson]

Comment on attachment 131943 [details]
Patch

Looks good, thanks for all the fixes.

[W. James MacLean]

(In reply to comment #30)
> (From update of attachment 131943 [details])
> Looks good, thanks for all the fixes.

NP, thanks for all the good ideas!

[W. James MacLean]

Committed r110898: <http://trac.webkit.org/changeset/110898>