Bug 46559 – [chromium] Refactor ChromiumDataObject to use getters/setters.

• Component: Platform
• Status: RESOLVED FIXED

[Daniel Cheng]

This is an intermediate step to converting ChromiumDataObject to use callbacks to the browser to retrieve data. Special handling for URL and text/uri-list has also been removed; they now treat their data argument as just an opaque blob of text.

[Daniel Cheng]

Created attachment 68817 [details]
Patch

Marking as R? so EWS runs this patch. I am not sure if removing special URL handling will require updating other platforms; if it does, I will split out removing special URL handling into a separate patch.

[Tony Chang]

Comment on attachment 68817 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68817&action=review

> LayoutTests/ChangeLog:14
> +        * editing/pasteboard/dataTransfer-setData-getData-expected.txt:
> +        * editing/pasteboard/script-tests/dataTransfer-setData-getData.js:

Can you explain what you changed in the test?

> LayoutTests/editing/pasteboard/script-tests/dataTransfer-setData-getData.js:93
> -    test("text/uri-list", ["http://test.com", "http://check.com"], 
> -         "text/uri-list", ["http://test.com", "http://check.com"]);
> +    test("text/uri-list", "http://test.com", 
> +         "text/uri-list", "http://test.com");

Why were the tests that take a list of URLs removed?  Shouldn't we still verify that \r\n or comments gets passed through properly?

> LayoutTests/editing/pasteboard/script-tests/dataTransfer-setData-getData.js:97
> -    test("text/uri-list", ["http://test.com", "http://check.com"], 
> -         "URL", ["http://test.com"]);
> +    test("text/uri-list", "http://test.com", 
> +         "URL", "http://test.com");

This seems like an important test to keep too (setting more than one urls and getting one back).

> WebCore/platform/chromium/ChromiumDataObject.cpp:108
> +    if (!m_url.isEmpty()) {
> +        results.add("URL");
> +        results.add("text/uri-list");
> +    }

The previous code in ClipboardChromium::types() filtered out file URLs.  Why doesn't that happen in the new code?

> WebCore/platform/chromium/ChromiumDataObject.cpp:121
> +    if (type == "text/plain") {

The various types should be constants somewhere.

> WebCore/platform/chromium/ChromiumDataObject.h:50
> -        static PassRefPtr<ChromiumDataObject> create()
> +      static PassRefPtr<ChromiumDataObject> create(Clipboard::ClipboardType clipboardType)

Nit: Indention looks wrong.

> WebCore/platform/chromium/ClipboardChromium.cpp:60
> -    // Includes two special cases for IE compatibility.
> -    if (cleanType == "text" || cleanType == "text/plain" || cleanType.startsWith("text/plain;"))
> -        return ClipboardDataTypePlainText;
> +    if (cleanType == "text")
> +        return "text/plain";

We seemed to have lost the "text/plain;" case.  Is that intentional?  Should we have a test that checks for this?

> WebCore/platform/chromium/ClipboardChromium.cpp:111
> +    return m_dataObject->getData(normalizeType(type), success);

I thought getData("URL") is supposed to only return the first URL in uri-list.  It looks like normalizing causes us to lose that distinction.

> WebKit/chromium/src/WebDragData.cpp:118
>  bool WebDragData::hasFileNames() const
>  {

Nit: We should probably try to rename this method (in a different set of changes) to containsFilenames since it seems like 'contains' is used more than 'has'.

[Tony Chang]

Thanks for doing this refactoring into a class.  I like the direction this is going, but there seems to be some behavioral changes included.  I think it's fine to change the underlying data representation for URLs and uri-lists to Strings, but some of the changes seem to do more than just that.

[Daniel Cheng]

Created attachment 68990 [details]
Patch

(In reply to comment #2)
> (From update of attachment 68817 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68817&action=review
> 
> > LayoutTests/ChangeLog:14
> > +        * editing/pasteboard/dataTransfer-setData-getData-expected.txt:
> > +        * editing/pasteboard/script-tests/dataTransfer-setData-getData.js:
> 
> Can you explain what you changed in the test?
> 

Done.

> > LayoutTests/editing/pasteboard/script-tests/dataTransfer-setData-getData.js:93
> > -    test("text/uri-list", ["http://test.com", "http://check.com"], 
> > -         "text/uri-list", ["http://test.com", "http://check.com"]);
> > +    test("text/uri-list", "http://test.com", 
> > +         "text/uri-list", "http://test.com");
> 
> Why were the tests that take a list of URLs removed?  Shouldn't we still verify that \r\n or comments gets passed through properly?

This was part of the change--removing special handling for any data handled by dataTransfer. Since we plan on adding more data types in the future, some of which can be arbitrarily user-defined, I'm uncomfortable with the idea that we're validating some types and potentially setting different data than what the user passed in and not validating other types. The various ports vary in how they implement this feature as well:

GTK: I might be reading it incorrectly, but it seems to me that setting URL or text/uri-list only manipulate the uri-list component. If you call setData("text/uri-list") and then call getData("URL"), there will be no data returned.
Mac: Will only return one URL for URL and possibly multiple URLs for text/uri-list. There is no special handling in setData() for splitting a text/uri-list into its component URLs.
QT: Does not appear to handle text/uri-list or URL specially in any way.
Windows: Handles text/uri-list and URL identically.

I am OK with reverting this behavior, but it seems far simpler to just not validate--and easier to make it consistent among the various ports as well.

> 
> > LayoutTests/editing/pasteboard/script-tests/dataTransfer-setData-getData.js:97
> > -    test("text/uri-list", ["http://test.com", "http://check.com"], 
> > -         "URL", ["http://test.com"]);
> > +    test("text/uri-list", "http://test.com", 
> > +         "URL", "http://test.com");
> 
> This seems like an important test to keep too (setting more than one urls and getting one back).

See above.

> 
> > WebCore/platform/chromium/ChromiumDataObject.cpp:108
> > +    if (!m_url.isEmpty()) {
> > +        results.add("URL");
> > +        results.add("text/uri-list");
> > +    }
> 
> The previous code in ClipboardChromium::types() filtered out file URLs.  Why doesn't that happen in the new code?
> 

This actually hasn't been necessary for a long time. I made another change in Chromium/WebKit that made the original hack unnecessary.

> > WebCore/platform/chromium/ChromiumDataObject.cpp:121
> > +    if (type == "text/plain") {
> 
> The various types should be constants somewhere.

Any suggestions on an appropriate location?

> 
> > WebCore/platform/chromium/ChromiumDataObject.h:50
> > -        static PassRefPtr<ChromiumDataObject> create()
> > +      static PassRefPtr<ChromiumDataObject> create(Clipboard::ClipboardType clipboardType)
> 
> Nit: Indention looks wrong.

Done. I wonder why the style checker didn't catch this.

> 
> > WebCore/platform/chromium/ClipboardChromium.cpp:60
> > -    // Includes two special cases for IE compatibility.
> > -    if (cleanType == "text" || cleanType == "text/plain" || cleanType.startsWith("text/plain;"))
> > -        return ClipboardDataTypePlainText;
> > +    if (cleanType == "text")
> > +        return "text/plain";
> 
> We seemed to have lost the "text/plain;" case.  Is that intentional?  Should we have a test that checks for this?
> 

It was an unintentional change. I added it back in, though I'm not entirely sure why we do this to begin with.

> > WebCore/platform/chromium/ClipboardChromium.cpp:111
> > +    return m_dataObject->getData(normalizeType(type), success);
> 
> I thought getData("URL") is supposed to only return the first URL in uri-list.  It looks like normalizing causes us to lose that distinction.
> 

A strict reading of the spec indicates that URL is simply supposed to be an alias for text/uri-list.

> > WebKit/chromium/src/WebDragData.cpp:118
> >  bool WebDragData::hasFileNames() const
> >  {
> 
> Nit: We should probably try to rename this method (in a different set of changes) to containsFilenames since it seems like 'contains' is used more than 'has'.

I'll make another patch after this one.

[Tony Chang]

http://www.whatwg.org/specs/web-apps/current-work/multipage/dnd.html#datatransfer
"If the format (after conversion to lowercase) is "url", then the data associated with the "text/uri-list" format must be parsed as appropriate for text/uri-list data, and the first URL from the list must be returned. If there is no data with that format, or if there is but it has no URLs, then the method must return the empty string. [RFC2483]"

This seems pretty clear to me that we have to have special code for handling getting of URL and text/uri-list.  If the data is not properly formatted, we have to return an empty string.  Am I reading the wrong spec?

[Daniel Cheng]

(In reply to comment #5)
> http://www.whatwg.org/specs/web-apps/current-work/multipage/dnd.html#datatransfer
> "If the format (after conversion to lowercase) is "url", then the data associated with the "text/uri-list" format must be parsed as appropriate for text/uri-list data, and the first URL from the list must be returned. If there is no data with that format, or if there is but it has no URLs, then the method must return the empty string. [RFC2483]"
> 
> This seems pretty clear to me that we have to have special code for handling getting of URL and text/uri-list.  If the data is not properly formatted, we have to return an empty string.  Am I reading the wrong spec?

I see. I don't know why I missed that section of the spec earlier.

[Daniel Cheng]

Created attachment 69135 [details]
Patch

[Daniel Cheng]

Created attachment 69136 [details]
Patch

I left out the ClipboardChromium changes by accident.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/chromium/ClipboardChromium.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 15 files


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

[Daniel Cheng]

Created attachment 69142 [details]
Patch

[Tony Chang]

Comment on attachment 69142 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69142&action=review

Almost done...  r- because of the WebDragData.h change.

> WebCore/ChangeLog:13
> +        Test: editing/pasteboard/dataTransfer-setData-getData.html

You might want to just say that this change is covered by existing tests since this test is no longer modified.

> WebCore/platform/chromium/ChromiumDataObject.h:112
> +        // These two are linked.
> +        KURL m_url;
> +        Vector<String> m_uriList;

Nit: Can you expand on this comment a bit?  'linked' is kind of vague.

> WebCore/platform/chromium/ClipboardChromium.cpp:57
> -static ClipboardDataType clipboardTypeFromMIMEType(const String& type)
> +static String normalizeType(const String& type)

FWIW, I preferred having ClipboardDataType since it's clear whether or not normalization has occurred yet.  I don't feel strongly about this, but it would be nice if we had a test case for the "text/plain;" normalization (I suspect on Windows if you're copying text in a different encoding, you might hit this).

> WebCore/platform/chromium/DragDataChromium.cpp:59
> +    if (containsURL(DoNotConvertFilenames)) {

Nit: Would it be clearer to write this as m_platformDragData->types().contains(mimeTypeURL)?

> WebKit/chromium/public/WebDragData.h:69
> -    WEBKIT_API WebURL url() const;
> +    WEBKIT_API WebString url() const;

Won't this break callers?

> WebKit/chromium/src/WebDragData.cpp:71
> +    return m_private->getData("text/uri-list", ignoredSuccess);

Should this file use the constants in ClipboardMimeTypes.h?  Other cases below.  For this specific case, should this use 'URL' instead of 'text/uri-list'?

[Daniel Cheng]

Created attachment 69312 [details]
Patch

(In reply to comment #11)
> (From update of attachment 69142 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69142&action=review
> 
> Almost done...  r- because of the WebDragData.h change.
> 
> > WebCore/ChangeLog:13
> > +        Test: editing/pasteboard/dataTransfer-setData-getData.html
> 
> You might want to just say that this change is covered by existing tests since this test is no longer modified.
> 

Done.

> > WebCore/platform/chromium/ChromiumDataObject.h:112
> > +        // These two are linked.
> > +        KURL m_url;
> > +        Vector<String> m_uriList;
> 
> Nit: Can you expand on this comment a bit?  'linked' is kind of vague.
> 

Done.

> > WebCore/platform/chromium/ClipboardChromium.cpp:57
> > -static ClipboardDataType clipboardTypeFromMIMEType(const String& type)
> > +static String normalizeType(const String& type)
> 
> FWIW, I preferred having ClipboardDataType since it's clear whether or not normalization has occurred yet.  I don't feel strongly about this, but it would be nice if we had a test case for the "text/plain;" normalization (I suspect on Windows if you're copying text in a different encoding, you might hit this).
> 

In the future, we need to handle arbitrary strings. We could still use ClipboardDataType, but that means we'd need a getData() that takes a ClipboardDataType and another that takes a string.

> > WebCore/platform/chromium/DragDataChromium.cpp:59
> > +    if (containsURL(DoNotConvertFilenames)) {
> 
> Nit: Would it be clearer to write this as m_platformDragData->types().contains(mimeTypeURL)?
> 

Done.

> > WebKit/chromium/public/WebDragData.h:69
> > -    WEBKIT_API WebURL url() const;
> > +    WEBKIT_API WebString url() const;
> 
> Won't this break callers?
> 

If I use WebURL, there's no implicit conversion from String to WebURL so the build will fail in WebKit. But on the Chromium side, WebString can automatically be converted to GURL, so it works.

> > WebKit/chromium/src/WebDragData.cpp:71
> > +    return m_private->getData("text/uri-list", ignoredSuccess);
> 
> Should this file use the constants in ClipboardMimeTypes.h?  Other cases below.  For this specific case, should this use 'URL' instead of 'text/uri-list'?

Done.

[Tony Chang]

(In reply to comment #12)
> > FWIW, I preferred having ClipboardDataType since it's clear whether or not normalization has occurred yet.  I don't feel strongly about this, but it would be nice if we had a test case for the "text/plain;" normalization (I suspect on Windows if you're copying text in a different encoding, you might hit this).
> > 
> 
> In the future, we need to handle arbitrary strings. We could still use ClipboardDataType, but that means we'd need a getData() that takes a ClipboardDataType and another that takes a string.

Ah, that's a good point.  Maybe we should have setData and getData both normalize the string so callers don't have to?  That's how it used to work, right?  I'm just worried about callers forgetting to normalize.

[WebKit Commit Bot]

Comment on attachment 69312 [details]
Patch

Clearing flags on attachment: 69312

Committed r68807: <http://trac.webkit.org/changeset/68807>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.