[webkit-reviews] review denied: [Bug 81458] [chromium] synthesize wheel events for fling on main thread : [Attachment 132513] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Robert Kroeger
<rjkroege at chromium.org>'s request for review:
Bug 81458: [chromium] synthesize wheel events for fling on main thread
https://bugs.webkit.org/show_bug.cgi?id=81458

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

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


Please remove the "[not ready for review]" from the bug title.

Otherwise, very close but not quite.

> Source/WebKit/chromium/src/WebViewImpl.cpp:370
> +    , m_flingModifier(0)

this member is always 0 as far as I can tell. why even have it?

> Source/WebKit/chromium/src/WebViewImpl.cpp:417
> +    m_lastWheelPosition = WebPoint(-1, -1);
> +    m_lastWheelGlobalPosition = WebPoint(-1, -1);

WebPoint members will be initialized to (0, 0) - I don't see any value in
initializing them again to a different arbitrary value

> Source/WebKit/chromium/src/WebViewImpl.cpp:616
> +    const float tickDivisor =
static_cast<float>(WebCore::WheelEvent::tickMultiplier);

do you need an explicit cast here? i don't think we have

> Source/WebKit/chromium/src/WebViewImpl.cpp:645
> +	   // FIXME: put this into a sub-function?

you don't need a sub-function for m_gestureAnimation.clear(), i don't think

> Source/WebKit/chromium/src/WebViewImpl.cpp:649
> +	   m_flingModifier = 0;
> +	   m_lastWheelPosition = WebPoint(-1, -1);
> +	   m_lastWheelGlobalPosition = WebPoint(-1, -1);

these all useless useless - these values won't be touched if m_gestureAnimation
is NULL

> Source/WebKit/chromium/src/WebViewImpl.cpp:1326
> +    // Create synthetic wheel events as necessary for fling.

this has to be in updateAnimations(). this function is not called at all when
using the threaded compositor

> Source/WebKit/chromium/src/WebViewImpl.cpp:1331
> +	   scheduleAnimation();
> +    else
> +	   m_gestureAnimation.clear();

indentation here is off