Bug 44917 – [chromium] Implement Readable/Writable versions of ChromiumDataObjectNew

• Component: Platform
• Status: RESOLVED FIXED

[Daniel Cheng]

ReadableDataObject calls the Chromium IPCs to retrieve any applicable data. WritableDataObject buffers data in an internal map. It currently has to cheat for cut/copy due to a limitation in the WebCore interface; it calls ClipboardWriteData directly. This situation will hopefully be addressed in a future patch.

[Daniel Cheng]

Created attachment 65988 [details]
Patch

[Daniel Cheng]

Created attachment 65991 [details]
Patch

[Tony Chang]

Comment on attachment 65991 [details]
Patch

> diff --git a/WebCore/platform/chromium/ReadableDataObject.cpp b/WebCore/platform/chromium/ReadableDataObject.cpp
> + * Copyright (c) 2008, 2009, Google Inc. All rights reserved.

Nit: 2010

> +static void ensureTypeCacheInitialized(bool isForDragging,
> +                                       bool& isTypeCacheInitialized,
> +                                       HashSet<String>& types,
> +                                       bool& containsFilenames)

It looks like all the calls to this pass in the same member variables.  Can we just make it a member function?

> diff --git a/WebCore/platform/chromium/ReadableDataObject.h b/WebCore/platform/chromium/ReadableDataObject.h
> + * Copyright (c) 2008, 2009, Google Inc. All rights reserved.

Nit: 2010

> +    // To avoid making a lot of IPC calls for each drag event, we cache some
> +    // values in the renderer.
> +    // TODO: Is this actually the right place to do the caching? Isn't the data

WebKit style is to use FIXME rather than TODO.

> diff --git a/WebCore/platform/chromium/WritableDataObject.cpp b/WebCore/platform/chromium/WritableDataObject.cpp
> + * Copyright (c) 2008, 2009, Google Inc. All rights reserved.

Nit: 2010

> +// TODO: Verify it 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.

FIXME, also, "Verify it the" sounds funny.  I'm also not sure I understand this comment.  Are you saying you're not sure if you need these methods at all?

> +void WritableDataObject::clearAllExceptFiles()
> +{
> +    // TODO: The spec does not provide a way to populate FileList currently. In
> +    // fact, the spec explicitly states that dragging files can only happen from
> +    // outside a browsing context.

FIXME

> +void WritableDataObject::clear()

What is the difference between clear() and clearData()?  Why do we need both?

> +HashSet<String> WritableDataObject::types() const
> +{

Would it be better for the caller to pass in a HasSet<String> to avoid a copy?

> +String WritableDataObject::getData(const String& type, bool& succeeded) const
> +{
> +    // TODO: For pasting, we need to read directly from the pasteboard. Right
> +    // now, Clipboard::getData doesn't allow reads when the clipboard is
> +    // writable, but we have may to address this in the future.

FIXME

> diff --git a/WebCore/platform/chromium/WritableDataObject.h b/WebCore/platform/chromium/WritableDataObject.h
> + * Copyright (c) 2008, 2009, Google Inc. All rights reserved.

Nit: 2010

> +    // Special accessors for URL and HTML since they carry additional
> +    // metadata.

I don't think you need to duplicate the comments from the parent class.

> +private:
> +    WritableDataObject(bool isForDragging);

Nit: explicit

[Daniel Cheng]

Created attachment 66076 [details]
Patch

(In reply to comment #3)
> (From update of attachment 65991 [details])
> > diff --git a/WebCore/platform/chromium/ReadableDataObject.cpp b/WebCore/platform/chromium/ReadableDataObject.cpp
> > + * Copyright (c) 2008, 2009, Google Inc. All rights reserved.
> 
> Nit: 2010
> 

Done.

> > +static void ensureTypeCacheInitialized(bool isForDragging,
> > +                                       bool& isTypeCacheInitialized,
> > +                                       HashSet<String>& types,
> > +                                       bool& containsFilenames)
> 
> It looks like all the calls to this pass in the same member variables.  Can we just make it a member function?
> 

Done.

> > diff --git a/WebCore/platform/chromium/ReadableDataObject.h b/WebCore/platform/chromium/ReadableDataObject.h
> > + * Copyright (c) 2008, 2009, Google Inc. All rights reserved.
> 
> Nit: 2010
> 

Done.

> > +    // To avoid making a lot of IPC calls for each drag event, we cache some
> > +    // values in the renderer.
> > +    // TODO: Is this actually the right place to do the caching? Isn't the data
> 
> WebKit style is to use FIXME rather than TODO.
> 

Removed the comment, as I don't think there's a better place to do it anyway.

> > diff --git a/WebCore/platform/chromium/WritableDataObject.cpp b/WebCore/platform/chromium/WritableDataObject.cpp
> > + * Copyright (c) 2008, 2009, Google Inc. All rights reserved.
> 
> Nit: 2010
> 

Done.

> > +// TODO: Verify it 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.
> 
> FIXME, also, "Verify it the" sounds funny.  I'm also not sure I understand this comment.  Are you saying you're not sure if you need these methods at all?

Sorry, it was a typo. It should have been 'verify if'. And yes, I'm not sure if an implementation is even needed for these methods.

> 
> > +void WritableDataObject::clearAllExceptFiles()
> > +{
> > +    // TODO: The spec does not provide a way to populate FileList currently. In
> > +    // fact, the spec explicitly states that dragging files can only happen from
> > +    // outside a browsing context.
> 
> FIXME
> 

Done.

> > +void WritableDataObject::clear()
> 
> What is the difference between clear() and clearData()?  Why do we need both?
> 

clear() clears all the data and clearData() clears only one type of data. It was originally inherited from Clipboard as well; I've renamed clear() to clearAll().

> > +HashSet<String> WritableDataObject::types() const
> > +{
> 
> Would it be better for the caller to pass in a HasSet<String> to avoid a copy?
> 

I opted to inherit most of the signatures from Clipboard.

> > +String WritableDataObject::getData(const String& type, bool& succeeded) const
> > +{
> > +    // TODO: For pasting, we need to read directly from the pasteboard. Right
> > +    // now, Clipboard::getData doesn't allow reads when the clipboard is
> > +    // writable, but we have may to address this in the future.
> 
> FIXME
> 

Done.

> > diff --git a/WebCore/platform/chromium/WritableDataObject.h b/WebCore/platform/chromium/WritableDataObject.h
> > + * Copyright (c) 2008, 2009, Google Inc. All rights reserved.
> 
> Nit: 2010

Done.

> 
> > +    // Special accessors for URL and HTML since they carry additional
> > +    // metadata.
> 
> I don't think you need to duplicate the comments from the parent class.
> 

Done.

> > +private:
> > +    WritableDataObject(bool isForDragging);
> 
> Nit: explicit

Done.

[Daniel Cheng]

Created attachment 66084 [details]
Patch

Per the discussion, remove the interface since RDO/WDO implement two mutually exclusive set of functions.

[WebKit Review Bot]

Attachment 66084 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/chromium/WritableDataObject.h:48:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/platform/chromium/WritableDataObject.h:48:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Cheng]

Created attachment 66085 [details]
Patch

Remove two spaces that snuck in.

[Tony Chang]

Comment on attachment 66085 [details]
Patch

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

[Daniel Cheng]

Created attachment 66102 [details]
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.

[Daniel Cheng]

Created attachment 66104 [details]
Patch

Post the right patch this time.

[Tony Chang]

Comment on attachment 66104 [details]
Patch

> +++ b/WebCore/platform/chromium/ClipboardMimeTypes.h
> +const char* const textPlainType = "text/plain";
> +const char* const textHtmlType = "text/html";
> +const char* const textUriListType = "text/uri-list";

This is the wrong way to make shared strings.  By putting the data in the header, the value will be copied into each .cpp file that includes this file.  An example of chromium code that does this is in base/base_switches.*.  Alternately, I would stick these values as static variables off an existing class.

[Daniel Cheng]

Created attachment 66133 [details]
Patch

I was in a hurry and just depending on the (undocumented?) behavior that the linker pools identical strings. Updated the patch to only define the constants in one place.

[WebKit Commit Bot]

Comment on attachment 66133 [details]
Patch

Clearing flags on attachment: 66133

Committed r66567: <http://trac.webkit.org/changeset/66567>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.