[webkit-reviews] review denied: [Bug 39725] [Chromium] Dragging over an element with scrollbars should scroll the element when dragging near edges : [Attachment 57634] patch v2

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied MORITA Hajime
<morrita at google.com>'s request for review:
Bug 39725: [Chromium] Dragging over an element with scrollbars should scroll
the element when dragging near edges
https://bugs.webkit.org/show_bug.cgi?id=39725

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

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
WebKit/chromium/public/WebView.h:173
 +	virtual void dragSourceMovedTo(const WebPoint& clientPoint, const
WebPoint& screenPoint, WebDragOperation) = 0;
nit: please use the same formatting as dragSourceEndedAt

WebKit/chromium/src/WebViewImpl.cpp:1550
 +  void WebViewImpl::dragSourceMovedTo(
nit: this method impl should be placed after dragSourceEndedAt
to match the declaration order.

WebKit/chromium/src/WebViewImpl.cpp:1557
 +  
nit: only one new line between method impls.

WebKit/chromium/src/WebViewImpl.cpp:1676
 +  static IntSize distanceTo(const WebCore::IntPoint& point, const
WebCore::IntRect& rect)
nit: no need for WebCore:: prefix since there is a using namespace WebCore
at the top of this file.  also, please move static helper functions up to
the top of the file after the static data declarations.

WebKit/chromium/src/WebViewImpl.cpp:1676
 +  static IntSize distanceTo(const WebCore::IntPoint& point, const
WebCore::IntRect& rect)
nit: how about naming this distanceToRect?

how about this as a comment:
// Computes the distance from a point outside a rect to the nearest edge of the
rect.

(I would also assert that the point is not contained within the rect if that
is indeed appropriate.)

WebKit/chromium/src/WebViewImpl.cpp:1695
 +	WebCore::FrameView* view = mainFrameImpl()->frameView();
nit: no WebCore:: prefix needed

WebKit/chromium/src/WebViewImpl.cpp:1693
 +	static const int kScrollMargin = 30;
nit: constants should not have the k prefix.  just do scrollMargin.

WebKit/chromium/src/WebViewImpl.h:136
 +	virtual void dragSourceMovedTo(const WebPoint& clientPoint, const
WebPoint& screenPoint, WebDragOperation);
please list this method in the same order that it is declared
in WebView.h

WebKit/chromium/src/WebViewImpl.cpp:1690
 +  void WebViewImpl::scrollForDragging(const WebPoint& clientPoint)
this method should be defined just above hideSelectPopup() so that
the order of methods in the .cpp file matches the order of the
methods in the .h file.