Improve default value handling for SVGSVGElement.width/height

Improve default value handling for SVGSVGElement.width/height

When a length attribute is either removed or set to an invalid value,
it would get set to '0'. It should however be set to the initial value
specified for the attribute (possibly presentation attribute) in
question.

The test svg/custom/disallow-non-lengths-in-attrs.html is tweaked to
accommodate for this.

BUG=604093
Committed: https://crrev.com/6cd726f3f3ed5d5fecfe725bb3c85dc2cac39e85
Cr-Commit-Position: refs/heads/master@{#388250}

[fs, 2016-04-18 16:27:12]

Description was changed from

==========
[WIP] Default value for SVGAnimatedLength

BUG=474523, 604093
==========

to

==========
Improve default value handling for SVGAnimatedLength

When a length attribute is either removed or set to an invalid value,
it would get set to '0'. It should however be set to the initial value
specified for the attribute (possibly presentation attribute) in
question.
This CL adds a mechanism that can be used to specify the initial value
for SVGAnimatedLength, and uses this for SVGSVGElement.width/height.

BUG=474523, 604093
==========

[fs, 2016-04-18 16:34:17]

Description was changed from

==========
Improve default value handling for SVGAnimatedLength

When a length attribute is either removed or set to an invalid value,
it would get set to '0'. It should however be set to the initial value
specified for the attribute (possibly presentation attribute) in
question.
This CL adds a mechanism that can be used to specify the initial value
for SVGAnimatedLength, and uses this for SVGSVGElement.width/height.

BUG=474523, 604093
==========

to

==========
Improve default value handling for SVGAnimatedLength

When a length attribute is either removed or set to an invalid value,
it would get set to '0'. It should however be set to the initial value
specified for the attribute (possibly presentation attribute) in
question.
This CL adds a mechanism that can be used to specify the initial value
for SVGAnimatedLength, and uses this for SVGSVGElement.width/height.

The test svg/custom/disallow-non-lengths-in-attrs.html is tweaked to
accommodate for this.

BUG=474523, 604093
==========

[fs, 2016-04-18 17:40:17]

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

[fs, 2016-04-18 17:40:18]

This is an "outtake" from a branch that aims to fix issue 474523 (and similar
issues), while also making some aspects of SVGAnimated* more data-driven. But
since this bug reared its ugly head in the form of issue 604093 - i.e made more
acute by allowing '0' intrinsic dimensions. One difference from the work on the
branch is that I don't expect to need a const char* like this.

PTAL

[fs, 2016-04-18 17:44:12]

On 2016/04/18 at 17:40:18, fs wrote:
> This is an "outtake" from a branch that aims to fix issue 474523 (and similar
issues), while also making some aspects of SVGAnimated* more data-driven. But
since this bug reared its ugly head in the form of issue 604093 - i.e made more
acute by allowing '0' intrinsic dimensions. One difference from the work on the
branch is that I don't expect to need a const char* like this.

(Oh, and setDefaultValue won't be called explicitly - and I'll likely
s/default/initial/gi to be more in-line with spec terms.)

[pdr., 2016-04-18 21:25:14]

https://codereview.chromium.org/1901673003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGAnimatedLength.h (right):

https://codereview.chromium.org/1901673003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGAnimatedLength.h:59: const char*
m_defaultValue;
This seems like a memory regression since it costs one pointer per length. Is
there any other way to solve this, for example by special-casing
svgAttributeChanged?

[pdr., 2016-04-18 21:36:24]

https://codereview.chromium.org/1901673003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGAnimatedLength.h (right):

https://codereview.chromium.org/1901673003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGAnimatedLength.h:59: const char*
m_defaultValue;
On 2016/04/18 at 21:25:14, pdr wrote:
> This seems like a memory regression since it costs one pointer per length. Is
there any other way to solve this, for example by special-casing
svgAttributeChanged?

Adding a little more info... I am wondering if we can avoid storing one default
per length, and instead store the default just once.

[fs, 2016-04-18 21:55:07]

https://codereview.chromium.org/1901673003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGAnimatedLength.h (right):

https://codereview.chromium.org/1901673003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGAnimatedLength.h:59: const char*
m_defaultValue;
On 2016/04/18 at 21:36:23, pdr wrote:
> On 2016/04/18 at 21:25:14, pdr wrote:
> > This seems like a memory regression since it costs one pointer per length.
Is there any other way to solve this, for example by special-casing
svgAttributeChanged?

We could probably fix it by a crutch in parseAttribute.

> Adding a little more info... I am wondering if we can avoid storing one
default per length, and instead store the default just once.

There are at least a few different methods (that I've been able to think of)
that avoids increasing the attribute objects. Some better and some worse I'm
sure - my current favorite is a "dictionary" (small index + string table).

[fs, 2016-04-19 08:33:10]

Description was changed from

==========
Improve default value handling for SVGAnimatedLength

When a length attribute is either removed or set to an invalid value,
it would get set to '0'. It should however be set to the initial value
specified for the attribute (possibly presentation attribute) in
question.
This CL adds a mechanism that can be used to specify the initial value
for SVGAnimatedLength, and uses this for SVGSVGElement.width/height.

The test svg/custom/disallow-non-lengths-in-attrs.html is tweaked to
accommodate for this.

BUG=474523, 604093
==========

to

==========
Improve default value handling for SVGAnimatedLength

When a length attribute is either removed or set to an invalid value,
it would get set to '0'. It should however be set to the initial value
specified for the attribute (possibly presentation attribute) in
question.

The test svg/custom/disallow-non-lengths-in-attrs.html is tweaked to
accommodate for this.

BUG=474523, 604093
==========

[fs, 2016-04-19 08:33:39]

Description was changed from

==========
Improve default value handling for SVGAnimatedLength

When a length attribute is either removed or set to an invalid value,
it would get set to '0'. It should however be set to the initial value
specified for the attribute (possibly presentation attribute) in
question.

The test svg/custom/disallow-non-lengths-in-attrs.html is tweaked to
accommodate for this.

BUG=474523, 604093
==========

to

==========
Improve default value handling for SVGSVGElement.width/height

When a length attribute is either removed or set to an invalid value,
it would get set to '0'. It should however be set to the initial value
specified for the attribute (possibly presentation attribute) in
question.

The test svg/custom/disallow-non-lengths-in-attrs.html is tweaked to
accommodate for this.

BUG=474523, 604093
==========

[fs, 2016-04-19 08:33:46]

Description was changed from

==========
Improve default value handling for SVGSVGElement.width/height

When a length attribute is either removed or set to an invalid value,
it would get set to '0'. It should however be set to the initial value
specified for the attribute (possibly presentation attribute) in
question.

The test svg/custom/disallow-non-lengths-in-attrs.html is tweaked to
accommodate for this.

BUG=474523, 604093
==========

to

==========
Improve default value handling for SVGSVGElement.width/height

When a length attribute is either removed or set to an invalid value,
it would get set to '0'. It should however be set to the initial value
specified for the attribute (possibly presentation attribute) in
question.

The test svg/custom/disallow-non-lengths-in-attrs.html is tweaked to
accommodate for this.

BUG=604093
==========

[davve, 2016-04-19 08:38:20]

On 2016/04/18 21:55:07, fs wrote:

> We could probably fix it by a crutch in parseAttribute.

I don't think the parseAttribute fix is as elegant but I see the motivation wrt
to memory. Especially since it seems to be a interim solution. LGTM.

[pdr., 2016-04-19 17:39:13]

On 2016/04/19 at 08:38:20, davve wrote:
> On 2016/04/18 21:55:07, fs wrote:
> 
> > We could probably fix it by a crutch in parseAttribute.
> 
> I don't think the parseAttribute fix is as elegant but I see the motivation
wrt to memory. Especially since it seems to be a interim solution. LGTM.

I agree, LGTM here for the same reasons.

[fs, 2016-04-19 18:12:45]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2016-04-19 18:13:07]

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

[commit-bot: I haz the power, 2016-04-19 18:23:31]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-22 19:13:56]

Description was changed from

==========
Improve default value handling for SVGSVGElement.width/height

When a length attribute is either removed or set to an invalid value,
it would get set to '0'. It should however be set to the initial value
specified for the attribute (possibly presentation attribute) in
question.

The test svg/custom/disallow-non-lengths-in-attrs.html is tweaked to
accommodate for this.

BUG=604093
==========

to

==========
Improve default value handling for SVGSVGElement.width/height

When a length attribute is either removed or set to an invalid value,
it would get set to '0'. It should however be set to the initial value
specified for the attribute (possibly presentation attribute) in
question.

The test svg/custom/disallow-non-lengths-in-attrs.html is tweaked to
accommodate for this.

BUG=604093

Committed: https://crrev.com/6cd726f3f3ed5d5fecfe725bb3c85dc2cac39e85
Cr-Commit-Position: refs/heads/master@{#388250}
==========

[commit-bot: I haz the power, 2016-04-22 19:14:01]

Patchset 3 (id:??) landed as
https://crrev.com/6cd726f3f3ed5d5fecfe725bb3c85dc2cac39e85
Cr-Commit-Position: refs/heads/master@{#388250}