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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Oct 8 14:10:30 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
https://bugs.webkit.org/show_bug.cgi?id=20534

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

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
 1190	  CFStringRef keyref = CFStringCreateWithCharactersNoCopy(0,
(UniChar*)_wcsdup(key), static_cast<CFIndex>(wcslen(key)), kCFAllocatorNull);

You're leaking keyref. I also see now that you were using _wcsdup, so in fact
kCFAllocatorMalloc was correct (sorry for misleading you!). But I think we
should just let CF handle the memory allocation, like this:

RetainPtr<CFStringRef> keyRef(AdoptCF, CFStringCreateWithCharacters(0, key,
wcslen(key)));

(This will also fix the leak of keyRef by putting RetainPtr in charge of
releasing it).

 1209	  for (int i = 0; i < count; ++i) {
 1210	      setValueForKey(static_cast<CFStringRef>(keys[i]), values[i]);
 1211	  }

The curly braces here should be removed.

 323	 // This method is meant for overriding preferences for tests run
 324	 // with DumpRenderTree only.
 325	 virtual HRESULT STDMETHODCALLTYPE overridePreference(
 326	     /* [in] */ BSTR key,
 327	     /* [in] */ BSTR value);
 328 
 329	 // This method is meant for resetting overridden preferences for tests
run
 330	 // with DumpRenderTree only.
 331	 virtual HRESULT STDMETHODCALLTYPE resetToDefaults();

Thanks for adding these comments. You should also put them in
IWebPreferencesPrivate.idl and WebPreferencesPrivate.h.

 645		 prefsPrivate->setAuthorAndUserStylesEnabled(TRUE);
 646		 prefsPrivate->resetToDefaults();

What is the effect of the call to resetToDefaults on
setAuthorAndUserStylesEnabled?

I think this is quite close! r- so that we can fix the remaining memory issues.
Thanks for working on this!


More information about the webkit-reviews mailing list