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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Sep 27 10:48:50 PDT 2010


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 68817: Patch
https://bugs.webkit.org/attachment.cgi?id=68817&action=review

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

> LayoutTests/ChangeLog:14
> +	   * editing/pasteboard/dataTransfer-setData-getData-expected.txt:
> +	   * editing/pasteboard/script-tests/dataTransfer-setData-getData.js:

Can you explain what you changed in the test?

>
LayoutTests/editing/pasteboard/script-tests/dataTransfer-setData-getData.js:93
> -    test("text/uri-list", ["http://test.com", "http://check.com"], 
> -	    "text/uri-list", ["http://test.com", "http://check.com"]);
> +    test("text/uri-list", "http://test.com", 
> +	    "text/uri-list", "http://test.com");

Why were the tests that take a list of URLs removed?  Shouldn't we still verify
that \r\n or comments gets passed through properly?

>
LayoutTests/editing/pasteboard/script-tests/dataTransfer-setData-getData.js:97
> -    test("text/uri-list", ["http://test.com", "http://check.com"], 
> -	    "URL", ["http://test.com"]);
> +    test("text/uri-list", "http://test.com", 
> +	    "URL", "http://test.com");

This seems like an important test to keep too (setting more than one urls and
getting one back).

> WebCore/platform/chromium/ChromiumDataObject.cpp:108
> +    if (!m_url.isEmpty()) {
> +	   results.add("URL");
> +	   results.add("text/uri-list");
> +    }

The previous code in ClipboardChromium::types() filtered out file URLs.  Why
doesn't that happen in the new code?

> WebCore/platform/chromium/ChromiumDataObject.cpp:121
> +    if (type == "text/plain") {

The various types should be constants somewhere.

> WebCore/platform/chromium/ChromiumDataObject.h:50
> -	   static PassRefPtr<ChromiumDataObject> create()
> +	 static PassRefPtr<ChromiumDataObject> create(Clipboard::ClipboardType
clipboardType)

Nit: Indention looks wrong.

> WebCore/platform/chromium/ClipboardChromium.cpp:60
> -    // Includes two special cases for IE compatibility.
> -    if (cleanType == "text" || cleanType == "text/plain" ||
cleanType.startsWith("text/plain;"))
> -	   return ClipboardDataTypePlainText;
> +    if (cleanType == "text")
> +	   return "text/plain";

We seemed to have lost the "text/plain;" case.	Is that intentional?  Should we
have a test that checks for this?

> WebCore/platform/chromium/ClipboardChromium.cpp:111
> +    return m_dataObject->getData(normalizeType(type), success);

I thought getData("URL") is supposed to only return the first URL in uri-list. 
It looks like normalizing causes us to lose that distinction.

> WebKit/chromium/src/WebDragData.cpp:118
>  bool WebDragData::hasFileNames() const
>  {

Nit: We should probably try to rename this method (in a different set of
changes) to containsFilenames since it seems like 'contains' is used more than
'has'.


More information about the webkit-reviews mailing list