[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 bugzilla-daemon at webkit.org
Thu Oct 9 06:57:51 PDT 2008

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

Attachment 24204: Improved possible patch to issue 20534

------- 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

 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!

More information about the webkit-reviews mailing list