Error reporting for SVG transform lists

Error reporting for SVG transform lists

Report too short/long argument lists to transform functions as well as
trailing garbage (separators). Could still be improved wrt invalid
transform functions and missing opening parenthesis.

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

[fs, 2016-02-01 21:03:32]

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

[pdr., 2016-02-02 05:06:23]

Looks great, just one question about the test output.

https://codereview.chromium.org/1659513002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/1659513002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/TestExpectations:71: crbug.com/231612
svg/custom/invalid-transforms.svg [ NeedsRebaseline ]
I see what looks like incorrect escaping (… instead of ') in the results for
these tests. Do you see that too? Possibly related to utf8 input--does the error
printing code handle both types of chars?

https://codereview.chromium.org/1659513002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGTransformList.cpp (right):

https://codereview.chromium.org/1659513002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGTransformList.cpp:125: const size_t
maxPossibleParams = required + optional;
This code was a bit hard for me to understand since the only two valid arg
counts are |required| or |required + optional|, but nothing between. WDYT about
renaming 'maxPossibleParams' to 'requiredWithOptional'?

[fs, 2016-02-02 09:30:35]

https://codereview.chromium.org/1659513002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/1659513002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/TestExpectations:71: crbug.com/231612
svg/custom/invalid-transforms.svg [ NeedsRebaseline ]
On 2016/02/02 at 05:06:23, pdr wrote:
> I see what looks like incorrect escaping (… instead of ') in the results for
these tests. Do you see that too? Possibly related to utf8 input--does the error
printing code handle both types of chars?

"…" is an ellipsis (U+2026) encoded in UTF-8, and it's added by the error
formatting code. Everything after that up to the end of the (elided - so
possibly one more ellipsis at the end) string will be escaped though.

https://codereview.chromium.org/1659513002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGTransformList.cpp (right):

https://codereview.chromium.org/1659513002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGTransformList.cpp:125: const size_t
maxPossibleParams = required + optional;
On 2016/02/02 at 05:06:23, pdr wrote:
> This code was a bit hard for me to understand since the only two valid arg
counts are |required| or |required + optional|, but nothing between. WDYT about
renaming 'maxPossibleParams' to 'requiredWithOptional'?

I agree it's somewhat confusing - even more so I'd say when reading the
condition at the end below - it helps to know what it's supposed to be doing...
Will send a separate CL to rename this.

[fs, 2016-02-02 10:25:55]

https://codereview.chromium.org/1659513002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGTransformList.cpp (right):

https://codereview.chromium.org/1659513002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGTransformList.cpp:125: const size_t
maxPossibleParams = required + optional;
On 2016/02/02 at 09:30:35, fs wrote:
> On 2016/02/02 at 05:06:23, pdr wrote:
> > This code was a bit hard for me to understand since the only two valid arg
counts are |required| or |required + optional|, but nothing between. WDYT about
renaming 'maxPossibleParams' to 'requiredWithOptional'?
> 
> I agree it's somewhat confusing - even more so I'd say when reading the
condition at the end below - it helps to know what it's supposed to be doing...
Will send a separate CL to rename this.

Is here: https://codereview.chromium.org/1655153002

[f(malita), 2016-02-02 13:49:04]

lgtm

[pdr., 2016-02-02 19:59:13]

On 2016/02/02 at 09:30:35, fs wrote:
>
https://codereview.chromium.org/1659513002/diff/20001/third_party/WebKit/Layo...
> File third_party/WebKit/LayoutTests/TestExpectations (right):
> 
>
https://codereview.chromium.org/1659513002/diff/20001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/TestExpectations:71: crbug.com/231612
svg/custom/invalid-transforms.svg [ NeedsRebaseline ]
> On 2016/02/02 at 05:06:23, pdr wrote:
> > I see what looks like incorrect escaping (… instead of ') in the results
for these tests. Do you see that too? Possibly related to utf8 input--does the
error printing code handle both types of chars?
> 
> "…" is an ellipsis (U+2026) encoded in UTF-8, and it's added by the error
formatting code. Everything after that up to the end of the (elided - so
possibly one more ellipsis at the end) string will be escaped though.

I'm still a little confused.. when I open the expected file, I see … instead
of an ellipsis. Are we mixing encodings in the expected files?

LGTM as-is because this is a bit of a tangent and not related to this patch.

[fs, 2016-02-02 20:22:20]

On 2016/02/02 at 19:59:13, pdr wrote:
> On 2016/02/02 at 09:30:35, fs wrote:
> >
https://codereview.chromium.org/1659513002/diff/20001/third_party/WebKit/Layo...
> > File third_party/WebKit/LayoutTests/TestExpectations (right):
> > 
> >
https://codereview.chromium.org/1659513002/diff/20001/third_party/WebKit/Layo...
> > third_party/WebKit/LayoutTests/TestExpectations:71: crbug.com/231612
svg/custom/invalid-transforms.svg [ NeedsRebaseline ]
> > On 2016/02/02 at 05:06:23, pdr wrote:
> > > I see what looks like incorrect escaping (… instead of ') in the results
for these tests. Do you see that too? Possibly related to utf8 input--does the
error printing code handle both types of chars?
> > 
> > "…" is an ellipsis (U+2026) encoded in UTF-8, and it's added by the error
formatting code. Everything after that up to the end of the (elided - so
possibly one more ellipsis at the end) string will be escaped though.
> 
> I'm still a little confused.. when I open the expected file, I see … instead
of an ellipsis. Are we mixing encodings in the expected files?

If I load

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

and force encoding to UTF-8 the ellipsis is displayed, so it's written as valid
UTF-8 at least. (I.e the encoding of the text document is auto-detected as
8859-1 - presumably we're at the mercy of steps 8-9 of
https://html.spec.whatwg.org/multipage/syntax.html#determining-the-character-...)

So I think that all is well.

[fs, 2016-02-02 20:25:20]

On 2016/02/02 at 20:22:20, fs wrote:
> On 2016/02/02 at 19:59:13, pdr wrote:
> > On 2016/02/02 at 09:30:35, fs wrote:
> > >
https://codereview.chromium.org/1659513002/diff/20001/third_party/WebKit/Layo...
> > > File third_party/WebKit/LayoutTests/TestExpectations (right):
> > > 
> > >
https://codereview.chromium.org/1659513002/diff/20001/third_party/WebKit/Layo...
> > > third_party/WebKit/LayoutTests/TestExpectations:71: crbug.com/231612
svg/custom/invalid-transforms.svg [ NeedsRebaseline ]
> > > On 2016/02/02 at 05:06:23, pdr wrote:
> > > > I see what looks like incorrect escaping (… instead of ') in the
results for these tests. Do you see that too? Possibly related to utf8
input--does the error printing code handle both types of chars?
> > > 
> > > "…" is an ellipsis (U+2026) encoded in UTF-8, and it's added by the
error formatting code. Everything after that up to the end of the (elided - so
possibly one more ellipsis at the end) string will be escaped though.
> > 
> > I'm still a little confused.. when I open the expected file, I see …
instead of an ellipsis. Are we mixing encodings in the expected files?
> 
> If I load
> 
>
https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel...
> 
> ...I.e the encoding of the text document is auto-detected as 8859-1...

Correction - win1252 not 8859-1

[fs, 2016-02-02 21:06:19]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2016-02-02 21:12:39]

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

[commit-bot: I haz the power, 2016-02-03 00:31:28]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-02-03 00:32:14]

Description was changed from

==========
Error reporting for SVG transform lists

Report too short/long argument lists to transform functions as well as
trailing garbage (separators). Could still be improved wrt invalid
transform functions and missing opening parenthesis.

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

to

==========
Error reporting for SVG transform lists

Report too short/long argument lists to transform functions as well as
trailing garbage (separators). Could still be improved wrt invalid
transform functions and missing opening parenthesis.

BUG=231612

Committed: https://crrev.com/c9683ab9c3bf9e6c782f6266bfab5e53bd9f7f2a
Cr-Commit-Position: refs/heads/master@{#373100}
==========

[commit-bot: I haz the power, 2016-02-03 00:32:15]

Patchset 2 (id:??) landed as
https://crrev.com/c9683ab9c3bf9e6c782f6266bfab5e53bd9f7f2a
Cr-Commit-Position: refs/heads/master@{#373100}