Bug 30888 – Implementation of String.quote() method

• Status: RESOLVED
• Resolution: WONTFIX
• Component: JavaScriptCore

[Ioseb Dzmanashvili]

Created attachment 42065 [details]
Implementation of String.quote() method

Hi all,

I've just implemented String.quote() method based on Mozilla's implementation: https://developer.mozilla.org/en/Core_JavaScript_1.5_Reference/Global_Objects/String

Actually this is not standard feature, but as far as it is often very useful to quote strings with this method and Mozilla already implemented it.

While trying to implement this method I found that there is code in JSON module that implements string quoting method but was not public. I moved this method to "JSGlobalObjectFunctions.cpp" (see "quoteString()" function) and changed JSON method to invoke this global function. "quote()" method uses this global function as well... 

Implementation and test cases are included in patch.

Regards,
Ioseb

[Darin Adler]

Comment on attachment 42065 [details]
Implementation of String.quote() method

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?

[Ioseb Dzmanashvili]

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

[Ioseb Dzmanashvili]

Created attachment 42100 [details]
String.quote() method patch

[Ioseb Dzmanashvili]

Hi again Darin,

What do you think about this patch? is it acceptable or do I need to change something more?

[Oliver Hunt]

I'm not entirely convinced of the benefit of this function -- it's not a standard feature, and doesn't give you anything JSON.stringify doesn't give you already.

[Ioseb Dzmanashvili]

(In reply to comment #5)
> I'm not entirely convinced of the benefit of this function -- it's not a
> standard feature, and doesn't give you anything JSON.stringify doesn't give you
> already.

Hi Oliver, 

This nonstandard method actually was in spec draft and as I think FF team implemented it then... though method was removed from spec later as far as it no longer exists in final draft now.

Only difference from SJSON.stringify() I was able to find is encoding of vertical tab character(\v)

One case this method might be more useful/comfortable over JSON.stringify(); is then if string needs to be quoted one can invoke quote() method directly on string object e.g. "some string".quote() from my point of view it is more semantically correct rather then using JSON.stringify() in all cases.

I'm not advocate of nonstandard methods but I thought it would be useful if Webkit has same functionality.

Regards,
Ioseb

[Oliver Hunt]

...
> Only difference from SJSON.stringify() I was able to find is encoding of
> vertical tab character(\v)
> 
> I'm not advocate of nonstandard methods but I thought it would be useful if
> Webkit has same functionality.
> 

If someone _really_ wanted this (noting that even if we provide it, it won't work in IE, Opera, or chrome) they can always do

String.prototype.quote = String.prototype.quote || JSON.stringify;

And they're done...

--Oliver

[Ioseb Dzmanashvili]

(In reply to comment #7)
> If someone _really_ wanted this (noting that even if we provide it, it won't
> work in IE, Opera, or chrome) they can always do
> 
> String.prototype.quote = String.prototype.quote || JSON.stringify;
> 
> And they're done...
> 

You are right, it is possible and correct to use existing functionality that way. I'm not against if this patch will be skipped especially if it adds nothing to Webkit at least I tried to do something useful :) :)

Regards,
Ioseb

[Oliver Hunt]

(In reply to comment #8)
> (In reply to comment #7)
> > If someone _really_ wanted this (noting that even if we provide it, it won't
> > work in IE, Opera, or chrome) they can always do
> > 
> > String.prototype.quote = String.prototype.quote || JSON.stringify;
> > 
> > And they're done...
> > 
> 
> You are right, it is possible and correct to use existing functionality that
> way. I'm not against if this patch will be skipped especially if it adds
> nothing to Webkit at least I tried to do something useful :) :)
> 
> Regards,
> Ioseb

:-D

There are a reasonable number of other low-lying things that we don't provide but that are useful -- looking through the ES5 spec for new APIs is a good place to start.

--Oliver

[Ioseb Dzmanashvili]

(In reply to comment #9)
> There are a reasonable number of other low-lying things that we don't provide
> but that are useful -- looking through the ES5 spec for new APIs is a good
> place to start.

ookay, thank you very much for discussion!

Best regards,
Ioseb

[Oliver Hunt]

Comment on attachment 42100 [details]
String.quote() method patch 

clearing review flag