[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


Daniel Cheng <dcheng at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #69142|0                           |1
        is obsolete|                            |
  Attachment #69312|                            |review?, commit-queue?
               Flag|                            |




--- Comment #12 from Daniel Cheng <dcheng at chromium.org>  2010-09-29 22:37:01 PST ---
Created an attachment (id=69312)
 --> (https://bugs.webkit.org/attachment.cgi?id=69312)
Patch

(In reply to comment #11)
> (From update of attachment 69142 [details])
> 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.
> 

Done.

> > 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.
> 

Done.

> > 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).
> 

In the future, we need to handle arbitrary strings. We could still use ClipboardDataType, but that means we'd need a getData() that takes a ClipboardDataType and another that takes a string.

> > WebCore/platform/chromium/DragDataChromium.cpp:59
> > +    if (containsURL(DoNotConvertFilenames)) {
> 
> Nit: Would it be clearer to write this as m_platformDragData->types().contains(mimeTypeURL)?
> 

Done.

> > WebKit/chromium/public/WebDragData.h:69
> > -    WEBKIT_API WebURL url() const;
> > +    WEBKIT_API WebString url() const;
> 
> Won't this break callers?
> 

If I use WebURL, there's no implicit conversion from String to WebURL so the build will fail in WebKit. But on the Chromium side, WebString can automatically be converted to GURL, so it works.

> > 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'?

Done.

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