Add a new GetInstance() method for singleton classes under chrome/service and /net.

Add a new GetInstance() method for singleton classes under chrome/service and /net.

This is a small step towards making all singleton classes use the Singleton<T> pattern within their code and not expect the callers to know about it.

This CL includes files under chrome/service and /net with related files elsewhere.

Suggested files to focus for reviewers:
- @sanjeevr for chrome/common and chrome/service
- @ukai for net/websockets
- @agl for rest of net

BUG=65298
TEST=all existing tests should continue to pass.

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

[agl, 2010-12-07 21:11:48]

I think this is ok, but you need to get willchan to review really.

[Satish, 2010-12-07 21:13:50]

Adding willchan@ to the review as recommended by agl@.

[willchan no longer on Chromium, 2010-12-08 00:58:46]

LGTM with nits.

http://codereview.chromium.org/5634005/diff/2001/chrome/common/service_proces...
File chrome/common/service_process_util.h (right):

http://codereview.chromium.org/5634005/diff/2001/chrome/common/service_proces...
chrome/common/service_process_util.h:56: // Returns false is another service
process is already running.
'is' should be 'if'.

http://codereview.chromium.org/5634005/diff/2001/chrome/common/service_proces...
chrome/common/service_process_util.h:73: private:
Add a newline.  This is recommended in
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Class_Format
which says "Except for the first instance, these keywords should be preceded by
a blank line. This rule is optional in small classes."

http://codereview.chromium.org/5634005/diff/2001/chrome/common/service_proces...
File chrome/common/service_process_util_unittest.cc (right):

http://codereview.chromium.org/5634005/diff/2001/chrome/common/service_proces...
chrome/common/service_process_util_unittest.cc:45: ServiceProcessState* state =
ServiceProcessState::GetInstance();
Are you sure this is recommended?  What do we do with the service process tests?
 Do we run all the same tests in the same process, with the same singleton
instance?  If so, then you may run into ordering issues.  What if another test
runs first that tries to access the ServiceProcessState singleton?  Will calling
Initialize() twice work properly?  I'm not sure this is advisable.  You might
want to revert to the same test code and make the constructor public again, or
friend the TEST_F.

http://codereview.chromium.org/5634005/diff/2001/net/socket_stream/socket_str...
File net/socket_stream/socket_stream_job_manager.h (right):

http://codereview.chromium.org/5634005/diff/2001/net/socket_stream/socket_str...
net/socket_stream/socket_stream_job_manager.h:36: typedef std::map<std::string,
SocketStreamJob::ProtocolFactory*> FactoryMap;
Types go before function declarations (including constructors / destructors).  I
generally move the friend declarations first too.

[ukai, 2010-12-08 01:44:14]

LGTM with nits.

http://codereview.chromium.org/5634005/diff/2001/net/spdy/spdy_stream.cc
File net/spdy/spdy_stream.cc (right):

http://codereview.chromium.org/5634005/diff/2001/net/spdy/spdy_stream.cc#newc...
net/spdy/spdy_stream.cc:39: ScopedBandwidthMetrics::ScopedBandwidthMetrics()
don't we have net/base/bandwidth_metrics.cc ?

I don't think it's good to have an implementation in net/spdy for class defined
in net/base

[willchan no longer on Chromium, 2010-12-08 04:02:47]

http://codereview.chromium.org/5634005/diff/2001/net/spdy/spdy_stream.cc
File net/spdy/spdy_stream.cc (right):

http://codereview.chromium.org/5634005/diff/2001/net/spdy/spdy_stream.cc#newc...
net/spdy/spdy_stream.cc:39: ScopedBandwidthMetrics::ScopedBandwidthMetrics()
On 2010/12/08 01:44:15, ukai wrote:
> don't we have net/base/bandwidth_metrics.cc ?
> 
> I don't think it's good to have an implementation in net/spdy for class
defined
> in net/base

Wow, I totally missed this.  I agree completely.

[Satish, 2010-12-08 12:58:22]

All comments addressed. Will wait for your LGTMs again before submitting.

http://codereview.chromium.org/5634005/diff/2001/chrome/common/service_proces...
File chrome/common/service_process_util.h (right):

http://codereview.chromium.org/5634005/diff/2001/chrome/common/service_proces...
chrome/common/service_process_util.h:56: // Returns false is another service
process is already running.
On 2010/12/08 00:58:46, willchan wrote:
> 'is' should be 'if'.

Done.

http://codereview.chromium.org/5634005/diff/2001/chrome/common/service_proces...
chrome/common/service_process_util.h:73: private:
On 2010/12/08 00:58:46, willchan wrote:
> Add a newline.  This is recommended in
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Class_Format
> which says "Except for the first instance, these keywords should be preceded
by
> a blank line. This rule is optional in small classes."

Done.

http://codereview.chromium.org/5634005/diff/2001/chrome/common/service_proces...
File chrome/common/service_process_util_unittest.cc (right):

http://codereview.chromium.org/5634005/diff/2001/chrome/common/service_proces...
chrome/common/service_process_util_unittest.cc:45: ServiceProcessState* state =
ServiceProcessState::GetInstance();
On 2010/12/08 00:58:46, willchan wrote:
> Are you sure this is recommended?  What do we do with the service process
tests?
>  Do we run all the same tests in the same process, with the same singleton
> instance?  If so, then you may run into ordering issues.  What if another test
> runs first that tries to access the ServiceProcessState singleton?  Will
calling
> Initialize() twice work properly?  I'm not sure this is advisable.  You might
> want to revert to the same test code and make the constructor public again, or
> friend the TEST_F.

Yes this is bound to have issues later on. I have now added a test fixture and
used base::ShadowingAtExitManager to ensure that this singleton gets created and
destroyed within each test and there is no sharing of state between two tests.
This model is used in other unit tests too.

http://codereview.chromium.org/5634005/diff/2001/net/socket_stream/socket_str...
File net/socket_stream/socket_stream_job_manager.h (right):

http://codereview.chromium.org/5634005/diff/2001/net/socket_stream/socket_str...
net/socket_stream/socket_stream_job_manager.h:36: typedef std::map<std::string,
SocketStreamJob::ProtocolFactory*> FactoryMap;
On 2010/12/08 00:58:46, willchan wrote:
> Types go before function declarations (including constructors / destructors). 
I
> generally move the friend declarations first too.

Done.

http://codereview.chromium.org/5634005/diff/2001/net/spdy/spdy_stream.cc
File net/spdy/spdy_stream.cc (right):

http://codereview.chromium.org/5634005/diff/2001/net/spdy/spdy_stream.cc#newc...
net/spdy/spdy_stream.cc:39: ScopedBandwidthMetrics::ScopedBandwidthMetrics()
On 2010/12/08 01:44:15, ukai wrote:
> don't we have net/base/bandwidth_metrics.cc ?
> 
> I don't think it's good to have an implementation in net/spdy for class
defined
> in net/base

We didn't have net/base/bandwidth_metrics.cc and I have added it now with just
this constructor.

[willchan no longer on Chromium, 2010-12-08 18:09:41]

http://codereview.chromium.org/5634005/diff/13001/net/socket_stream/socket_st...
File net/socket_stream/socket_stream_job_manager.h (right):

http://codereview.chromium.org/5634005/diff/13001/net/socket_stream/socket_st...
net/socket_stream/socket_stream_job_manager.h:12: #include "base/singleton.h"
As joth mentioned in another code review, I think you should forward declare
rather than #include.

[Satish, 2010-12-08 22:06:39]

http://codereview.chromium.org/5634005/diff/13001/net/socket_stream/socket_st...
File net/socket_stream/socket_stream_job_manager.h (right):

http://codereview.chromium.org/5634005/diff/13001/net/socket_stream/socket_st...
net/socket_stream/socket_stream_job_manager.h:12: #include "base/singleton.h"
On 2010/12/08 18:09:41, willchan wrote:
> As joth mentioned in another code review, I think you should forward declare
> rather than #include.

Done.

[ukai, 2010-12-09 00:20:44]

LGTM