Bug 46943 – [chromium] getData('text/uri-list') should return the same thing that was passed to setData('text/uri-list')

• Component: Platform
• Status: RESOLVED FIXED

[Daniel Cheng]

Right now, we strip out comments and potentially change line endings. It's probably nice to have it return the same thing that was passed in (the comments might be useful, for instance).

How do other browsers handle it?
IE8: Calling setData('URL') doesn't work. The only time you can set URLs in IE seems to be when you're dragging anchors--and it's not you setting it, it's implicitly set by the browser.
IE9: Same as IE8 as far as I can tell. I could be wrong though.
Firefox: Seems to handle URL and text/uri-list as separate types. Also allows anything to be set in the 'URL' member.
Safari: My test isn't working in Safari at the moment, but as far as I can tell, it probably returns what you set for text/uri-list (and URL as well, though that's probably a bug).

As I interpret the standard, getData('URL') should never return an invalid URL.

[Daniel Cheng]

Created attachment 69713 [details]
Patch

[Tony Chang]

Comment on attachment 69713 [details]
Patch

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

The code looks OK but I would like to see more tests.

> LayoutTests/editing/pasteboard/script-tests/dataTransfer-setData-getData.js:103
> +    test("text/uri-list", "# comment\r\nhttp://test.com\r\nhttp://check.com", 
> +         "URL", "http://test.com/");

Can you add some more test cases like setting URL and getting text/uri-list?  What about setting a text/uri-list with comments but no urls and getting URL?  What about setting a URL with multiple urls and getting with URL?

[Daniel Cheng]

Created attachment 69763 [details]
Patch

[Tony Chang]

Comment on attachment 69763 [details]
Patch

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

> LayoutTests/editing/pasteboard/script-tests/dataTransfer-setData-getData.js:119
> +    test("text/uri-list", "# comment\r\n# comment 2\r\n# comment 3", 
> +         "URL", null);

The spec says this should return an empty string, not null:
http://dev.w3.org/html5/spec/dnd.html
"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."

> WebCore/platform/chromium/ChromiumDataObject.cpp:145
>          success = !m_url.isEmpty();
>          return m_url.string();

Maybe the fix is as simple as
  success = !m_url.isEmpty() || !m_uriList.isEmpty();
?

[Daniel Cheng]

(In reply to comment #4)
> (From update of attachment 69763 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69763&action=review
> 
> > LayoutTests/editing/pasteboard/script-tests/dataTransfer-setData-getData.js:119
> > +    test("text/uri-list", "# comment\r\n# comment 2\r\n# comment 3", 
> > +         "URL", null);
> 
> The spec says this should return an empty string, not null:
> http://dev.w3.org/html5/spec/dnd.html
> "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."
> 
> > WebCore/platform/chromium/ChromiumDataObject.cpp:145
> >          success = !m_url.isEmpty();
> >          return m_url.string();
> 
> Maybe the fix is as simple as
>   success = !m_url.isEmpty() || !m_uriList.isEmpty();
> ?

It probably makes sense to address this in another patch (since getData("text") has the same problem as well then). Maybe we should just delete the empty cases from this patch before landing it and fix the undefined behavior in a separate patch and add test cases there?

[Tony Chang]

Fixing in a follow up patch sounds fine since it sounds like this isn't a regression.  Instead of deleting the test case, keep the test case with the correct value:
test("text/uri-list", "# comment\r\n# comment 2\r\n# comment 3", 
     "URL", "");

But check in an -expected.txt file with the word FAIL in it.  When we fix the code, we can check in the new -expected.txt file.  We do this for other tests.

[Daniel Cheng]

Created attachment 69815 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 69815 [details]
Patch

Clearing flags on attachment: 69815

Committed r69138: <http://trac.webkit.org/changeset/69138>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.