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

Glenn Wilson <gwilson at google.com> has asked  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 Glenn Wilson <gwilson at google.com>
Ok, the latest patch should clean up these issues.

>I think the new functions added to WebPreferences should have some explicit
>comments near their declarations stating more precisely what their purpose is
>(i.e., these are for DRT only).

Done, added comments to WebPreferences.h describing that these functions are
used in DumpRenderTree testing only.

>+    CFStringRef keyref = CFStringCreateWithCharactersNoCopy(0,
>(UniChar*)_wcsdup(key), (CFIndex)_tcslen(key), kCFAllocatorMalloc);
>
>This string is being leaked.
>Passing in kCFAllocatorMalloc here is wrong. This will cause CF to call
>free(key) when keyref is finalized.

Oops.  I'll admit my ignorance about kCFAllocatorMalloc here.  I've changed
this to kCFAllocatorNull, since the caller should take care of the cleanup.  I
debated using kCFAllocatorDefault, but I didn't want WebPreferences to get
overzealous in cleaning up memory it didn't allocate.

I also changed one of the c-style casts to a static_cast.  I tried changing
both, by my compiler didn't know of a good cast between a wide character and an
unsigned short.  So I left this as a c-style cast for now (it is done this way
elsewhere in WebPreferences).

>+	 setStringValue(keyref, value);
>
>Indentation here looks funny.

Fixed this.

>+	  BSTR val = stringValueForKey(static_cast<CFStringRef>(keys[i]));
>
>val is being leaked here. It also isn't used at all.

Yep, removed this.  A remnant of another way I was looping through values here.


>+    wstring wKey = jsStringRefToWString(key);
>+    wstring wFlag = jsStringRefToWString(flag);
>+
>+    BSTR keyBSTR = SysAllocStringLen((OLECHAR*)wKey.c_str(), wKey.length());
>+    BSTR flagBSTR = SysAllocStringLen((OLECHAR*)wFlag.c_str(),
>wFlag.length());
>
>You can use JSStringCopyBSTR here (from JSStringRefBSTR.h) instead.

Aha -- that made this much easier than allocating more memory.	I had to add an
include for JSStringRefBSTR.

Thanks for the review!