[webkit-reviews] review denied: [Bug 65044] [chromium] Layering violations in gesture recognizer : [Attachment 101894] 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 65044: [chromium] Layering violations in gesture recognizer
https://bugs.webkit.org/show_bug.cgi?id=65044

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

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


This looks good.  Below are a bunch of nit-picky style issues, but I'd like to
see this patch one more time before landing.  Thanks for the patch!

> Source/WebCore/ChangeLog:12
> +    Divided the gesture recognizer up to correct a layering
> +    violation by moving gesture implementation from it to
> +    EventHandler::handleGestureEvent so that the gesture recognizer
> +    could simply be an engine for generating gesture events from
> +    touch events.

Look like you might have trouble with tabs / indent.

> Source/WebCore/page/EventHandler.cpp:2225
> +	   // FIXME: refactor for hit testing only once having addressed the
above fixme.

Can you make this comment a complete sentence?

> Source/WebCore/page/EventHandler.cpp:2234
> +	   }
> +	   break;

Do we need this break if we always return true?  (Also, the } should be
indented one level less.)

> Source/WebCore/page/EventHandler.cpp:2236
> +	   // FIXME: interim implementation pending addressing the fixme above.


Comments should be complete sentences.

> Source/WebCore/page/EventHandler.cpp:2237
> +	   PlatformWheelEvent syntheticWheelEvent(gestureEvent.position(),
gestureEvent.globalPosition(), gestureEvent.deltaX(), gestureEvent.deltaY(),
gestureEvent.deltaX() / 120.0f, gestureEvent.deltaY() / 120.0f,
ScrollByPixelWheelEvent, /* isAccepted */ false, gestureEvent.shiftKey(),
gestureEvent.ctrlKey(), gestureEvent.altKey(), gestureEvent.metaKey());

Should 120.0f be a named constant?

> Source/WebCore/page/EventHandler.cpp:2241
> +	   }
> +	   break;

Same comment.

> Source/WebCore/page/EventHandler.cpp:2248
> +	   view->handleGestureEvent(gestureEvent);

Strange that this case falls through but the other cases return true
explicitly.

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:37
>  

Can we remove the EventHandler include now?

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:106
> +void InnerGestureRecognizer::constructClickGestureEvent(const
PlatformTouchPoint& touchPoint, Gestures* gestures)

construct => append ?

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:118
> +    Vector<PlatformGestureEvent>* gestures = new
Vector<PlatformGestureEvent>();

OwnPtr pls.  We shouldn't have (hardly) any "naked new" calls.	In this case,
new should be wrapped in adoptPtr immediately.

> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:130
> +void InnerGestureRecognizer::constructScrollGesture(const
PlatformTouchPoint& touchPoint, Gestures *gestures)

construct => append as well, probably.

> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:534
> +// FIXME: Opportunity to refactor this with EventHandler::handleGestureEvent


Another comment that should be a complete sentence.

> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:547
> +	   return true;
> +	   }
> +	   break;

Same comment.

> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:555
> +    default:
> +	   break;

We usually omit the default case and list any non-handled enums explicitly.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1999
> +	   webView->resetGestureRecognizer();

Do you mean to intent this line this far?  If so, you might need { } around the
if above.

> Source/WebKit/chromium/src/WebPopupMenuImpl.cpp:145
> +    m_widget->handleGestureEvent((*gestureEvents)[i]);

Bad indent

> Source/WebKit/chromium/src/WebPopupMenuImpl.cpp:268
> +	   return TouchEvent(*static_cast<const WebTouchEvent*>(&inputEvent));
> +	   break;

Another example of return-followed-by-break.

> Source/WebKit/chromium/src/WebViewImpl.cpp:771
> +    bool
defaultPrevented(mainFrameImpl()->frame()->eventHandler()->handleTouchEvent(tou
chEventBuilder));

Please use the assignment form of the constructor.

> Source/WebKit/chromium/src/WebViewImpl.cpp:776
> +	      
mainFrameImpl()->frame()->eventHandler()->handleGestureEvent((*gestureEvents)[i
]);

Another wild intent.

(*gestureEvents)[i] can be written more nicely as gestureEvents->at(i)