Bug 100701

Summary: AtomicString(CFStringRef) shouldn't unconditionally create a StringImpl
Product: WebKit Reporter: Anders Carlsson <andersca>
Component: Web Template FrameworkAssignee: Anders Carlsson <andersca>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, darin, msaboff, ossy, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 100727    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch mitz: review+, buildbot: commit-queue-

Description Anders Carlsson 2012-10-29 14:46:05 PDT 
AtomicString(CFStringRef) shouldn't unconditionally create a StringImpl
Comment 1 Anders Carlsson 2012-10-29 14:54:34 PDT 
Created attachment 171316 [details]
Patch
Comment 2 Anders Carlsson 2012-10-29 15:04:32 PDT 
Created attachment 171319 [details]
Patch
Comment 3 Anders Carlsson 2012-10-29 15:22:34 PDT 
Created attachment 171322 [details]
Patch
Comment 4 mitz 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.
Comment 5 Build Bot 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
Comment 6 Anders Carlsson 2012-10-29 16:22:52 PDT 
Committed r132858: <http://trac.webkit.org/changeset/132858>
Comment 7 Benjamin Poulain 2012-10-29 17:00:39 PDT 
<rdar://problem/12494603>

Thanks for doing this, it is a nice change.
Comment 8 Csaba Osztrogonác 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
Comment 9 Darin Adler 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?
Comment 10 Anders Carlsson 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.
Comment 11 Benjamin Poulain 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.)
Comment 12 Darin Adler 2013-01-15 10:15:08 PST 
Anders and I checked, and I had remembered wrong. Code is fine.