Bug 34030 - ClipboardMac::setData() should be case-insensitive for the format parameter
Summary: ClipboardMac::setData() should be case-insensitive for the format parameter
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac (Intel) OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 33877
  Show dependency treegraph
 
Reported: 2010-01-22 18:49 PST by Daniel Cheng
Modified: 2010-06-10 20:49 PDT (History)
1 user (show)

See Also:


Attachments
Test case (467 bytes, text/html)
2010-01-22 19:05 PST, Daniel Cheng
no flags Details
Patch (3.75 KB, patch)
2010-01-22 19:16 PST, Daniel Cheng
darin: review-
darin: commit-queue-
Details | Formatted Diff | Diff
IE test case (307 bytes, text/html)
2010-01-22 19:57 PST, Daniel Cheng
no flags Details
Patch (7.63 KB, patch)
2010-02-03 18:56 PST, Daniel Cheng
levin: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Cheng 2010-01-22 18:49:44 PST
A simple test case has been attached.
Comment 1 Daniel Cheng 2010-01-22 19:05:03 PST
Created attachment 47259 [details]
Test case

Really attach the attachment this time.
Comment 2 Daniel Cheng 2010-01-22 19:16:01 PST
Created attachment 47261 [details]
Patch
Comment 3 Daniel Bates 2010-01-22 19:37:47 PST
This bug is related to bug #31003.
Comment 4 Daniel Cheng 2010-01-22 19:57:26 PST
Created attachment 47263 [details]
IE test case

The spec has this to say:
Formats are generally given by MIME types, with some values special-cased for legacy reasons. For the purposes of this API, however, the format strings are opaque, case-sensitive, strings, and the empty string is a valid format string.

According to RFC 2616, "The type, subtype, and parameter attribute names are case-insensitive[ in a MIME type]."

If formats are typically MIME types, I think it would make sense to ignore case.
IE also ignores case, as the attached IE test case shows.

Currently, Mac and Qt both respect case. It seems only the Windows implementation does not.
Comment 5 Darin Adler 2010-01-23 10:29:48 PST
Comment on attachment 47261 [details]
Patch

The right way to be case insensitive is to use equalIgnoringCase, not to call lower.
Comment 6 Daniel Cheng 2010-01-23 11:18:19 PST
(In reply to comment #5)
> (From update of attachment 47261 [details])
> The right way to be case insensitive is to use equalIgnoringCase, not to call
> lower.

I would do that, but there's more than just string comparisons. For example:
>     if (qType == "text/plain" || qType.startsWith("text/plain;"))

I've also asked WHAT for clarification. I think there's a case to be made for ignoring case in format strings, but I'm going to hold the patch until there's a clear answer here.
Comment 7 Daniel Cheng 2010-02-03 18:56:49 PST
Created attachment 48090 [details]
Patch
Comment 8 Daniel Cheng 2010-02-03 18:59:39 PST
The HTML5 draft has changed on this point:
http://www.whatwg.org/specs/web-apps/current-work/multipage/dnd.html#datatransfer

"DataTransfer objects can hold pieces of data, each associated with a unique format. Formats are generally given by MIME types, with some values special-cased for legacy reasons. However, the API does not enforce this; non-MIME-type values can be added as well. All formats are identified by strings that are converted to ASCII lowercase by the API."
Comment 9 Darin Adler 2010-02-04 13:14:55 PST
Comment on attachment 48090 [details]
Patch

Change seems OK.

The preferred way to compare and ignore case differences is to use the equalIgnoringCase function and the CaseFoldingHash hash function. I'd prefer to see it working that way.

The use of "q" in "qType" is an ancient relic from when WebCore/platform didn’t exist and the code was using QString. So it’s strange to introduce it in new code.
Comment 10 Daniel Cheng 2010-02-04 15:35:19 PST
(In reply to comment #9)
> (From update of attachment 48090 [details])
> Change seems OK.
> 
> The preferred way to compare and ignore case differences is to use the
> equalIgnoringCase function and the CaseFoldingHash hash function. I'd prefer to
> see it working that way.

The way I interpret the spec, the UA ought to lower case all formats. That being said, I think there are problems with either approach.

If we only use equalIgnoringCase and CaseFoldingHash, it becomes much harder to deal with the system pasteboard. For example, in qt, removing a format from the clipboard is done with this snippet:
  m_writableData->removeFormat(qType);
To implement that, a UA would need to enumerate all types in the clipboard and then compare each one in a case-insensitive manner. In addition, what happens when multiple formats match?

However, if we convert everything to lowercase, then there are native pasteboard/drag and drop types the interface will never be able to retrieve.

Given the problems with both approaches, maybe the spec should actually say:
Only "text" and "url" should be treated in a case-insensitive manner; all other format names must be handled in a case-sensitive manner. What do you think?

> 
> The use of "q" in "qType" is an ancient relic from when WebCore/platform didn’t
> exist and the code was using QString. So it’s strange to introduce it in new
> code.

Interesting. I didn't know that; I took the naming from WebCore/platform/win. I'll change it with next update to this patch.
Comment 11 David Levin 2010-02-04 16:41:14 PST
Comment on attachment 48090 [details]
Patch

r- based on comments already in the bug.