Eliminate CrosLibrary::EnsureLoaded

Eliminate CrosLibrary::EnsureLoaded


BUG=chromium-os:19886
TEST=All chromeos tests compile and run.

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

[stevenjb, 2011-11-16 22:59:40]

We almost always wanted to use libcros_loaded() where we were using
EnsureLoaded(), and in some cases we didn't need a check at all; all libcros
libraries now fall back to stub if we fail to load libcros.

Things to check for:
* Any call that call directly into libcros, or might fail on non chromeos OS's
should be protected by libcros_loaded().
* Calls to chromeos::CrosLibrary::Get()->GetFooLibrary() don't need to be
protected. The stub implementation should Do The Right Thing. There are a few
cases where stub behavior may be incorrect (e.g. a callback may not get called),
so we check for libcros_loaded() anyway.

[DaveMoore, 2011-11-16 23:31:22]

lgtm

[satorux1, 2011-11-16 23:51:34]

Thank you for the clean-up.

I think libcros_loaded() is unnecessary in many places, or should be replaced
with system::runtime_environment::IsRunningOnChromeOS()

http://codereview.chromium.org/8588006/diff/2001/chrome/browser/automation/te...
File chrome/browser/automation/testing_automation_provider_chromeos.cc (right):

http://codereview.chromium.org/8588006/diff/2001/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider_chromeos.cc:58: return
true;
Can we remove the function? This is a wrapper around EnsureLoaded()

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

http://codereview.chromium.org/8588006/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.cc:643: if
(CrosLibrary::Get()->libcros_loaded()) {
IsRunningOnChrome() instead, or remove it?

The code inside the block doesn't seem to depend on libcros.

http://codereview.chromium.org/8588006/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.cc:808: if
(CrosLibrary::Get()->libcros_loaded()) {
ditto.

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

http://codereview.chromium.org/8588006/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/version_info_updater.cc:46: if
(CrosLibrary::Get()->libcros_loaded()) {
The version loadeder doesn't seem to depend on libcros. Maybe we should check
system::runtime_environment::IsRunningOnChromeOS() instead, or simply remove the
check.

http://codereview.chromium.org/8588006/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/version_info_updater.cc:81: if
(!CrosLibrary::Get()->libcros_loaded()) {
ditto.

http://codereview.chromium.org/8588006/diff/2001/chrome/browser/chromeos/sms_...
File chrome/browser/chromeos/sms_observer.cc (right):

http://codereview.chromium.org/8588006/diff/2001/chrome/browser/chromeos/sms_...
chrome/browser/chromeos/sms_observer.cc:31: if
(!CrosLibrary::Get()->libcros_loaded())
This is a valid check. Let's add some comment like

// Guard chromeos::DisconnectSMSMonitor()

http://codereview.chromium.org/8588006/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_info_private_api_chromeos.cc (right):

http://codereview.chromium.org/8588006/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_info_private_api_chromeos.cc:62: if
(CrosLibrary::Get()->libcros_loaded()) {
looks unnecessary

http://codereview.chromium.org/8588006/diff/2001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_manager.cc (right):

http://codereview.chromium.org/8588006/diff/2001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager.cc:426: if
(chromeos::CrosLibrary::Get()->libcros_loaded() &&
I think we don't need this, as
chromeos::CrosLibrary::Get()->GetCryptohomeLibrary() should work, right?

http://codereview.chromium.org/8588006/diff/2001/chrome/browser/ui/browser_li...
File chrome/browser/ui/browser_list.cc (right):

http://codereview.chromium.org/8588006/diff/2001/chrome/browser/ui/browser_li...
chrome/browser/ui/browser_list.cc:230: if
(chromeos::CrosLibrary::Get()->libcros_loaded()
we don't need to call this, as it's also checked in
BrowserList::NotifyAndTerminate()

http://codereview.chromium.org/8588006/diff/2001/chrome/browser/ui/browser_li...
chrome/browser/ui/browser_list.cc:322: if (cros_library->libcros_loaded()) {
I think system::runtime_environment::IsRunningOnChromeOS() should be used
instead

http://codereview.chromium.org/8588006/diff/2001/chrome/browser/ui/browser_li...
chrome/browser/ui/browser_list.cc:333: // If running the Chrome OS build, but
we're not on the device, fall through
per this comment.

http://codereview.chromium.org/8588006/diff/2001/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/register_page_ui.cc (right):

http://codereview.chromium.org/8588006/diff/2001/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/register_page_ui.cc:235: if
(chromeos::CrosLibrary::Get()->libcros_loaded()) {
IsRunningOnChromeOS()?

http://codereview.chromium.org/8588006/diff/2001/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/register_page_ui.cc:241:
SkipRegistration("CrosLibrary is not loaded.");
"Not running on Chrome OS" ?

[stevenjb, 2011-11-17 00:23:33]

http://codereview.chromium.org/8588006/diff/2001/chrome/browser/automation/te...
File chrome/browser/automation/testing_automation_provider_chromeos.cc (right):

http://codereview.chromium.org/8588006/diff/2001/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider_chromeos.cc:58: return
true;
On 2011/11/16 23:51:34, satorux1 wrote:
> Can we remove the function? This is a wrapper around EnsureLoaded()
Yes, that looks safe to do.
Done.

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

http://codereview.chromium.org/8588006/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.cc:643: if
(CrosLibrary::Get()->libcros_loaded()) {
On 2011/11/16 23:51:34, satorux1 wrote:
> IsRunningOnChrome() instead, or remove it?
> 
> The code inside the block doesn't seem to depend on libcros.

I'm not sure what might break with login / tests if we run this on linux, so
will use system::runtime_environment::IsRunningOnChromeOS() for now.

http://codereview.chromium.org/8588006/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.cc:808: if
(CrosLibrary::Get()->libcros_loaded()) {
On 2011/11/16 23:51:34, satorux1 wrote:
> ditto.
ditto.

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

http://codereview.chromium.org/8588006/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/version_info_updater.cc:46: if
(CrosLibrary::Get()->libcros_loaded()) {
On 2011/11/16 23:51:34, satorux1 wrote:
> The version loadeder doesn't seem to depend on libcros. Maybe we should check
> system::runtime_environment::IsRunningOnChromeOS() instead, or simply remove
the
> check.

Will use system::runtime_environment::IsRunningOnChromeOS().  Didn't want to
mess with the version messaging on linux for now.

http://codereview.chromium.org/8588006/diff/2001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/version_info_updater.cc:81: if
(!CrosLibrary::Get()->libcros_loaded()) {
On 2011/11/16 23:51:34, satorux1 wrote:
> ditto.
ditto.

http://codereview.chromium.org/8588006/diff/2001/chrome/browser/chromeos/sms_...
File chrome/browser/chromeos/sms_observer.cc (right):

http://codereview.chromium.org/8588006/diff/2001/chrome/browser/chromeos/sms_...
chrome/browser/chromeos/sms_observer.cc:31: if
(!CrosLibrary::Get()->libcros_loaded())
On 2011/11/16 23:51:34, satorux1 wrote:
> This is a valid check. Let's add some comment like
> 
> // Guard chromeos::DisconnectSMSMonitor()

Done.

http://codereview.chromium.org/8588006/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_info_private_api_chromeos.cc (right):

http://codereview.chromium.org/8588006/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_info_private_api_chromeos.cc:62: if
(CrosLibrary::Get()->libcros_loaded()) {
On 2011/11/16 23:51:34, satorux1 wrote:
> looks unnecessary

Done.

http://codereview.chromium.org/8588006/diff/2001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_manager.cc (right):

http://codereview.chromium.org/8588006/diff/2001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager.cc:426: if
(chromeos::CrosLibrary::Get()->libcros_loaded() &&
On 2011/11/16 23:51:34, satorux1 wrote:
> I think we don't need this, as
> chromeos::CrosLibrary::Get()->GetCryptohomeLibrary() should work, right?
Double checked that IsMounted() returns |true| in the stub, so removed the test.

http://codereview.chromium.org/8588006/diff/2001/chrome/browser/ui/browser_li...
File chrome/browser/ui/browser_list.cc (right):

http://codereview.chromium.org/8588006/diff/2001/chrome/browser/ui/browser_li...
chrome/browser/ui/browser_list.cc:230: if
(chromeos::CrosLibrary::Get()->libcros_loaded()
On 2011/11/16 23:51:34, satorux1 wrote:
> we don't need to call this, as it's also checked in
> BrowserList::NotifyAndTerminate()
But it does affect the return value, so I didn't want to mess with it yet. Did
replace with system::runtime_environment::IsRunningOnChromeOS() though.

http://codereview.chromium.org/8588006/diff/2001/chrome/browser/ui/browser_li...
chrome/browser/ui/browser_list.cc:322: if (cros_library->libcros_loaded()) {
On 2011/11/16 23:51:34, satorux1 wrote:
> I think system::runtime_environment::IsRunningOnChromeOS() should be used
> instead 

Done.

http://codereview.chromium.org/8588006/diff/2001/chrome/browser/ui/browser_li...
chrome/browser/ui/browser_list.cc:333: // If running the Chrome OS build, but
we're not on the device, fall through
On 2011/11/16 23:51:34, satorux1 wrote:
> per this comment.

Done.

http://codereview.chromium.org/8588006/diff/2001/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/register_page_ui.cc (right):

http://codereview.chromium.org/8588006/diff/2001/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/register_page_ui.cc:235: if
(chromeos::CrosLibrary::Get()->libcros_loaded()) {
On 2011/11/16 23:51:34, satorux1 wrote:
> IsRunningOnChromeOS()?

Done.

http://codereview.chromium.org/8588006/diff/2001/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/register_page_ui.cc:241:
SkipRegistration("CrosLibrary is not loaded.");
On 2011/11/16 23:51:34, satorux1 wrote:
> "Not running on Chrome OS" ?

Done.

[satorux1, 2011-11-17 00:37:59]

LGTM.

Please include chrome/browser/chromeos/system/runtime_environment.h from files
where IsRunningOnChromeOS() is used, as needed.

[commit-bot: I haz the power, 2011-11-17 08:24:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/8588006/4007

[commit-bot: I haz the power, 2011-11-17 08:24:52]

Can't apply patch for file
chrome/browser/chromeos/extensions/file_browser_event_router.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/chromeos/extensions/file_browser_event_router.cc
Hunk #1 FAILED at 95.
Hunk #2 FAILED at 107.
2 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/chromeos/extensions/file_browser_event_router.cc.rej

[commit-bot: I haz the power, 2011-11-17 08:41:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/8588006/5007

[commit-bot: I haz the power, 2011-11-17 08:41:25]

Can't apply patch for file
chrome/browser/chromeos/extensions/file_browser_event_router.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/chromeos/extensions/file_browser_event_router.cc
Hunk #1 FAILED at 95.
Hunk #2 FAILED at 107.
2 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/chromeos/extensions/file_browser_event_router.cc.rej

[Nikita (slow), 2011-11-17 11:10:44]

chromeos/login lgtm

[commit-bot: I haz the power, 2011-11-17 17:05:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/8588006/4010

[commit-bot: I haz the power, 2011-11-17 17:05:55]

Presubmit check for 8588006-4010 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
chrome/browser/policy/browser_policy_connector.cc

Presubmit checks took 3.5s to calculate.