Bug 170198 – MotionMark tests using Date.now() to drive animations don't work under web-page-replay

• Status: NEW
• Component: Tools / Tests

A number of the MotionMark tests animate properties based on the Date.now() value.  This is via utility methods Stage.dateCounterValue() or Stage.dateFractionalValue().

Chromium's web-page-replay stubs out Date.now() to improve page replay determinism, and this breaks the animation progression on some of the tests.

Would it make sense to replace the Data.now() usage with consistent use of the Benchmark.timestamp()?

Created attachment 305671 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 305671 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=305671&action=review

> PerformanceTests/ChangeLog:7
> +

Thanks for contributing the patch but please describe your change and add inline comments.
r- due to lack of change log comments.

Created attachment 305682 [details]
Patch

(In reply to Ryosuke Niwa from comment #2)
> Comment on attachment 305671 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=305671&action=review
> 
> > PerformanceTests/ChangeLog:7
> > +
> 
> Thanks for contributing the patch but please describe your change and add
> inline comments.
> r- due to lack of change log comments.

Thanks for the quick review.  Added change description.

For inline comments, I tried to match the style of the existing code.  Are there specific parts you think could benefit from additional comments?

[Ryosuke Niwa]

Comment on attachment 305682 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=305682&action=review

Useful inline comments would explain why you're making each code change, and how it impacts the rest of the codebase.

> PerformanceTests/MotionMark/tests/3d/resources/webgl.js:143
> -                this._startTime = Stage.dateCounterValue(1000);
> -            var elapsedTime = Stage.dateCounterValue(1000) - this._startTime;
> +                this._startTime = this.timestampCounterValue(1000);
> +            var elapsedTime = this.timestampCounterValue(1000) - this._startTime;

Why are we replacing Stage.dateCounterValue with this.timestampCounterValue?
What's the significance of it? e.g. does Stage.dateCounterValue use Date.now() and this.timestampCounterValue doesn't?
None of that is clear from your main change log description.

> PerformanceTests/MotionMark/tests/master/resources/canvas-tests.js:19
> +        this._stage = stage;

A useful inline comment here would be explaining why we need to store "stage" as an instance variable.

> PerformanceTests/MotionMark/tests/resources/main.js:654
> +    set timestamp(stamp)
> +    {
> +        this._currentTimestamp = stamp;
> +    },
> +
> +    get timestamp()
> +    {
> +        return this._currentTimestamp;
> +    },
> +

What are these getters and setters used for? Who is getting the value?

> PerformanceTests/MotionMark/tests/resources/main.js:676
> +    rotatingColor: function(cycleLengthMs, saturation, lightness)
> +    {

Why are you moving these methods here?
That should be explained in the change log.

[Maciej Stachowiak]

Comment on attachment 305682 [details]
Patch

Seems like a good idea. Unfortunately, the patch no longer applies as-is.