Bug 30416

*** Bug 55837 has been marked as a duplicate of this bug. ***

*** Bug 73968 has been marked as a duplicate of this bug. ***

This is covered by IETC Test: http://samples.msdn.microsoft.com/ietestcenter/html5/dragdrop/types_attribute.htm

And http://www.whatwg.org/specs/web-apps/current-work/#the-datatransfer-interface

Created attachment 126878 [details] Patch

Attachment 126878 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1 Source/WebCore/dom/Clipboard.h:36: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 26 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 126878 [details] Patch Attachment 126878 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11522007

Comment on attachment 126878 [details] Patch Attachment 126878 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11523020

Comment on attachment 126878 [details] Patch Attachment 126878 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11518075 New failing tests: http/tests/security/clipboard/clipboard-file-access.html http/tests/local/fileapi/send-dragged-file.html http/tests/local/fileapi/file-last-modified.html http/tests/local/fileapi/send-sliced-dragged-file.html

Comment on attachment 126878 [details] Patch Attachment 126878 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11515140

Created attachment 127014 [details] Patch

Comment on attachment 127014 [details] Patch Attachment 127014 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11523349

Created attachment 127027 [details] Patch Fixed compile on all platforms

Comment on attachment 127027 [details] Patch I would like to review this before you land. When I scanned earlier, I had doubts. I'll look shortly.

Comment on attachment 127027 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127027&action=review I strongly recommend reading http://www.webkit.org/coding/RefPtr.html if you haven't already. I think Señor Levin might also want to take another look. > Source/WebCore/dom/Clipboard.cpp:151 > + return types()->contains(type); It's sad that we're creating a new object every time. :( I guess we were doing that before, but it wasn't as obvious. Also, this is no longer null-safe. If platform Clipboard implementations ever return NULL this will crash. :( > Source/WebCore/platform/chromium/ChromiumDataObject.cpp:115 > + for (HashSet<String>::const_iterator it = hashedResults.begin(); > + it != hashedResults.end(); > + ++it) { WebKit has no 80c rule. Also, I'm not sure if we use { } for single line for statements. > Source/WebCore/platform/chromium/ChromiumDataObject.cpp:118 > + return results; This should be results.release(); > Source/WebCore/platform/chromium/ClipboardChromium.cpp:306 > return results; results.release(); > Source/WebCore/platform/efl/ClipboardEfl.cpp:84 > + return 0; This will cause crashes based on other changes you made. :( > Source/WebCore/platform/win/ClipboardWin.cpp:489 > +static void addMimeTypesForFormat(PassRefPtr<DOMStringList> results, const FORMATETC& format) This should not be a PassRefPtr. This function does not take ownership of the List. RefPtr& or just a raw ptr is fine. > LayoutTests/ChangeLog:8 > + Update tests to use contains() instead of indexOf(). Lame that it doensn't have indexOf. I wonder if this will break sites... http://www.w3.org/TR/DOM-Level-3-Core/core.html#DOMStringList > LayoutTests/ChangeLog:9 > + I would like to see a test of clipboard.types == clipboard.types. I suspect they're !=. I suspect they were != before (since it looks like our code created a new array for each call, but it would be nice to confirm, and have an expectation for that behavior. (It would also be nice to know what FF does.)

Created attachment 127084 [details] Patch

Comment on attachment 127084 [details] Patch Seems OK. I would like to see you comment on the possible compatibility concern in the ChangeLog. There is also now a risk that we could be adding types twice. DOMStringList is ordered and will not remove duplicates like the HashSet would have. Do you know if we care about the order at all? Do we match our previous ordering behavior? (I know these seem like small things, but it's best to think about them now, instead of when we have strange compatibility troubles.)

(In reply to comment #16) > (From update of attachment 127027 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=127027&action=review > > I strongly recommend reading http://www.webkit.org/coding/RefPtr.html if you haven't already. I think Señor Levin might also want to take another look. > I've fixed all the RefPtr and null issues. Whether or not this is the way we should fix it remains TBD. > > Source/WebCore/dom/Clipboard.cpp:151 > > + return types()->contains(type); > > It's sad that we're creating a new object every time. :( I guess we were doing that before, but it wasn't as obvious. > > Also, this is no longer null-safe. If platform Clipboard implementations ever return NULL this will crash. :( > I've addressed this for now by changing all implementations to return a DOMStringList. > > Source/WebCore/platform/chromium/ChromiumDataObject.cpp:115 > > + for (HashSet<String>::const_iterator it = hashedResults.begin(); > > + it != hashedResults.end(); > > + ++it) { > > WebKit has no 80c rule. Also, I'm not sure if we use { } for single line for statements. > I'm surprised the style checker didn't catch this. Fixed. > > Source/WebCore/platform/chromium/ChromiumDataObject.cpp:118 > > + return results; > > This should be results.release(); > > > Source/WebCore/platform/chromium/ClipboardChromium.cpp:306 > > return results; > > results.release(); > > > Source/WebCore/platform/efl/ClipboardEfl.cpp:84 > > + return 0; > > This will cause crashes based on other changes you made. :( > > > Source/WebCore/platform/win/ClipboardWin.cpp:489 > > +static void addMimeTypesForFormat(PassRefPtr<DOMStringList> results, const FORMATETC& format) > > This should not be a PassRefPtr. This function does not take ownership of the List. RefPtr& or just a raw ptr is fine. > > > LayoutTests/ChangeLog:8 > > + Update tests to use contains() instead of indexOf(). > > Lame that it doensn't have indexOf. I wonder if this will break sites... > FF doesn't support indexOf, so websites that want to work cross-browser /should/ be using if (types.indexOf) { } else if (types.contains) { }. > http://www.w3.org/TR/DOM-Level-3-Core/core.html#DOMStringList > > > LayoutTests/ChangeLog:9 > > + > > I would like to see a test of clipboard.types == clipboard.types. I suspect they're !=. I suspect they were != before (since it looks like our code created a new array for each call, but it would be nice to confirm, and have an expectation for that behavior. (It would also be nice to know what FF does.) Hmm. From the spec: The types attribute must return a live DOMStringList giving the strings that the following steps would produce. The same object must be returned each time. Firefox currently returns a different object each time. Fixing this is probably a separate patch, but I don't see a way to do it without coming into conflict with layering issues (to be fair, Clipboard and its various implementations are already a pretty big mess in this regard). If we return a HashSet<String>, it'd be impossible to make it live since the Clipboard object won't know how to tell the glue object to update its view. Note that the "items" and "files" attributes currently suffer from this problem as well, since they aren't live and always return a copy. In order to implement a live view like some other things like document.getElementsByname, I think we'd have to create TypesDOMStringList, keep a cached pointer to it in Clipboard once it's created, and make sure all Clipboard implementations update it as appropriate.

Comment on attachment 127084 [details] Patch (In reply to comment #18) > (From update of attachment 127084 [details]) > Seems OK. I would like to see you comment on the possible compatibility concern in the ChangeLog. There is also now a risk that we could be adding types twice. DOMStringList is ordered and will not remove duplicates like the HashSet would have. Do you know if we care about the order at all? Do we match our previous ordering behavior? (I know these seem like small things, but it's best to think about them now, instead of when we have strange compatibility troubles.) I'll update the ChangeLogs with my findings about Firefox (IE, to my knowledge, did not previously implement the types attribute at all). Duplicates: That reminds me--I should add a new layout test for this. Previously, we didn't conform completely to the spec--it actually allows for "Files" to show up twice in the types attribute if there are files in the drag as well as data associated with the type "Files". Order: Unimportant, since we used to use a HashSet, which gave no ordering guarantees as far as I know.

Attachment 127014 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1 Source/WebCore/dom/Clipboard.h:36: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 127027 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1 Source/WebCore/dom/Clipboard.h:36: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 30 files If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 127084 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1 Source/WebCore/dom/Clipboard.h:36: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 30 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 127287 [details] Patch

Upon further inspection, I realized that "Files" will never appear twice anyway, since we always lowercase the format name in setData(). I still added a test to document the fact that we don't conform to the spec for returning the same, live object each time for some datatransfer attributes though.

Comment on attachment 127287 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127287&action=review LGTM. Thanks. > Source/WebCore/platform/chromium/ChromiumDataObject.cpp:113 > + for (HashSet<String>::const_iterator it = hashedResults.begin(); it != hashedResults.end(); ++it) > + results->append(*it); I think it would be totally OK to add a DOMStringList::fromHashSet function which did this. But this is also OK.

Committed r107894: <http://trac.webkit.org/changeset/107894>

This is a serious step backwards. Before this change things like indexOf, map, forEach all worked. DOMStringList is deprecated and should not be used in new APIs

If we intend to roll this change back, we should do it sooner rather than later. As far as I'm aware, WebKit was the only browser that returned an Array rather than a DOMStringList. Once this change goes out to stable versions of Safari and Chrome, pages will stop using the compatibility code they used to need.

I'm all for avoiding DOMStringList, it's ugly. But as Daniel points out, we were the odd-man out. I'm happy to revert this if you plan to lead the evangelizing charge arv?

I think we should just add contains to Array.prototype and then we will be compatible with Firefox. I did start the evangelizing. DOM4 now have a warning that DOMStringList should not be used for new APIs and the discussion about adding Array.prototype.contains to ES6 has started. Cameron also suggested adding [ArrayClass] to DOMTokenList to make existing cases work.

Since event.dataTransfer.types is not a legacy API and DOMStringList is deprecated, I think the best course of action would be to evangelize changing the property type in the spec to Array rather than add contains to Array.prototype.

So it looks like there's a good chance we'll be rolling this change back... http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2012-February/034954.html

As far as the spec goes, do whichever you think makes sense (sounds like returning an Array is it) and then mention on the HTML spec bug that you've done that, and I'll make the spec match the implementations.

Reverted r107894 for reason: dataTransfer.types should be an Array since DOMStringList is deprecated. Committed r109182: <http://trac.webkit.org/changeset/109182>

*** Bug 108767 has been marked as a duplicate of this bug. ***

FWIW years later we're trying to align all browsers on this using the new FrozenArray construct. I filed https://bugs.webkit.org/show_bug.cgi?id=162932 to track that.