[webkit-reviews] review denied: [Bug 20534] DumpRenderTree needs a way to override settings on a per-test basis : [Attachment 24204] Improved possible patch to issue 20534

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has denied Glenn Wilson
<gwilson at google.com>'s request for review:
Bug 20534: DumpRenderTree needs a way to override settings on a per-test basis
https://bugs.webkit.org/show_bug.cgi?id=20534

Attachment 24204: Improved possible patch to issue 20534
https://bugs.webkit.org/attachment.cgi?id=24204&action=edit

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
 1060	  [self _postPreferencesChangesNotification];	
 1061	  [self _setBoolValue:false forKey:WebKitDefaultPreferencesOverridden];


On Windows you perform these operations in the opposite order. Which one is
better?

 1190	  RetainPtr<CFStringRef> keyRef(AdoptCF,
CFStringCreateWithCharacters(0, reinterpret_cast<UniChar*>(key), wcslen(key)));


I just remembered that 0 is a valid BSTR. What should be our behavior when 0 is
passed in? Currently I believe we will crash. Using SysStringLen instead of
wcslen would help, but I don't think we should be passing 0 for the characters
pointer to CFStringCreateWithCharacters. Probably we should just bail out with
E_FAIL in this case.

 335	 controller->overridePreference(key.get(), JSValueToStringCopy(context,
arguments[1], exception));

It looks like you're leaking the second parameter to overridePreference here.

Sorry for not catching these on the last go-round. But once these are fixed we
should be able to land this!