[webkit-reviews] review denied: [Bug 28293] [Chromium] event.datatransfer.getdata("text/uri-list") treated the same as getdata("URL") : [Attachment 34819] patch - handle "text/uri-list" (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 34819: patch - handle "text/uri-list" (Windows only atm)
https://bugs.webkit.org/attachment.cgi?id=34819&action=review

------- Additional Comments from David Levin <levin at chromium.org>
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);
> +    }