Bug 144487 – Update speedometer patch to provide better UI and fix merge result bug.

• Status: RESOLVED
• Resolution: WONTFIX
• Component: New Bugs

Update speedometer patch to provide better UI and fix merge result bug.

Created attachment 252132 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 252132 [details]
Patch

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

> Tools/Scripts/webkitpy/benchmark_runner/data/patches/Speedometer.patch:80
> ++            var enabledSuites = Suites.filter(function (suite) { return !suite.disabled });
> ++            var values = timeValues.map(function (time) { return 60 * 1000 * enabledSuites.length / time});

Alternatively, we can just use measuredValues.length.  That's probably better here.

> Tools/Scripts/webkitpy/benchmark_runner/data/patches/Speedometer.patch:104
> ++            console.log(results);

Superflous logging.

> Tools/Scripts/webkitpy/benchmark_runner/data/patches/Speedometer.patch:116
> -+ 
> ++

What's up with this change?

> Tools/Scripts/webkitpy/benchmark_runner/data/patches/Speedometer.patch:120
> -+ 
> ++

Ditto.

Created attachment 252136 [details]
Patch

Comment on attachment 252132 [details]
Patch

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

>> Tools/Scripts/webkitpy/benchmark_runner/data/patches/Speedometer.patch:116
>> ++
> 
> What's up with this change?

Delete unnecessary space for blank line.

[Ryosuke Niwa]

Comment on attachment 252136 [details]
Patch

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

> Tools/Scripts/webkitpy/benchmark_runner/data/patches/Speedometer.patch:80
> ++            var values = timeValues.map(function (time) { return 60 * 1000 * enabledSuites.length / time});

I really don't think we should be duplicating the logic to compute the score in this file.
Please modify Performance/Speedometer/resources/main.js to expose this function as a global function and use it here.

Created attachment 252140 [details]
Patch

Created attachment 252142 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 252142 [details]
Patch

Clearing flags on attachment: 252142

Committed r183685: <http://trac.webkit.org/changeset/183685>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 252142 [details]
Patch

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

> PerformanceTests/Speedometer/resources/main.js:55
> +            return computerScore(time);

this says computerScore

> PerformanceTests/Speedometer/resources/main.js:161
> +function computeScore(time) {

this says computeScore

Since they don’t match, does this work? How did we test?

Comment on attachment 252142 [details]
Patch

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

>> PerformanceTests/Speedometer/resources/main.js:161
>> +function computeScore(time) {
> 
> this says computeScore
> 
> Since they don’t match, does this work? How did we test?

Sorry for the typo. I should have tested for both automatic test as well as manual test. Since automatic test will override the benchmarkClient, this error is not trigger. Sorry again. I will fix it immediately.

Reopening to attach new patch.

Created attachment 252173 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 252173 [details]
Patch

This patch can't be applied because the previous patch has already been landed.
Please rebase your patch against ToT.