[webkit-reviews] review granted: [Bug 28293] [Chromium] event.datatransfer.getdata("text/uri-list") treated the same as getdata("URL") : [Attachment 48010] patch - correct handling of "URL" vs. "text/uri-list" for Chromium, incl. new layout test

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has granted Roland Steiner
<rolandsteiner at chromium.org>'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 48010: patch - correct handling of "URL" vs. "text/uri-list" for
Chromium, incl. new layout test
https://bugs.webkit.org/attachment.cgi?id=48010&action=review

------- Additional Comments from David Levin <levin at chromium.org>
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
> +2010-02-03  Roland Steiner  <rolandsteiner at chromium.org>
> +
> +	   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...