Add explicit tests for histogram factory matching.

Add explicit tests for histogram factory matching.

Two instances of the same histogram has worked since landed with http://src.chromium.org/viewvc/chrome?view=rev&revision=33933 , but wasn't tested. This CL adds a short test that makes sure we have this functionality.

In order to keep the tests isolated from each other (so they don't inadvertantly start depending on each other), we have to add a test fixture to initialize and clear a StatisticsRecorder for each test.

R=jar@chromium.org,rvargas@chromium.org
BUG=None


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

[gavinp, 2012-10-26 17:35:03]

Ricardo, over in http://codereview.chromium.org/11267028/ you asked if
histograms can be instantiated twice in the same place. I wasn't sure before
writing comment #4, so I wrote a quick test. Here it is in unit test form.

jar: can you take a look? I also put a few minor cleanups that made sense to do
at the same time.

[jar (doing other things), 2012-10-26 17:51:40]

lgtm

[rvargas (doing something else), 2012-10-26 18:25:23]

I think if we want to support that model (and not consider it an implementation
detail), there should be a mention/example of at the top of histogram.h

Other than that, lgtm.

[gavinp, 2012-10-26 20:55:50]

On 2012/10/26 18:25:23, rvargas wrote:
> I think if we want to support that model (and not consider it an
implementation
> detail), there should be a mention/example of at the top of histogram.h
> 
> Other than that, lgtm.

See the third sentence of the final paragraph in the header file comment.

This functionality is old (nearly three years), and depended on throughout
Chrome. It is not an implementation detail. I count at least sixty histograms
that depend on this; if we regress on this we'll have bigger problems than this
unit test.

[commit-bot: I haz the power, 2012-10-26 21:01:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/11272044/1

[rvargas (doing something else), 2012-10-26 21:09:21]

On 2012/10/26 20:55:50, gavinp wrote:
> On 2012/10/26 18:25:23, rvargas wrote:
> > I think if we want to support that model (and not consider it an
> implementation
> > detail), there should be a mention/example of at the top of histogram.h
> > 
> > Other than that, lgtm.
> 
> See the third sentence of the final paragraph in the header file comment.
> 
> This functionality is old (nearly three years), and depended on throughout
> Chrome. It is not an implementation detail. I count at least sixty histograms
> that depend on this; if we regress on this we'll have bigger problems than
this
> unit test.

I read the hole comment before commenting here... and I'm not saying it is new
functionality, I actually looked at the CLs that implemented it... but the
documentation is 100% about avoiding races on multithread scenarios (and that's
also my recollection about the change), not about the having multiple histograms
around the code. I still think the class documentation should mention the use
case.

[Ilya Sherman, 2012-10-26 22:15:49]

Drive-by nit

https://chromiumcodereview.appspot.com/11272044/diff/1/base/metrics/histogram...
File base/metrics/histogram_unittest.cc (right):

https://chromiumcodereview.appspot.com/11272044/diff/1/base/metrics/histogram...
base/metrics/histogram_unittest.cc:37: statistics_recorder_ = new
StatisticsRecorder();
nit: y no scoped_ptr?

[gavinp, 2012-10-26 23:01:49]

https://chromiumcodereview.appspot.com/11272044/diff/1/base/metrics/histogram...
File base/metrics/histogram_unittest.cc (right):

https://chromiumcodereview.appspot.com/11272044/diff/1/base/metrics/histogram...
base/metrics/histogram_unittest.cc:37: statistics_recorder_ = new
StatisticsRecorder();
On 2012/10/26 22:15:50, Ilya Sherman wrote:
> nit: y no scoped_ptr?

There are two answers. The first is that the code I cut and paste from
statistics_recorder_unittest uses raw pointers, and I didn't question that as I
did the cut and paste.

The second is that the StatisticsRecorder has a private destructor (because
normally there's only one), and it explicitly friends the unit tests to allow
them to delete it on a narrower per test scope. So I think we should let this
sleeping dog lie, since I can't make scoped_ptr<StatisticsRecorder> a friend of
StatisticsRecorder without effectively making the destructor public.

[commit-bot: I haz the power, 2012-10-26 23:03:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/11272044/13001

[commit-bot: I haz the power, 2012-10-27 12:22:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/11272044/13001

[commit-bot: I haz the power, 2012-10-27 12:44:09]

Change committed as 164536