tools/telemetry/support/html_output/results-template.html - Issue 1523743002: [Telemetry] 'Time'/'Memory' buttons added to results.html dynamically for available result types.

Side by Side Diff: tools/telemetry/support/html_output/results-template.html

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

Patch Set: Created 5 years ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch

OLD	NEW
1 <!DOCTYPE html>	1 <!DOCTYPE html>

2 <html>	2 <html>

3 <head>	3 <head>

4 <title>Telemetry Performance Test Results</title>	4 <title>Telemetry Performance Test Results</title>

5 <style type="text/css">	5 <style type="text/css">

6	6

7 section {	7 section {

8 background: white;	8 background: white;

9 padding: 10px;	9 padding: 10px;

10 position: relative;	10 position: relative;

(...skipping 231 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
242 }	242 }

243	243

244 .closeAllButton:hover {	244 .closeAllButton:hover {

245 background: #f04040;	245 background: #f04040;

246 }	246 }

247	247

248 </style>	248 </style>

249 </head>	249 </head>

250 <body onload="init()">	250 <body onload="init()">

251 <div style="padding: 0 10px; white-space: nowrap;">	251 <div style="padding: 0 10px; white-space: nowrap;">

252 Result <span id="time-memory" class="checkbox"><span class="checked">Time</span> <span>Memory</span></span>	252 Result <span id="time-memory" class="checkbox"></span>

253 Reference <span id="reference" class="checkbox"></span>	253 Reference <span id="reference" class="checkbox"></span>

254 Style <span id="scatter-line" class="checkbox"><span class="checked">Scatter</sp an><span>Line</span></span>	254 Style <span id="scatter-line" class="checkbox"><span class="checked">Scatter</sp an><span>Line</span></span>

255 <span class="checkbox"><span class="checked" id="undelete">Undelete</span></span ><br>	255 <span class="checkbox"><span class="checked" id="undelete">Undelete</span></span ><br>

256 Run your test with --reset-results to clear all runs	256 Run your test with --reset-results to clear all runs

257 </div>	257 </div>

258 <table id="container"></table>	258 <table id="container"></table>

259 <script src="https://ajax.googleapis.com/ajax/libs/jquery/1.6.4/jquery.min.js">< /script>	259 <script src="https://ajax.googleapis.com/ajax/libs/jquery/1.6.4/jquery.min.js">< /script>

260 <script>	260 <script>

261 %plugins%	261 %plugins%

262 </script>	262 </script>

(...skipping 420 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
683 $('#nameColumn').click(sortByName);	683 $('#nameColumn').click(sortByName);

684	684

685 $('#unitColumn').click(sortByUnit);	685 $('#unitColumn').click(sortByUnit);

686	686

687 runs.forEach(function(run) {	687 runs.forEach(function(run) {

688 $('#' + run.id()).click(sortByResult);	688 $('#' + run.id()).click(sortByResult);

689 $('#' + run.id() + COMPARISON_SUFFIX).click(sortByReference);	689 $('#' + run.id() + COMPARISON_SUFFIX).click(sortByReference);

690 });	690 });

691 }	691 }

692	692

	693 function TestSelector(tests) {
	petrcermak 2015/12/14 18:18:23 Naming: You seem to be mixing "test" and "test typ Naming: You seem to be mixing "test" and "test type" in many of you method and variable names. For example, the class should be a "TestTypeSelector", because it selects the type of the test (rather than which particular test should be run). To make things clearer, it might make sense to call everything either a "test type" or a "test case". picksi 2015/12/15 17:17:06 Done. Show quoted text On 2015/12/14 18:18:23, petrcermak wrote: > Naming: You seem to be mixing "test" and "test type" in many of you method and > variable names. For example, the class should be a "TestTypeSelector", because > it selects the type of the test (rather than which particular test should be > run). To make things clearer, it might make sense to call everything either a > "test type" or a "test case". Done.
	694 this.recognisers = {
	petrcermak 2015/12/14 18:18:23 There's no need to define this on every single ins There's no need to define this on every single instance. Just define it on the prototype: TestSelector.prototype = { ... recognisers: { 'Time': ... 'Memory': ... } ... } petrcermak 2015/12/14 18:18:23 nit: "recogniZers" for consistency nit: "recogniZers" for consistency picksi 2015/12/15 17:17:06 Done. Show quoted text On 2015/12/14 18:18:23, petrcermak wrote: > There's no need to define this on every single instance. Just define it on the > prototype: > > TestSelector.prototype = { > ... > recognisers: { > 'Time': ... > 'Memory': ... > } > ... > } Done.
	695 "Time": (function(test){return test.isMemoryTest();}),
	petrcermak 2015/12/14 18:18:23 nit: please remove extra parentheses around the fu nit: please remove extra parentheses around the function petrcermak 2015/12/14 18:18:23 nit: spacing: function(test) { return test.isMemo nit: spacing: function(test) { return test.isMemoryTest(); } petrcermak 2015/12/14 18:18:23 nit: s/"/'/ everywhere (for consistency) nit: s/"/'/ everywhere (for consistency) picksi 2015/12/15 17:17:06 Done. Show quoted text On 2015/12/14 18:18:23, petrcermak wrote: > nit: please remove extra parentheses around the function Done. picksi 2015/12/15 17:17:06 Done. Show quoted text On 2015/12/14 18:18:23, petrcermak wrote: > nit: spacing: > > function(test) { return test.isMemoryTest(); } Done. picksi 2015/12/15 17:17:07 Done. Show quoted text On 2015/12/14 18:18:23, petrcermak wrote: > nit: s/"/'/ everywhere (for consistency) Done.
	696 "Memory": (function(test){return !test.isMemoryTest();})

	697 };

	698 this.buttonHTML = this.generateButtonHTML(tests);
	petrcermak 2015/12/14 18:18:23 Instead of storing the button HTML (which then nee Instead of storing the button HTML (which then needs to be modified using String.prototype.replace anyway), I suggest you just store the list of test types and generate the button HTML on demand. picksi 2015/12/15 17:17:05 Done. The TestTypeSelector is now HTML agnostic. I Done. The TestTypeSelector is now HTML agnostic. I've added a standalone helper function that takes a list of button names & an active type and spits out the HTML.
	699 }

	700

	701 TestSelector.prototype = {

	702

	703 setTestType: function (testName) {
	petrcermak 2015/12/14 18:18:23 nit: there should NOT be a space between "function nit: there should NOT be a space between "function" and the opening parenthesis "(" (multiple times throughout the file). petrcermak 2015/12/14 18:18:23 nit: s/testName/testType/ nit: s/testName/testType/ picksi 2015/12/15 17:17:06 I was following the style from the surrounding fun I was following the style from the surrounding functions. I've now fixed this throughout the file; there are lots of occurances. I'm slightly worried that it makes the CL look huge. If you are concerned I could do this as a two stage process. petrcermak 2015/12/17 11:17:14 I'm personally fine with this (since it's "just" a Show quoted text On 2015/12/15 17:17:06, picksi wrote: > I was following the style from the surrounding functions. I've now fixed this > throughout the file; there are lots of occurances. I'm slightly worried that it > makes the CL look huge. If you are concerned I could do this as a two stage > process. I'm personally fine with this (since it's "just" a refactor anyway), but I'll leave this to the owner.
	704 this.recogniserDelegate = this.recognisers[testName];

	705 },

	706

	707 showTest: function (test) {

	708 return this.recogniserDelegate(test);
	petrcermak 2015/12/14 18:18:23 It is surprising that the "show" method returns so It is surprising that the "show" method returns something. You would expect it to be a command (rather than a boolean check). It should probably be renamed to "shouldShowTest" or something like that. picksi 2015/12/15 17:17:06 Good call. Changed it. Good call. Changed it.
	709 },

	710

	711 getButtonHTML: function() {

	712 return this.buttonHTML;
	petrcermak 2015/12/14 18:18:22 No need for a getter here. If you consider buttonH No need for a getter here. If you consider buttonHTML to be private, then you should rename it to "buttonHTML_" and add a proper getter instead: get buttonHTML() { return this.buttonHTML_; } which you can then access as a regular field. picksi 2015/12/15 17:17:06 I've kept the button names 'public', so the rest o I've kept the button names 'public', so the rest of the code can just access them directly.
	713 },

	714

	715 getFirstCheckboxName: function () {
	petrcermak 2015/12/14 18:18:23 This is a TestSelector, so the name of this method This is a TestSelector, so the name of this method should be getFirstTestType or something along those line. Again, this could just be a getter. picksi 2015/12/15 17:17:06 This function has now gone. This function has now gone.
	716 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 nit: I'd prefer for the return statement to be on a separate line. picksi 2015/12/15 17:17:06 Done. Show quoted text On 2015/12/14 18:18:22, petrcermak wrote: > nit: I'd prefer for the return statement to be on a separate line. Done.
	717 },

	718

	719 generateButtonHTML: function (tests) {

	720
	petrcermak 2015/12/14 18:18:23 nit: no need for blank line. If you want to have a nit: no need for blank line. If you want to have a blank line above the function definition, I suggest you first define the local variables. picksi 2015/12/15 17:17:05 Done. Show quoted text On 2015/12/14 18:18:23, petrcermak wrote: > nit: no need for blank line. If you want to have a blank line above the function > definition, I suggest you first define the local variables. Done.
	721 function span(text) {

	722 return "<span>" + text + "</span>";

	723 }

	724

	725 var buttonCount = 0;

	726 var buttonHTML = "";

	727

	728 for (var checkboxName in this.recognisers) {

	729 this.setTestType(checkboxName)
	petrcermak 2015/12/14 18:18:23 This is very strange - you are modifying the test This is very strange - you are modifying the test type of the selector whilst you're creating it just so that you could use it inside showTest. Please change this to: for (var checkboxName in this.recognizers) { var recognizer = this.recognizers[checkboxName]; for (var testName in tests) { var test = tests[testName]; if (recognizer(test)) { buttonHTML += span(checkboxName); buttonCount++; break; } } } picksi 2015/12/15 17:17:06 I go around the houses here on purpose, for two re I go around the houses here on purpose, for two reasons: 1. On principle I always use an accessor if I have one. 2. It keeps this function (semi) agnostic as to how the recognizer stuff is implemented; I don't want too much implementation bleeding needlessly. The fact that the second half of the function directly adds new recognizers to the list keeps me awake at night :) petrcermak 2015/12/17 11:17:14 I can see your points. Nevertheless, I maintain th Show quoted text On 2015/12/15 17:17:06, picksi wrote: > I go around the houses here on purpose, for two reasons: > 1. On principle I always use an accessor if I have one. > 2. It keeps this function (semi) agnostic as to how the recognizer stuff is > implemented; I don't want too much implementation bleeding needlessly. The fact > that the second half of the function directly adds new recognizers to the list > keeps me awake at night :) I can see your points. Nevertheless, I maintain that this is, in general, bad code design (unless there's a need for it such as performance, neither of which applies here). It makes the code difficult to follow (line 728 just doesn't seem to make sense) and after the method finishes, the selector is left with some random test type. The generator method should not be repeatedly modifying the test type of the selector. You should do the following instead: for (var testTypeName in this.recognizers) { var recognizer = this.recognizers[testTypeName]; for (var testName in tests) { if (recognizer(tests[testName])) { testTypeNames.push(testName); break; } } }
	730 for (var testName in tests) {

	731 var test = tests[testName];

	732 if (this.showTest(test)) {

	733 // This is a test that is recognised so add a checkbox for i t.

	734 buttonHTML += span(checkboxName);

	735 buttonCount++;

	736 break;

	737 }

	738 }

	739 }

	740 if (buttonCount == 0) {
	petrcermak 2015/12/14 18:18:23 nit: Please add a blank line before this one to ma nit: Please add a blank line before this one to make the code easier to read. picksi 2015/12/15 17:17:06 Done. Show quoted text On 2015/12/14 18:18:23, petrcermak wrote: > nit: Please add a blank line before this one to make the code easier to read. Done.
	741 // Add 'no results' button if there are no results

	742 var newButtonName = "No Results"

	743 this.recognisers[newButtonName] = (function(){return false;});
	petrcermak 2015/12/14 18:18:23 nit: no need for parentheses around function. Also nit: no need for parentheses around function. Also please add spaces to the anonymous function appropriately. (both comments also apply to line 749. picksi 2015/12/15 17:17:06 Done. Show quoted text On 2015/12/14 18:18:23, petrcermak wrote: > nit: no need for parentheses around function. Also please add spaces to the > anonymous function appropriately. (both comments also apply to line 749. Done.
	744 buttonHTML += span(newButtonName);

	745 }
	petrcermak 2015/12/14 18:18:23 There shouldn't be a line break here (i.e. it shou There shouldn't be a line break here (i.e. it should be "} else if (...) {" picksi 2015/12/15 17:17:05 Done. Show quoted text On 2015/12/14 18:18:23, petrcermak wrote: > There shouldn't be a line break here (i.e. it should be "} else if (...) {" Done.
	746 else if (buttonCount > 1) {

	747 // If we have more than one test add an 'all' button.

	748 var newButtonName = "All"

	749 this.recognisers[newButtonName] = (function(){return true;});

	750 buttonHTML += span(newButtonName);

	751 }

	752

	753 return buttonHTML;

	754 }

	755 };

	756

693 var topLevelRows;	757 var topLevelRows;

694 var allTableRows;	758 var allTableRows;

695	759

696 function createTable(tests, runs, shouldIgnoreMemory, referenceIndex, useLargeLi nePlots) {	760 function displayTable(tests, runs, testSelector, referenceIndex, useLargeLinePlo ts) {

697 var resultHeaders = runs.map(function (run, index) {	761 var resultHeaders = runs.map(function (run, index) {

698 var header = '<th id="' + run.id() + '" ' +	762 var header = '<th id="' + run.id() + '" ' +

699 'colspan=2 ' +	763 'colspan=2 ' +

700 'title="' + run.description() + '">' +	764 'title="' + run.description() + '">' +

701 '<span class="label" ' +	765 '<span class="label" ' +

702 'title="Edit run label">' +	766 'title="Edit run label">' +

703 run.label() +	767 run.label() +

704 '</span>' +	768 '</span>' +

705 '<div class="closeButton" ' +	769 '<div class="closeButton" ' +

706 'title="Delete run">' +	770 'title="Delete run">' +

(...skipping 35 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
742	806

743 $('#container').html(htmlString);	807 $('#container').html(htmlString);

744	808

745 var testNames = [];	809 var testNames = [];

746 for (testName in tests)	810 for (testName in tests)

747 testNames.push(testName);	811 testNames.push(testName);

748	812

749 allTableRows = [];	813 allTableRows = [];

750 testNames.forEach(function(testName) {	814 testNames.forEach(function(testName) {

751 var test = tests[testName];	815 var test = tests[testName];

752 if (test.isMemoryTest() === shouldIgnoreMemory) {	816 if (testSelector.showTest(test)) {
	petrcermak 2015/12/14 18:18:23 Out of interest, is there a reason why you inverte Out of interest, is there a reason why you inverted the logic? The most straightforward change would have simply been to change the condition of the if-statement to !testSelector.showTest(test). picksi 2015/12/15 17:17:06 I dislike negative if statements! I think the inte I dislike negative if statements! I think the intention of the code is clearer framed this way round - i.e. if this is a test we should show then add it. petrcermak 2015/12/17 11:17:14 SGTM Show quoted text On 2015/12/15 17:17:06, picksi wrote: > I dislike negative if statements! I think the intention of the code is clearer > framed this way round - i.e. if this is a test we should show then add it. SGTM
753 return;	817 allTableRows.push(new TableRow(runs, test, referenceIndex, useLargeL inePlots));

754 }	818 }

755 allTableRows.push(new TableRow(runs, test, referenceIndex, useLargeLineP lots));

756 });	819 });

757	820

758 // Build a list of top level rows with attached children	821 // Build a list of top level rows with attached children

759 topLevelRows = [];	822 topLevelRows = [];

760 allTableRows.forEach(function(row) {	823 allTableRows.forEach(function(row) {

761 // Add us to top level if we are a top-level row...	824 // Add us to top level if we are a top-level row...

762 if (row.hasNoURL) {	825 if (row.hasNoURL) {

763 topLevelRows.push(row);	826 topLevelRows.push(row);

764 // Add a duplicate child row that holds the graph for the parent	827 // Add a duplicate child row that holds the graph for the parent

765 var graphHolder = new TableRow(runs, row.test, referenceIndex, useLa rgeLinePlots);	828 var graphHolder = new TableRow(runs, row.test, referenceIndex, useLa rgeLinePlots);

(...skipping 602 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
1368 new TestResult(metric, rawMetrics[metricName].current,	1431 new TestResult(metric, rawMetrics[metricName].current,

1369 run, rawMetrics[metricName]['std'], rawMetrics[metri cName]['degrees_of_freedom']));	1432 run, rawMetrics[metricName]['std'], rawMetrics[metri cName]['degrees_of_freedom']));

1370 }	1433 }

1371 }	1434 }

1372 }	1435 }

1373	1436

1374 addTests(entry.tests);	1437 addTests(entry.tests);

1375 });	1438 });

1376	1439

1377 var useLargeLinePlots = false;	1440 var useLargeLinePlots = false;

1378 var shouldIgnoreMemory= true;

1379 var referenceIndex = 0;	1441 var referenceIndex = 0;

1380	1442

1381 createTable(metrics, runs, shouldIgnoreMemory, referenceIndex, useLargeLineP lots);

1382

1383 $('#time-memory').bind('change', function (event, checkedElement) {

1384 shouldIgnoreMemory = checkedElement.textContent == 'Time';

1385 createTable(metrics, runs, shouldIgnoreMemory, referenceIndex, useLargeL inePlots);

1386 });

1387

1388 $('#scatter-line').bind('change', function (event, checkedElement) {	1443 $('#scatter-line').bind('change', function (event, checkedElement) {

1389 useLargeLinePlots = checkedElement.textContent == 'Line';	1444 useLargeLinePlots = checkedElement.textContent == 'Line';

1390 createTable(metrics, runs, shouldIgnoreMemory, referenceIndex, useLargeL inePlots);	1445 displayTable(metrics, runs, testSelector, referenceIndex, useLargeLinePl ots);

1391 });	1446 });

1392	1447

1393 runs.map(function (run, index) {	1448 runs.map(function (run, index) {

1394 $('#reference').append('<span value="' + index + '"' + (index == referen ceIndex ? ' class="checked"' : '') + ' title="' + run.description() + '">' + run .label() + '</span>');	1449 $('#reference').append('<span value="' + index + '"' + (index == referen ceIndex ? ' class="checked"' : '') + ' title="' + run.description() + '">' + run .label() + '</span>');

1395 })	1450 })

1396	1451

	1452 $('#time-memory').bind('change', function (event, checkedElement) {

	1453 testSelector.setTestType(checkedElement.textContent);

	1454 displayTable(metrics, runs, testSelector, referenceIndex, useLargeLinePl ots);;

	1455 });

	1456

1397 $('#reference').bind('change', function (event, checkedElement) {	1457 $('#reference').bind('change', function (event, checkedElement) {

1398 referenceIndex = parseInt(checkedElement.getAttribute('value'));	1458 referenceIndex = parseInt(checkedElement.getAttribute('value'));

1399 createTable(metrics, runs, shouldIgnoreMemory, referenceIndex, useLargeL inePlots);	1459 displayTable(metrics, runs, testSelector, referenceIndex, useLargeLinePl ots);

1400 });	1460 });

1401	1461

	1462 var testSelector = new TestSelector(metrics);

	1463 var initialTestName = testSelector.getFirstCheckboxName();

	1464 var buttonHTML = testSelector.getButtonHTML();

	1465

	1466 // Make the initial test-type button pre-selected.

	1467 buttonHTML = buttonHTML.replace("<span>" + initialTestName, "<span class=che cked>" + initialTestName);
	petrcermak 2015/12/14 18:18:23 I don't think this is a good idea (what if the tes I don't think this is a good idea (what if the test selector doesn't return just a span one day?). Instead, I think you should do the following: var testSelector = new TestSelector(metrics); // I think that TestSelector should already know its initial test type. var buttonHTML = testSelector.getButtonHTML(); // This method should take the current test type into account. picksi 2015/12/15 17:17:05 I've kept the decision as to which test-type is de I've kept the decision as to which test-type is defaulted to out of the TestTypeSelector (as it has been renamed), as I think it should be owned by the caller function. This has been refactored a bit now, and I've also kicked the HTML out of the TestTypeSelector, as it never really belonged there. I've added a helper function that takes a list of button names and an active button and generates the correctly set-up HTML.
	1468 $('#time-memory').append(buttonHTML);

	1469

	1470 testSelector.setTestType(initialTestName);

	1471 displayTable(metrics, runs, testSelector, referenceIndex, useLargeLinePlots) ;;
	petrcermak 2015/12/14 18:18:23 s/;;/;/ s/;;/;/ picksi 2015/12/15 17:17:05 Done. Show quoted text On 2015/12/14 18:18:23, petrcermak wrote: > s/;;/;/ Done.
	1472

1402 $('.checkbox').each(function (index, checkbox) {	1473 $('.checkbox').each(function (index, checkbox) {

1403 $(checkbox).children('span').click(function (event) {	1474 $(checkbox).children('span').click(function (event) {

1404 if ($(this).hasClass('checked'))	1475 if ($(this).hasClass('checked'))

1405 return;	1476 return;

1406 $(checkbox).children('span').removeClass('checked');	1477 $(checkbox).children('span').removeClass('checked');

1407 $(this).addClass('checked');	1478 $(this).addClass('checked');

1408 $(checkbox).trigger('change', $(this));	1479 $(checkbox).trigger('change', $(this));

1409 });	1480 });

1410 });	1481 });

1411	1482

(...skipping 10 matching lines...) Expand all Loading...
1422 } else {	1493 } else {

1423 $('#undelete').hide();	1494 $('#undelete').hide();

1424 }	1495 }

1425 }	1496 }

1426	1497

1427 </script>	1498 </script>

1428 <script id="results-json" type="application/json">%json_results%</script>	1499 <script id="results-json" type="application/json">%json_results%</script>

1429 <script id="units-json" type="application/json">%json_units%</script>	1500 <script id="units-json" type="application/json">%json_units%</script>

1430 </body>	1501 </body>

1431 </html>	1502 </html>

OLD	NEW

« no previous file with comments | « no previous file | no next file » | no next file with comments »