[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





--- Comment #4 from Brian Weinstein <bweinstein at apple.com>  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!

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