[Webkit-unassigned] [Bug 34305] Drag and Drop: Windows uses "stop" sign as cursor when dragging

[bugzilla-daemon at webkit.org]

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


Adam Roben (aroben) <aroben at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #47722|review?                     |review-
               Flag|                            |




--- Comment #3 from Adam Roben (aroben) <aroben at apple.com>  2010-01-29 14:14:41 PST ---
(From update of attachment 47722)
> +    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.)

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