[webkit-reviews] review denied: [Bug 44917] [chromium] Implement Readable/Writable versions of ChromiumDataObjectNew : [Attachment 66085] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Daniel Cheng <dcheng at chromium.org>'s
request for review:
Bug 44917: [chromium] Implement Readable/Writable versions of
ChromiumDataObjectNew
https://bugs.webkit.org/show_bug.cgi?id=44917

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

------- Additional Comments from Tony Chang <tony at chromium.org>
I think you need to remove the include for ChromiumDataObjectNew.h.  Did you
try compiling?

> diff --git a/WebCore/platform/chromium/ReadableDataObject.cpp
b/WebCore/platform/chromium/ReadableDataObject.cpp
> +ReadableDataObject::ReadableDataObject(bool isForDragging)
> +    : m_isForDragging(isForDragging), m_isTypeCacheInitialized(false)

What about m_containsFilenames?

> diff --git a/WebCore/platform/chromium/ReadableDataObject.h
b/WebCore/platform/chromium/ReadableDataObject.h
> +    // To avoid making a lot of IPC calls for each drag event, we cache some

> +    // values in the renderer.
> +    mutable HashSet<String> m_types;
> +    mutable bool m_containsFilenames;
> +    mutable bool m_isTypeCacheInitialized;

I'm not sure there's a lot of benefit to having all const methods and making
these all mutable, but this seems ok.

> diff --git a/WebCore/platform/chromium/WritableDataObject.cpp
b/WebCore/platform/chromium/WritableDataObject.cpp
> +// FIXME: Verify if the implementations for read-only methods are needed.
Per the
> +// code currently in ClipboardChromium, it's actually unnecessary, but we
should
> +// double check if this is intended by the spec, especially since it doesn't

> +// really address when the clipboard object should be readable and when it
> +// should be writable.

I think this comment is now obsolete.

> +void WritableDataObject::clearAll()
> +{
> +    m_dataMap.clear();
> +    m_urlTitle = "";
> +    m_htmlBaseURL = KURL();

Should we clear m_fileExtension here?

> +void WritableDataObject::setURL(const String& url, const String& title)
> +{
> +    setData("text/uri-list", url);

Can we make constants for "text/uri-list" and "text/html", etc?  I'm surprised
"text/html" doesn't already exist somewhere.

> diff --git a/WebCore/platform/chromium/WritableDataObject.h
b/WebCore/platform/chromium/WritableDataObject.h
> +    HashMap<String, String> m_dataMap;

Should this be a map from String to SharedBuffer?  It doesn't make sense for a
String to hold binary data (since String is for unicode bytes).