[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


Daniel Cheng <dcheng at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #66085|0                           |1
        is obsolete|                            |
  Attachment #66102|                            |review?, commit-queue?
               Flag|                            |




--- Comment #9 from Daniel Cheng <dcheng at chromium.org>  2010-08-31 14:16:48 PST ---
Created an attachment (id=66102)
 --> (https://bugs.webkit.org/attachment.cgi?id=66102)
Patch

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

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