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

[bugzilla-daemon at webkit.org]

Adam Barth <abarth 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 106903: Patch
https://bugs.webkit.org/attachment.cgi?id=106903&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=106903&action=review


I didn't review the whole patch, but this needs a bunch of work.  I'm sorry
that I don't have time to help you with this patch.  You might try dglazkov. 
He's done a bunch of work with event handling.

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

This looks dangerous.  This point could become junk while dispatching events. 
It's probably best to grab the document point from the frame each time you need
it.

> Source/WebCore/page/EventHandler.cpp:2226
> +	   if (m_gestureEventWidgetTarget.get() &&
m_gestureEventWidgetTarget->refCount() > 1 &&
passGestureEventToWidget(gestureEvent, m_gestureEventWidgetTarget.get()))

Crazy.	It's very rare to branch on the refCount of an object.	I suspect this
isn't the right thing to do.

> Source/WebCore/page/EventHandler.cpp:2228
> +	   if (m_gestureEventWidgetTarget.get() &&
m_gestureEventWidgetTarget->hasOneRef())

No need to call get() to convert a RefPtr to a bool.

> Source/WebCore/page/EventHandler.cpp:2232
> +	       RenderLayer* layer =
((RenderBox*)m_gestureTargetNode->renderer())->layer();

Please use C++ style casts.  How do you know its a RenderBox??

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

vPoint => please use full words when naming variables.

> Source/WebCore/page/EventHandler.cpp:2251
> +	   Node* node;
> +	   bool isOverWidget;

Please initialize scalars.

> Source/WebCore/page/EventHandler.cpp:2257
> +	   node = result.innerNode();

Why not just declare node at this point?  Then we don't need to worry about
someone using it between when it is declared and when it is initialized.  Also,
consider taking a reference to the node so that it doesn't get deallocated out
from under you.

> Source/WebCore/page/EventHandler.cpp:2260
> +	   if (node) {

Prefer early return.

> Source/WebCore/page/EventHandler.cpp:2269
> +	       node = node->shadowAncestorNode();

Why are we re-using the same variable?	/me is confused.

> Source/WebCore/page/EventHandler.cpp:2281
> +	       while (target && !target->isRoot()) {
> +		   if (target->isBox()) {
> +		       RenderBox* rb = (RenderBox*)target;
> +		       if (rb->canBeScrolledAndHasScrollableArea()) {
> +			   m_gestureTargetNode = rb->node();
> +			   rb->layer()->handleGestureEvent(gestureEvent);
> +			   return true; // FIXME: Prove that this is correct.
> +		       }
> +		   }
> +		   target = target->parent();

This looks like a lot of render tree manipulation to be doing outside of the
rendering directory.