cros: Apply the Init() model to BrightnessLibrary API.

cros: Apply the Init() model to BrightnessLibrary API.

This applies the model introduced here http://codereview.chromium.org/7524014/,
at revision r94482.

Init() should not get called in the constructor, it should get called from
GetImpl().
Also, move the EnsureLoaded() check into Init() surrounding the libcros call.

BUG=None
TEST=None

R=stevenjb@chromium.org

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

[stevenjb, 2011-07-29 19:19:15]

http://codereview.chromium.org/7493061/diff/1003/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/brightness_library.cc (right):

http://codereview.chromium.org/7493061/diff/1003/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/brightness_library.cc:19: BrightnessLibraryImpl()
{}
Still need to initialize brightness_connection_ to NULL

http://codereview.chromium.org/7493061/diff/1003/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/brightness_library.cc:28:
DCHECK(!brightness_connection_) << "Already intialized";
This should either be a CHECK, or we should call DisconnectBrightness on an
existing connection (or just return).

[tfarina, 2011-07-29 19:44:22]

http://codereview.chromium.org/7493061/diff/1003/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/brightness_library.cc (right):

http://codereview.chromium.org/7493061/diff/1003/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/brightness_library.cc:19: BrightnessLibraryImpl()
{}
On 2011/07/29 19:19:15, Steven Bennetts wrote:
> Still need to initialize brightness_connection_ to NULL

Like in ScreenLockLibraryImpl, we didn't need this because
brightness_connection_ is allocated on the stack.

http://codereview.chromium.org/7493061/diff/1003/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/brightness_library.cc:28:
DCHECK(!brightness_connection_) << "Already intialized";
On 2011/07/29 19:19:15, Steven Bennetts wrote:
> This should either be a CHECK, or we should call DisconnectBrightness on an
> existing connection (or just return).

Changed to CHECK.

[stevenjb, 2011-07-29 19:56:30]

On 2011/07/29 19:44:22, tfarina wrote:
>
http://codereview.chromium.org/7493061/diff/1003/chrome/browser/chromeos/cros...
> File chrome/browser/chromeos/cros/brightness_library.cc (right):
> 
>
http://codereview.chromium.org/7493061/diff/1003/chrome/browser/chromeos/cros...
> chrome/browser/chromeos/cros/brightness_library.cc:19: BrightnessLibraryImpl()
> {}
> On 2011/07/29 19:19:15, Steven Bennetts wrote:
> > Still need to initialize brightness_connection_ to NULL
> 
> Like in ScreenLockLibraryImpl, we didn't need this because
> brightness_connection_ is allocated on the stack.
> 
I don't understand. Maybe I missed this in another review, but even though
brightness_connection_ is set in Init(), it should still be initialized to a
known value in the constructor. chromeos::BrightnessConnection is typedefed to 
OpaqueBrightnessConnection*; values without constructors should always be
initialized in the the class constructor.

>
http://codereview.chromium.org/7493061/diff/1003/chrome/browser/chromeos/cros...
> chrome/browser/chromeos/cros/brightness_library.cc:28:
> DCHECK(!brightness_connection_) << "Already intialized";
> On 2011/07/29 19:19:15, Steven Bennetts wrote:
> > This should either be a CHECK, or we should call DisconnectBrightness on an
> > existing connection (or just return).
> 
> Changed to CHECK.

[tfarina, 2011-07-29 20:17:59]

On 2011/07/29 19:56:30, Steven Bennetts wrote:
> I don't understand. Maybe I missed this in another review, but even though
> brightness_connection_ is set in Init(), it should still be initialized to a
> known value in the constructor. chromeos::BrightnessConnection is typedefed to

> OpaqueBrightnessConnection*; values without constructors should always be
> initialized in the the class constructor.
> 
Yeah, you are absolutely right. I missed that BrightnessConnection is declared
as:
typedef OpaqueBrightnessConnection* BrightnessConnection;
So, brightness_connection_ is a pointer and should be initialized with NULL in
the ctor initializer list. And, yes, we missed that in the ScreenLockLibrary,
which should be fixed as well.
Patch set 3, fixed this problem here. Please, take another look.

[stevenjb, 2011-07-29 20:26:08]

LGTM
Thanks again for doing this cleanup!