Refactor SVGAnimatedLength negative values mode

Refactor SVGAnimatedLength negative values mode

The SVGAnimatedLength constructor takes an additional argument (compared to the
arguments passed to other SVGAnimatedProperty classes) which represents whether
the length may be negative. Although this is stored as a property of
SVGAnimatedLength, the value is determined by the particular SVG attribute that
the SVGAnimatedLength is representing, and does not change.

In order to facilitate the implementation of additive animations for
AnimatedLength, this patch removes this additional argument, and instead
implements a helper function in SVGLength.h to look up the value for a given SVG
attribute.

BUG=530436
Committed: https://crrev.com/acabbbf906dfc4c71bf2ad097527b87cc2227db6
Cr-Commit-Position: refs/heads/master@{#362941}

[suzyh_UTC10 (ex-contributor), 2015-12-02 03:35:44]

suzyh@chromium.org changed reviewers:
+ dstockwell@chromium.org

[suzyh_UTC10 (ex-contributor), 2015-12-02 03:51:43]

suzyh@chromium.org changed reviewers:
+ fs@opera.com, pdr@chromium.org
- dstockwell@chromium.org

[alancutter (OOO until 2018), 2015-12-02 03:55:34]

alancutter@chromium.org changed reviewers:
+ alancutter@chromium.org, dstockwell@chromium.org
- fs@opera.com, pdr@chromium.org

[alancutter (OOO until 2018), 2015-12-02 03:55:34]

Non OWNER lgtm with nit.

https://codereview.chromium.org/1491023003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/SVGLength.cpp (right):

https://codereview.chromium.org/1491023003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGLength.cpp:261: return
SVGLengthNegativeValuesMode::AllowNegativeLengths;
ASSERT_NOT_REACHED();

[fs, 2015-12-02 14:45:41]

fs@opera.com changed reviewers:
+ fs@opera.com

[fs, 2015-12-02 14:45:42]

This is great! I just have one small adjustment I'd like to see.

https://codereview.chromium.org/1491023003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/SVGAnimatedLength.cpp (right):

https://codereview.chromium.org/1491023003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGAnimatedLength.cpp:54: } else if
(SVGLength::negativeValuesModeForAnimatedLengthAttribute(attributeName()) ==
ForbidNegativeLengths && baseValue()->valueInSpecifiedUnits() < 0) {
It's possible that we ought/need to re-order these condition now (since the
first one is more expensive now), but we can wait for the (perf)bots to come
down with the verdict on that first I think.

https://codereview.chromium.org/1491023003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/SVGLength.cpp (right):

https://codereview.chromium.org/1491023003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGLength.cpp:261: return
SVGLengthNegativeValuesMode::AllowNegativeLengths;
On 2015/12/02 at 03:55:34, alancutter wrote:
> ASSERT_NOT_REACHED();

Actually, since this is essentially a "binary" value (allow/forbid), I think it
would make sense to use a HashSet instead, and let that contain the QNames that
needs ForbidNegativeLengths. This also means that we can save on the hash
lookups (contains() will suffice.) Could just make this helper return a bool
then and remove the enum.

(Similarly the SVGLengthMode map above shouldn't need to store Other, but we
should probably add a comment to that effect too. I've pondered getting rid of
that one too just like this...)

[suzyh_UTC10 (ex-contributor), 2015-12-03 04:35:32]

On 2015/12/02 at 14:45:42, fs wrote:
> This is great! I just have one small adjustment I'd like to see.
> 
>
https://codereview.chromium.org/1491023003/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/svg/SVGAnimatedLength.cpp (right):
> 
>
https://codereview.chromium.org/1491023003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/svg/SVGAnimatedLength.cpp:54: } else if
(SVGLength::negativeValuesModeForAnimatedLengthAttribute(attributeName()) ==
ForbidNegativeLengths && baseValue()->valueInSpecifiedUnits() < 0) {
> It's possible that we ought/need to re-order these condition now (since the
first one is more expensive now), but we can wait for the (perf)bots to come
down with the verdict on that first I think.
> 
>
https://codereview.chromium.org/1491023003/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/svg/SVGLength.cpp (right):
> 
>
https://codereview.chromium.org/1491023003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/svg/SVGLength.cpp:261: return
SVGLengthNegativeValuesMode::AllowNegativeLengths;
> On 2015/12/02 at 03:55:34, alancutter wrote:
> > ASSERT_NOT_REACHED();
> 
> Actually, since this is essentially a "binary" value (allow/forbid), I think
it would make sense to use a HashSet instead, and let that contain the QNames
that needs ForbidNegativeLengths. This also means that we can save on the hash
lookups (contains() will suffice.) Could just make this helper return a bool
then and remove the enum.
> 
> (Similarly the SVGLengthMode map above shouldn't need to store Other, but we
should probably add a comment to that effect too. I've pondered getting rid of
that one too just like this...)

Thanks for the review! Done.

[suzyh_UTC10 (ex-contributor), 2015-12-03 04:37:18]

suzyh@chromium.org changed reviewers:
+ pdr@chromium.org
- dstockwell@chromium.org

[fs, 2015-12-03 08:43:48]

The CQ bit was checked by fs@opera.com

[fs, 2015-12-03 08:43:48]

lgtm

[fs, 2015-12-03 08:43:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
alancutter@chromium.org
Link to the patchset: https://codereview.chromium.org/1491023003/#ps20001
(title: "Replace SVGLengthNegativeValuesMode enum with bool")

[commit-bot: I haz the power, 2015-12-03 08:43:59]

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

[commit-bot: I haz the power, 2015-12-03 10:21:03]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-12-03 10:22:34]

Description was changed from

==========
Refactor SVGAnimatedLength negative values mode

The SVGAnimatedLength constructor takes an additional argument (compared to the
arguments passed to other SVGAnimatedProperty classes) which represents whether
the length may be negative. Although this is stored as a property of
SVGAnimatedLength, the value is determined by the particular SVG attribute that
the SVGAnimatedLength is representing, and does not change.

In order to facilitate the implementation of additive animations for
AnimatedLength, this patch removes this additional argument, and instead
implements a helper function in SVGLength.h to look up the value for a given SVG
attribute.

BUG=530436
==========

to

==========
Refactor SVGAnimatedLength negative values mode

The SVGAnimatedLength constructor takes an additional argument (compared to the
arguments passed to other SVGAnimatedProperty classes) which represents whether
the length may be negative. Although this is stored as a property of
SVGAnimatedLength, the value is determined by the particular SVG attribute that
the SVGAnimatedLength is representing, and does not change.

In order to facilitate the implementation of additive animations for
AnimatedLength, this patch removes this additional argument, and instead
implements a helper function in SVGLength.h to look up the value for a given SVG
attribute.

BUG=530436

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

[commit-bot: I haz the power, 2015-12-03 10:22:35]

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