Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1523743002:
[Telemetry] 'Time'/'Memory' buttons added to results.html dynamically for available result types. (Closed)
Description'Time'/'Memory' buttons added to results.html dynamically for available result types.
If no results are available an unpressable 'no results' button is added.
If both types of results are available an 'all' option is added.
This refactor makes the results screen more 'stateful' and the code a
little easier to understand than the previous implementation that relied
on an 'ignore' bool to skip results.
BUG=569522
Committed: https://crrev.com/1df7f885db651781239a2d5bcfcec843a12ef2f4
Cr-Commit-Position: refs/heads/master@{#367576}
Patch Set 1 #
Total comments: 43
Patch Set 2 : Code review updates #
Total comments: 34
Patch Set 3 : More code review updates #
Total comments: 12
Patch Set 4 : Code review nits #Patch Set 5 : Rebase #Messages
Total messages: 25 (12 generated)
picksi@google.com changed reviewers: + perezju@chromium.org, petrcermak@chromium.org
I think this makes the code more readable ... but certainly a little longer. WDYT? I've added a class that contains 'recognisers', you shove the results at it and it returns some HTML for the buttons that are needed for all the different result types. Makes adding new test types easy (but not sure if we'll ever have different test types though?).
Thanks for taking the time to refactor the code :-) Patch description nits: - The first paragraph of the description must be a single line (because it's copied into the patch title). Please update both accordingly. - Please make the first paragraph (after the title) easier to understand, e.g. "This patch refactors the telemetry results.html code by generating the switch for displaying ... dynamically." - (nit) There should be commas after "if no results are available" and "if both types of results are available" Thanks, Petr https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... File tools/telemetry/support/html_output/results-template.html (right): https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... tools/telemetry/support/html_output/results-template.html:693: function TestSelector(tests) { 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". https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... tools/telemetry/support/html_output/results-template.html:694: this.recognisers = { There's no need to define this on every single instance. Just define it on the prototype: TestSelector.prototype = { ... recognisers: { 'Time': ... 'Memory': ... } ... } https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... tools/telemetry/support/html_output/results-template.html:694: this.recognisers = { nit: "recogniZers" for consistency https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... tools/telemetry/support/html_output/results-template.html:695: "Time": (function(test){return test.isMemoryTest();}), nit: spacing: function(test) { return test.isMemoryTest(); } https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... tools/telemetry/support/html_output/results-template.html:695: "Time": (function(test){return test.isMemoryTest();}), nit: please remove extra parentheses around the function https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... tools/telemetry/support/html_output/results-template.html:695: "Time": (function(test){return test.isMemoryTest();}), nit: s/"/'/ everywhere (for consistency) https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... tools/telemetry/support/html_output/results-template.html:698: this.buttonHTML = this.generateButtonHTML(tests); 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. https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... tools/telemetry/support/html_output/results-template.html:703: setTestType: function (testName) { nit: there should NOT be a space between "function" and the opening parenthesis "(" (multiple times throughout the file). https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... tools/telemetry/support/html_output/results-template.html:703: setTestType: function (testName) { nit: s/testName/testType/ https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... tools/telemetry/support/html_output/results-template.html:708: return this.recogniserDelegate(test); 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. https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... tools/telemetry/support/html_output/results-template.html:712: return this.buttonHTML; 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. https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... tools/telemetry/support/html_output/results-template.html:715: getFirstCheckboxName: function () { 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. https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... tools/telemetry/support/html_output/results-template.html:716: for (var key in this.recognisers) return key; nit: I'd prefer for the return statement to be on a separate line. https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... tools/telemetry/support/html_output/results-template.html:720: 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. https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... tools/telemetry/support/html_output/results-template.html:729: this.setTestType(checkboxName) 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; } } } https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... tools/telemetry/support/html_output/results-template.html:740: if (buttonCount == 0) { nit: Please add a blank line before this one to make the code easier to read. https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... tools/telemetry/support/html_output/results-template.html:743: this.recognisers[newButtonName] = (function(){return false;}); nit: no need for parentheses around function. Also please add spaces to the anonymous function appropriately. (both comments also apply to line 749. https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... tools/telemetry/support/html_output/results-template.html:745: } There shouldn't be a line break here (i.e. it should be "} else if (...) {" https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... tools/telemetry/support/html_output/results-template.html:816: if (testSelector.showTest(test)) { 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). https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... tools/telemetry/support/html_output/results-template.html:1467: buttonHTML = buttonHTML.replace("<span>" + initialTestName, "<span class=checked>" + initialTestName); 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. https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... tools/telemetry/support/html_output/results-template.html:1471: displayTable(metrics, runs, testSelector, referenceIndex, useLargeLinePlots);; s/;;/;/
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Updates done. PTAL. CL has grown a little by removal of the space between 'function' and '()' as it was widespread throughout the code. https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... File tools/telemetry/support/html_output/results-template.html (right): https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... tools/telemetry/support/html_output/results-template.html:693: function TestSelector(tests) { 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. https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... tools/telemetry/support/html_output/results-template.html:694: this.recognisers = { 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. https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... tools/telemetry/support/html_output/results-template.html:695: "Time": (function(test){return test.isMemoryTest();}), On 2015/12/14 18:18:23, petrcermak wrote: > nit: please remove extra parentheses around the function Done. https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... tools/telemetry/support/html_output/results-template.html:695: "Time": (function(test){return test.isMemoryTest();}), On 2015/12/14 18:18:23, petrcermak wrote: > nit: spacing: > > function(test) { return test.isMemoryTest(); } Done. https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... tools/telemetry/support/html_output/results-template.html:695: "Time": (function(test){return test.isMemoryTest();}), On 2015/12/14 18:18:23, petrcermak wrote: > nit: s/"/'/ everywhere (for consistency) Done. https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... tools/telemetry/support/html_output/results-template.html:698: this.buttonHTML = this.generateButtonHTML(tests); 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. https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... tools/telemetry/support/html_output/results-template.html:703: setTestType: function (testName) { 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. https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... tools/telemetry/support/html_output/results-template.html:708: return this.recogniserDelegate(test); Good call. Changed it. https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... tools/telemetry/support/html_output/results-template.html:712: return this.buttonHTML; I've kept the button names 'public', so the rest of the code can just access them directly. https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... tools/telemetry/support/html_output/results-template.html:715: getFirstCheckboxName: function () { This function has now gone. https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... tools/telemetry/support/html_output/results-template.html:716: for (var key in this.recognisers) return key; On 2015/12/14 18:18:22, petrcermak wrote: > nit: I'd prefer for the return statement to be on a separate line. Done. https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... tools/telemetry/support/html_output/results-template.html:720: 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. https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... tools/telemetry/support/html_output/results-template.html:729: this.setTestType(checkboxName) 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 :) https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... tools/telemetry/support/html_output/results-template.html:740: if (buttonCount == 0) { 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. https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... tools/telemetry/support/html_output/results-template.html:743: this.recognisers[newButtonName] = (function(){return false;}); 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. https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... tools/telemetry/support/html_output/results-template.html:745: } 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. https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... tools/telemetry/support/html_output/results-template.html:816: if (testSelector.showTest(test)) { 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. https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... tools/telemetry/support/html_output/results-template.html:1467: buttonHTML = buttonHTML.replace("<span>" + initialTestName, "<span class=checked>" + initialTestName); 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. https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm... tools/telemetry/support/html_output/results-template.html:1471: displayTable(metrics, runs, testSelector, referenceIndex, useLargeLinePlots);; On 2015/12/14 18:18:23, petrcermak wrote: > s/;;/;/ Done.
Description was changed from ========== Buttons to show 'Time' or 'Memory' options added dynamically to results.html based on the types of tests in the results. If no results are available an unpressable 'no results' button is added. If both types of results are available an 'all' option is added. This refactor makes the results screen more 'stateful' and the code a little easier to understand than the previous implementation that relied on an 'ignore' bool to skip results. BUG=569522 ========== to ========== 'Time'/'Memory' buttons added to results.html dynamically for available result types. If no results are available an unpressable 'no results' button is added. If both types of results are available an 'all' option is added. This refactor makes the results screen more 'stateful' and the code a little easier to understand than the previous implementation that relied on an 'ignore' bool to skip results. BUG=569522 ==========
Thanks for taking the feedback into account :-) I added a few more inline
comments. The main messages are:
1. I'm personally perfectly fine with modifying the function spacing in the
whole file as part of the patch (since it's "just" a refactor anyway), but this
is really for an owner to decide.
2. You are still mixing model ('test type name') and UI ('button name') concepts
in the selector.
3. I maintain that repeatedly modifying the object in the generator loop is bad
code design, which doesn't give any real benefits and makes the code confusing
and fragile (wrt the resulting state of the selector).
Thanks,
Petr
https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm...
File tools/telemetry/support/html_output/results-template.html (right):
https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm...
tools/telemetry/support/html_output/results-template.html:703: setTestType:
function (testName) {
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.
https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm...
tools/telemetry/support/html_output/results-template.html:729:
this.setTestType(checkboxName)
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;
}
}
}
https://codereview.chromium.org/1523743002/diff/1/tools/telemetry/support/htm...
tools/telemetry/support/html_output/results-template.html:816: if
(testSelector.showTest(test)) {
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
https://codereview.chromium.org/1523743002/diff/60001/tools/telemetry/support...
File tools/telemetry/support/html_output/results-template.html (right):
https://codereview.chromium.org/1523743002/diff/60001/tools/telemetry/support...
tools/telemetry/support/html_output/results-template.html:693: function
buildButtonHTMLFromButtonNames(buttonNames, activeButton) {
nit: s/activeButton/activeButtonName/ (this way it seems like you're passing in
the actual active button itself).
https://codereview.chromium.org/1523743002/diff/60001/tools/telemetry/support...
tools/telemetry/support/html_output/results-template.html:693: function
buildButtonHTMLFromButtonNames(buttonNames, activeButton) {
This function requires too much work on the part of the called. I think it
should just take a single testTypeSelecector argument instead (it knows the list
of test type names and it also knows which type is active).
https://codereview.chromium.org/1523743002/diff/60001/tools/telemetry/support...
tools/telemetry/support/html_output/results-template.html:695: for (var button
in buttonNames) {
You should *never* iterate over an array in JS like this. Either use a for loop,
or in this case I think the following would be the most elegant (and have better
performance because you avoid appending strings, which is usually costly):
return buttonNames.map(function(buttonName) {
var classAttribute = buttonName === activeButtonName ? ' class=checked': '';
return '<span' + classAttribute + '>' + buttonName + '</span>';
}).join('');
https://codereview.chromium.org/1523743002/diff/60001/tools/telemetry/support...
tools/telemetry/support/html_output/results-template.html:697: var classType =
buttonName === activeButton ? 'class=checked' : '';
supernit: I think "classAttribute" is more precise here.
https://codereview.chromium.org/1523743002/diff/60001/tools/telemetry/support...
tools/telemetry/support/html_output/results-template.html:698: buttonHTML +=
'<span ' + classType + '>' + buttonName + '</span>';
nit: This will result in "<span >" for inactive buttons. I suggest you move the
extra space from line 698 to the first ternary operator case on line 697.
https://codereview.chromium.org/1523743002/diff/60001/tools/telemetry/support...
tools/telemetry/support/html_output/results-template.html:704: this.buttonNames
= this.generateButtonHTML(tests);
This should be called "testTypeNames". You use its elements to set the test type
(it's strange to assign a "button name" to a "test type").
https://codereview.chromium.org/1523743002/diff/60001/tools/telemetry/support...
tools/telemetry/support/html_output/results-template.html:708: recognizers: {
I've just realized that you do modify the list of recognizers on lines 739 and
744, so you need to define this separately on each instance (i.e. not here, but
in the constructor). Sorry for the confusion.
https://codereview.chromium.org/1523743002/diff/60001/tools/telemetry/support...
tools/telemetry/support/html_output/results-template.html:709: 'Time':
function(test) { return test.isMemoryTest(); },
style: It's usually discouraged to vertically align blocks of code using spaces
like this. You should just have one space after each colon.
https://codereview.chromium.org/1523743002/diff/60001/tools/telemetry/support...
tools/telemetry/support/html_output/results-template.html:712:
Please change this to a setter:
set testTypeName(testTypeName) {
this.recognizerDelegate = this.recognizers[testTypeName];
}
https://codereview.chromium.org/1523743002/diff/60001/tools/telemetry/support...
tools/telemetry/support/html_output/results-template.html:717: shouldShowTest:
function(test) {
This function seems to be an unnecessary proxy to recognizerDelegate. Instead,
you could just set:
this.shouldShowTest = this.recognizers[testType];
This would be simpler and potentially faster (remember that JS is not always
compiled).
Note that the interface of the whole class will stay exactly the same, so if you
ever do need to do "crazier" stuff, you can just reintroduce this wrapper.
If you really want to keep it (even though I don't see why), then please make
recognizerDelegate "private" (i.e. append an underscore to its name).
https://codereview.chromium.org/1523743002/diff/60001/tools/telemetry/support...
tools/telemetry/support/html_output/results-template.html:721:
generateButtonHTML: function(tests) {
This function does not generate HTML code anymore, so it should be renamed.
https://codereview.chromium.org/1523743002/diff/60001/tools/telemetry/support...
tools/telemetry/support/html_output/results-template.html:736: if
(buttonNames.length == 0) {
nit: s/==/===/
https://codereview.chromium.org/1523743002/diff/60001/tools/telemetry/support...
tools/telemetry/support/html_output/results-template.html:739:
this.recognizers[newButtonName] = function(){return false;};
nit: spaces in function
https://codereview.chromium.org/1523743002/diff/60001/tools/telemetry/support...
tools/telemetry/support/html_output/results-template.html:744:
this.recognizers[newButtonName] = function(){return true;};
ditto
https://codereview.chromium.org/1523743002/diff/60001/tools/telemetry/support...
tools/telemetry/support/html_output/results-template.html:1439:
useLargeLinePlots = checkedElement.textContent == 'Line';
You're creating a global variable here. Please prefix with "var":
var useLargeLinePlots = ...
https://codereview.chromium.org/1523743002/diff/60001/tools/telemetry/support...
tools/telemetry/support/html_output/results-template.html:1453: referenceIndex =
parseInt(checkedElement.getAttribute('value'));
Again, needs 'var'
https://codereview.chromium.org/1523743002/diff/60001/tools/telemetry/support...
tools/telemetry/support/html_output/results-template.html:1457: var
testTypeSelector = new TestTypeSelector(metrics);
I suggest that you move the whole initialization of the selector above line
1438, i.e. before it is referenced for the first time on line 1440. It makes the
code more robust (in case some of the callbacks fired immediately) and, most
importantly, much more readable.
https://codereview.chromium.org/1523743002/diff/60001/tools/telemetry/support...
tools/telemetry/support/html_output/results-template.html:1458: var
initialTestName = testTypeSelector.buttonNames[0];
I can see that there is a certain benefit in setting the initialTestName from
the outside (I would prefer otherwise, but I'm fine with this as well), but I
don't like the fact that this code now knows way too much about the testNames
etc. I think that you should set the testType once and then let the
testTypeSelector take care of everything else:
var testTypeSelector = new TestTypeSelector(metrics);
var initialTestTypeName = testTypeSelector.testTypeNames[0];
testTypeSelector.testTypeName = initialTestTypeName;
var buttonHTML = testTypeSelector.generateButtonHTML();
Changes made! https://codereview.chromium.org/1523743002/diff/60001/tools/telemetry/support... File tools/telemetry/support/html_output/results-template.html (right): https://codereview.chromium.org/1523743002/diff/60001/tools/telemetry/support... tools/telemetry/support/html_output/results-template.html:693: function buildButtonHTMLFromButtonNames(buttonNames, activeButton) { On 2015/12/17 11:17:15, petrcermak wrote: > nit: s/activeButton/activeButtonName/ (this way it seems like you're passing in > the actual active button itself). Done. https://codereview.chromium.org/1523743002/diff/60001/tools/telemetry/support... tools/telemetry/support/html_output/results-template.html:695: for (var button in buttonNames) { I find your version a little dense, but I've used it in the name of performance :) Poor old Orion is busy banging his head on the table trying to get For-In working, and here we are actively avoiding it! https://codereview.chromium.org/1523743002/diff/60001/tools/telemetry/support... tools/telemetry/support/html_output/results-template.html:697: var classType = buttonName === activeButton ? 'class=checked' : ''; On 2015/12/17 11:17:15, petrcermak wrote: > supernit: I think "classAttribute" is more precise here. Done. https://codereview.chromium.org/1523743002/diff/60001/tools/telemetry/support... tools/telemetry/support/html_output/results-template.html:698: buttonHTML += '<span ' + classType + '>' + buttonName + '</span>'; On 2015/12/17 11:17:15, petrcermak wrote: > nit: This will result in "<span >" for inactive buttons. I suggest you move the > extra space from line 698 to the first ternary operator case on line 697. Done. https://codereview.chromium.org/1523743002/diff/60001/tools/telemetry/support... tools/telemetry/support/html_output/results-template.html:704: this.buttonNames = this.generateButtonHTML(tests); On 2015/12/17 11:17:15, petrcermak wrote: > This should be called "testTypeNames". You use its elements to set the test type > (it's strange to assign a "button name" to a "test type"). Done. https://codereview.chromium.org/1523743002/diff/60001/tools/telemetry/support... tools/telemetry/support/html_output/results-template.html:708: recognizers: { On 2015/12/17 11:17:15, petrcermak wrote: > I've just realized that you do modify the list of recognizers on lines 739 and > 744, so you need to define this separately on each instance (i.e. not here, but > in the constructor). Sorry for the confusion. Done. https://codereview.chromium.org/1523743002/diff/60001/tools/telemetry/support... tools/telemetry/support/html_output/results-template.html:709: 'Time': function(test) { return test.isMemoryTest(); }, On 2015/12/17 11:17:15, petrcermak wrote: > style: It's usually discouraged to vertically align blocks of code using spaces > like this. You should just have one space after each colon. Done. https://codereview.chromium.org/1523743002/diff/60001/tools/telemetry/support... tools/telemetry/support/html_output/results-template.html:712: On 2015/12/17 11:17:15, petrcermak wrote: > Please change this to a setter: > > set testTypeName(testTypeName) { > this.recognizerDelegate = this.recognizers[testTypeName]; > } Done. https://codereview.chromium.org/1523743002/diff/60001/tools/telemetry/support... tools/telemetry/support/html_output/results-template.html:717: shouldShowTest: function(test) { This is cool! JS is awesome in its weirdness. https://codereview.chromium.org/1523743002/diff/60001/tools/telemetry/support... tools/telemetry/support/html_output/results-template.html:721: generateButtonHTML: function(tests) { D'Oh. Done! https://codereview.chromium.org/1523743002/diff/60001/tools/telemetry/support... tools/telemetry/support/html_output/results-template.html:739: this.recognizers[newButtonName] = function(){return false;}; On 2015/12/17 11:17:15, petrcermak wrote: > nit: spaces in function Done. https://codereview.chromium.org/1523743002/diff/60001/tools/telemetry/support... tools/telemetry/support/html_output/results-template.html:744: this.recognizers[newButtonName] = function(){return true;}; On 2015/12/17 11:17:15, petrcermak wrote: > ditto Done. https://codereview.chromium.org/1523743002/diff/60001/tools/telemetry/support... tools/telemetry/support/html_output/results-template.html:1439: useLargeLinePlots = checkedElement.textContent == 'Line'; Yes, this is nasty and is part of the original code that we inherited. This currently needs to be a global (and is defined just above on line 1435) as the bound functions (e.g. the event handlers in #scatter-line and #reference) need access to these so they can rebuild the tables correctly. I am intending (with the next CL...) to put useLargeLinePlots and referenceIndex into the testTypeSelector. This reduces the number of parameters passed to displayTable and gets rid of the globals. https://codereview.chromium.org/1523743002/diff/60001/tools/telemetry/support... tools/telemetry/support/html_output/results-template.html:1453: referenceIndex = parseInt(checkedElement.getAttribute('value')); See comment about useLargeLineplots. https://codereview.chromium.org/1523743002/diff/60001/tools/telemetry/support... tools/telemetry/support/html_output/results-template.html:1457: var testTypeSelector = new TestTypeSelector(metrics); On 2015/12/17 11:17:15, petrcermak wrote: > I suggest that you move the whole initialization of the selector above line > 1438, i.e. before it is referenced for the first time on line 1440. It makes the > code more robust (in case some of the callbacks fired immediately) and, most > importantly, much more readable. Done. https://codereview.chromium.org/1523743002/diff/60001/tools/telemetry/support... tools/telemetry/support/html_output/results-template.html:1458: var initialTestName = testTypeSelector.buttonNames[0]; OK. I've done a 360 on this. I've moved the HTML generation back into the testTypeSelector as you have suggested, and made the calling code agnostic as to the initial test type. I'm not terribly keen on having HTML generated by the testTypeSelector class (as this is surely not the concern of a test related class?) but it simplifies and encapsulates the code.
LGTM with a few more nits. Thanks for being patient with me :-) Petr https://codereview.chromium.org/1523743002/diff/80001/tools/telemetry/support... File tools/telemetry/support/html_output/results-template.html (right): https://codereview.chromium.org/1523743002/diff/80001/tools/telemetry/support... tools/telemetry/support/html_output/results-template.html:712: for (var recognisedTestName in this.recognizers) { nit: s/recognised/recognized/ for consistency https://codereview.chromium.org/1523743002/diff/80001/tools/telemetry/support... tools/telemetry/support/html_output/results-template.html:713: var recognizes = this.recognizers[recognisedTestName]; nit: I'd say 'var recognizer', but feel free to ignore. https://codereview.chromium.org/1523743002/diff/80001/tools/telemetry/support... tools/telemetry/support/html_output/results-template.html:726: this.recognizers[noResults] = function(){ return false; }; nit: There should be a space between "()" and "{". https://codereview.chromium.org/1523743002/diff/80001/tools/telemetry/support... tools/telemetry/support/html_output/results-template.html:731: this.recognizers[allResults] = function(){ return true; }; ditto nit https://codereview.chromium.org/1523743002/diff/80001/tools/telemetry/support... tools/telemetry/support/html_output/results-template.html:742: var classAttribute = testTypeName === selectedTestTypeName ? ' class=checked': ''; nit: missing space before ':'. https://codereview.chromium.org/1523743002/diff/80001/tools/telemetry/support... tools/telemetry/support/html_output/results-template.html:746: nit: no need for a blank line here (it's customary not to have a blank line between the last method and the end of the prototype).
picksi@google.com changed reviewers: + sullivan@chromium.org
Also adding Annie as an owner. Here's some example output... A page containing just Memory results: https://x20web.corp.google.com/~picksi/results_just_memory.html And a page with both memory and time results (note that the page adds an 'all' button as well): https://x20web.corp.google.com/~picksi/results_all.html If there are no results the button says "No Results". https://codereview.chromium.org/1523743002/diff/80001/tools/telemetry/support... File tools/telemetry/support/html_output/results-template.html (right): https://codereview.chromium.org/1523743002/diff/80001/tools/telemetry/support... tools/telemetry/support/html_output/results-template.html:712: for (var recognisedTestName in this.recognizers) { On 2016/01/04 10:44:04, petrcermak wrote: > nit: s/recognised/recognized/ for consistency Done. https://codereview.chromium.org/1523743002/diff/80001/tools/telemetry/support... tools/telemetry/support/html_output/results-template.html:713: var recognizes = this.recognizers[recognisedTestName]; I've left it as recognizes to keep the "if (recognizes(test)) " line readable English, which pleases me :) https://codereview.chromium.org/1523743002/diff/80001/tools/telemetry/support... tools/telemetry/support/html_output/results-template.html:726: this.recognizers[noResults] = function(){ return false; }; On 2016/01/04 10:44:04, petrcermak wrote: > nit: There should be a space between "()" and "{". Done. https://codereview.chromium.org/1523743002/diff/80001/tools/telemetry/support... tools/telemetry/support/html_output/results-template.html:731: this.recognizers[allResults] = function(){ return true; }; On 2016/01/04 10:44:04, petrcermak wrote: > ditto nit Done. https://codereview.chromium.org/1523743002/diff/80001/tools/telemetry/support... tools/telemetry/support/html_output/results-template.html:742: var classAttribute = testTypeName === selectedTestTypeName ? ' class=checked': ''; On 2016/01/04 10:44:05, petrcermak wrote: > nit: missing space before ':'. Done. https://codereview.chromium.org/1523743002/diff/80001/tools/telemetry/support... tools/telemetry/support/html_output/results-template.html:746: On 2016/01/04 10:44:04, petrcermak wrote: > nit: no need for a blank line here (it's customary not to have a blank line > between the last method and the end of the prototype). Done.
lgtm
The CQ bit was checked by picksi@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from petrcermak@chromium.org Link to the patchset: https://codereview.chromium.org/1523743002/#ps100001 (title: "Code review nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1523743002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1523743002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by picksi@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from petrcermak@chromium.org, sullivan@chromium.org Link to the patchset: https://codereview.chromium.org/1523743002/#ps120001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1523743002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1523743002/120001
Message was sent while issue was closed.
Description was changed from ========== 'Time'/'Memory' buttons added to results.html dynamically for available result types. If no results are available an unpressable 'no results' button is added. If both types of results are available an 'all' option is added. This refactor makes the results screen more 'stateful' and the code a little easier to understand than the previous implementation that relied on an 'ignore' bool to skip results. BUG=569522 ========== to ========== 'Time'/'Memory' buttons added to results.html dynamically for available result types. If no results are available an unpressable 'no results' button is added. If both types of results are available an 'all' option is added. This refactor makes the results screen more 'stateful' and the code a little easier to understand than the previous implementation that relied on an 'ignore' bool to skip results. BUG=569522 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== 'Time'/'Memory' buttons added to results.html dynamically for available result types. If no results are available an unpressable 'no results' button is added. If both types of results are available an 'all' option is added. This refactor makes the results screen more 'stateful' and the code a little easier to understand than the previous implementation that relied on an 'ignore' bool to skip results. BUG=569522 ========== to ========== 'Time'/'Memory' buttons added to results.html dynamically for available result types. If no results are available an unpressable 'no results' button is added. If both types of results are available an 'all' option is added. This refactor makes the results screen more 'stateful' and the code a little easier to understand than the previous implementation that relied on an 'ignore' bool to skip results. BUG=569522 Committed: https://crrev.com/1df7f885db651781239a2d5bcfcec843a12ef2f4 Cr-Commit-Position: refs/heads/master@{#367576} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/1df7f885db651781239a2d5bcfcec843a12ef2f4 Cr-Commit-Position: refs/heads/master@{#367576} |
