Closed Bug 1312301 Opened 8 years ago Closed 7 years ago

Refactor type-per-property.html to be used for other procedures (i.e. addition and accumulation)

Categories

(Core :: DOM: Animation, defect, P3)

Product:
Core
Component:
DOM: Animation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(13 files)

An html containing a script to dump a new gCSSProperties which includes properties both in layout/style/test/property_database.js and type-per-property.html
2.11 KB, text/html
Details
A sample output generated by attachment 8804972
27.17 KB, application/javascript
Details
Bug 1312301 - Part 1: Initialize keyframes with computed property name.
58 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1312301 - Part 2: Remove quotation mark from property name.
58 bytes, text/x-review-board-request
daisuke
: review+
Details
Bug 1312301 - Part 3: Use single quote instead of double for consistency.
58 bytes, text/x-review-board-request
daisuke
: review+
Details
Bug 1312301 - Part 4: Change gCSSProperties structure to be reused for other procedures.
58 bytes, text/x-review-board-request
boris
: review+
Details
Bug 1312301 - Part 5: Factor out gCSSProperties and utility functions.
58 bytes, text/x-review-board-request
boris
: review+
Details
Bug 1312301 - Part 6: Rename type-per-property.html to interpolation-per-property.html
58 bytes, text/x-review-board-request
boris
: review+
Details
Bug 1312301 - Part 7: Update all properties with layout/style/test/property_database.js.
58 bytes, text/x-review-board-request
boris
: review+
Details
Bug 1312301 - Part 8: Test properties that are animatable as length.
58 bytes, text/x-review-board-request
boris
: review+
Details
Bug 1312301 - Part 9: Tests for interpolation as color.
58 bytes, text/x-review-board-request
boris
: review+
Details
Bug 1312301 - Part 10: Move some utility functions into test_common.js.
58 bytes, text/x-review-board-request
boris
: review+
Details
Bug 1312301 - Part 11: Tests for interpolation as transformlist.
58 bytes, text/x-review-board-request
boris
: review+
Details 
What I have in mind now is;

1) Factor out gCSSProperties and other utility functions (e.g. isSupported) in testing/web-platform/tests/web-animations/animation-model/animation-types/properties.html

2) Make discrete and other functions an object method in type-per-property.html like this;

  var Interpolation = {
    discrete: function(property, from, to, options) {
      test(function(t) {
        ...
      }
    },
    length: function(property, options) {
    },
  };

Also make 'tests' property an object like this;

  var gCSSProperties = {
    "flex-basis": {
      // https://drafts.csswg.org/css-flexbox/#propdef-flex-basis
      testCases: {
        // Each property name is a method name to be tested.
        // Each property value is an array of arguments to be passed to test method.
        lengthPercentrageOrCals: null,
        discrete: ["auto", "10px"]
      }
  }

3) Then create a utility function to run tests against all properties in gCSSProperties.

  // procedure is an object which has 'discrete' and other methods.
  function runProcedureTest(procedure) {
    for (var property in gCSSProperies) {
      if (!isSupported(property)) {
        continue;
      }
      for (var testCase in gCSSProperties[property].testCases) {
        procedure[testCase].apply(procedure, [property].concat(|test-case-arguments-array|);
      }
    }
  }

4) Rename type-per-properties.js to interpolation-per-properties.js.

With above changes, we will just need to write |Addition| and |Accumulate| object for addition and accumulate respectively.
Apart from this refactoring I've been thinking about a test case that fails if we did not implement 'accumulate' or 'additive' operations when we add new CSS properties,  we can use layout/style/property_database.js for the purpose but I have no idea yet.
For item (2) if we want to test multiple pairs of values passed to the 'discrete' function we'll have to introduce some array-of-arrays handling (since we can't duplicate object property keys) I think. That seems to add more complexity and I wonder if we can just keep the original arrangement and extend it?

For example, we know that discrete never adds so can we just use the passed-in values twice, offset the two animations, and check the higher-priority values clobber the underlying ones?

For lengthPercentageOrCalc() can we just add more sub-tests to length() and friends for addition etc.?

I guess I'm just a little hesitant about any added complexity because it makes creating reduced test cases for debugging more involved (and we once had a bad bug in SMIL code for animationMotion because in there was a typo in the outer loop for the test code meaning we completely missed testing half the functionality). The existing approach seems quite readable and flexible (you can easily add a new function to extend / modify the existing tests) so I think it would be nice to keep using it if it's possible.
(In reply to Brian Birtles (:birtles, high review load) from comment #2)
> For item (2) if we want to test multiple pairs of values passed to the
> 'discrete' function we'll have to introduce some array-of-arrays handling
> (since we can't duplicate object property keys) I think. 

Right, actually I did use the array of arrays initially but dropped it in this proposal.

> That seems to add
> more complexity and I wonder if we can just keep the original arrangement
> and extend it?

The only way I can think of is that adding more entries in gCSSProperties and adding test code in each 'discrete' or 'lengthPercentageOrCalc', etc.  But it seems to me that the file will get bigger and bigger.  The bigger file is hard to understand. (e.g. it's hard to know scope).  I'd like to categorize them somehow in separate files.

If there is a way to keep the current structure and separate files (in category), it would be nice.
I think we have no agreement about this refactoring but attachment 8804972 [details] will be useful in any case.
I came up with another idea (which is slightly better I think).  

var gCSSProperties = {
  "flex-basis": {
    // https://drafts.csswg.org/css-flexbox/#propdef-flex-basis
    tests: [ "lengthPercentageOrCalc", "discrete" ],  // 'test' should be 'types'? 
    discreteKeyframes: [  // This should be 'discreteValuePair'?
      [ "auto", "10px" ],
      ...
    ]
  },
};

And discrete function is like this (it's inside an object):

var Interpolation = {
  discrete: function(property, options) {
    options.discreteKeyframes.forEach(function(keyframes) {
      assert_equals(keyframes.length, 2,
                    "keyframe must be a pair of from and to values");
      var [ from, to ] = keyframes;
      test(function(t) {
        ...
      },

And a utility function runs all of test cases:

// |procedure| should be an object that has following methods.
// See |Interpolation| in interpolation-per-property.html for example.
//
// discrete: function(property, options)
//   https://w3c.github.io/web-animations/#discrete-animation-type-section
//
// realNumber: function(property, options)
//   https://w3c.github.io/web-animations/#real-number-animation-type-section
//
// lengthPercentageOrCalc: function(property, options)
//   https://w3c.github.io/web-animations/#length-percentage-or-calc-animation-type-section
//
// color: function(property, options)
//   https://w3c.github.io/web-animations/#color-animation-type-section
//
// transformList: function(property, options)
//   https://w3c.github.io/web-animations/#transform-list-animation-type-section
//
// positiveNumber: function(property, options)
//   https://w3c.github.io/web-animations/?
//
// integer: function(property, options)
//   https://w3c.github.io/web-animations/?
//
// visibility: function(property, options)
//   https://w3c.github.io/web-animations/?
//
function runProcedureTests(procedure) {
  for (var property in gCSSProperties) {
    if (!isSupported(property)) {
      continue;
    }
    var testData = gCSSProperties[property];
    testData.tests.forEach(function(testFunction) {
      procedure[testFunction].apply(
        procedure, [property].concat(testData));
    });
  }
}

Then,
  runProcedureTests(Interpolation);
Blocks: 1320839
Blocks: 1311620
Apart from renaming the current type-per-property.html to interpolation-per-property.html, I will add color and transform list test cases here, because they are important for accumulation, (color accumulation causes overflow on intermediate calculation, and we had a bug (bug 1304886) for transform.  I understand filter and other properties are also important, but I would like to go forward one by one.  (I am actually hoping someone helps it).
Brian, I am sorry for review bombs again (as usual).   If you agree with the structure in comment 6, I will ask Boris to review part 4.  But I want to ask you for a decision about using computed property name (part 1).

Thanks!
Flags: needinfo?(bbirtles)
Comment on attachment 8815134 [details]
Bug 1312301 - Part 5: Factor out gCSSProperties and utility functions.

https://reviewboard.mozilla.org/r/96138/#review96360
Attachment #8815134 - Flags: review?(boris.chiou) → review+
Comment on attachment 8815136 [details]
Bug 1312301 - Part 7: Update all properties with layout/style/test/property_database.js.

https://reviewboard.mozilla.org/r/96142/#review96392
Attachment #8815136 - Flags: review?(boris.chiou) → review+
Comment on attachment 8815139 [details]
Bug 1312301 - Part 10: Move some utility functions into test_common.js.

https://reviewboard.mozilla.org/r/96148/#review96398

::: testing/web-platform/tests/web-animations/testcommon.js:236
(Diff revision 1)
> +    actual.match(matrixRegExp)[1].split(',').map(Number);
> +  var expectedMatrixArray =
> +    expected.match(matrixRegExp)[1].split(',').map(Number);
> +
> +  assert_equals(actualMatrixArray.length, expectedMatrixArray.length,
> +    'dimention of the matrix: ' + description);

nit: s/dimention/dimension/

::: testing/web-platform/tests/web-animations/testcommon.js:238
(Diff revision 1)
> +    expected.match(matrixRegExp)[1].split(',').map(Number);
> +
> +  assert_equals(actualMatrixArray.length, expectedMatrixArray.length,
> +    'dimention of the matrix: ' + description);
> +  for (var i = 0; i < actualMatrixArray.length; i++) {
> +    assert_approx_equals(actualMatrixArray[i], expectedMatrixArray[i], 0.01,

nit: is it possible to use a smaller epsilon, e.g. 0.0001?
Attachment #8815139 - Flags: review?(boris.chiou) → review+
(In reply to Hiroyuki Ikezoe (:hiro) from comment #20)
> Brian, I am sorry for review bombs again (as usual).   If you agree with the
> structure in comment 6, I will ask Boris to review part 4.  But I want to
> ask you for a decision about using computed property name (part 1).
> 
> Thanks!

I will review other parts after part 4 is review granted.
Comment on attachment 8815131 [details]
Bug 1312301 - Part 2: Remove quotation mark from property name.

https://reviewboard.mozilla.org/r/96132/#review96704
Attachment #8815131 - Flags: review?(daisuke) → review+
Comment on attachment 8815132 [details]
Bug 1312301 - Part 3: Use single quote instead of double for consistency.

https://reviewboard.mozilla.org/r/96134/#review96712

The following files also do not have this consistency.
Let's modify them later.

- testing/web-platform/tests/web-animations/animation-model/animation-types/
 - spacing-keyframes-filters.html
 - spacing-keyframes-shapes.html
 - spacing-keyframes-transform.html
- testing/web-platform/tests/web-animations/animation-model/keyframe-effects/
 - spacing-keyframes.html
 - the-effect-value-of-a-keyframe-effect.html
- testing/web-platform/tests/web-animations/interfaces/Animation/
 - constructor.html
 - effect.html
 - finished.html
 - id.html
 - oncancel.html
 - onfinish.html
 - pause.html
 - playbackRate.html
 - ready.html
 - reverse.html
- testing/web-platform/tests/web-animations/interfaces/AnimationEffectTiming/
 - endDelay.html
 - fill.html
- testing/web-platform/tests/web-animations/interfaces/Document/
 - getAnimations.html
- testing/web-platform/tests/web-animations/interfaces/DocumentTimeline/
 - constructor.html
- testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/
 - constructor.html
 - copy-contructor.html
 - effect-easing.html
 - getComputedTiming.html
 - iterationComposite.html
 - setTarget.html
 - spacing.html
- testing/web-platform/tests/web-animations/interfaces/KeyframeEffectReadOnly/
 - copy-contructor.html
 - spacing.html
- testing/web-platform/tests/web-animations/resources/keyframe-utils.js
Attachment #8815132 - Flags: review?(daisuke) → review+
(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> I came up with another idea (which is slightly better I think).  
> 
> var gCSSProperties = {
>   "flex-basis": {
>     // https://drafts.csswg.org/css-flexbox/#propdef-flex-basis
>     tests: [ "lengthPercentageOrCalc", "discrete" ],  // 'test' should be
> 'types'? 
>     discreteKeyframes: [  // This should be 'discreteValuePair'?
>       [ "auto", "10px" ],
>       ...
>     ]
>   },
> };
> 
> And discrete function is like this (it's inside an object):
> 
> var Interpolation = {
>   discrete: function(property, options) {
>     options.discreteKeyframes.forEach(function(keyframes) {
>       assert_equals(keyframes.length, 2,
>                     "keyframe must be a pair of from and to values");
>       var [ from, to ] = keyframes;
>       test(function(t) {
>         ...
>       },

That seems fine. I wonder how it will work with other types that need options though.

Would it be better to make tests an array of either strings or objects containing a 'type' and 'options' member.

e.g.

var properties = {
 'flex-basis': [
    'lengthPercentageOrCalc',
    { type: 'discrete',
      options: {
        pairs: [ [ "auto", "10px" ], ... ]
      }
    }
  ],
  opacity: 'number'
};

That feels a little more javascript-y to me? What do you think?
Flags: needinfo?(bbirtles)
Comment on attachment 8815130 [details]
Bug 1312301 - Part 1: Initialize keyframes with computed property name.

https://reviewboard.mozilla.org/r/96130/#review97384

I think a better reference would be:

  https://kangax.github.io/compat-table/es6/#test-object_literal_extensions
Attachment #8815130 - Flags: review?(bbirtles) → review+
Comment on attachment 8815133 [details]
Bug 1312301 - Part 4: Change gCSSProperties structure to be reused for other procedures.

https://reviewboard.mozilla.org/r/96136/#review97390

I think you said in comment 20 you would ask Boris to review this?

Also, I would be interested to hear your thoughts on comment 27.

I don't mind this, but I wonder if it would become a bit clumsy once other types start to require different options to be passed-in (which I expect they will based on our experience with property_database.js).
Attachment #8815133 - Flags: review?(bbirtles)
(In reply to Brian Birtles (:birtles) from comment #29)
> Comment on attachment 8815133 [details]
> Bug 1312301 - Part 4: Change gCSSProperties structure to be reused for other
> procedures.
> 
> https://reviewboard.mozilla.org/r/96136/#review97390
> 
> I think you said in comment 20 you would ask Boris to review this?
> 
> Also, I would be interested to hear your thoughts on comment 27.
> 
> I don't mind this, but I wonder if it would become a bit clumsy once other
> types start to require different options to be passed-in (which I expect
> they will based on our experience with property_database.js).

I really like the idea, I am going to rewrite with that structure,  I am not sure it works perfectly.  As you know, there needs a global option, see part 9 patch (including an option prerequisite style).  Anyway I will go with the idea.  Thanks!
Looking at part 6, I think I misunderstood comment 6. Is it at all possible to divide the logic up by *type* not by *procedure*.

If I implement a new type (e.g. transform lists), it seems more natural to introduce a new TransformList object with 'interpolate', 'add', and 'accumulate' procedures; than add procedures to Interpolation, Accumulation objects etc.? I am thinking especially of spec authors etc. who in future need to add their type to this framework.

I think it also makes it easier for the framework to deduce which procedure to use. If TransformList.accumulate does not exist, use TransformList.add, and if that does not exist test the default behavior?
Comment on attachment 8815135 [details]
Bug 1312301 - Part 6: Rename type-per-property.html to interpolation-per-property.html

https://reviewboard.mozilla.org/r/96140/#review97398

Cancelling review on this for now based on comment 30 and 31.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/properties.js:1023
(Diff revision 1)
> +// positiveNumber:         function(property, options) // only for flex-grow and
> +//                                                        flex-shrink.

Having to extend in this direction for particular properties feels a bit odd. I think it would be preferable if we can bundle test methods by animation type and not by procedure.
Attachment #8815135 - Flags: review?(bbirtles)
(In reply to Brian Birtles (:birtles) from comment #31)
> Looking at part 6, I think I misunderstood comment 6. Is it at all possible
> to divide the logic up by *type* not by *procedure*.
> 
> If I implement a new type (e.g. transform lists), it seems more natural to
> introduce a new TransformList object with 'interpolate', 'add', and
> 'accumulate' procedures; than add procedures to Interpolation, Accumulation
> objects etc.? I am thinking especially of spec authors etc. who in future
> need to add their type to this framework.
> 
> I think it also makes it easier for the framework to deduce which procedure
> to use. If TransformList.accumulate does not exist, use TransformList.add,
> and if that does not exist test the default behavior?

OK. I will split test files like this:

animation-types/Discrete.html
               /Integer.html
               /Color.html
               /TransformList.html

I am not sure for now we can re-use Length.html inside LengthPercentageOrCalc.html if we split Length.html, but anyway I will try.  And I guess this work will need more time than bug 1311620. So I might drop those test cases from bug 1311620.
Sorry, I just mean the definitions of the functions, not the files. I think it's fine to have a test file for interpolation, and a test file for addition. However, I imagine that we could have a separate property-animation-types.js file which has all the properties and definitions?
So, for example, we could have:

property-list.js:

  const properties = {
    opacity: 'number',
    ...
  };

property-types.js:

  const numberType: {
    testInterpolation: function() {
    },
    testAddition: function() {
    }
  };

  ...

  const types = {
    number: numberType,
    ...
  };

Or something along those lines?
IIRC when I and Brian discussed this at all hands, the property-list.js should become look like this;

const properties = {
  'flex-basis': {
    types: [
      'lengthPercentageOrCalc',
      { type: 'discrete', pairs: [[ 'auto', '10px' ]]
    ]
  },
  'border-bottom-width': {
    types: [ 'length' ],
    setup: function(t) {
      // setting up something which needs to be animated this style. e.g. setting prerequisite styles
    }
  }
};

Two important notes here:
1) We can't put animation types as an array of the each properties, (e.g. 'flex-basis': [ 'length', 'lengthPercentageOrCalc' ]) because we need an extra information for some properties such as prerequisiteStyles or pseudo flag,  so we need 'types' property in each entry.
2) The extra information is set up in 'setup' function,  which will be called before running each test cases.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #36)
> 2) The extra information is set up in 'setup' function,  which will be
> called before running each test cases.

This is not correct.  the setup function should be called at the top of each test function because the function needs a test object of testharness.js. e.g.:

testInterpolate: function(property, setup) {
  test(function(t) {
    var element = setup(t);
  },
  test(function(t) {
    var element = setup(t);
  },
}

As you can see, we should return an element which will be used as the target of animation.
Hello Boris, could you please review all of patches that does not get r+ yet?  Brian has been still busy.
Brian, if there are any suspicions that you are concerned, please feel free to leave comments.

Thank you, both! 

Oops, one more note:  These patches use arrow functions (not all over the place though), we also discussed about using arrow function, and we came to the conclusion we can use there because major browsers already support it.

https://kangax.github.io/compat-table/es6/#test-arrow_functions

Thanks!
(In reply to Hiroyuki Ikezoe (:hiro) from comment #49)
> Hello Boris, could you please review all of patches that does not get r+
> yet?  Brian has been still busy.
> Brian, if there are any suspicions that you are concerned, please feel free
> to leave comments.
> 
> Thank you, both! 
> 
> Oops, one more note:  These patches use arrow functions (not all over the
> place though), we also discussed about using arrow function, and we came to
> the conclusion we can use there because major browsers already support it.
> 
> https://kangax.github.io/compat-table/es6/#test-arrow_functions
> 
> Thanks!

OK, but I'm still in Hawaii, so I might review them a little bit slowly. :)
(In reply to Boris Chiou [:boris] (away, 12/12~12/19) from comment #50)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #49)
> > Hello Boris, could you please review all of patches that does not get r+
> > yet?  Brian has been still busy.
> > Brian, if there are any suspicions that you are concerned, please feel free
> > to leave comments.
> > 
> > Thank you, both! 
> > 
> > Oops, one more note:  These patches use arrow functions (not all over the
> > place though), we also discussed about using arrow function, and we came to
> > the conclusion we can use there because major browsers already support it.
> > 
> > https://kangax.github.io/compat-table/es6/#test-arrow_functions
> > 
> > Thanks!
> 
> OK, but I'm still in Hawaii, so I might review them a little bit slowly. :)

Wow!  No problem after you come back to normal. Enjoy the trip!
Comment on attachment 8815133 [details]
Bug 1312301 - Part 4: Change gCSSProperties structure to be reused for other procedures.

https://reviewboard.mozilla.org/r/96136/#review100088

::: testing/web-platform/tests/web-animations/animation-model/animation-types/type-per-property.html:65
(Diff revision 2)
> +        options: [ [ 'url("http://localhost/test-1")',
> +                   'url("http://localhost/test-2")' ] ] }

nit: the indentation
```
...
options: [ [ 'url("http://localhost/test-1")',
             'url("http://localhost/test-2")' ] ] }
```

::: testing/web-platform/tests/web-animations/animation-model/animation-types/type-per-property.html:115
(Diff revision 2)
> +        options: [ [ 'url("http://localhost/test-1")',
> +                   'url("http://localhost/test-2")' ] ] }

nit: the indentation, as above.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/type-per-property.html:485
(Diff revision 2)
> +        options: [ [ 'url("http://localhost/test-1")',
> +                   'url("http://localhost/test-2")' ] ] }

nit: the indentation, as above metioned.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/type-per-property.html:505
(Diff revision 2)
> +        options: [ [ 'url("http://localhost/test-1")',
> +                   'url("http://localhost/test-2")' ] ] }

nit: the indentation, as above mentioned.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/type-per-property.html:513
(Diff revision 2)
> +        options: [ [ 'url("http://localhost/test-1")',
> +                   'url("http://localhost/test-2")' ] ] }

nit: the indentation, as above mentioned.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/type-per-property.html:521
(Diff revision 2)
> +        options: [ [ 'url("http://localhost/test-1")',
> +                   'url("http://localhost/test-2")' ] ] }

nit: the indentation, as above mentioned.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/type-per-property.html:529
(Diff revision 2)
> +        options: [ [ 'url("http://localhost/test-1")',
> +                   'url("http://localhost/test-2")' ] ] }

nit: the indentation, as above mentioned.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/type-per-property.html:549
(Diff revision 2)
> +        options: [ [ 'url("http://localhost/test-1")',
> +                   'url("http://localhost/test-2")' ] ] }

nit: the indentation, as above mentioned.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/type-per-property.html:708
(Diff revision 2)
> +        options: [ [ 'url("http://localhost/test-1")',
> +                   'url("http://localhost/test-2")' ] ] }

nit: the indentation, as above mentioned.
Attachment #8815133 - Flags: review?(boris.chiou) → review+
Comment on attachment 8815135 [details]
Bug 1312301 - Part 6: Rename type-per-property.html to interpolation-per-property.html

https://reviewboard.mozilla.org/r/96140/#review100104
Attachment #8815135 - Flags: review?(boris.chiou) → review+
Comment on attachment 8815137 [details]
Bug 1312301 - Part 8: Test properties that are animatable as length.

https://reviewboard.mozilla.org/r/96144/#review100110
Attachment #8815137 - Flags: review?(boris.chiou) → review+
Comment on attachment 8815138 [details]
Bug 1312301 - Part 9: Tests for interpolation as color.

https://reviewboard.mozilla.org/r/96146/#review100116
Attachment #8815138 - Flags: review?(boris.chiou) → review+
Comment on attachment 8815140 [details]
Bug 1312301 - Part 11: Tests for interpolation as transformlist.

https://reviewboard.mozilla.org/r/96150/#review100118

LGTM, thanks.
There are some properties whose "types" are still empty, e.g. top, so I think we don't test them now. Do you plan to finish them in other patches/bugs?

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:337
(Diff revision 2)
> +        [{ time: 500,  expected: [ Math.cos(Math.PI * 2 / 4),
> +                                   Math.sin(Math.PI * 2 / 4),
> +                                  -Math.sin(Math.PI * 2 / 4),
> +                                   Math.cos(Math.PI * 2 / 4),

nit: I think using "Math.PI / 2" may be clearer.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:376
(Diff revision 2)
> +        [{ time: 500,  expected: [ Math.cos(Math.PI * 2 / 4),
> +                                   Math.sin(Math.PI * 2 / 4),
> +                                  -Math.sin(Math.PI * 2 / 4),
> +                                   Math.cos(Math.PI * 2 / 4),

nit: as above, "Math.PI / 2".

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:392
(Diff revision 2)
> +        [{ time: 500, expected: [ Math.cos(Math.PI * 2 / 4),
> +                                  Math.sin(Math.PI * 2 / 4),
> +                                 -Math.sin(Math.PI * 2 / 4),
> +                                  Math.cos(Math.PI * 2 / 4),
> +                                  150 * Math.cos(Math.PI * 2 / 4),
> +                                  150 * Math.sin(Math.PI * 2 / 4) ] }]);

nit: as above, "Math.PI / 2".
Attachment #8815140 - Flags: review?(boris.chiou) → review+
Comment on attachment 8815140 [details]
Bug 1312301 - Part 11: Tests for interpolation as transformlist.

https://reviewboard.mozilla.org/r/96150/#review100118

I don't actually have any plans for now, I am hoping someone will do it, but if nobody takes care it at some point , I will do it (I guess while I am implementing additive composite feature in stylo).
Comment on attachment 8815140 [details]
Bug 1312301 - Part 11: Tests for interpolation as transformlist.

https://reviewboard.mozilla.org/r/96150/#review100118

OK. I see. We can finish them later.
Thank you for reviewing, Boris!
Assignee: nobody → hiikezoe
Status: NEW → ASSIGNED
Pushed by hiikezoe@mozilla-japan.org:
https://hg.mozilla.org/integration/autoland/rev/72b448d2273d
Part 1: Initialize keyframes with computed property name. r=birtles
https://hg.mozilla.org/integration/autoland/rev/d3aac357718b
Part 2: Remove quotation mark from property name. r=daisuke
https://hg.mozilla.org/integration/autoland/rev/b91086db4b4b
Part 3: Use single quote instead of double for consistency. r=daisuke
https://hg.mozilla.org/integration/autoland/rev/38f1df3c3f5d
Part 4: Change gCSSProperties structure to be reused for other procedures. r=boris
https://hg.mozilla.org/integration/autoland/rev/065285e9ca84
Part 5: Factor out gCSSProperties and utility functions. r=boris
https://hg.mozilla.org/integration/autoland/rev/47653c9f7281
Part 6: Rename type-per-property.html to interpolation-per-property.html r=boris
https://hg.mozilla.org/integration/autoland/rev/7be30b0d210b
Part 7: Update all properties with layout/style/test/property_database.js. r=boris
https://hg.mozilla.org/integration/autoland/rev/c58bfe9b72d4
Part 8: Test properties that are animatable as length. r=boris
https://hg.mozilla.org/integration/autoland/rev/3976850ae1ac
Part 9: Tests for interpolation as color. r=boris
https://hg.mozilla.org/integration/autoland/rev/e5b3c6c1e14b
Part 10: Move some utility functions into test_common.js. r=boris
https://hg.mozilla.org/integration/autoland/rev/79af3c242ab5
Part 11: Tests for interpolation as transformlist. r=boris
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: