[webkit-reviews] review requested: [Bug 20534] DumpRenderTree needs a way to override settings on a per-test basis : [Attachment 24237] 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 24237: Improved possible patch to issue 20534
https://bugs.webkit.org/attachment.cgi?id=24237&action=edit

------- Additional Comments from Glenn Wilson <gwilson at google.com>
> 1190	   if (!key || SysStringLen(key) == 0)
> 1191	       return E_FAIL;
>
>We prefer to use ! instead of ==0. We also need the same check for value. And
>as mentioned above, the null-check isn't necessary.

Changed.  I added value to the check and used ! instead:

    if (!SysStringLen(key) || !SysStringLen(value))
	return E_FAIL;

> 331	  JSRetainPtr<JSStringRef> key(Adopt, JSValueToStringCopy(context,
>arguments[0], exception));
> 332	  ASSERT(!*exception);
> 333	 JSRetainPtr<JSStringRef> value(Adopt, JSValueToStringCopy(context,
>arguments[1], exception));
>
>If you're going to assert about one you should assert about both.

I wasn't sure whether it was better to try to assert twice (after trying to
create each one), or just once after they're both attempted.  I went with the
former, since it is probably better not to try to create the JSStringRef to
value if the creation of the JSStringRef to key failed.  Other functions in
this class use multiple asserts as well, so this should be consistent.

Thanks!  We're almost there, I think.