Make neutral keyframes explicit in SVG attribute interpolation test API

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

 /* Copyright 2015 The Chromium Authors. All rights reserved.
  * Use of this source code is governed by a BSD-style license that can be
  * 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;
 
   function createElement(tagName, container) {
     var element = document.createElement(tagName);
     if (container) {
       container.appendChild(element);
     }
     return element;
@@ skipping 285 matching lines @@
         } else {
           console.warn(`Unable to test SMIL from ${params.from} to ${params.to}`);
           target.container.remove();
           target.measure = function() {};
         }
         break;
       case 'Web Animations':
         // 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',
           duration: 1,
           easing: createEasing(expectation.at),
           delay: -0.5,
           iterations: 0.5,
         });
         break;
       default:
@@ skipping 15 matching lines @@
       }, `${method}: ${description} at (${expectation.at}) is [${expectation.is}]`);
     };
 
     return target;
   }
 
   function createTestTargets(interpolationTests, container, rebaselineContainer) {
     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({
   property: '${params.property}',
   underlying: '${params.underlying}',${fromCode}${toCode}
 }, [\n`));
         var rebaselineExpectation;
         rebaseline.appendChild(rebaselineExpectation = document.createTextNode(''));
         rebaseline.appendChild(document.createTextNode(']);\n\n'));
       }
@@ skipping 52 matching lines @@
   loadScript('../../resources/testharness.js').then(() => {
     return loadScript('../../resources/testharnessreport.js');
   }).then(() => {
     var asyncHandle = async_test('This test uses interpolation-test.js.')
     requestAnimationFrame(() => {
       runTests().then(() => asyncHandle.done());
     });
   });
 
   window.assertAttributeInterpolation = assertAttributeInterpolation;
+  window.neutralKeyframe = neutralKeyframe;
 })();