Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
654203006:
Turning TableRow into a class (Closed)
DescriptionTurning TableRow into a class as part of a refactor to make the results
viewer hierarchy aware.
BUG=425017
Committed: https://crrev.com/d8d2131e8ae8362901a07521af21cc3a9c70cde9
Cr-Commit-Position: refs/heads/master@{#301111}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Layout fixes + removal of construct function #
Total comments: 2
Messages
Total messages: 12 (2 generated)
picksi@chromium.org changed reviewers: + alexclarke@chromium.org, ernstm@chromium.org, petrcermak@chromium.org, skyostil@chromium.org
A small refactor to convert a closure into a class; this forms the first part of a larger refactor to make the benchmark viewer more hierarchical. The code diffs are actually relatively small - new'ing up a TableRow and calling a Construct() function on it, plus addition on 'this.' in many places. The code-diffs make it look like I've added and removed lots of code, but I haven't!
https://codereview.chromium.org/654203006/diff/1/tools/telemetry/support/html... File tools/telemetry/support/html_output/results-template.html (right): https://codereview.chromium.org/654203006/diff/1/tools/telemetry/support/html... tools/telemetry/support/html_output/results-template.html:694: TableRow.prototype.Construct = function() { Is there any reason for having a separate Construct function? After all, isn't that what the constructor is meant for? https://codereview.chromium.org/654203006/diff/1/tools/telemetry/support/html... tools/telemetry/support/html_output/results-template.html:727: Since you're already refactoring this, I would suggest getting rid of the extra blank lines. https://codereview.chromium.org/654203006/diff/1/tools/telemetry/support/html... tools/telemetry/support/html_output/results-template.html:758: else if (values && values.length > 3) { nit - "} else if {" https://codereview.chromium.org/654203006/diff/1/tools/telemetry/support/html... tools/telemetry/support/html_output/results-template.html:762: if (regressionResult.rSquared > 0.6 && Math.abs(regressionResult.slope) > 0.01) { I find most of the code below quite difficult to understand (I know you didn't change it) because there is no limit on line length and many lines consequently wrap. I would use an 80-column limit (see http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml).
Thanks Simon. This has no functional changes, right? https://codereview.chromium.org/654203006/diff/20001/tools/telemetry/support/... File tools/telemetry/support/html_output/results-template.html (right): https://codereview.chromium.org/654203006/diff/20001/tools/telemetry/support/... tools/telemetry/support/html_output/results-template.html:602: new TableRow(runs, test, referenceIndex, useLargeLinePlots); I assume the upcoming CL will keep a reference to the object created here and subsequently use it? Otherwise it making this into a class at this level seems a little odd :) If the class is still only used internally, I'd keep createTableRow() here and have it use the new TableRow class.
There are no functional changes. The returned class instance is going to be used in the next step of the refactor; There will be a top-level array of parent TableRows (i.e. the rows that are currently bold), and these parents will each hold an array of their children... and so on if we end up with deeper nesting.
https://codereview.chromium.org/654203006/diff/1/tools/telemetry/support/html... File tools/telemetry/support/html_output/results-template.html (right): https://codereview.chromium.org/654203006/diff/1/tools/telemetry/support/html... tools/telemetry/support/html_output/results-template.html:694: TableRow.prototype.Construct = function() { On 2014/10/20 15:06:29, petrcermak wrote: > Is there any reason for having a separate Construct function? After all, isn't > that what the constructor is meant for? Done. https://codereview.chromium.org/654203006/diff/1/tools/telemetry/support/html... tools/telemetry/support/html_output/results-template.html:727: I've removed some whitespace around here, but I want to keep the diffs minimal at this stage. I'll tidy up this with subsequent CLs. https://codereview.chromium.org/654203006/diff/1/tools/telemetry/support/html... tools/telemetry/support/html_output/results-template.html:758: else if (values && values.length > 3) { On 2014/10/20 15:06:30, petrcermak wrote: > nit - "} else if {" Done. https://codereview.chromium.org/654203006/diff/1/tools/telemetry/support/html... tools/telemetry/support/html_output/results-template.html:762: if (regressionResult.rSquared > 0.6 && Math.abs(regressionResult.slope) > 0.01) { There are a lot of long lines in this file. I'll not fix these now, so as to minimise diffs for this checkin, but I will sort these out in subsequent CLs.
On 2014/10/20 17:11:35, picksi1 wrote: > https://codereview.chromium.org/654203006/diff/1/tools/telemetry/support/html... > File tools/telemetry/support/html_output/results-template.html (right): > > https://codereview.chromium.org/654203006/diff/1/tools/telemetry/support/html... > tools/telemetry/support/html_output/results-template.html:694: > TableRow.prototype.Construct = function() { > On 2014/10/20 15:06:29, petrcermak wrote: > > Is there any reason for having a separate Construct function? After all, isn't > > that what the constructor is meant for? > > Done. > > https://codereview.chromium.org/654203006/diff/1/tools/telemetry/support/html... > tools/telemetry/support/html_output/results-template.html:727: > I've removed some whitespace around here, but I want to keep the diffs minimal > at this stage. I'll tidy up this with subsequent CLs. > > https://codereview.chromium.org/654203006/diff/1/tools/telemetry/support/html... > tools/telemetry/support/html_output/results-template.html:758: else if (values > && values.length > 3) { > On 2014/10/20 15:06:30, petrcermak wrote: > > nit - "} else if {" > > Done. > > https://codereview.chromium.org/654203006/diff/1/tools/telemetry/support/html... > tools/telemetry/support/html_output/results-template.html:762: if > (regressionResult.rSquared > 0.6 && Math.abs(regressionResult.slope) > 0.01) { > There are a lot of long lines in this file. I'll not fix these now, so as to > minimise diffs for this checkin, but I will sort these out in subsequent CLs. LGTM
The CQ bit was checked by picksi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/654203006/20001
https://codereview.chromium.org/654203006/diff/20001/tools/telemetry/support/... File tools/telemetry/support/html_output/results-template.html (right): https://codereview.chromium.org/654203006/diff/20001/tools/telemetry/support/... tools/telemetry/support/html_output/results-template.html:602: new TableRow(runs, test, referenceIndex, useLargeLinePlots); The object created will be used a lot in the following refactor!
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/d8d2131e8ae8362901a07521af21cc3a9c70cde9 Cr-Commit-Position: refs/heads/master@{#301111} |
