Initialize StatisticsRecorder in Child Process to hold

Initialize StatisticsRecorder in Child Process to hold
all histograms in child processes.

Made StatisticsRecorder a leaky lazy singleton.

R=jar@chromium.org, jam@chromium.org
BUG=114013
TEST=renderer histograms should still work.

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

[ramant (doing other things), 2012-07-18 01:13:31]

Hi Jim and John Abd-El-Malek,
  Small change to initialize histogram statistics gathering system in all child
processes. Verified that the browser process is getting histograms from
renderer, plugin and GPU processes. I hope this is the right place to make this
change (would appreciate your advise on where this could be moved if this is not
the correct place).

thanks much,
raman

[jam, 2012-07-18 01:22:01]

lgtm

http://codereview.chromium.org/10779040/diff/17001/content/common/child_proce...
File content/common/child_process.cc (right):

http://codereview.chromium.org/10779040/diff/17001/content/common/child_proce...
content/common/child_process.cc:60: statistics_recorder_.reset();
nit: is this necessary? if not, leave it out

[ramant (doing other things), 2012-07-18 03:20:32]

Hi Jim and John Abd-El-Malek,
  Made StatisticsRecorder a leaky lazy singleton instance (per discussion with
Jim). Would appreciate comments.

thanks,
raman

http://codereview.chromium.org/10779040/diff/17001/content/common/child_proce...
File content/common/child_process.cc (right):

http://codereview.chromium.org/10779040/diff/17001/content/common/child_proce...
content/common/child_process.cc:60: statistics_recorder_.reset();
On 2012/07/18 01:22:06, John Abd-El-Malek wrote:
> nit: is this necessary? if not, leave it out

Done.

[jam, 2012-07-18 05:14:16]

http://codereview.chromium.org/10779040/diff/20009/base/metrics/statistics_re...
File base/metrics/statistics_recorder.h (right):

http://codereview.chromium.org/10779040/diff/20009/base/metrics/statistics_re...
base/metrics/statistics_recorder.h:30: StatisticsRecorder();
if it's a singleton, then the constructor and destructor should be private

[jar (doing other things), 2012-07-18 16:00:01]

http://codereview.chromium.org/10779040/diff/20009/base/metrics/statistics_re...
File base/metrics/statistics_recorder.h (right):

http://codereview.chromium.org/10779040/diff/20009/base/metrics/statistics_re...
base/metrics/statistics_recorder.h:30: StatisticsRecorder();
I think this was done to facilitate testability.

In tests, the instance is constructed and destroyed, but in regular use, it is
not.

Would you argue for private constructor and destructor, and then add FriendTest
etc. for testing?

Truth be known, there are minimal (no?) ill effects to additional constructions.
The construction is (effectively) done for side effects (initializing a group of
statics, including a lock, in a well defined order).  The singleton is used in
the browser to support race-free (permanent) initialization.

On 2012/07/18 05:14:17, John Abd-El-Malek wrote:
> if it's a singleton, then the constructor and destructor should be private

[jam, 2012-07-18 16:36:11]

http://codereview.chromium.org/10779040/diff/20009/base/metrics/statistics_re...
File base/metrics/statistics_recorder.h (right):

http://codereview.chromium.org/10779040/diff/20009/base/metrics/statistics_re...
base/metrics/statistics_recorder.h:30: StatisticsRecorder();
On 2012/07/18 16:00:01, jar wrote:
> I think this was done to facilitate testability.
> 
> In tests, the instance is constructed and destroyed, but in regular use, it is
> not.
> 
> Would you argue for private constructor and destructor, and then add
FriendTest
> etc. for testing?

I think that would be better, because that enforces that no production code
creates another instance of this class by mistake.

> 
> Truth be known, there are minimal (no?) ill effects to additional
constructions.
> The construction is (effectively) done for side effects (initializing a group
of
> statics, including a lock, in a well defined order).  The singleton is used in
> the browser to support race-free (permanent) initialization.
> 
> On 2012/07/18 05:14:17, John Abd-El-Malek wrote:
> > if it's a singleton, then the constructor and destructor should be private
>

[ramant (doing other things), 2012-07-18 18:54:34]

Made StatisticsRecorder's constructor and destructor private and added
FRIEND_TEST for histogram_unittest (which was testing StatisticsRecorder).

thanks,
raman

http://codereview.chromium.org/10779040/diff/20009/base/metrics/statistics_re...
File base/metrics/statistics_recorder.h (right):

http://codereview.chromium.org/10779040/diff/20009/base/metrics/statistics_re...
base/metrics/statistics_recorder.h:30: StatisticsRecorder();
On 2012/07/18 16:36:12, John Abd-El-Malek wrote:
> On 2012/07/18 16:00:01, jar wrote:
> > I think this was done to facilitate testability.
> > 
> > In tests, the instance is constructed and destroyed, but in regular use, it
is
> > not.
> > 
> > Would you argue for private constructor and destructor, and then add
> FriendTest
> > etc. for testing?
> 
> I think that would be better, because that enforces that no production code
> creates another instance of this class by mistake.
> 
> > 
> > Truth be known, there are minimal (no?) ill effects to additional
> constructions.
> > The construction is (effectively) done for side effects (initializing a
group
> of
> > statics, including a lock, in a well defined order).  The singleton is used
in
> > the browser to support race-free (permanent) initialization.
> > 
> > On 2012/07/18 05:14:17, John Abd-El-Malek wrote:
> > > if it's a singleton, then the constructor and destructor should be private
> > 
> 

Done.

[jam, 2012-07-19 07:59:01]

lgtm

[jar (doing other things), 2012-07-19 17:30:43]

lgtm

[commit-bot: I haz the power, 2012-07-19 17:31:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/10779040/26004

[commit-bot: I haz the power, 2012-07-19 19:17:34]

Change committed as 147501