Bug 28293 – [Chromium] event.datatransfer.getdata("text/uri-list") treated the same as getdata("URL")

• Component: Platform
• Status: RESOLVED FIXED

[Roland Steiner]

In Chromium, getData("text/uri-list") uses the same code path as getData("URL"), so always ever returns a single URL, even if multiple files are dragged.

[Roland Steiner]

Created attachment 34819 [details]
patch - handle "text/uri-list" (Windows only atm)

I changed the GetData functionality to also handle multiple files
(cf. also WebCore/platform/mac/ClipboardMac.mm)

Conversion of file paths to file URLs is implemented for Windows only at the moment. For other platforms the code will fallback to the current (single URL case) handling.

[David Levin]

Comment on attachment 34819 [details]
patch - handle "text/uri-list" (Windows only atm)

Please either add a Layout test or in the change log {explain why one cannot be added or indicate what tests already cover this}.

> Index: WebCore/platform/chromium/ClipboardChromium.cpp
> @@ -125,10 +125,27 @@
>      } else if (dataType == ClipboardDataTypeURL) {
>          // FIXME: Handle the cut/paste event.  This requires adding a new IPC
>          // message to get the URL from the clipboard directly.
> +        
> +        bool multipleURLs = (type.lower() == "text/uri-list")
> +                         && !m_dataObject->filenames.isEmpty();

Should only be a 4 four space indent beyond bool.

> +        // Multiple URL case
> +        if (multipleURLs) {
> +            size_t count = m_dataObject->filenames.size();
> +            for (size_t i = 0; i < count; ++i) {
> +                String url = filenameAsURL(m_dataObject->filenames[i]);
> +                if (!url.isEmpty()) {

if (url.isEmpty())
    continue;
Prefer "fail fast"

> Index: WebCore/platform/chromium/ClipboardChromium.h
> +        // This returns a String rather than a KURL as for the use in Clipboard
> +        // operations this is sufficient and easier (at least for the time being).

..."because" this is sufficent....

> Index: WebCore/platform/chromium/ClipboardChromiumWin.cpp
>  #include <shlwapi.h>
> +#include <WinInet.h>

sort case sensitive. so 'W' < 's'

> +    wchar_t buf[INTERNET_MAX_URL_LENGTH];

s/buf/buffer/
Use whole words.

> +    DWORD len = INTERNET_MAX_URL_LENGTH;

DWORD len = sizeof(buf )/ sizeof(buf[0]);

s/len/length/

> +    const wchar_t* chars = filename.charactersWithNullTermination();
> +    if (SUCCEEDED(::UrlCreateFromPathW(chars, buf, &len, 0))) {

No {} for single line block after if.

> +        return String(buf, len);
> +    }

[Roland Steiner]

Created attachment 38469 [details]
patch: handle "text/uri-list" (Windows only atm), updated

Changed patch according to review notes. Also removed a superfluous #include.

Note on layout tests: I actually wrote a test for the functionality that works fine on Safari Mac, but haven't included it here since Chromium doesn't (yet) support the necessary interfaces (and it felt strange to include a layout test and immediately disable it for the platform for which one submits the patch).

[David Levin]

(In reply to comment #3)

> Note on layout tests: I actually wrote a test for the functionality that works
> fine on Safari Mac, but haven't included it here since Chromium doesn't (yet)
> support the necessary interfaces 
Please include your layout test.  At least it will test webkit for this condition.

Also, I think the support for the interface is being added to chromium:

   http://code.google.com/p/chromium/issues/detail?id=19884

and if this doesn't cover it, then please consider adding support for the necessary interfaces to Chromium to test this.

[Roland Steiner]

Created attachment 38541 [details]
patch: layout test for getData("text/uri-list")

Added patch containing layout test as requested (separate file since it's not yet actually working in the platform targeted by the functionality patch).

Added the test to the "Skipped" list of all platforms except Mac.

[Roland Steiner]

Created attachment 38603 [details]
patch - handle "text/uri-list" (getData: Windows only atm)

Largely the same functionality as the previous patch, but cleaned up the code for better readability.

Also added corresponding handling of "text/uri-list" to clearData() and setData().

[Eric Seidel (no email)]

Comment on attachment 38541 [details]
patch: layout test for getData("text/uri-list")

http/tests/security seems like a strange place for these.  Unless you need something about this being a http tests, I've been putting other clipboard tests in edting/pasteboard.

[Eric Seidel (no email)]

Comment on attachment 38603 [details]
patch - handle "text/uri-list" (getData: Windows only atm)

I bet we could pretty easily guess at the implementations for linux and mac.

For mac:
        NSURL *url = [NSURL fileURLWithPath:string];
Except that that's a .cpp file, so we might have to use CF instead.  I guess the chromium mac people would know better.

[Roland Steiner]

Created attachment 38804 [details]
patch: layout test for getData("text/uri-list")

(cf. comment #7)

You're right - the reason why I put it into http/tests/security/clipboard was that I adapted the other layout test there and wanted to re-use the same resource files. However, those same file are present in editing/pasteboard as well, so I moved the test there as suggested.

In doing this I also had to adapt the test, since for reasons unknown to me the file URL returned in the new location was suddenly absolute rather than relative. However, relying on one or the other probably isn't a good idea (and won't work iwth absolute URLs anyway), so I changed the test to just check for "file://" at the beginning and the correct file name at the end.

[David Levin]

Comment on attachment 38804 [details]
patch: layout test for getData("text/uri-list")

r- for this: The skipped files still refer to the old test location.

Personally I would still prefer this layout tests to be with the patch for the bug.  Even though it currently doesn't test chromium due to the layout test controller support that needs to be added, it will test the patch when that support is added shortly.

Lastly, I would clean up the changelog some (e.g. "var" isn't a function).


As a note to other readers, the name of the test doesn't seem to fit the pattern of other test names, but with layout tests non-uniformity of this sort is good to actually test WebKit better.

[David Levin]

Comment on attachment 38603 [details]
patch - handle "text/uri-list" (getData: Windows only atm)

Sorry for taking so long to get back to this. Just a few last things to address.


> Index: WebCore/ChangeLog
> ===================================================================
> +        This patch does not include a layout test, since Chromium does not yet support the
> +        necessary interfaces.
> +        (cf. LayoutTests/http/tests/security/clipboard/clipboard-file-access.html)

Just include it. :)

> Index: WebCore/platform/chromium/ClipboardChromium.cpp
> ===================================================================
> --- WebCore/platform/chromium/ClipboardChromium.cpp	(revision 47748)
> +++ WebCore/platform/chromium/ClipboardChromium.cpp	(working copy)
> @@ -52,17 +52,24 @@
>  // We provide the IE clipboard types (URL and Text), and the clipboard types specified in the WHATWG Web Applications 1.0 draft
>  // see http://www.whatwg.org/specs/web-apps/current-work/ Section 6.3.5.3

It isn't part of your patch but I looked at this to validate this addition and the section reference is incorrect.  Perhaps instead of referring to a numerical section it could just give the spec url and a section title or something less mutable?

>  

> +enum ClipboardDataType { ClipboardDataTypeNone, 
> +                         ClipboardDataTypeURL, 
> +                         ClipboardDataTypeURIList, 
> +                         ClipboardDataTypeText };

This is formatted in an odd way. I would expect something like this:
enum ClipboardDataType {
    ClipboardDataTypeNone, 
    ClipboardDataTypeURL, 
    ClipboardDataTypeURIList, 
    ClipboardDataTypeText
};


> +    if (cleanType == "url")
> +        return ClipboardDataTypeURL;
> +    if (cleanType == "text/uri-list")
> +        return ClipboardDataTypeURIList;
> +    // includes two special cases for IE compatibility

Format comments like sentences (start with capital, end with period).
   // Includes two special cases for IE compatibility.

> +    switch (clipboardTypeFromMIMEType(type)) {
> +    case ClipboardDataTypeURL:
>          m_dataObject->url = KURL();
>          m_dataObject->urlTitle = "";
> +        break;
> +
> +    case ClipboardDataTypeURIList:
> +        m_dataObject->filenames.clear();
> +        break;

Why doesn't this fall through to ClipboardDataTypeURL since the handling of this type does?

> +
> +    case ClipboardDataTypeText:
> +        m_dataObject->plainText = "";
> +        break;

Add a 
       case ClipboardDataTypeNone:
           break;
to allow for turning on warnings that all enum values are handled.

Also, I would change all "break;" to "return;" and add a ASSERT_NOT_REACHED(); at the end of this function.



> +    case ClipboardDataTypeURL:
>          // FIXME: Handle the cut/paste event.  This requires adding a new IPC
>          // message to get the URL from the clipboard directly.
>          if (!m_dataObject->url.isEmpty()) {
>              success = true;
>              text = m_dataObject->url.string();
>          }
> +        break;

Add a 
       case ClipboardDataTypeNone:
           break;
to allow for turning on warnings that all enum values are handled.
>      }


> +    switch (clipboardTypeFromMIMEType(type)) {
> +    case ClipboardDataTypeURL:
>          m_dataObject->url = KURL(data);
>          return m_dataObject->url.isValid();

> +    case ClipboardDataTypeURIList:
> +        data.split(filenamesSeparator, m_dataObject->filenames);
> +        return true;
> +
> +    case ClipboardDataTypeText:
>          m_dataObject->plainText = data;
>          return true;

Add a 
       case ClipboardDataTypeNone:
           return false;
to allow for turning on warnings that all enum values are handled.

Also, I would add a ASSERT_NOT_REACHED(); at the end of this function.


> Index: WebCore/platform/chromium/ClipboardChromiumWin.cpp
> ===================================================================
> +String ClipboardChromium::filenameAsURL(String filename)
> +{
> +    if (filename.isEmpty())
> +        return String();
> +
> +    wchar_t buffer[INTERNET_MAX_URL_LENGTH];
> +    DWORD length = sizeof(buffer)/sizeof(buffer[0]);

Add spaces around /

[Erik Arvidsson]

Comment on attachment 38804 [details]
patch: layout test for getData("text/uri-list")

> Index: LayoutTests/editing/pasteboard/resources/dataTransfer-getData.js

> +dragTarget.addEventListener("dragentered", function() {
> +    event.dataTransfer.dropEffect = "copy";
> +    event.preventDefault();
> +}, false);

The event name is dragenter.

[Erik Arvidsson]

This does not seem to handle comments in the uri-list correctly.

See http://www.rfc-editor.org/rfc/rfc2483.txt for more info on the text/uri-list format.

The following should work:

var CRLF = '\r\n';
var SAMPLE_URI_LIST = [
  '# This is a comment',
  'http://webkit.org',
  'http://chromium.org'
].join(CRLF);

function handleDragStart(e) {
  e.dataTransfer.setData('text/uri-list', SAMPLE_URI_LIST);
}

function handleDrop(e) {
  var uris = e.dataTransfer.getData('text/uri-list').split(CRLF);
  // remove potential comments
  uris = uris.filter(function(uri) {
    return uri[0] !== '#';
  });

  assertEquals(2, uris.length);
  assertEquals('http://webkit.org', uris[0]);
  assertEquals('http://chromium.org', uris[1]);
}

I don't think we need to preserve the comments but I think it would be acceptable if we did.

[Roland Steiner]

Created attachment 48010 [details]
patch - correct handling of "URL" vs. "text/uri-list" for Chromium, incl. new layout test

New-and-improved version of the patch, including an all-new layout test.

This now also should handle comments in the URI list correctly.

The patch also tries to correctly handle clearData() in that file data is not deleted on a call to clearData() without arguments, as specified in the HTML5 spec.

Also added is a new layout test to verify the handling of getData & setData for "URL" vs. "text/uri-list". The test is skipped on all other platforms, since this functionality is not fully implemented there either. (The previous layout test is contained in the patch for the new issue 34512).

Nevertheless, I would also consider this Chromium patch just an interim patch, since it still doesn't support the HTML5 spec fully. Esp. it doesn't yet handle arbitrary MIME types in getData and setData.

[Adam Barth]

@levin: Any chance you could finish reviewing this patch?  I was going to dig into it, but you seem to have been thought it a couple times.

[David Levin]

Comment on attachment 48010 [details]
patch - correct handling of "URL" vs. "text/uri-list" for Chromium, incl. new layout test

Please consider a few small adjustments before landing as mentioned below.

There were a few places where whitespace was just changed for no reason. It would be nice to remove these changes.
  line 85 of WebCore/platform/chromium/ClipboardChromium.cpp
  line 311 of WebCore/platform/chromium/ClipboardChromium.cpp

> Index: WebCore/ChangeLog
> [phone]  Roland Steiner  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Bug 28293 -  [Chromium] event.datatransfer.getdata("text/uri-list") treated the same as getdata("URL")

I can't parse this easily. Do you mean "should be treated" instead of "treated"?

> +        (https://bugs.webkit.org/show_bug.cgi?id=28293)

No need for parenthesis here.


> Index: WebCore/platform/chromium/ChromiumDataObject.h
>      private:
> +        // URL and uri-list are linked -> should not be accessed individually.
Consider:
  URL and uri-list are linked, so they should not be accessed individually.

> Index: WebCore/platform/chromium/ClipboardChromium.cpp

> +// Per RFC 2483, line separator for "text/..." MIME types is CR-LF.

s/line/the line/

> +static char const * const textMIMETypeLineSeparator = "\r\n";

static char const* const textMIMETypeLineSeparator = "\r\n";

No space between the const and the *.


> +    case ClipboardDataTypeOther:
> +        // not yet implement, see https://bugs.webkit.org/show_bug.cgi?id=34410

Consider
// Not yet implemented, see https://bugs.webkit.org/show_bug.cgi?id=34410.



> +    case ClipboardDataTypeURL:
> +        // In case of a previous setData('text/uri-list'), setData() has already
> +        // prepared the 'url' member, so we can just retrieve it here.
> +        if (!m_dataObject->url.isEmpty()) {
> +            success = true;
> +            return m_dataObject->url.string();
> +        }
> +        // Otherwse check if we have a file that we could convert to a file:// URL.

s/Otherwse/Otherwise/



> +    case ClipboardDataTypeOther:
> +        // not yet implement, see https://bugs.webkit.org/show_bug.cgi?id=34410

Consider
// Not yet implemented, see https://bugs.webkit.org/show_bug.cgi?id=34410.

> +        // Copy the first valid URL into the 'url' member as well.
> +        // In case noe entry is a valid URL (i.e., remarks and URNs only), then we leave 'url' empty.

s/noe/no/


> +    case ClipboardDataTypeOther:
> +        // not yet implement, see https://bugs.webkit.org/show_bug.cgi?id=34410

As before...

[Roland Steiner]

Created attachment 49884 [details]
patch - fixed after mid-air collision, improved robustness, new test case

The previous r+'d patch had a mid-air collision with another patch that modified the same code and renamed member variables.

The new patch corrects the resulting conflicts. I also took the opportunity to apply the result of some discussions on the topic and changed the code so that it will accept \r\n as line separators in setData (as per the RFC) as well as just \n (e.g., FireFox also is fine with just \n), in order to add some robustness.

When creating uri-lists, however, this code will still adhere to the RFC and use \r\n as line separators.

Consequently I also added a new test case that checks for the case where just \n is used as line separator.

All in all, with above changes I felt that the new patch should undergo another review before committing.

[WebKit Commit Bot]

Comment on attachment 48010 [details]
patch - correct handling of "URL" vs. "text/uri-list" for Chromium, incl. new layout test

Cleared David Levin's review+ from obsolete attachment 48010 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[David Levin]

Comment on attachment 49884 [details]
patch - fixed after mid-air collision, improved robustness, new test case

Please address the comments below and submit it. (Please save any refinements for another patch.)


> Index: WebCore/ChangeLog
> [phone]  Roland Steiner  <[e-mail]>
> +
> +        Reviewed by David Levin.

This is slightly premature. It should remain as NOBODY (OOPS!). until right before it is submitted basically but I guess it is correct now.



> Index: WebCore/platform/chromium/ClipboardChromium.cpp

> -    if (dataType == ClipboardDataTypeText)
> +    case ClipboardDataTypeDownloadURL:
> +        m_dataObject->downloadMetadata = "";
> +        return;
> +        
> +    case ClipboardDataTypePlainText:
>          m_dataObject->plainText = "";
> +        return;
> +
> +    case ClipboardDataTypeOther:
> +        // Not yet implement, see https://bugs.webkit.org/show_bug.cgi?id=34410

Add "ed" like this:
  // Not yet implemented, see https://bugs.webkit.org/show_bug.cgi?id=34410


>  bool ClipboardChromium::setData(const String& type, const String& data)
...
> +    case ClipboardDataTypeURIList:
> +        m_dataObject->url = KURL();
> +        // Line separator is \r\n per RFC 2483 - however, for compatibility reasons
> +        // we also allow just \n here. 
> +        data.split('\n', m_dataObject->uriList);
> +        // Strip white space on all lines, including trailing \r from above split.
> +        // If this leaves a line empty, remove it completely.
> +        //
> +        // Also, copy the first valid URL into the 'url' member as well.
> +        // In case no entry is a valid URL (i.e., remarks only), then we leave 'url' empty.
> +        // I.e., in that case subsequent calls to getData("URL") will get an empty string.
> +        // This is in line with the HTML5 spec (see "The DragEvent and DataTransfer interfaces").
> +        for (size_t n = 0; n < m_dataObject->uriList.size(); /**/ ) {

/**/ is odd. Why not put "++n" here and then do n-- below if an item is removed from the uriList.
Also, n is an odd loop variable. Why not just use i?


> Index: LayoutTests/editing/pasteboard/dataTransfer-setData-getData-expected.txt

> ___________________________________________________________________
> Added: svn:executable

This should not be "svn:executable".Please remove this property.

[Roland Steiner]

Landed as rev. 55766 (build fix in 55767)

> > +        for (size_t n = 0; n < m_dataObject->uriList.size(); /**/ ) {
> 
> /**/ is odd. Why not put "++n" here and then do n-- below if an item is removed
> from the uriList.

Perhaps I'm too much of a stickler here, but since size_t is unsigned, n-- could cause a value underflow, that's why I preferred to avoid it here.