[SVG] SVGAnimatedString{,List} migration to new SVG property impl.

[SVG] SVGAnimatedString{,List} migration to new SVG property impl.

This CL replaces SVGAnimatedString{,List} with NewSVGProperty based implementation.

The original SVGStringList had a hack where it parsed empty string to a list with a single empty string [''], but this hack is removed.

This CL also fixes a bug in SVGTest where 'systemLanguage' constraint was always rejected when multiple languages were specified.

For details, see design doc: https://docs.google.com/document/d/1bg7CUyUszqdwmENY3JX6_PoQD6uHRCNcRPJMlC4qlkw/edit?usp=sharing

BUG=308818
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=166227

[kouhei (in TOK), 2014-01-29 02:15:38]

haraken, pdr: Would you take a look?

[haraken, 2014-01-29 05:17:59]

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGCurso...
File Source/core/svg/SVGCursorElement.cpp (left):

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGCurso...
Source/core/svg/SVGCursorElement.cpp:87: ||
SVGURIReference::parseAttribute(name, value)) {

Why can you remove SVGURIReference::parseAttribute?

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGFEIma...
File Source/core/svg/SVGFEImageElement.cpp (right):

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGFEIma...
Source/core/svg/SVGFEImageElement.cpp:136: } else if
(name.matches(XLinkNames::hrefAttr)) {

Would you add a comment why you're using .matches instead of == ? (You can add
the comment here or in XLinkNames.)

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGForei...
File Source/core/svg/SVGForeignObjectElement.cpp (right):

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGForei...
Source/core/svg/SVGForeignObjectElement.cpp:89: else

You need to add a check for name.matches(SVGLinks::hrefAttr) here.

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGGlyph...
File Source/core/svg/SVGGlyphRefElement.cpp (right):

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGGlyph...
Source/core/svg/SVGGlyphRefElement.cpp:71: // FIXME: We need some error handling
here.

What does this FIXME imply? I wonder why this code doesn't have the following
pattern:

SVGParsingError parseError = NoError;
if (...)
  ...;
else if (...)
  ...;
else
  ASSERT_NOT_REACHED();

reportAttributeParsingError(parseError, name, value);

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGMPath...
File Source/core/svg/SVGMPathElement.cpp (right):

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGMPath...
Source/core/svg/SVGMPathElement.cpp:114: if (!isSupportedAttribute(name)) {

Can you move this check to line 121?

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGScrip...
File Source/core/svg/SVGScriptElement.cpp (right):

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGScrip...
Source/core/svg/SVGScriptElement.cpp:76: if (name == SVGNames::typeAttr) {

Can you move this check to line 88?

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGScrip...
Source/core/svg/SVGScriptElement.cpp:81: if (name == HTMLNames::onerrorAttr) {

Ditto.

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGStrin...
File Source/core/svg/SVGString.cpp (right):

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGStrin...
Source/core/svg/SVGString.cpp:44: // No paced animations for strings.

It's confusing to me which elements can animate and which elements cannot
animate. Probably we might want to implement 'virtual bool isAnimated()' in
NewSVGPropertyBase and override the isAnimated() in each subclass.

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGStrin...
File Source/core/svg/SVGStringList.cpp (right):

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGStrin...
Source/core/svg/SVGStringList.cpp:50: ASSERT(index < m_values.size());

You don't need this.

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGStrin...
File Source/core/svg/SVGStringList.h (right):

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGStrin...
Source/core/svg/SVGStringList.h:46: // SVGStringList operates directly on
DOMString.

Would you elaborate on the relationship between SVGStringList, SVGString and
DOMString here?

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGStrin...
File Source/core/svg/SVGStringListTearOff.h (right):

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGStrin...
Source/core/svg/SVGStringListTearOff.h:61: target()->clear();

Don't you need commitChange() here?

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGStrin...
Source/core/svg/SVGStringListTearOff.h:110: String removedItem =
target()->removeItem(index, exceptionState);

Don't you need to check isImmutable() before removing the item?

[kouhei (in TOK), 2014-01-30 08:18:55]

haraken: PTAL

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGCurso...
File Source/core/svg/SVGCursorElement.cpp (left):

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGCurso...
Source/core/svg/SVGCursorElement.cpp:87: ||
SVGURIReference::parseAttribute(name, value)) {
On 2014/01/29 05:18:00, haraken wrote:
> 
> Why can you remove SVGURIReference::parseAttribute?

It was a helper for handling |m_href|. I expanded it here.

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGFEIma...
File Source/core/svg/SVGFEImageElement.cpp (right):

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGFEIma...
Source/core/svg/SVGFEImageElement.cpp:136: } else if
(name.matches(XLinkNames::hrefAttr)) {
On 2014/01/29 05:18:00, haraken wrote:
> 
> Would you add a comment why you're using .matches instead of == ? (You can add
> the comment here or in XLinkNames.)

Discussed offline. This method is going away anyway.

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGForei...
File Source/core/svg/SVGForeignObjectElement.cpp (right):

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGForei...
Source/core/svg/SVGForeignObjectElement.cpp:89: else
On 2014/01/29 05:18:00, haraken wrote:
> 
> You need to add a check for name.matches(SVGLinks::hrefAttr) here.
Actually, it seems like its a bug that SVGForeignObjectElement has |m_href|.
removed.

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGGlyph...
File Source/core/svg/SVGGlyphRefElement.cpp (right):

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGGlyph...
Source/core/svg/SVGGlyphRefElement.cpp:71: // FIXME: We need some error handling
here.
On 2014/01/29 05:18:00, haraken wrote:
> 
> What does this FIXME imply? I wonder why this code doesn't have the following
> pattern:
> 
> SVGParsingError parseError = NoError;
> if (...)
>   ...;
> else if (...)
>   ...;
> else
>   ASSERT_NOT_REACHED();
> 
> reportAttributeParsingError(parseError, name, value);

I will do that when I land SVGNumber{,List} migration CL. I'd like to keep as
this is in this CL.

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGMPath...
File Source/core/svg/SVGMPathElement.cpp (right):

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGMPath...
Source/core/svg/SVGMPathElement.cpp:114: if (!isSupportedAttribute(name)) {
On 2014/01/29 05:18:00, haraken wrote:
> 
> Can you move this check to line 121?

Done.

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGStrin...
File Source/core/svg/SVGString.cpp (right):

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGStrin...
Source/core/svg/SVGString.cpp:44: // No paced animations for strings.
On 2014/01/29 05:18:00, haraken wrote:
> 
> It's confusing to me which elements can animate and which elements cannot
> animate. Probably we might want to implement 'virtual bool isAnimated()' in
> NewSVGPropertyBase and override the isAnimated() in each subclass.

"Paced" animations are not supported, but normal discrete animations are
supported for SVGString.

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGStrin...
File Source/core/svg/SVGStringList.cpp (right):

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGStrin...
Source/core/svg/SVGStringList.cpp:50: ASSERT(index < m_values.size());
On 2014/01/29 05:18:00, haraken wrote:
> 
> You don't need this.

Done.

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGStrin...
File Source/core/svg/SVGStringList.h (right):

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGStrin...
Source/core/svg/SVGStringList.h:46: // SVGStringList operates directly on
DOMString.
On 2014/01/29 05:18:00, haraken wrote:
> 
> Would you elaborate on the relationship between SVGStringList, SVGString and
> DOMString here?

Done.

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGStrin...
File Source/core/svg/SVGStringListTearOff.h (right):

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGStrin...
Source/core/svg/SVGStringListTearOff.h:61: target()->clear();
On 2014/01/29 05:18:00, haraken wrote:
> 
> Don't you need commitChange() here?

Yes, I do. Thanks for the catch.

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGStrin...
Source/core/svg/SVGStringListTearOff.h:110: String removedItem =
target()->removeItem(index, exceptionState);
On 2014/01/29 05:18:00, haraken wrote:
> 
> Don't you need to check isImmutable() before removing the item?

Done.

[haraken, 2014-01-30 11:00:07]

I scanned the CL again, LGTM.

This CL is doing almost mechanical changes. If all tests pass, it would be fine
to go.

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGStrin...
File Source/core/svg/SVGString.cpp (right):

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGStrin...
Source/core/svg/SVGString.cpp:44: // No paced animations for strings.
On 2014/01/30 08:18:56, kouhei wrote:
> On 2014/01/29 05:18:00, haraken wrote:
> > 
> > It's confusing to me which elements can animate and which elements cannot
> > animate. Probably we might want to implement 'virtual bool isAnimated()' in
> > NewSVGPropertyBase and override the isAnimated() in each subclass.
> 
> "Paced" animations are not supported, but normal discrete animations are
> supported for SVGString.

Makes sense. Then we might want to have isPacedAnimationSupported() and
isDiscreteAnimationSupported(). Then you could use these methods to add ASSERTs.
(You don't need to do that in this CL though.)

[fs, 2014-01-30 12:22:31]

(Patch Set 3 looks like it would be "[SVG] SVGAnimatedPathSeg{,List}
migration..." - rebase/upload gone awry?)

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGForei...
File Source/core/svg/SVGForeignObjectElement.cpp (right):

https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGForei...
Source/core/svg/SVGForeignObjectElement.cpp:89: else
On 2014/01/30 08:18:56, kouhei wrote:
> On 2014/01/29 05:18:00, haraken wrote:
> > 
> > You need to add a check for name.matches(SVGLinks::hrefAttr) here.
> Actually, it seems like its a bug that SVGForeignObjectElement has |m_href|.
> removed.

Probably a benign such if so - there doesn't appear to be any handling of that
case (xlink:href on 'foreignObject' was/is a SVG Tiny 1.2 feature.)

[kouhei (in TOK), 2014-01-31 04:52:32]

On 2014/01/30 12:22:31, fs wrote:
> (Patch Set 3 looks like it would be "[SVG] SVGAnimatedPathSeg{,List}
> migration..." - rebase/upload gone awry?)
> 
>
https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGForei...
> File Source/core/svg/SVGForeignObjectElement.cpp (right):
> 
>
https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGForei...
> Source/core/svg/SVGForeignObjectElement.cpp:89: else
> On 2014/01/30 08:18:56, kouhei wrote:
> > On 2014/01/29 05:18:00, haraken wrote:
> > > 
> > > You need to add a check for name.matches(SVGLinks::hrefAttr) here.
> > Actually, it seems like its a bug that SVGForeignObjectElement has |m_href|.
> > removed.
> 
> Probably a benign such if so - there doesn't appear to be any handling of that
> case (xlink:href on 'foreignObject' was/is a SVG Tiny 1.2 feature.)

Thanks for the catch!

[kouhei (in TOK), 2014-01-31 04:52:33]

On 2014/01/30 12:22:31, fs wrote:
> (Patch Set 3 looks like it would be "[SVG] SVGAnimatedPathSeg{,List}
> migration..." - rebase/upload gone awry?)
> 
>
https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGForei...
> File Source/core/svg/SVGForeignObjectElement.cpp (right):
> 
>
https://codereview.chromium.org/148173018/diff/20001/Source/core/svg/SVGForei...
> Source/core/svg/SVGForeignObjectElement.cpp:89: else
> On 2014/01/30 08:18:56, kouhei wrote:
> > On 2014/01/29 05:18:00, haraken wrote:
> > > 
> > > You need to add a check for name.matches(SVGLinks::hrefAttr) here.
> > Actually, it seems like its a bug that SVGForeignObjectElement has |m_href|.
> > removed.
> 
> Probably a benign such if so - there doesn't appear to be any handling of that
> case (xlink:href on 'foreignObject' was/is a SVG Tiny 1.2 feature.)

Thanks for the catch!

[commit-bot: I haz the power, 2014-01-31 04:55:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/148173018/60001

[commit-bot: I haz the power, 2014-01-31 05:23:22]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-31 05:23:47]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-31 05:27:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/148173018/80001

[commit-bot: I haz the power, 2014-01-31 07:59:24]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2014-01-31 07:59:28]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-31 07:59:30]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-31 08:00:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/148173018/80001

[commit-bot: I haz the power, 2014-01-31 11:04:14]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2014-01-31 11:04:20]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-31 11:04:22]

CQ bit was unchecked on CL. Ignoring.

[kouhei (in TOK), 2014-01-31 14:21:40]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2014-01-31 14:21:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/148173018/80001

[commit-bot: I haz the power, 2014-01-31 18:05:30]

Change committed as 166227

[commit-bot: I haz the power, 2014-01-31 18:05:34]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-31 18:05:37]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-31 18:05:37]

CQ bit was unchecked on CL. Ignoring.