[webkit-reviews] review granted: [Bug 76367] Add DataTransferItems support for drag-and-drop'ed files and texts : [Attachment 123095] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has granted Kinuko Yasuda
<kinuko at chromium.org>'s request for review:
Bug 76367: Add DataTransferItems support for drag-and-drop'ed files and texts
https://bugs.webkit.org/show_bug.cgi?id=76367

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

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=123095&action=review


> Source/WebCore/bindings/v8/custom/V8BlobCustom.cpp:41
> +#include "V8Attr.h"
> +#include "V8Binding.h"
> +#include "V8BindingState.h"
> +#include "V8Blob.h"
> +#include "V8DirectoryEntry.h"
> +#include "V8File.h"
> +#include "V8Proxy.h"
> +#include <wtf/RefPtr.h>

Do we need to include all of these files?

> Source/WebCore/platform/chromium/ClipboardChromium.cpp:360
> +    if (items.size() > 0U && !isStorageUpdated())

Nit: !items.isEmpty()

> Source/WebCore/platform/chromium/ClipboardChromium.cpp:393
> +bool ClipboardChromium::isStorageUpdated() const

Nit: Maybe name this storageHasUpdated().

> Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:80
> +    : DataTransferItem(owner, DataTransferItem::kindFile, file.get() ?
file->type() : "")

Nit: "" -> String()

> Source/WebCore/platform/chromium/DataTransferItemListChromium.cpp:91
> +    RefPtr<ChromiumDataObject> dataObject =
static_cast<ClipboardChromium*>(m_owner.get())->dataObject();

Nit: Maybe add a helper method for
"static_cast<ClipboardChromium*>(m_owner.get())"?

> Source/WebCore/platform/chromium/DataTransferItemListChromium.h:52
> +    // Overrides.

Nit: I would name the idl file these are from.

> Source/WebCore/platform/qt/DataTransferItemQt.cpp:49
> +    return adoptRef(new DataTransferItemQt(owner, context,
DataTransferItemQta::InternalSource, kindString, type, data));

DataTransferItemQta? Is the 'a' intentional?  Actually, can you just use
InternalSource?

> Source/WebCore/platform/qt/DataTransferItemQt.cpp:56
> +    return adoptRef(new DataTransferItemQt(owner, context,
DataTransferItemQta::InternalSource, file));

Ditto.

> Source/WebCore/platform/qt/DataTransferItemQt.cpp:85
> +    : DataTransferItem(owner, DataTransferItem::kindFile, file.get() ?
file->type() : "")

Nit: "" -> String()