[webkit-reviews] review denied: [Bug 81663] [chromium] make trackpad gesture curves configurable externally : [Attachment 137162] 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 81663: [chromium] make trackpad gesture curves configurable externally
https://bugs.webkit.org/show_bug.cgi?id=81663

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

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


> Source/WebCore/ChangeLog:6
> +	   Reviewed by James Robinson.

Leave this "Reviewed by NOBODY (OOPS!)" until you get an r+ on a patch.

> Source/WebCore/platform/TouchpadFlingPlatformGestureCurve.cpp:58
> +const float defaultEmpircalParameters[] = {1.5395e+01, 2.0466e+04,
-2.9899e+04, 2.0577e+04, -5.4966e+03, 1.128445};

In this patch we have the default values here (near the end use) and lot of
code leading up to this point that has to deal with the fact that a user may
specify their own values or want to use these values. Seems like the whole
world would be simpler if these default values were higher up the stack and
always set if the user didn't specify them explicitly.

> Source/WebCore/platform/TouchpadFlingPlatformGestureCurve.cpp:71
> +    for (unsigned i = 0; i < sizeof(defaultEmpircalParameters) /
sizeof(float); i++)

"sizeof(defaultEmpircalParameters) / sizeof(float)" is a mouthful to repeat -
could you store it in a local? i notice that in other places in the code this
is just a static number. why the pretend generality here?

> Source/WebKit/chromium/public/WebInputEvent.h:388
> +	   const float NaN = 0.0f / 0.0f;

It's very weird to initialize to NaN - why would we want to do that instead of
initializing to something sane (like the default curve parameters) or something
like all 0s?  NaN is a footgun

> Source/WebKit/chromium/src/WebViewImpl.cpp:638
> +	   if (event.size == sizeof(WebGestureFlingEvent)) {

Branching on the event.size seems pretty bizarre.  Is there no better way to do
this?