[webkit-reviews] review denied: [Bug 90667] [JSC] HTML extensions to String.prototype should escape " as " in argument values : [Attachment 172746] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Christophe Dumez
<christophe.dumez at intel.com>'s request for review:
Bug 90667: [JSC] HTML extensions to String.prototype should escape " as &quot;
in argument values
https://bugs.webkit.org/show_bug.cgi?id=90667

Attachment 172746: Patch
https://bugs.webkit.org/attachment.cgi?id=172746&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=172746&action=review


The patch itself looks good. I think the tests could be better.

> LayoutTests/ChangeLog:14
> +	   * fast/js/script-tests/string-prototype-escape-attribute.js: Added.
> +	   * fast/js/string-prototype-escape-attribute-expected.txt: Added.
> +	   * fast/js/string-prototype-escape-attribute.html: Added.

I think the test names are not descriptive enough.
Wouldn't it be better to create one test for each of those functions? A quick
grep in fast/js tells me their coverage may be bad, but maybe they are tested
elsewhere?

> LayoutTests/fast/js/script-tests/string-prototype-escape-attribute.js:5
> +shouldBe("'_'.anchor('\x22')", '"<a name=\\"&quot;\\">_</a>"');

Why use \x22 instead of "? it looks more confusing.
You should also have tests with the quote being with some text and
pathologically bad examples, e.g:
"_".anchor('" href="http://evil.com"')