Chromium Code Reviews Chromium Code Reviews
Issue 1523743002:
[Telemetry] 'Time'/'Memory' buttons added to results.html dynamically for available result types. (Closed)
Base URL: https://chromium.googlesource.com/chromium/src.git@master| Index: tools/telemetry/support/html_output/results-template.html |
| diff --git a/tools/telemetry/support/html_output/results-template.html b/tools/telemetry/support/html_output/results-template.html |
| index 3801aded5dca0a034389a590b4fabcd265dd5eb1..f6675849fd8062001e383bea0f1214756d41a137 100644 |
| --- a/tools/telemetry/support/html_output/results-template.html |
| +++ b/tools/telemetry/support/html_output/results-template.html |
| @@ -249,7 +249,7 @@ td.missingReference { |
| </head> |
| <body onload="init()"> |
| <div style="padding: 0 10px; white-space: nowrap;"> |
| -Result <span id="time-memory" class="checkbox"><span class="checked">Time</span><span>Memory</span></span> |
| +Result <span id="time-memory" class="checkbox"></span> |
| Reference <span id="reference" class="checkbox"></span> |
| Style <span id="scatter-line" class="checkbox"><span class="checked">Scatter</span><span>Line</span></span> |
| <span class="checkbox"><span class="checked" id="undelete">Undelete</span></span><br> |
| @@ -690,10 +690,74 @@ function setUpSortClicks(runs) |
| }); |
| } |
| +function TestSelector(tests) { |
|
petrcermak
2015/12/14 18:18:23
Naming: You seem to be mixing "test" and "test typ
picksi
2015/12/15 17:17:06
Done.
|
| + this.recognisers = { |
|
petrcermak
2015/12/14 18:18:23
There's no need to define this on every single ins
petrcermak
2015/12/14 18:18:23
nit: "recogniZers" for consistency
picksi
2015/12/15 17:17:06
Done.
|
| + "Time": (function(test){return test.isMemoryTest();}), |
|
petrcermak
2015/12/14 18:18:23
nit: please remove extra parentheses around the fu
petrcermak
2015/12/14 18:18:23
nit: spacing:
function(test) { return test.isMemo
petrcermak
2015/12/14 18:18:23
nit: s/"/'/ everywhere (for consistency)
picksi
2015/12/15 17:17:06
Done.
picksi
2015/12/15 17:17:06
Done.
picksi
2015/12/15 17:17:07
Done.
|
| + "Memory": (function(test){return !test.isMemoryTest();}) |
| + }; |
| + this.buttonHTML = this.generateButtonHTML(tests); |
|
petrcermak
2015/12/14 18:18:23
Instead of storing the button HTML (which then nee
picksi
2015/12/15 17:17:05
Done. The TestTypeSelector is now HTML agnostic. I
|
| +} |
| + |
| +TestSelector.prototype = { |
| + |
| + setTestType: function (testName) { |
|
petrcermak
2015/12/14 18:18:23
nit: there should NOT be a space between "function
petrcermak
2015/12/14 18:18:23
nit: s/testName/testType/
picksi
2015/12/15 17:17:06
I was following the style from the surrounding fun
petrcermak
2015/12/17 11:17:14
I'm personally fine with this (since it's "just" a
|
| + this.recogniserDelegate = this.recognisers[testName]; |
| + }, |
| + |
| + showTest: function (test) { |
| + return this.recogniserDelegate(test); |
|
petrcermak
2015/12/14 18:18:23
It is surprising that the "show" method returns so
picksi
2015/12/15 17:17:06
Good call. Changed it.
|
| + }, |
| + |
| + getButtonHTML: function() { |
| + return this.buttonHTML; |
|
petrcermak
2015/12/14 18:18:22
No need for a getter here. If you consider buttonH
picksi
2015/12/15 17:17:06
I've kept the button names 'public', so the rest o
|
| + }, |
| + |
| + getFirstCheckboxName: function () { |
|
petrcermak
2015/12/14 18:18:23
This is a TestSelector, so the name of this method
picksi
2015/12/15 17:17:06
This function has now gone.
|
| + for (var key in this.recognisers) return key; |
|
petrcermak
2015/12/14 18:18:22
nit: I'd prefer for the return statement to be on
picksi
2015/12/15 17:17:06
Done.
|
| + }, |
| + |
| + generateButtonHTML: function (tests) { |
| + |
|
petrcermak
2015/12/14 18:18:23
nit: no need for blank line. If you want to have a
picksi
2015/12/15 17:17:05
Done.
|
| + function span(text) { |
| + return "<span>" + text + "</span>"; |
| + } |
| + |
| + var buttonCount = 0; |
| + var buttonHTML = ""; |
| + |
| + for (var checkboxName in this.recognisers) { |
| + this.setTestType(checkboxName) |
|
petrcermak
2015/12/14 18:18:23
This is very strange - you are modifying the test
picksi
2015/12/15 17:17:06
I go around the houses here on purpose, for two re
petrcermak
2015/12/17 11:17:14
I can see your points. Nevertheless, I maintain th
|
| + for (var testName in tests) { |
| + var test = tests[testName]; |
| + if (this.showTest(test)) { |
| + // This is a test that is recognised so add a checkbox for it. |
| + buttonHTML += span(checkboxName); |
| + buttonCount++; |
| + break; |
| + } |
| + } |
| + } |
| + if (buttonCount == 0) { |
|
petrcermak
2015/12/14 18:18:23
nit: Please add a blank line before this one to ma
picksi
2015/12/15 17:17:06
Done.
|
| + // Add 'no results' button if there are no results |
| + var newButtonName = "No Results" |
| + this.recognisers[newButtonName] = (function(){return false;}); |
|
petrcermak
2015/12/14 18:18:23
nit: no need for parentheses around function. Also
picksi
2015/12/15 17:17:06
Done.
|
| + buttonHTML += span(newButtonName); |
| + } |
|
petrcermak
2015/12/14 18:18:23
There shouldn't be a line break here (i.e. it shou
picksi
2015/12/15 17:17:05
Done.
|
| + else if (buttonCount > 1) { |
| + // If we have more than one test add an 'all' button. |
| + var newButtonName = "All" |
| + this.recognisers[newButtonName] = (function(){return true;}); |
| + buttonHTML += span(newButtonName); |
| + } |
| + |
| + return buttonHTML; |
| + } |
| +}; |
| + |
| var topLevelRows; |
| var allTableRows; |
| -function createTable(tests, runs, shouldIgnoreMemory, referenceIndex, useLargeLinePlots) { |
| +function displayTable(tests, runs, testSelector, referenceIndex, useLargeLinePlots) { |
| var resultHeaders = runs.map(function (run, index) { |
| var header = '<th id="' + run.id() + '" ' + |
| 'colspan=2 ' + |
| @@ -749,10 +813,9 @@ function createTable(tests, runs, shouldIgnoreMemory, referenceIndex, useLargeLi |
| allTableRows = []; |
| testNames.forEach(function(testName) { |
| var test = tests[testName]; |
| - if (test.isMemoryTest() === shouldIgnoreMemory) { |
| - return; |
| + if (testSelector.showTest(test)) { |
|
petrcermak
2015/12/14 18:18:23
Out of interest, is there a reason why you inverte
picksi
2015/12/15 17:17:06
I dislike negative if statements! I think the inte
petrcermak
2015/12/17 11:17:14
SGTM
|
| + allTableRows.push(new TableRow(runs, test, referenceIndex, useLargeLinePlots)); |
| } |
| - allTableRows.push(new TableRow(runs, test, referenceIndex, useLargeLinePlots)); |
| }); |
| // Build a list of top level rows with attached children |
| @@ -1375,30 +1438,38 @@ function init() { |
| }); |
| var useLargeLinePlots = false; |
| - var shouldIgnoreMemory= true; |
| var referenceIndex = 0; |
| - createTable(metrics, runs, shouldIgnoreMemory, referenceIndex, useLargeLinePlots); |
| - |
| - $('#time-memory').bind('change', function (event, checkedElement) { |
| - shouldIgnoreMemory = checkedElement.textContent == 'Time'; |
| - createTable(metrics, runs, shouldIgnoreMemory, referenceIndex, useLargeLinePlots); |
| - }); |
| - |
| $('#scatter-line').bind('change', function (event, checkedElement) { |
| useLargeLinePlots = checkedElement.textContent == 'Line'; |
| - createTable(metrics, runs, shouldIgnoreMemory, referenceIndex, useLargeLinePlots); |
| + displayTable(metrics, runs, testSelector, referenceIndex, useLargeLinePlots); |
| }); |
| runs.map(function (run, index) { |
| $('#reference').append('<span value="' + index + '"' + (index == referenceIndex ? ' class="checked"' : '') + ' title="' + run.description() + '">' + run.label() + '</span>'); |
| }) |
| + $('#time-memory').bind('change', function (event, checkedElement) { |
| + testSelector.setTestType(checkedElement.textContent); |
| + displayTable(metrics, runs, testSelector, referenceIndex, useLargeLinePlots);; |
| + }); |
| + |
| $('#reference').bind('change', function (event, checkedElement) { |
| referenceIndex = parseInt(checkedElement.getAttribute('value')); |
| - createTable(metrics, runs, shouldIgnoreMemory, referenceIndex, useLargeLinePlots); |
| + displayTable(metrics, runs, testSelector, referenceIndex, useLargeLinePlots); |
| }); |
| + var testSelector = new TestSelector(metrics); |
| + var initialTestName = testSelector.getFirstCheckboxName(); |
| + var buttonHTML = testSelector.getButtonHTML(); |
| + |
| + // Make the initial test-type button pre-selected. |
| + buttonHTML = buttonHTML.replace("<span>" + initialTestName, "<span class=checked>" + initialTestName); |
|
petrcermak
2015/12/14 18:18:23
I don't think this is a good idea (what if the tes
picksi
2015/12/15 17:17:05
I've kept the decision as to which test-type is de
|
| + $('#time-memory').append(buttonHTML); |
| + |
| + testSelector.setTestType(initialTestName); |
| + displayTable(metrics, runs, testSelector, referenceIndex, useLargeLinePlots);; |
|
petrcermak
2015/12/14 18:18:23
s/;;/;/
picksi
2015/12/15 17:17:05
Done.
|
| + |
| $('.checkbox').each(function (index, checkbox) { |
| $(checkbox).children('span').click(function (event) { |
| if ($(this).hasClass('checked')) |
