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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Oct 9 12:27:40 PDT 2008


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 24232: Improved possible patch to issue 20534
https://bugs.webkit.org/attachment.cgi?id=24232&action=edit

------- Additional Comments from Glenn Wilson <gwilson at google.com>
>On Windows you perform these operations in the opposite order. Which one is
>better?

The way I was doing this on Windows is probably better, because it avoids a
race condition.  The changes notification should happen after the flag gets
reset, so that any listeners who care about the flag being reset will see the
correct state.	So, I flipped these lines on the Mac implementation of this.

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

So I'm not totally sure this is the right way to do this, but I added this
check:

    if (!key || SysStringLen(key) == 0)
		return E_FAIL;

I figured that the BSTR could be either 0 (like you said), or an empty string
(the case in which it is non-zero but empty).  So I added a bailout on either
one, since we probably don't want empty string key.  It doesn't seem to have
this type of problem on the Mac version, since it just passes the pointer
through...no allocation of memory needed.

> 335	  controller->overridePreference(key.get(),
JSValueToStringCopy(context,
>arguments[1], exception));
>
>It looks like you're leaking the second parameter to overridePreference here.
>

Ah, yes...good catch.  I got the key RetainPtr right, but not the value.  This
has been changed so both are made into RetainPtrs before being passed in.

Please let me know if you find anything else that should be fixed!  Thanks for
all of your feedback.


More information about the webkit-reviews mailing list