Bug 128517 - Stop using String::deprecatedCharacters to call WTF::Collator
Summary: Stop using String::deprecatedCharacters to call WTF::Collator
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-02-09 22:33 PST by Darin Adler
Modified: 2014-02-10 12:07 PST (History)
6 users (show)

See Also:

Attachments
Patch (24.51 KB, patch)
2014-02-09 22:51 PST, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (24.51 KB, patch)
2014-02-09 22:58 PST, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (24.78 KB, patch)
2014-02-09 23:25 PST, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (24.72 KB, patch)
2014-02-10 07:59 PST, Darin Adler 		ap: review+
ap: commit-queue-
Details | Formatted Diff | Diff
Patch (25.05 KB, patch)
2014-02-10 09:20 PST, Darin Adler 		no flags Details | Formatted Diff | Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2014-02-09 22:33:51 PST 
Stop using String::deprecatedCharacters to call WTF::Collator
Comment 1 Darin Adler 2014-02-09 22:51:01 PST 
Created attachment 223676 [details]
Patch
Comment 2 Darin Adler 2014-02-09 22:58:59 PST 
Created attachment 223677 [details]
Patch
Comment 3 Darin Adler 2014-02-09 23:25:55 PST 
Created attachment 223679 [details]
Patch
Comment 4 Darin Adler 2014-02-10 07:59:01 PST 
Created attachment 223713 [details]
Patch
Comment 5 Alexey Proskuryakov 2014-02-10 08:32:56 PST 
Comment on attachment 223713 [details]
Patch

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

r=me

This patch doesn't apply any more, so it would be useful to let EWS work on an updated version.

> Source/WTF/wtf/unicode/icu/CollatorICU.cpp:71
> +static inline char* copyASCIIString(CFStringRef string)

Maybe a name like copyShortASCIIString or copyLocaleNameToASCIIString would make it more obvious why the implementation is not general purpose.

> Source/WTF/wtf/unicode/icu/CollatorICU.cpp:95
> +    static char* defaultLocale = copyDefaultLocale();

I like to ASSERT(isMainThread()) everywhere a static variables are used like this.

Or maybe this could be made thread safe.
Comment 6 Darin Adler 2014-02-10 09:18:47 PST 
Comment on attachment 223713 [details]
Patch

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

>> Source/WTF/wtf/unicode/icu/CollatorICU.cpp:71
>> +static inline char* copyASCIIString(CFStringRef string)
> 
> Maybe a name like copyShortASCIIString or copyLocaleNameToASCIIString would make it more obvious why the implementation is not general purpose.

Great idea. Done.

>> Source/WTF/wtf/unicode/icu/CollatorICU.cpp:95
>> +    static char* defaultLocale = copyDefaultLocale();
> 
> I like to ASSERT(isMainThread()) everywhere a static variables are used like this.
> 
> Or maybe this could be made thread safe.

Great suggestion. I used std::call_once to make this thread safe.
Comment 7 Darin Adler 2014-02-10 09:20:14 PST 
Created attachment 223719 [details]
Patch
Comment 8 WebKit Commit Bot 2014-02-10 09:22:37 PST 
Attachment 223719 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/unicode/icu/CollatorICU.cpp:98:  Missing space before {  [whitespace/braces] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Darin Adler 2014-02-10 09:41:31 PST 
Committed r163792: <http://trac.webkit.org/changeset/163792>
Comment 10 Enrica Casucci 2014-02-10 12:00:21 PST 
This causes a crash on iOS. I'll post a patch to fix it.
Comment 11 Enrica Casucci 2014-02-10 12:07:18 PST 
Patch attached to https://bugs.webkit.org/show_bug.cgi?id=128548