[webkit-reviews] review denied: [Bug 55115] Add support for DataTransferItems : [Attachment 84992] Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Daniel Cheng
<dcheng at chromium.org>'s request for review:
Bug 55115: Add support for DataTransferItems
https://bugs.webkit.org/show_bug.cgi?id=55115

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

------- Additional Comments from David Levin <levin at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=84992&action=review

Initial pass of comments.

> Source/WebCore/dom/DataTransferItem.h:36
> +#include "PlatformString.h"

fwd decl?

> Source/WebCore/dom/DataTransferItem.h:37
> +#include "StringCallback.h"

fwd decl?

> Source/WebCore/dom/DataTransferItems.h:36
> +#include "DataTransferItem.h"

Add fed decl instead?

> Source/WebCore/dom/DataTransferItems.h:37
> +#include "ExceptionCode.h"

Seems typical to put in a typedef instead.

> Source/WebCore/dom/DataTransferItems.h:39
> +#include "PlatformString.h"

Use fwd decl (fwiw, I think it is wtf/text/WTFString.h now.)

> Source/WebCore/dom/DataTransferItems.h:41
> +#include <wtf/RefCounted.h>

Why is this needed?

> Source/WebCore/dom/StringCallback.h:36
> +

Should include WTFString.h

> Source/WebCore/dom/StringCallback.h:45
> +    void scheduleCallback(ScriptExecutionContext* context, const String&
data)

Consider moving to cpp file and changing the ScriptExecutionContext include to
a fwd decl.

> Source/WebCore/dom/StringCallback.h:53
> +	   DispatchCallbackTask(PassRefPtr<StringCallback> callback, const
String& data)

Hide the constructor and expose a static create method which returns a
PassOwnPtr.

> Source/WebCore/dom/StringCallback.h:54
> +	       : m_callback(callback), m_data(data)

separate lines.

> Source/WebCore/dom/StringCallback.h:60
> +	       m_callback->handleEvent(m_data);

This isn't inlined (due to being virtual) so put it in a cpp file.

> Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:45
> +    : m_owner(owner), m_context(context), m_kind(kindString), m_type(type),
m_data(data)

separate lines.

> Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:52
> +	   return "";

Why "" as opposed to String() ?

> Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:59
> +	   return "";

Ditto.

> Source/WebCore/platform/chromium/DataTransferItemChromium.h:36
> +#include "Clipboard.h"

a fwd decl should suffice if you declare a destructor and put it in the cpp
file.

> Source/WebCore/platform/chromium/DataTransferItemChromium.h:40
> +

include wtfstring.h

> Source/WebCore/platform/chromium/DataTransferItemChromium.h:56
> +    ScriptExecutionContext* m_context;

I would put in a fwd decl for ScriptExecutionContext.

> Source/WebCore/platform/chromium/DataTransferItemsChromium.cpp:43
> +DataTransferItemsChromium::DataTransferItemsChromium(PassRefPtr<Clipboard>
owner, ScriptExecutionContext* context) : m_owner(owner), m_context(context)

separate lines for the initializers.

> Source/WebCore/platform/chromium/DataTransferItemsChromium.cpp:68
> +    if (index >= length())

Why doesn't this cause an exception code to be set?

> Source/WebCore/platform/chromium/DataTransferItemsChromium.cpp:84
> +    if (m_owner->policy() != ClipboardWritable)

Why doesn't this cause an exception code to be set?

> Source/WebCore/platform/chromium/DataTransferItemsChromium.cpp:88
> +	 if (m_items[i]->type() == type && m_items[i]->kind() ==
DataTransferItem::kindString) {

indentation should be 4 spaces.

> Source/WebCore/platform/chromium/DataTransferItemsChromium.h:36
> +#include "Clipboard.h"

fwd decl.

> Source/WebCore/platform/chromium/DataTransferItemsChromium.h:55
> +    virtual void add(const String& data, const String& type,
ExceptionCode&);

typedef for ExceptionCode?

fwd decl for String?

> Source/WebCore/platform/chromium/DataTransferItemsChromium.h:58
> +    DataTransferItemsChromium(PassRefPtr<Clipboard>,
ScriptExecutionContext*);

fwd decl for ScriptExecutionContext?