Bug 175810 - Consolidate the code to normalize MIME type in DataTransfer
Summary: Consolidate the code to normalize MIME type in DataTransfer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-08-21 20:42 PDT by Ryosuke Niwa
Modified: 2017-08-24 10:28 PDT (History)
9 users (show)

See Also:


Attachments
Cleanup (12.42 KB, patch)
2017-08-21 20:48 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixes iOS builds (12.54 KB, patch)
2017-08-21 21:15 PDT, Ryosuke Niwa
wenson_hsieh: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2017-08-21 20:42:17 PDT
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.
Comment 1 Ryosuke Niwa 2017-08-21 20:48:05 PDT
Created attachment 318729 [details]
Cleanup
Comment 2 Ryosuke Niwa 2017-08-21 21:15:24 PDT
Created attachment 318730 [details]
Fixes iOS builds
Comment 3 Wenson Hsieh 2017-08-22 19:00:05 PDT
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?
Comment 4 Wenson Hsieh 2017-08-22 19:00:53 PDT
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".
Comment 5 Ryosuke Niwa 2017-08-22 19:18:49 PDT
(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
Comment 6 Wenson Hsieh 2017-08-22 19:28:49 PDT
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?)
Comment 7 Ryosuke Niwa 2017-08-22 19:56:44 PDT
(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.
Comment 8 Ryosuke Niwa 2017-08-22 19:57:23 PDT
Committed r221063: <http://trac.webkit.org/changeset/221063>
Comment 9 Radar WebKit Bug Importer 2017-08-22 19:58:25 PDT
<rdar://problem/34027678>
Comment 10 Ryosuke Niwa 2017-08-22 21:55:02 PDT
Committed r221067: <http://trac.webkit.org/changeset/221067>
Comment 11 Darin Adler 2017-08-24 10:28:49 PDT
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.