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

[bugzilla-daemon at webkit.org]

Andreas Kling <kling at webkit.org> has denied 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 97975: Patch.
https://bugs.webkit.org/attachment.cgi?id=97975&action=review

------- Additional Comments from Andreas Kling <kling at webkit.org>
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?