Bug 62838 – [Qt] [WK2] Add drag and drop support

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Yael]

Qt WebKit 2 should support drag and drop.

[Yael]

Created attachment 97536 [details]
WIP

This is work in progress, comments are welcome.
This patch could be simplified once https://bugs.webkit.org/show_bug.cgi?id=60621 is fixed.
One known problem in this patch is that if we drag a selection, the drag image comes to the UI process as a black square.
If we drag an image, the drag image looks ok.

[Kenneth Rohde Christiansen]

Comment on attachment 97536 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=97536&action=review

> Source/WebCore/platform/DragData.h:127
> +    DragData() {}

Need a space in there :-)

> Source/WebKit2/Shared/qt/ArgumentCodersQt.cpp:33
> +typedef HashMap<String , Vector<uint8_t> > MimeDataHashMap;

shouldnt MIME be uppercase?

> Source/WebKit2/Shared/qt/ArgumentCodersQt.cpp:47
> +    encoder->encodeBool(true);

What is this for? /* platformData */ ? hasPlatformData?

> Source/WebKit2/Shared/qt/ArgumentCodersQt.cpp:53
> +        Vector<uint8_t> vdata;

vdata? vector of data? maybe we can find a better name

> Source/WebKit2/Shared/qt/ArgumentCodersQt.cpp:55
> +        uint8_t* data = (uint8_t*)(bytes.data());

Why are you not using C++ casts?

> Source/WebKit2/UIProcess/API/qt/qwkpage.cpp:898
> +    QWidget* widget = view->scene()->views()[0];

Didnt we have a convenience method for getting the right view? or maybe that is only on our branch?

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1696
> +    if (data)
> +        delete data;

no, that if is not needed in C++

[Yael]

(In reply to comment #2)
> (From update of attachment 97536 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97536&action=review
> 
Thank you for the review.
I will address your comments in the next patch.

[Yael]

Created attachment 97920 [details]
Patch.

It seems that the transparency issue I had was solved (perhaps by r88797) so this patch is ready for review.
Manually tested all combinations of LayoutTests/fast/events/drag-and-drop.html and they all pass. 
As an extra bonus, with this patch we can drop files into MiniBrowser.

[WebKit Review Bot]

Attachment 97920 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit2/UIProcess/API/qt/qwkpage.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/WebPageProxy.h:448:  The parameter name "dragData" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/qt/qwkpage_p.h:75:  The parameter name "dragData" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/Shared/qt/ArgumentCodersQt.cpp:38:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/WebProcess/WebPage/WebPage.h:382:  The parameter name "dragData" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/Shared/qt/ArgumentCodersQt.h:29:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/Shared/qt/ArgumentCodersQt.h:33:  The parameter name "encoder" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/Shared/qt/ArgumentCodersQt.h:33:  The parameter name "data" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/Shared/qt/ArgumentCodersQt.h:34:  The parameter name "decoder" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/Shared/qt/ArgumentCodersQt.h:34:  The parameter name "data" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/PageClient.h:107:  The parameter name "dragData" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 11 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yael]

Created attachment 97963 [details]
Patch.

Forgot to run check-webkit-style before :(

[WebKit Review Bot]

Comment on attachment 97963 [details]
Patch.

Attachment 97963 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8909945

New failing tests:
fast/events/drag-and-drop.html

[WebKit Review Bot]

Created attachment 97967 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Yael]

Created attachment 97975 [details]
Patch.

Remove unrelated changes. Sorry:(

[Andreas Kling]

Comment on attachment 97975 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=97975&action=review

> Source/WebKit2/Shared/qt/ArgumentCodersQt.cpp:30
> +

Extra newline here.

> Source/WebKit2/Shared/qt/ArgumentCodersQt.cpp:73
> +    decoder->decode(clientPosition);
> +    decoder->decode(globalPosition);
> +    decoder->decode(sourceOperationMask);
> +    decoder->decode(flags);

If any of the decode() calls in this function fails, we should return 'false' immediately.

> Source/WebKit2/Shared/qt/ArgumentCodersQt.cpp:75
> +    bool b;

Should be called 'hasPlatformData' no?

> Source/WebKit2/Shared/qt/ArgumentCodersQt.cpp:82
> +        bool success =  decoder->decode(map);
> +        if (!success)
> +            return false;

No need for 'success' to be in a variable.

> Source/WebKit2/UIProcess/PageClient.h:107
> +    virtual void startDrag(const WebCore::DragData&, PassRefPtr<ShareableBitmap>) = 0;

Let's have a name for the bitmap argument here, i.e 'dragImage'.

> Source/WebKit2/UIProcess/WebPageProxy.h:448
> +    void startDrag(const WebCore::DragData&, const ShareableBitmap::Handle&);

Let's have a name for the bitmap argument here, i.e 'dragImage'.

> Source/WebKit2/UIProcess/API/qt/qwkpage.cpp:493
> +#endif

Do we need an #else block here with Q_UNUSED(ev)?

> Source/WebKit2/UIProcess/API/qt/qwkpage.cpp:501
> +#endif

Same question.

> Source/WebKit2/UIProcess/API/qt/qwkpage.cpp:512
> +#endif

Again.

> Source/WebKit2/UIProcess/API/qt/qwkpage.cpp:523
> +#endif

Wohoo!

> Source/WebKit2/UIProcess/API/qt/qwkpage.cpp:961
> +    if (view->scene()->views().size() < 1)
> +        return;
> +
> +    QWidget* widget = view->scene()->views()[0];

Don't we have a helper function for this (getting the first view on a scene)? If not, we should probably add one.

> Source/WebKit2/UIProcess/API/qt/qwkpage.cpp:962
> +    if (widget) {

Use early return here to avoid nesting.

> Source/WebKit2/UIProcess/API/qt/qwkpage_p.h:75
> +    virtual void startDrag(const WebCore::DragData&, PassRefPtr<ShareableBitmap>);

Let's have a name for the second argument here, i.e 'dragImage'.

> Source/WebKit2/WebProcess/WebCoreSupport/qt/WebDragClientQt.cpp:41
> +static PassRefPtr<ShareableBitmap> convertImageToBitmap(QPixmap* pixmap)

Let's call this convertQPixmapToShareableBitmap() for maximum awesomeness.

> Source/WebKit2/WebProcess/WebCoreSupport/qt/WebDragClientQt.cpp:43
> +    // FIXME: We need this hack until https://bugs.webkit.org/show_bug.cgi?id=60621 is fixed

It's not clear from the comment what the actual hack here is.

> Source/WebKit2/WebProcess/WebCoreSupport/qt/WebDragClientQt.cpp:69
> +#endif

Do we need an #else block here with some Q_UNUSED()?

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1728
> +    QMimeData* data = const_cast<QMimeData*>(dragData.platformData());
> +    delete data;

This should have a comment explaining that the WebCore::DragData doesn't kill its platformData() due to the ownership model.

> Source/WebKit2/WebProcess/WebPage/WebPage.h:61
> +#if PLATFORM(QT)
> +#include "ArgumentCodersQt.h"
> +#endif

Is this include necessary here?

[Yael]

(In reply to comment #10)
> (From update of attachment 97975 [details])
Thank you for reviewing.
> > Source/WebKit2/UIProcess/API/qt/qwkpage.cpp:961
> > +    if (view->scene()->views().size() < 1)
> > +        return;
> > +
> > +    QWidget* widget = view->scene()->views()[0];
> 
> Don't we have a helper function for this (getting the first view on a scene)? If not, we should probably add one.
> 
Created https://bugs.webkit.org/show_bug.cgi?id=63095.

> > Source/WebKit2/WebProcess/WebPage/WebPage.h:61
> > +#if PLATFORM(QT)
> > +#include "ArgumentCodersQt.h"
> > +#endif
> 
> Is this include necessary here?
It does not compile otherwise.

[Yael]

Created attachment 98058 [details]
Patch.

Address comment #10.

[Andreas Kling]

Comment on attachment 98058 [details]
Patch.

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.

[Yael]

Created attachment 98359 [details]
Patch.

Address comment #13 and fix an issue that dragEnded was called prematurely. (Re-reviewed by Kling off-line).

[WebKit Review Bot]

Comment on attachment 98359 [details]
Patch.

Clearing flags on attachment: 98359

Committed r89582: <http://trac.webkit.org/changeset/89582>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.