Histogram type support in HistogramBase and remove SetRangeDescription function

Histogram type support in HistogramBase and remove SetRangeDescription function


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

[Ilya Sherman, 2012-10-31 00:35:19]

LGTM

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

http://codereview.chromium.org/11342060/diff/1/base/metrics/histogram.cc#oldc...
base/metrics/histogram.cc:196: DCHECK_NE(NOT_VALID_IN_RENDERER,
histogram.histogram_type());
Please double-check with Jim that this is ok to remove, as I don't know the
history for why it was added in the first place.

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

http://codereview.chromium.org/11342060/diff/1/base/metrics/histogram.cc#newc...
base/metrics/histogram.cc:644: if (descriptions != NULL) {
nit: This is usually written as "if (descriptions)"

http://codereview.chromium.org/11342060/diff/1/base/metrics/histogram.cc#newc...
base/metrics/histogram.cc:645: for (int i =0; descriptions[i].description; ++i)
{
nit: "=0" -> "= 0"

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

http://codereview.chromium.org/11342060/diff/1/base/metrics/histogram.h#newco...
base/metrics/histogram.h:597: // its "description" field.
nit: You should document that |descriptions| can be null.

http://codereview.chromium.org/11342060/diff/1/base/metrics/histogram_base.cc
File base/metrics/histogram_base.cc (right):

http://codereview.chromium.org/11342060/diff/1/base/metrics/histogram_base.cc...
base/metrics/histogram_base.cc:35: break;
nit: No need for any break stmts in this switch, since each case has a return
stmt, except for the default case which is the last case.

http://codereview.chromium.org/11342060/diff/1/base/metrics/histogram_base.h
File base/metrics/histogram_base.h (right):

http://codereview.chromium.org/11342060/diff/1/base/metrics/histogram_base.h#...
base/metrics/histogram_base.h:22: // processes into the browser. If you create
another class inherits
nit: "class inherits" -> "class that inherits from"

http://codereview.chromium.org/11342060/diff/1/base/metrics/histogram_base.h#...
base/metrics/histogram_base.h:33: std::string HistogramTypeName(HistogramType
type);
Optional nit: Perhaps "HistogramTypeToString"?

[kaiwang, 2012-10-31 01:12:56]

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

http://codereview.chromium.org/11342060/diff/1/base/metrics/histogram.cc#newc...
base/metrics/histogram.cc:644: if (descriptions != NULL) {
On 2012/10/31 00:35:19, Ilya Sherman wrote:
> nit: This is usually written as "if (descriptions)"

Done.

http://codereview.chromium.org/11342060/diff/1/base/metrics/histogram.cc#newc...
base/metrics/histogram.cc:645: for (int i =0; descriptions[i].description; ++i)
{
On 2012/10/31 00:35:19, Ilya Sherman wrote:
> nit: "=0" -> "= 0"

Done.

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

http://codereview.chromium.org/11342060/diff/1/base/metrics/histogram.h#newco...
base/metrics/histogram.h:597: // its "description" field.
On 2012/10/31 00:35:19, Ilya Sherman wrote:
> nit: You should document that |descriptions| can be null.

Done.

http://codereview.chromium.org/11342060/diff/1/base/metrics/histogram_base.cc
File base/metrics/histogram_base.cc (right):

http://codereview.chromium.org/11342060/diff/1/base/metrics/histogram_base.cc...
base/metrics/histogram_base.cc:35: break;
On 2012/10/31 00:35:19, Ilya Sherman wrote:
> nit: No need for any break stmts in this switch, since each case has a return
> stmt, except for the default case which is the last case.

Done.

http://codereview.chromium.org/11342060/diff/1/base/metrics/histogram_base.h
File base/metrics/histogram_base.h (right):

http://codereview.chromium.org/11342060/diff/1/base/metrics/histogram_base.h#...
base/metrics/histogram_base.h:22: // processes into the browser. If you create
another class inherits
On 2012/10/31 00:35:19, Ilya Sherman wrote:
> nit: "class inherits" -> "class that inherits from"

Done.

http://codereview.chromium.org/11342060/diff/1/base/metrics/histogram_base.h#...
base/metrics/histogram_base.h:33: std::string HistogramTypeName(HistogramType
type);
On 2012/10/31 00:35:19, Ilya Sherman wrote:
> Optional nit: Perhaps "HistogramTypeToString"?

Done.

[kaiwang, 2012-10-31 01:16:17]

Reviewers, please help to review the following dirs:

jar: base
yoz: chrome/browser/extensions
rvargas: net/disk_cache

Thanks a lot!

[Yoyo Zhou, 2012-10-31 01:38:48]

LGTM for extensions, but please remove using statements from metrics.cc
On Oct 30, 2012 6:16 PM, <kaiwang@chromium.org> wrote:

> Reviewers, please help to review the following dirs:
>
> jar: base
> yoz: chrome/browser/extensions
> rvargas: net/disk_cache
>
> Thanks a lot!
>
>
http://codereview.chromium.**org/11342060/<http://codereview.chromium.org/113...
>

[jar (doing other things), 2012-10-31 01:40:18]

lgtm

[rvargas (doing something else), 2012-10-31 02:07:01]

net lgtm

[commit-bot: I haz the power, 2012-10-31 02:38:05]

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

[kaiwang, 2012-10-31 02:38:34]

Thanks for all of your quick reviews!
Submitting...

[commit-bot: I haz the power, 2012-10-31 06:20:27]

Change committed as 165116