[webkit-reviews] review denied: [Bug 30888] Implementation of String.quote() method : [Attachment 42065] Implementation of String.quote() method

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Ioseb Dzmanashvili
<ioseb.dzmanashvili at gmail.com>'s request for review:
Bug 30888: Implementation of String.quote() method
https://bugs.webkit.org/show_bug.cgi?id=30888

Attachment 42065: Implementation of String.quote() method
https://bugs.webkit.org/attachment.cgi?id=42065&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Thanks for tackling this.

> +void quoteString(UString& builder, const UString& value)

This function should be named appendQuotedString, because it appends a quoted
copy of value to builder rather than overwriting it.

>  void Stringifier::appendQuotedString(StringBuilder& builder, const UString&
value)
>  {
> +    quoteString(builder, value);
>  }

You should not be adding an extra level of function call here. Instead the
callers should call the new appendQuotedString directly.

> +	shouldBe("actual.quote()", "expected");
> +	shouldBe("String.prototype.quote.call(actual)", "expected");
> +	shouldBe("String.prototype.quote.apply(actual)", "expected");

The goal is normally to have the test cases readable in the test output. By
passing string constants to shouldBe, you're making it so the test output
doesn't show the test cases.

You need to have other test cases such as calling quote with no argument, or an
argument of various types.

There are far too few test cases. You need a test case of the empty string, a
test case where you don't pass a string at all, and test cases that include the
other control characters and non-ASCII characters.

Take a look at fast/js/script-tests/string-trim.js for an example that has
better coverage. Although I don't like everything about that test it's pretty
good.

Otherwise the patch look pretty good, and I think this is a reasonable thing to
add.

Did you try the test case with Firefox to see if it behaves the same way?