[webkit-reviews] review denied: [Bug 34305] Drag and Drop: Windows uses "stop" sign as cursor when dragging : [Attachment 47722] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jan 29 14:14:40 PST 2010


Adam Roben (aroben) <aroben at apple.com> has denied Brian Weinstein
<bweinstein at apple.com>'s request for review:
Bug 34305: Drag and Drop: Windows uses "stop" sign as cursor when dragging
https://bugs.webkit.org/show_bug.cgi?id=34305

Attachment 47722: Patch
https://bugs.webkit.org/attachment.cgi?id=47722&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> +    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.)


More information about the webkit-reviews mailing list