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

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

------- Additional Comments from Adam Roben (aroben) <aroben at apple.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.

 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.

So close!