Bug 144487

Summary: Update speedometer patch to provide better UI and fix merge result bug.
Product: WebKit Reporter: dewei_zhu
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED WONTFIX    
Severity: Normal CC: commit-queue, dewei_zhu, glenn, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch rniwa: review-, rniwa: commit-queue-

Description dewei_zhu 2015-04-30 20:14:52 PDT
Update speedometer patch to provide better UI and fix merge result bug.
Comment 1 dewei_zhu 2015-04-30 20:17:13 PDT
Created attachment 252132 [details]
Patch
Comment 2 Ryosuke Niwa 2015-04-30 20:28:55 PDT
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.
Comment 3 dewei_zhu 2015-05-01 00:36:12 PDT
Created attachment 252136 [details]
Patch
Comment 4 dewei_zhu 2015-05-01 00:43:58 PDT
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.
Comment 5 Ryosuke Niwa 2015-05-01 01:01:49 PDT
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.
Comment 6 dewei_zhu 2015-05-01 01:46:20 PDT
Created attachment 252140 [details]
Patch
Comment 7 dewei_zhu 2015-05-01 02:00:06 PDT
Created attachment 252142 [details]
Patch
Comment 8 WebKit Commit Bot 2015-05-01 12:43:51 PDT
Comment on attachment 252142 [details]
Patch

Clearing flags on attachment: 252142

Committed r183685: <http://trac.webkit.org/changeset/183685>
Comment 9 WebKit Commit Bot 2015-05-01 12:43:55 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Darin Adler 2015-05-01 14:50:29 PDT
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 11 dewei_zhu 2015-05-01 15:08:25 PDT
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.
Comment 12 dewei_zhu 2015-05-01 15:14:32 PDT
Reopening to attach new patch.
Comment 13 dewei_zhu 2015-05-01 15:14:34 PDT
Created attachment 252173 [details]
Patch
Comment 14 Ryosuke Niwa 2015-05-01 15:15:28 PDT
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.