Bug 128517

Summary: Stop using String::deprecatedCharacters to call WTF::Collator
Product: WebKit Reporter: Darin Adler <darin>
Component: Web Template FrameworkAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, ap, benjamin, cmarcelo, commit-queue, enrica
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
ap: review+, ap: commit-queue-
Patch none

Darin Adler
Reported 2014-02-09 22:33:51 PST
Stop using String::deprecatedCharacters to call WTF::Collator
Attachments
Patch (24.51 KB, patch)
2014-02-09 22:51 PST, Darin Adler 		no flags
DetailsFormatted DiffDiff
Patch (24.51 KB, patch)
2014-02-09 22:58 PST, Darin Adler 		no flags
DetailsFormatted DiffDiff
Patch (24.78 KB, patch)
2014-02-09 23:25 PST, Darin Adler 		no flags
DetailsFormatted DiffDiff
Patch (24.72 KB, patch)
2014-02-10 07:59 PST, Darin Adler 		ap: review+
ap: commit-queue-
DetailsFormatted DiffDiff
Patch (25.05 KB, patch)
2014-02-10 09:20 PST, Darin Adler 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Darin Adler
Comment 1 2014-02-09 22:51:01 PST
Created attachment 223676 [details] Patch
Darin Adler
Comment 2 2014-02-09 22:58:59 PST
Created attachment 223677 [details] Patch
Darin Adler
Comment 3 2014-02-09 23:25:55 PST
Created attachment 223679 [details] Patch
Darin Adler
Comment 4 2014-02-10 07:59:01 PST
Created attachment 223713 [details] Patch
Alexey Proskuryakov
Comment 5 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.
Darin Adler
Comment 6 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.
Darin Adler
Comment 7 2014-02-10 09:20:14 PST
Created attachment 223719 [details] Patch
WebKit Commit Bot
Comment 8 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.
Darin Adler
Comment 9 2014-02-10 09:41:31 PST
Committed r163792: <http://trac.webkit.org/changeset/163792>
Enrica Casucci
Comment 10 2014-02-10 12:00:21 PST
This causes a crash on iOS. I'll post a patch to fix it.
Enrica Casucci
Comment 11 2014-02-10 12:07:18 PST
Patch attached to https://bugs.webkit.org/show_bug.cgi?id=128548
Note You need to log in before you can comment on or make changes to this bug.