[webkit-reviews] review granted: [Bug 80311] Handle more Gesture* events by performing scrolls on the correct target ScrollableArea : [Attachment 131095] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has granted Robert Kroeger
<rjkroege at chromium.org>'s request for review:
Bug 80311: Handle more Gesture* events by performing scrolls on the correct
target ScrollableArea
https://bugs.webkit.org/show_bug.cgi?id=80311

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

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


I think this'll work fine for now. Left a number of nits to deal with.

> Source/WebCore/page/EventHandler.cpp:2370
> +    TemporaryChange<PlatformEvent::Type> baseEventType(m_baseEventType,
gestureEvent.type());

can you put a FIXME here linked to the ::scroll() bug, if that is what'll
resolve this?

> Source/WebCore/page/EventHandler.cpp:2417
> +    PlatformWheelEvent syntheticWheelEvent(point, globalPoint,
gestureEvent.deltaX(), gestureEvent.deltaY(), gestureEvent.deltaX() /
tickDivisor, gestureEvent.deltaY() / tickDivisor, granularity,
gestureEvent.shiftKey(), gestureEvent.ctrlKey(), gestureEvent.altKey(),
gestureEvent.metaKey());

WebKit doesn't have a fixed line limit length but I'm finding this one pretty
crazy. Would you mind breaking it up into a few lines - maybe one for the
point, one for all the deltas, one for granularity, and one for all the
modifier keys?

> Source/WebCore/platform/ScrollAnimator.cpp:102
> +		   scroll(VerticalScrollbar, ScrollByPixelVelocity, 0,
-deltaY);

this is indented too far, should be 4 spaces

> Source/WebCore/platform/ScrollAnimatorNone.cpp:382
> +    , m_firstVelocity(0.f)

this should just be 0 (see
http://www.webkit.org/coding/coding-style.html#float-suffixes)

> Source/WebCore/platform/ScrollAnimatorNone.cpp:393
> +void ScrollAnimatorNone::fireUpAnAnimation(float x, float y)

this might get you an unused variable warning on some platforms (I forget which
set the compile flags that generate this). The standard ways to avoid this are
to either omit the parameter names, comment them out, or use the UNUSED_PARAM()
macro in the function body

> Source/WebCore/platform/ScrollAnimatorNone.h:142
> +    virtual void fireUpAnAnimation(float, float);

should this take a FloatPoint? most things that take an x/y pair do

> Source/WebKit/chromium/tests/ScrollAnimatorNoneTest.cpp:85
> +    float m_f1;
> +    float m_f2;

are these an X/Y pair? if so can they be a FloatPoint or FloatSize? That will
also give you zero-initialization for free

> Source/WebKit/chromium/tests/ScrollAnimatorNoneTest.cpp:145
> +    scrollAnimatorNone.scroll(HorizontalScrollbar, ScrollByPixelVelocity,
111.f, -42.f);
> +    scrollAnimatorNone.scroll(VerticalScrollbar, ScrollByPixelVelocity,
222.f, 42.f);

drop the .f's here and in the rest of the test files