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

[bugzilla-daemon at webkit.org]

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

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

------- Additional Comments from Daniel Cheng <dcheng at chromium.org>
(In reply to comment #8)
> (From update of attachment 66085 [details])
> 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?

ensureTypeCacheIsInitialized() will properly set it, but it is probably a good
idea to set a default value here anyway.

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

I believe this is because Clipboard has some methods that are marked const, so
we can only call const methods on its members.

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

Removed.

> > +void WritableDataObject::clearAll()
> > +{
> > +	 m_dataMap.clear();
> > +	 m_urlTitle = "";
> > +	 m_htmlBaseURL = KURL();
> 
> Should we clear m_fileExtension here?
> 

Done.

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

Any suggestions on a better location for these constants? I did a quick search
and didn't find much, though I did find many hardcoded "text/html" strings.

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

The current API only allows for DOM strings. Once this part was checked in, I
was planning on updating Clipboard.idl to allow manipulation of binary types
and add in binary handling then. It will make the individual patches a bit
smaller as well.