[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