Use stub impl when libcros fails to load

Use stub impl when libcros fails to load

Previous behavior would either:
a. Successfully load libcros. CrosLibrary::EnsureLoaded() returns true. Calls to libcros succeed.
b. Fail to load libcros. CrosLibrary::EnsureLoaded() returns false. Login is blocked. Calls to libcros fail (crash) if not protected and login is skipped for debugging.
c. Load a stub implementation if --stub-cros is specified. CrosLibrary::EnsureLoaded() returns true. Calls to libcros fail (crash).

Problems with this behavior:
1. CrosLibrary::EnsureLoaded() would be used incorrectly to protect calls to libcros outside of CrosLibrary non-stub implementations.
2. CrosLibrary non-stub implementations would still be called if libcros failed to load, so all calls to libcros in the non-stub implementations needed to be protected.

New behavior:
a. Successfully load libcros. CrosLibrary::libcros_loaded() returns true. Calls to libcros succeed.
b. Fail to load libcros. CrosLibrary::libcros_loaded() returns false. Stub implementation is used instead. load_error_string() specifies an error and login is blocked.
c. Load a stub implementation if --stub-cros is specified. CrosLibrary::libcros_loaded() returns false. Stub implementation is used.

BUG=chromium-os:20350
TEST=Test chrome for chromeos on linux and chromeos, with and without --stub-cros

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

[stevenjb, 2011-09-15 23:13:08]

This isn't as bad as it looks, most of the changes are small.

Dave, please be sure to look at the changes to power_library and
power_menu_button.cc.

The test failures appear to be unrelated.

[satorux1, 2011-09-16 20:05:55]

Thank you for doing this! I'm very happy to see EnsureLoaded() to be gone.

I like this change, but I think we should make it a fatal error if we failed to
load libcros on the chromebook. Other than that, looks good.

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

http://codereview.chromium.org/7891021/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/brightness_library.cc:50:
chromeos::IncreaseScreenBrightness();
Indentation is off?

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

http://codereview.chromium.org/7891021/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/cros_library.cc:68: << " Will use stub
implementations.";
This is okay for Linux desktop, but I think this should be a fatal error on the
chromebook. Can we do something like

if (g_cros_library->LoadLibcros(&error)) {
 VLOG(1) << "CrosLibrary Initialized, version = " << kCrosAPIVersion;
} else if (error == LIBCROS_NOT_FOUND) {
   // This is likely on Linux desktop so go ahead with a warning
} else {
  LOG(FATAL) << ...
}

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

http://codereview.chromium.org/7891021/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/mount_library.cc:820:
DISABLE_RUNNABLE_METHOD_REFCOUNT(chromeos::MountLibraryImpl);
Do we need this? RunnableMethod doesn't seem to be used in the file.

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

http://codereview.chromium.org/7891021/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/power_library.cc:194: battery_percentage_(80),
This seems to be unrelated to the refactoring. What about doing this in a
separate patch?

http://codereview.chromium.org/7891021/diff/2001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/power_library.h (right):

http://codereview.chromium.org/7891021/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/power_library.h:72: virtual void
RequestStatusUpdate() = 0;
Is this a new functionality introduced? If so, wouldn't it be better to do this
in a separate patch?

[stevenjb, 2011-09-16 21:44:24]

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

http://codereview.chromium.org/7891021/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/brightness_library.cc:50:
chromeos::IncreaseScreenBrightness();
On 2011/09/16 20:05:55, satorux1 wrote:
> Indentation is off?

Done.

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

http://codereview.chromium.org/7891021/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/cros_library.cc:68: << " Will use stub
implementations.";
I actually prefer to use a warning here. We already block login on ChromeOS, and
the network menu UI already shows a warning icon when running with a stub impl.

On 2011/09/16 20:05:55, satorux1 wrote:
> This is okay for Linux desktop, but I think this should be a fatal error on
the
> chromebook. Can we do something like
> 
> if (g_cros_library->LoadLibcros(&error)) {
>  VLOG(1) << "CrosLibrary Initialized, version = " << kCrosAPIVersion;
> } else if (error == LIBCROS_NOT_FOUND) {
>    // This is likely on Linux desktop so go ahead with a warning
> } else {
>   LOG(FATAL) << ...
> }

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

http://codereview.chromium.org/7891021/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/mount_library.cc:820:
DISABLE_RUNNABLE_METHOD_REFCOUNT(chromeos::MountLibraryImpl);
On 2011/09/16 20:05:55, satorux1 wrote:
> Do we need this? RunnableMethod doesn't seem to be used in the file.

Good catch; done.

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

http://codereview.chromium.org/7891021/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/power_library.cc:194: battery_percentage_(80),
On 2011/09/16 20:05:55, satorux1 wrote:
> This seems to be unrelated to the refactoring. What about doing this in a
> separate patch?
Will do.

http://codereview.chromium.org/7891021/diff/2001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/power_library.h (right):

http://codereview.chromium.org/7891021/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/power_library.h:72: virtual void
RequestStatusUpdate() = 0;
On 2011/09/16 20:05:55, satorux1 wrote:
> Is this a new functionality introduced? If so, wouldn't it be better to do
this
> in a separate patch?
Will do.

[satorux1, 2011-09-16 21:59:43]

LGTM, but one question below:

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

http://codereview.chromium.org/7891021/diff/2001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/cros_library.cc:68: << " Will use stub
implementations.";
On 2011/09/16 21:44:24, Steven Bennetts wrote:
> I actually prefer to use a warning here. We already block login on ChromeOS,
and
> the network menu UI already shows a warning icon when running with a stub
impl.
> 
> On 2011/09/16 20:05:55, satorux1 wrote:
> > This is okay for Linux desktop, but I think this should be a fatal error on
> the
> > chromebook. Can we do something like
> > 
> > if (g_cros_library->LoadLibcros(&error)) {
> >  VLOG(1) << "CrosLibrary Initialized, version = " << kCrosAPIVersion;
> > } else if (error == LIBCROS_NOT_FOUND) {
> >    // This is likely on Linux desktop so go ahead with a warning
> > } else {
> >   LOG(FATAL) << ...
> > }
> 

I misunderstood. I thought you changed this from a fatal error to a warning, but
the original code was also using LOG(WARNING).

I thought we had a mechanism to show libcros mismatch on the login screen. Where
is the code, should we modify the code?

[stevenjb, 2011-09-17 00:31:33]

There is a mechanism for that in the login code. I think that the code that
displays it is in VersionInfoUpdater::UpdateVersionLabel(). I'm not sure where
we block login on a load error. The existing logic seems fine - normal updates
will be prevented from logging in on an incompatible version, but running chrome
from the command line will still work (with a stub impl) if libcros can not be
loaded.

On 2011/09/16 21:59:43, satorux1 wrote:

> LGTM, but one question below:
> 
>
http://codereview.chromium.org/7891021/diff/2001/chrome/browser/chromeos/cros...
> File chrome/browser/chromeos/cros/cros_library.cc (right):
> 
>
http://codereview.chromium.org/7891021/diff/2001/chrome/browser/chromeos/cros...
> chrome/browser/chromeos/cros/cros_library.cc:68: << " Will use stub
> implementations.";
> On 2011/09/16 21:44:24, Steven Bennetts wrote:
> > I actually prefer to use a warning here. We already block login on ChromeOS,
> and
> > the network menu UI already shows a warning icon when running with a stub
> impl.
> > 
> > On 2011/09/16 20:05:55, satorux1 wrote:
> > > This is okay for Linux desktop, but I think this should be a fatal error
on
> > the
> > > chromebook. Can we do something like
> > > 
> > > if (g_cros_library->LoadLibcros(&error)) {
> > >  VLOG(1) << "CrosLibrary Initialized, version = " << kCrosAPIVersion;
> > > } else if (error == LIBCROS_NOT_FOUND) {
> > >    // This is likely on Linux desktop so go ahead with a warning
> > > } else {
> > >   LOG(FATAL) << ...
> > > }
> > 
> 
> I misunderstood. I thought you changed this from a fatal error to a warning,
but
> the original code was also using LOG(WARNING).
> 
> I thought we had a mechanism to show libcros mismatch on the login screen.
Where
> is the code, should we modify the code?

[stevenjb, 2011-09-19 18:56:24]

PTAL
Rebased with the power_library changes from
http://codereview.chromium.org/7920019/. Only power_library.cc and
power_menu_button.cc have non rebase changes from the previous patchset.

[satorux1, 2011-09-19 19:29:29]

LGTM.

The patch description looks a bit too brief. Would be nice to add some comment
about why we are doing this and what we are archiving from this?