[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