[webkit-reviews] review denied: [Bug 31090] [Chromium] Add DownloadURL format to Chromium clipboard. : [Attachment 42533] Proposed Patch

[bugzilla-daemon at webkit.org]

Dmitry Titov <dimich at chromium.org> has denied Jian Li <jianli at chromium.org>'s
request for review:
Bug 31090: [Chromium] Add DownloadURL format to Chromium clipboard.
https://bugs.webkit.org/show_bug.cgi?id=31090

Attachment 42533: Proposed Patch
https://bugs.webkit.org/attachment.cgi?id=42533&action=review

------- Additional Comments from Dmitry Titov <dimich at chromium.org>
> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> +	   Bug 31090 - [Chromium] Add DownloadURL format to Chromium clipboard.

> +	   https://bugs.webkit.org/show_bug.cgi?id=31090

I think ChangeLog could contain more info not only on what is done but also
why.
A link to a spec or mail list discussion is good to have, especially since the
bug itself does not have it.

Also, need to say why no test.

> +	   bool needToDownloadUrl;

Better name (or a comment) would be good here. It is unclear if the url itself
should be downloaded?
Maybe 'useUrlToDownloadData'?

> +	   ChromiumDataObject() : needToDownloadUrl(false) {}

Also should be set to false in ChromiumDataObject::clear()

> diff --git a/WebCore/platform/chromium/ClipboardChromium.cpp
b/WebCore/platform/chromium/ClipboardChromium.cpp
>  // 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

This comment is not true anymore, should be updated.

Also, I think the suggestion from Tony is good. Please consider KURL instead of
bool.

r- for now, but close.