[webkit-reviews] review denied: [Bug 26699] [Win] HTML5 Drag and drop, dragend is not fired when pressing Esc : [Attachment 32104] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has denied Erik Arvidsson
<arv at chromium.org>'s request for review:
Bug 26699: [Win] HTML5 Drag and drop, dragend is not fired when pressing Esc
https://bugs.webkit.org/show_bug.cgi?id=26699

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

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog (revision 45412)
> +++ WebCore/ChangeLog (working copy)
> @@ -1,3 +1,13 @@
> +2009-06-30  Erik Arvidsson  <arv at chromium.org>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Fixes issue where escape did not cancel the drag and drop operation
on windows.

You should add the URL and title of this bug to your ChangeLogs.

> Property changes on: WebCore/manual-tests/drag-escape.html
> ___________________________________________________________________
> Added: svn:executable
>    + *

Please remove the executable bit. It would also be good to set svn:eol-style to
native.

> 
> Index: WebCore/page/EventHandler.cpp
> ===================================================================
> --- WebCore/page/EventHandler.cpp	(revision 45393)
> +++ WebCore/page/EventHandler.cpp	(working copy)
> @@ -2102,6 +2102,7 @@ void EventHandler::dragSourceEndedAt(con
>      }
>      freeClipboard();
>      dragState().m_dragSrc = 0;
> +    m_mouseDownMayStartDrag = false;
>  }

It's probably worth adding a comment here about why this is needed (i.e., to
handle situations where the drag is cancelled while the mouse button is still
down).

>  STDMETHODIMP WebDropSource::QueryContinueDrag(BOOL fEscapePressed, DWORD
grfKeyState)
>  {
> -    if(fEscapePressed)
> -	   return DRAGDROP_S_CANCEL;
> -
> -    if(!(grfKeyState & (MK_LBUTTON|MK_RBUTTON))) {
> -	   m_dropped = true;
> +    if(fEscapePressed || !(grfKeyState & (MK_LBUTTON|MK_RBUTTON))) {

Please add a space after "if" while you're modifying this line.

> +	   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), DragOperationCopy);

It doesn't seem right to pass DragOperationCopy if Escape was pressed.

> -	   return DRAGDROP_S_DROP;
> +		return fEscapePressed? DRAGDROP_S_CANCEL : DRAGDROP_S_DROP;

Tabs! Please use 4-space indents.

Thanks for looking at this!