[Webkit-unassigned] [Bug 62838] [Qt] [WK2] Add drag and drop support

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=62838


Andreas Kling <kling at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #97975|review?                     |review-
               Flag|                            |




--- Comment #10 from Andreas Kling <kling at webkit.org>  2011-06-21 12:10:17 PST ---
(From update of attachment 97975)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.