Make members of Singleton<T> private and only visible to the singleton type.

Make members of Singleton<T> private and only visible to the singleton type. This enforces that the Singleton<T> pattern can only be used within classes which want singleton-ness.

As part of this CL I have also fixed up files which got missed in my previous CLs to use a GetInstance() method and use Singleton<T> from the source file.

There are a small number of places where I have also switched to LazyInstance as that was more appropriate for types used in a single source file.

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

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

[M-A Ruel, 2010-12-10 14:52:18]

http://codereview.chromium.org/5682008/diff/2001/base/i18n/number_formatting.cc
File base/i18n/number_formatting.cc (right):

http://codereview.chromium.org/5682008/diff/2001/base/i18n/number_formatting....
base/i18n/number_formatting.cc:24: // This can cause problems if a different
allocator is used by this file than
80 cols

http://codereview.chromium.org/5682008/diff/2001/base/singleton.h
File base/singleton.h (left):

http://codereview.chromium.org/5682008/diff/2001/base/singleton.h#oldcode252
base/singleton.h:252: private:
Why make it public?

http://codereview.chromium.org/5682008/diff/2001/base/singleton.h
File base/singleton.h (right):

http://codereview.chromium.org/5682008/diff/2001/base/singleton.h#newcode163
base/singleton.h:163: // instantiated. It does no harm to instantiate it and use
it as a class member
Remove that too, we don't want to encourage multiple instantiation.

http://codereview.chromium.org/5682008/diff/2001/base/singleton.h#newcode199
base/singleton.h:199: friend typename FriendMaker<Type>::FriendType;
I think you don't need this utility class; try this instead:

typedef typename Type Type;
friend Type;

(Yes it looks redundant but it isn't)

http://codereview.chromium.org/5682008/diff/2001/base/singleton.h#newcode264
base/singleton.h:264: // Shortcuts.
Probably time to remove these shortcuts. Maybe worth doing in a separate change
first before this one?

http://codereview.chromium.org/5682008/diff/2001/base/singleton_unittest.cc
File base/singleton_unittest.cc (right):

http://codereview.chromium.org/5682008/diff/2001/base/singleton_unittest.cc#n...
base/singleton_unittest.cc:67: // Note that any type can be used; int, float,
std::wstring...
The second line of the comment shouldn't be at that place.

http://codereview.chromium.org/5682008/diff/2001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/signed_settings_helper.cc (right):

http://codereview.chromium.org/5682008/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/signed_settings_helper.cc:316: static
base::LazyInstance<SignedSettingsHelperImpl>
I highly prefer to not make these file local static.

http://codereview.chromium.org/5682008/diff/2001/net/base/bandwidth_metrics.cc
File net/base/bandwidth_metrics.cc (right):

http://codereview.chromium.org/5682008/diff/2001/net/base/bandwidth_metrics.c...
net/base/bandwidth_metrics.cc:14: : metrics_(g_bandwidth_metrics.Pointer()),
I'm wondering about the usefulness of the metrics_ variable at all. Unless it is
used for mocking but I didn't see a unit test at first glance.

http://codereview.chromium.org/5682008/diff/2001/net/base/cert_database_nss_u...
File net/base/cert_database_nss_unittest.cc (right):

http://codereview.chromium.org/5682008/diff/2001/net/base/cert_database_nss_u...
net/base/cert_database_nss_unittest.cc:106: virtual void SetUp() {
Isn't there already a base class that creates a temp dir for you? If not, it
would probably be a good idea but that's outside the scope of this test.

http://codereview.chromium.org/5682008/diff/2001/net/base/cert_database_nss_u...
net/base/cert_database_nss_unittest.cc:136: scoped_ptr<ScopedTempDir>
CertDatabaseNSSTest::temp_db_dir_;
And why do you think it's better to use a scoped_ptr here? Even if it's harmless
in a unit test, I'm afraid people would copy-paste it on real source code.

[Satish, 2010-12-10 15:04:57]

Thanks for the quick review.

http://codereview.chromium.org/5682008/diff/2001/base/singleton.h
File base/singleton.h (right):

http://codereview.chromium.org/5682008/diff/2001/base/singleton.h#newcode199
base/singleton.h:199: friend typename FriendMaker<Type>::FriendType;
On 2010/12/10 14:52:18, Marc-Antoine Ruel wrote:
> I think you don't need this utility class; try this instead:
> 
> typedef typename Type Type;
> friend Type;
> 
> (Yes it looks redundant but it isn't)

Doesn't work in GCC
"expected nested-name-specifier before 'Type'"

http://codereview.chromium.org/5682008/diff/2001/net/base/bandwidth_metrics.cc
File net/base/bandwidth_metrics.cc (right):

http://codereview.chromium.org/5682008/diff/2001/net/base/bandwidth_metrics.c...
net/base/bandwidth_metrics.cc:14: : metrics_(g_bandwidth_metrics.Pointer()),
On 2010/12/10 14:52:18, Marc-Antoine Ruel wrote:
> I'm wondering about the usefulness of the metrics_ variable at all. Unless it
is
> used for mocking but I didn't see a unit test at first glance.

I'll move the implementation of this class's methods to this cc file and with
that the metrics_ variable can be removed and g_bandwidth_metrics used instead.

http://codereview.chromium.org/5682008/diff/2001/net/base/cert_database_nss_u...
File net/base/cert_database_nss_unittest.cc (right):

http://codereview.chromium.org/5682008/diff/2001/net/base/cert_database_nss_u...
net/base/cert_database_nss_unittest.cc:136: scoped_ptr<ScopedTempDir>
CertDatabaseNSSTest::temp_db_dir_;
On 2010/12/10 14:52:18, Marc-Antoine Ruel wrote:
> And why do you think it's better to use a scoped_ptr here? Even if it's
harmless
> in a unit test, I'm afraid people would copy-paste it on real source code.

When getting rid of the singleton I considered either a LazyInstance or a
scopedptr, and scopedptr seemed to fit with the class. However it is different
from the singleton because the ScopedTempDir was earlier living across tests,
intentional or not. I can switch it to LazyInstance outside this class if you
think that is better.

[M-A Ruel, 2010-12-10 15:09:42]

http://codereview.chromium.org/5682008/diff/2001/base/singleton.h
File base/singleton.h (right):

http://codereview.chromium.org/5682008/diff/2001/base/singleton.h#newcode199
base/singleton.h:199: friend typename FriendMaker<Type>::FriendType;
On 2010/12/10 15:04:57, Satish wrote:
> On 2010/12/10 14:52:18, Marc-Antoine Ruel wrote:
> > I think you don't need this utility class; try this instead:
> > 
> > typedef typename Type Type;
> > friend Type;
> > 
> > (Yes it looks redundant but it isn't)
> 
> Doesn't work in GCC
> "expected nested-name-specifier before 'Type'"

Try this instead.
typedef typename Type Type;
friend typename Type;

If this still fails, I'll give up on this one.

http://codereview.chromium.org/5682008/diff/2001/net/base/bandwidth_metrics.cc
File net/base/bandwidth_metrics.cc (right):

http://codereview.chromium.org/5682008/diff/2001/net/base/bandwidth_metrics.c...
net/base/bandwidth_metrics.cc:14: : metrics_(g_bandwidth_metrics.Pointer()),
On 2010/12/10 15:04:57, Satish wrote:
> On 2010/12/10 14:52:18, Marc-Antoine Ruel wrote:
> > I'm wondering about the usefulness of the metrics_ variable at all. Unless
it
> is
> > used for mocking but I didn't see a unit test at first glance.
> 
> I'll move the implementation of this class's methods to this cc file and with
> that the metrics_ variable can be removed and g_bandwidth_metrics used
instead.

Cool

http://codereview.chromium.org/5682008/diff/2001/net/base/cert_database_nss_u...
File net/base/cert_database_nss_unittest.cc (right):

http://codereview.chromium.org/5682008/diff/2001/net/base/cert_database_nss_u...
net/base/cert_database_nss_unittest.cc:136: scoped_ptr<ScopedTempDir>
CertDatabaseNSSTest::temp_db_dir_;
On 2010/12/10 15:04:57, Satish wrote:
> On 2010/12/10 14:52:18, Marc-Antoine Ruel wrote:
> > And why do you think it's better to use a scoped_ptr here? Even if it's
> harmless
> > in a unit test, I'm afraid people would copy-paste it on real source code.
> 
> When getting rid of the singleton I considered either a LazyInstance or a
> scopedptr, and scopedptr seemed to fit with the class. However it is different
> from the singleton because the ScopedTempDir was earlier living across tests,
> intentional or not. I can switch it to LazyInstance outside this class if you
> think that is better.

I'm simply afraid of showing bad use cases. A global scoped_ptr is a bad
example.

[Satish, 2010-12-10 16:10:11]

http://codereview.chromium.org/5682008/diff/2001/base/singleton.h
File base/singleton.h (right):

http://codereview.chromium.org/5682008/diff/2001/base/singleton.h#newcode199
base/singleton.h:199: friend typename FriendMaker<Type>::FriendType;
On 2010/12/10 15:09:42, Marc-Antoine Ruel wrote:
> On 2010/12/10 15:04:57, Satish wrote:
> > On 2010/12/10 14:52:18, Marc-Antoine Ruel wrote:
> > > I think you don't need this utility class; try this instead:
> > > 
> > > typedef typename Type Type;
> > > friend Type;
> > > 
> > > (Yes it looks redundant but it isn't)
> > 
> > Doesn't work in GCC
> > "expected nested-name-specifier before 'Type'"
> 
> Try this instead.
> typedef typename Type Type;
> friend typename Type;
> 
> If this still fails, I'll give up on this one.

Yes I tried that too :) didn't work. I'll keep this one as is for now

http://codereview.chromium.org/5682008/diff/2001/net/base/cert_database_nss_u...
File net/base/cert_database_nss_unittest.cc (right):

http://codereview.chromium.org/5682008/diff/2001/net/base/cert_database_nss_u...
net/base/cert_database_nss_unittest.cc:136: scoped_ptr<ScopedTempDir>
CertDatabaseNSSTest::temp_db_dir_;
On 2010/12/10 15:09:42, Marc-Antoine Ruel wrote:
> On 2010/12/10 15:04:57, Satish wrote:
> > On 2010/12/10 14:52:18, Marc-Antoine Ruel wrote:
> > > And why do you think it's better to use a scoped_ptr here? Even if it's
> > harmless
> > > in a unit test, I'm afraid people would copy-paste it on real source code.
> > 
> > When getting rid of the singleton I considered either a LazyInstance or a
> > scopedptr, and scopedptr seemed to fit with the class. However it is
different
> > from the singleton because the ScopedTempDir was earlier living across
tests,
> > intentional or not. I can switch it to LazyInstance outside this class if
you
> > think that is better.
> 
> I'm simply afraid of showing bad use cases. A global scoped_ptr is a bad
> example.

I agree, though this is really just declaring the class's static variable it
could be just used as a global during cut/paste. But a global LazyInstance would
stay alive after these tests finish. I thought the //static comment was
sufficient to identify this is not a plain global. Do you have any suggestion on
what I should change this to?

[M-A Ruel, 2010-12-10 16:18:36]

http://codereview.chromium.org/5682008/diff/2001/net/base/cert_database_nss_u...
File net/base/cert_database_nss_unittest.cc (right):

http://codereview.chromium.org/5682008/diff/2001/net/base/cert_database_nss_u...
net/base/cert_database_nss_unittest.cc:136: scoped_ptr<ScopedTempDir>
CertDatabaseNSSTest::temp_db_dir_;
On 2010/12/10 16:10:11, Satish wrote:
> I agree, though this is really just declaring the class's static variable it
> could be just used as a global during cut/paste. But a global LazyInstance
would
> stay alive after these tests finish. I thought the //static comment was
> sufficient to identify this is not a plain global. Do you have any suggestion
on
> what I should change this to?

A class member variable. Creating the directory on each unit test doesn't take
long and reduce inter-tests side-effects. I don't think there's any need for any
global state here.

[Satish, 2010-12-10 17:13:47]

New patch uploaded addressing all comments so far.

http://codereview.chromium.org/5682008/diff/2001/base/i18n/number_formatting.cc
File base/i18n/number_formatting.cc (right):

http://codereview.chromium.org/5682008/diff/2001/base/i18n/number_formatting....
base/i18n/number_formatting.cc:24: // This can cause problems if a different
allocator is used by this file than
On 2010/12/10 14:52:18, Marc-Antoine Ruel wrote:
> 80 cols

Done.

http://codereview.chromium.org/5682008/diff/2001/base/singleton.h
File base/singleton.h (left):

http://codereview.chromium.org/5682008/diff/2001/base/singleton.h#oldcode252
base/singleton.h:252: private:
On 2010/12/10 14:52:18, Marc-Antoine Ruel wrote:
> Why make it public?

This is still private, along with the rest of this template.

http://codereview.chromium.org/5682008/diff/2001/base/singleton.h
File base/singleton.h (right):

http://codereview.chromium.org/5682008/diff/2001/base/singleton.h#newcode163
base/singleton.h:163: // instantiated. It does no harm to instantiate it and use
it as a class member
On 2010/12/10 14:52:18, Marc-Antoine Ruel wrote:
> Remove that too, we don't want to encourage multiple instantiation.

Done.

http://codereview.chromium.org/5682008/diff/2001/base/singleton.h#newcode264
base/singleton.h:264: // Shortcuts.
On 2010/12/10 14:52:18, Marc-Antoine Ruel wrote:
> Probably time to remove these shortcuts. Maybe worth doing in a separate
change
> first before this one?

Done.

http://codereview.chromium.org/5682008/diff/2001/base/singleton_unittest.cc
File base/singleton_unittest.cc (right):

http://codereview.chromium.org/5682008/diff/2001/base/singleton_unittest.cc#n...
base/singleton_unittest.cc:67: // Note that any type can be used; int, float,
std::wstring...
On 2010/12/10 14:52:18, Marc-Antoine Ruel wrote:
> The second line of the comment shouldn't be at that place.

I removed this line since it doesn't make sense with the current restrictions

http://codereview.chromium.org/5682008/diff/2001/net/base/bandwidth_metrics.cc
File net/base/bandwidth_metrics.cc (right):

http://codereview.chromium.org/5682008/diff/2001/net/base/bandwidth_metrics.c...
net/base/bandwidth_metrics.cc:14: : metrics_(g_bandwidth_metrics.Pointer()),
On 2010/12/10 15:04:57, Satish wrote:
> On 2010/12/10 14:52:18, Marc-Antoine Ruel wrote:
> > I'm wondering about the usefulness of the metrics_ variable at all. Unless
it
> is
> > used for mocking but I didn't see a unit test at first glance.
> 
> I'll move the implementation of this class's methods to this cc file and with
> that the metrics_ variable can be removed and g_bandwidth_metrics used
instead.

Done.

http://codereview.chromium.org/5682008/diff/2001/net/base/cert_database_nss_u...
File net/base/cert_database_nss_unittest.cc (right):

http://codereview.chromium.org/5682008/diff/2001/net/base/cert_database_nss_u...
net/base/cert_database_nss_unittest.cc:136: scoped_ptr<ScopedTempDir>
CertDatabaseNSSTest::temp_db_dir_;
On 2010/12/10 16:18:37, Marc-Antoine Ruel wrote:
> On 2010/12/10 16:10:11, Satish wrote:
> > I agree, though this is really just declaring the class's static variable it
> > could be just used as a global during cut/paste. But a global LazyInstance
> would
> > stay alive after these tests finish. I thought the //static comment was
> > sufficient to identify this is not a plain global. Do you have any
suggestion
> on
> > what I should change this to?
> 
> A class member variable. Creating the directory on each unit test doesn't take
> long and reduce inter-tests side-effects. I don't think there's any need for
any
> global state here.

Done.

[M-A Ruel, 2010-12-10 17:20:20]

lgtm

[rvargas (doing something else), 2010-12-10 20:55:54]

drive by.

http://codereview.chromium.org/5682008/diff/16001/net/disk_cache/file_win.cc
File net/disk_cache/file_win.cc (right):

http://codereview.chromium.org/5682008/diff/16001/net/disk_cache/file_win.cc#...
net/disk_cache/file_win.cc:9: #include "base/lazy_instance.h"
nit: order of includes.

http://codereview.chromium.org/5682008/diff/16001/net/disk_cache/file_win.cc#...
net/disk_cache/file_win.cc:36: static ::base::LazyInstance<CompletionHandler>
g_completion_handler(
nit: remove the initial ::

[Satish, 2010-12-13 17:24:16]

The Clang compiler did not like the code Patch 3, singleton.h. So I had to
change the way Singleton<T> restricts access.

The latest patch expects a GetInstance() method to be present in the template
Type and declares that as a friend to give access. I have updated bulk of the
code in another CL earlier to follow this GetInstance() convention.

I also saw that LazyInstance was missing a proper AtExit handler like how
Singleton<T> has. This was causing test failures because when tests invoke
AtExitHandler manually, the LazyInstance destructor was invoked as expected but
the internal state was not reset. So subsequent accesses returned a stale
object/pointer. I have fixed that now in the style of Singleton<T>.

Looking at the diff of Patch 4 vs. Patch 3 in this CL indicates all these
changes clearly (except some parts where other CLs which came in the middle show
up as deletes which isn't true).

Please take another look. Thanks.

[M-A Ruel, 2010-12-13 17:30:50]

On 2010/12/13 17:24:16, Satish wrote:
> The Clang compiler did not like the code Patch 3, singleton.h. So I had to
> change the way Singleton<T> restricts access.

Ah clang... Maybe you should file a bug report?
 
> The latest patch expects a GetInstance() method to be present in the template
> Type and declares that as a friend to give access. I have updated bulk of the
> code in another CL earlier to follow this GetInstance() convention.

Looks nicer and is less code.

lgtm

http://codereview.chromium.org/5682008/diff/41001/base/lazy_instance.h
File base/lazy_instance.h (right):

http://codereview.chromium.org/5682008/diff/41001/base/lazy_instance.h#newcod...
base/lazy_instance.h:130: if (Traits::Delete != NULL)  // Will be null for
LeakyLazyInstannceTraits
LeakyLazyInstanceTraits

[M-A Ruel, 2010-12-13 17:32:56]

[Focus failure when pressing enter]

I added Evan and Nico in case they want to send a bug upstream about the issue
Satish faced with the template friend stuff.

[Satish, 2010-12-13 17:34:40]

On 2010/12/13 17:32:56, Marc-Antoine Ruel wrote:
> [Focus failure when pressing enter]
> 
> I added Evan and Nico in case they want to send a bug upstream about the issue
> Satish faced with the template friend stuff.

Nico knows, he was the one who pointed out the build failure. I also came across
http://stackoverflow.com/questions/702650/making-a-template-parameter-a-friend
which says the earlier code I used was in fact invalid c++ :) So all seems well
now.

[M-A Ruel, 2010-12-13 17:35:24]

On 2010/12/13 17:34:40, Satish wrote:
> On 2010/12/13 17:32:56, Marc-Antoine Ruel wrote:
> > [Focus failure when pressing enter]
> > 
> > I added Evan and Nico in case they want to send a bug upstream about the
issue
> > Satish faced with the template friend stuff.
> 
> Nico knows, he was the one who pointed out the build failure. I also came
across
> http://stackoverflow.com/questions/702650/making-a-template-parameter-a-friend
> which says the earlier code I used was in fact invalid c++ :) So all seems
well
> now.

Cool, sorry for the noise.

[Evan Martin, 2010-12-13 17:38:30]

I don't think it matters much, but Chrome style is to put file-local variables
in an anonymous namespace block rather than tagging them with "static".  I put
stars by a few instances I saw where you could either just remove the "static"
or remove it and move the decl up a few lines.

Again, I don't think it matters much, but if you end up needing to do another
pass through all of these instances you might consider fixing it.

http://codereview.chromium.org/5682008/diff/41001/base/i18n/number_formatting.cc
File base/i18n/number_formatting.cc (right):

http://codereview.chromium.org/5682008/diff/41001/base/i18n/number_formatting...
base/i18n/number_formatting.cc:36: static LazyInstance<NumberFormatWrapper>
g_number_format(LINKER_INITIALIZED);
*

http://codereview.chromium.org/5682008/diff/41001/base/path_service.cc
File base/path_service.cc (right):

http://codereview.chromium.org/5682008/diff/41001/base/path_service.cc#newcod...
base/path_service.cc:121: static base::LazyInstance<PathData>
g_path_data(base::LINKER_INITIALIZED);
*

http://codereview.chromium.org/5682008/diff/41001/base/worker_pool_mac.mm
File base/worker_pool_mac.mm (right):

http://codereview.chromium.org/5682008/diff/41001/base/worker_pool_mac.mm#new...
base/worker_pool_mac.mm:47: static base::LazyInstance<NSOperationQueueWrapper>
g_nsoperation_queue(
*

http://codereview.chromium.org/5682008/diff/41001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/screen_locker.cc (right):

http://codereview.chromium.org/5682008/diff/41001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screen_locker.cc:197: static
base::LazyInstance<ScreenLockObserver> g_screen_lock_observer(
*

http://codereview.chromium.org/5682008/diff/41001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/signed_settings_helper.cc (right):

http://codereview.chromium.org/5682008/diff/41001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/signed_settings_helper.cc:269: static
base::LazyInstance<SignedSettingsHelperImpl>
*

[Nico, 2010-12-13 17:38:46]

On Mon, Dec 13, 2010 at 9:34 AM,  <satish@chromium.org> wrote:
> On 2010/12/13 17:32:56, Marc-Antoine Ruel wrote:
>>
>> [Focus failure when pressing enter]
>
>> I added Evan and Nico in case they want to send a bug upstream about the
>> issue
>> Satish faced with the template friend stuff.
>
> Nico knows, he was the one who pointed out the build failure. I also came
> across
> http://stackoverflow.com/questions/702650/making-a-template-parameter-a-friend
> which says the earlier code I used was in fact invalid c++ :) So all seems
> well now.

For posterity, that discussion happened at
http://groups.google.com/a/chromium.org/group/clang/browse_thread/thread/e88c...

>
> http://codereview.chromium.org/5682008/
>