SVGElement::parseAttribute resolves property via property map (Step 1)

SVGElement::parseAttribute resolves property via property map (Step 1)

This CL introduces |SVGElement::parseAttributeNew|, a new
|SVGElement::parseAttribute| implementation based on property map.

Before this patch, |SVG(.*)Element::parseAttribute| used chained if stmts
to resolve SVGAnimatedProperty. This patch changes it to resolve using
property map.

This is to replace the existing |SVGElement::parseAttribute| implementation,
but as there are many callers, we would like to proceed this incrementally.
a) Introduce |SVGElement::parseAttributeNew|
b) Convert all overrides |SVGElement::parseAttribute| to
   a |parseAttributeNew| call.
c) Replace existing |parseAttribute| with |parseAttributeNew|, and remove all
   overrides simply calling |parseAttributeNew|

This patch is (a). This patch also converts |SVG{Graphics,Rect}Element::parseAttribute| to start with (b).

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

[kouhei (in TOK), 2014-07-28 03:44:59]

Would you take a look?

https://codereview.chromium.org/421863002/diff/1/Source/core/svg/SVGGraphicsE...
File Source/core/svg/SVGGraphicsElement.cpp (left):

https://codereview.chromium.org/421863002/diff/1/Source/core/svg/SVGGraphicsE...
Source/core/svg/SVGGraphicsElement.cpp:191: else if
(SVGTests::parseAttribute(name, value))
All attributes used in SVGTests are already registered in the map.

[haraken, 2014-07-28 04:49:14]

https://codereview.chromium.org/421863002/diff/20001/Source/core/svg/SVGRectE...
File Source/core/svg/SVGRectElement.cpp (left):

https://codereview.chromium.org/421863002/diff/20001/Source/core/svg/SVGRectE...
Source/core/svg/SVGRectElement.cpp:70: if (!isSupportedAttribute(name))

Just help me understand: How is isSupportedAttribute(name) handled in the
parseAttributeNew()?

[kouhei (in TOK), 2014-07-28 04:51:29]

https://codereview.chromium.org/421863002/diff/20001/Source/core/svg/SVGRectE...
File Source/core/svg/SVGRectElement.cpp (left):

https://codereview.chromium.org/421863002/diff/20001/Source/core/svg/SVGRectE...
Source/core/svg/SVGRectElement.cpp:70: if (!isSupportedAttribute(name))
On 2014/07/28 04:49:13, haraken wrote:
> 
> Just help me understand: How is isSupportedAttribute(name) handled in the
> parseAttributeNew()?

IIUC, isSupportedAttribute() is an optimization to avoid the if chains if the
property is not in the HashSet. We do not need it anymore as we are replacing it
with a single HashMap lookup in SVGElement::parseAttributeNew

[haraken, 2014-07-28 04:54:34]

On 2014/07/28 04:51:29, kouhei wrote:
>
https://codereview.chromium.org/421863002/diff/20001/Source/core/svg/SVGRectE...
> File Source/core/svg/SVGRectElement.cpp (left):
> 
>
https://codereview.chromium.org/421863002/diff/20001/Source/core/svg/SVGRectE...
> Source/core/svg/SVGRectElement.cpp:70: if (!isSupportedAttribute(name))
> On 2014/07/28 04:49:13, haraken wrote:
> > 
> > Just help me understand: How is isSupportedAttribute(name) handled in the
> > parseAttributeNew()?
> 
> IIUC, isSupportedAttribute() is an optimization to avoid the if chains if the
> property is not in the HashSet. We do not need it anymore as we are replacing
it
> with a single HashMap lookup in SVGElement::parseAttributeNew

ok, LGTM. I want to have pdr@ or fs@ to take another eye on it.

[fs, 2014-07-28 08:00:45]

https://codereview.chromium.org/421863002/diff/20001/Source/core/svg/SVGEleme...
File Source/core/svg/SVGElement.cpp (right):

https://codereview.chromium.org/421863002/diff/20001/Source/core/svg/SVGEleme...
Source/core/svg/SVGElement.cpp:612: }
Should there be a 'return' here?

[kouhei (in TOK), 2014-07-28 08:51:03]

https://codereview.chromium.org/421863002/diff/20001/Source/core/svg/SVGEleme...
File Source/core/svg/SVGElement.cpp (right):

https://codereview.chromium.org/421863002/diff/20001/Source/core/svg/SVGEleme...
Source/core/svg/SVGElement.cpp:612: }
On 2014/07/28 08:00:45, fs wrote:
> Should there be a 'return' here?
Yes. Thanks for the catch.

[fs, 2014-07-28 09:04:03]

On 2014/07/28 08:51:03, kouhei wrote:
>
https://codereview.chromium.org/421863002/diff/20001/Source/core/svg/SVGEleme...
> File Source/core/svg/SVGElement.cpp (right):
> 
>
https://codereview.chromium.org/421863002/diff/20001/Source/core/svg/SVGEleme...
> Source/core/svg/SVGElement.cpp:612: }
> On 2014/07/28 08:00:45, fs wrote:
> > Should there be a 'return' here?
> Yes. Thanks for the catch.

LGTM

[kouhei (in TOK), 2014-07-28 09:05:25]

Thanks for review!

[kouhei (in TOK), 2014-07-28 09:05:28]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2014-07-28 09:05:51]

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

[commit-bot: I haz the power, 2014-07-28 09:58:06]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/19190)

[commit-bot: I haz the power, 2014-07-28 10:34:40]

Change committed as 179019