RESOLVED FIXED 100701
AtomicString(CFStringRef) shouldn't unconditionally create a StringImpl 
https://bugs.webkit.org/show_bug.cgi?id=100701
Summary AtomicString(CFStringRef) shouldn't unconditionally create a StringImpl
Anders Carlsson
Reported 2012-10-29 14:46:05 PDT
AtomicString(CFStringRef) shouldn't unconditionally create a StringImpl
Attachments
Patch (13.27 KB, patch)
2012-10-29 14:54 PDT, Anders Carlsson 		no flags
DetailsFormatted DiffDiff
Patch (13.28 KB, patch)
2012-10-29 15:04 PDT, Anders Carlsson 		no flags
DetailsFormatted DiffDiff
Patch (13.31 KB, patch)
2012-10-29 15:22 PDT, Anders Carlsson 		mitz: review+
buildbot: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Anders Carlsson
Comment 1 2012-10-29 14:54:34 PDT
Created attachment 171316 [details] Patch
Anders Carlsson
Comment 2 2012-10-29 15:04:32 PDT
Created attachment 171319 [details] Patch
Anders Carlsson
Comment 3 2012-10-29 15:22:34 PDT
Created attachment 171322 [details] Patch
mitz
Comment 4 2012-10-29 15:42:38 PDT
Comment on attachment 171322 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171322&action=review > Source/WebCore/ChangeLog:9 > + Add __ZN3WTF12AtomicString3addEPK10__CFString. You can use the demangled name here. > Source/WebCore/platform/text/cf/AtomicStringCF.cpp:2 > + * Copyright (C) 2010, 2011, 2012 Apple Inc. All rights reserved. I think this is all from 2012.
Build Bot
Comment 5 2012-10-29 16:07:54 PDT
Comment on attachment 171322 [details] Patch Attachment 171322 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14631283
Anders Carlsson
Comment 6 2012-10-29 16:22:52 PDT
Committed r132858: <http://trac.webkit.org/changeset/132858>
Benjamin Poulain
Comment 7 2012-10-29 17:00:39 PDT
<rdar://problem/12494603> Thanks for doing this, it is a nice change.
Csaba Osztrogonác
Comment 8 2012-10-29 23:02:32 PDT
(In reply to comment #6) > Committed r132858: <http://trac.webkit.org/changeset/132858> It broke the build on Qt Mac - https://bugs.webkit.org/show_bug.cgi?id=100727
Darin Adler
Comment 9 2013-01-14 09:49:57 PST
Comment on attachment 171322 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171322&action=review > Source/WebCore/platform/text/cf/AtomicStringCF.cpp:39 > + if (const LChar* ptr = reinterpret_cast<const LChar*>(CFStringGetCStringPtr(string, kCFStringEncodingISOLatin1))) Anders, I seem to recall something that Michael Saboff had learned, where CFStringGetCStringPtr returns a non-null pointer far more often when passed ASCII as the encoding rather than ISO Latin-1 on newer versions of Foundation. Am I remembering that right? Do you know about that?
Anders Carlsson
Comment 10 2013-01-14 10:25:09 PST
(In reply to comment #9) > (From update of attachment 171322 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=171322&action=review > > > Source/WebCore/platform/text/cf/AtomicStringCF.cpp:39 > > + if (const LChar* ptr = reinterpret_cast<const LChar*>(CFStringGetCStringPtr(string, kCFStringEncodingISOLatin1))) > > Anders, I seem to recall something that Michael Saboff had learned, where CFStringGetCStringPtr returns a non-null pointer far more often when passed ASCII as the encoding rather than ISO Latin-1 on newer versions of Foundation. Am I remembering that right? Do you know about that? Hmm, that doesn't ring a bell. Should be easy to test though.
Benjamin Poulain
Comment 11 2013-01-14 13:25:45 PST
> Anders, I seem to recall something that Michael Saboff had learned, where CFStringGetCStringPtr returns a non-null pointer far more often when passed ASCII as the encoding rather than ISO Latin-1 on newer versions of Foundation. Am I remembering that right? Do you know about that? (We should probably look if we should fix Foundation instead of WebKit.)
Darin Adler
Comment 12 2013-01-15 10:15:08 PST
Anders and I checked, and I had remembered wrong. Code is fine.
Note You need to log in before you can comment on or make changes to this bug.