Bug 175810 – Consolidate the code to normalize MIME type in DataTransfer

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

Right now, each port's Pasteboard class is converting "text" and "URL" to "text/plain" and "text/uri-list"
as well as lowercasing & stripping whitespaces.

Consolidate that code into DataTransfer class itself to share.

[Ryosuke Niwa]

Created attachment 318729 [details]
Cleanup

[Ryosuke Niwa]

Created attachment 318730 [details]
Fixes iOS builds

[Wenson Hsieh]

Comment on attachment 318730 [details]
Fixes iOS builds

View in context: https://bugs.webkit.org/attachment.cgi?id=318730&action=review

> Source/WebCore/dom/DataTransfer.cpp:104
> +static String normalizeType(const String& type)

Nit - I think the name "normalizedType" or "normalizedType" would be better suited here (normalizeType sounds like it would mutate the given type instead of returning something).

> Source/WebCore/dom/DataTransfer.cpp:125
> +    String normalizedType = normalizeType(type);

Did you mean to use normalizedType afterwards here?

[Wenson Hsieh]

Comment on attachment 318730 [details]
Fixes iOS builds

View in context: https://bugs.webkit.org/attachment.cgi?id=318730&action=review

>> Source/WebCore/dom/DataTransfer.cpp:104
>> +static String normalizeType(const String& type)
> 
> Nit - I think the name "normalizedType" or "normalizedType" would be better suited here (normalizeType sounds like it would mutate the given type instead of returning something).

Whoops, I meant to say "normalizedType" or "toNormalizedType".

[Ryosuke Niwa]

(In reply to Wenson Hsieh from comment #4)
> Comment on attachment 318730 [details]
> Fixes iOS builds
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318730&action=review
> 
> >> Source/WebCore/dom/DataTransfer.cpp:104
> >> +static String normalizeType(const String& type)
> > 
> > Nit - I think the name "normalizedType" or "normalizedType" would be better suited here (normalizeType sounds like it would mutate the given type instead of returning something).
> 
> Whoops, I meant to say "normalizedType" or "toNormalizedType".

No, a function name needs to a verb: https://webkit.org/code-style-guidelines/#names-verb

[Wenson Hsieh]

Comment on attachment 318730 [details]
Fixes iOS builds

View in context: https://bugs.webkit.org/attachment.cgi?id=318730&action=review

>>>> Source/WebCore/dom/DataTransfer.cpp:104
>>>> +static String normalizeType(const String& type)
>>> 
>>> Nit - I think the name "normalizedType" or "normalizedType" would be better suited here (normalizeType sounds like it would mutate the given type instead of returning something).
>> 
>> Whoops, I meant to say "normalizedType" or "toNormalizedType".
> 
> No, a function name needs to a verb: https://webkit.org/code-style-guidelines/#names-verb

Ah ok, fair enough -- I didn't realize this was in the style guidelines :P

(There are a number of method names in WebKit of the form ::toSomeType(), but maybe this makes more sense for method names?)

[Ryosuke Niwa]

(In reply to Wenson Hsieh from comment #6)
> Comment on attachment 318730 [details]
> Fixes iOS builds
>
> Ah ok, fair enough -- I didn't realize this was in the style guidelines :P
> 
> (There are a number of method names in WebKit of the form ::toSomeType(),
> but maybe this makes more sense for method names?)

We used to use toX() instead of downcast<X>(). We just need to replace with downcast<X> functions.

[Ryosuke Niwa]

Committed r221063: <http://trac.webkit.org/changeset/221063>

[Radar WebKit Bug Importer]

<rdar://problem/34027678>

[Ryosuke Niwa]

Committed r221067: <http://trac.webkit.org/changeset/221067>

[Darin Adler]

Comment on attachment 318730 [details]
Fixes iOS builds

View in context: https://bugs.webkit.org/attachment.cgi?id=318730&action=review

> Source/WebCore/dom/DataTransfer.cpp:107
> +    if (type.isNull())
> +        return type;

Why this special case for null? The code below works fine for both null and empty strings; do we need this special case as a performance optimization?

> Source/WebCore/dom/DataTransfer.cpp:109
> +    String lowercaseType = type.stripWhiteSpace().convertToASCIILowercase();

It’s not good to strip arbitrary Unicode whitespace here, and possibly even dangerous. Stripping HTML spaces is better.

> Source/WebCore/dom/DataTransfer.cpp:110
> +    if (lowercaseType == "text" || lowercaseType.startsWithIgnoringASCIICase("text/plain;"))

Seems unnecessary to use the "ignoring ASCII case" variant after we have already lowercased it.

> Source/WebCore/dom/DataTransfer.cpp:112
> +    if (lowercaseType == "url" || lowercaseType.startsWithIgnoringASCIICase("text/uri-list;"))

Ditto.

> Source/WebCore/dom/DataTransfer.cpp:114
> +    if (lowercaseType.startsWithIgnoringASCIICase("text/html;"))

Ditto.