[webkit-reviews] review denied: [Bug 24731] [Win] dropEffect is always set to "copy" at dragend : [Attachment 41057] Patch with test case

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has denied Daniel Bates
<dbates at webkit.org>'s request for review:
Bug 24731: [Win] dropEffect is always set to "copy" at dragend
https://bugs.webkit.org/show_bug.cgi?id=24731

Attachment 41057: Patch with test case
https://bugs.webkit.org/attachment.cgi?id=41057&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> Index: WebKit/win/ChangeLog
> ===================================================================
> --- WebKit/win/ChangeLog	(revision 49429)
> +++ WebKit/win/ChangeLog	(working copy)
> @@ -1,3 +1,38 @@
> +2009-10-11  Daniel Bates  <dbates at webkit.org>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   https://bugs.webkit.org/show_bug.cgi?id=24731
> +	   And
> +	   rdar://problem/5015961
> +	   
> +	   Implements support for DHTML drag-and-drop operations (i.e.
ondragstart, ondragend)
> +	   in the Windows build so that it conforms to the Mac OS X build.
Hence, dropEffect is
> +	   correctly set.
> +	   
> +	   The WebView and WebDropSource drag-and-drop functions, as called by
function
> +	   DoDragDrop in its event loop, neither used the drop effect as
specified by
> +	   event.dataTransfer.dropEffect nor respected
event.dataTransfer.effectsAllowed.
> +	   Instead, these functions defaulted to some hardcoded drop effect and
set of
> +	   allowed drop effects, respectively.
> +
> +	   Tests: fast/events/drag-and-drop.html
> +
> +	   * WebCoreSupport/WebDragClient.cpp:
> +	   (WebDragClient::startDrag):
> +	   * WebDropSource.cpp:
> +	   (WebDropSource::QueryContinueDrag): Moved call to
EventHandler::dragSourceEndedAt
> +	   into method WebDragClient::startDrag.
> +	   * WebDropSource.h:
> +	   * WebView.cpp:
> +	   (WebView::keyStateToDragOperation): Fixes <rdar://problem/5015961>.
Determines
> +	   appropriate drop effect from state of keyboard and allowed effects
> +	   m_page->dragController()->sourceDragOperation().
> +	   (WebView::DragEnter):
> +	   (WebView::DragOver):
> +	   (WebView::Drop):
> +	   * WebView.h:
> +
>  2009-10-09  Adam Barth  <abarth at webkit.org>
>  
>	   Reviewed by Darin Adler.
> Index: WebKit/win/WebDropSource.cpp
> ===================================================================
> --- WebKit/win/WebDropSource.cpp	(revision 49291)
> +++ WebKit/win/WebDropSource.cpp	(working copy)
> @@ -106,10 +106,6 @@ STDMETHODIMP WebDropSource::QueryContinu
>  {
>      if (fEscapePressed || !(grfKeyState & (MK_LBUTTON|MK_RBUTTON))) {
>	   m_dropped = !fEscapePressed;
> -	   if (Page* page = m_webView->page())
> -	       if (Frame* frame = page->mainFrame()) 
> -		   //FIXME: We need to figure out how to find out what actually
happened in the drag <rdar://problem/5015961>
> -		  
frame->eventHandler()->dragSourceEndedAt(generateMouseEvent(m_webView.get(),
false), fEscapePressed ? DragOperationNone : DragOperationCopy);
>	   return fEscapePressed? DRAGDROP_S_CANCEL : DRAGDROP_S_DROP;
>      } else if (Page* page = m_webView->page())
>	   if (Frame* frame = page->mainFrame()) 
> Index: WebKit/win/WebDropSource.h
> ===================================================================
> --- WebKit/win/WebDropSource.h	(revision 49291)
> +++ WebKit/win/WebDropSource.h	(working copy)
> @@ -32,6 +32,12 @@
>  
>  class WebView;
>  
> +namespace WebCore {
> +class PlatformMouseEvent;
> +} // namespace WebCore

I like that you're following our coding style for namespaces in headers. But I
think we normally use a different style for a "non-main" namespace.
Unfortunately this isn't documented anywhere! I think our de facto style in
this case is to indent the "class" line and leave off the comment after the
closing brace.

> @@ -172,16 +172,26 @@ void WebDragClient::startDrag(DragImageR
>	   }
>  
>	   DWORD okEffect =
draggingSourceOperationMaskToDragCursors(m_webView->page()->dragController()->s
ourceDragOperation());
> -	   DWORD effect;
> +	   DWORD effect = DROPEFFECT_NONE;
>	   COMPtr<IWebUIDelegate> ui;
> +	   HRESULT hr = DRAGDROP_S_CANCEL;
>	   if (SUCCEEDED(m_webView->uiDelegate(&ui))) {
>	       COMPtr<IWebUIDelegatePrivate> uiPrivate;
>	       if (SUCCEEDED(ui->QueryInterface(IID_IWebUIDelegatePrivate,
(void**)&uiPrivate)))
> -		   if (SUCCEEDED(uiPrivate->doDragDrop(m_webView,
dataObject.get(), source.get(), okEffect, &effect)))
> -		       return;
> +		   hr = uiPrivate->doDragDrop(m_webView, dataObject.get(),
source.get(), okEffect, &effect);
> +	   } else
> +	       hr = DoDragDrop(dataObject.get(), source.get(), okEffect,
&effect);

This has the same bug as the last version of this patch where you're requiring
all implementors of IWebUIDelegatePrivate to implement doDragDrop. I think all
you need to do is change the "else" to "if (hr == E_NOTIMPL)". That way
implementors of IWebUIDelegatePrivate can just return E_NOTIMPL from doDragDrop
and WebKit will fall back to the default behavior.

Once we get this doDragDrop thing fixed, I think this is ready to go!