[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 #69142|review?                     |review-
               Flag|                            |




--- Comment #11 from Tony Chang <tony at chromium.org>  2010-09-28 18:04:51 PST ---
(From update of attachment 69142)
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'?

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