[Webkit-unassigned] [Bug 55115] Add support for DataTransferItems

[bugzilla-daemon at webkit.org]

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


David Levin <levin at chromium.org> changed:

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




--- Comment #12 from David Levin <levin at chromium.org>  2011-03-07 16:38:19 PST ---
(From update of attachment 84992)
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?

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