[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 #6 from MORITA Hajime <morrita at google.com>  2010-06-02 01:49:19 PST ---
Hi Jian, thank you for reviewing!
I updated the patch.

(In reply to comment #4)
> (From update of attachment 57298 [details])
> WebCore/platform/graphics/IntRect.cpp:57
>  +  IntSize IntRect::directionTo(const WebCore::IntPoint& target) const
> The name directionTo seems to be confusing. Will it be better to distanceTo? Also better to comment what distance to rectangle is meant.
> In addition, I am thinking if this routine is only needed in chromium platform, probably we do not want to add it to IntRect.*.
Renamed, and moved it to WebViewImpl.cpp local

> 
> WebCore/platform/graphics/IntRect.h:119
>  +      IntSize directionTo(const WebCore::IntPoint& target) const;
> The argument name 'target' can be omitted.
Done.

> 
> WebKit/chromium/public/WebView.h:172
>  +      // Notified the WebView that a dragging is going on.
> Notified => Notifies
Done.
> 
> WebKit/chromium/public/WebView.h:173
>  +      virtual void dragSourceMovedTo(
> No need to fold into multiple lines.
Done.
> 
> WebKit/chromium/src/WebViewImpl.cpp:1550
>  +  void WebViewImpl::dragSourceMovedTo(
> ditto.
Done.

> 
> WebKit/chromium/src/WebViewImpl.cpp:1678
>  +      WebCore::FrameView* frameView = mainFrameImpl()->frameView();
> Do we need to check if frameView is NULL?
Guarded null case.

> 
> WebKit/chromium/src/WebViewImpl.cpp:1684
>  +      bounds.setHeight(bounds.height() - margin*2);
> Should have one space before and after '*'.
Done.

> 
> WebKit/chromium/src/WebViewImpl.cpp:1686
>  +      bounds.setWidth(bounds.width() - margin*2);
> ditto.
Done.

> 
> WebKit/chromium/src/WebViewImpl.h:136
>  +      virtual void dragSourceMovedTo(
> ditto.
Done.

> 
> WebKit/chromium/src/WebViewImpl.h:331
>  +      static const int kScrollMarginRatio = 20;
> This const definition should be moved to WebViewImpl::scrollForDragging. Also please comment why choosing value 20?
Moved to function static.
The value has left under experience. It was bad thiing...
I changed the value based on the observation on Safari behavior.

> 
> WebKit/chromium/src/WebViewImpl.h:353
>  +      void scrollForDragging(const WebPoint& clientPoint);
> The argument name can be omitted.
Done.

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