Bug 149683 – Add the test runner for a new a graphics benchmark

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

[Said Abou-Hallawa]

We need a framework for a graphics benchmark test runner. This patch is separated from the patch attached to https://bugs.webkit.org/show_bug.cgi?id=149053.

[Said Abou-Hallawa]

Created attachment 262199 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 262199 [details]
Patch

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

> PerformanceTests/ChangeLog:14
> +        * Animometer: Added.

Please skip this directory in PerformanceTests/Skipped.

> PerformanceTests/Animometer/runner/animometer.html:5
> +    <script src="https://cdnjs.cloudflare.com/ajax/libs/d3/3.5.5/d3.min.js" defer></script>  

We should not pull scripts from CDNs.  We should put a copy in the repo instead.

> PerformanceTests/Animometer/runner/resources/animometer.js:104
> +    var browser = benchmarkRunnerClient.browser();    
> +    if (browser.length)
> +        element.textContent += " [browser = " + browser + "]";

Why does this need to be included in the results?
The user running the browser is perfectly aware of which browser he/she is running.
Please remove this code as well as benchmarkRunnerClient.browser
so that people don't mistakenly think we're running different code based on user agent strings.

> PerformanceTests/Animometer/runner/resources/benchmark-runner.js:1
> +function BenchmarkRunnerState(suites)

This code is basically copied from Speedometer.
Instead of making a copy-paste, please do "svn cp" so that we can see the diff.

> PerformanceTests/Animometer/runner/resources/tests.js:47
> +    name: "HTML Bouncing Particles",

It's best if we added these suites as we added the actual test contents.
Please remove them for now.

[Jon Lee]

(In reply to comment #2)
> Comment on attachment 262199 [details]
> Patch
> 
> We should not pull scripts from CDNs.  We should put a copy in the repo
> instead.

This script is used to visualize the data after the test is run; it is not used for any test. Right now we can't include a copy, so my suggestion is we leave it as is now, and file a follow-up task to either get rid of it completely or get approval.

[Ryosuke Niwa]

Well, we can just svn cp from Websites/perf.webkit.org/public/v2/js/d3/d3.min.js

[Dean Jackson]

Comment on attachment 262199 [details]
Patch

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

>> PerformanceTests/Animometer/runner/animometer.html:5
>> +    <script src="https://cdnjs.cloudflare.com/ajax/libs/d3/3.5.5/d3.min.js" defer></script>  
> 
> We should not pull scripts from CDNs.  We should put a copy in the repo instead.

Just make sure we've got approval to put this in the repository.

> PerformanceTests/Animometer/runner/resources/animometer.css:221
> +.left-samples {
> +  fill: none;
> +  stroke: steelblue;
> +  stroke-width: 1.5px;
> +}
> +
> +.right-samples {
> +    fill: none;
> +    stroke: red;
> +    stroke-width: 1.5px;
> +}
> +
> +.smaple-time {
> +    fill: none;
> +    stroke: green;
> +    stroke-width: 1.5px;
> +}
> +
> +.left-mean {
> +    fill: none;
> +    stroke: yellow;
> +    stroke-width: 1.5px;
> +}
> +
> +.right-mean {
> +    fill: none;
> +    stroke: magenta;
> +    stroke-width: 1.5px;
> +}

You can share the rules for fill and stroke-width here.

[Said Abou-Hallawa]

Created attachment 262303 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 262199 [details]
Patch

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

>> PerformanceTests/ChangeLog:14
>> +        * Animometer: Added.
> 
> Please skip this directory in PerformanceTests/Skipped.

Done.

>>> PerformanceTests/Animometer/runner/animometer.html:5
>>> +    <script src="https://cdnjs.cloudflare.com/ajax/libs/d3/3.5.5/d3.min.js" defer></script>  
>> 
>> We should not pull scripts from CDNs.  We should put a copy in the repo instead.
> 
> Just make sure we've got approval to put this in the repository.

Done. I made svn cp Websites/perf.webkit.org/public/v2/js/d3/d3.min.js.

>> PerformanceTests/Animometer/runner/resources/animometer.js:104
>> +        element.textContent += " [browser = " + browser + "]";
> 
> Why does this need to be included in the results?
> The user running the browser is perfectly aware of which browser he/she is running.
> Please remove this code as well as benchmarkRunnerClient.browser
> so that people don't mistakenly think we're running different code based on user agent strings.

Done. The browser name was removed. I was helpful when getting a screenshot of the results and paste it in an email.

>> PerformanceTests/Animometer/runner/resources/benchmark-runner.js:1
>> +function BenchmarkRunnerState(suites)
> 
> This code is basically copied from Speedometer.
> Instead of making a copy-paste, please do "svn cp" so that we can see the diff.

Done.

>> PerformanceTests/Animometer/runner/resources/tests.js:47
>> +    name: "HTML Bouncing Particles",
> 
> It's best if we added these suites as we added the actual test contents.
> Please remove them for now.

Done. This file is included along with the tests in the patch attached to https://bugs.webkit.org/show_bug.cgi?id=149053.

[Ryosuke Niwa]

Comment on attachment 262303 [details]
Patch

rs=me.

[WebKit Commit Bot]

Comment on attachment 262303 [details]
Patch

Clearing flags on attachment: 262303

Committed r190526: <http://trac.webkit.org/changeset/190526>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.