[webkit-reviews] review denied: [Bug 46559] [chromium] Refactor ChromiumDataObject to use getters/setters. : [Attachment 69142] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Daniel Cheng <dcheng at chromium.org>'s
request for review:
Bug 46559: [chromium] Refactor ChromiumDataObject to use getters/setters.
https://bugs.webkit.org/show_bug.cgi?id=46559

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

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=69142&action=review

Almost done...	r- because of the WebDragData.h change.

> WebCore/ChangeLog:13
> +	   Test: editing/pasteboard/dataTransfer-setData-getData.html

You might want to just say that this change is covered by existing tests since
this test is no longer modified.

> WebCore/platform/chromium/ChromiumDataObject.h:112
> +	   // These two are linked.
> +	   KURL m_url;
> +	   Vector<String> m_uriList;

Nit: Can you expand on this comment a bit?  'linked' is kind of vague.

> WebCore/platform/chromium/ClipboardChromium.cpp:57
> -static ClipboardDataType clipboardTypeFromMIMEType(const String& type)
> +static String normalizeType(const String& type)

FWIW, I preferred having ClipboardDataType since it's clear whether or not
normalization has occurred yet.  I don't feel strongly about this, but it would
be nice if we had a test case for the "text/plain;" normalization (I suspect on
Windows if you're copying text in a different encoding, you might hit this).

> WebCore/platform/chromium/DragDataChromium.cpp:59
> +    if (containsURL(DoNotConvertFilenames)) {

Nit: Would it be clearer to write this as
m_platformDragData->types().contains(mimeTypeURL)?

> WebKit/chromium/public/WebDragData.h:69
> -    WEBKIT_API WebURL url() const;
> +    WEBKIT_API WebString url() const;

Won't this break callers?

> WebKit/chromium/src/WebDragData.cpp:71
> +    return m_private->getData("text/uri-list", ignoredSuccess);

Should this file use the constants in ClipboardMimeTypes.h?  Other cases below.
 For this specific case, should this use 'URL' instead of 'text/uri-list'?