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

[bugzilla-daemon at webkit.org]

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

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

------- Additional Comments from Daniel Cheng <dcheng at chromium.org>
(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.