Make neutral keyframes explicit in SVG attribute interpolation test API

third_party/WebKit/LayoutTests/animations/svg-attribute-interpolation/resources/interpolation-test.js

Index: third_party/WebKit/LayoutTests/animations/svg-attribute-interpolation/resources/interpolation-test.js
diff --git a/third_party/WebKit/LayoutTests/animations/svg-attribute-interpolation/resources/interpolation-test.js b/third_party/WebKit/LayoutTests/animations/svg-attribute-interpolation/resources/interpolation-test.js
index 6e24685ba098206b429ab55e75d6bf01141b577d..4d0ddd23548bdf83d925ccd5cd97986ff019e0eb 100644
--- a/third_party/WebKit/LayoutTests/animations/svg-attribute-interpolation/resources/interpolation-test.js
+++ b/third_party/WebKit/LayoutTests/animations/svg-attribute-interpolation/resources/interpolation-test.js
@@ -3,18 +3,18 @@
  * found in the LICENSE file.
  *
  * Exported function:
- *  - assertAttributeInterpolation({property, [from], [to], [fromComposite], [toComposite], [underlying]}, [{at: fraction, is: value}])
+ *  - assertAttributeInterpolation({property, from, to, [fromComposite], [toComposite], [underlying]}, [{at: fraction, is: value}])
  *        Constructs a test case for each fraction that asserts the expected value
  *        equals the value produced by interpolation between from and to composited
  *        onto underlying by fromComposite and toComposite respectively using
  *        SMIL and Web Animations.
- *        If from or to are missing then a neutral keyframe will be used and the
- *        composite mode will be forced to be 'add'.
+ *        Set from/to to the exported neutralKeyframe object to specify neutral keyframes.
  *        SMIL will only be tested with equal fromComposite and toComposite values.
 */
 'use strict';
 (() => {
   var interpolationTests = [];
+  var neutralKeyframe = {};
 
   // Set to true to output rebaselined test expectations.
   var rebaselineTests = false;
@@ -320,23 +320,19 @@
         // Replace 'transform' with 'svgTransform', etc. This avoids collisions with CSS properties or the Web Animations API (offset).
         var prefixedProperty = 'svg' + params.property[0].toUpperCase() + params.property.slice(1);
         var keyframes = [];
-        if ('from' in params) {
+        if (params.from !== neutralKeyframe) {

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

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.

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

There is no longer an else branch.

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

haha whoops, I misread the diff.

           keyframes.push({
             offset: 0,
             [prefixedProperty]: params.from,
             composite: params.fromComposite,
           });
-        } else {
-          console.assert(params.fromComposite === 'add');
         }
-        if ('to' in params) {
+        if (params.to !== neutralKeyframe) {
           keyframes.push({
             offset: 1,
             [prefixedProperty]: params.to,
             composite: params.toComposite,
           });
-        } else {
-          console.assert(params.toComposite === 'add');
         }
         target.animate(keyframes, {
           fill: 'forwards',
@@ -372,23 +368,28 @@
     var targets = [];
     for (var interpolationTest of interpolationTests) {
       var params = interpolationTest.params;
-      params.fromComposite = 'from' in params ? (params.fromComposite || 'replace') : 'add';
-      params.toComposite = 'to' in params ? (params.toComposite || 'replace') : 'add';
+      assert_true('property' in params);
+      assert_true('from' in params);
+      assert_true('to' in params);
+      params.fromComposite = params.from === neutralKeyframe ? 'add' : (params.fromComposite || 'replace');
+      params.toComposite = params.to === neutralKeyframe ? 'add' : (params.toComposite || 'replace');
       var underlyingText = params.underlying ? `with underlying [${params.underlying}] ` : '';
-      var fromText = 'from' in params ? `${params.fromComposite} [${params.from}]` : 'neutral';
-      var toText = 'to' in params ? `${params.toComposite} [${params.to}]` : 'neutral';
+      var fromText = params.from === neutralKeyframe ? 'neutral' : `${params.fromComposite} [${params.from}]`;
+      var toText = params.to === neutralKeyframe ? 'neutral' : `${params.toComposite} [${params.to}]`;
       var description = `Interpolate attribute <${params.property}> ${underlyingText}from ${fromText} to ${toText}`;
 
     if (rebaselineTests) {
         var rebaseline = createElement('pre', rebaselineContainer);
 
-        var fromCode = 'from' in params ? `
+        var fromCode = params.from === neutralKeyframe ? `
+  from: neutralKeyframe,` : `

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

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?

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

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.

   from: '${params.from}',
-  fromComposite: '${params.fromComposite}',` : '';
+  fromComposite: '${params.fromComposite}',`;
 
-        var toCode = 'to' in params ? `
+        var toCode = params.to === neutralKeyframe ? `

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

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]

Done.

+  to: neutralKeyframe,` : `
   to: '${params.to}',
-  toComposite: '${params.toComposite}',` : '';
+  toComposite: '${params.toComposite}',`;
 
         rebaseline.appendChild(document.createTextNode(`\
 assertAttributeInterpolation({
@@ -461,4 +462,5 @@ assertAttributeInterpolation({
   });
 
   window.assertAttributeInterpolation = assertAttributeInterpolation;
+  window.neutralKeyframe = neutralKeyframe;
 })();