Extended error reporting for SVG attribute parsing

Extended error reporting for SVG attribute parsing

This CL extends the SVG attribute parsing error reporting functionality
to allow more precise (and hopefully helpful) reporting.
The main improvements consist of:

 1) More (precise) status codes
    Avoids generic error messages.

 2) Locus support
    Allows reducing the amount of context, which should make it easier
    to pinpoint the actual error. (Preferably the offending character
    should be highlighted somehow in the error message, but that is
    left as future work.)

To achieve this, the SVGParsingError enumeration is turned into a
thin wrapper class around a status code and a locus. The status codes
move to a new enumeration 'SVGStatus'.

Formatting of error messages are moved out of
SVGElement::reportAttributeParsingError and into SVGParsingError.cpp
(new file).

This CL start adding extended reporting to a few of the value classes:
SVGBoolean, SVGEnumeration and SVGPreserverAspectRatio; to illustrate
the mechanism. Further value classes will be annotated in later CLs.
For that reason the "generic" errors are kept in their current form - to
be removed as more value class parsers get converted.

BUG=231612
Committed: https://crrev.com/1611c9b2863c1ba9cd3067186090e8917a8a23c3
Cr-Commit-Position: refs/heads/master@{#370479}

[fs, 2016-01-15 16:34:30]

fs@opera.com changed reviewers:
+ fmalita@chromium.org, pdr@chromium.org

[fs, 2016-01-15 16:34:31]

Somethings that may be worth bikeshedding on here - like format and formulation
of the messages.

"Samples" can be found in the affected tests:

https://storage.googleapis.com/chromium-layout-test-archives/linux_chromium_r...

Most of this is just migration from enum to enum class, and interesting bits are
in SVGPreserveAspectRatio.cpp and SVGPaarsingError.{cpp,h}.

Happy bikeshedding!

(Looks like I missed a PLATFORM_EXPORT for the new function in JSONValues.h,
will add that.)

[f(malita), 2016-01-15 20:03:38]

https://codereview.chromium.org/1588993005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGParsingError.cpp (right):

https://codereview.chromium.org/1588993005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGParsingError.cpp:60: ASSERT(locus >= 0 &&
locus <= value.length());
nit: first part is obvious for unsigned.

https://codereview.chromium.org/1588993005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGParsingError.cpp:66: unsigned contextEnd =
std::min(locus + kContext, value.length());
nit: I think these are safe, but just for sanity, maybe

  ASSERT(contextStart <= contextEnd);
  ASSERT(contextEnd <= value.length());
?

https://codereview.chromium.org/1588993005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGParsingError.cpp:69:
escapeStringForJSON(value.string().substring(contextStart, contextEnd -
contextStart), &builder);
So the parse error is somewhere within this (potentially) 32-char string. 
Ideally we'd have some indication of exactly which char the parser choked on,
but not sure what's the best way for that.  We can't exactly add another line
with a caret pointing up at it (^, a-la clang compile errors), and adding
arbitrary markers around it (say "foo>Z<bar") would just be... arbitrary.  So
yeah, no bright ideas here.

[fs, 2016-01-15 20:46:15]

https://codereview.chromium.org/1588993005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGParsingError.cpp (right):

https://codereview.chromium.org/1588993005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGParsingError.cpp:60: ASSERT(locus >= 0 &&
locus <= value.length());
On 2016/01/15 at 20:03:38, f(malita) wrote:
> nit: first part is obvious for unsigned.

Indeed, will fix.

https://codereview.chromium.org/1588993005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGParsingError.cpp:66: unsigned contextEnd =
std::min(locus + kContext, value.length());
On 2016/01/15 at 20:03:38, f(malita) wrote:
> nit: I think these are safe, but just for sanity, maybe
> 
>   ASSERT(contextStart <= contextEnd);
>   ASSERT(contextEnd <= value.length());
> ?

String::substring is pretty resilient to weird ranges (and these are unsigned,
so underflow won't hurt any kittens.) I'll throw in the asserts for good measure
though.

https://codereview.chromium.org/1588993005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGParsingError.cpp:69:
escapeStringForJSON(value.string().substring(contextStart, contextEnd -
contextStart), &builder);
On 2016/01/15 at 20:03:38, f(malita) wrote:
> So the parse error is somewhere within this (potentially) 32-char string. 
Ideally we'd have some indication of exactly which char the parser choked on,
but not sure what's the best way for that.  We can't exactly add another line
with a caret pointing up at it (^, a-la clang compile errors), and adding
arbitrary markers around it (say "foo>Z<bar") would just be... arbitrary.  So
yeah, no bright ideas here.

Yeah, initially I had this displaying a caret using a "pipe" ('|') and then
played around with some fancy unicode characters (like U+21e5 [1]), but sort of
concluded that that might just be confusing. I tried just adding ', at <index>
"..."' to the message but didn't quite like that either. My best idea (and plan)
so far is to add a "highlight range" to ConsoleMessage and then apply those in
the console rendering. Haven't looked into that yet though, and it's bound to
slip onto the backburner (I need to get off this side-track soonish =)).

[1] http://www.fileformat.info/info/unicode/char/21e5/index.htm

[pdr., 2016-01-15 20:56:53]

https://codereview.chromium.org/1588993005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGParsingError.h (right):

https://codereview.chromium.org/1588993005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGParsingError.h:37: enum class SVGStatus {
Bikeshed: SVGParseStatus? Or anything a little less generic.

https://codereview.chromium.org/1588993005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGParsingError.h:78: // long values will be
output in their entirety. That should however be
will *not* be output

https://codereview.chromium.org/1588993005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGParsingError.h:84: unsigned m_locus :
kLocusBits;
Can you add a comment about what locus is?

[fs, 2016-01-15 21:05:13]

https://codereview.chromium.org/1588993005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGParsingError.h (right):

https://codereview.chromium.org/1588993005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGParsingError.h:37: enum class SVGStatus {
On 2016/01/15 at 20:56:53, pdr wrote:
> Bikeshed: SVGParseStatus? Or anything a little less generic.

I guess we could go with that - I was trying for something not too lengthy (the
main downside of enum classes is their qualification...)

https://codereview.chromium.org/1588993005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGParsingError.h:78: // long values will be
output in their entirety. That should however be
On 2016/01/15 at 20:56:53, pdr wrote:
> will *not* be output

It's actually correct as is, but I take it that I should clarify that this is
the case when the locus requires more than 24 bits of storage.

https://codereview.chromium.org/1588993005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGParsingError.h:84: unsigned m_locus :
kLocusBits;
On 2016/01/15 at 20:56:53, pdr wrote:
> Can you add a comment about what locus is?

And here I thought this was as obvious a name you could get... =)

[fs, 2016-01-18 13:56:51]

https://codereview.chromium.org/1588993005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGParsingError.cpp (right):

https://codereview.chromium.org/1588993005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGParsingError.cpp:60: ASSERT(locus >= 0 &&
locus <= value.length());
On 2016/01/15 at 20:03:38, f(malita) wrote:
> nit: first part is obvious for unsigned.

Done.

https://codereview.chromium.org/1588993005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGParsingError.cpp:66: unsigned contextEnd =
std::min(locus + kContext, value.length());
On 2016/01/15 at 20:03:38, f(malita) wrote:
> nit: I think these are safe, but just for sanity, maybe
> 
>   ASSERT(contextStart <= contextEnd);
>   ASSERT(contextEnd <= value.length());
> ?

Added.

https://codereview.chromium.org/1588993005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGParsingError.h (right):

https://codereview.chromium.org/1588993005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGParsingError.h:37: enum class SVGStatus {
On 2016/01/15 at 20:56:53, pdr wrote:
> Bikeshed: SVGParseStatus? Or anything a little less generic.

Done.

https://codereview.chromium.org/1588993005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGParsingError.h:78: // long values will be
output in their entirety. That should however be
On 2016/01/15 at 21:05:12, fs wrote:
> On 2016/01/15 at 20:56:53, pdr wrote:
> > will *not* be output
> 
> It's actually correct as is, but I take it that I should clarify that this is
the case when the locus requires more than 24 bits of storage.

Added an extra sentence and renamed the helper to checkLocus.

https://codereview.chromium.org/1588993005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGParsingError.h:84: unsigned m_locus :
kLocusBits;
On 2016/01/15 at 20:56:53, pdr wrote:
> Can you add a comment about what locus is?

Comment added.

[f(malita), 2016-01-18 14:34:44]

lgtm

[fs, 2016-01-20 15:40:19]

pdr: ok?

[pdr., 2016-01-20 18:41:05]

pdr: ok!

LGTM

[fs, 2016-01-20 19:01:26]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2016-01-20 19:04:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1588993005/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1588993005/140001

[commit-bot: I haz the power, 2016-01-20 20:40:36]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-01-20 20:41:46]

Description was changed from

==========
Extended error reporting for SVG attribute parsing

This CL extends the SVG attribute parsing error reporting functionality
to allow more precise (and hopefully helpful) reporting.
The main improvements consist of:

 1) More (precise) status codes
    Avoids generic error messages.

 2) Locus support
    Allows reducing the amount of context, which should make it easier
    to pinpoint the actual error. (Preferably the offending character
    should be highlighted somehow in the error message, but that is
    left as future work.)

To achieve this, the SVGParsingError enumeration is turned into a
thin wrapper class around a status code and a locus. The status codes
move to a new enumeration 'SVGStatus'.

Formatting of error messages are moved out of
SVGElement::reportAttributeParsingError and into SVGParsingError.cpp
(new file).

This CL start adding extended reporting to a few of the value classes:
SVGBoolean, SVGEnumeration and SVGPreserverAspectRatio; to illustrate
the mechanism. Further value classes will be annotated in later CLs.
For that reason the "generic" errors are kept in their current form - to
be removed as more value class parsers get converted.

BUG=231612
==========

to

==========
Extended error reporting for SVG attribute parsing

This CL extends the SVG attribute parsing error reporting functionality
to allow more precise (and hopefully helpful) reporting.
The main improvements consist of:

 1) More (precise) status codes
    Avoids generic error messages.

 2) Locus support
    Allows reducing the amount of context, which should make it easier
    to pinpoint the actual error. (Preferably the offending character
    should be highlighted somehow in the error message, but that is
    left as future work.)

To achieve this, the SVGParsingError enumeration is turned into a
thin wrapper class around a status code and a locus. The status codes
move to a new enumeration 'SVGStatus'.

Formatting of error messages are moved out of
SVGElement::reportAttributeParsingError and into SVGParsingError.cpp
(new file).

This CL start adding extended reporting to a few of the value classes:
SVGBoolean, SVGEnumeration and SVGPreserverAspectRatio; to illustrate
the mechanism. Further value classes will be annotated in later CLs.
For that reason the "generic" errors are kept in their current form - to
be removed as more value class parsers get converted.

BUG=231612

Committed: https://crrev.com/1611c9b2863c1ba9cd3067186090e8917a8a23c3
Cr-Commit-Position: refs/heads/master@{#370479}
==========

[commit-bot: I haz the power, 2016-01-20 20:41:47]

Patchset 8 (id:??) landed as
https://crrev.com/1611c9b2863c1ba9cd3067186090e8917a8a23c3
Cr-Commit-Position: refs/heads/master@{#370479}