chromeos: Add power supply info reading capability

chrome/browser/chromeos/cros/power_library.cc

Index: chrome/browser/chromeos/cros/power_library.cc
diff --git a/chrome/browser/chromeos/cros/power_library.cc b/chrome/browser/chromeos/cros/power_library.cc
index 6ebb11df7d26e2fb8b8157e4af2a1b1e651c1d1e..9fc7e63b5c3c3cf233f6df9f81e58d22c82d582f 100644
--- a/chrome/browser/chromeos/cros/power_library.cc
+++ b/chrome/browser/chromeos/cros/power_library.cc
@@ -5,12 +5,14 @@
 #include "chrome/browser/chromeos/cros/power_library.h"
 
 #include "base/basictypes.h"
+#include "base/bind.h"
 #include "base/compiler_specific.h"
 #include "base/logging.h"
 #include "base/observer_list.h"
 #include "base/time.h"
 #include "base/timer.h"
 #include "chrome/browser/chromeos/cros/cros_library.h"
+#include "chrome/browser/chromeos/dbus/dbus_thread_manager.h"
 #include "content/browser/browser_thread.h"
 #include "third_party/cros/chromeos_power.h"
 #include "third_party/cros/chromeos_resume.h"
@@ -20,16 +22,11 @@ namespace chromeos {
 class PowerLibraryImpl : public PowerLibrary {
  public:
   PowerLibraryImpl()
-      : power_status_connection_(NULL),
-        resume_status_connection_(NULL),
+      : resume_status_connection_(NULL),
         status_(chromeos::PowerStatus()) {
   }
 
   virtual ~PowerLibraryImpl() {
-    if (power_status_connection_) {
-      chromeos::DisconnectPowerStatus(power_status_connection_);
-      power_status_connection_ = NULL;
-    }
     if (resume_status_connection_) {
       chromeos::DisconnectResume(resume_status_connection_);
       resume_status_connection_ = NULL;
@@ -39,8 +36,6 @@ class PowerLibraryImpl : public PowerLibrary {
   // Begin PowerLibrary implementation.
   virtual void Init() OVERRIDE {
     DCHECK(CrosLibrary::Get()->libcros_loaded());
-    power_status_connection_ =
-        chromeos::MonitorPowerStatus(&PowerStatusChangedHandler, this);
     resume_status_connection_ =
         chromeos::MonitorResume(&SystemResumedHandler, this);
   }
@@ -84,14 +79,9 @@ class PowerLibraryImpl : public PowerLibrary {
   }
 
   virtual void EnableScreenLock(bool enable) OVERRIDE {
-    // Make sure we run on FILE thread becuase chromeos::EnableScreenLock
+    // Make sure we run on FILE thread because chromeos::EnableScreenLock
     // would write power manager config file to disk.
-    if (!BrowserThread::CurrentlyOn(BrowserThread::FILE)) {
-      BrowserThread::PostTask(
-          BrowserThread::FILE, FROM_HERE,
-          NewRunnableMethod(this, &PowerLibraryImpl::EnableScreenLock, enable));
-      return;
-    }
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
     chromeos::EnableScreenLock(enable);
   }
@@ -110,6 +100,20 @@ class PowerLibraryImpl : public PowerLibrary {
     // when we migrate the DBus code from libcros to here.
   }
 
+  // PowerManagerClient::Observer implementation.
+  virtual void UpdatePowerStatus(const chromeos::PowerStatus& status) OVERRIDE {
+    // Make this is being called from UI thread.
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+    DVLOG(1) << "Power lpo=" << status.line_power_on

[satorux1, 2011/10/14 05:56:28]

Please just use VLOG(1). DVLOG is rarely useful.

[Simon Que, 2011/10/18 19:23:06]

Since this is just being moved, can I make this change (and the next few
suggestions) in another patch?

+             << " sta=" << status.battery_state

[satorux1, 2011/10/14 05:56:28]

We eschew abbreviations like "sta" per our C++ style guide:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml

+             << " per=" << status.battery_percentage
+             << " tte=" << status.battery_time_to_empty
+             << " ttf=" << status.battery_time_to_full;

[satorux1, 2011/10/14 05:56:28]

I'd suggest to add ToString() function to PowerStatus, and change code to
something like

VLOG(1) << "PowerStatus: " << status.ToString();

+    status_ = status;
+    FOR_EACH_OBSERVER(Observer, observers_, PowerChanged(this));

[satorux1, 2011/10/14 05:56:28]

This looks rather redundant.

UpdatePowerStatus() is called from PowerManagerClient via FOR_EACH_OBSERVER(),
and then again notifying other observers here by FOR_EACH_OBSERVER(). Would be
simpler to get rid of this layer.

However, the redundancy is currently necessary as you need to update |status_|
here. but I think you can remove |status_| by getting rid of all getters like 
battery_fully_charged(). Instead of exposing these getters, I think you can pass
|status| to PowerChanged() like PowerChanged(status) instead of
PowerChanged(this).

Seems to me that the observer is implemented in PowerMenuButton like

void PowerMenuButton::PowerChanged(PowerLibrary* obj) {
  UpdateIconAndLabelInfo();
}

and |obj| is unsed as you see. In UpdateIconAndLabelInfo(), getters like 
 line_power_on() are used, but these callers can be removed if you pass |status|
to UpdateIconAndLabelInfo().

I'd suggest to refactor code around |status_| as a separate patch and submit it
before this patch.

[Simon Que, 2011/10/18 21:09:34]

What about in
src/chrome/browser/automation/testing_automation_provider_chromeos.cc?  It still
depends on those Get() functions...

[satorux1, 2011/10/18 22:07:14]

Ugh, you are right. We cannot get rid of the getters. For now, forget about my
comments about getting getters...

I think ToString() comment is still valid.

+  }
+
   // End PowerLibrary implementation.
 
  private:
@@ -139,43 +143,15 @@ class PowerLibraryImpl : public PowerLibrary {
     power->SystemResumed();
   }
 
-  void UpdatePowerStatus(const chromeos::PowerStatus& status) {
-    // Make sure we run on UI thread.
-    if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
-      BrowserThread::PostTask(
-          BrowserThread::UI, FROM_HERE,
-          NewRunnableMethod(
-              this, &PowerLibraryImpl::UpdatePowerStatus, status));
-      return;
-    }
-
-    DVLOG(1) << "Power lpo=" << status.line_power_on
-             << " sta=" << status.battery_state
-             << " per=" << status.battery_percentage
-             << " tte=" << status.battery_time_to_empty
-             << " ttf=" << status.battery_time_to_full;
-    status_ = status;
-    FOR_EACH_OBSERVER(Observer, observers_, PowerChanged(this));
-  }
-
   void SystemResumed() {
     // Make sure we run on the UI thread.
-    if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
-      BrowserThread::PostTask(
-          BrowserThread::UI, FROM_HERE,
-          NewRunnableMethod(this, &PowerLibraryImpl::SystemResumed));
-      return;
-    }
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
     FOR_EACH_OBSERVER(Observer, observers_, SystemResumed());
   }
 
   ObserverList<Observer> observers_;
 
-  // A reference to the power battery API, to allow callbacks when the battery
-  // status changes.
-  chromeos::PowerStatusConnection power_status_connection_;
-
   // A reference to the resume alerts.
   chromeos::ResumeConnection resume_status_connection_;
 
@@ -260,6 +236,9 @@ class PowerLibraryStubImpl : public PowerLibrary {
     }
   }
 
+  virtual void UpdatePowerStatus(const chromeos::PowerStatus& status) OVERRIDE {
+  }
+
   // End PowerLibrary implementation.
 
  private: