Refactor of Histogram related code

This is a major refactor of Histogram related code:
1. Remove duplicated code from histogram.h/.cc,
   including validating related code and BucketRanges related.
2. Constness of BucketRanges from Histograms, to prevent accidentally modification and provide a simpler interface.
3. Add/move tests.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149495

[kaiwang, 2012-07-26 01:45:48]

I'm still adding some test to this change.
But it is a quite big change, so I send the review early.

jam: could you help to review content/common and chrome/common? Thanks a lot!

[jam, 2012-07-26 21:56:22]

On 2012/07/26 01:45:48, kaiwang wrote:
> I'm still adding some test to this change.
> But it is a quite big change, so I send the review early.
> 
> jam: could you help to review content/common and chrome/common? Thanks a lot!

these two lgtm

[kaiwang, 2012-07-27 19:47:39]

Thanks! Seems some tests are really flaky and I think the errors are not caused
by my change...
This CL is ready for review

[jar (doing other things), 2012-08-01 00:26:10]

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.cc
File base/metrics/histogram.cc (left):

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.cc#old...
base/metrics/histogram.cc:319: DCHECK_EQ(render_histogram->range_checksum(),
range_checksum);
Perhaps this should be preserved, induce a return false; when we don't have a
match.

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.cc
File base/metrics/histogram.cc (right):

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.cc#new...
base/metrics/histogram.cc:550: void Histogram::WriteAsciiBucketGraph(double
current_size, double max_size,
nit: style fix here too (push arg to next line)

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.cc#new...
base/metrics/histogram.cc:761: (bucket_count - 2);
nit: indent only 4 (even though old code did it this way)

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.h
File base/metrics/histogram.h (left):

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.h#oldc...
base/metrics/histogram.h:644: // The dimension of ranges_ in bucket_ranges_ is
bucket_count + 1.
nit: Preserve this comment, since we don't explain (elsewhere)

... or figure out where to place it.

I think it is helpful to describe a slot.... but perhaps you wanted to try to
describe it at the "setter" which is in the constructor.... I'm not sure.

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.h
File base/metrics/histogram.h (right):

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.h#newc...
base/metrics/histogram.h:13: // the minimal range is 1 (instead of 0), maximal
range is
nit: suggest "minimal range" --> "minimum for a declared range"

same for maximal.

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.h#newc...
base/metrics/histogram.h:14: // (HistogramBase::kSampleType_MAX - 1). Currently
you can give ranges input
nit: suggest "can give ranges input..." --> 

"can declare histograms with ranges exceeding those limits, but those excesses
will be silently clamped to those limits (for backwards compatibility with
existing code)."

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.h#newc...
base/metrics/histogram.h:17: // (HistogramBase::kSampleType_MAX - 1). But you
should never depend on this!
nit: "But..." --> "Best practice is to not exceed the limits."

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.h#newc...
base/metrics/histogram.h:19: // For Histogram and LinearHistogram, maximal range
should always be larger (not
nit: "maximal range" --> "maximum for a declared range"

same for minimum

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.h#newc...
base/metrics/histogram.h:20: // equal) than minmal range. And 0 and
HistogramBase::kSampleType_MAX are always
nit: "And 0" --> "Zero"

"are always added" --> "are implicitly added"

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.h#newc...
base/metrics/histogram.h:21: // added as first and last ranges, so the minimal
bucket count is 3. However
nit: "minimal bucket count" --> "smallest legal bucket_count is 3."

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.h#newc...
base/metrics/histogram.h:24: // For these 3 kind histograms, the max bucket
count is always
"kind" --> "kinds of"

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.h#newc...
base/metrics/histogram.h:27: // The buckets layout of Histogram is exponential.
For example, buckets might
"Histogram" --> "class Histogram"

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.h#newc...
base/metrics/histogram.h:480: static std::string SerializeHistogramInfo(const
Histogram& histogram,
note that grouping by "static" is not in style guide... so the old order was
perchance based on utility.

Perhaps it should remain where it was (until you do a more major rewrite).

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.h#newc...
base/metrics/histogram.h:543: virtual ~Histogram();
nit: move to near top of public section.

Perhaps this should stay as protected... and eventually be private when we stop
deriving from this class.

For now, the derived class destructors should probably be protected, or private.

We want to strongly discourage deletion... and at most, only allow such in test
code (which may FRIEND to facilitate).

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.h#newc...
base/metrics/histogram.h:557: Histogram(const std::string& name,
nit: push to top of section.

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.h#newc...
base/metrics/histogram.h:729: // |custom_ranges| contains a set of ranges. Each
of them should be > 0 and
nit: "a set of ranges" --> "a vector of limits on ranges".

"Each of them" --> "Each limit"

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.h#newc...
base/metrics/histogram.h:731: // compatibility). The ranges can be unordered or
contain duplication, but
nit: "ranges" --> "limits"

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram_unitte...
File base/metrics/histogram_unittest.cc (right):

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram_unitte...
base/metrics/histogram_unittest.cc:125:
CustomHistogram::ArrayToCustomRanges(ranges, 3);
As per discussion, consider changing names to better match enum ranges, and
sorted enum ranges, or arbitrary ranges.

maybe ArrayToCustomEnumRanges

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram_unitte...
base/metrics/histogram_unittest.cc:180: // This is a buggy case: Histogram needs
to have at least 3 buckets (4
Nit: Per your suggestion, replace 
"This is buggy code" 
with 

"This test exploits the fact that the sub class can have 2 buckets, while the
base class is *supposed* to have at least 3 buckets.  We should probably change
the restriction on the base class (or not use the base class!)."

http://codereview.chromium.org/10834011/diff/12/base/metrics/statistics_recor...
File base/metrics/statistics_recorder.cc (left):

http://codereview.chromium.org/10834011/diff/12/base/metrics/statistics_recor...
base/metrics/statistics_recorder.cc:249: if (!histograms_)
better to access under lock.

http://codereview.chromium.org/10834011/diff/12/base/metrics/statistics_recor...
File base/metrics/statistics_recorder.cc (right):

http://codereview.chromium.org/10834011/diff/12/base/metrics/statistics_recor...
base/metrics/statistics_recorder.cc:14: using std::list;
nit: perhaps alphabetize

http://codereview.chromium.org/10834011/diff/12/base/metrics/statistics_recor...
base/metrics/statistics_recorder.cc:53: if (lock_ == NULL || histograms_ ==
NULL) {
I'd prefer to have all accesses to histograms_ made with the lock held (as in
original code, line 102).

http://codereview.chromium.org/10834011/diff/12/base/metrics/statistics_recor...
base/metrics/statistics_recorder.cc:68: delete histogram;
Better would be to do the delete outside the lock.

http://codereview.chromium.org/10834011/diff/12/base/metrics/statistics_recor...
base/metrics/statistics_recorder.cc:79: if (lock_ == NULL || ranges_ == NULL) {
Try to avoid access to ranges_ without the lock being held.

http://codereview.chromium.org/10834011/diff/12/base/metrics/statistics_recor...
base/metrics/statistics_recorder.cc:105: delete ranges;
Use scoped_ptr to implicitly delete after we release the lock.

http://codereview.chromium.org/10834011/diff/12/base/metrics/statistics_recor...
base/metrics/statistics_recorder.cc:214: if (lock_ == NULL ||  ranges_ == NULL)
access ranges_ under lock

several times below

http://codereview.chromium.org/10834011/diff/12/base/metrics/statistics_recor...
base/metrics/statistics_recorder.cc:297: delete ranges;
This was moved... but could be cleaner with smart_ptr

http://codereview.chromium.org/10834011/diff/12/base/metrics/statistics_recor...
File base/metrics/statistics_recorder.h (right):

http://codereview.chromium.org/10834011/diff/12/base/metrics/statistics_recor...
base/metrics/statistics_recorder.h:5: // StatisticsRecorder holds all Histograms
and BucketRanges that used by
nit: "that used" --> "that are used"

http://codereview.chromium.org/10834011/diff/12/base/metrics/statistics_recor...
base/metrics/statistics_recorder.h:104: // lock protects access to above maps.
nit: "Lock"

http://codereview.chromium.org/10834011/diff/12/base/metrics/statistics_recor...
File base/metrics/statistics_recorder_unittest.cc (right):

http://codereview.chromium.org/10834011/diff/12/base/metrics/statistics_recor...
base/metrics/statistics_recorder_unittest.cc:58: static_cast<const
BucketRanges*>(ranges),
nit: you can push 58 back onto 57, and then align 59 with the open paren.  same
change below (unless we do hit 80 chars :-(  )

http://codereview.chromium.org/10834011/diff/12/base/metrics/statistics_recor...
base/metrics/statistics_recorder_unittest.cc:90: EXPECT_EQ(3u, ranges1->size());
Assert

http://codereview.chromium.org/10834011/diff/12/base/metrics/statistics_recor...
base/metrics/statistics_recorder_unittest.cc:99: static_cast<const
BucketRanges*>(ranges1),  // returning ranges1
nit: probably don't need cast to compare pointers.

http://codereview.chromium.org/10834011/diff/12/base/metrics/statistics_recor...
base/metrics/statistics_recorder_unittest.cc:200: histogram =
Histogram::FactoryGet(
Suggest using a separate variable, and verifying you get back the same value as
you had on line 169.

http://codereview.chromium.org/10834011/diff/12/base/metrics/statistics_recor...
base/metrics/statistics_recorder_unittest.cc:208: StatisticsRecorder::Histograms
registered_histograms;
Perhaps another comprare macro to a FactoryGet for same type.

[kaiwang, 2012-08-01 04:13:20]

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.cc
File base/metrics/histogram.cc (left):

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.cc#old...
base/metrics/histogram.cc:319: DCHECK_EQ(render_histogram->range_checksum(),
range_checksum);
On 2012/08/01 00:26:10, jar wrote:
> Perhaps this should be preserved, induce a return false; when we don't have a
> match.

Done.

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.cc
File base/metrics/histogram.cc (right):

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.cc#new...
base/metrics/histogram.cc:550: void Histogram::WriteAsciiBucketGraph(double
current_size, double max_size,
On 2012/08/01 00:26:10, jar wrote:
> nit: style fix here too (push arg to next line)

Done.

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.cc#new...
base/metrics/histogram.cc:761: (bucket_count - 2);
merged into second line

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.h
File base/metrics/histogram.h (left):

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.h#oldc...
base/metrics/histogram.h:644: // The dimension of ranges_ in bucket_ranges_ is
bucket_count + 1.
Done. Simplified the comments and added to Histogram constructor

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.h
File base/metrics/histogram.h (right):

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.h#newc...
base/metrics/histogram.h:13: // the minimal range is 1 (instead of 0), maximal
range is
On 2012/08/01 00:26:10, jar wrote:
> nit: suggest "minimal range" --> "minimum for a declared range"
> 
> same for maximal.

Done.

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.h#newc...
base/metrics/histogram.h:14: // (HistogramBase::kSampleType_MAX - 1). Currently
you can give ranges input
On 2012/08/01 00:26:10, jar wrote:
> nit: suggest "can give ranges input..." --> 
> 
> "can declare histograms with ranges exceeding those limits, but those excesses
> will be silently clamped to those limits (for backwards compatibility with
> existing code)."

Done.

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.h#newc...
base/metrics/histogram.h:17: // (HistogramBase::kSampleType_MAX - 1). But you
should never depend on this!
On 2012/08/01 00:26:10, jar wrote:
> nit: "But..." --> "Best practice is to not exceed the limits."

Done.

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.h#newc...
base/metrics/histogram.h:19: // For Histogram and LinearHistogram, maximal range
should always be larger (not
On 2012/08/01 00:26:10, jar wrote:
> nit: "maximal range" --> "maximum for a declared range"
> 
> same for minimum

Done.

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.h#newc...
base/metrics/histogram.h:20: // equal) than minmal range. And 0 and
HistogramBase::kSampleType_MAX are always
On 2012/08/01 00:26:10, jar wrote:
> nit: "And 0" --> "Zero"
> 
> "are always added" --> "are implicitly added"
> 

Done.

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.h#newc...
base/metrics/histogram.h:21: // added as first and last ranges, so the minimal
bucket count is 3. However
On 2012/08/01 00:26:10, jar wrote:
> nit: "minimal bucket count" --> "smallest legal bucket_count is 3."

Done.

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.h#newc...
base/metrics/histogram.h:24: // For these 3 kind histograms, the max bucket
count is always
On 2012/08/01 00:26:10, jar wrote:
> "kind" --> "kinds of"

Done.

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.h#newc...
base/metrics/histogram.h:27: // The buckets layout of Histogram is exponential.
For example, buckets might
On 2012/08/01 00:26:10, jar wrote:
> "Histogram" --> "class Histogram"

Done.

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.h#newc...
base/metrics/histogram.h:480: static std::string SerializeHistogramInfo(const
Histogram& histogram,
On 2012/08/01 00:26:10, jar wrote:
> note that grouping by "static" is not in style guide... so the old order was
> perchance based on utility.
> 
> Perhaps it should remain where it was (until you do a more major rewrite).

Done.

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.h#newc...
base/metrics/histogram.h:543: virtual ~Histogram();
On 2012/08/01 00:26:10, jar wrote:
> nit: move to near top of public section.
> 
> Perhaps this should stay as protected... and eventually be private when we
stop
> deriving from this class.
> 
> For now, the derived class destructors should probably be protected, or
private.
> 
> We want to strongly discourage deletion... and at most, only allow such in
test
> code (which may FRIEND to facilitate).

Done.

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.h#newc...
base/metrics/histogram.h:557: Histogram(const std::string& name,
On 2012/08/01 00:26:10, jar wrote:
> nit: push to top of section.

Done.

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.h#newc...
base/metrics/histogram.h:729: // |custom_ranges| contains a set of ranges. Each
of them should be > 0 and
On 2012/08/01 00:26:10, jar wrote:
> nit: "a set of ranges" --> "a vector of limits on ranges".
> 
> "Each of them" --> "Each limit"

Done.

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram.h#newc...
base/metrics/histogram.h:731: // compatibility). The ranges can be unordered or
contain duplication, but
On 2012/08/01 00:26:10, jar wrote:
> nit: "ranges" --> "limits"

Done.

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram_unitte...
File base/metrics/histogram_unittest.cc (right):

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram_unitte...
base/metrics/histogram_unittest.cc:125:
CustomHistogram::ArrayToCustomRanges(ranges, 3);
It's used in many files. Will change it in next CL (so I don't expand this CL
too much)

http://codereview.chromium.org/10834011/diff/12/base/metrics/histogram_unitte...
base/metrics/histogram_unittest.cc:180: // This is a buggy case: Histogram needs
to have at least 3 buckets (4
On 2012/08/01 00:26:10, jar wrote:
> Nit: Per your suggestion, replace 
> "This is buggy code" 
> with 
> 
> "This test exploits the fact that the sub class can have 2 buckets, while the
> base class is *supposed* to have at least 3 buckets.  We should probably
change
> the restriction on the base class (or not use the base class!)."

Done.

http://codereview.chromium.org/10834011/diff/12/base/metrics/statistics_recor...
File base/metrics/statistics_recorder.cc (right):

http://codereview.chromium.org/10834011/diff/12/base/metrics/statistics_recor...
base/metrics/statistics_recorder.cc:14: using std::list;
On 2012/08/01 00:26:10, jar wrote:
> nit: perhaps alphabetize

Done.

http://codereview.chromium.org/10834011/diff/12/base/metrics/statistics_recor...
base/metrics/statistics_recorder.cc:53: if (lock_ == NULL || histograms_ ==
NULL) {
On 2012/08/01 00:26:10, jar wrote:
> I'd prefer to have all accesses to histograms_ made with the lock held (as in
> original code, line 102).

Done.

http://codereview.chromium.org/10834011/diff/12/base/metrics/statistics_recor...
base/metrics/statistics_recorder.cc:68: delete histogram;
On 2012/08/01 00:26:10, jar wrote:
> Better would be to do the delete outside the lock.

Done.

http://codereview.chromium.org/10834011/diff/12/base/metrics/statistics_recor...
base/metrics/statistics_recorder.cc:79: if (lock_ == NULL || ranges_ == NULL) {
On 2012/08/01 00:26:10, jar wrote:
> Try to avoid access to ranges_ without the lock being held.

Done.

http://codereview.chromium.org/10834011/diff/12/base/metrics/statistics_recor...
base/metrics/statistics_recorder.cc:105: delete ranges;
On 2012/08/01 00:26:10, jar wrote:
> Use scoped_ptr to implicitly delete after we release the lock.

Done.

http://codereview.chromium.org/10834011/diff/12/base/metrics/statistics_recor...
base/metrics/statistics_recorder.cc:214: if (lock_ == NULL ||  ranges_ == NULL)
On 2012/08/01 00:26:10, jar wrote:
> access ranges_ under lock
> 
> several times below

Done.

http://codereview.chromium.org/10834011/diff/12/base/metrics/statistics_recor...
base/metrics/statistics_recorder.cc:297: delete ranges;
On 2012/08/01 00:26:10, jar wrote:
> This was moved... but could be cleaner with smart_ptr

Done.

http://codereview.chromium.org/10834011/diff/12/base/metrics/statistics_recor...
File base/metrics/statistics_recorder.h (right):

http://codereview.chromium.org/10834011/diff/12/base/metrics/statistics_recor...
base/metrics/statistics_recorder.h:5: // StatisticsRecorder holds all Histograms
and BucketRanges that used by
On 2012/08/01 00:26:10, jar wrote:
> nit: "that used" --> "that are used"

Done.

http://codereview.chromium.org/10834011/diff/12/base/metrics/statistics_recor...
base/metrics/statistics_recorder.h:104: // lock protects access to above maps.
On 2012/08/01 00:26:10, jar wrote:
> nit: "Lock"

Done.

http://codereview.chromium.org/10834011/diff/12/base/metrics/statistics_recor...
File base/metrics/statistics_recorder_unittest.cc (right):

http://codereview.chromium.org/10834011/diff/12/base/metrics/statistics_recor...
base/metrics/statistics_recorder_unittest.cc:58: static_cast<const
BucketRanges*>(ranges),
On 2012/08/01 00:26:10, jar wrote:
> nit: you can push 58 back onto 57, and then align 59 with the open paren. 
same
> change below (unless we do hit 80 chars :-(  )

Done.

http://codereview.chromium.org/10834011/diff/12/base/metrics/statistics_recor...
base/metrics/statistics_recorder_unittest.cc:90: EXPECT_EQ(3u, ranges1->size());
On 2012/08/01 00:26:10, jar wrote:
> Assert

Done.

http://codereview.chromium.org/10834011/diff/12/base/metrics/statistics_recor...
base/metrics/statistics_recorder_unittest.cc:99: static_cast<const
BucketRanges*>(ranges1),  // returning ranges1
On 2012/08/01 00:26:10, jar wrote:
> nit: probably don't need cast to compare pointers.

Done.

http://codereview.chromium.org/10834011/diff/12/base/metrics/statistics_recor...
base/metrics/statistics_recorder_unittest.cc:200: histogram =
Histogram::FactoryGet(
On 2012/08/01 00:26:10, jar wrote:
> Suggest using a separate variable, and verifying you get back the same value
as
> you had on line 169.

Done.

http://codereview.chromium.org/10834011/diff/12/base/metrics/statistics_recor...
base/metrics/statistics_recorder_unittest.cc:208: StatisticsRecorder::Histograms
registered_histograms;
On 2012/08/01 00:26:10, jar wrote:
> Perhaps another comprare macro to a FactoryGet for same type.

Done.

[jar (doing other things), 2012-08-01 16:41:25]

With changes listed below, LGTM.

http://codereview.chromium.org/10834011/diff/1027/base/metrics/statistics_rec...
File base/metrics/statistics_recorder.cc (right):

http://codereview.chromium.org/10834011/diff/1027/base/metrics/statistics_rec...
base/metrics/statistics_recorder.cc:97: scoped_ptr<const BucketRanges>
ranges_deleter(ranges);
IMO, you should wait till the last moment to push the value |ranges| into this
ptr. You should do it at the point where you decide to do the delete, but want
it deferred until the lock is released.  Initializing it now complicates the
code in lines 101, and 122.

IMO, you should leave this scoped_ptr initialized to NULL, and then set its
value only after line 123.

http://codereview.chromium.org/10834011/diff/1027/base/metrics/statistics_rec...
base/metrics/statistics_recorder.cc:310: scoped_ptr<RangesMap>
ranges_deleter(ranges_);
For both of these, you should wait until you are inside the lock (lines 323) to
snatch the values and put it into the scoped_ptr instances.

[kaiwang, 2012-08-01 19:48:18]

http://codereview.chromium.org/10834011/diff/1027/base/metrics/statistics_rec...
File base/metrics/statistics_recorder.cc (right):

http://codereview.chromium.org/10834011/diff/1027/base/metrics/statistics_rec...
base/metrics/statistics_recorder.cc:97: scoped_ptr<const BucketRanges>
ranges_deleter(ranges);
On 2012/08/01 16:41:25, jar wrote:
> IMO, you should wait till the last moment to push the value |ranges| into this
> ptr. You should do it at the point where you decide to do the delete, but want
> it deferred until the lock is released.  Initializing it now complicates the
> code in lines 101, and 122.
> 
> IMO, you should leave this scoped_ptr initialized to NULL, and then set its
> value only after line 123.

Done.

http://codereview.chromium.org/10834011/diff/1027/base/metrics/statistics_rec...
base/metrics/statistics_recorder.cc:310: scoped_ptr<RangesMap>
ranges_deleter(ranges_);
On 2012/08/01 16:41:25, jar wrote:
> For both of these, you should wait until you are inside the lock (lines 323)
to
> snatch the values and put it into the scoped_ptr instances.

Done.

[commit-bot: I haz the power, 2012-08-01 19:50:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaiwang@chromium.org/10834011/2014

[commit-bot: I haz the power, 2012-08-01 21:34:09]

Change committed as 149495

[jar (doing other things), 2012-08-01 22:56:17]

lgtm

[Lei Zhang, 2012-08-02 04:20:49]

https://chromiumcodereview.appspot.com/10834011/diff/2014/base/metrics/histog...
File base/metrics/histogram.cc (right):

https://chromiumcodereview.appspot.com/10834011/diff/2014/base/metrics/histog...
base/metrics/histogram.cc:461: DLOG(WARNING) << "Histogram: " << name << " Bad
minimum: " << *minimum;
Does this have to be DLOG? Can you use a DVLOG instead? My debug build suddenly
got very spammy. :(

[Lei Zhang, 2012-08-02 04:22:52]

https://chromiumcodereview.appspot.com/10834011/diff/2014/base/metrics/histog...
File base/metrics/histogram.cc (right):

https://chromiumcodereview.appspot.com/10834011/diff/2014/base/metrics/histog...
base/metrics/histogram.cc:461: DLOG(WARNING) << "Histogram: " << name << " Bad
minimum: " << *minimum;
On 2012/08/02 04:20:50, Lei Zhang wrote:
> Does this have to be DLOG? Can you use a DVLOG instead? My debug build
suddenly
> got very spammy. :(

Already fixed in r149541. Yay! :)