Bug 152458 – Split benchmark into two different pages

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Jon Lee]

One version is simpler to use, the other has more options

[Jon Lee]

Created attachment 267858 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/24002508>

[Jon Lee]

Created attachment 267859 [details]
1. Move benchmark resources out

[Jon Lee]

Created attachment 267860 [details]
2. Split tests.js into two

[Jon Lee]

Created attachment 267861 [details]
3. Get rid of utilities.js

[Jon Lee]

Created attachment 267862 [details]
4. Add newer version of harness

[Jon Lee]

Created attachment 267863 [details]
5. A little stylistic cleanup.

[Jon Lee]

Created attachment 267864 [details]
6. Fix a few fly-by errors.

[Jon Lee]

Created attachment 267865 [details]
7. Make it possible to select

[Jon Lee]

Created attachment 267866 [details]
8. Add tests to 'animometer' suite.

[Jon Lee]

Created attachment 267867 [details]
9. Fix selector

[Jon Lee]

Created attachment 267868 [details]
10. Update debug runner

[Jon Lee]

Created attachment 267869 [details]
11. Update the tests.

[Jon Lee]

Created attachment 267871 [details]
12. Update the structure of the harness.

[Jon Lee]

Created attachment 267872 [details]
13. Get rid of prefixed flex properties for now.

[Jon Lee]

Created attachment 267873 [details]
14. Adjustment styles for iOS.

[Jon Lee]

Created attachment 267874 [details]
15. Adjust styles for iPad.

[Simon Fraser (smfr)]

Comment on attachment 267862 [details]
4. Add newer version of harness

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

> PerformanceTests/Animometer/resources/runner/animometer.css:67
> +    border: 2px solid rgb(96, 96, 96);
> +    color: rgb(96, 96, 96);

Shame we can't use variables!

[Jon Lee]

Created attachment 267875 [details]
16. Move Array functions to extensions.js

[Simon Fraser (smfr)]

Comment on attachment 267865 [details]
7. Make it possible to select

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

> PerformanceTests/Animometer/resources/runner/animometer.js:92
> +        if (event.charCode != 115) // 's'
> +            return;

Is there any visible text that documents this?

[Jon Lee]

(In reply to comment #20)
> Comment on attachment 267865 [details]
> 7. Make it possible to select
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267865&action=review
> 
> > PerformanceTests/Animometer/resources/runner/animometer.js:92
> > +        if (event.charCode != 115) // 's'
> > +            return;
> 
> Is there any visible text that documents this?

No, but I do mention it in developer.html.

[Simon Fraser (smfr)]

Comment on attachment 267866 [details]
8. Add tests to 'animometer' suite.

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

> PerformanceTests/Animometer/tests/master/resources/canvas-tests.js:5
> +function CanvasLineSegment(stage) {

Braces on a new line please.

[Simon Fraser (smfr)]

Comment on attachment 267867 [details]
9. Fix selector

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

> PerformanceTests/Animometer/resources/debug-runner/animometer.js:15
> +        this.recordTable = new ResultsTable(document.querySelector("#test-container .results-table"), Headers);

Shouldn't results-table be an ID if there's only one?

[Simon Fraser (smfr)]

Comment on attachment 267869 [details]
11. Update the tests.

I would like to see new tests, and test changes landed separately from the benchmark harness changes.

[Simon Fraser (smfr)]

Comment on attachment 267873 [details]
14. Adjustment styles for iOS.

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

> PerformanceTests/Animometer/resources/runner/animometer.css:171
> +        width: calc(100%);
> +        height: calc(100%);

Is that just equivalent to 100%?

[Simon Fraser (smfr)]

Comment on attachment 267875 [details]
16. Move Array functions to extensions.js

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

> PerformanceTests/Animometer/resources/extensions.js:9
> +if (!Array.prototype.fill)  {

Odd spacing.

> PerformanceTests/Animometer/resources/extensions.js:23
> +        for (; k < final; k++) {
> +            object[k] = value;
> +        }

No braces.

> PerformanceTests/Animometer/resources/extensions.js:32
> +        if (this === null)
> +            throw new TypeError('Array.prototype.find called on null or undefined');

Why check here, and not for fill?

[Jon Lee]

(In reply to comment #22)
> Comment on attachment 267866 [details]
> 8. Add tests to 'animometer' suite.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267866&action=review
> 
> > PerformanceTests/Animometer/tests/master/resources/canvas-tests.js:5
> > +function CanvasLineSegment(stage) {
> 
> Braces on a new line please.

Done.

(In reply to comment #23)
> Comment on attachment 267867 [details]
> 9. Fix selector
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267867&action=review
> 
> > PerformanceTests/Animometer/resources/debug-runner/animometer.js:15
> > +        this.recordTable = new ResultsTable(document.querySelector("#test-container .results-table"), Headers);
> 
> Shouldn't results-table be an ID if there's only one?

This gets removed later.

(In reply to comment #24)
> Comment on attachment 267869 [details]
> 11. Update the tests.
> 
> I would like to see new tests, and test changes landed separately from the
> benchmark harness changes.

Will do.

(In reply to comment #25)
> Comment on attachment 267873 [details]
> 14. Adjustment styles for iOS.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267873&action=review
> 
> > PerformanceTests/Animometer/resources/runner/animometer.css:171
> > +        width: calc(100%);
> > +        height: calc(100%);
> 
> Is that just equivalent to 100%?

Yes. Fixed.

(In reply to comment #26)
> Comment on attachment 267875 [details]
> 16. Move Array functions to extensions.js
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267875&action=review
> 
> > PerformanceTests/Animometer/resources/extensions.js:9
> > +if (!Array.prototype.fill)  {
> 
> Odd spacing.

Fixed.

> 
> > PerformanceTests/Animometer/resources/extensions.js:23
> > +        for (; k < final; k++) {
> > +            object[k] = value;
> > +        }
> 
> No braces.

Fixed.

> > PerformanceTests/Animometer/resources/extensions.js:32
> > +        if (this === null)
> > +            throw new TypeError('Array.prototype.find called on null or undefined');
> 
> Why check here, and not for fill?

It should. Added. The check also needs to be fixed to ==.

[Jon Lee]

Created attachment 267878 [details]
17. Follow-up patch

[Jon Lee]

Created attachment 267881 [details]
Patch to commit; harness

[Jon Lee]

Created attachment 267882 [details]
Patch to commit; tests

[Simon Fraser (smfr)]

Comment on attachment 267881 [details]
Patch to commit; harness

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

> PerformanceTests/Animometer/resources/debug-runner/animometer.js:25
> +window.optionsManager =
> +{

brace on same line.

> PerformanceTests/Animometer/resources/debug-runner/benchmark-runner.js:140
> +    if (state.isFirstTest()) {
> +        this._appendFrame();
> +    }

No braces.

> PerformanceTests/Animometer/resources/runner/animometer.js:29
> +window.sectionsManager =
> +{

brace on same line.

[Simon Fraser (smfr)]

Comment on attachment 267882 [details]
Patch to commit; tests

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

> PerformanceTests/Animometer/tests/master/resources/canvas-stage.js:9
> +}
> +SimpleCanvasStage.prototype = Object.create(Stage.prototype);

I would prefer blank lines after functions.

> PerformanceTests/Animometer/tests/master/resources/canvas-stage.js:58
> +function SimpleCanvasBenchmark(suite, test, options, progressBar) {
> +    StageBenchmark.call(this, suite, test, options, progressBar);
> +}
> +SimpleCanvasBenchmark.prototype = Object.create(StageBenchmark.prototype);
> +SimpleCanvasBenchmark.prototype.constructor = SimpleCanvasBenchmark;
> +SimpleCanvasBenchmark.prototype.createAnimator = function() {

Braces on new line.

> PerformanceTests/Animometer/tests/master/resources/canvas-tests.js:221
> +})();

LOL JS

[Jon Lee]

Committed r194407: <http://trac.webkit.org/changeset/194407>

[Jon Lee]

Committed r194408: <http://trac.webkit.org/changeset/194408>