[Webkit-unassigned] [Bug 44917] [chromium] Implement Readable/Writable versions of ChromiumDataObjectNew

[bugzilla-daemon at webkit.org]

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


Tony Chang <tony at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #66085|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #8 from Tony Chang <tony at chromium.org>  2010-08-31 13:46:31 PST ---
(From update of attachment 66085)
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).

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