Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(135)

Issue 1588993005: Extended error reporting for SVG attribute parsing (Closed)

Created:
4 years, 11 months ago by fs
Modified:
4 years, 11 months ago
Reviewers:
pdr., f(malita)
CC:
darktears, blink-reviews, blink-reviews-animation_chromium.org, chromium-reviews, krit, Eric Willigers, f(malita), fs, gyuyoung2, kinuko+watch, kouhei+svg_chromium.org, pdr+svgwatchlist_chromium.org, rjwright, rwlbuis, Stephen Chennney, shans
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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}

Patch Set 1 #

Patch Set 2 : Fix assert. #

Patch Set 3 : TestExpectations #

Total comments: 17

Patch Set 4 : SVGStatus->SVGParseStatus; ASSERTs; comment #

Patch Set 5 : One more comment. #

Patch Set 6 : Rename narrowLocus to checkLocus #

Patch Set 7 : kNoLocus should be unsigned #

Patch Set 8 : Add PLATFORM_EXPORT #

Unified diffs Side-by-side diffs Delta from patch set Stats (+276 lines, -117 lines) Patch
M third_party/WebKit/LayoutTests/TestExpectations View 1 2 1 chunk +8 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/core.gypi View 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/core/svg/SVGAngle.cpp View 1 2 3 2 chunks +5 lines, -5 lines 0 comments Download
M third_party/WebKit/Source/core/svg/SVGAngleTearOff.cpp View 1 2 3 1 chunk +3 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/core/svg/SVGAnimatedLength.cpp View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/svg/SVGAnimationElement.cpp View 1 2 3 3 chunks +4 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/core/svg/SVGBoolean.cpp View 1 2 3 1 chunk +3 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/core/svg/SVGElement.cpp View 1 2 3 2 chunks +2 lines, -17 lines 0 comments Download
M third_party/WebKit/Source/core/svg/SVGEnumeration.cpp View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/svg/SVGFEConvolveMatrixElement.cpp View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/svg/SVGFitToViewBox.cpp View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/svg/SVGInteger.cpp View 1 2 3 2 chunks +3 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/core/svg/SVGIntegerOptionalInteger.cpp View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/svg/SVGLength.cpp View 1 2 3 2 chunks +5 lines, -6 lines 0 comments Download
M third_party/WebKit/Source/core/svg/SVGLengthList.cpp View 1 2 3 1 chunk +3 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/core/svg/SVGLengthTearOff.cpp View 1 2 3 1 chunk +3 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/core/svg/SVGNumber.cpp View 1 2 3 3 chunks +5 lines, -5 lines 0 comments Download
M third_party/WebKit/Source/core/svg/SVGNumberList.cpp View 1 2 3 2 chunks +3 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/core/svg/SVGNumberOptionalNumber.cpp View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/svg/SVGParsingError.h View 1 2 3 4 5 6 1 chunk +60 lines, -3 lines 0 comments Download
A third_party/WebKit/Source/core/svg/SVGParsingError.cpp View 1 2 3 1 chunk +105 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/svg/SVGPath.cpp View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/svg/SVGPathElement.cpp View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/svg/SVGPoint.cpp View 1 2 3 2 chunks +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/svg/SVGPointList.cpp View 1 2 3 2 chunks +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/svg/SVGPreserveAspectRatio.h View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/svg/SVGPreserveAspectRatio.cpp View 1 2 3 7 chunks +26 lines, -28 lines 0 comments Download
M third_party/WebKit/Source/core/svg/SVGRect.cpp View 1 2 3 2 chunks +3 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/core/svg/SVGString.h View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/svg/SVGStringList.cpp View 1 2 3 2 chunks +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/svg/SVGTransformList.cpp View 1 2 3 2 chunks +3 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/platform/JSONValues.h View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/JSONValues.cpp View 2 chunks +7 lines, -2 lines 0 comments Download

Messages

Total messages: 15 (3 generated)
fs
Somethings that may be worth bikeshedding on here - like format and formulation of the ...
4 years, 11 months ago (2016-01-15 16:34:31 UTC) #2
f(malita)
https://codereview.chromium.org/1588993005/diff/40001/third_party/WebKit/Source/core/svg/SVGParsingError.cpp File third_party/WebKit/Source/core/svg/SVGParsingError.cpp (right): https://codereview.chromium.org/1588993005/diff/40001/third_party/WebKit/Source/core/svg/SVGParsingError.cpp#newcode60 third_party/WebKit/Source/core/svg/SVGParsingError.cpp:60: ASSERT(locus >= 0 && locus <= value.length()); nit: first ...
4 years, 11 months ago (2016-01-15 20:03:38 UTC) #3
fs
https://codereview.chromium.org/1588993005/diff/40001/third_party/WebKit/Source/core/svg/SVGParsingError.cpp File third_party/WebKit/Source/core/svg/SVGParsingError.cpp (right): https://codereview.chromium.org/1588993005/diff/40001/third_party/WebKit/Source/core/svg/SVGParsingError.cpp#newcode60 third_party/WebKit/Source/core/svg/SVGParsingError.cpp:60: ASSERT(locus >= 0 && locus <= value.length()); On 2016/01/15 ...
4 years, 11 months ago (2016-01-15 20:46:15 UTC) #4
pdr.
https://codereview.chromium.org/1588993005/diff/40001/third_party/WebKit/Source/core/svg/SVGParsingError.h File third_party/WebKit/Source/core/svg/SVGParsingError.h (right): https://codereview.chromium.org/1588993005/diff/40001/third_party/WebKit/Source/core/svg/SVGParsingError.h#newcode37 third_party/WebKit/Source/core/svg/SVGParsingError.h:37: enum class SVGStatus { Bikeshed: SVGParseStatus? Or anything a ...
4 years, 11 months ago (2016-01-15 20:56:53 UTC) #5
fs
https://codereview.chromium.org/1588993005/diff/40001/third_party/WebKit/Source/core/svg/SVGParsingError.h File third_party/WebKit/Source/core/svg/SVGParsingError.h (right): https://codereview.chromium.org/1588993005/diff/40001/third_party/WebKit/Source/core/svg/SVGParsingError.h#newcode37 third_party/WebKit/Source/core/svg/SVGParsingError.h:37: enum class SVGStatus { On 2016/01/15 at 20:56:53, pdr ...
4 years, 11 months ago (2016-01-15 21:05:13 UTC) #6
fs
https://codereview.chromium.org/1588993005/diff/40001/third_party/WebKit/Source/core/svg/SVGParsingError.cpp File third_party/WebKit/Source/core/svg/SVGParsingError.cpp (right): https://codereview.chromium.org/1588993005/diff/40001/third_party/WebKit/Source/core/svg/SVGParsingError.cpp#newcode60 third_party/WebKit/Source/core/svg/SVGParsingError.cpp:60: ASSERT(locus >= 0 && locus <= value.length()); On 2016/01/15 ...
4 years, 11 months ago (2016-01-18 13:56:51 UTC) #7
f(malita)
lgtm
4 years, 11 months ago (2016-01-18 14:34:44 UTC) #8
fs
pdr: ok?
4 years, 11 months ago (2016-01-20 15:40:19 UTC) #9
pdr.
pdr: ok! LGTM
4 years, 11 months ago (2016-01-20 18:41:05 UTC) #10
commit-bot: I haz the power
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
4 years, 11 months ago (2016-01-20 19:04:18 UTC) #12
commit-bot: I haz the power
Committed patchset #8 (id:140001)
4 years, 11 months ago (2016-01-20 20:40:36 UTC) #13
commit-bot: I haz the power
4 years, 11 months ago (2016-01-20 20:41:47 UTC) #15
Message was sent while issue was closed.
Patchset 8 (id:??) landed as
https://crrev.com/1611c9b2863c1ba9cd3067186090e8917a8a23c3
Cr-Commit-Position: refs/heads/master@{#370479}

Powered by Google App Engine
This is Rietveld 408576698