[Webkit-unassigned] [Bug 46559] [chromium] Refactor ChromiumDataObject to use getters/setters.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=46559


Tony Chang <tony at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #68817|review?                     |review-
               Flag|                            |




--- Comment #2 from Tony Chang <tony at chromium.org>  2010-09-27 10:48:50 PST ---
(From update of attachment 68817)
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'.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.