SampleSet -> HistogramSamples (will be reused by SparseHistogram)

SampleSet -> HistogramSamples which can be reused by SparseHistogram

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

[Ilya Sherman, 2012-08-23 07:50:54]

This CL is simultaneously large and non-trivial, which makes it hard to review. 
Can you split it up into multiple smaller CLs?

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
File base/metrics/histogram.cc (left):

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.cc:394: // Validate a sample and related histogram.
nit: Why did you remove this comment?

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.cc:572: // Note locking not done in this version!!!
nit: Please keep this comment around by the relevant new code.

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
File base/metrics/histogram.cc (right):

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.cc:38: CHECK_GE(bucket_ranges_->size(), 2u);
nit: Does this need to be a CHECK, or would a DCHECK suffice?

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.cc:43: size_t i = BucketIndex(value, bucket_ranges_);
nit: i -> bucket (prefer to use descriptive names)

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.cc:45: sum_ += count * value;
Any need to check for overflow?

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.cc:94: } while (true);
Can we use a standard binary search implementation, or is it important to write
our own here, e.g. to get rounding to match what the previous algorithm did or
something like that?  If we need to use our own implementation, please include a
comment documenting the reason for this.

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.cc:105: while (index < counts_.size() && !iter->Done()) {
Please add a comment describing what this double-iteration is doing.

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.cc:110: counts_[index] += is_add ? count : - count;
nit: The unary minus operator should not be followed by a space.

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.cc:145: if (min != NULL)
nit: I saw nothing in the method description that indicated that these could be
NULL.  If this is an intended use case, please document that in the relevant
header...

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.cc:429: histogram.SerializeRanges(&pickle);
Why did you move this line?

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.cc:517: 
nit: No need for this blank line.

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.cc:522: const int
Histogram::kCommonRaceBasedCountMismatch = 5;
Why did you move this constant into the Histogram class, rather than keeping it
local to the implementation file?

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.cc:595: }
nit: No need for curly braces

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
File base/metrics/histogram.h (right):

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.h:349: class BASE_EXPORT_PRIVATE BucketHistogramSamples :
public HistogramSamples {
I don't really understand what a "BucketHistogram" is -- don't all histograms
have buckets?

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.h:351: BucketHistogramSamples(const BucketRanges*
bucket_ranges);
nit: explicit

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.h:351: BucketHistogramSamples(const BucketRanges*
bucket_ranges);
nit: Can this be passed by const-ref rather than const-pointer?

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.h:354: virtual void Accumulate(HistogramBase::Sample
value,
nit: Please label these methods with something like "// HistogramSamples
implementation."

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.h:363: HistogramBase::Count
GetCountFromBucketIndex(size_t bucket_index) const;
nit: "From" -> "For"

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.h:369: static size_t BucketIndex(HistogramBase::Sample
value,
nit: GetBucketIndex()

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.h:370: const BucketRanges* bucket_ranges);
nit: Can this be passed by const-reference?

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.h:373: DISALLOW_COPY_AND_ASSIGN(BucketHistogramSamples);
Optional nit: I think this is usually the very last line in a class.

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.h:379: const BucketRanges* bucket_ranges_;
nit: Can this be a const reference or a const BucketRanges* const?

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.h:386: const std::vector<HistogramBase::Count>* counts,
nit: Can this be passed by const-reference?

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.h:387: const BucketRanges* bucket_ranges);
nit: Ditto

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.h:395: void ForwardIndex();  // Find the next place with
data.
nit: Please choose a more descriptive name rather than relying on the comment.

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.h:402: };
Please declare & define these classes in separate files.

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
File base/metrics/histogram_samples.cc (right):

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram_samples.cc:36: : iter_(iter), is_done_(false) {
nit: Initializations should be one per line:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constructor_In...

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram_samples.cc:39: }
nit: No need for curly braces

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram_samples.cc:39: }
nit: Why not just call Next()?

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram_samples.cc:47: CHECK(!Done());
Does this really need to be a CHECK rather than a DCHECK?

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram_samples.cc:56: CHECK(!Done());
Ditto

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram_samples.cc:68: return true;
nit: This can be written as

return iter_->ReadInt(&min_) && iter_->ReadInt(&max) && iter_->ReadInt(&count_);

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram_samples.cc:74: : sum_(0), redundant_count_(0) {}
nit: One per line

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram_samples.cc:81:
CHECK(AddSubtractHelper(other.Iterator().get(), true));
This looks like it will leak memory.

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram_samples.cc:81:
CHECK(AddSubtractHelper(other.Iterator().get(), true));
nit: Does this need to be a CHECK, or would a DCHECK suffice?  (Ditto below.)

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram_samples.cc:94: new SampleCountPickleIterator(iter));
nit: Why not stack-allocate this?

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
File base/metrics/histogram_samples.h (right):

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram_samples.h:5: // HistogramSamples is a container storing
all samples of a histogram.
Optional nit: Perhaps move this comment closer to the class declaration?

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram_samples.h:10: #include <utility>
nit: What is this used for?

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram_samples.h:34: virtual bool Add(PickleIterator* iter);  //
Add from serialized samples.
Why is this method needed, rather than first reading a HistogramSamples from a
Pickle, and then using the regular Add method?  Function overloading is
generally discouraged in Chromium (though it looks like the style guide has been
updated to allow it in some cases).

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram_samples.h:37: virtual scoped_ptr<SampleCountIterator>
Iterator() const = 0;
Optional nit: Perhaps CreateIterator()?

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram_samples.h:46: virtual bool
AddSubtractHelper(SampleCountIterator* iter, bool is_add) = 0;
nit: Please give this a more descriptive name.  I cannot tell what this method
is meant to do, but it's also pure-virtual, so I really have no good way to find
out.

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram_samples.h:46: virtual bool
AddSubtractHelper(SampleCountIterator* iter, bool is_add) = 0;
nit: Interfaces like this should take an enumerated value rather than a boolean
as a parameter, so that the meaning of the parameter is clear from the call
site.

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram_samples.h:48: int64 sum_;
nit: Members should be private:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Access_Control
(Ditto below)

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram_samples.h:51: // the number of samples we've accumulated.
We can compare this count to the
nit: Split the first sentence as:

|redundant_count| helps identify memory corruption.  It redundantly stores the
total number of samples accumulated in the histogram.

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram_samples.h:64: virtual bool Done() = 0;
nit: Perhaps IsDone() or IsAtEnd()?

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram_samples.h:65: virtual void Next() = 0;
nit: Perhaps Advance()?

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
File base/metrics/histogram_snapshot_manager.cc (left):

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram_snapshot_manager.cc:78: // snapshot anything that we've
already recorded.
nit: Please preserve (or improve upon) this comment.

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram_snapshot_manager.cc:101: // Deduct any stats we've
already logged from our snapshot.
nit: Please preserve (or improve upon) this comment.

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
File base/metrics/histogram_snapshot_manager.cc (right):

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram_snapshot_manager.cc:12: using std::string;
nit: Please just use the full std:: qualified names in the code -- that seems
more consistent with the usual Chrome coding style.

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram_snapshot_manager.cc:65: DLOG(ERROR) << "Histogram: " <<
histogram_name
nit: Should this be a DLOG(FATAL)?

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram_snapshot_manager.cc:85: // This histogram is not logged
before, add a new entry.
nit: "is not logged before" -> "has not been logged before"

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram_snapshot_manager.cc:100: }
I can't immediately tell if you're changing the behavior of this implementation,
or simply trying to clean it up.  Is it possible to maintain a more parallel
structure within this CL, and split the cleanup/changes out into a separate,
follow-up CL?

https://chromiumcodereview.appspot.com/10829466/diff/15003/chrome/common/metr...
File chrome/common/metrics/metrics_log_base.cc (left):

https://chromiumcodereview.appspot.com/10829466/diff/15003/chrome/common/metr...
chrome/common/metrics/metrics_log_base.cc:455:
DCHECK_EQ(histogram.bucket_count(), snapshot.size());
Why is this DCHECK no longer appropriate?

https://chromiumcodereview.appspot.com/10829466/diff/15003/chrome/common/metr...
File chrome/common/metrics/metrics_log_base.cc (right):

https://chromiumcodereview.appspot.com/10829466/diff/15003/chrome/common/metr...
chrome/common/metrics/metrics_log_base.cc:480: HistogramBase::Count count;
nit: These should be probably be scoped within the for loop.

https://chromiumcodereview.appspot.com/10829466/diff/15003/chrome/common/metr...
chrome/common/metrics/metrics_log_base.cc:503: // TODO(kaiwang): do we need
bucket_index field?
Jim, are we realistically ever going to use this field, or is it ok to drop?

[kaiwang, 2012-08-24 04:17:57]

Thanks for your review!

Sorry it's quite big. But you can treat it as 2 CLs.
The major one is just these 6 files, that changed from SampleSet to
HistogramSamples:
base/base.gypi
base/metrics/histogram.h
base/metrics/histogram.cc
base/metrics/histogram_samples.h
base/metrics/histogram_samples.cc
base/metrics/histogram_unittest.cc

All the rest are small changes using new class/functions

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

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram.cc#...
base/metrics/histogram.cc:394: // Validate a sample and related histogram.
better function comment in .h file

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram.cc#...
base/metrics/histogram.cc:572: // Note locking not done in this version!!!
this function is not needed any more. All Histograms are never locked (this
behavior is also mentioned in other comments).

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

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram.cc#...
base/metrics/histogram.cc:38: CHECK_GE(bucket_ranges_->size(), 2u);
This check has litter performance impact, I don't see a reason to change to
DCHECK.
If bucket ranges size is smaller than 2, there will be some crash later if the
execution by pass this check, which will be much harder to find out.

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram.cc#...
base/metrics/histogram.cc:43: size_t i = BucketIndex(value, bucket_ranges_);
i -> bucket_index

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram.cc#...
base/metrics/histogram.cc:45: sum_ += count * value;
sum is int64 and count/value are both int32.
Also, even if it's a overflow, we can not do much here

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram.cc#...
base/metrics/histogram.cc:94: } while (true);
This code is copied from below.
I think for refactor code, it's best not to change function implementation

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram.cc#...
base/metrics/histogram.cc:105: while (index < counts_.size() && !iter->Done()) {
On 2012/08/23 07:50:54, Ilya Sherman wrote:
> Please add a comment describing what this double-iteration is doing.

Done.

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram.cc#...
base/metrics/histogram.cc:110: counts_[index] += is_add ? count : - count;
On 2012/08/23 07:50:54, Ilya Sherman wrote:
> nit: The unary minus operator should not be followed by a space.

Done.

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram.cc#...
base/metrics/histogram.cc:145: if (min != NULL)
added to interface comment

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram.cc#...
base/metrics/histogram.cc:429: histogram.SerializeRanges(&pickle);
this is part of "histogram info", while the code below is only related to
samples.
Moving this line makes deserialize easier.

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram.cc#...
base/metrics/histogram.cc:517: 
On 2012/08/23 07:50:54, Ilya Sherman wrote:
> nit: No need for this blank line.

Done.

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram.cc#...
base/metrics/histogram.cc:522: const int
Histogram::kCommonRaceBasedCountMismatch = 5;
It's needed in histogram_snapshot_manager.cc
The old implementation of histogram_snapshot_manager.cc had problems.

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram.cc#...
base/metrics/histogram.cc:595: }
On 2012/08/23 07:50:54, Ilya Sherman wrote:
> nit: No need for curly braces

Done.

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

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram.h#n...
base/metrics/histogram.h:349: class BASE_EXPORT_PRIVATE BucketHistogramSamples :
public HistogramSamples {
SparseHistograms don't

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram.h#n...
base/metrics/histogram.h:351: BucketHistogramSamples(const BucketRanges*
bucket_ranges);
On 2012/08/23 07:50:54, Ilya Sherman wrote:
> nit: explicit

Done.

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram.h#n...
base/metrics/histogram.h:354: virtual void Accumulate(HistogramBase::Sample
value,
On 2012/08/23 07:50:54, Ilya Sherman wrote:
> nit: Please label these methods with something like "// HistogramSamples
> implementation."

Done.

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram.h#n...
base/metrics/histogram.h:363: HistogramBase::Count
GetCountFromBucketIndex(size_t bucket_index) const;
I really don't think "For" is better than "From", but if you like, I can change
it to "GetCountWithBucketIndex", which may sound better to me.

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram.h#n...
base/metrics/histogram.h:369: static size_t BucketIndex(HistogramBase::Sample
value,
On 2012/08/23 07:50:54, Ilya Sherman wrote:
> nit: GetBucketIndex()

Done.

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram.h#n...
base/metrics/histogram.h:373: DISALLOW_COPY_AND_ASSIGN(BucketHistogramSamples);
On 2012/08/23 07:50:54, Ilya Sherman wrote:
> Optional nit: I think this is usually the very last line in a class.

Done.

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram.h#n...
base/metrics/histogram.h:379: const BucketRanges* bucket_ranges_;
On 2012/08/23 07:50:54, Ilya Sherman wrote:
> nit: Can this be a const reference or a const BucketRanges* const?

Done.

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram.h#n...
base/metrics/histogram.h:387: const BucketRanges* bucket_ranges);
For all the reference related comments:
IMHO, I rarely see reference class members in google code base. I feel it's
better to use pointers here, because the objects are owned by others and stored
in other class as pointers (so the pointer here is weak). But in C++, reference
is just a "name", and does not have the "weak" semantic. So in this case it may
cause confusions.
Passing in const pointers are perfectly valid according to style guide.

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram.h#n...
base/metrics/histogram.h:395: void ForwardIndex();  // Find the next place with
data.
On 2012/08/23 07:50:54, Ilya Sherman wrote:
> nit: Please choose a more descriptive name rather than relying on the comment.

Done.

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram.h#n...
base/metrics/histogram.h:402: };
On 2012/08/23 07:50:54, Ilya Sherman wrote:
> Please declare & define these classes in separate files.

This file currently contains all stuff related to Histogram (5 classes and
macros). Only moving samples related code out does not help to clarify.

I agree the best way is to move every class to different file. But it needs a
lot of code change. I'd prefer to do it in seperate CLs

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram_sam...
File base/metrics/histogram_samples.cc (right):

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram_sam...
base/metrics/histogram_samples.cc:36: : iter_(iter), is_done_(false) {
actually it only says subsequent lines should indent 4 spaces, but not
everything should be on same line...

Anyway, fixed with your suggestion

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram_sam...
base/metrics/histogram_samples.cc:39: }
On 2012/08/23 07:50:54, Ilya Sherman wrote:
> nit: No need for curly braces

Done.

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram_sam...
base/metrics/histogram_samples.cc:47: CHECK(!Done());
Using DCHECK asks code owner to consider 2 situations: in release and in debug,
this complicates the code. And style guide does not suggest to "gracefully
handle DCHECK failure".

I think the only reason to use DCHECK is to prevent impacting performance

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram_sam...
base/metrics/histogram_samples.cc:68: return true;
On 2012/08/23 07:50:54, Ilya Sherman wrote:
> nit: This can be written as
> 
> return iter_->ReadInt(&min_) && iter_->ReadInt(&max) &&
iter_->ReadInt(&count_);

Done.

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram_sam...
base/metrics/histogram_samples.cc:74: : sum_(0), redundant_count_(0) {}
moved to a single line

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram_sam...
base/metrics/histogram_samples.cc:81:
CHECK(AddSubtractHelper(other.Iterator().get(), true));
You mean iterator? It's in a scoped_ptr

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram_sam...
base/metrics/histogram_samples.cc:94: new SampleCountPickleIterator(iter));
On 2012/08/23 07:50:54, Ilya Sherman wrote:
> nit: Why not stack-allocate this?

Done.

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram_sam...
File base/metrics/histogram_samples.h (right):

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram_sam...
base/metrics/histogram_samples.h:5: // HistogramSamples is a container storing
all samples of a histogram.
On 2012/08/23 07:50:54, Ilya Sherman wrote:
> Optional nit: Perhaps move this comment closer to the class declaration?

Done.

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram_sam...
base/metrics/histogram_samples.h:10: #include <utility>
On 2012/08/23 07:50:54, Ilya Sherman wrote:
> nit: What is this used for?

Done.

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram_sam...
base/metrics/histogram_samples.h:34: virtual bool Add(PickleIterator* iter);  //
Add from serialized samples.
Changed to "AddFromPickle"

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram_sam...
base/metrics/histogram_samples.h:46: virtual bool
AddSubtractHelper(SampleCountIterator* iter, bool is_add) = 0;
Changed to AddSubtractImpl

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram_sam...
base/metrics/histogram_samples.h:48: int64 sum_;
On 2012/08/23 07:50:54, Ilya Sherman wrote:
> nit: Members should be private:
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Access_Control
> (Ditto below)

Done.

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram_sam...
base/metrics/histogram_samples.h:51: // the number of samples we've accumulated.
We can compare this count to the
On 2012/08/23 07:50:54, Ilya Sherman wrote:
> nit: Split the first sentence as:
> 
> |redundant_count| helps identify memory corruption.  It redundantly stores the
> total number of samples accumulated in the histogram.

Done.

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram_sam...
base/metrics/histogram_samples.h:65: virtual void Next() = 0;
On 2012/08/23 07:50:54, Ilya Sherman wrote:
> nit: Perhaps Advance()?

Done.

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram_sam...
base/metrics/histogram_samples.h:65: virtual void Next() = 0;
These 2 names are very conventional in google code:

https://cs.corp.google.com/#search/&sq=package:google3%20-file:%5Eexperimenta...

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram_sna...
File base/metrics/histogram_snapshot_manager.cc (right):

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram_sna...
base/metrics/histogram_snapshot_manager.cc:12: using std::string;
I really don't think so. You can find a lot of "using ..." in Chromium code.
Every C++ programmer should be familiar with map, string, vector, etc. And
people should not define classes with the same name.
Keeping all the "std::" only makes lines longer and hard to read.
It's only in .cc file and has a limited scoped.

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram_sna...
base/metrics/histogram_snapshot_manager.cc:65: DLOG(ERROR) << "Histogram: " <<
histogram_name
DLOG(FATAL) is like DCHECK.
We generally should not handle DCHECKs. See:
http://www.chromium.org/developers/coding-style#TOC-CHECK-vs-DCHECK-and-NOTRE...

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram_sna...
base/metrics/histogram_snapshot_manager.cc:85: // This histogram is not logged
before, add a new entry.
On 2012/08/23 07:50:54, Ilya Sherman wrote:
> nit: "is not logged before" -> "has not been logged before"

Done.

http://codereview.chromium.org/10829466/diff/15003/base/metrics/histogram_sna...
base/metrics/histogram_snapshot_manager.cc:100: }
I didn't change the logic. The problem is because the signature change of
SnapshotSample(s), I have to rewrite the above function. Then it doesn't make
sense I keep the original bad style.

http://codereview.chromium.org/10829466/diff/15003/chrome/common/metrics/metr...
File chrome/common/metrics/metrics_log_base.cc (left):

http://codereview.chromium.org/10829466/diff/15003/chrome/common/metrics/metr...
chrome/common/metrics/metrics_log_base.cc:455:
DCHECK_EQ(histogram.bucket_count(), snapshot.size());
There's no "size" anymore. It's just some duplicated information and does not
work with sparse histogram.

http://codereview.chromium.org/10829466/diff/15003/chrome/common/metrics/metr...
File chrome/common/metrics/metrics_log_base.cc (right):

http://codereview.chromium.org/10829466/diff/15003/chrome/common/metrics/metr...
chrome/common/metrics/metrics_log_base.cc:480: HistogramBase::Count count;
On 2012/08/23 07:50:54, Ilya Sherman wrote:
> nit: These should be probably be scoped within the for loop.

Done.

[Ilya Sherman, 2012-08-29 08:48:15]

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
File base/metrics/histogram.cc (right):

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.cc:38: CHECK_GE(bucket_ranges_->size(), 2u);
On 2012/08/24 04:17:58, kaiwang wrote:
> This check has litter performance impact, I don't see a reason to change to
> DCHECK.
> If bucket ranges size is smaller than 2, there will be some crash later if the
> execution by pass this check, which will be much harder to find out.

That is an argument for only ever using CHECKs, and never using DCHECKs. 
http://www.chromium.org/developers/coding-style#TOC-CHECK-vs-DCHECK-and-NOTRE...
lists the cases when CHECK is appropriate -- this doesn't seem to me to be one
of them.

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.cc:45: sum_ += count * value;
On 2012/08/24 04:17:58, kaiwang wrote:
> sum is int64 and count/value are both int32.
> Also, even if it's a overflow, we can not do much here

Why isn't clamping to the max value for int64 the appropriate way to handle
overflow?

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
File base/metrics/histogram.h (right):

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.h:349: class BASE_EXPORT_PRIVATE BucketHistogramSamples :
public HistogramSamples {
On 2012/08/24 04:17:58, kaiwang wrote:
> SparseHistograms don't

SparseHistograms don't have *contiguous* buckets, but surely they still have
buckets.  A histogram bucket is essentially a key/value pair.  Am I missing
something?

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.h:363: HistogramBase::Count
GetCountFromBucketIndex(size_t bucket_index) const;
On 2012/08/24 04:17:58, kaiwang wrote:
> I really don't think "For" is better than "From", but if you like, I can
change
> it to "GetCountWithBucketIndex", which may sound better to me.

If you want to use "from", the name should be "GetCountFromBucketWithIndex()" --
the bucket *index* does not have a count.  The count also does not have a bucket
index, so the alternate name you suggested doesn't quite work either.

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.h:402: };
On 2012/08/24 04:17:58, kaiwang wrote:
> On 2012/08/23 07:50:54, Ilya Sherman wrote:
> > Please declare & define these classes in separate files.
> 
> This file currently contains all stuff related to Histogram (5 classes and
> macros). Only moving samples related code out does not help to clarify.
> 
> I agree the best way is to move every class to different file. But it needs a
> lot of code change. I'd prefer to do it in seperate CLs

You are adding a new class, so moving the new class out does not add extra code.
 Please move this code out in this CL.  I agree that splitting up this header
further does not make sense as part of this CL.

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
File base/metrics/histogram_samples.cc (right):

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram_samples.cc:39: }
On 2012/08/23 07:50:54, Ilya Sherman wrote:
> nit: Why not just call Next()?

(bump)

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram_samples.cc:47: CHECK(!Done());
On 2012/08/24 04:17:58, kaiwang wrote:
> Using DCHECK asks code owner to consider 2 situations: in release and in
debug,
> this complicates the code. And style guide does not suggest to "gracefully
> handle DCHECK failure".
> 
> I think the only reason to use DCHECK is to prevent impacting performance

The Chromium default is to use DCHECK rather than CHECK, both to avoid impacting
performance and to avoid impacting the binary size.  CHECK is only used when
DCHECK does not suffice for some reason, e.g. when you are trying to track down
crashes that seem to be violating your assertion.

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram_samples.cc:81:
CHECK(AddSubtractHelper(other.Iterator().get(), true));
On 2012/08/24 04:17:58, kaiwang wrote:
> You mean iterator? It's in a scoped_ptr

Sorry, you're right, this should be fine.

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
File base/metrics/histogram_samples.h (right):

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram_samples.h:46: virtual bool
AddSubtractHelper(SampleCountIterator* iter, bool is_add) = 0;
On 2012/08/23 07:50:54, Ilya Sherman wrote:
> nit: Interfaces like this should take an enumerated value rather than a
boolean
> as a parameter, so that the meaning of the parameter is clear from the call
> site.

(bump)

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
File base/metrics/histogram.cc (left):

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
base/metrics/histogram.cc:454: // Note locking not done in this version!!!
nit: Any reason not to preserve this comment?

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
base/metrics/histogram.cc:532: DCHECK_GE(over, under);
Why did you remove this DCHECK?

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
base/metrics/histogram.cc:543: CHECK_GT(ranges(mid+1), value);
Why did you remove these checks?

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
File base/metrics/histogram.cc (right):

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
base/metrics/histogram.cc:46: set_redundant_count(redundant_count() + count);
Optional nit: Perhaps the HistogramSamples() class should instead expose methods
IncrementSum() and IncrementRedundantCount()?

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
base/metrics/histogram.cc:63: size_t bucket_index) const {
nit: DCHECK that the index is in range?

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
base/metrics/histogram.cc:104: // Go through the iterator and add the counts
into correct bucket.
The loop has three cases.  Please describe each of these, either within the
while loop or as part of this comment.

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
base/metrics/histogram.cc:128: index_(-1),
nit: It looks like the code would be slightly cleaner if |index_| were a size_t
and started at 0.

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
base/metrics/histogram.cc:420: return render_histogram->AddSamples(&iter);
Should failing to deserialize the Pickle trigger a DCHECK?

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
File base/metrics/histogram.h (right):

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
base/metrics/histogram.h:354: // Implements HistogramSamples:
nit: This is typically written as "HistogramSamples implementation:" or just
"HistogramSamples:"

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
base/metrics/histogram.h:370: const BucketRanges* bucket_ranges);
nit: Why is this a static function rather than an instance method?  It looks
like you always pass |bucket_ranges_| as the second parameter.

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
base/metrics/histogram.h:370: const BucketRanges* bucket_ranges);
Can this be private?

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
base/metrics/histogram.h:485: bool AddSamples(PickleIterator* iter);
Optional nit: Perhaps |AddSamplesFromPickle()|?

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
File base/metrics/histogram_samples.cc (right):

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
base/metrics/histogram_samples.cc:27: PickleIterator* iter_;
nit: PickleIterator* const iter_;?

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
File base/metrics/histogram_samples.h (right):

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
base/metrics/histogram_samples.h:70: virtual bool Done() = 0;
nit: const?

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
base/metrics/histogram_samples.h:74: // |min| |max| and |count| can be NULL if
the value is not interested.
nit: "interested" -> "of interest"

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
base/metrics/histogram_samples.h:77: HistogramBase::Count* count) = 0;
nit: const?

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
File base/metrics/histogram_snapshot_manager.cc (left):

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
base/metrics/histogram_snapshot_manager.cc:91: NOTREACHED();  // Already_logged
has become corrupt.
Why did you remove this NOTREACHED()?

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
File base/metrics/histogram_snapshot_manager.cc (right):

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
base/metrics/histogram_snapshot_manager.cc:86: already_logged =
snapshot.release();
nit: It seems clearer to just assign to |to_log| here, and move the
|already_logged| variable into the else-stmt

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
base/metrics/histogram_snapshot_manager.cc:99: }
nit: No need for curly braces.

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
base/metrics/histogram_snapshot_manager.cc:107: if (discrepancy) {
Optional nit: You can reduce the level of indentation below by reversing this
condition and returning early.

[kaiwang, 2012-08-29 22:42:16]

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

http://codereview.chromium.org/10829466/diff/31007/base/metrics/histogram.cc#...
base/metrics/histogram.cc:454: // Note locking not done in this version!!!
Lock is never used in Histograms and there's no plan to add it.
I think it's better to comment about what we have instead of what we don't have.
Actually when I first read this comment I thought locking is done in some
sub-classes. It's confusing.

Probably the better way to comment this is to add a "thread safety" section into
class description in header file.

http://codereview.chromium.org/10829466/diff/31007/base/metrics/histogram.cc#...
base/metrics/histogram.cc:543: CHECK_GT(ranges(mid+1), value);
For above checks, they are guaranteed by current code logic + DCHECKs. So they
are like testing the code logic, which should be in unittests instead.

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

http://codereview.chromium.org/10829466/diff/31007/base/metrics/histogram.cc#...
base/metrics/histogram.cc:46: set_redundant_count(redundant_count() + count);
Changed IncreaseSum and IncreaseRedundantCount

http://codereview.chromium.org/10829466/diff/31007/base/metrics/histogram.cc#...
base/metrics/histogram.cc:63: size_t bucket_index) const {
On 2012/08/29 08:48:16, Ilya Sherman wrote:
> nit: DCHECK that the index is in range?

Done.

http://codereview.chromium.org/10829466/diff/31007/base/metrics/histogram.cc#...
base/metrics/histogram.cc:104: // Go through the iterator and add the counts
into correct bucket.
On 2012/08/29 08:48:16, Ilya Sherman wrote:
> The loop has three cases.  Please describe each of these, either within the
> while loop or as part of this comment.

Done.

http://codereview.chromium.org/10829466/diff/31007/base/metrics/histogram.cc#...
base/metrics/histogram.cc:128: index_(-1),
Need a way to tell it's on first bucket (index == 0) or it has no data at all
(index == -1)

http://codereview.chromium.org/10829466/diff/31007/base/metrics/histogram.cc#...
base/metrics/histogram.cc:420: return render_histogram->AddSamples(&iter);
We don't want to crash browser when some (corrupted) renderer gives some bad
histogram.
Returning false means just ignore these bad histograms, which is expected
behavior

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

http://codereview.chromium.org/10829466/diff/31007/base/metrics/histogram.h#n...
base/metrics/histogram.h:354: // Implements HistogramSamples:
On 2012/08/29 08:48:16, Ilya Sherman wrote:
> nit: This is typically written as "HistogramSamples implementation:" or just
> "HistogramSamples:"

Done.

http://codereview.chromium.org/10829466/diff/31007/base/metrics/histogram.h#n...
base/metrics/histogram.h:370: const BucketRanges* bucket_ranges);
Will change it to private.

For static/non-static, there's a coding best practice(inside and outside of
google) to define private functions that does not change object state all
static. Unfortunately I can not find the internal pages at hand.

The good thing about static is, it does not depends on the whole object (no this
pointer). So it's much easier for code reader to understand what is going on if
you pass the
members that affect this method. So basically static function has less
dependency then non-static functions.

Another reason is, making private functions static does not help encapsulation,
because external client can not call these functions. While many public
functions has to be non-static, because external client need them to change
object state.

I do find some design docs of another project mentioning this. It's the maps
frontend testing design doc:
https://www.corp.google.com/eng/designdocs/maps/testing.html#s13

http://codereview.chromium.org/10829466/diff/31007/base/metrics/histogram.h#n...
base/metrics/histogram.h:485: bool AddSamples(PickleIterator* iter);
On 2012/08/29 08:48:16, Ilya Sherman wrote:
> Optional nit: Perhaps |AddSamplesFromPickle()|?

Done.

http://codereview.chromium.org/10829466/diff/31007/base/metrics/histogram_sam...
File base/metrics/histogram_samples.cc (right):

http://codereview.chromium.org/10829466/diff/31007/base/metrics/histogram_sam...
base/metrics/histogram_samples.cc:27: PickleIterator* iter_;
On 2012/08/29 08:48:16, Ilya Sherman wrote:
> nit: PickleIterator* const iter_;?

Done.

http://codereview.chromium.org/10829466/diff/31007/base/metrics/histogram_sam...
File base/metrics/histogram_samples.h (right):

http://codereview.chromium.org/10829466/diff/31007/base/metrics/histogram_sam...
base/metrics/histogram_samples.h:70: virtual bool Done() = 0;
On 2012/08/29 08:48:16, Ilya Sherman wrote:
> nit: const?

Done.

http://codereview.chromium.org/10829466/diff/31007/base/metrics/histogram_sam...
base/metrics/histogram_samples.h:74: // |min| |max| and |count| can be NULL if
the value is not interested.
On 2012/08/29 08:48:16, Ilya Sherman wrote:
> nit: "interested" -> "of interest"

Done.

http://codereview.chromium.org/10829466/diff/31007/base/metrics/histogram_sam...
base/metrics/histogram_samples.h:77: HistogramBase::Count* count) = 0;
On 2012/08/29 08:48:16, Ilya Sherman wrote:
> nit: const?

Done.

http://codereview.chromium.org/10829466/diff/31007/base/metrics/histogram_sna...
File base/metrics/histogram_snapshot_manager.cc (left):

http://codereview.chromium.org/10829466/diff/31007/base/metrics/histogram_sna...
base/metrics/histogram_snapshot_manager.cc:91: NOTREACHED();  // Already_logged
has become corrupt.
It's trying to handle DCHECK error, which is not recommended by style guide. And
this "NOTREACHED" situation is always reached in Chrome release

http://codereview.chromium.org/10829466/diff/31007/base/metrics/histogram_sna...
File base/metrics/histogram_snapshot_manager.cc (right):

http://codereview.chromium.org/10829466/diff/31007/base/metrics/histogram_sna...
base/metrics/histogram_snapshot_manager.cc:86: already_logged =
snapshot.release();
On 2012/08/29 08:48:16, Ilya Sherman wrote:
> nit: It seems clearer to just assign to |to_log| here, and move the
> |already_logged| variable into the else-stmt

Done.

http://codereview.chromium.org/10829466/diff/31007/base/metrics/histogram_sna...
base/metrics/histogram_snapshot_manager.cc:99: }
On 2012/08/29 08:48:16, Ilya Sherman wrote:
> nit: No need for curly braces.

Done.

http://codereview.chromium.org/10829466/diff/31007/base/metrics/histogram_sna...
base/metrics/histogram_snapshot_manager.cc:107: if (discrepancy) {
On 2012/08/29 08:48:16, Ilya Sherman wrote:
> Optional nit: You can reduce the level of indentation below by reversing this
> condition and returning early.

Done.

[Ilya Sherman, 2012-08-29 23:41:27]

Please try to reply to all comments other than optional nits and other such
minor comments.  Otherwise, it is hard for me to keep track of the state of the
review.  I shouldn't need to (bump) comments like I am doing below.  Thanks.

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
File base/metrics/histogram.h (right):

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.h:349: class BASE_EXPORT_PRIVATE BucketHistogramSamples :
public HistogramSamples {
On 2012/08/29 08:48:16, Ilya Sherman wrote:
> On 2012/08/24 04:17:58, kaiwang wrote:
> > SparseHistograms don't
> 
> SparseHistograms don't have *contiguous* buckets, but surely they still have
> buckets.  A histogram bucket is essentially a key/value pair.  Am I missing
> something?

(bump)

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.h:402: };
On 2012/08/29 08:48:16, Ilya Sherman wrote:
> On 2012/08/24 04:17:58, kaiwang wrote:
> > On 2012/08/23 07:50:54, Ilya Sherman wrote:
> > > Please declare & define these classes in separate files.
> > 
> > This file currently contains all stuff related to Histogram (5 classes and
> > macros). Only moving samples related code out does not help to clarify.
> > 
> > I agree the best way is to move every class to different file. But it needs
a
> > lot of code change. I'd prefer to do it in seperate CLs
> 
> You are adding a new class, so moving the new class out does not add extra
code.
>  Please move this code out in this CL.  I agree that splitting up this header
> further does not make sense as part of this CL.

(bump)

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
File base/metrics/histogram.cc (left):

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
base/metrics/histogram.cc:543: CHECK_GT(ranges(mid+1), value);
On 2012/08/29 22:42:16, kaiwang wrote:
> For above checks, they are guaranteed by current code logic + DCHECKs. So they
> are like testing the code logic, which should be in unittests instead.

The purpose of DCHECKs, among other things, is to test code invariants.  Unless
you're extremely careful, tests only give you partial coverage.  DCHECKs give
you more complete coverage, by testing on actual inputs seen during the run of
the program.  And since they're only compiled in in Debug mode, they have no
negative side-effects on the Release binary.

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
File base/metrics/histogram.cc (right):

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
base/metrics/histogram.cc:128: index_(-1),
On 2012/08/29 22:42:16, kaiwang wrote:
> Need a way to tell it's on first bucket (index == 0) or it has no data at all
> (index == -1)

Can't you tell this by the check "index_ < counts_->size()"?

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
File base/metrics/histogram.h (right):

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
base/metrics/histogram.h:370: const BucketRanges* bucket_ranges);
On 2012/08/29 22:42:16, kaiwang wrote:
> Will change it to private.
> 
> For static/non-static, there's a coding best practice(inside and outside of
> google) to define private functions that does not change object state all
> static. Unfortunately I can not find the internal pages at hand.
> 
> The good thing about static is, it does not depends on the whole object (no
this
> pointer). So it's much easier for code reader to understand what is going on
if
> you pass the
> members that affect this method. So basically static function has less
> dependency then non-static functions.
> 
> Another reason is, making private functions static does not help
encapsulation,
> because external client can not call these functions. While many public
> functions has to be non-static, because external client need them to change
> object state.
> 
> I do find some design docs of another project mentioning this. It's the maps
> frontend testing design doc:
> https://www.corp.google.com/eng/designdocs/maps/testing.html#s13

As discussed offline, Chromium code usually prefers free functions hidden in the
implementation file's anonymous namespace to private static functions.

Also, please keep in mind that this code review site is used by more than just
Googlers, so it is generally not appropriate to link to internal-only pages,
including documentation.  As much as possible, all relevant resources should be
made equally accessible to all Chromium developers.  (In this case it's not such
a big issue, since you explained the contents fairly clearly, and they are not
of a sensitive nature.  Just something to keep in mind.)

https://chromiumcodereview.appspot.com/10829466/diff/34001/base/metrics/histo...
File base/metrics/histogram.cc (right):

https://chromiumcodereview.appspot.com/10829466/diff/34001/base/metrics/histo...
base/metrics/histogram.cc:93: // smallest to largest, so the sample value must
be run.
What does "must be run" mean?

https://chromiumcodereview.appspot.com/10829466/diff/34001/base/metrics/histo...
File base/metrics/histogram_samples.h (right):

https://chromiumcodereview.appspot.com/10829466/diff/34001/base/metrics/histo...
base/metrics/histogram_samples.h:47: virtual bool
AddSubtractImpl(SampleCountIterator* iter, bool is_add) = 0;
Rather than passing a boolean parameter, please use an enum.  For an example,
see
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/autofill/fo...

https://chromiumcodereview.appspot.com/10829466/diff/34001/base/metrics/histo...
base/metrics/histogram_samples.h:49: void IncreaseSum(int64 diff);
Optional nit: Perhaps "IncreaseSumBy"?

[Ilya Sherman, 2012-08-29 23:45:59]

+Brett, John for question regarding CHECK vs. DCHECK.

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
File base/metrics/histogram.cc (right):

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.cc:38: CHECK_GE(bucket_ranges_->size(), 2u);
On 2012/08/29 08:48:16, Ilya Sherman wrote:
> On 2012/08/24 04:17:58, kaiwang wrote:
> > This check has litter performance impact, I don't see a reason to change to
> > DCHECK.
> > If bucket ranges size is smaller than 2, there will be some crash later if
the
> > execution by pass this check, which will be much harder to find out.
> 
> That is an argument for only ever using CHECKs, and never using DCHECKs. 
>
http://www.chromium.org/developers/coding-style#TOC-CHECK-vs-DCHECK-and-NOTRE...
> lists the cases when CHECK is appropriate -- this doesn't seem to me to be one
> of them.

Brett or John, could one of you chime in on this question?  When is it
appropriate to use CHECK() rather than DCHECK()?

[kaiwang, 2012-08-30 03:13:21]

Sorry I only scanned the most recent snapshot and didn't noticed other comments.

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
File base/metrics/histogram.h (right):

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.h:349: class BASE_EXPORT_PRIVATE BucketHistogramSamples :
public HistogramSamples {
Original Histogram separates the whole sample value space into different parts,
and we call these different parts "buckets". By this definition, sparse
histograms do not have predefined buckets, they only have precise values.

Based on this difference, I suggested to Jim to call Histogram
"BucketHistogram", comparing to "SparseHistogram". But it's hard to change the
existing class name now.

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.h:363: HistogramBase::Count
GetCountFromBucketIndex(size_t bucket_index) const;
What do you think of GetCountAtIndex? from Jim's suggestion

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.h:402: };
On 2012/08/29 23:41:27, Ilya Sherman wrote:
> On 2012/08/29 08:48:16, Ilya Sherman wrote:
> > On 2012/08/24 04:17:58, kaiwang wrote:
> > > On 2012/08/23 07:50:54, Ilya Sherman wrote:
> > > > Please declare & define these classes in separate files.
> > > 
> > > This file currently contains all stuff related to Histogram (5 classes and
> > > macros). Only moving samples related code out does not help to clarify.
> > > 
> > > I agree the best way is to move every class to different file. But it
needs
> a
> > > lot of code change. I'd prefer to do it in seperate CLs
> > 
> > You are adding a new class, so moving the new class out does not add extra
> code.
> >  Please move this code out in this CL.  I agree that splitting up this
header
> > further does not make sense as part of this CL.
> 
> (bump)

Done.

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
File base/metrics/histogram_samples.cc (right):

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram_samples.cc:39: }
On 2012/08/29 08:48:16, Ilya Sherman wrote:
> On 2012/08/23 07:50:54, Ilya Sherman wrote:
> > nit: Why not just call Next()?
> 
> (bump)

Done.

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
File base/metrics/histogram_samples.h (right):

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram_samples.h:46: virtual bool
AddSubtractHelper(SampleCountIterator* iter, bool is_add) = 0;
If it's not add, then it's subtract, added comment about this. A lot of
functions are using bool in this way.
Changing this bool to a enum complicates the implementation (switch ...). Also
the reader of the function signature will be harder to get the idea.

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
File base/metrics/histogram.cc (left):

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
base/metrics/histogram.cc:543: CHECK_GT(ranges(mid+1), value);
A lot of DCHECKs actually reduce the code readability. e.g. we never write code
like:
int i = ...;
i++;
DCHECK(i > original_i);

If you do find some potential bugs in the code, I'd be happy to add a unittest

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
File base/metrics/histogram.cc (right):

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
base/metrics/histogram.cc:128: index_(-1),
even when counts_ has a large size, it's still possible this iterator can return
nothing, because the corresponding HistogramSamples has no samples at all.
When index == 0, it means we have scanned to index 0, and this bucket has
samples. While index == -1 means a begin state and no value has been scanned
yet.

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
File base/metrics/histogram.h (right):

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
base/metrics/histogram.h:370: const BucketRanges* bucket_ranges);
Sorry I forgot a reason to make it a non-static member: subclass can override it
in a more efficient way. E.g. LinearHistogram should be able to directly
calculate the index.
Changed it back to non-static.

https://chromiumcodereview.appspot.com/10829466/diff/34001/base/metrics/histo...
File base/metrics/histogram.cc (right):

https://chromiumcodereview.appspot.com/10829466/diff/34001/base/metrics/histo...
base/metrics/histogram.cc:93: // smallest to largest, so the sample value must
be run.
typo, changed to invalid

https://chromiumcodereview.appspot.com/10829466/diff/34001/base/metrics/histo...
File base/metrics/histogram_samples.h (right):

https://chromiumcodereview.appspot.com/10829466/diff/34001/base/metrics/histo...
base/metrics/histogram_samples.h:47: virtual bool
AddSubtractImpl(SampleCountIterator* iter, bool is_add) = 0;
As replied in earlier comment, I really don't see a reason to make an enum. It's
not like "RequesetMethod", but always only havs 2 states: either add or
subtract.
If there's no coding style against this, I'd prefer to make the function
signature simpler.

Also comment is added to make it more clear.

[Ilya Sherman, 2012-08-30 07:57:39]

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
File base/metrics/histogram.h (right):

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.h:349: class BASE_EXPORT_PRIVATE BucketHistogramSamples :
public HistogramSamples {
On 2012/08/30 03:13:21, kaiwang wrote:
> Original Histogram separates the whole sample value space into different
parts,
> and we call these different parts "buckets". By this definition, sparse
> histograms do not have predefined buckets, they only have precise values.
> 
> Based on this difference, I suggested to Jim to call Histogram
> "BucketHistogram", comparing to "SparseHistogram". But it's hard to change the
> existing class name now.

I think we disagree on what a histogram "bucket" means.  Unless I'm mistaken,
most people would describe a SparseHistogram as also having buckets -- just
sparse ones.  A bucket (or, as the Wikipedia page calls them [1], a "bin"), is
simply the interval to which a set of samples correspond.  Sparse histograms
still group samples into bins/buckets.

How about ContinuousHistogramSamples, to distinguish from (presumably)
SparseHistogramSamples?

[1] http://en.wikipedia.org/wiki/Histogram

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.h:363: HistogramBase::Count
GetCountFromBucketIndex(size_t bucket_index) const;
On 2012/08/30 03:13:21, kaiwang wrote:
> What do you think of GetCountAtIndex? from Jim's suggestion

Works for me :)

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
File base/metrics/histogram.cc (left):

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
base/metrics/histogram.cc:543: CHECK_GT(ranges(mid+1), value);
On 2012/08/30 03:13:22, kaiwang wrote:
> A lot of DCHECKs actually reduce the code readability. e.g. we never write
code
> like:
> int i = ...;
> i++;
> DCHECK(i > original_i);
> 
> If you do find some potential bugs in the code, I'd be happy to add a unittest

I agree that some DCHECKs are unhelpful, but I disagree that these are
unhelpful.  Please preserve them in this CL.  If you want to remove them, please
separate that change into a follow-up CL.  Thanks.

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
File base/metrics/histogram.cc (right):

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
base/metrics/histogram.cc:128: index_(-1),
On 2012/08/30 03:13:22, kaiwang wrote:
> even when counts_ has a large size, it's still possible this iterator can
return
> nothing, because the corresponding HistogramSamples has no samples at all.
> When index == 0, it means we have scanned to index 0, and this bucket has
> samples. While index == -1 means a begin state and no value has been scanned
> yet.

But, the constructor immediately scans over to the first non-empty bucket... so
if there were no nonempty buckets, you would have index := counts_->size().  As
the code is currently written, by the end of the constructor, index will have a
nonnegative value unless |counts_| is empty.

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
base/metrics/histogram.cc:135: return is_done_;
Rather than keeping a separate variable for this, you can:

return index_ < static_cast<int>(counts_->size()) - 1;

Or, with my suggested change for index_, you can simply:

return index_ < counts_->size();

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
base/metrics/histogram.cc:155: CHECK(!Done());
This should probably be part of the public method Next(), rather than the
protected method ForwardToNextNonemptyBucket().  (This doesn't much matter if
you keep the |is_done_| variable, but it helps if you remove that variable.)

https://chromiumcodereview.appspot.com/10829466/diff/34001/base/metrics/histo...
File base/metrics/histogram_samples.h (right):

https://chromiumcodereview.appspot.com/10829466/diff/34001/base/metrics/histo...
base/metrics/histogram_samples.h:47: virtual bool
AddSubtractImpl(SampleCountIterator* iter, bool is_add) = 0;
On 2012/08/30 03:13:22, kaiwang wrote:
> As replied in earlier comment, I really don't see a reason to make an enum.
It's
> not like "RequesetMethod", but always only havs 2 states: either add or
> subtract.
> If there's no coding style against this, I'd prefer to make the function
> signature simpler.
> 
> Also comment is added to make it more clear.

The reason to use an enum rather than a boolean is so that it is clear at the
call site what the code is doing.  Compare the following:

CHECK(AddSubtractImpl(iter, false));
CHECK(AddSubtractImpl(iter, SUBTRACT));

The meaning of |false| in the first line is unclear; you need to jump to the
HistogramSamples header to understand it.  The meaning of |SUBTRACT| in the
second line is much clearer.

This, like many other common practices, is not explicitly listed in the style
guide.  However, Jim and I -- and many other Chromium developers -- prefer this
style.  Please use it here.

[jar (doing other things), 2012-08-30 22:54:38]

Drive by comments based on some chatting with Kai.

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
File base/metrics/histogram.cc (left):

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.cc:331: !sample.Histogram::SampleSet::Deserialize(&iter))
{
How are these samples recovered?  I don't see the corresponding unpickling code.

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
File base/metrics/histogram.cc (right):

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.cc:379: if (value > ranges(bucket_count_) - 1)
You appear to have changed a constant comparisons and constant assignments into
an array lookups (for both).  Perhaps you can do assertions to ensure they are
equivalent, and then do the constant comparisons?

Per discussion: revert but add DCHECK to verify that end-of-range values are 0
and kSampleType_MAX.

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
File base/metrics/histogram.h (right):

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.h:349: class BASE_EXPORT_PRIVATE BucketHistogramSamples :
public HistogramSamples {
I think perchance you meant ContiGuousHistogramSamples instead of
ContinuousHistogramSamples... since you used that word... but both are pretty
long :-(.

How about using SampleVector here, and then using SampleMap for sparse
histograms??  This seemed plausible when I chatted with Kai.

Ilya: WDYT?

On 2012/08/30 07:57:39, Ilya Sherman wrote:
> On 2012/08/30 03:13:21, kaiwang wrote:
> > Original Histogram separates the whole sample value space into different
> parts,
> > and we call these different parts "buckets". By this definition, sparse
> > histograms do not have predefined buckets, they only have precise values.
> > 
> > Based on this difference, I suggested to Jim to call Histogram
> > "BucketHistogram", comparing to "SparseHistogram". But it's hard to change
the
> > existing class name now.
> 
> I think we disagree on what a histogram "bucket" means.  Unless I'm mistaken,
> most people would describe a SparseHistogram as also having buckets -- just
> sparse ones.  A bucket (or, as the Wikipedia page calls them [1], a "bin"), is
> simply the interval to which a set of samples correspond.  Sparse histograms
> still group samples into bins/buckets.
> 
> How about ContinuousHistogramSamples, to distinguish from (presumably)
> SparseHistogramSamples?
> 
> [1] http://en.wikipedia.org/wiki/Histogram

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.h:560: // taken asynchronously relative to sampling, and
out counting code currently
nit: out --> our

[Ilya Sherman, 2012-08-30 23:01:46]

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
File base/metrics/histogram.h (right):

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.h:349: class BASE_EXPORT_PRIVATE BucketHistogramSamples :
public HistogramSamples {
On 2012/08/30 22:54:38, jar wrote:
> I think perchance you meant ContiGuousHistogramSamples instead of
> ContinuousHistogramSamples... since you used that word... but both are pretty
> long :-(.
> 
> How about using SampleVector here, and then using SampleMap for sparse
> histograms??  This seemed plausible when I chatted with Kai.
> 
> Ilya: WDYT?

SGTM

> 
> On 2012/08/30 07:57:39, Ilya Sherman wrote:
> > On 2012/08/30 03:13:21, kaiwang wrote:
> > > Original Histogram separates the whole sample value space into different
> > parts,
> > > and we call these different parts "buckets". By this definition, sparse
> > > histograms do not have predefined buckets, they only have precise values.
> > > 
> > > Based on this difference, I suggested to Jim to call Histogram
> > > "BucketHistogram", comparing to "SparseHistogram". But it's hard to change
> the
> > > existing class name now.
> > 
> > I think we disagree on what a histogram "bucket" means.  Unless I'm
mistaken,
> > most people would describe a SparseHistogram as also having buckets -- just
> > sparse ones.  A bucket (or, as the Wikipedia page calls them [1], a "bin"),
is
> > simply the interval to which a set of samples correspond.  Sparse histograms
> > still group samples into bins/buckets.
> > 
> > How about ContinuousHistogramSamples, to distinguish from (presumably)
> > SparseHistogramSamples?
> > 
> > [1] http://en.wikipedia.org/wiki/Histogram
>

[kaiwang, 2012-09-07 01:14:11]

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
File base/metrics/histogram.cc (left):

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.cc:331: !sample.Histogram::SampleSet::Deserialize(&iter))
{
See line 387, it adds all samples from a PickleIterator

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
File base/metrics/histogram.cc (right):

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.cc:379: if (value > ranges(bucket_count_) - 1)
On 2012/08/30 22:54:38, jar wrote:
> You appear to have changed a constant comparisons and constant assignments
into
> an array lookups (for both).  Perhaps you can do assertions to ensure they are
> equivalent, and then do the constant comparisons?
> 
> Per discussion: revert but add DCHECK to verify that end-of-range values are 0
> and kSampleType_MAX.

Done.

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
File base/metrics/histogram.h (right):

https://chromiumcodereview.appspot.com/10829466/diff/15003/base/metrics/histo...
base/metrics/histogram.h:560: // taken asynchronously relative to sampling, and
out counting code currently
On 2012/08/30 22:54:38, jar wrote:
> nit: out --> our

Done.

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
File base/metrics/histogram.cc (left):

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
base/metrics/histogram.cc:532: DCHECK_GE(over, under);
On 2012/08/29 08:48:16, Ilya Sherman wrote:
> Why did you remove this DCHECK?

Done.

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
base/metrics/histogram.cc:543: CHECK_GT(ranges(mid+1), value);
On 2012/08/30 07:57:39, Ilya Sherman wrote:
> On 2012/08/30 03:13:22, kaiwang wrote:
> > A lot of DCHECKs actually reduce the code readability. e.g. we never write
> code
> > like:
> > int i = ...;
> > i++;
> > DCHECK(i > original_i);
> > 
> > If you do find some potential bugs in the code, I'd be happy to add a
unittest
> 
> I agree that some DCHECKs are unhelpful, but I disagree that these are
> unhelpful.  Please preserve them in this CL.  If you want to remove them,
please
> separate that change into a follow-up CL.  Thanks.

Done.

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
File base/metrics/histogram.cc (right):

https://chromiumcodereview.appspot.com/10829466/diff/31007/base/metrics/histo...
base/metrics/histogram.cc:128: index_(-1),
Changed according to your suggestion. (note, I'll need to change the name of
ForwardToNextNonemptyBucket).
2 different impl always have pros and cons. Sometimes just let others choose the
impl they like?

https://chromiumcodereview.appspot.com/10829466/diff/34001/base/metrics/histo...
File base/metrics/histogram_samples.h (right):

https://chromiumcodereview.appspot.com/10829466/diff/34001/base/metrics/histo...
base/metrics/histogram_samples.h:47: virtual bool
AddSubtractImpl(SampleCountIterator* iter, bool is_add) = 0;
On 2012/08/30 07:57:40, Ilya Sherman wrote:
> On 2012/08/30 03:13:22, kaiwang wrote:
> > As replied in earlier comment, I really don't see a reason to make an enum.
> It's
> > not like "RequesetMethod", but always only havs 2 states: either add or
> > subtract.
> > If there's no coding style against this, I'd prefer to make the function
> > signature simpler.
> > 
> > Also comment is added to make it more clear.
> 
> The reason to use an enum rather than a boolean is so that it is clear at the
> call site what the code is doing.  Compare the following:
> 
> CHECK(AddSubtractImpl(iter, false));
> CHECK(AddSubtractImpl(iter, SUBTRACT));
> 
> The meaning of |false| in the first line is unclear; you need to jump to the
> HistogramSamples header to understand it.  The meaning of |SUBTRACT| in the
> second line is much clearer.
> 
> This, like many other common practices, is not explicitly listed in the style
> guide.  However, Jim and I -- and many other Chromium developers -- prefer
this
> style.  Please use it here.

Done.

[kaiwang, 2012-09-07 03:45:07]

Added GetBucketIndex support

[kaiwang, 2012-09-07 22:59:28]

Reviewers, please help to review this CL:

jar: base, net and chrome/browser/net
isherman: base/metrics
sky: chrome/browser and chrome/common and chrome/test
erikwright: chrome_frame/
avi: content/

Thanks a lot!

[Avi (use Gerrit), 2012-09-07 23:20:26]

content lgtm

[sky, 2012-09-08 00:00:25]

LGTM

http://codereview.chromium.org/10829466/diff/53008/base/metrics/histogram_sam...
File base/metrics/histogram_samples.h (right):

http://codereview.chromium.org/10829466/diff/53008/base/metrics/histogram_sam...
base/metrics/histogram_samples.h:37: virtual scoped_ptr<SampleCountIterator>
Iterator() const = 0;
It's very unusual for an iterator to not be by value.

[Ilya Sherman, 2012-09-12 03:20:58]

Nearly there :)

https://chromiumcodereview.appspot.com/10829466/diff/53008/base/metrics/histo...
File base/metrics/histogram.h (right):

https://chromiumcodereview.appspot.com/10829466/diff/53008/base/metrics/histo...
base/metrics/histogram.h:345: class SampleVector;
nit: Please leave these alpha-sorted with the other forward-declarations.

https://chromiumcodereview.appspot.com/10829466/diff/53008/base/metrics/histo...
File base/metrics/histogram_samples.cc (right):

https://chromiumcodereview.appspot.com/10829466/diff/53008/base/metrics/histo...
base/metrics/histogram_samples.cc:59: bool SampleCountPickleIterator::ReadNext()
{
Optional nit: This method is only used in one place -- is it worth decomposing?

https://chromiumcodereview.appspot.com/10829466/diff/53008/base/metrics/histo...
base/metrics/histogram_samples.cc:126: CHECK(!Done());
nit: DCHECK

https://chromiumcodereview.appspot.com/10829466/diff/53008/base/metrics/histo...
File base/metrics/histogram_samples.h (right):

https://chromiumcodereview.appspot.com/10829466/diff/53008/base/metrics/histo...
base/metrics/histogram_samples.h:46: // Add samples when |is_add| is true, else
subtract samples.
nit: Please update this comment.

https://chromiumcodereview.appspot.com/10829466/diff/53008/base/metrics/histo...
File base/metrics/histogram_snapshot_manager.h (right):

https://chromiumcodereview.appspot.com/10829466/diff/53008/base/metrics/histo...
base/metrics/histogram_snapshot_manager.h:49: // List of histograms found
corrupt to be corrupt, and their problems.
nit: "found corrupt to be corrupt" -> "found to be corrupt"

https://chromiumcodereview.appspot.com/10829466/diff/53008/chrome/browser/net...
File chrome/browser/net/http_pipelining_compatibility_client_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10829466/diff/53008/chrome/browser/net...
chrome/browser/net/http_pipelining_compatibility_client_unittest.cc:154: if
(!samples.get())
Should this be an ASSERT_TRUE(samples.get()), i.e. should this trigger a test
failure?  (Same comment applies to other cases below.)

https://chromiumcodereview.appspot.com/10829466/diff/53008/chrome/common/metr...
File chrome/common/metrics/metrics_log_base.cc (left):

https://chromiumcodereview.appspot.com/10829466/diff/53008/chrome/common/metr...
chrome/common/metrics/metrics_log_base.cc:455:
DCHECK_EQ(histogram.bucket_count(), snapshot.size());
nit: Can this DCHECK be restored?

https://chromiumcodereview.appspot.com/10829466/diff/53008/chrome/common/metr...
chrome/common/metrics/metrics_log_base.cc:476:
OPEN_ELEMENT_FOR_SCOPE("histogrambucket");
What happened to this line?

https://chromiumcodereview.appspot.com/10829466/diff/53008/chrome/test/nacl/n...
File chrome/test/nacl/nacl_browsertest_uma.cc (right):

https://chromiumcodereview.appspot.com/10829466/diff/53008/chrome/test/nacl/n...
chrome/test/nacl/nacl_browsertest_uma.cc:122: "NaCl.LoadStatus.Plugin",
plugin::ERROR_LOAD_SUCCESS, 1);
nit: No need to re-wrap this line; the previous wrapping is fine.

https://chromiumcodereview.appspot.com/10829466/diff/53008/chrome/test/nacl/n...
chrome/test/nacl/nacl_browsertest_uma.cc:126: "NaCl.LoadStatus.SelLdr", LOAD_OK,
1);
nit: No need to re-wrap this line; the previous wrapping is fine.

https://chromiumcodereview.appspot.com/10829466/diff/53008/content/common/chi...
File content/common/child_histogram_message_filter.cc (left):

https://chromiumcodereview.appspot.com/10829466/diff/53008/content/common/chi...
content/common/child_histogram_message_filter.cc:76:
DCHECK_EQ(histogram.bucket_count(), snapshot.size());
Can this DCHECK be preserved?

https://chromiumcodereview.appspot.com/10829466/diff/53008/net/disk_cache/sta...
File net/disk_cache/stats_histogram.cc (right):

https://chromiumcodereview.appspot.com/10829466/diff/53008/net/disk_cache/sta...
net/disk_cache/stats_histogram.cc:9: #include "base/metrics/sample_vector.h"
nit: Alpha-sort

https://chromiumcodereview.appspot.com/10829466/diff/53008/net/disk_cache/sta...
net/disk_cache/stats_histogram.cc:34: void
StatsHistogram::InitializeBucketRanges(const Stats* stats,
nit: Add a comment indicating that this is static (unless you tuck it into an
anonymous namespace per my suggestion in the header file).

https://chromiumcodereview.appspot.com/10829466/diff/53008/net/disk_cache/sta...
net/disk_cache/stats_histogram.cc:49: CHECK(stats);
nit: DCHECK

https://chromiumcodereview.appspot.com/10829466/diff/53008/net/disk_cache/sta...
net/disk_cache/stats_histogram.cc:51: BucketRanges* ranges = new
BucketRanges(bucket_count + 1);
Who owns this memory?  Please either write this into a scoped_ptr<> that you
then Pass() (preferred), or a comment describing the memory ownership.

https://chromiumcodereview.appspot.com/10829466/diff/53008/net/disk_cache/sta...
File net/disk_cache/stats_histogram.h (right):

https://chromiumcodereview.appspot.com/10829466/diff/53008/net/disk_cache/sta...
net/disk_cache/stats_histogram.h:17: };
nit: No need for this semicolon

https://chromiumcodereview.appspot.com/10829466/diff/53008/net/disk_cache/sta...
net/disk_cache/stats_histogram.h:28: // may eventually go away.
Any chance of implementing this TODO and migrating away from deriving from
Histogram instead?

https://chromiumcodereview.appspot.com/10829466/diff/53008/net/disk_cache/sta...
net/disk_cache/stats_histogram.h:37: const Stats* stats);
Can this have private visibility, since clients are seemingly expected to use
the FactoryGet() factory instead?

https://chromiumcodereview.appspot.com/10829466/diff/53008/net/disk_cache/sta...
net/disk_cache/stats_histogram.h:41: base::BucketRanges* ranges);
nit: Can this be tucked into an anonymous namespace in the implementation file?

https://chromiumcodereview.appspot.com/10829466/diff/53008/net/disk_cache/sta...
net/disk_cache/stats_histogram.h:50: const Stats* stats_;
nit: const Stats* const stats_?

https://chromiumcodereview.appspot.com/10829466/diff/53008/net/socket_stream/...
File net/socket_stream/socket_stream_metrics_unittest.cc (right):

https://chromiumcodereview.appspot.com/10829466/diff/53008/net/socket_stream/...
net/socket_stream/socket_stream_metrics_unittest.cc:46: if (original.get()) {
Should this be an ASSERT(original.get())?

https://chromiumcodereview.appspot.com/10829466/diff/53008/net/socket_stream/...
net/socket_stream/socket_stream_metrics_unittest.cc:81: if (original.get()) {
Ditto

https://chromiumcodereview.appspot.com/10829466/diff/53008/net/socket_stream/...
net/socket_stream/socket_stream_metrics_unittest.cc:112: if (original.get()) {
Ditto

https://chromiumcodereview.appspot.com/10829466/diff/53008/net/url_request/ur...
File net/url_request/url_request_throttler_unittest.cc (right):

https://chromiumcodereview.appspot.com/10829466/diff/53008/net/url_request/ur...
net/url_request/url_request_throttler_unittest.cc:238: if (original != NULL)
nit: You can just write this as "if (original)"

https://chromiumcodereview.appspot.com/10829466/diff/53008/net/url_request/ur...
net/url_request/url_request_throttler_unittest.cc:240: samples_[name] =
samples.release();
Is |samples_[name]| guaranteed to be empty prior to this?  If not, this looks
like it will leak memory.

[kaiwang, 2012-09-20 22:54:59]

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

http://codereview.chromium.org/10829466/diff/53008/base/metrics/histogram.h#n...
base/metrics/histogram.h:345: class SampleVector;
it's suggested by Jim, because BucketRanges and SampleVector are supporting
classes for below histograms. So putting them into seperate categories

http://codereview.chromium.org/10829466/diff/53008/base/metrics/histogram_sam...
File base/metrics/histogram_samples.cc (right):

http://codereview.chromium.org/10829466/diff/53008/base/metrics/histogram_sam...
base/metrics/histogram_samples.cc:59: bool SampleCountPickleIterator::ReadNext()
{
On 2012/09/12 03:20:58, Ilya Sherman wrote:
> Optional nit: This method is only used in one place -- is it worth
decomposing?

Done.

http://codereview.chromium.org/10829466/diff/53008/base/metrics/histogram_sam...
File base/metrics/histogram_samples.h (right):

http://codereview.chromium.org/10829466/diff/53008/base/metrics/histogram_sam...
base/metrics/histogram_samples.h:46: // Add samples when |is_add| is true, else
subtract samples.
On 2012/09/12 03:20:58, Ilya Sherman wrote:
> nit: Please update this comment.

Done.

http://codereview.chromium.org/10829466/diff/53008/base/metrics/histogram_sna...
File base/metrics/histogram_snapshot_manager.h (right):

http://codereview.chromium.org/10829466/diff/53008/base/metrics/histogram_sna...
base/metrics/histogram_snapshot_manager.h:49: // List of histograms found
corrupt to be corrupt, and their problems.
On 2012/09/12 03:20:58, Ilya Sherman wrote:
> nit: "found corrupt to be corrupt" -> "found to be corrupt"

Done.

http://codereview.chromium.org/10829466/diff/53008/chrome/browser/net/http_pi...
File chrome/browser/net/http_pipelining_compatibility_client_unittest.cc
(right):

http://codereview.chromium.org/10829466/diff/53008/chrome/browser/net/http_pi...
chrome/browser/net/http_pipelining_compatibility_client_unittest.cc:154: if
(!samples.get())
The old GetHistogram function can return an empty SampleSet. But new
implementation will return an empty scoped_ptr because we don't initiate
concrete class SampleVector here.

According to the unittest logic, the following lines are no-op when samples is
empty. So it's safe to early return

http://codereview.chromium.org/10829466/diff/53008/chrome/common/metrics/metr...
File chrome/common/metrics/metrics_log_base.cc (left):

http://codereview.chromium.org/10829466/diff/53008/chrome/common/metrics/metr...
chrome/common/metrics/metrics_log_base.cc:455:
DCHECK_EQ(histogram.bucket_count(), snapshot.size());
now it's always true (enforced by HistogramSamples, which is created based on
bucket_count)

http://codereview.chromium.org/10829466/diff/53008/chrome/common/metrics/metr...
chrome/common/metrics/metrics_log_base.cc:476:
OPEN_ELEMENT_FOR_SCOPE("histogrambucket");
On 2012/09/12 03:20:58, Ilya Sherman wrote:
> What happened to this line?

Done.

http://codereview.chromium.org/10829466/diff/53008/chrome/test/nacl/nacl_brow...
File chrome/test/nacl/nacl_browsertest_uma.cc (right):

http://codereview.chromium.org/10829466/diff/53008/chrome/test/nacl/nacl_brow...
chrome/test/nacl/nacl_browsertest_uma.cc:126: "NaCl.LoadStatus.SelLdr", LOAD_OK,
1);
they are more readable, in previous wrapping, it's hard to notice the function
call has 3 parameters

http://codereview.chromium.org/10829466/diff/53008/content/common/child_histo...
File content/common/child_histogram_message_filter.cc (left):

http://codereview.chromium.org/10829466/diff/53008/content/common/child_histo...
content/common/child_histogram_message_filter.cc:76:
DCHECK_EQ(histogram.bucket_count(), snapshot.size());
it's enforced by SampleVector logic, which is created from
histogram.bucket_ranges().
Note, HistogramSamples is a base class, so it does not have information like
bucket_count. (Does not apply to SampleMap)

http://codereview.chromium.org/10829466/diff/53008/net/disk_cache/stats_histo...
File net/disk_cache/stats_histogram.cc (right):

http://codereview.chromium.org/10829466/diff/53008/net/disk_cache/stats_histo...
net/disk_cache/stats_histogram.cc:9: #include "base/metrics/sample_vector.h"
On 2012/09/12 03:20:58, Ilya Sherman wrote:
> nit: Alpha-sort

Done.

http://codereview.chromium.org/10829466/diff/53008/net/disk_cache/stats_histo...
net/disk_cache/stats_histogram.cc:34: void
StatsHistogram::InitializeBucketRanges(const Stats* stats,
On 2012/09/12 03:20:58, Ilya Sherman wrote:
> nit: Add a comment indicating that this is static (unless you tuck it into an
> anonymous namespace per my suggestion in the header file).

Done.

http://codereview.chromium.org/10829466/diff/53008/net/disk_cache/stats_histo...
net/disk_cache/stats_histogram.cc:49: CHECK(stats);
On 2012/09/12 03:20:58, Ilya Sherman wrote:
> nit: DCHECK

Done.

http://codereview.chromium.org/10829466/diff/53008/net/disk_cache/stats_histo...
net/disk_cache/stats_histogram.cc:51: BucketRanges* ranges = new
BucketRanges(bucket_count + 1);
On 2012/09/12 03:20:58, Ilya Sherman wrote:
> Who owns this memory?  Please either write this into a scoped_ptr<> that you
> then Pass() (preferred), or a comment describing the memory ownership.

Done.

http://codereview.chromium.org/10829466/diff/53008/net/disk_cache/stats_histo...
File net/disk_cache/stats_histogram.h (right):

http://codereview.chromium.org/10829466/diff/53008/net/disk_cache/stats_histo...
net/disk_cache/stats_histogram.h:17: };
On 2012/09/12 03:20:58, Ilya Sherman wrote:
> nit: No need for this semicolon

Done.

http://codereview.chromium.org/10829466/diff/53008/net/disk_cache/stats_histo...
net/disk_cache/stats_histogram.h:28: // may eventually go away.
Still have a more change needed. Feel free to file a CL and address this

http://codereview.chromium.org/10829466/diff/53008/net/url_request/url_reques...
File net/url_request/url_request_throttler_unittest.cc (right):

http://codereview.chromium.org/10829466/diff/53008/net/url_request/url_reques...
net/url_request/url_request_throttler_unittest.cc:238: if (original != NULL)
On 2012/09/12 03:20:58, Ilya Sherman wrote:
> nit: You can just write this as "if (original)"

Done.

http://codereview.chromium.org/10829466/diff/53008/net/url_request/url_reques...
net/url_request/url_request_throttler_unittest.cc:240: samples_[name] =
samples.release();
it's guranteed to be empty

[kaiwang, 2012-09-20 23:40:58]

For the code under net/, I'll consult Jim to get the owner's opinion first

[jar (doing other things), 2012-09-21 00:15:12]

drive by.... for net/disk_cache/*, be sure to get reviews and closure from
Ricardo (I added him as a cc).

[erikwright (departed), 2012-09-21 01:19:25]

chrome_frame/* LGTM

[rvargas (doing something else), 2012-09-21 01:30:52]

net lgtm

http://codereview.chromium.org/10829466/diff/70004/net/socket_stream/socket_s...
File net/socket_stream/socket_stream_metrics_unittest.cc (right):

http://codereview.chromium.org/10829466/diff/70004/net/socket_stream/socket_s...
net/socket_stream/socket_stream_metrics_unittest.cc:51: EXPECT_EQ(
nit:
EXPECT_EQ(3,
          samples...);

http://codereview.chromium.org/10829466/diff/70004/net/url_request/url_reques...
File net/url_request/url_request_throttler_unittest.cc (right):

http://codereview.chromium.org/10829466/diff/70004/net/url_request/url_reques...
net/url_request/url_request_throttler_unittest.cc:216: original_samples_[name] =
histogram->SnapshotSamples().release();
nit: This is kind of ugly.

Am I the only one that finds the use of scoped_ptr for the return value to be
overkill / forced ?

Note that's also the official position of the Google C++ Primer.(see the
paragraph just before scoped_ptr, under smart pointers).

[kaiwang, 2012-09-21 02:51:22]

Talked to rvargas. Will keep the scoped_ptr return value for now

http://codereview.chromium.org/10829466/diff/70004/net/socket_stream/socket_s...
File net/socket_stream/socket_stream_metrics_unittest.cc (right):

http://codereview.chromium.org/10829466/diff/70004/net/socket_stream/socket_s...
net/socket_stream/socket_stream_metrics_unittest.cc:51: EXPECT_EQ(
On 2012/09/21 01:30:53, rvargas wrote:
> nit:
> EXPECT_EQ(3,
>           samples...);

Done.

[Ilya Sherman, 2012-09-21 08:52:11]

LGTM with a last few nits

https://chromiumcodereview.appspot.com/10829466/diff/53008/chrome/common/metr...
File chrome/common/metrics/metrics_log_base.cc (left):

https://chromiumcodereview.appspot.com/10829466/diff/53008/chrome/common/metr...
chrome/common/metrics/metrics_log_base.cc:455:
DCHECK_EQ(histogram.bucket_count(), snapshot.size());
On 2012/09/20 22:54:59, kaiwang wrote:
> now it's always true (enforced by HistogramSamples, which is created based on
> bucket_count)

Sorry, I don't follow.  These are being passed in as separate arguments --
nothing guarantees that they are for the same histogram...  What am I missing?

https://chromiumcodereview.appspot.com/10829466/diff/53008/net/disk_cache/sta...
File net/disk_cache/stats_histogram.h (right):

https://chromiumcodereview.appspot.com/10829466/diff/53008/net/disk_cache/sta...
net/disk_cache/stats_histogram.h:37: const Stats* stats);
On 2012/09/12 03:20:58, Ilya Sherman wrote:
> Can this have private visibility, since clients are seemingly expected to use
> the FactoryGet() factory instead?

Ping.

https://chromiumcodereview.appspot.com/10829466/diff/53008/net/disk_cache/sta...
net/disk_cache/stats_histogram.h:41: base::BucketRanges* ranges);
On 2012/09/12 03:20:58, Ilya Sherman wrote:
> nit: Can this be tucked into an anonymous namespace in the implementation
file?

Ping.

https://chromiumcodereview.appspot.com/10829466/diff/53008/net/disk_cache/sta...
net/disk_cache/stats_histogram.h:50: const Stats* stats_;
On 2012/09/12 03:20:58, Ilya Sherman wrote:
> nit: const Stats* const stats_?

Ping.

https://chromiumcodereview.appspot.com/10829466/diff/72006/base/metrics/histo...
File base/metrics/histogram_samples.cc (right):

https://chromiumcodereview.appspot.com/10829466/diff/72006/base/metrics/histo...
base/metrics/histogram_samples.cc:43: CHECK(!Done());
nit: DCHECK

https://chromiumcodereview.appspot.com/10829466/diff/72006/base/metrics/histo...
base/metrics/histogram_samples.cc:53: CHECK(!Done());
nit: DCHECK

https://chromiumcodereview.appspot.com/10829466/diff/72006/base/metrics/histo...
base/metrics/histogram_samples.cc:68:
CHECK(AddSubtractImpl(other.Iterator().get(), ADD));
nit: DCHECK (which also means you'll need to move the actual function call onto
a separate line, so that it still runs in release builds).

https://chromiumcodereview.appspot.com/10829466/diff/72006/base/metrics/histo...
base/metrics/histogram_samples.cc:87:
CHECK(AddSubtractImpl(other.Iterator().get(), SUBTRACT));
nit: DCHECK (which also means you'll need to move the actual function call onto
a separate line, so that it still runs in release builds).

https://chromiumcodereview.appspot.com/10829466/diff/72006/base/metrics/sampl...
File base/metrics/sample_vector.cc (right):

https://chromiumcodereview.appspot.com/10829466/diff/72006/base/metrics/sampl...
base/metrics/sample_vector.cc:125: CHECK(!Done());
nit: DCHECK

https://chromiumcodereview.appspot.com/10829466/diff/72006/base/metrics/sampl...
base/metrics/sample_vector.cc:133: CHECK(!Done());
nit: DCHECK

https://chromiumcodereview.appspot.com/10829466/diff/72006/base/metrics/sampl...
base/metrics/sample_vector.cc:143: CHECK(!Done());
nit: DCHECK

[kaiwang, 2012-09-21 19:51:18]

https://chromiumcodereview.appspot.com/10829466/diff/53008/chrome/common/metr...
File chrome/common/metrics/metrics_log_base.cc (left):

https://chromiumcodereview.appspot.com/10829466/diff/53008/chrome/common/metr...
chrome/common/metrics/metrics_log_base.cc:455:
DCHECK_EQ(histogram.bucket_count(), snapshot.size());
Sorry I didn't make it clear.
HistogramSamples does not have size() member function.
For this function, it actually outputs information separately from |histogram|
and |snapshot|. It's not a problem when snapshot is not from histogram. If
snapshot is from histogram, other code in Histogram enforces bucket_count()
always equals size()

https://chromiumcodereview.appspot.com/10829466/diff/53008/net/disk_cache/sta...
File net/disk_cache/stats_histogram.h (right):

https://chromiumcodereview.appspot.com/10829466/diff/53008/net/disk_cache/sta...
net/disk_cache/stats_histogram.h:50: const Stats* stats_;
For above 3 comments, I tried to follow the original code (public and const),
and code in base/metricts(Initialize). I'm trying to limit my change only to
SampleSet -> HistogramSamples, so it doesnot make sense to change more in this
CL.
Current OWNER looked good. If you feel these code is a problem, feel free to
file another CL.

https://chromiumcodereview.appspot.com/10829466/diff/72006/base/metrics/histo...
File base/metrics/histogram_samples.cc (right):

https://chromiumcodereview.appspot.com/10829466/diff/72006/base/metrics/histo...
base/metrics/histogram_samples.cc:43: CHECK(!Done());
On 2012/09/21 08:52:12, Ilya Sherman wrote:
> nit: DCHECK

Done.

https://chromiumcodereview.appspot.com/10829466/diff/72006/base/metrics/histo...
base/metrics/histogram_samples.cc:53: CHECK(!Done());
On 2012/09/21 08:52:12, Ilya Sherman wrote:
> nit: DCHECK

Done.

https://chromiumcodereview.appspot.com/10829466/diff/72006/base/metrics/histo...
base/metrics/histogram_samples.cc:68:
CHECK(AddSubtractImpl(other.Iterator().get(), ADD));
On 2012/09/21 08:52:12, Ilya Sherman wrote:
> nit: DCHECK (which also means you'll need to move the actual function call
onto
> a separate line, so that it still runs in release builds).

Done.

https://chromiumcodereview.appspot.com/10829466/diff/72006/base/metrics/histo...
base/metrics/histogram_samples.cc:87:
CHECK(AddSubtractImpl(other.Iterator().get(), SUBTRACT));
On 2012/09/21 08:52:12, Ilya Sherman wrote:
> nit: DCHECK (which also means you'll need to move the actual function call
onto
> a separate line, so that it still runs in release builds).

Done.

https://chromiumcodereview.appspot.com/10829466/diff/72006/base/metrics/sampl...
File base/metrics/sample_vector.cc (right):

https://chromiumcodereview.appspot.com/10829466/diff/72006/base/metrics/sampl...
base/metrics/sample_vector.cc:125: CHECK(!Done());
On 2012/09/21 08:52:12, Ilya Sherman wrote:
> nit: DCHECK

Done.

https://chromiumcodereview.appspot.com/10829466/diff/72006/base/metrics/sampl...
base/metrics/sample_vector.cc:133: CHECK(!Done());
On 2012/09/21 08:52:12, Ilya Sherman wrote:
> nit: DCHECK

Done.

https://chromiumcodereview.appspot.com/10829466/diff/72006/base/metrics/sampl...
base/metrics/sample_vector.cc:143: CHECK(!Done());
On 2012/09/21 08:52:12, Ilya Sherman wrote:
> nit: DCHECK

Done.

[jar (doing other things), 2012-09-21 19:57:41]

base gyp stuff, and chrome/browser/net LGTM

https://chromiumcodereview.appspot.com/10829466/diff/70004/net/disk_cache/sta...
File net/disk_cache/stats_histogram.h (right):

https://chromiumcodereview.appspot.com/10829466/diff/70004/net/disk_cache/sta...
net/disk_cache/stats_histogram.h:16: class HistogramSamples;
tiny nit: probably alphabetize.

[kaiwang, 2012-09-21 19:59:35]

https://chromiumcodereview.appspot.com/10829466/diff/70004/net/disk_cache/sta...
File net/disk_cache/stats_histogram.h (right):

https://chromiumcodereview.appspot.com/10829466/diff/70004/net/disk_cache/sta...
net/disk_cache/stats_histogram.h:16: class HistogramSamples;
On 2012/09/21 19:57:41, jar wrote:
> tiny nit: probably alphabetize.

Done.

[commit-bot: I haz the power, 2012-09-21 23:26:16]

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

[commit-bot: I haz the power, 2012-09-22 00:48:46]

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

[commit-bot: I haz the power, 2012-09-22 03:42:14]

Change committed as 158166