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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon May 9 10:20:48 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 92763: Patch
https://bugs.webkit.org/attachment.cgi?id=92763&action=review

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

Can we split this into 2 patches?  The first patch would just be a refactoring
patch and not change any behavior.  It would introduce the DragState class and
probably move draggableNode into DragController.  The second patch would be the
behavioral change.

> LayoutTests/ChangeLog:11
> +	   * fast/css/user-drag-none.html:

Can you add a sentence or two explaining why these changed?

> LayoutTests/editing/pasteboard/drag-selected-image-to-contenteditable.html:47

> +    eventSender.leapForward(500);

Hmm, how was this passing on Mac before?

> Source/WebCore/page/DragController.cpp:580
> +// Determine which node the user wants to drag. In order of priority:
> +// 1. The user is dragging a selection.
> +// 2. The user is dragging an element where draggable=true.
> +// 3. The user is dragging an element where draggable is the default auto
value (e.g. unset) and
> +// the element is either an anchor element with an href attribute or an
image element.

WebKit normally avoids comments that describe what the code is doing.  I think
the preferred style is to make each of these a function, e.g.,
isDraggingSelection, isDraggableElement, and isElementDraggableByDefault.

> Source/WebCore/page/DragController.cpp:609
> +		       && (m_dragSourceAction& DragSourceActionImage)) {

Nit: missing space before &

> Source/WebCore/page/DragController.cpp:746
> +	       m_dragOffset = IntPoint((int)(dragOrigin.x() - dragLoc.x()),
(int)(dragOrigin.y() - dragLoc.y()));

Nit: static_cast instead of (int)

> Source/WebCore/page/DragState.h:31
> +#include <wtf/FastAllocBase.h>
> +#include <wtf/Noncopyable.h>
> +#include <wtf/RefPtr.h>

Can we use wtf/Forward.h for some of these?

> Source/WebCore/page/EventHandler.cpp:2640
> +    default:
> +	   break;

Can we enumerate the other cases here rather than using default:?  That will
give us a compile break if someone adds a new value to the enum rather than
silently falling through.


More information about the webkit-reviews mailing list