[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 #68817|0                           |1
        is obsolete|                            |
  Attachment #68990|                            |review?
               Flag|                            |




--- Comment #4 from Daniel Cheng <dcheng at chromium.org>  2010-09-27 16:47:37 PST ---
Created an attachment (id=68990)
 --> (https://bugs.webkit.org/attachment.cgi?id=68990)
Patch

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

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