Bug 34305 - Drag and Drop: Windows uses "stop" sign as cursor when dragging
Summary: Drag and Drop: Windows uses "stop" sign as cursor when dragging
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Windows XP
: P2 Normal
Assignee: Brian Weinstein
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-28 21:18 PST by Brian Weinstein
Modified: 2010-02-01 16:14 PST (History)
2 users (show)

See Also:


Attachments
Patch (6.34 KB, patch)
2010-01-28 21:30 PST, Brian Weinstein
no flags Details | Formatted Diff | Diff
Patch (7.91 KB, patch)
2010-01-29 10:59 PST, Brian Weinstein
no flags Details | Formatted Diff | Diff
Patch (7.80 KB, patch)
2010-01-29 15:36 PST, Brian Weinstein
aroben: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Weinstein 2010-01-28 21:18:45 PST
Drag and Drop: Windows uses "stop" sign as cursor when dragging
Comment 1 Brian Weinstein 2010-01-28 21:30:22 PST
Created attachment 47672 [details]
Patch
Comment 2 Brian Weinstein 2010-01-29 10:59:18 PST
Created attachment 47722 [details]
Patch
Comment 3 Adam Roben (:aroben) 2010-01-29 14:14:41 PST
Comment on attachment 47722 [details]
Patch

> +    RECT webViewRect;
> +    HWND viewWindow;
> +    if (FAILED(m_webView->viewWindow((OLE_HANDLE*)&viewWindow)))
> +        return DRAGDROP_S_USEDEFAULTCURSORS;
> +    ::GetWindowRect(viewWindow, &webViewRect);

reinterpret_cast would be better than the C-style cast.

You should wait to declare webViewRect until just before you call GetWindowRect.

> +    if (point.x() > (webViewRect.right - webViewRect.left) || point.x() < 0 || point.y() > (webViewRect.bottom - webViewRect.top) || point.y() < 0) {
> +        // If our cursor is outside the bounds of the webView, we want to let Windows select the cursor.
> +        return DRAGDROP_S_USEDEFAULTCURSORS;
> +    }

Comparing an X coordinate to the width, and a Y coordinate to the height, only makes sense if you are assuming that the rect has its origin at 0,0. But even then you might as well compare the X coordinate to the left and right edges of the window, and the Y coordinate to the top and bottom edges of the window. Comparing to the width and height just makes the code more fragile and less clear.

In this case, the rect *does not* have its origin at 0,0, so this code is wrong. GetWindowRect returns a rect in screen coordinates. I would rewrite this like so:

RECT webViewRect;
GetWindowRect(&webViewRect);

POINT point;
GetCursorPos(&point);

if (!IntRect(webViewRect).contains(point)) {

> +    FrameView* view = m_webView->page()->mainFrame()->view();
> +    if (view) {

You should make this an early return, just like all the other ifs in this function.

> +    virtual HRESULT STDMETHODCALLTYPE setShowCustomDragCursors(BOOL);
> +    virtual HRESULT STDMETHODCALLTYPE showCustomDragCursors(BOOL*);

This name makes me think that when I call setShowCustomDragCursors(TRUE), suddenly custom cursors will appear on my screen. I think putting "Enabled" in the name of these functions would fix this. Something like [set]CustomDragCursorsEnabled. (You should change the preference key to match.)
Comment 4 Brian Weinstein 2010-01-29 14:17:24 PST
(In reply to comment #3)
> (From update of attachment 47722 [details])
> > +    RECT webViewRect;
> > +    HWND viewWindow;
> > +    if (FAILED(m_webView->viewWindow((OLE_HANDLE*)&viewWindow)))
> > +        return DRAGDROP_S_USEDEFAULTCURSORS;
> > +    ::GetWindowRect(viewWindow, &webViewRect);
> 
> reinterpret_cast would be better than the C-style cast.
> 
> You should wait to declare webViewRect until just before you call
> GetWindowRect.
> 

Will do, got this code from a combination of WebDropSource and WebView, so I'll clean it up.

> > +    if (point.x() > (webViewRect.right - webViewRect.left) || point.x() < 0 || point.y() > (webViewRect.bottom - webViewRect.top) || point.y() < 0) {
> > +        // If our cursor is outside the bounds of the webView, we want to let Windows select the cursor.
> > +        return DRAGDROP_S_USEDEFAULTCURSORS;
> > +    }
> 
> Comparing an X coordinate to the width, and a Y coordinate to the height, only
> makes sense if you are assuming that the rect has its origin at 0,0. But even
> then you might as well compare the X coordinate to the left and right edges of
> the window, and the Y coordinate to the top and bottom edges of the window.
> Comparing to the width and height just makes the code more fragile and less
> clear.
> 
> In this case, the rect *does not* have its origin at 0,0, so this code is
> wrong. GetWindowRect returns a rect in screen coordinates. I would rewrite this
> like so:
> 
> RECT webViewRect;
> GetWindowRect(&webViewRect);
> 
> POINT point;
> GetCursorPos(&point);
> 
> if (!IntRect(webViewRect).contains(point)) {

That's a lot cleaner, I'll fix.

> 
> > +    FrameView* view = m_webView->page()->mainFrame()->view();
> > +    if (view) {
> 
> You should make this an early return, just like all the other ifs in this
> function.

Will do.

> 
> > +    virtual HRESULT STDMETHODCALLTYPE setShowCustomDragCursors(BOOL);
> > +    virtual HRESULT STDMETHODCALLTYPE showCustomDragCursors(BOOL*);
> 
> This name makes me think that when I call setShowCustomDragCursors(TRUE),
> suddenly custom cursors will appear on my screen. I think putting "Enabled" in
> the name of these functions would fix this. Something like
> [set]CustomDragCursorsEnabled. (You should change the preference key to match.)

I think that makes more sense. 

A new patch will be out shortly, thanks for the review!
Comment 5 Brian Weinstein 2010-01-29 15:36:04 PST
Created attachment 47738 [details]
Patch
Comment 6 Adam Roben (:aroben) 2010-01-29 15:42:42 PST
Comment on attachment 47738 [details]
Patch

> +    if (!IntRect(webViewRect).contains(cursorPoint)) {

You could actually use PtInRect here instead.

r=me
Comment 7 Eric Seidel (no email) 2010-02-01 16:13:28 PST
Attachment 47738 [details] was posted by a committer and has review+, assigning to Brian Weinstein for commit.
Comment 8 Brian Weinstein 2010-02-01 16:14:56 PST
Sorry, landed in r54083.