[webkit-reviews] review denied: [Bug 63339] JSON errors should be informative : [Attachment 101791] Proposed patch

[bugzilla-daemon at webkit.org]

Oliver Hunt <oliver at apple.com> has denied Juan C. Montemayor
<jmont at apple.com>'s request for review:
Bug 63339: JSON errors should be informative
https://bugs.webkit.org/show_bug.cgi?id=63339

Attachment 101791: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=101791&action=review

------- Additional Comments from Oliver Hunt <oliver at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=101791&action=review


> Source/JavaScriptCore/runtime/LiteralParser.cpp:339
> +		       m_lexErrorMessage = "JSON Lexer error: Unable to read
string";

Invalid escape character %c

> Source/JavaScriptCore/runtime/LiteralParser.cpp:346
> +	   m_lexErrorMessage = "JSON Lexer error: Unterminated string";

No lexer, only parse errors.

"JSON Parse error: "  is repeated everywhere, i think you should just drop that
prefix, and add it at the end when you're making the error message.

> Source/JavaScriptCore/runtime/LiteralParser.cpp:436
> +	       m_lexErrorMessage = "JSON Lexer error: Exponent symbols should
be followed by an optional '+' or '-' and then by at least one number";

Should just be JSON Parse error -- the lexer vs. parser difference isn't really
relevant to develoeprs

> Source/JavaScriptCore/runtime/LiteralParser.cpp:634
> +		       case TokRBracket:
> +			   m_parseErrorMessage = "JSON Parse error: Could not
parse right bracket";
> +			   return JSValue();
> +		       case TokRBrace:
> +			   m_parseErrorMessage = "JSON Parse error: Could not
parse right brace";
> +			   return JSValue();
> +		       case TokIdentifier:
> +			   m_parseErrorMessage = String::format("JSON Parse
error: Could not parse \"%s\"",
UString(m_lexer.currentToken().stringToken).ascii().data()).impl();
> +			   return JSValue();
> +		       case TokColon:
> +			   m_parseErrorMessage = "JSON Parse error: Could not
parse colon";
> +			   return JSValue();
> +		       case TokLParen:
> +			   m_parseErrorMessage = "JSON Parse error: Could not
parse left parenthesis";
> +			   return JSValue();
> +		       case TokRParen:
> +			   m_parseErrorMessage = "JSON Parse error: Could not
parse right parenthesis";
> +			   return JSValue();
> +		       case TokComma:
> +			   m_parseErrorMessage = "JSON Parse error: Could not
parse comma";
> +			   return JSValue();
> +		       case TokDot:
> +			   m_parseErrorMessage = "JSON Parse error: Could not
parse dot";
> +			   return JSValue();
> +		       case TokAssign:
> +			   m_parseErrorMessage = "JSON Parse error: Could not
parse assignment";
> +			   return JSValue();
> +		       case TokSemi:
> +			   m_parseErrorMessage = "JSON Parse error: Could not
parse semicolon";
> +			   return JSValue();

These error messages are icky, it should be something along the lines of
"Unexpected token <blah>"

> Source/JavaScriptCore/runtime/LiteralParser.cpp:635
> +		       case TokEnd:

This should be unexpected EOF or some such.

> Source/JavaScriptCore/runtime/LiteralParser.cpp:657
> +			   m_parseErrorMessage = "JSON statement must begin
with a [, number, string, or (";

I'd rather go with unexpected token <blah>

> Source/JavaScriptCore/runtime/LiteralParser.cpp:664
> +		       m_parseErrorMessage = "JSON Parse error: Expected right
parenthesis after property name in expression";

This error is bogus.  It's testing to ensure that you have balanced the
parenthesis.  However you can't actually test for it as this path is only taken
when preparsing content in the hope it's json.