Bug 149053 – Add a graphics benchmark

• Component: Animations
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

The goals are:

-- Can be publicly released.
-- Can track its results with the Perf Monitor.
-- Can be expanded to include new tests easily.

[Said Abou-Hallawa]

Created attachment 261037 [details]
Patch

[Said Abou-Hallawa]

Created attachment 261323 [details]
Patch

[Said Abou-Hallawa]

Added options to
1. Fix the test particles after the warmup period.
2. Use the measured FPS instead of using the Kalman's filtered FPS.

Use the Student's t distribution to calculate the confidence interval delta for the sampled data at 95% confidence level.

[Simon Fraser (smfr)]

Comment on attachment 261323 [details]
Patch

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

> PerformanceTests/ChangeLog:45
> +        * Animometer/bouncing-canvas-images.html: Added.
> +        * Animometer/bouncing-canvas-shapes.html: Added.
> +        * Animometer/bouncing-images.html: Added.
> +        * Animometer/bouncing-shapes.html: Added.
> +        * Animometer/bouncing-svg-images.html: Added.
> +        * Animometer/bouncing-svg-shapes.html: Added.

I think the tests should go into their own subdirectory, and be as self-contained as possible. We might end up with multiple sets of tests eventually.

> PerformanceTests/Animometer/bouncing-canvas-images.html:12
> +  <script src="resources/main.js"></script>
> +  <script src="resources/statistics.js"></script>

I don't think tests should pull in main.js.

You may want a utilities.js for tests. I don't know why they need statistics.js.

> PerformanceTests/Animometer/bouncing-svg-images.html:4
> +  <link rel="stylesheet" href="resources/benchmark.css">

I don't think tests should pull in any CSS from the harness.

> PerformanceTests/Animometer/resources/benchmark.js:63
> +        var currentTime = new Date().getTime();

performance.now()?

> PerformanceTests/Animometer/resources/benchmark.js:134
> +Benchmark.prototype = {
> +    parseParameters : function() {
> +        var query = window.location.search.substr(1);
> +        var result = {};
> +        query.split("&").forEach(function(part) {
> +            var item = part.split("=");
> +            result[item[0]] = decodeURIComponent(item[1]);
> +        });
> +      return result;
> +    },
> +    // Called from the load event listener or from this.run()

I think this should go into its own file. Separate "harness" code from "benchmark" code.

> PerformanceTests/Animometer/resources/bouncing-canvas-images.js:9
> +function BouncingCanvasImage(stage) {
> +    BouncingParticle.call(this, stage);
> +    this._context = stage.context;
> +    this._imageElement = stage.imageElement;
> +}
> +
> +BouncingCanvasImage.prototype = Object.create(BouncingParticle.prototype);
> +BouncingCanvasImage.prototype.constructor = BouncingCanvasImage;
> +

Each of these files should go into a directory along with its html and resources.

[Said Abou-Hallawa]

Created attachment 261567 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 261323 [details]
Patch

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

>> PerformanceTests/Animometer/bouncing-canvas-images.html:12
>> +  <script src="resources/statistics.js"></script>
> 
> I don't think tests should pull in main.js.
> 
> You may want a utilities.js for tests. I don't know why they need statistics.js.

I restructured the test in the following hierarchy:

resources/  (includes the shared resources and scripts between the runner and the tests)
runner/ (includes the test harness and its resources)
   animometer.html (the  test harness page)
   resources/  (includes the resources and scripts which are used by the  test harness)
tests/
   resources/ (includes the shared resources and the scripts which are needed to run the benchmark for every individual test)
   bouncing-particles/ (includes all the bouncing particles based test pages)
      resources/ (includes all the scripts which are used by the bouncing particles based test pages)
   text/ (includes all the text based test pages)
      resources/ (includes all the scripts which are used by the text based test pages)

>> PerformanceTests/Animometer/bouncing-svg-images.html:4
>> +  <link rel="stylesheet" href="resources/benchmark.css">
> 
> I don't think tests should pull in any CSS from the harness.

There two css files right now:

Animometer/tests/resources/benchmark.css (which is used by all the tests)
Animometer/runner/resources/animometer.css (which us used by the test harness)

>> PerformanceTests/Animometer/resources/benchmark.js:63
>> +        var currentTime = new Date().getTime();
> 
> performance.now()?

Done.

>> PerformanceTests/Animometer/resources/benchmark.js:134
>> +    // Called from the load event listener or from this.run()
> 
> I think this should go into its own file. Separate "harness" code from "benchmark" code.

Done. It is moved to Animometer/tests/resources/utilities.js.

>> PerformanceTests/Animometer/resources/bouncing-canvas-images.js:9
>> +
> 
> Each of these files should go into a directory along with its html and resources.

All the bouncing particles based tests are now in the directory Animometer/tests/bouncing-particles.

[Said Abou-Hallawa]

Created attachment 261571 [details]
Patch

[Said Abou-Hallawa]

Created attachment 261666 [details]
Patch

[Said Abou-Hallawa]

(In reply to comment #8)
> Created attachment 261666 [details]
> Patch

This patch forces repainting all the text in the layering-text.html test.

[Said Abou-Hallawa]

Created attachment 261783 [details]
Patch

[Said Abou-Hallawa]

Created attachment 261784 [details]
results-worst-5-percent

[Said Abou-Hallawa]

(In reply to comment #10)
> Created attachment 261783 [details]
> Patch

This patch include the worst 5% measurement. It shows how smooth the test is running. The bigger the difference between its value and the average is, the more jagged the rendering will be. The final score should be penalized by this how big this difference is for every individual test.

[Said Abou-Hallawa]

Created attachment 261849 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 261849 [details]
Patch

This patch adds a score column for every test. See the picture results-individual-scores.png. The test score calculation is:

test_score = geometric_mean([sample_mean, average_worst_5_percent]).

I fixed also the final score calculation to be:

final_score = geometric_mean(test_scores)

[Said Abou-Hallawa]

Created attachment 261850 [details]
results-individual-scores

[Said Abou-Hallawa]

Created attachment 261856 [details]
Patch

[Said Abou-Hallawa]

Created attachment 261858 [details]
Patch

[Dean Jackson]

Comment on attachment 261858 [details]
Patch

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

This patch is way too huge to review. I'm not sure what most things do. The best I can do is give some comments and rubber-stamp.

If you want to address the comments and then split it into smaller patches, then we can probably do a more careful review.

> PerformanceTests/Animometer/resources/algorithm.js:19
> +    _parentIndex : function(i) {
> +        return i > 0 ? Math.floor((i - 1) / 2) : -1;
> +    },
> +    _leftIndex : function(i) {

In general, WebKit's JS programming style is to not put a space between the symbol name and the definition. And to put the { on the next line.

_parentIndex: function(i)
{

},

Then a blank line to the next thing in the prototype.

(I won't repeat this comment, but it obviously applies to all files)

> PerformanceTests/Animometer/resources/algorithm.js:25
> +    // Returns the child index that may violate the heap property at index i

Remember to end comments with a .
(I won't repeat this everywhere)

> PerformanceTests/Animometer/resources/algorithm.js:39
> +        return !this._size ? NaN : this._values[0];

This would make more sense as
return this._size ? this._values[0] : NaN;

> PerformanceTests/Animometer/resources/main.js:47
> +    width : function() {
> +        return this.left + this.right;
> +    },
> +    height : function() {
> +        return this.top + this.bottom;
> +    }

You could make these getters.

get width()
{
  return this.left + this.right;
}

Then you can just use point.width rather than insets.width().

Same applies to the Point class, and other stuff in the patch.

> PerformanceTests/Animometer/resources/main.js:80
> +SimplePromise.prototype.then = function (callback) {
> +    if (this._callback)
> +        throw "SimplePromise doesn't support multiple calls to then";
> +    this._callback = callback;
> +    this._chainedPromise = new SimplePromise;
> +    
> +    if (this._resolved)
> +        this.resolve(this._resolvedValue);
> +
> +    return this._chainedPromise;
> +}
> +
> +SimplePromise.prototype.resolve = function (value) {
> +    if (!this._callback) {
> +        this._resolved = true;
> +        this._resolvedValue = value;
> +        return;
> +    }
> +
> +    var result = this._callback(value);
> +    if (result instanceof SimplePromise) {
> +        var chainedPromise = this._chainedPromise;
> +        result.then(function (result) { chainedPromise.resolve(result); });
> +    } else
> +        this._chainedPromise.resolve(result);
> +}

Could you use the system Promises if they exist?

> PerformanceTests/Animometer/resources/main.js:103
> +function ProgressBar(element, ranges) {
> +    this.element = element;
> +    this.ranges = ranges;
> +    this.currentRange = 0;
> +}
> +
> +ProgressBar.prototype = {
> +    _progressToPercent : function(progress) {
> +        return progress * (100 / this.ranges);
> +    },
> +    incRange : function() {
> +        ++this.currentRange;
> +    },
> +    setPos : function(progress) {
> +        this.element.style.width = this._progressToPercent(this.currentRange + progress) + "%";
> +    }
> +}

This is a pretty weird class.

It's not really a progress bar. It's something that sets the width of a specified element.

> PerformanceTests/Animometer/resources/main.js:115
> +        for (var title = 0; title < titles.length; ++title) {

You can write this as:

titles.forEach(function (title) {
  // no need to call titles[title] any more

});

> PerformanceTests/Animometer/resources/main.js:117
> +            th.innerHTML = titles[title].text;

Use textContent instead.
th.textContent = title.text;

> PerformanceTests/Animometer/resources/main.js:139
> +            for (var i = 0, len = queue.length; i < len; ++i) {

Seems like this could be a forEach too.

> PerformanceTests/Animometer/resources/main.js:156
> +                for (var i = 0; i < list.length; ++i)

And this.

> PerformanceTests/Animometer/resources/main.js:170
> +    _showSamples : function(row, testName, axises, samples, samplingTimeOffset) {

Should be axes not axises.

> PerformanceTests/Animometer/resources/main.js:176
> +            window.showGraph(testName, axises, samples, samplingTimeOffset);

Seems strange that you install showGraph on the global object. Isn't there a controller object of some kind?

> PerformanceTests/Animometer/resources/main.js:179
> +        button.textContent = "Click...";

Maybe the label should be "Show Graph", or "Graph"

> PerformanceTests/Animometer/resources/main.js:197
> +        for (var measure = 0; measure < sampler.experiments.length; ++measure) {
> +            this._showValue(row, testName, sampler.experiments[measure].mean());
> +            this._showValue(row, testName, sampler.experiments[measure].confidenceIntervalDelta(95));
> +            this._showValue(row, testName, sampler.experiments[measure].concern(5));
> +            this._showValue(row, testName, sampler.experiments[measure].standardDeviation());
> +            this._showValue(row, testName, sampler.experiments[measure].percentage());
> +            axises.push(titles[measure + 1].text);
> +        }

It's a shame you can't replace this with a forEach, because you use the loop counter to look up the title.

As for the delta and concern values, I suggest these be extracted out into some form of global constant at the top of the file.

this._showValue(row, testName, sampler.experiments[measure].concern(INITIAL_CONCERN));

This same comment applies in a few places.

> PerformanceTests/Animometer/resources/main.js:210
> +        for (var testName in suiteSamplers) {

Definitely use suiteSamplers.forEach(function (testName) {.... here.

You'll have to .bind(this), but I still think this is a better overall style.

> PerformanceTests/Animometer/resources/main.js:229
> +            for (var suiteName in iterationsSamplers[i]) {

And again.

> PerformanceTests/Animometer/runner/resources/animometer.css:16
> +main {
> +    display: block;
> +    position: absolute;
> +    width: 800px;
> +    height: 600px;
> +    top: 50%;
> +    left: 50%;
> +    margin-top: -321px;
> +    margin-left: -421px;
> +    padding: 15px;

Seems like you're doing this to center the main element in the page. Just use flexbox.

html, body {
  height: 100%;
}

body {
  display: flex;
  align-items: center;
  justify-content: center;
}

main {
  width: 800px;
  height: 600px;
}

> PerformanceTests/Animometer/runner/resources/animometer.js:105
> +    graph("#graphContainer", new Point(700, 400), new Insets(20, 50, 20, 50), axises, samples, samplingTimeOffset);

This code seems very dependent on the sizes given in the CSS file.

> PerformanceTests/Animometer/runner/resources/benchmark-runner.js:72
> +    frame.style.width = "800px";
> +    frame.style.height = "600px";
> +    frame.style.border = "0px none";
> +    frame.style.position = "absolute";

Same here. Why can't you do this in the CSS?

> PerformanceTests/Animometer/runner/resources/benchmark-runner.js:151
> +    var self = this;
> +    return state.prepareCurrentTest(this, this._frame).then(function(prepareReturnValue) {
> +        return self._runTestAndRecordResults(state);
> +    });

Use bind.

return state.prepareCurrentTest(this, this._frame).then(function(prepareReturnValue) {
   return this._runTestAndRecordResults(state);
 }.bind(this));

> PerformanceTests/Animometer/runner/resources/graph.js:3
> +    var element = element = document.querySelector(selector);

Did you mean to have that second element there?

> PerformanceTests/Animometer/runner/resources/graph.js:5
> +    while (element.firstChild)
> +        element.removeChild(element.firstChild);

Why not call element.textContent = null; ?

> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-canvas-images.js:40
> +    var imageSrc = options["imageSrc"] || "resources/yin-yang.svg";
> +    
> +    var imageElements = document.getElementsByClassName("hidden");
> +    console.assert(imageElements.length);
> +        
> +    for (var i = 0; i < imageElements.length; ++i) {
> +        if (imageElements[i].getAttribute("src") == imageSrc) {
> +            this.imageElement = imageElements[i];
> +            break;
> +        }                                   
> +    }

I think you could probably do this with a single document.querySelector that looks for .hidden and the attribute selector.

You also don't want the console.log.

> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-canvas-shapes.js:25
> +    default:
> +        this._context.fillStyle = this._color0;
> +        break;
> +

Don't put the default case first.

> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-canvas-shapes.js:142
> +window.benchmarkClient.create = function(suite, test, options, recordTable, progressBar) {
> +    window.benchmark = new BouncingCanvasShapesBenchmark(suite, test, options, recordTable, progressBar);
> +}

Again, I feel like it would be better to have a single global controller object, and everything else hanging off it.

> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-particles.js:94
> +    this.svgNamespace = "http://www.w3.org/2000/svg";
> +    this.xlinkNamespace = "http://www.w3.org/1999/xlink";

These should be globals.

The way you're doing it here means you're allocating the string for every instance.

> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-particles.js:116
> +    for(var i = 0; i < this._particles.length; ++i)

forEach opportunity

> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-shapes.js:10
> +    default:
> +        this.element.style.backgroundColor = stage.randomColor();
> +        break;

Don't put default at the start of the switch.

> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-svg-shapes.js:7
> +    default:

Another default first.

> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-svg-shapes.js:21
> +    switch (stage.clip) {
> +    case "star":
> +        stage.ensureClipStarIsCreated();    
> +        this.element.setAttribute("clip-path", "url(#star-clip)");
> +        break;
> +    }

There isn't a point having a switch with only one case and no default. Just make it a conditional.

> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-svg-shapes.js:98
> +    if (typeof this._gadientsCount === "undefined")
> +        this._gadientsCount = 0;    

Typo.

> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-svg-shapes.js:99
> +    gradient.setAttribute("id", "gadient-" + ++this._gadientsCount);

And here.

> PerformanceTests/Animometer/tests/resources/benchmark.js:65
> +        if (typeof this._referenceTime === "undefined")

In cases like this, I think it's ok to simply ask

if (!this._referenceTime)

You can probably initialise it to zero in the constructor to be even more safe.

> PerformanceTests/Animometer/tests/resources/math.js:131
> +function Controller(K, ysp, ulow, uhigh) {

This is a very generic name. What does it actually do?

> PerformanceTests/Animometer/tests/resources/utilities.js:7
> +            if (value[0] == '\'' )

I think you can simply do:

if (value[0] == "'")

[Radar WebKit Bug Importer]

<rdar://problem/22882991>

[Said Abou-Hallawa]

Created attachment 262169 [details]
[shared]

[Said Abou-Hallawa]

Created attachment 262170 [details]
[benchmark]

[Said Abou-Hallawa]

Created attachment 262171 [details]
[stage]

[Said Abou-Hallawa]

Created attachment 262172 [details]
[bouncing-css-particles]

[Said Abou-Hallawa]

Created attachment 262173 [details]
[bouncing-canvas-particles]

[Said Abou-Hallawa]

Created attachment 262174 [details]
[bouncing-svg-particles]

[Said Abou-Hallawa]

Created attachment 262175 [details]
[text]

[Said Abou-Hallawa]

Created attachment 262176 [details]
[template]

[Said Abou-Hallawa]

Created attachment 262177 [details]
[runner]

[Said Abou-Hallawa]

Created attachment 262179 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 262175 [details]
[text]

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

> PerformanceTests/Animometer/tests/text/resources/layering-text.js:42
> +    "<div id='id_00' class='text-layer'>",
> +        "<h2 id='id_01'>Customized styles</h2>",
> +        "<p id='id_02'>If you want formatting choices that are not available from the built-in styles, Quick Style sets, and themes, you can create custom styles to suit your needs.</p>",
> +        "<p id='id_03'>The easiest way to create a custom style is to modify a built-in style and then save it as a new style.</p>",
> +        "<p id='id_04'>For example, you might want to format a paragraph of quoted material with a half-inch indent from the left and right margins, single spaced. There is no built-in style to accommodate this, but you can create a custom style by doing the following:</p>",
> +        "<ol type='1' id='id_05'>",
> +            "<li id='id_06'><p>Click in the paragraph you want to format.</p></li>",
> +            "<li id='id_07'><p>On the <b>Home</b> tab, click the <b>Paragraph</b> Dialog Box Launcher.</p></li>",
> +            "<li id='id_08'><p>In the <b>Indentation</b> section, type <b class='ocpLiteral'>0.5'</b> in the <b>Left</b> and <b>Right</b> boxes.</p></li>",
> +            "<li id='id_09'><p>In the <b>Spacing</b> section, in the <b>Line spacing</b> list, click <b>Single</b>.</p></li>",
> +            "<li id='id_10'><p>Click <b>OK</b>.</p></li>",
> +            "<li id='id_11'><p>Right-click in the paragraph, point to <b>Styles</b>, and then click <b>Save Selection as a New Quick Style</b>.</p></li>",
> +            "<li id='id_12'><p>In the <b>Name</b> box, type a name for the style, such as <b>Block quote</b>. </p></li>",
> +            "<li id='id_13'><p>If you want the style to be included in the gallery of styles on the <b>Home</b> tab, and if you want the style to be a linked style, click <b>OK</b>.</p></li>",
> +            "<li id='id_14'>",
> +                "<p id='id_15'>If you don't want the style to be included in the gallery, or if you want the style to be either a paragraph or a character style, click <b>Modify</b> and do one or both of the following:</p>",
> +                "<ul id='id_16'>",
> +                    "<li id='id_17'><p>At the bottom of the dialog box, clear the <b>Add to Quick Style</b> list box.</p></li>",
> +                    "<li id='id_18'><p>In the <b>Style type</b> list, click <b>Paragraph</b> or <b>Character</b>.</p></li>",
> +                "</ul>",
> +            "</li>",
> +        "</ol>",
> +        "<p id='id_19'>If you switch to a different Quick Style set, you may need to adjust the settings of your custom style. In the example here, if you create the Block quote style while the Word 2007 Quick Style set is applied, and then you switch to the Traditional Quick Style set, you can change the Block quote style to remove the first-line indentation that the Traditional Quick Style set introduces. To change a style, do the following:</p>",
> +        "<ol type='1' id='id_20'>",
> +            "<li id='id_21'><p>On the <b>Home</b> tab in the <b>Styles</b> group, right-click <b>Block quote</b>, and then click <b>Modify</b>.</p></li>",
> +            "<li id='id_22'><p>Click <b>Format</b>, and then click <b>Paragraph</b>.</p></li>",
> +            "<li id='id_23'><p>In the <b>Indentation</b> section, in the <b>Special</b> list, click <b>(none)</b>.</p></li>",
> +        "</ol>",
> +        "<p id='id_24'>The more characteristics that you specify for the style, the less the style is affected by switching Quick Style sets or themes.</p>",
> +    "</div>"

Can we use some text known to be free of copyright? Maybe Shakespeare? Are the id's important? This looks like an exported Word document.

> PerformanceTests/Animometer/tests/text/resources/layering-text.js:47
> +    var praseResult = {};

typo: praseResult

> PerformanceTests/Animometer/tests/text/resources/layering-text.js:53
> +    var spaceIndex = praseResult.tagStart.indexOf(" ");
> +    praseResult.nodeName = praseResult.tagStart.substring(1, spaceIndex != -1 ? spaceIndex : praseResult.tagStart.length - 1);
> +    praseResult.args = spaceIndex != -1 ? window.benchmarkClient.parseArguments(praseResult.tagStart.substring(spaceIndex + 1, praseResult.tagStart.length - 1)) : {};
> +    var tagEnd = "</" + praseResult.nodeName + ">";                
> +    praseResult.tagEnd = textItem.endsWith(tagEnd) ? tagEnd : "";

Can we do this with DOM and not parsing? Could make a document fragment.

> PerformanceTests/Animometer/tests/text/resources/layering-text.js:118
> +    window.benchmarkClient.extendObject(textItemFlags, LayeringTextStage.textItemsFlags[this._textItemIndex]);

Odd to be calling into window.benchmarkClient here.

> PerformanceTests/Animometer/tests/text/resources/layering-text.js:126
> +    window.benchmarkClient.extendObject(textItemFlags, LayeringTextStage.textItemsFlags[this._textItemIndex]);    

And here.

[Simon Fraser (smfr)]

Comment on attachment 262176 [details]
[template]

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

> PerformanceTests/Animometer/tests/template/template-canvas.html:7
> +    <script src="../../resources/main.js"></script>

Should refactor to not pull main.js into tests.

[Simon Fraser (smfr)]

Comment on attachment 262172 [details]
[bouncing-css-particles]

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

> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-css-images.js:45
> +    if (particle.element && particle.element.parentNode && particle.element.parentNode == this.element)
> +        this.element.removeChild(particle.element);

Can this just be particle.element.remove()?

> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-css-shapes.js:63
> +    if (particle.element && particle.element.parentNode && particle.element.parentNode == this.element)
> +        this.element.removeChild(particle.element);

.remove()?

[Simon Fraser (smfr)]

Comment on attachment 262173 [details]
[bouncing-canvas-particles]

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

> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-canvas-images.js:14
> +    this._applyRotation();
> +    this._context.drawImage(this._imageElement, 0, 0, this._size.x, this._size.y);

I think you should push and pop the context state around these lines.

> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-canvas-images.js:48
> +    window.benchmark = new BouncingCanvasImagesBenchmark(suite, test, options, recordTable, progressBar);

Odd to set window.benchmark. Why not return the benchmark?

> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-canvas-shapes.js:55
> +    this._applyFill();
> +    this._applyRotation();
> +    this._applyClipping();
> +    this._drawShape();

You should push/pop around these too.

> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-canvas-shapes.js:87
> +    window.benchmark = new BouncingCanvasShapesBenchmark(suite, test, options, recordTable, progressBar);

Return it?

[Ryosuke Niwa]

Can we not review/land multiple patches on a single bug please?  It's extremely confusing to follow.

[Dean Jackson]

Comment on attachment 262169 [details]
[shared]

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

> PerformanceTests/Animometer/resources/algorithm.js:32
> +    _parentIndex: function(i)
> +    {
> +        return i > 0 ? Math.floor((i - 1) / 2) : -1;
> +    },
> +    
> +    _leftIndex: function(i)
> +    {
> +        return i * 2 + 1 < this._size ? i * 2 + 1 : -1;
> +    },
> +    
> +    _rightIndex: function(i)
> +    {
> +        return i * 2 + 2 < this._size ? i * 2 + 2 : -1;
> +    },

These expressions are a bit hard to parse in my head. Could you add () to make it more clear?

> PerformanceTests/Animometer/resources/algorithm.js:44
> +    {
> +        var l = this._leftIndex(i);
> +        var r = this._rightIndex(i);
> +
> +        if (l != -1 && r != -1)
> +            return this._compare(this._values[l], this._values[r]) > 0 ? l : r;
> +        
> +        return l != -1 ? l : r;
> +    },

Use left and right as variable names because l looks a lot like 1 :)

> PerformanceTests/Animometer/resources/algorithm.js:80
> +        // Fixes the heap poperty at index i given that parent is the only node that

Typo: property

> PerformanceTests/Animometer/resources/algorithm.js:92
> +        // Fixes the heap poperty at index i given that each of the left and the right

Same.

> PerformanceTests/Animometer/resources/algorithm.js:121
> +var Algorithm =
> +{

Strangely, I think for these we do it all on one line.

> PerformanceTests/Animometer/resources/main.js:9
> +    // Used when the point object is used as a size object

Remember punctuation.

> PerformanceTests/Animometer/resources/main.js:15
> +    // Used when the point object is used as a size object    

Ditto.

> PerformanceTests/Animometer/resources/main.js:24
> +    get center()
> +    {
> +        return new Point(this.x / 2, this.y / 2);
> +    },

It's weird that a Point can have a center, so you should comment that this happens when treating a Point as a Size.

[Dean Jackson]

Comment on attachment 262170 [details]
[benchmark]

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

> PerformanceTests/Animometer/tests/resources/benchmark.js:12
> +    messages: [ "Warming up", "Sampling", "Finished" ]

You should put a comment here to say that it must be in the same order/index as the keywords above.

> PerformanceTests/Animometer/tests/resources/math.js:229
> +        // Current error
> +        var e = this._ysp - y;
> +
> +        // Proportional term
> +        var P = this._Kp * e;
> +        
> +        // Derivative term is the slope of the curve at the current time
> +        var D = this._Kd * (e - this._eold) / h;
> +
> +        // Integral term is the area under the curve starting from the begining till the current time
> +        this._I += this._Ki * ((e + this._eold) / 2) * h;
> +
> +        // pid controller value
> +        var v = P + D + this._I;
> +        
> +        // Avoid spikes by applying actuator satuartion
> +        var u = this._sat(v, this._ulow, this._uhigh);        

Punctuation. Plus a typo in saturation.

> PerformanceTests/Animometer/tests/resources/math.js:271
> +        // Update the transition matrix
> +        this._matA[Matrix3.pos(0, 2)] = 1;
> +
> +        // Predicted state and covariance
> +        var vecX_prd = Matrix3.multiplyVector3(this._matA, this._vecX_est);
> +        var matP_prd = Matrix3.add(Matrix3.multiplyMatrix3(Matrix3.multiplyMatrix3(this._matA, this._matP_est), Matrix3.transpose(this._matA)), this._matQ);
> +
> +        // Estimation
> +        var vecB = Vector3.multiplyMatrix3(this._vecH, Matrix3.transpose(matP_prd));
> +        var S = Vector3.multiplyVector3(vecB, this._vecH) + this._R;
> +
> +        var vecGain = Vector3.scale(1/S, vecB);
> +        
> +        // Estimated state and covariance
> +        this._vecX_est = Vector3.add(vecX_prd, Vector3.scale(current - Vector3.multiplyVector3(this._vecH, vecX_prd), vecGain));
> +        this._matP_est = Matrix3.subtract(matP_prd, Matrix3.scale(Vector3.multiplyVector3(vecGain, this._vecH), matP_prd));
> +
> +        // Compute the estimated measurement

Punctuation.

[Dean Jackson]

Comment on attachment 262174 [details]
[bouncing-svg-particles]

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

> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-svg-images.js:7
> +    var attrs = { 'x': 0, 'y': 0, 'width': this._size.x, 'height': this._size.y };
> +    var xlinkAttrs = { 'href': stage.imageSrc };

You don't need the '' around the keys here.

> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-svg-shapes.js:21
> +        var attrs = { 'x': 0, 'y': 0, 'width': this._size.x, 'height': this._size.y };

Or here.

> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-svg-shapes.js:27
> +        var attrs = { 'cx': this._size.x / 2, 'cy': this._size.y / 2, 'r': Math.min(this._size.x, this._size.y) / 2 };

Or here.

[Said Abou-Hallawa]

Comment on attachment 262169 [details]
[shared]

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

>> PerformanceTests/Animometer/resources/algorithm.js:32
>> +    },
> 
> These expressions are a bit hard to parse in my head. Could you add () to make it more clear?

Done.

>> PerformanceTests/Animometer/resources/algorithm.js:44
>> +    },
> 
> Use left and right as variable names because l looks a lot like 1 :)

Done.

>> PerformanceTests/Animometer/resources/algorithm.js:80
>> +        // Fixes the heap poperty at index i given that parent is the only node that
> 
> Typo: property

Done.

>> PerformanceTests/Animometer/resources/algorithm.js:92
>> +        // Fixes the heap poperty at index i given that each of the left and the right
> 
> Same.

Done.

>> PerformanceTests/Animometer/resources/algorithm.js:121
>> +{
> 
> Strangely, I think for these we do it all on one line.

Done.

>> PerformanceTests/Animometer/resources/main.js:9
>> +    // Used when the point object is used as a size object
> 
> Remember punctuation.

Done.

>> PerformanceTests/Animometer/resources/main.js:15
>> +    // Used when the point object is used as a size object    
> 
> Ditto.

Done.

>> PerformanceTests/Animometer/resources/main.js:24
>> +    },
> 
> It's weird that a Point can have a center, so you should comment that this happens when treating a Point as a Size.

Done.

This patch was moved to https://bugs.webkit.org/show_bug.cgi?id=149691.

[Said Abou-Hallawa]

Created attachment 262284 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 262170 [details]
[benchmark]

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

>> PerformanceTests/Animometer/tests/resources/math.js:229
>> +        var u = this._sat(v, this._ulow, this._uhigh);        
> 
> Punctuation. Plus a typo in saturation.

Done.

>> PerformanceTests/Animometer/tests/resources/math.js:271
>> +        // Compute the estimated measurement
> 
> Punctuation.

Done.

[Said Abou-Hallawa]

Comment on attachment 262172 [details]
[bouncing-css-particles]

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

>> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-css-images.js:45
>> +        this.element.removeChild(particle.element);
> 
> Can this just be particle.element.remove()?

Done.

>> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-css-shapes.js:63
>> +        this.element.removeChild(particle.element);
> 
> .remove()?

Done.

[Said Abou-Hallawa]

Comment on attachment 262173 [details]
[bouncing-canvas-particles]

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

>> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-canvas-images.js:48
>> +    window.benchmark = new BouncingCanvasImagesBenchmark(suite, test, options, recordTable, progressBar);
> 
> Odd to set window.benchmark. Why not return the benchmark?

Done. It now return new BouncingCanvasImagesBenchmark and the caller sets the return value to window.benchmark.

>> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-canvas-shapes.js:55
>> +    this._drawShape();
> 
> You should push/pop around these too.

Done.

>> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-canvas-shapes.js:87
>> +    window.benchmark = new BouncingCanvasShapesBenchmark(suite, test, options, recordTable, progressBar);
> 
> Return it?

Done. It now return new BouncingCanvasShapesBenchmark and the caller sets the return value to window.benchmark.

[Said Abou-Hallawa]

Comment on attachment 262174 [details]
[bouncing-svg-particles]

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

>> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-svg-images.js:7
>> +    var xlinkAttrs = { 'href': stage.imageSrc };
> 
> You don't need the '' around the keys here.

All ' ' were removed expect for 'stop-color' in BouncingSvgShapesStage.prototype.createGradient.

>> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-svg-shapes.js:21
>> +        var attrs = { 'x': 0, 'y': 0, 'width': this._size.x, 'height': this._size.y };
> 
> Or here.

Done.

>> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-svg-shapes.js:27
>> +        var attrs = { 'cx': this._size.x / 2, 'cy': this._size.y / 2, 'r': Math.min(this._size.x, this._size.y) / 2 };
> 
> Or here.

Done.

[Said Abou-Hallawa]

Comment on attachment 262175 [details]
[text]

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

>> PerformanceTests/Animometer/tests/text/resources/layering-text.js:42
>> +    "</div>"
> 
> Can we use some text known to be free of copyright? Maybe Shakespeare? Are the id's important? This looks like an exported Word document.

Done. I got some text from https://en.wikipedia.org/wiki/Benchmark_(computing).

>> PerformanceTests/Animometer/tests/text/resources/layering-text.js:47
>> +    var praseResult = {};
> 
> typo: praseResult

Fixed.

>> PerformanceTests/Animometer/tests/text/resources/layering-text.js:53
>> +    praseResult.tagEnd = textItem.endsWith(tagEnd) ? tagEnd : "";
> 
> Can we do this with DOM and not parsing? Could make a document fragment.

Will do that later.

>> PerformanceTests/Animometer/tests/text/resources/layering-text.js:118
>> +    window.benchmarkClient.extendObject(textItemFlags, LayeringTextStage.textItemsFlags[this._textItemIndex]);
> 
> Odd to be calling into window.benchmarkClient here.

Done. extendObject() was moved to the Utilities object.

>> PerformanceTests/Animometer/tests/text/resources/layering-text.js:126
>> +    window.benchmarkClient.extendObject(textItemFlags, LayeringTextStage.textItemsFlags[this._textItemIndex]);    
> 
> And here.

Done. extendObject() was moved to the Utilities object.

[Said Abou-Hallawa]

Comment on attachment 262176 [details]
[template]

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

>> PerformanceTests/Animometer/tests/template/template-canvas.html:7
>> +    <script src="../../resources/main.js"></script>
> 
> Should refactor to not pull main.js into tests.

Done. The shared objects like Point, Insets, etc were moved to a new file called Animometer/resources/extensions.js. The Animator, the Benchmark and running the benchmark itself are in Animometer/tests/resources/main.js.

[Said Abou-Hallawa]

Created attachment 262304 [details]
Patch

[Said Abou-Hallawa]

Now there three bugs with three separate patches:

https://bugs.webkit.org/show_bug.cgi?id=149691: has shared code between the runner and the test benchmark.
This bug: has the benchmark and the tests.
https://bugs.webkit.org/show_bug.cgi?id=149683: has the benchmark runner.

They can be landed separately.

[Said Abou-Hallawa]

Created attachment 262368 [details]
Patch

[Said Abou-Hallawa]

Added two examples for other animation techniques: 

tests/examples/canvas-electrons.html
tests/examples/canvas-stars.html

[Said Abou-Hallawa]

Created attachment 262369 [details]
Patch

[Said Abou-Hallawa]

The references to the statistics.js is removed. The confidence interval delta measurement is removed from the results table.

[Ryosuke Niwa]

Comment on attachment 262369 [details]
Patch

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

> PerformanceTests/ChangeLog:296
> +        * Animometer/tests/resources/yin-yang.png: Added.
> +        * Animometer/tests/resources/yin-yang.svg: Added.
> +        These images are shared among all the tests.

Did you make these png images?

If not, are they BSD-licensed? Depending on the terms of their licenses, we may still need to include the credits.

> PerformanceTests/Animometer/tests/resources/stage.css:2
> +  height: 100%;

Wrong indentation. Use 4 spaces instead.

> PerformanceTests/Animometer/tests/resources/yin-yang.svg:2
> +  <defs>

Again, wrong indentation.
Use 4 spaces instead.

> PerformanceTests/Animometer/tests/template/resources/template-canvas.js:6
> +    // TODO: Fill in your object data.

We don't use TODO.
This since is a template, I don't think FIXME is appropriate either.
Just remove TODO: everywhere in this file.

> PerformanceTests/Animometer/tests/template/resources/template-css.js:15
> +    // TODO: Change objects in the stage.

Ditto about removing TODO:s in this file.

> PerformanceTests/Animometer/tests/template/resources/template-svg.js:15
> +    // TODO: Change objects in the stage.

Ditto.

[Said Abou-Hallawa]

Created attachment 262378 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 262369 [details]
Patch

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

>> PerformanceTests/ChangeLog:296
>> +        These images are shared among all the tests.
> 
> Did you make these png images?
> 
> If not, are they BSD-licensed? Depending on the terms of their licenses, we may still need to include the credits.

Yes. I made the SVG by hand and then took a screenshot from it and save it as PNG.

>> PerformanceTests/Animometer/tests/resources/stage.css:2
>> +  height: 100%;
> 
> Wrong indentation. Use 4 spaces instead.

Done.

>> PerformanceTests/Animometer/tests/resources/yin-yang.svg:2
>> +  <defs>
> 
> Again, wrong indentation.
> Use 4 spaces instead.

Done.

>> PerformanceTests/Animometer/tests/template/resources/template-canvas.js:6
>> +    // TODO: Fill in your object data.
> 
> We don't use TODO.
> This since is a template, I don't think FIXME is appropriate either.
> Just remove TODO: everywhere in this file.

Done.

>> PerformanceTests/Animometer/tests/template/resources/template-css.js:15
>> +    // TODO: Change objects in the stage.
> 
> Ditto about removing TODO:s in this file.

Done.

>> PerformanceTests/Animometer/tests/template/resources/template-svg.js:15
>> +    // TODO: Change objects in the stage.
> 
> Ditto.

Done.

[Dean Jackson]

Comment on attachment 262378 [details]
Patch

rs=me, based on previous reviews, and that rniwa's comments were addressed.

[WebKit Commit Bot]

Comment on attachment 262378 [details]
Patch

Clearing flags on attachment: 262378

Committed r190575: <http://trac.webkit.org/changeset/190575>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.