[webkit-reviews] review denied: [Bug 123803] [WIN] Add a wrapper for setting the default settings : [Attachment 216040] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied Patrick R. Gansterer
<paroga at paroga.com>'s request for review:
Bug 123803: [WIN] Add a wrapper for setting the default settings
https://bugs.webkit.org/show_bug.cgi?id=123803

Attachment 216040: Patch
https://bugs.webkit.org/attachment.cgi?id=216040&action=review

------- Additional Comments from Brent Fulgham <bfulgham at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=216040&action=review


This change is likely to introduce stability problems as versions of
CoreFoundation or WebKit are changed out independently.

> Source/WebKit/win/WebPreferences.cpp:116
> +	   RetainPtr<CFStringRef> cfValue =
adoptCF(CFStringCreateWithCStringNoCopy(0, value, kCFStringEncodingASCII, 0));

This doesn't properly duplicate the behavior of the CFSTR macro.  CFSTR(a) maps
to __CFStringMakeConstantString(a), which builds a cached string value
internally so that subsequent calls get the 'same' object back.

We definitely don't want to use "CFStringCreateWithCStringNoCopy" because we
can have allocator differences between the C++ RTL and whatever was used to
build the specific version of CoreFoundation used with WebKit.	It would be
better to change this to CFStringCreateWithCString (allowing a copy to be made
by the CoreFoundation allocator), and better still to use the
__CFStringMakeConstantString method call.