[webkit-reviews] review denied: [Bug 33055] [Qt] Drag & drop layout tests fail even when run manually : [Attachment 45671] Patch.
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jan 5 08:52:32 PST 2010
Kenneth Rohde Christiansen <kenneth at webkit.org> has denied Yael
<yael.aharon at nokia.com>'s request for review:
Bug 33055: [Qt] Drag & drop layout tests fail even when run manually
https://bugs.webkit.org/show_bug.cgi?id=33055
Attachment 45671: Patch.
https://bugs.webkit.org/attachment.cgi?id=45671&action=review
------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
> +static inline Qt::DropActions dragOpToDropActions(unsigned op)
> +{
> + Qt::DropActions result = Qt::IgnoreAction;
> + if (op & DragOperationCopy)
> + result = Qt::CopyAction;
> + if (op & DragOperationMove)
> + result |= Qt::MoveAction;
> + if (op & DragOperationGeneric)
> + result |= Qt::MoveAction;
> + if (op & DragOperationLink)
> + result |= Qt::LinkAction;
> + return result;
> +}
please write out operation in the method name.
> +
> +static inline DragOperation dropActionToDragOp(Qt::DropActions action)
> +{
> + DragOperation result = DragOperationNone;
> + if (action & Qt::CopyAction)
> + result = DragOperationCopy;
> + if (action & Qt::LinkAction)
> + result = DragOperationLink;
> + if (action & Qt::MoveAction)
> + result = DragOperationMove;
> + return result;
> +}
same
> +
> DragDestinationAction DragClientQt::actionMaskForDrag(DragData*)
> {
> return DragDestinationActionAny;
> @@ -58,7 +86,7 @@
> {
> }
>
> -void DragClientQt::startDrag(DragImageRef, const IntPoint&, const IntPoint&,
Clipboard* clipboard, Frame*, bool)
> +void DragClientQt::startDrag(DragImageRef, const IntPoint&, const IntPoint&,
Clipboard* clipboard, Frame* frame, bool)
> {
> #ifndef QT_NO_DRAGANDDROP
> QMimeData* clipboardData =
static_cast<ClipboardQt*>(clipboard)->clipboardData();
> @@ -66,10 +94,16 @@
> QWidget* view = m_webPage->view();
> if (view) {
> QDrag *drag = new QDrag(view);
> - if (clipboardData->hasImage())
> + if (clipboardData && clipboardData->hasImage())
>
drag->setPixmap(qvariant_cast<QPixmap>(clipboardData->imageData()));
> + DragOperation dragOperationMask = DragOperationEvery;
> + clipboard->sourceOperation(dragOperationMask);
> drag->setMimeData(clipboardData);
> - drag->start();
> + Qt::DropAction actualDropAction =
drag->exec(dragOpToDropActions(dragOperationMask));
You only seem to use the dragOpToDropActions here and dragOperationMask always
seems to be the same. Any reason for having that method then?
> +
> + // Send dragEnd event
> + PlatformMouseEvent
me(m_webPage->view()->mapFromGlobal(QCursor::pos()), QCursor::pos(),
LeftButton, MouseEventMoved, 0, false, false, false, false, 0);
> + frame->eventHandler()->dragSourceEndedAt(me,
dropActionToDragOp(actualDropAction));
> }
> #endif
> }
> Index: WebKit/qt/ChangeLog
> ===================================================================
> --- WebKit/qt/ChangeLog (revision 52663)
> +++ WebKit/qt/ChangeLog (working copy)
> @@ -1,3 +1,32 @@
> +2009-12-30 Yael Aharon <yael.aharon at nokia.com>
> +
> + Reviewed by NOBODY (OOPS!).
> +
> + Drag & drop layout tests fail even when run manually
> + https://bugs.webkit.org/show_bug.cgi?id=33055
> +
> + No new tests. Fix 3 layout tests when run manually.
> + fast/events/drag-and-drop.html
> + fast/events/drag-and-drop-dataTransfer-types-nocrash.html
> + fast/events/drag-and-drop-fire-drag-dragover.html
> + Running these tests in DRT will be fixed in 31332.
> +
> + * Api/qwebpage.cpp:
> + (dropActionToDragOp):
> + (dragOpToDropAction):
> + (QWebPagePrivate::dragEnterEvent):
> + (QWebPagePrivate::dragMoveEvent):
> + (QWebPagePrivate::dropEvent):
> + Accept drag events even if they are not over a drop target.
> + This is to ensure that drag events will continue to be delivered.
> +
> + * Api/qwebpage_p.h:
> + * WebCoreSupport/DragClientQt.cpp:
> + (WebCore::dragOpToDropActions):
> + (WebCore::dropActionToDragOp):
> + (WebCore::DragClientQt::startDrag):
> + Send dragEnd event.
> +
> 2009-12-30 Laszlo Gombos <laszlo.1.gombos at nokia.com>
>
> Reviewed by Simon Hausmann.
> Index: WebKit/qt/Api/qwebpage.cpp
> ===================================================================
> --- WebKit/qt/Api/qwebpage.cpp (revision 52606)
> +++ WebKit/qt/Api/qwebpage.cpp (working copy)
> @@ -345,7 +345,7 @@
> if (actions & Qt::CopyAction)
> result |= DragOperationCopy;
> if (actions & Qt::MoveAction)
> - result |= DragOperationMove;
> + result |= (DragOperationMove | DragOperationGeneric);
Maybe add a comment here as the change is not obvious
> if (actions & Qt::LinkAction)
> result |= DragOperationLink;
> return (DragOperation)result;
> @@ -358,6 +358,8 @@
> result = Qt::CopyAction;
> else if (actions & DragOperationMove)
> result = Qt::MoveAction;
> + else if (actions & DragOperationGeneric)
> + result = Qt::MoveAction;
Same here
> else if (actions & DragOperationLink)
> result = Qt::LinkAction;
> return result;
> @@ -1091,8 +1093,9 @@
> dropActionToDragOp(ev->possibleActions()));
> Qt::DropAction action =
dragOpToDropAction(page->dragController()->dragEntered(&dragData));
> ev->setDropAction(action);
> - if (action != Qt::IgnoreAction)
> - ev->accept();
> + // We must accept this event in order to receive the drag move events
that are sent
> + // while the drag and drop action is in progress.
> + ev->accept();
> #endif
> }
>
> @@ -1132,9 +1135,11 @@
> DragData dragData(ev->mimeData(), ev->pos(), QCursor::pos(),
> dropActionToDragOp(ev->possibleActions()));
> Qt::DropAction action =
dragOpToDropAction(page->dragController()->dragUpdated(&dragData));
> + m_lastDropAction = action;
> ev->setDropAction(action);
> - if (action != Qt::IgnoreAction)
> - ev->accept();
> + // We must accept this event in order to receive the drag move events
that are sent
> + // while the drag and drop action is in progress.
> + ev->accept();
> #endif
> }
>
> @@ -1143,8 +1148,7 @@
> #ifndef QT_NO_DRAGANDDROP
> DragData dragData(ev->mimeData(), ev->pos().toPoint(),
> QCursor::pos(), dropActionToDragOp(ev->possibleActions()));
> - Qt::DropAction action =
dragOpToDropAction(page->dragController()->performDrag(&dragData));
> - if (action != Qt::IgnoreAction)
> + if (page->dragController()->performDrag(&dragData))
> ev->accept();
> #endif
> }
> @@ -1152,10 +1156,11 @@
> void QWebPagePrivate::dropEvent(QDropEvent* ev)
> {
> #ifndef QT_NO_DRAGANDDROP
> + // Overwrite the defaults set byQDragManager::defaultAction()
missing space byQDragManager
> + ev->setDropAction(m_lastDropAction);
You don't need to clear this m_lastDropAction somewhere?
> DragData dragData(ev->mimeData(), ev->pos(), QCursor::pos(),
> - dropActionToDragOp(ev->possibleActions()));
> - Qt::DropAction action =
dragOpToDropAction(page->dragController()->performDrag(&dragData));
> - if (action != Qt::IgnoreAction)
> + dropActionToDragOp(Qt::DropAction(ev->dropAction())));
> + if (page->dragController()->performDrag(&dragData))
> ev->accept();
> #endif
> }
> Index: WebKit/qt/Api/qwebpage_p.h
> ===================================================================
> --- WebKit/qt/Api/qwebpage_p.h (revision 52606)
> +++ WebKit/qt/Api/qwebpage_p.h (working copy)
> @@ -180,6 +180,7 @@
> QWidget* inspectorFrontend;
> QWebInspector* inspector;
> bool inspectorIsInternalOnly; // True if created through the Inspect
context menu action
> + Qt::DropAction m_lastDropAction;
>
> static bool drtRun;
> };
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog (revision 52663)
> +++ WebCore/ChangeLog (working copy)
> @@ -1,3 +1,21 @@
> +2009-12-30 Yael Aharon <yael.aharon at nokia.com>
> +
> + Reviewed by NOBODY (OOPS!).
> +
> + [Qt] Drag & drop layout tests fail even when run manually
> + https://bugs.webkit.org/show_bug.cgi?id=33055
> +
> + No new tests. Fix 3 layout tests when run manually.
> + fast/events/drag-and-drop.html
> + fast/events/drag-and-drop-dataTransfer-types-nocrash.html
> + fast/events/drag-and-drop-fire-drag-dragover.html
> + Running these tests in DRT will be fixed in 31332.
> +
> + * page/qt/DragControllerQt.cpp:
> + (WebCore::DragController::cleanupAfterSystemDrag):
> + Cleanup the drag operation if it failed to complete,
> + Otherwise, new drag operations will not be possible.
> +
> 2009-12-30 Laszlo Gombos <laszlo.1.gombos at nokia.com>
>
> Reviewed by Simon Hausmann.
> Index: WebCore/page/qt/DragControllerQt.cpp
> ===================================================================
> --- WebCore/page/qt/DragControllerQt.cpp (revision 52606)
> +++ WebCore/page/qt/DragControllerQt.cpp (working copy)
> @@ -66,6 +66,7 @@
>
> void DragController::cleanupAfterSystemDrag()
> {
> + dragEnded();
> }
>
> }
More information about the webkit-reviews
mailing list