[Webkit-unassigned] [Bug 39725] [Chromium] Dragging over an element with scrollbars should scroll the element when dragging near edges

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=39725





--- Comment #11 from MORITA Hajime <morrita at google.com>  2010-06-02 20:26:30 PST ---
Hi Darin, thank you for revewing!

(In reply to comment #7)
> (From update of attachment 57634 [details])
> 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
Done.

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

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

> 
> 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.
Done.

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

> 
> 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.)
In some case it goes (0,0) because IntRect::contains() doesn't count its borders as its area.
So I added check instead assertion.


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

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

> 
> 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
Done.

> 
> 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.
Done.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.