[webkit-reviews] review granted: [Bug 62838] [Qt] [WK2] Add drag and drop support : [Attachment 98058] Patch.

[bugzilla-daemon at webkit.org]

Andreas Kling <kling at webkit.org> has granted Yael <yael.aharon at nokia.com>'s
request for review:
Bug 62838: [Qt] [WK2] Add drag and drop support
https://bugs.webkit.org/show_bug.cgi?id=62838

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

------- Additional Comments from Andreas Kling <kling at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=98058&action=review

r=us
But please fix these tiny little things..

> Source/WebCore/platform/DragData.h:127
> +#if PLATFORM(QT)
> +    DragData() { }

Should have a comment that this is only used by WebKit2 IPC.

> Source/WebCore/platform/DragData.h:129
> +    DragData& operator =(const DragData &data)

OMG CODING STYLE

> Source/WebKit2/Shared/qt/ArgumentCodersQt.cpp:31
> +using namespace std;

We don't need this.

> Source/WebKit2/Shared/qt/ArgumentCodersQt.cpp:55
> +	   size_t size = bytes.size();

This shadows the earlier 'size'.
You don't need the local anyway, same with 'data'.

> Source/WebKit2/Shared/qt/ArgumentCodersQt.cpp:79
> +    decoder->decodeBool(hasPlatformData);

We should return false if this call fails.

> Source/WebKit2/Shared/qt/ArgumentCodersQt.cpp:96
> +    DragData data(mimeData, clientPosition, globalPosition,
(DragOperation)sourceOperationMask, (DragApplicationFlags)flags);
> +    dragData = data;

dragData = DragData(...);

> Source/WebKit2/UIProcess/API/qt/qwkpage.cpp:94
> +static inline DragOperation dropActionToDragOp(Qt::DropActions actions)

dropAction_s_ToDragOperation

> Source/WebKit2/UIProcess/API/qt/qwkpage.cpp:108
> +static inline Qt::DropAction dragOpToDropAction(unsigned action)

dragOperationToDropAction
Argument should be called dragOperation, not action. :)

> Source/WebKit2/UIProcess/API/qt/qwkpage.cpp:122
> +static inline Qt::DropActions dragOpToDropActions(unsigned actions)

dragOperationToDropActions

> Source/WebKit2/UIProcess/API/qt/qwkpage.cpp:502
> +    DragData dragData(ev->mimeData(), QPointF(ev->pos()).toPoint(),
QCursor::pos(), dropActionToDragOp(ev->possibleActions()));

QPointF(ev->pos()).toPoint() -> ev->pos().toPoint(), repeated for each
occurrence.

> Source/WebKit2/UIProcess/API/qt/qwkpage.cpp:980
> +    if (view->scene()->views().size() < 1)
> +	   return;
> +

Covered by ownerWidget()

> Source/WebKit2/WebProcess/WebCoreSupport/qt/WebDragClientQt.cpp:52
> +    QRectF rect(0, 0, p.width(), p.height());
> +    graphicsContext->platformContext()->drawPixmap(rect, p, rect);

graphicsContext...->drawPixmap(0, 0, p);

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1704
> +	   return;
> +    }

Should probably delete the dragData's platform data here.