[webkit-reviews] review denied: [Bug 103952] Scroll gestures should not create wheel events : [Attachment 181938] Patch

[bugzilla-daemon at webkit.org]

Antonio Gomes <tonikitoo at webkit.org> has denied Terry Anderson
<tdanderson at chromium.org>'s request for review:
Bug 103952: Scroll gestures should not create wheel events
https://bugs.webkit.org/show_bug.cgi?id=103952

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

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


Looks generally good, although I did not complete review it. I have some
comments and requests, and I think it is worth it another interaction. r- due
to that.

> Source/WebCore/page/EventHandler.cpp:2497
> +	       RenderObject* latchedRenderer = node->renderer();
> +	       if (m_lastHitTestResultOverWidget && latchedRenderer &&
latchedRenderer->isWidget()) {
> +		   Widget* widget = toRenderWidget(latchedRenderer)->widget();
> +		   if (widget)
> +		       passGestureEventToWidget(gestureEvent, widget);
> +	       }

there is another similar block of code below. Should we create a static local
shouldPassGestureToWidget method, and avoid duplications?

> Source/WebCore/page/EventHandler.cpp:2626
> +    bool shouldPropagate = (gestureEvent.type() ==
PlatformEvent::GestureScrollUpdatePropagated) ? true : false;

nit: no " a? b :c;" needed here.

> Source/WebCore/page/EventHandler.cpp:2670
> +	   m_scrollGestureHandlingNode =
closestScrollableNodeInDocumentIfPossible(result.innerNode());

I remember that the BlackBerry port has a recursive version of
"closestScrollableNodeInDocumentIfPossible" that returns a list of scrollable
layers, from the deepest to the "shallowest", whereas given the initial user
scroll direction and the amount of offset left to be scrolled in that
direction, it could pick the proper layer.

see
http://trac.webkit.org/browser/trunk/Source/WebKit/blackberry/Api/InRegionScrol
ler.cpp#L202 - void
InRegionScrollerPrivate::calculateInRegionScrollableAreasForPoint(const
WebCore::IntPoint& documentPoint)

Also, once a layer was picked, it would stick with it for the whole scrolling
action.

How does it behavior in your case?

> Source/WebCore/page/EventHandler.h:379
> +    bool hitTestForScrollGestureHandlingNode(const PlatformGestureEvent&);

I guess a "IfNeeded" suffix to the method name would have made it clearer to
James that it might bail out earlier without hit testing.

> Source/WebCore/page/EventHandler.h:470
> +    bool m_lastHitTestResultOverWidget;

do you need to manually default initialize it in the ctor?

> Source/WebCore/page/chromium/EventHandlerChromium.cpp:97
> +    return
static_cast<FrameView*>(widget)->frame()->eventHandler()->handleGestureEvent(ge
stureEvent);

do not you need to convert the gestureEvent position to the widget's
coordinates?

> Source/WebCore/page/gtk/EventHandlerGtk.cpp:86
> +#if ENABLE(GESTURE_EVENTS)
> +bool EventHandler::passGestureEventToWidget(const PlatformGestureEvent&,
Widget*)
> +{
> +    notImplemented();
> +    return false;
> +}
> +#endif
> +

have you considered to add a default stub/empty implementation to
EventHandler.h instead of adding stubs to all non-chromium port specific files?


What do other similar cases do?

> Source/WebCore/rendering/RenderLayer.cpp:1965
> +    scrollBy(delta, clamp, true);

bools are to be avoided. At the very least add a comment: /*whatIsThisBool*/,
but an enum is highly recommended.