Fold SVGAnimatedTypeAnimator into SVGAnimateElement

Fold SVGAnimatedTypeAnimator into SVGAnimateElement

This folds the functionality of SVGAnimatedTypeAnimator, mostly
verbatim, into SVGAnimateElement. Some methods are renamed, and a new
interface is defined, which allows <animateTransform> to be more
isolated from the rest of the <animate> implementation.
SVGAnimateElement::m_animatedProperty is renamed to m_animatedValue, and
SVGAnimatedTypeAnimator::m_animatedProperty is renamed to
m_targetProperty.
Dial down or remove some asserts in favor of other asserts.

BUG=641437
Committed: https://crrev.com/eaf764b300511979136eeed7907c1692bcc14a47
Cr-Commit-Position: refs/heads/master@{#440613}

[fs, 2016-12-22 17:02:08]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-22 17:02:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fs, 2016-12-22 17:05:31]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-22 17:05:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

The CQ bit was unchecked by commit-bot@chromium.org

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

Dry run: This issue passed the CQ dry run.

[fs, 2016-12-22 19:22:52]

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

[Stephen Chennney, 2016-12-22 20:07:24]

schenney@chromium.org changed reviewers:
+ schenney@chromium.org

[Stephen Chennney, 2016-12-22 20:07:25]

LGTM with nit to avoid presubmit annoyance in the future.

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

https://codereview.chromium.org/2595393002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGAnimateElement.cpp:299: ASSERT(percentage
>= 0 && percentage <= 1);
I wonder how we got in a state where there are DCHECK and ASSERT in the same
file? Please change them all to DCHECK if possible.

[fs, 2016-12-22 20:25:31]

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

https://codereview.chromium.org/2595393002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGAnimateElement.cpp:299: ASSERT(percentage
>= 0 && percentage <= 1);
On 2016/12/22 at 20:07:25, Stephen Chennney wrote:
> I wonder how we got in a state where there are DCHECK and ASSERT in the same
file? Please change them all to DCHECK if possible.

That's most likely an artifact of whoever changed code here (likely me) only
changing ASSERT -> DCHECK on otherwise change lines, and presubmit only
complaining about changed lines. Coincidence? =) I'll try to fix this file to
only be on one side of the fence though.

[fs, 2016-12-22 21:34:14]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-22 21:34:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fs, 2016-12-22 21:45:49]

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

https://codereview.chromium.org/2595393002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGAnimateElement.cpp:299: ASSERT(percentage
>= 0 && percentage <= 1);
On 2016/12/22 at 20:25:31, fs wrote:
> On 2016/12/22 at 20:07:25, Stephen Chennney wrote:
> > I wonder how we got in a state where there are DCHECK and ASSERT in the same
file? Please change them all to DCHECK if possible.
> 
> That's most likely an artifact of whoever changed code here (likely me) only
changing ASSERT -> DCHECK on otherwise change lines, and presubmit only
complaining about changed lines. Coincidence? =) I'll try to fix this file to
only be on one side of the fence though.

Done.

[blink-reviews, 2016-12-22 21:59:38]

Feel free to land as is.


On Dec 22, 2016 3:45 PM, <fs@opera.com> wrote:

>
> https://codereview.chromium.org/2595393002/diff/20001/
> third_party/WebKit/Source/core/svg/SVGAnimateElement.cpp
> File third_party/WebKit/Source/core/svg/SVGAnimateElement.cpp (right):
>
> https://codereview.chromium.org/2595393002/diff/20001/
> third_party/WebKit/Source/core/svg/SVGAnimateElement.cpp#newcode299
> third_party/WebKit/Source/core/svg/SVGAnimateElement.cpp:299:
> ASSERT(percentage >= 0 && percentage <= 1);
> On 2016/12/22 at 20:25:31, fs wrote:
> > On 2016/12/22 at 20:07:25, Stephen Chennney wrote:
> > > I wonder how we got in a state where there are DCHECK and ASSERT in
> the same file? Please change them all to DCHECK if possible.
> >
> > That's most likely an artifact of whoever changed code here (likely
> me) only changing ASSERT -> DCHECK on otherwise change lines, and
> presubmit only complaining about changed lines. Coincidence? =) I'll try
> to fix this file to only be on one side of the fence though.
>
> Done.
>
> https://codereview.chromium.org/2595393002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2016-12-22 21:59:38]

Feel free to land as is.


On Dec 22, 2016 3:45 PM, <fs@opera.com> wrote:

>
> https://codereview.chromium.org/2595393002/diff/20001/
> third_party/WebKit/Source/core/svg/SVGAnimateElement.cpp
> File third_party/WebKit/Source/core/svg/SVGAnimateElement.cpp (right):
>
> https://codereview.chromium.org/2595393002/diff/20001/
> third_party/WebKit/Source/core/svg/SVGAnimateElement.cpp#newcode299
> third_party/WebKit/Source/core/svg/SVGAnimateElement.cpp:299:
> ASSERT(percentage >= 0 && percentage <= 1);
> On 2016/12/22 at 20:25:31, fs wrote:
> > On 2016/12/22 at 20:07:25, Stephen Chennney wrote:
> > > I wonder how we got in a state where there are DCHECK and ASSERT in
> the same file? Please change them all to DCHECK if possible.
> >
> > That's most likely an artifact of whoever changed code here (likely
> me) only changing ASSERT -> DCHECK on otherwise change lines, and
> presubmit only complaining about changed lines. Coincidence? =) I'll try
> to fix this file to only be on one side of the fence though.
>
> Done.
>
> https://codereview.chromium.org/2595393002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[fs, 2016-12-22 21:59:43]

The CQ bit was unchecked by fs@opera.com

[fs, 2016-12-22 21:59:46]

The CQ bit was checked by fs@opera.com

[fs, 2016-12-22 21:59:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
schenney@chromium.org
Link to the patchset: https://codereview.chromium.org/2595393002/#ps40001
(title: "More ASSERT to DCHECK")

[commit-bot: I haz the power, 2016-12-22 22:00:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-22 23:18:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-22 23:18:24]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[fs, 2016-12-23 10:20:52]

The CQ bit was checked by fs@opera.com

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

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-23 10:24:50]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1482488452056420,
"parent_rev": "44ded333f71410a9ed772a2c34e254655b50489f", "commit_rev":
"327ae8ee35c9c24db2f23bfa47d730f9aa9acb60"}

[commit-bot: I haz the power, 2016-12-23 10:25:20]

Description was changed from

==========
Fold SVGAnimatedTypeAnimator into SVGAnimateElement

This folds the functionality of SVGAnimatedTypeAnimator, mostly
verbatim, into SVGAnimateElement. Some methods are renamed, and a new
interface is defined, which allows <animateTransform> to be more
isolated from the rest of the <animate> implementation.
SVGAnimateElement::m_animatedProperty is renamed to m_animatedValue, and
SVGAnimatedTypeAnimator::m_animatedProperty is renamed to
m_targetProperty.
Dial down or remove some asserts in favor of other asserts.

BUG=641437
==========

to

==========
Fold SVGAnimatedTypeAnimator into SVGAnimateElement

This folds the functionality of SVGAnimatedTypeAnimator, mostly
verbatim, into SVGAnimateElement. Some methods are renamed, and a new
interface is defined, which allows <animateTransform> to be more
isolated from the rest of the <animate> implementation.
SVGAnimateElement::m_animatedProperty is renamed to m_animatedValue, and
SVGAnimatedTypeAnimator::m_animatedProperty is renamed to
m_targetProperty.
Dial down or remove some asserts in favor of other asserts.

BUG=641437

Review-Url: https://codereview.chromium.org/2595393002
==========

[commit-bot: I haz the power, 2016-12-23 10:25:21]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-23 10:27:14]

Description was changed from

==========
Fold SVGAnimatedTypeAnimator into SVGAnimateElement

This folds the functionality of SVGAnimatedTypeAnimator, mostly
verbatim, into SVGAnimateElement. Some methods are renamed, and a new
interface is defined, which allows <animateTransform> to be more
isolated from the rest of the <animate> implementation.
SVGAnimateElement::m_animatedProperty is renamed to m_animatedValue, and
SVGAnimatedTypeAnimator::m_animatedProperty is renamed to
m_targetProperty.
Dial down or remove some asserts in favor of other asserts.

BUG=641437

Review-Url: https://codereview.chromium.org/2595393002
==========

to

==========
Fold SVGAnimatedTypeAnimator into SVGAnimateElement

This folds the functionality of SVGAnimatedTypeAnimator, mostly
verbatim, into SVGAnimateElement. Some methods are renamed, and a new
interface is defined, which allows <animateTransform> to be more
isolated from the rest of the <animate> implementation.
SVGAnimateElement::m_animatedProperty is renamed to m_animatedValue, and
SVGAnimatedTypeAnimator::m_animatedProperty is renamed to
m_targetProperty.
Dial down or remove some asserts in favor of other asserts.

BUG=641437

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

[commit-bot: I haz the power, 2016-12-23 10:27:15]

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