[webkit-reviews] review denied: [Bug 67822] PlatformGestureEvent::handleGestureEvent should hit test while dispatching Scroll* events. : [Attachment 112607] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Robert Kroeger
<rjkroege at chromium.org>'s request for review:
Bug 67822: PlatformGestureEvent::handleGestureEvent should hit test while
dispatching Scroll* events.
https://bugs.webkit.org/show_bug.cgi?id=67822

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

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=112607&action=review


> Source/WebCore/ChangeLog:7
> +

You need to explain what kind of changes you're making.

> Source/WebCore/ChangeLog:16
> +	   (WebCore::wheelGranularityToScrollGranularity):
> +	   Helper method.
> +	   (WebCore::scrollNode):
> +	   Refactored.

Please put the description on the same line as the function name after :.

> Source/WebCore/page/EventHandler.cpp:178
> +static inline bool gestureNode(const PlatformGestureEvent& e, Node* node)

Please don't use one-letter variable like e.

> Source/WebCore/page/EventHandler.cpp:2238
> +bool EventHandler::passGestureEventToWidgetHelper(PassRefPtr<Node>
targetNode, const PlatformGestureEvent& gestureEvent, const HitTestResult&
result)

Why does this function need to be a member function of Event Handler? It can be
a static non-member function, no?

Why is this function called "helper"? We need a more descriptive name.

> Source/WebCore/page/EventHandler.cpp:2249
> +    if (isOverWidget && target && target->isWidget()) {
> +	  Widget* widget = toRenderWidget(target)->widget();
> +	   if (widget && passGestureEventToWidget(gestureEvent, widget))

We prefer early exit over nested ifs.

> Source/WebCore/page/EventHandler.cpp:2255
> +bool EventHandler::passGestureEventToFameView(const PlatformGestureEvent&
gestureEvent)

Ditto. Also, typo: Fame -> Frame.

> Source/WebCore/page/EventHandler.cpp:2267
> +// Helper to always unlatch target node on leaving scope.
> +class Unlatcher {
> +public:

What's the point of this class? I don't understand what you mean by "unlatch".

Can't you just use RefPtr? What's the point of wrapping it with a class?

> Source/WebCore/page/EventHandler.cpp:2283
> +    Document* doc = m_frame->document();

Please don't use abbreviations like doc.

> Source/WebCore/page/EventHandler.cpp:2292
> +    LayoutPoint vPoint = view->windowToContents(gestureEvent.position());

What does v stand for? Please don't use abbreviations.

> Source/WebCore/page/EventHandler.cpp:2303
> +	   // FIXME: Refactor this code to not hit test multiple times by using
the code
> +	   // written for scroll.

Why are you splitting this comment into 2 lines? It fit the line perfectly.

> Source/WebCore/page/EventHandler.cpp:2320
> +	   bool xscroll = node && scrollNode(gestureEvent.deltaX(),
ScrollByPixel, ScrollLeft, ScrollRight, node, (Node**)0);

Nit: camelCase. xScroll, and s/(Node**)0/0/;

> Source/WebCore/page/EventHandler.cpp:2321
> +	   bool yscroll = node && scrollNode(gestureEvent.deltaY(),
ScrollByPixel, ScrollUp, ScrollDown, node, (Node**)0);

Ditto.

> Source/WebCore/platform/ScrollAnimator.cpp:138
> +	   float deltaX = (event.deltaX() < 0.f) ? max(event.deltaX(), 
-float(maxForwardScrollDelta.width())) : min(event.deltaX(),
float(maxBackwardScrollDelta.width()));

Nit: two spaces before -float. Also, please use C++-style casting e.g.
static_cast<float>().

> Source/WebCore/rendering/RenderBox.cpp:677
> +    fprintf(stderr, "RenderBox::gesture\n");

What's up with this fprintf?

> Source/WebCore/rendering/RenderBox.cpp:678
> +    RenderLayer* layer = layer();

I'm surprised that this didn't cause a compilation error. r- because this is
certainly going to cause a compilation failure on one of ports.