[v8-dev] Re: - Added ability to call histograms from within v8...

2009-03-12 Thread davemoore
http://codereview.chromium.org/42020/diff/1/7 File src/counters.h (right): http://codereview.chromium.org/42020/diff/1/7#newcode179 Line 179: // HistogramTimer t = { L"t:foo", NULL, false }; On 2009/03/10 23:34:23, iposva wrote: > Histograms are not created with a "t:" prefix as far as I can tel

[v8-dev] Re: - Added ability to call histograms from within v8...

2009-03-11 Thread deanm
http://codereview.chromium.org/42020/diff/1011/1020 File src/v8-counters.cc (right): http://codereview.chromium.org/42020/diff/1011/1020#newcode35 Line 35: HistogramTimer Counters::name = { "" #caption, NULL, false, 0, 0 }; \ You don't need the "" here do you? http://codereview.chromium.org/420

[v8-dev] Re: - Added ability to call histograms from within v8...

2009-03-11 Thread davemoore
http://codereview.chromium.org/42020/diff/1/2 File include/v8.h (right): http://codereview.chromium.org/42020/diff/1/2#newcode1916 Line 1916: static void SetAddHistogramSampleFunction(AddHistogramSampleCallback); On 2009/03/10 23:34:23, iposva wrote: > Please add comments documenting these two n

[v8-dev] Re: - Added ability to call histograms from within v8...

2009-03-10 Thread iposva
Many comments about comments, but otherwise LGTM. -Ivan http://codereview.chromium.org/42020/diff/1/2 File include/v8.h (right): http://codereview.chromium.org/42020/diff/1/2#newcode1916 Line 1916: static void SetAddHistogramSampleFunction(AddHistogramSampleCallback); Please add comments doc

[v8-dev] Re: - Added ability to call histograms from within v8...

2009-03-08 Thread davemoore
http://codereview.chromium.org/39256/diff/1/6 File src/counters.cc (right): http://codereview.chromium.org/39256/diff/1/6#newcode61 Line 61: if (!histogram_) On 2009/03/06 17:24:44, iposva wrote: > Two things here: > - GetHistogram() sounds like it returns a value and it does and the next line >

[v8-dev] Re: - Added ability to call histograms from within v8...

2009-03-08 Thread sgjesse
Just a few fly-by comments. http://codereview.chromium.org/39256/diff/1/6 File src/counters.cc (right): http://codereview.chromium.org/39256/diff/1/6#newcode60 Line 60: GetHistogram(); How about calling GetHistogram EnsureHistogram and make its return value void? http://codereview.chromium.org

[v8-dev] Re: - Added ability to call histograms from within v8...

2009-03-06 Thread iposva
First set of comments. As promised I will run the tests for you later tonight. -Ivan P.S. Does it lint? http://codereview.chromium.org/39256/diff/1/6 File src/counters.cc (right): http://codereview.chromium.org/39256/diff/1/6#newcode61 Line 61: if (!histogram_) Two things here: - GetHistogram