[webkit-reviews] review requested: [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]

Glenn Wilson <gwilson at google.com> has asked Adam Roben (aroben)
<aroben at apple.com> 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 Glenn Wilson <gwilson at google.com>
>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).

Ah, much easier this way :)  I added the line you suggested with one change: a
reinterpret_cast was necessary to cast the BSTR to UniChar*, so thanks for that
tip!

>The curly braces here should be removed.

Removed.

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

Added the same comments to both places.  I changed the previous style of
comment that I used in WebPreferencesPrivate to the style the rest of the file
used (just "//"s).  The only other place I can see comments being useful is in
all the LayoutTestController files, but I decided against it since the
LayoutTestController is only for running tests anyway (...or is it?)

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

Since the default is the same as what is being set on this line, the net effect
is zero.  *But* since the default for this setting could change in the future,
I've flipped these lines so the reset call won't clobber this setting.	The
modified Mac version of DumpRenderTree works this way, so it makes sense for
consistency.

Thanks for your help!!