Issue 249700043: Add histograms and boxplots.
|
Created:
8 years, 9 months ago by Toby Sargeant Modified:
8 years, 8 months ago CC:
tracing-review_chromium.org Base URL:
https://github.com/google/trace-viewer.git@master Visibility:
Public. |
DescriptionAdd histograms and boxplots.
BUG=
Patch Set 1 #
Total comments: 138
Patch Set 2 : address comments #Patch Set 3 : Fix typo; give up on using a base class svg transform to reorient plots. #
Total comments: 11
MessagesTotal messages: 15
PTAL
Sign in to reply to this message.
What are these for? Is there I bug I can look at for context?
Sign in to reply to this message.
On 2015/07/10 14:16:40, dsinclair wrote: > What are these for? Is there I bug I can look at for context? There's no bug. They're for some follow-up work that I intend to do regarding plotting aggregate timing statistics.
Sign in to reply to this message.
On 2015/07/10 14:18:49, Toby Sargeant wrote: > On 2015/07/10 14:16:40, dsinclair wrote: > > What are these for? Is there I bug I can look at for context? > > There's no bug. They're for some follow-up work that I intend to do regarding > plotting aggregate timing statistics. Can you file a bug for context? Where is this plotting going? One of the analysis panes or a side panel? What aggregate statistics?
Sign in to reply to this message.
Looks good overall! Here's a couple of (mostly style) comments. Thanks, Petr https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... File tracing/tracing/ui/base/boxplot.css (right): https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.css:1: /* Copyright 2015 The Chromium Authors. All rights reserved. We now favor inline css (e.g. this one in boxplot.html) over separate css files. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... File tracing/tracing/ui/base/boxplot.html (right): https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:3: Copyright (c) 2014 The Chromium Authors. All rights reserved. s/(c) 2014/2015/ https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:7: <link rel="import" href="/ui/base/d3.html"> Please sort the imports alphabetically https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:7: <link rel="import" href="/ui/base/d3.html"> nit: I'd add a blank line between the licence header and the first import. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:10: <script> nit: I'd add a blank line before the script https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:32: this.bins_ = undefined; // user defined bins nit: Comments typically start with a capital letter and end with a fullstop (even if they're not sentences). https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:32: this.bins_ = undefined; // user defined bins nit: s/user defined/user-defined/ https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:33: this.extent_ = undefined; // calculate from data What is "extent"? Domain? Range? The comment doesn't explain the meaning (unlike for the other fields). https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:38: this.boxPad_ = 0.5; // inter-box padding How about *inner* and outer padding (like in CSS)? https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:65: this.vertical_ = !!!horizontal; A little too many exclamation marks here :-) (one will be enough) https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:101: styleXAxis_: function(xAxis) { This function doesn't seem to do anything useful. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:108: styleYAxis_: function(yAxis) { ditto https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:150: if (this.data_ === undefined) return; Please put the return statement on a separate lines (still no need for braces). https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:152: var box_width = this.datasetScale_.rangeBand(); nit: we mostly use camel case for variable names https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:155: if (this.maxBoxWidth_ && box_width > this.maxBoxWidth_) { This will fail if this.maxBoxWidth_ is 0. Is that the desired behavior? https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:157: for (var i = 0; i < box_pos.length; ++i) { nit: no need for braces https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:164: var v = new Float32Array(d.values); I think that "values" and "stats" would be better variable names than "v" and "o". https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:166: var o = { There are two issues with this object: (1) it contains a lot of fields that are only meant to be internal to this function and not returned (mean, q*, label), (2) its fields have different units (this is especially striking with q50 and median, which one would intuitively expect to be the same). I propose changing the internal variables to locals (this will also have the nice side-effect of improving performance): var q05 = d3.quantile(v, .05); var q25 = d3.quantile(v, .25); var q50 = d3.quantile(v, .50); var q75 = d3.quantile(v, .75); var q95 = d3.quantile(v, .95); return { outliers: d.values.filter(function(x) { return x < q05 || x > q95; }).map(self.valueScale_), boxrange: d3.extent([ self.valueScale_(q25), self.valueScale_(q75) ]), whisker: d3.extent([ self.valueScale_(q05), self.valueScale_(q95) ]), median: self.valueScale_(q50); }; https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:191: var boxes = boxplot.selectAll('g.box').data(data); I find it a little difficult to see how the elements created by this function map to the resulting histogram. Could you please add a comment explaining how this works. I think that a sample resulting element hierarchy would be ideal — something along the following lines: <g id="boxplot"> <!-- Buckets --> <g class="box"> <g> <!-- explain what these are --> <rect> </g> <g class="box">...</g> ... </g> In addition, I think it would be really helpful if you (1) added short inline comments explaining the meanings of variables/statements and (2) gave variables more descriptive names (e.g. "g" is too generic). https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:195: .enter() The indentation is a little off here. If you want to keep this on a separate line, then it should be indented by a multiple of 2. I think it would be fine to put it on the previous line though. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:199: .attr('transform', function(d, i) { nit: I think this would be just fine on the previous line. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:204: boxes can't you just continue with attr('x', 0) after the classed('box', true) call above? https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:218: var ww = Math.min(10, box_width / 4); Please give this variable a better name. "whiskerWidth"? https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:225: 'm' + (-ww) + ' 0H' + (+ww) + I think that this line should be merged with the previous one and lines 225-227 should be indented 2 more spaces: return 'M 0 ' + d.boxrange[1] + 'V' + d.whisker[1] + 'm' + (-ww) + ' 0H' + (+ww) + 'M 0 ' + d.boxrange[0] + 'V' + d.whisker[0] + 'm' + (-ww) + ' 0H' + (+ww); https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:232: .exit() Again, this should either be on the previous line, or indented two spaces. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:240: .enter() ditto https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:248: .exit() ditto https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... File tracing/tracing/ui/base/boxplot_test.html (right): https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot_test.html:3: Copyright (c) 2015 The Chromium Authors. All rights reserved. New files should not have the "(c)" anymore (see https://www.chromium.org/developers/coding-style#TOC-File-headers). https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot_test.html:7: <link rel="import" href="/ui/base/boxplot.html"> nit: I would add a blank line above the import https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot_test.html:8: <script> nit: I'd add a blank line before the script https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot_test.html:12: var makeChart = function() { nit: "function makeChart()" is preferable. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/chart... File tracing/tracing/ui/base/chart_base.html (right): https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/chart... tracing/tracing/ui/base/chart_base.html:94: this.oma_ = { top: 20, right: 20, bottom: 40, left: 50 }; Please give this field a better name. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/chart... tracing/tracing/ui/base/chart_base.html:232: if (this.xTicks_ !== undefined) { nit: no need for braces. We usually prefer early-outs (if (variable === undefined) { return; } /* actual code */), but I don't think we need it here. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/chart... tracing/tracing/ui/base/chart_base.html:238: if (this.yTicks_ !== undefined) { ditto https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/chart... tracing/tracing/ui/base/chart_base.html:252: var xAxisRenderer = d3.svg There's a bit of code duplication in the two if statements. How about defining an inner function to reuse the common code structure? https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... File tracing/tracing/ui/base/histogram.css (right): https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.css:1: /* Copyright 2015 The Chromium Authors. All rights reserved. Again, we favor inline CSS. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... File tracing/tracing/ui/base/histogram.html (right): https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:3: Copyright (c) 2014 The Chromium Authors. All rights reserved. Again s/(c) 2014/2015/ https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:7: <link rel="import" href="/ui/base/d3.html"> nit: I'd add the new line https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:7: <link rel="import" href="/ui/base/d3.html"> sort the imports alphabetically please https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:10: <script> nit: I'd add a blank line before the script https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:32: this.bins_ = undefined; // user defined bins See my coments in boxplot.html. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:76: this.vertical_ = !!!horizontal; s/!!!/!/ https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:84: set rwidth(rwidth) { why not relativeWidth? This is unnecessarily ambiguous. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:96: for (var i = 0; i < N; ++i) { nit: no need for braces https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:105: if (extent[0] == extent[1]) { use === instead of == https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:106: this.breaks_ = new Float32Array([ How come this is a 2-element array here but an (N+1)-element array on line 111? https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:121: // list of breaks. nit: Please start comments with uppercase letters. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:124: } else if (typeof(this.bins_) === typeof(0)) { I'd use "'number'" instead of "typeof(0)" https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:125: // count of equally sized bins. nit: Please start comments with uppercase letters. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:128: this.calculateBreaks_( nit: I think it's more natural as follows (note that the indent should be 4 places): this.calculateBreaks_( Math.max(2, Math.floor(this.data_.size() / 10.0))); https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:140: this.data_.forEach(function(k, v) { "k" and "v" suggest "key" and "value" of a dictionary. Why not use "d" and "i" like everywhere else (seems to be the "standard" in d3)? https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:144: } else if (v == self.breaks_[N]) { === https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:155: if (this.vertical_) { This might be a little simpler to understand as follows: var breakScale = [ this.breaks_[0], this.breaks_[this.breaks_.length - 1] ]; var valueScale = [ 0.0, d3.max(this.counts_) ]; if (this.vertical_) { this.xScale_.domain(breakScale); this.yScale_.domain(valueScale); } else { this.xScale_.domain(valueScale); this.yScale_.domain(breakScale); } https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:181: var p = []; Please use better descriptive variable names in this method. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:184: this.breaks_, this should be indented 4 spaces https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:185: this.vertical_ ? this.xScale_ : this.yScale_); ditto (+ this will possibly fit on the previous line) https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:187: this.counts_, ditto https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:192: if (this.vertical_) { you won't need the braces if you collapse the objects below onto single lines. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:194: x1: b[i], x2: b[i + 1], y1: c[i], y2: c0 no need to put this on a separate line https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:198: x1: c0, x2: c[i], y1: b[i + 1], y2: b[i] ditto https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:208: w = (p[i][c2] - p[i][c1]) / 2; nit: I'd personally make this 2 separate var statements. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:218: var self = this; no need for this I think https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:226: .enter() even indentation https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:236: .exit() even indentation https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... File tracing/tracing/ui/base/histogram_test.html (right): https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram_test.html:3: Copyright (c) 2015 The Chromium Authors. All rights reserved. s/(c) // https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram_test.html:7: <link rel="import" href="/ui/base/histogram.html"> nit: blank line above and below https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram_test.html:17: var data = [ nit: I don't think there's any need to declare the variable and split it over multiple lines. The following would work perfectly well: chart.data = [1, 2, 3, 3, 4, 4, 4, 5, 5, 6, 7] https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram_test.html:18: 1, 2, 3, 3, 4, 4, 4, 5, 5, 6, 7 Would it be better to give it an unsorted list?
Sign in to reply to this message.
https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... File tracing/tracing/ui/base/boxplot.html (right): https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:20: var BoxPlot = tr.ui.b.define('boxplot', ChartBase); These should probably all be polymer components instead of tr.ui components.
Sign in to reply to this message.
> These should probably all be polymer components instead of tr.ui components. i think that requires us movign all the old chartbase-derived components to polymer; iirw, there was some issue there around svg? anyway thats probably a separate project to adding these widgets....
Sign in to reply to this message.
https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... File tracing/tracing/ui/base/boxplot.html (right): https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:9: <link rel="stylesheet" href="/ui/base/boxplot.css"> please inline your css files into a <style> ... your stuff here </style> tag. we only do external css files for legacy reasons or when there is direct style sharing between different files
Sign in to reply to this message.
Not finished here, but I've gone through and addressed most of the comments, and cleaned up a few other rough edges as well. What do you think about refactoring the plot orientation down into ChartBase? I suspect that it could reduce the complexity of the histogram and boxplot classes without affecting any other subclasses that don't care about orientation. [I need to test to see whether it can be done without the top-left origin of svg getting in the way...] https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... File tracing/tracing/ui/base/boxplot.html (right): https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:3: Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2015/07/14 12:26:12, petrcermak wrote: > s/(c) 2014/2015/ Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:7: <link rel="import" href="/ui/base/d3.html"> On 2015/07/14 12:26:12, petrcermak wrote: > Please sort the imports alphabetically Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:7: <link rel="import" href="/ui/base/d3.html"> On 2015/07/14 12:26:12, petrcermak wrote: > nit: I'd add a blank line between the licence header and the first import. Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:9: <link rel="stylesheet" href="/ui/base/boxplot.css"> On 2015/07/15 23:49:48, nduca(google) wrote: > please inline your css files into a <style> ... your stuff here </style> tag. > we only do external css files for legacy reasons or when there is direct style > sharing between different files Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:10: <script> On 2015/07/14 12:26:12, petrcermak wrote: > nit: I'd add a blank line before the script Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:20: var BoxPlot = tr.ui.b.define('boxplot', ChartBase); On 2015/07/14 13:53:26, dsinclair wrote: > These should probably all be polymer components instead of tr.ui components. I don't think this can happen without ChartBase being converted first. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:32: this.bins_ = undefined; // user defined bins On 2015/07/14 12:26:11, petrcermak wrote: > nit: s/user defined/user-defined/ Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:32: this.bins_ = undefined; // user defined bins On 2015/07/14 12:26:13, petrcermak wrote: > nit: Comments typically start with a capital letter and end with a fullstop > (even if they're not sentences). Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:33: this.extent_ = undefined; // calculate from data On 2015/07/14 12:26:12, petrcermak wrote: > What is "extent"? Domain? Range? The comment doesn't explain the meaning (unlike > for the other fields). Removed. Was cut and paste from histogram.html https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:38: this.boxPad_ = 0.5; // inter-box padding On 2015/07/14 12:26:11, petrcermak wrote: > How about *inner* and outer padding (like in CSS)? Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:65: this.vertical_ = !!!horizontal; On 2015/07/14 12:26:13, petrcermak wrote: > A little too many exclamation marks here :-) (one will be enough) Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:101: styleXAxis_: function(xAxis) { On 2015/07/14 12:26:13, petrcermak wrote: > This function doesn't seem to do anything useful. Removed. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:108: styleYAxis_: function(yAxis) { On 2015/07/14 12:26:12, petrcermak wrote: > ditto Removed. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:150: if (this.data_ === undefined) return; On 2015/07/14 12:26:12, petrcermak wrote: > Please put the return statement on a separate lines (still no need for braces). Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:152: var box_width = this.datasetScale_.rangeBand(); On 2015/07/14 12:26:12, petrcermak wrote: > nit: we mostly use camel case for variable names Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:155: if (this.maxBoxWidth_ && box_width > this.maxBoxWidth_) { On 2015/07/14 12:26:11, petrcermak wrote: > This will fail if this.maxBoxWidth_ is 0. Is that the desired behavior? Yes. If maxBoxWidth_ were zero, nothing would be drawn. Maybe >= 1 would be desirable? https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:157: for (var i = 0; i < box_pos.length; ++i) { On 2015/07/14 12:26:12, petrcermak wrote: > nit: no need for braces Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:164: var v = new Float32Array(d.values); On 2015/07/14 12:26:11, petrcermak wrote: > I think that "values" and "stats" would be better variable names than "v" and > "o". Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:166: var o = { On 2015/07/14 12:26:12, petrcermak wrote: > There are two issues with this object: (1) it contains a lot of fields that are > only meant to be internal to this function and not returned (mean, q*, label), > (2) its fields have different units (this is especially striking with q50 and > median, which one would intuitively expect to be the same). I propose changing > the internal variables to locals (this will also have the nice side-effect of > improving performance): > > var q05 = d3.quantile(v, .05); > var q25 = d3.quantile(v, .25); > var q50 = d3.quantile(v, .50); > var q75 = d3.quantile(v, .75); > var q95 = d3.quantile(v, .95); > > return { > outliers: d.values.filter(function(x) { > return x < q05 || x > q95; > }).map(self.valueScale_), > boxrange: d3.extent([ > self.valueScale_(q25), self.valueScale_(q75) > ]), > whisker: d3.extent([ > self.valueScale_(q05), self.valueScale_(q95) > ]), > median: self.valueScale_(q50); > }; Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:191: var boxes = boxplot.selectAll('g.box').data(data); On 2015/07/14 12:26:13, petrcermak wrote: > I find it a little difficult to see how the elements created by this function > map to the resulting histogram. Could you please add a comment explaining how > this works. I think that a sample resulting element hierarchy would be ideal — > something along the following lines: > > <g id="boxplot"> > <!-- Buckets --> > <g class="box"> > <g> <!-- explain what these are --> > <rect> > </g> > <g class="box">...</g> > ... > </g> > > In addition, I think it would be really helpful if you (1) added short inline > comments explaining the meanings of variables/statements and (2) gave variables > more descriptive names (e.g. "g" is too generic). Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:195: .enter() On 2015/07/14 12:26:12, petrcermak wrote: > The indentation is a little off here. If you want to keep this on a separate > line, then it should be indented by a multiple of 2. I think it would be fine to > put it on the previous line though. Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:199: .attr('transform', function(d, i) { On 2015/07/14 12:26:13, petrcermak wrote: > nit: I think this would be just fine on the previous line. Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:204: boxes On 2015/07/14 12:26:12, petrcermak wrote: > can't you just continue with attr('x', 0) after the classed('box', true) call > above? No, because g refers to the entering selection, and we want to update the entering+updating selection here. I might reorder to put all the creation of new elements together and comment so that this is more clear. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:218: var ww = Math.min(10, box_width / 4); On 2015/07/14 12:26:12, petrcermak wrote: > Please give this variable a better name. "whiskerWidth"? Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:225: 'm' + (-ww) + ' 0H' + (+ww) + On 2015/07/14 12:26:12, petrcermak wrote: > I think that this line should be merged with the previous one and lines 225-227 > should be indented 2 more spaces: > > return 'M 0 ' + d.boxrange[1] + 'V' + d.whisker[1] + > 'm' + (-ww) + ' 0H' + (+ww) + > 'M 0 ' + d.boxrange[0] + 'V' + d.whisker[0] + > 'm' + (-ww) + ' 0H' + (+ww); Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:232: .exit() On 2015/07/14 12:26:12, petrcermak wrote: > Again, this should either be on the previous line, or indented two spaces. Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:240: .enter() On 2015/07/14 12:26:12, petrcermak wrote: > ditto Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot.html:248: .exit() On 2015/07/14 12:26:12, petrcermak wrote: > ditto Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... File tracing/tracing/ui/base/boxplot_test.html (right): https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot_test.html:3: Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2015/07/14 12:26:13, petrcermak wrote: > New files should not have the "(c)" anymore (see > https://www.chromium.org/developers/coding-style#TOC-File-headers). Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot_test.html:7: <link rel="import" href="/ui/base/boxplot.html"> On 2015/07/14 12:26:13, petrcermak wrote: > nit: I would add a blank line above the import Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot_test.html:8: <script> On 2015/07/14 12:26:13, petrcermak wrote: > nit: I'd add a blank line before the script Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/boxpl... tracing/tracing/ui/base/boxplot_test.html:12: var makeChart = function() { On 2015/07/14 12:26:13, petrcermak wrote: > nit: "function makeChart()" is preferable. Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/chart... File tracing/tracing/ui/base/chart_base.html (right): https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/chart... tracing/tracing/ui/base/chart_base.html:94: this.oma_ = { top: 20, right: 20, bottom: 40, left: 50 }; On 2015/07/14 12:26:13, petrcermak wrote: > Please give this field a better name. Done, and cleaned up the inset to be innerMargin. Not that it's an excuse, but oma is what R uses for controlling margins in plots, so I'm used to seeing it :-P https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/chart... tracing/tracing/ui/base/chart_base.html:232: if (this.xTicks_ !== undefined) { On 2015/07/14 12:26:13, petrcermak wrote: > nit: no need for braces. > > We usually prefer early-outs (if (variable === undefined) { return; } /* actual > code */), but I don't think we need it here. Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/chart... tracing/tracing/ui/base/chart_base.html:238: if (this.yTicks_ !== undefined) { On 2015/07/14 12:26:13, petrcermak wrote: > ditto Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/chart... tracing/tracing/ui/base/chart_base.html:252: var xAxisRenderer = d3.svg On 2015/07/14 12:26:13, petrcermak wrote: > There's a bit of code duplication in the two if statements. How about defining > an inner function to reuse the common code structure? I've tightened it up, but there are enough differences that an inner function didn't really make sense. I'm considering refactoring the plot orientation code down to here, which might mean revisiting this. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... File tracing/tracing/ui/base/histogram.html (right): https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:3: Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2015/07/14 12:26:13, petrcermak wrote: > Again s/(c) 2014/2015/ Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:7: <link rel="import" href="/ui/base/d3.html"> On 2015/07/14 12:26:13, petrcermak wrote: > nit: I'd add the new line Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:7: <link rel="import" href="/ui/base/d3.html"> On 2015/07/14 12:26:13, petrcermak wrote: > sort the imports alphabetically please Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:10: <script> On 2015/07/14 12:26:14, petrcermak wrote: > nit: I'd add a blank line before the script Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:32: this.bins_ = undefined; // user defined bins On 2015/07/14 12:26:14, petrcermak wrote: > See my coments in boxplot.html. I think this is clearer now. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:76: this.vertical_ = !!!horizontal; On 2015/07/14 12:26:14, petrcermak wrote: > s/!!!/!/ Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:84: set rwidth(rwidth) { On 2015/07/14 12:26:13, petrcermak wrote: > why not relativeWidth? This is unnecessarily ambiguous. Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:96: for (var i = 0; i < N; ++i) { On 2015/07/14 12:26:14, petrcermak wrote: > nit: no need for braces Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:105: if (extent[0] == extent[1]) { On 2015/07/14 12:26:13, petrcermak wrote: > use === instead of == Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:106: this.breaks_ = new Float32Array([ On 2015/07/14 12:26:14, petrcermak wrote: > How come this is a 2-element array here but an (N+1)-element array on line 111? [ breaks_[i], breaks_[i+1] ) is the extent of bin i (except for the last bin, which is closed, not half-open). In the case where the extent of the bins is zero, we artificially construct one bin, centered around the extent, with a width of 1. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:121: // list of breaks. On 2015/07/14 12:26:14, petrcermak wrote: > nit: Please start comments with uppercase letters. Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:124: } else if (typeof(this.bins_) === typeof(0)) { On 2015/07/14 12:26:14, petrcermak wrote: > I'd use "'number'" instead of "typeof(0)" Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:125: // count of equally sized bins. On 2015/07/14 12:26:14, petrcermak wrote: > nit: Please start comments with uppercase letters. Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:128: this.calculateBreaks_( On 2015/07/14 12:26:14, petrcermak wrote: > nit: I think it's more natural as follows (note that the indent should be 4 > places): > > this.calculateBreaks_( > Math.max(2, Math.floor(this.data_.size() / 10.0))); I've moved the trailing ); but sublime likes to indent this by two spaces, not 4. Is that a definite style ruling, or preference? https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:140: this.data_.forEach(function(k, v) { On 2015/07/14 12:26:14, petrcermak wrote: > "k" and "v" suggest "key" and "value" of a dictionary. Why not use "d" and "i" > like everywhere else (seems to be the "standard" in d3)? this.data_ is actually a map (so that it is in theory possible to get from a bin back to a set of identifiers). https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:144: } else if (v == self.breaks_[N]) { On 2015/07/14 12:26:14, petrcermak wrote: > === Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:155: if (this.vertical_) { On 2015/07/14 12:26:14, petrcermak wrote: > This might be a little simpler to understand as follows: > > var breakScale = [ > this.breaks_[0], > this.breaks_[this.breaks_.length - 1] > ]; > var valueScale = [ > 0.0, > d3.max(this.counts_) > ]; > > if (this.vertical_) { > this.xScale_.domain(breakScale); > this.yScale_.domain(valueScale); > } else { > this.xScale_.domain(valueScale); > this.yScale_.domain(breakScale); > } Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:181: var p = []; On 2015/07/14 12:26:14, petrcermak wrote: > Please use better descriptive variable names in this method. Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:184: this.breaks_, On 2015/07/14 12:26:13, petrcermak wrote: > this should be indented 4 spaces Acknowledged. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:185: this.vertical_ ? this.xScale_ : this.yScale_); On 2015/07/14 12:26:14, petrcermak wrote: > ditto (+ this will possibly fit on the previous line) Acknowledged. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:187: this.counts_, On 2015/07/14 12:26:14, petrcermak wrote: > ditto Acknowledged. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:192: if (this.vertical_) { On 2015/07/14 12:26:14, petrcermak wrote: > you won't need the braces if you collapse the objects below onto single lines. Acknowledged. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:194: x1: b[i], x2: b[i + 1], y1: c[i], y2: c0 On 2015/07/14 12:26:14, petrcermak wrote: > no need to put this on a separate line Acknowledged. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:198: x1: c0, x2: c[i], y1: b[i + 1], y2: b[i] On 2015/07/14 12:26:14, petrcermak wrote: > ditto Acknowledged. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:208: w = (p[i][c2] - p[i][c1]) / 2; On 2015/07/14 12:26:14, petrcermak wrote: > nit: I'd personally make this 2 separate var statements. Acknowledged. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:218: var self = this; On 2015/07/14 12:26:14, petrcermak wrote: > no need for this I think Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:226: .enter() On 2015/07/14 12:26:14, petrcermak wrote: > even indentation Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram.html:236: .exit() On 2015/07/14 12:26:13, petrcermak wrote: > even indentation Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... File tracing/tracing/ui/base/histogram_test.html (right): https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram_test.html:3: Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2015/07/14 12:26:15, petrcermak wrote: > s/(c) // Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram_test.html:7: <link rel="import" href="/ui/base/histogram.html"> On 2015/07/14 12:26:14, petrcermak wrote: > nit: blank line above and below Done. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram_test.html:17: var data = [ On 2015/07/14 12:26:15, petrcermak wrote: > nit: I don't think there's any need to declare the variable and split it over > multiple lines. The following would work perfectly well: > > chart.data = [1, 2, 3, 3, 4, 4, 4, 5, 5, 6, 7] Acknowledged. https://codereview.appspot.com/249700043/diff/1/tracing/tracing/ui/base/histo... tracing/tracing/ui/base/histogram_test.html:18: 1, 2, 3, 3, 4, 4, 4, 5, 5, 6, 7 On 2015/07/14 12:26:15, petrcermak wrote: > Would it be better to give it an unsorted list? Missed this and the above. Will follow up. I think it's a good idea.
Sign in to reply to this message.
Can you file a bug with some context please?
Sign in to reply to this message.
Yes, I'll do that next. Sorry I didn't do it before addressing the other comments.
Sign in to reply to this message.
https://codereview.appspot.com/249700043/diff/40001/tracing/tracing/ui/base/b... File tracing/tracing/ui/base/boxplot.html (right): https://codereview.appspot.com/249700043/diff/40001/tracing/tracing/ui/base/b... tracing/tracing/ui/base/boxplot.html:12: * /deep/ .boxplot path.median { Do these need to be /deep/? /deep/ is deprecated in chrome and, I assume, going to disappear on us at some point in the near future. https://codereview.appspot.com/249700043/diff/40001/tracing/tracing/ui/base/b... tracing/tracing/ui/base/boxplot.html:124: this.maxBoxWidth_ = maxBoxWidth; For most of these set methods we can do: if (this.maxBoxWidth_ === maxBoxWidth) return; To keep us from doing all the updating work if we don't need too. https://codereview.appspot.com/249700043/diff/40001/tracing/tracing/ui/base/b... tracing/tracing/ui/base/boxplot.html:147: if (this.data_ !== undefined) { if (this.data_ === undefined) return; https://codereview.appspot.com/249700043/diff/40001/tracing/tracing/ui/base/b... tracing/tracing/ui/base/boxplot.html:232: }); If you do }.bind(this)); you can change the self on line 212 to this and drop the var self = this; on line 188 https://codereview.appspot.com/249700043/diff/40001/tracing/tracing/ui/base/b... File tracing/tracing/ui/base/boxplot_test.html (right): https://codereview.appspot.com/249700043/diff/40001/tracing/tracing/ui/base/b... tracing/tracing/ui/base/boxplot_test.html:13: /* global tr */ Remove if not needed. https://codereview.appspot.com/249700043/diff/40001/tracing/tracing/ui/base/b... tracing/tracing/ui/base/boxplot_test.html:60: test('vertical', function() { Can you name these instantiate_vertical and instantiate_horizontal? https://codereview.appspot.com/249700043/diff/40001/tracing/tracing/ui/base/b... tracing/tracing/ui/base/boxplot_test.html:62: this.addHTMLOutput(chart); Might want to do a chart.vertical = true; just to guard against the default changing in the future. https://codereview.appspot.com/249700043/diff/40001/tracing/tracing/ui/base/c... File tracing/tracing/ui/base/chart_base.html (right): https://codereview.appspot.com/249700043/diff/40001/tracing/tracing/ui/base/c... tracing/tracing/ui/base/chart_base.html:171: this.xLabelOutset_ = xLabelOutset; If same return (and other set's as well) https://codereview.appspot.com/249700043/diff/40001/tracing/tracing/ui/base/h... File tracing/tracing/ui/base/histogram.html (right): https://codereview.appspot.com/249700043/diff/40001/tracing/tracing/ui/base/h... tracing/tracing/ui/base/histogram.html:103: this.bins_ = bins; if same return https://codereview.appspot.com/249700043/diff/40001/tracing/tracing/ui/base/h... File tracing/tracing/ui/base/histogram_test.html (right): https://codereview.appspot.com/249700043/diff/40001/tracing/tracing/ui/base/h... tracing/tracing/ui/base/histogram_test.html:29: test('vertical', function() { instantiate*** for each of the tests that does addHTMLOutput https://codereview.appspot.com/249700043/diff/40001/tracing/tracing/ui/base/h... tracing/tracing/ui/base/histogram_test.html:30: var chart = makeChart(); chart.vertical = true;
Sign in to reply to this message.
What's the status of this CL?
Sign in to reply to this message.
On 2015/08/05 15:41:20, dsinclair wrote: > What's the status of this CL? I still want to pursue it, but I've been on vacation, and haven't had a huge amount of time. I've addressed most of your comments locally, and I'm looking at what will be required to incorporate graphing of durations in the trace viewer in order to profitably write up the bug.
Sign in to reply to this message.
On 2015/08/05 15:47:50, Toby Sargeant wrote: > On 2015/08/05 15:41:20, dsinclair wrote: > > What's the status of this CL? > > I still want to pursue it, but I've been on vacation, and haven't had a huge > amount of time. I've addressed most of your comments locally, and I'm looking at > what will be required to incorporate graphing of durations in the trace viewer > in order to profitably write up the bug. You'll need to move this over to the catapult repo in order to continue.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
