cros: Remove the unneed default ctor from ScreenLockLibrary interface.

cros: Remove the unneed default ctor from ScreenLockLibrary interface.

And while I'm here some header clean ups.

BUG=None
TEST=None

R=stevenjb@chromium.org

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

[tfarina, 2011-07-28 00:10:52]

http://codereview.chromium.org/7524014/diff/2001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/screen_lock_library.cc (right):

http://codereview.chromium.org/7524014/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/screen_lock_library.cc:25:
chromeos::DisconnectScreenLock(screen_lock_connection_);
After disconnecting, the PowerLibraryImpl sets its power_status_connection_ to
NULL. Shouldn't we do the same here?

[tfarina, 2011-07-28 01:00:49]

http://codereview.chromium.org/7524014/diff/6001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/screen_lock_library.cc (right):

http://codereview.chromium.org/7524014/diff/6001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/screen_lock_library.cc:122: void
AddObserver(Observer* observer) {}
These all needs "virtual" and OVERRIDE. And the above methods too. For a follow
up patch though.

[tfarina, 2011-07-28 01:05:25]

On 2011/07/28 00:10:52, tfarina wrote:
>
http://codereview.chromium.org/7524014/diff/2001/chrome/browser/chromeos/cros...
> File chrome/browser/chromeos/cros/screen_lock_library.cc (right):
> 
>
http://codereview.chromium.org/7524014/diff/2001/chrome/browser/chromeos/cros...
> chrome/browser/chromeos/cros/screen_lock_library.cc:25:
> chromeos::DisconnectScreenLock(screen_lock_connection_);
> After disconnecting, the PowerLibraryImpl sets its power_status_connection_ to
> NULL. Shouldn't we do the same here?

If I'm not wrong, looking more carefully now, I think PowerLibraryImpl is wrong,
since the object is allocated on the stack and thus wouldn't need to set the
power_status_connection_ to NULL nor initialize it to NULL in the ctor
initializer list?

[stevenjb, 2011-07-28 01:23:20]

On 2011/07/28 01:05:25, tfarina wrote:
> On 2011/07/28 00:10:52, tfarina wrote:
> >
>
http://codereview.chromium.org/7524014/diff/2001/chrome/browser/chromeos/cros...
> > File chrome/browser/chromeos/cros/screen_lock_library.cc (right):
> > 
> >
>
http://codereview.chromium.org/7524014/diff/2001/chrome/browser/chromeos/cros...
> > chrome/browser/chromeos/cros/screen_lock_library.cc:25:
> > chromeos::DisconnectScreenLock(screen_lock_connection_);
> > After disconnecting, the PowerLibraryImpl sets its power_status_connection_
to
> > NULL. Shouldn't we do the same here?
> 
> If I'm not wrong, looking more carefully now, I think PowerLibraryImpl is
wrong,
> since the object is allocated on the stack and thus wouldn't need to set the
> power_status_connection_ to NULL nor initialize it to NULL in the ctor
> initializer list?

Setting a member variable to NULL in the destructor is generally redundant,
however sometimes in complex destructors this is done to prevent access to the
deleted member variable later in the destructor (e.g. in case a cleanup function
gets added to the destructor that also deletes the same value). In a small
destructor like this, I don't think it matters either way. In this case,
screen_lock_connection_ is a handle instead of a raw pointer, so it should
already be protected from double deletion anyway.

[stevenjb, 2011-07-28 01:23:30]

http://codereview.chromium.org/7524014/diff/6001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/screen_lock_library.cc (right):

http://codereview.chromium.org/7524014/diff/6001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/screen_lock_library.cc:20: if
(CrosLibrary::Get()->EnsureLoaded())
Init() should not get called in the constructor, it should get called from
GetImpl().
Also, we should move the EnsureLoaded() check into Init() surrounding the
libcros call.

http://codereview.chromium.org/7524014/diff/6001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/screen_lock_library.h (right):

http://codereview.chromium.org/7524014/diff/6001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/screen_lock_library.h:22: 
nit: While an empty constructor is not obligatory, providing one is self
documenting that no special initialization is performed by the base class.

[tfarina, 2011-07-28 01:37:28]

http://codereview.chromium.org/7524014/diff/6001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/screen_lock_library.cc (right):

http://codereview.chromium.org/7524014/diff/6001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/screen_lock_library.cc:20: if
(CrosLibrary::Get()->EnsureLoaded())
On 2011/07/28 01:23:30, Steven Bennetts wrote:
> Init() should not get called in the constructor, it should get called from
> GetImpl().

By this, you mean:
ScreeLockLibrary* ScreenLockLibrary::GetImpl(bool stub) {
  if (stub)
    return new ScreenLockLibraryStubImpl();
  else {
    ScreenLockLibraryImpl* lib = new ScreenLockLibraryImpl();
    lib->Init();
    return lib;
  }
}

> Also, we should move the EnsureLoaded() check into Init() surrounding the
> libcros call.

[stevenjb, 2011-07-28 01:44:06]

On 2011/07/28 01:37:28, tfarina wrote:
>
http://codereview.chromium.org/7524014/diff/6001/chrome/browser/chromeos/cros...
> File chrome/browser/chromeos/cros/screen_lock_library.cc (right):
> 
>
http://codereview.chromium.org/7524014/diff/6001/chrome/browser/chromeos/cros...
> chrome/browser/chromeos/cros/screen_lock_library.cc:20: if
> (CrosLibrary::Get()->EnsureLoaded())
> On 2011/07/28 01:23:30, Steven Bennetts wrote:
> > Init() should not get called in the constructor, it should get called from
> > GetImpl().
> 
> By this, you mean:
> ScreeLockLibrary* ScreenLockLibrary::GetImpl(bool stub) {
>   if (stub)
>     return new ScreenLockLibraryStubImpl();
>   else {
>     ScreenLockLibraryImpl* lib = new ScreenLockLibraryImpl();
>     lib->Init();
>     return lib;
>   }
> }
> 
> > Also, we should move the EnsureLoaded() check into Init() surrounding the
> > libcros call.

Yes, although it might be cleaner to make Init() a virtual in the base class and
do this:

ScreenLockLibrary* impl;
if (stub)
  impl = new ScreenLockLibraryStubImpl();
else
  impl = new ScreenLockLibraryImpl();
impl->Init();
return impl;

[tfarina, 2011-07-28 01:47:04]

On 2011/07/28 01:44:06, Steven Bennetts wrote:
> On 2011/07/28 01:37:28, tfarina wrote:
> >
>
http://codereview.chromium.org/7524014/diff/6001/chrome/browser/chromeos/cros...
> > File chrome/browser/chromeos/cros/screen_lock_library.cc (right):
> > 
> >
>
http://codereview.chromium.org/7524014/diff/6001/chrome/browser/chromeos/cros...
> > chrome/browser/chromeos/cros/screen_lock_library.cc:20: if
> > (CrosLibrary::Get()->EnsureLoaded())
> > On 2011/07/28 01:23:30, Steven Bennetts wrote:
> > > Init() should not get called in the constructor, it should get called from
> > > GetImpl().
> > 
> > By this, you mean:
> > ScreeLockLibrary* ScreenLockLibrary::GetImpl(bool stub) {
> >   if (stub)
> >     return new ScreenLockLibraryStubImpl();
> >   else {
> >     ScreenLockLibraryImpl* lib = new ScreenLockLibraryImpl();
> >     lib->Init();
> >     return lib;
> >   }
> > }
> > 
> > > Also, we should move the EnsureLoaded() check into Init() surrounding the
> > > libcros call.
> 
> Yes, although it might be cleaner to make Init() a virtual in the base class
and
> do this:
> 
> ScreenLockLibrary* impl;
> if (stub)
>   impl = new ScreenLockLibraryStubImpl();
> else
>   impl = new ScreenLockLibraryImpl();
> impl->Init();
> return impl;

Sure, I'm fine either way. I'm just concerned that we would be exposing an
implementation detail through the public API, and I'm not sure if it's worth (it
doesn't matter much either way I think).

[tfarina, 2011-07-28 01:52:32]

On 2011/07/28 01:44:06, Steven Bennetts wrote:
> Yes, although it might be cleaner to make Init() a virtual in the base class
and
> do this:
> 
> ScreenLockLibrary* impl;
> if (stub)
>   impl = new ScreenLockLibraryStubImpl();
> else
>   impl = new ScreenLockLibraryImpl();
> impl->Init();
> return impl;

Please, see patch set 8, for this implementation.

[stevenjb (google-dont-use), 2011-07-28 02:17:52]

Typically any non trivial initialization is performed in an Init() function.
This function has the same visibility as the constructor (i.e. public for a
public constructor) so that whatever calls the Init() function has access to
it. Here it's a pretty trivial Init() call, but it does call into libcros,
so it's probably better to use this model instead of folding the call into
the constructor.

On Wed, Jul 27, 2011 at 6:47 PM, <tfarina@chromium.org> wrote:

> On 2011/07/28 01:44:06, Steven Bennetts wrote:
>
>> On 2011/07/28 01:37:28, tfarina wrote:
>> >
>>
>
> http://codereview.chromium.**org/7524014/diff/6001/chrome/**
>
browser/chromeos/cros/screen_**lock_library.cc<http://codereview.chromium.org/7524014/diff/6001/chrome/browser/chromeos/cros/screen_lock_library.cc>
>
>> > File chrome/browser/chromeos/cros/**screen_lock_library.cc (right):
>> >
>> >
>>
>
> http://codereview.chromium.**org/7524014/diff/6001/chrome/**
>
browser/chromeos/cros/screen_**lock_library.cc#newcode20<http://codereview.chromium.org/7524014/diff/6001/chrome/browser/chromeos/cros/screen_lock_library.cc#newcode20>
>
>> > chrome/browser/chromeos/cros/**screen_lock_library.cc:20: if
>> > (CrosLibrary::Get()->**EnsureLoaded())
>> > On 2011/07/28 01:23:30, Steven Bennetts wrote:
>> > > Init() should not get called in the constructor, it should get called
>> from
>> > > GetImpl().
>> >
>> > By this, you mean:
>> > ScreeLockLibrary* ScreenLockLibrary::GetImpl(**bool stub) {
>> >   if (stub)
>> >     return new ScreenLockLibraryStubImpl();
>> >   else {
>> >     ScreenLockLibraryImpl* lib = new ScreenLockLibraryImpl();
>> >     lib->Init();
>> >     return lib;
>> >   }
>> > }
>> >
>> > > Also, we should move the EnsureLoaded() check into Init() surrounding
>> the
>> > > libcros call.
>>
>
>  Yes, although it might be cleaner to make Init() a virtual in the base
>> class
>>
> and
>
>> do this:
>>
>
>  ScreenLockLibrary* impl;
>> if (stub)
>>   impl = new ScreenLockLibraryStubImpl();
>> else
>>   impl = new ScreenLockLibraryImpl();
>> impl->Init();
>> return impl;
>>
>
> Sure, I'm fine either way. I'm just concerned that we would be exposing an
> implementation detail through the public API, and I'm not sure if it's
> worth (it
> doesn't matter much either way I think).
>
>
>
http://codereview.chromium.**org/7524014/<http://codereview.chromium.org/7524...
>

[stevenjb, 2011-07-28 02:18:58]

LGTM