Bug 150528 – Add an option to make the graphics benchmark runs a specific test

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

[Said Abou-Hallawa]

This option should be useful for profiling and testing a specific test.

[Radar WebKit Bug Importer]

<rdar://problem/23246614>

[Jon Lee]

Created attachment 264123 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 264123 [details]
Patch

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

> PerformanceTests/Animometer/runner/animometer.html:34
> +                    <label><input id="show-running-results" type="checkbox" /> Show running results</label>

void elements like <br> and <input> do not require slashes at the end.

> PerformanceTests/Animometer/runner/resources/animometer.js:76
> +    for (var i = 0; i < suiteItems.length; ++i) {

Can't we use forEach() here?

> PerformanceTests/Animometer/runner/resources/animometer.js:85
> +        for (var j = 0; j < testCheckboxes.length; ++j) {

Can't we use forEach() here?

> PerformanceTests/Animometer/runner/resources/animometer.js:87
> +            if (testCheckbox.checked) {

Do we need to flip the condition and use continue like the if(!suiteCheckbox.checked) above?

> PerformanceTests/Animometer/runner/resources/animometer.js:171
> +function initialize() {

I am not sure about the style guidelines here. I was putting the opening brace of the function and the class on the same line like what you do. But I got a comment from Dean that the opening braces of the function and the class should be in a separate line.

> PerformanceTests/Animometer/runner/resources/animometer.js:186
> +    for (var i = 0; i < testCheckboxes.length; ++i) {

Can't we use forEach() here?

> PerformanceTests/Animometer/runner/resources/animometer.js:200
> +    var noneSelected = true;

Can't we used:
    var cSelected = 0;

> PerformanceTests/Animometer/runner/resources/animometer.js:205
> +        noneSelected = noneSelected && !testCheckbox.checked;

cSelected += testCheckbox.checked;

> PerformanceTests/Animometer/runner/resources/animometer.js:207
> +    if (allSelected && !noneSelected) {

if (cSelected == testCheckboxes.length)

> PerformanceTests/Animometer/runner/resources/animometer.js:211
> +    else if (!allSelected && noneSelected) {

if (!cSelected)

> PerformanceTests/Animometer/runner/resources/animometer.js:218
> +    }

We can even simplify the code and replace the whole if-statment by:
    suiteCheckbox.checked = cSelected > 0;
    suiteCheckbox.indeterminate = cSelected > 0 && cSelected < testCheckboxes.length;

[Jon Lee]

Comment on attachment 264123 [details]
Patch

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

>> PerformanceTests/Animometer/runner/animometer.html:34
>> +                    <label><input id="show-running-results" type="checkbox" /> Show running results</label>
> 
> void elements like <br> and <input> do not require slashes at the end.

I can remove. That generally always bothered me but I don't know if we have a style policy of this.

>> PerformanceTests/Animometer/runner/resources/animometer.js:76
>> +    for (var i = 0; i < suiteItems.length; ++i) {
> 
> Can't we use forEach() here?

NodeLists are not Arrays.

>> PerformanceTests/Animometer/runner/resources/animometer.js:85
>> +        for (var j = 0; j < testCheckboxes.length; ++j) {
> 
> Can't we use forEach() here?

NodeLists are not Arrays.

>> PerformanceTests/Animometer/runner/resources/animometer.js:87
>> +            if (testCheckbox.checked) {
> 
> Do we need to flip the condition and use continue like the if(!suiteCheckbox.checked) above?

I didn't because the body wasn't large enough. But I can do it.

>> PerformanceTests/Animometer/runner/resources/animometer.js:186
>> +    for (var i = 0; i < testCheckboxes.length; ++i) {
> 
> Can't we use forEach() here?

NodeLists are not Arrays.

>> PerformanceTests/Animometer/runner/resources/animometer.js:218
>> +    }
> 
> We can even simplify the code and replace the whole if-statment by:
>     suiteCheckbox.checked = cSelected > 0;
>     suiteCheckbox.indeterminate = cSelected > 0 && cSelected < testCheckboxes.length;

Done.

[Jon Lee]

Created attachment 264189 [details]
Patch

[Said Abou-Hallawa]

unofficial r=me

[Jon Lee]

Committed r191701: <http://trac.webkit.org/changeset/191701>