[webkit-reviews] review denied: [Bug 59409] Improve drag start logic : [Attachment 93391] Patch

Fri May 13 10:22:23 PDT 2011

Tony Chang <tony at chromium.org> has denied Daniel Cheng <dcheng at chromium.org>'s
request for review:
Bug 59409: Improve drag start logic
https://bugs.webkit.org/show_bug.cgi?id=59409

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

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=93391&action=review

This is looking really good.  One more round and I think we'll be done.

>
LayoutTests/platform/mac/editing/pasteboard/drag-selections-to-contenteditable.
html:1
> +<html>

Morita is right, this could be a dump as markup test.  You can include
LayoutTest/resources/dump-as-markup.js to use Markup.dump.  It would make the
pixel result unnecessary (seems like dumping the DOM tree of the
contentEditable region is sufficient).

> Source/WebCore/page/DragController.cpp:686
> +    if (!state.m_dragSrc->contains(hitTestResult.innerNode()))
> +	   // The original node being dragged isn't under the mouse for some
reason... bail out.
> +	   return false;

Is this check new?  Can the comment describe why this might happen rather than
just describing what the code is doing?

> Source/WebCore/page/EventHandler.cpp:2732
> +	       // 4. We enter this block again, and since it's now marked as a
selection drag, we
> +	       //    cancel the drag.
> +	       m_mouseDownTimestamp = event.event().timestamp() - 2 *
TextDragDelay;

This seems hacky-- is there other code that depends on m_mouseDownTimestamp? 
Maybe we can put an extra bool in dragState() to keep track of this case?

> Source/WebCore/page/EventHandler.cpp:2745
> +    if (!ExactlyOneBitSet(dragState().m_dragType)) {
> +	   ASSERT((dragState().m_dragType & DragSourceActionSelection));
> +	   ASSERT((dragState().m_dragType & ~DragSourceActionSelection) ==
DragSourceActionDHTML
> +		   || (dragState().m_dragType & ~DragSourceActionSelection) ==
DragSourceActionImage
> +		   || (dragState().m_dragType & ~DragSourceActionSelection) ==
DragSourceActionLink);
> +	   dragState().m_dragType = DragSourceActionSelection;
> +    }

Why is this needed?