Last Comment Bug 662173 - DataTransfer.getData should lowercase its argument
: DataTransfer.getData should lowercase its argument
Status: RESOLVED FIXED
[mentor=Neil Deakin]
: html5
Product: Core
Classification: Components
Component: Drag and Drop (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Timothy Zhu
:
Mentors:
http://www.whatwg.org/html/#dom-datat...
Depends on: 701146 702035
Blocks: 669004
  Show dependency treegraph
 
Reported: 2011-06-05 11:33 PDT by :Ms2ger (⌚ UTC+1/+2)
Modified: 2011-11-12 19:29 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (1.50 KB, patch)
2011-10-21 11:56 PDT, Timothy Zhu
no flags Details | Diff | Splinter Review
Another patch! (1.09 KB, patch)
2011-10-27 19:35 PDT, Timothy Zhu
no flags Details | Diff | Splinter Review
Patch (2.02 KB, patch)
2011-11-06 15:13 PST, Timothy Zhu
enndeakin: review+
Details | Diff | Splinter Review
Fix indentation (2.03 KB, patch)
2011-11-07 12:05 PST, Timothy Zhu
enndeakin: review+
Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2011-06-05 11:33:50 PDT

    
Comment 1 Neil Deakin 2011-09-28 08:25:32 PDT
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 Timothy Zhu 2011-10-21 11:56:25 PDT
Created attachment 568727 [details] [diff] [review]
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-21 12:00:31 PDT
Comment on attachment 568727 [details] [diff] [review]
Patch

Thanks for the patch!

Neil, can you review this?
Comment 4 Neil Deakin 2011-10-21 13:59:17 PDT
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 (⌚ UTC+1/+2) 2011-10-22 05:05:39 PDT
(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 Timothy Zhu 2011-10-23 01:58:25 PDT
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 (⌚ UTC+1/+2) 2011-10-23 02:29:13 PDT
Yes.
Comment 8 Timothy Zhu 2011-10-27 19:34:03 PDT
Here's my second try.
Comment 9 Timothy Zhu 2011-10-27 19:35:00 PDT
Created attachment 570164 [details] [diff] [review]
Another patch!
Comment 10 Neil Deakin 2011-10-31 10:20:43 PDT
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 Timothy Zhu 2011-11-06 15:13:30 PST
Created attachment 572347 [details] [diff] [review]
Patch

Not sure if I did the test part right.
Comment 12 Neil Deakin 2011-11-07 05:19:47 PST
Comment on attachment 572347 [details] [diff] [review]
Patch

Fix up the indenting and this is ok.
Comment 13 Timothy Zhu 2011-11-07 12:05:04 PST
Created attachment 572566 [details] [diff] [review]
Fix indentation

Do I have to write review? every time?
Comment 14 Neil Deakin 2011-11-07 12:28:34 PST
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 Timothy Zhu 2011-11-07 12:31:12 PST
Is that "checkin?" or "checkin+"?
Comment 16 Ed Morley [:emorley] 2011-11-08 00:43:01 PST
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] 2011-11-08 03:50:43 PST
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] 2011-11-08 07:06:09 PST
https://hg.mozilla.org/mozilla-central/rev/f25597d036ec

Note You need to log in before you can comment on or make changes to this bug.