Summary: | Update speedometer patch to provide better UI and fix merge result bug. | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | dewei_zhu | ||||||||||||
Component: | New Bugs | Assignee: | 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
dewei_zhu
2015-04-30 20:14:52 PDT
Created attachment 252132 [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: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. 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
Comment on attachment 252142 [details] Patch Clearing flags on attachment: 252142 Committed r183685: <http://trac.webkit.org/changeset/183685> All reviewed patches have been landed. Closing bug. 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
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.
|