[Webkit-unassigned] [Bug 30888] Implementation of String.quote() method

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=30888





--- Comment #2 from Ioseb Dzmanashvili <ioseb.dzmanashvili at gmail.com>  2009-10-29 08:18:29 PDT ---
Hi Darin,

Thank you very much for detailed review!

(In reply to comment #1)

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

Done, I changed name to "appendQuotedString"

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

Done, extra level of function call is removed.

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

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

I added more test cases as you suggested. I copied part of them from
string-trim, and part of them from Mozilla's test cases(I found few)

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

Thank you very much!

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

I tried and got similar results in FF and found one difference in character
quoting. Mozilla's implementation quotes vertical space differently
particularly it returns "\v" whereas my attempt to use "appendQuotedString()"
returned different result "\u000B". I red JSON spec and vertical space is not
included in whitespace list... so I added new, third parameter to
appendQuotedString() string method that defaults to "true". function's default
behavior is same as was before(compatible to JSON spec) but if "false" is
passed instead it encodes vertical space character as "\v"

I hope approach I used doesn't violates any conventions.

With kindest regards,
Ioseb

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.