[webkit-reviews] review denied: [Bug 28293] [Chromium] event.datatransfer.getdata("text/uri-list") treated the same as getdata("URL") : [Attachment 38603] patch - handle "text/uri-list" (getData: Windows only atm)

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Roland Steiner
<rolandsteiner at google.com>'s request for review:
Bug 28293: [Chromium] event.datatransfer.getdata("text/uri-list") treated the
same as getdata("URL")
https://bugs.webkit.org/show_bug.cgi?id=28293

Attachment 38603: patch - handle "text/uri-list" (getData: Windows only atm)
https://bugs.webkit.org/attachment.cgi?id=38603&action=review

------- Additional Comments from David Levin <levin at chromium.org>
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 /