DataTransfer.getData should lowercase its argument

RESOLVED FIXED in mozilla10

Status

()

Core
Drag and Drop
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Ms2ger, Assigned: Timothy Zhu)

Tracking

(Blocks: 1 bug, {html5})

Trunk
mozilla10
html5
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=Neil Deakin], URL)

Attachments

(1 attachment, 3 obsolete attachments)

2.03 KB, patch
Neil Deakin (not available until Aug 9)
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
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.
Blocks: 669004

Updated

6 years ago
Whiteboard: [mentor=Neil Deakin]
(Assignee)

Comment 2

6 years ago
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 on attachment 568727 [details] [diff] [review]
Patch

Thanks for the patch!

Neil, can you review this?
Attachment #568727 - Flags: review?(enndeakin)
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.
(Reporter)

Comment 5

6 years ago
(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.
(Assignee)

Comment 6

6 years ago
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.
(Reporter)

Comment 7

6 years ago
Yes.
(Assignee)

Comment 8

6 years ago
Here's my second try.
(Assignee)

Comment 9

6 years ago
Created attachment 570164 [details] [diff] [review]
Another patch!
Attachment #568727 - Attachment is obsolete: true
Attachment #568727 - Flags: review?(enndeakin)
(Assignee)

Updated

6 years ago
Attachment #570164 - Flags: review?(enndeakin)
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.
(Assignee)

Comment 11

6 years ago
Created attachment 572347 [details] [diff] [review]
Patch

Not sure if I did the test part right.
Attachment #572347 - Flags: review?(enndeakin)
Comment on attachment 572347 [details] [diff] [review]
Patch

Fix up the indenting and this is ok.
Attachment #572347 - Flags: review?(enndeakin) → review+
Attachment #570164 - Flags: review?(enndeakin)
(Assignee)

Comment 13

6 years ago
Created attachment 572566 [details] [diff] [review]
Fix indentation

Do I have to write review? every time?
Attachment #570164 - Attachment is obsolete: true
Attachment #572347 - Attachment is obsolete: true
Attachment #572566 - Flags: review?(enndeakin)
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.
Attachment #572566 - Flags: review?(enndeakin) → review+
(Assignee)

Comment 15

6 years ago
Is that "checkin?" or "checkin+"?

Updated

6 years ago
Keywords: checkin-needed
Assignee: nobody → nattofriends
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! :-)
Status: NEW → ASSIGNED
Keywords: checkin-needed
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 :-)
https://hg.mozilla.org/mozilla-central/rev/f25597d036ec
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10

Updated

6 years ago
Depends on: 701146

Updated

6 years ago
Depends on: 702035
You need to log in before you can comment on or make changes to this bug.