[webkit-reviews] review denied: [Bug 49345] [Chromium] Add a basic GestureManager to Chromium : [Attachment 76281] fourth patch

[bugzilla-daemon at webkit.org]

Antonio Gomes <tonikitoo at webkit.org> has denied Robert Kroeger
<rjkroege at chromium.org>'s request for review:
Bug 49345: [Chromium] Add a basic GestureManager to Chromium
https://bugs.webkit.org/show_bug.cgi?id=49345

Attachment 76281: fourth patch
https://bugs.webkit.org/attachment.cgi?id=76281&action=review

------- Additional Comments from Antonio Gomes <tonikitoo at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=76281&action=review

I like Benjamin's review. His comments should be addressed.

> WebCore/page/GestureManager.cpp:46
> +// Edge functions.
> +static bool click(GestureManager*, const PlatformTouchPoint&);
> +static bool clickOrScroll(GestureManager*, const PlatformTouchPoint&);
> +static bool inScroll(GestureManager*, const PlatformTouchPoint&);
> +static bool reset(GestureManager*, const PlatformTouchPoint&);
> +static bool touchDown(GestureManager*, const PlatformTouchPoint&);
> +

If you are passing the GM here anyways, why not putting these methods as class
methods?

> WebCore/platform/PlatformWheelEvent.h:114
>  
> +	   PlatformWheelEvent(IntPoint position,
> +			      IntPoint globalPosition,
> +			      float deltaX,
> +			      float deltaY,
> +			      float wheelTicksX,
> +			      float wheelTicksY,
> +			      PlatformWheelEventGranularity granularity,
> +			      bool isAccepted,
> +			      bool shiftKey,
> +			      bool ctrlKey,
> +			      bool altKey,
> +			      bool metaKey)
> +	       : m_position(position)
> +	       , m_globalPosition(globalPosition)
> +	       , m_deltaX(deltaX)
> +	       , m_deltaY(deltaY)
> +	       , m_wheelTicksX(wheelTicksX)
> +	       , m_wheelTicksY(wheelTicksY)
> +	       , m_granularity(granularity)
> +	       , m_isAccepted(isAccepted)
> +	       , m_shiftKey(shiftKey)
> +	       , m_ctrlKey(ctrlKey)
> +	       , m_altKey(altKey)
> +	       , m_metaKey(metaKey)
> +	   {
> +	   }

You should say in the changelog why you are adding this. (Or it could come on
its own patch(?)