[webkit-reviews] review requested: [Bug 46559] [chromium] Refactor ChromiumDataObject to use getters/setters. : [Attachment 68990] 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 68990: Patch
https://bugs.webkit.org/attachment.cgi?id=68990&action=review

------- Additional Comments from Daniel Cheng <dcheng at chromium.org>
(In reply to comment #2)
> (From update of attachment 68817 [details])
> 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?
> 

Done.

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

This was part of the change--removing special handling for any data handled by
dataTransfer. Since we plan on adding more data types in the future, some of
which can be arbitrarily user-defined, I'm uncomfortable with the idea that
we're validating some types and potentially setting different data than what
the user passed in and not validating other types. The various ports vary in
how they implement this feature as well:

GTK: I might be reading it incorrectly, but it seems to me that setting URL or
text/uri-list only manipulate the uri-list component. If you call
setData("text/uri-list") and then call getData("URL"), there will be no data
returned.
Mac: Will only return one URL for URL and possibly multiple URLs for
text/uri-list. There is no special handling in setData() for splitting a
text/uri-list into its component URLs.
QT: Does not appear to handle text/uri-list or URL specially in any way.
Windows: Handles text/uri-list and URL identically.

I am OK with reverting this behavior, but it seems far simpler to just not
validate--and easier to make it consistent among the various ports as well.

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

See above.

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

This actually hasn't been necessary for a long time. I made another change in
Chromium/WebKit that made the original hack unnecessary.

> > WebCore/platform/chromium/ChromiumDataObject.cpp:121
> > +	 if (type == "text/plain") {
> 
> The various types should be constants somewhere.

Any suggestions on an appropriate location?

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

Done. I wonder why the style checker didn't catch this.

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

It was an unintentional change. I added it back in, though I'm not entirely
sure why we do this to begin with.

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

A strict reading of the spec indicates that URL is simply supposed to be an
alias for text/uri-list.

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

I'll make another patch after this one.