Make neutral keyframes explicit in SVG attribute interpolation test API

Make neutral keyframes explicit in SVG attribute interpolation test API

This patch changes the way neutral keyframes are specified in
SVG attribute interpolation tests to be more explicit using an exported
neutralKeyframe object.

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

[alancutter (OOO until 2018), 2015-11-19 07:14:39]

Description was changed from

==========
Make neutral keyframes explicit in SVG attribute interpolation test API

This patch changes the way neutral keyframes are specified in
SVG attribute interpolation tests.

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

to

==========
Make neutral keyframes explicit in SVG attribute interpolation test API

This patch changes the way neutral keyframes are specified in
SVG attribute interpolation tests to be more explicit using an exported
neutralKeyframe object.

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

[alancutter (OOO until 2018), 2015-11-19 07:29:24]

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

[suzyh_UTC10 (ex-contributor), 2015-11-19 22:48:28]

https://codereview.chromium.org/1459093002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/animations/svg-attribute-interpolation/resources/interpolation-test.js
(right):

https://codereview.chromium.org/1459093002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/animations/svg-attribute-interpolation/resources/interpolation-test.js:323:
if (params.from !== neutralKeyframe) {
if equal { A } else { B }
is easier to understand than
if not equal { B } else { A }
so I recommend swapping the order of these clauses.

https://codereview.chromium.org/1459093002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/animations/svg-attribute-interpolation/resources/interpolation-test.js:385:
from: neutralKeyframe,` : `
This code is quite difficult to read. Can the `...` strings be factored out into
helper functions so it doesn't interfere with the indentation and so the higher
level structure (...?...:...) is easier to see at a glance?

https://codereview.chromium.org/1459093002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/animations/svg-attribute-interpolation/resources/interpolation-test.js:389:
var toCode = params.to === neutralKeyframe ? `
Given the number of times you've had to write this test, consider defining a
function isNeutral(x) { return x === neutralKeyframe }. I think it would be
fractionally easier to read, save you a tiny amount of typing each time, and
reduce the risk of getting the === wrong.

[alancutter (OOO until 2018), 2015-11-20 02:21:41]

https://codereview.chromium.org/1459093002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/animations/svg-attribute-interpolation/resources/interpolation-test.js
(right):

https://codereview.chromium.org/1459093002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/animations/svg-attribute-interpolation/resources/interpolation-test.js:323:
if (params.from !== neutralKeyframe) {
On 2015/11/19 at 22:48:28, suzyh wrote:
> if equal { A } else { B }
> is easier to understand than
> if not equal { B } else { A }
> so I recommend swapping the order of these clauses.

There is no longer an else branch.

https://codereview.chromium.org/1459093002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/animations/svg-attribute-interpolation/resources/interpolation-test.js:385:
from: neutralKeyframe,` : `
On 2015/11/19 at 22:48:28, suzyh wrote:
> This code is quite difficult to read. Can the `...` strings be factored out
into helper functions so it doesn't interfere with the indentation and so the
higher level structure (...?...:...) is easier to see at a glance?

Agreed, this is hard to read. Rewrote it to avoid ternary operators. Still hard
to read because of the mix of logic indentation vs generated code indentation
but this time I favoured the logic's indentation.

https://codereview.chromium.org/1459093002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/animations/svg-attribute-interpolation/resources/interpolation-test.js:389:
var toCode = params.to === neutralKeyframe ? `
On 2015/11/19 at 22:48:28, suzyh wrote:
> Given the number of times you've had to write this test, consider defining a
function isNeutral(x) { return x === neutralKeyframe }. I think it would be
fractionally easier to read, save you a tiny amount of typing each time, and
reduce the risk of getting the === wrong.

Done.

[suzyh_UTC10 (ex-contributor), 2015-11-20 04:37:17]

lgtm

https://codereview.chromium.org/1459093002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/animations/svg-attribute-interpolation/resources/interpolation-test.js
(right):

https://codereview.chromium.org/1459093002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/animations/svg-attribute-interpolation/resources/interpolation-test.js:323:
if (params.from !== neutralKeyframe) {
On 2015/11/20 02:21:41, alancutter wrote:
> On 2015/11/19 at 22:48:28, suzyh wrote:
> > if equal { A } else { B }
> > is easier to understand than
> > if not equal { B } else { A }
> > so I recommend swapping the order of these clauses.
> 
> There is no longer an else branch.

haha whoops, I misread the diff.

[alancutter (OOO until 2018), 2015-11-23 00:13:57]

The CQ bit was checked by alancutter@chromium.org

[commit-bot: I haz the power, 2015-11-23 00:14:18]

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

[commit-bot: I haz the power, 2015-11-23 00:59:50]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-23 01:01:18]

Patchset 4 (id:??) landed as
https://crrev.com/5a94fda86ddb4a9eb976c42aa06348d5fa315a66
Cr-Commit-Position: refs/heads/master@{#361063}