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

[bugzilla-daemon at webkit.org]

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





--- Comment #14 from Daniel Cheng <dcheng at chromium.org>  2011-03-07 18:00:30 PST ---
(In reply to comment #12)
> (From update of attachment 84992 [details])
> 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?
> 

Done.

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

Done.

> > Source/WebCore/dom/DataTransferItems.h:36
> > +#include "DataTransferItem.h"
> 
> Add fed decl instead?
> 

Done.

> > Source/WebCore/dom/DataTransferItems.h:37
> > +#include "ExceptionCode.h"
> 
> Seems typical to put in a typedef instead.
> 

Done.

> > Source/WebCore/dom/DataTransferItems.h:39
> > +#include "PlatformString.h"
> 
> Use fwd decl (fwiw, I think it is wtf/text/WTFString.h now.)
> 

Done.

> > Source/WebCore/dom/DataTransferItems.h:41
> > +#include <wtf/RefCounted.h>
> 
> Why is this needed?
> 

Base template for DataTransferItems.

> > 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.

Done.

> 
> > 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.
> 

I stole this from DOMFileSystem.h. Are they doing it wrong?

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

Done.

> 
> > 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.
> 

Done.

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

Done.

> > Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:52
> > +        return "";
> 
> Why "" as opposed to String() ?
> 
> > Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:59
> > +        return "";
> 
> Ditto.
> 

I've seen it both ways in the code.

> > 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
> 

Done.

> > Source/WebCore/platform/chromium/DataTransferItemChromium.h:56
> > +    ScriptExecutionContext* m_context;
> 
> I would put in a fwd decl for ScriptExecutionContext.
> 

Done.

> > 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.
> 

Done.

> > Source/WebCore/platform/chromium/DataTransferItemsChromium.cpp:68
> > +    if (index >= length())
> 
> Why doesn't this cause an exception code to be set?
> 

The spec doesn't say to.

> > Source/WebCore/platform/chromium/DataTransferItemsChromium.cpp:84
> > +    if (m_owner->policy() != ClipboardWritable)
> 
> Why doesn't this cause an exception code to be set?
> 

Strict interpretation of the spec.

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

Done.

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

Done.

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

Done for ExceptionCode, though see my previous comment about WTFString.

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

Done.

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