Bug 100701 - AtomicString(CFStringRef) shouldn't unconditionally create a StringImpl
Summary: AtomicString(CFStringRef) shouldn't unconditionally create a StringImpl
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Anders Carlsson
URL:
Keywords: InRadar
Depends on: 100727
Blocks:
  Show dependency tree / graph
 
Reported: 2012-10-29 14:46 PDT by Anders Carlsson
Modified: 2013-01-15 10:15 PST (History)
5 users (show)

See Also:

Attachments
Patch (13.27 KB, patch)
2012-10-29 14:54 PDT, Anders Carlsson 		no flags Details | Formatted Diff | Diff
Patch (13.28 KB, patch)
2012-10-29 15:04 PDT, Anders Carlsson 		no flags Details | Formatted Diff | Diff
Patch (13.31 KB, patch)
2012-10-29 15:22 PDT, Anders Carlsson 		mitz: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Show Obsolete (2) 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 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.