662173 - DataTransfer.getData should lowercase its argument

Status: RESOLVED FIXED

(Core :: DOM: Copy & Paste and Drag & Drop, defect)

Comment 1

[Neil Deakin]

Should be a simple one-line fix to nsDOMDataTransfer::GetRealFormat, one additional line to nsDOMDataTransfer::GetData for the 'URL' check, plus tests added to test_dragstart.html.

Comment 2

[nattofriends]

Attachment: Patch

Hi, 

This is my first time submitting a patch to Mozilla. I'm not very familiar with how everything works here, so I hope I'm doing this right...

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 568727 [details] [diff] [review]
Patch

Thanks for the patch!

Neil, can you review this?

Comment 4

[Neil Deakin]

Comment on attachment 568727 [details] [diff] [review]
Patch

Review of attachment 568727 [details] [diff] [review]:
-----------------------------------------------------------------

You want to do the lowercase conversion in GetRealFormat, so it can be handled by getData, setData and clearData and the At variants.

Also, just use the global ToLowerCase function to convert to lowercase, and LowerCaseEqualsLiteral to compare.

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Neil Deakin from comment #4)
> Comment on attachment 568727 [details] [diff] [review] [diff] [details] [review]
> Patch
> 
> Review of attachment 568727 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> You want to do the lowercase conversion in GetRealFormat, so it can be
> handled by getData, setData and clearData and the At variants.
> 
> Also, just use the global ToLowerCase function to convert to lowercase, and
> LowerCaseEqualsLiteral to compare.

It should explicitly lowercase only in the ASCII range. The functions you mention do Unicode lowercasing.

Comment 6

[nattofriends]

In that case, is my usage of ASCIIToLower correct?

(In reply to Ms2ger from comment #5)
> (In reply to Neil Deakin from comment #4)
> > Comment on attachment 568727 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > Patch
> > 
> > Review of attachment 568727 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]:
> > -----------------------------------------------------------------
> > 
> > You want to do the lowercase conversion in GetRealFormat, so it can be
> > handled by getData, setData and clearData and the At variants.
> > 
> > Also, just use the global ToLowerCase function to convert to lowercase, and
> > LowerCaseEqualsLiteral to compare.
> 
> It should explicitly lowercase only in the ASCII range. The functions you
> mention do Unicode lowercasing.

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Yes.

Comment 8

[nattofriends]

Here's my second try.

Comment 9

[nattofriends]

Attachment: Another patch!

Comment 10

[Neil Deakin]

Comment on attachment 570164 [details] [diff] [review]
Another patch!

>diff -r 1fa31fa85082 content/events/src/nsDOMDataTransfer.cpp
>     // for the URL type, parse out the first URI from the list. The URIs are
>     // separated by newlines
>-    if (aFormat.EqualsLiteral("URL")) {
>+    if (aFormat.EqualsLiteral("url")) {

Convert to lowercase here.

>   // treat text/unicode as equivalent to text/plain
>-  if (aInFormat.EqualsLiteral("Text") || aInFormat.EqualsLiteral("text/unicode"))
>+  if (aInFormat.LowerCaseEqualsLiteral("text") || aInFormat.LowerCaseEqualsLiteral("text/unicode"))
>     aOutFormat.AssignLiteral("text/plain");
>-  else if (aInFormat.EqualsLiteral("URL"))
>+  else if (aInFormat.LowerCaseEqualsLiteral("url"))
>     aOutFormat.AssignLiteral("text/uri-list");
>   else
>     aOutFormat.Assign(aInFormat);

I think the spec says to lowercase all formats, not just 'Text' and 'URL'. We could just call: nsContentUtils::ASCIIToLower(aInFormat, aOutFormat) somewhere.

Also, if you're up to it, some tests for this could be added to content/events/test/test_dragstart.html . Let me know if you need help.

Comment 11

[nattofriends]

Attachment: Patch

Not sure if I did the test part right.

Comment 12

[Neil Deakin]

Comment on attachment 572347 [details] [diff] [review]
Patch

Fix up the indenting and this is ok.

Comment 13

[nattofriends]

Attachment: Fix indentation

Do I have to write review? every time?

Comment 14

[Neil Deakin]

Comment on attachment 572566 [details] [diff] [review]
Fix indentation

> Do I have to write review? every time?

You don't need to once the patch is marked with a +, but you can do if you'd like to confirm your patch.

Thanks for the patch. If you don't have permission to check the patch in yourself, add the 'checkin-needed' to the keyword field.

Comment 15

[nattofriends]

Is that "checkin?" or "checkin+"?

Comment 16

[Ed Morley [:emorley]]

Pushed to try, presuming all green, will push to inbound after.
https://tbpl.mozilla.org/?tree=Try&rev=1e7c86487f42

To save time for future patches (this one is fine for now), could you set your hgrc to include the author automatically & also add a commit message, along the lines of:
http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed

Thanks for the patch! :-)

Comment 17

[Ed Morley [:emorley]]

Pushed to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f25597d036ec

(This will get merged to mozilla-central by the end of the day; leaving milestone blank for now, since not sure if it will make today's aurora uplift).

Congratulations on your first patch in the tree! Hope to see you on IRC in #developers soon (see https://wiki.mozilla.org/IRC#Getting_Started for details). 

If you'd like to fix another bug (it would be awesome if you did!) but need some inspiration, pop on & say hi - and we'll find something for you :-)

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f25597d036ec