Added asynchronous notification of readiness to the StatisticsProvider, and

chrome/browser/chromeos/system/statistics_provider.cc

Index: chrome/browser/chromeos/system/statistics_provider.cc
diff --git a/chrome/browser/chromeos/system/statistics_provider.cc b/chrome/browser/chromeos/system/statistics_provider.cc
index 28118635c75ba22ebcadc78211511a041f5f2bff..fcfd52b261a562351153b54c150e42a7990645c2 100644
--- a/chrome/browser/chromeos/system/statistics_provider.cc
+++ b/chrome/browser/chromeos/system/statistics_provider.cc
@@ -10,6 +10,9 @@
 #include "base/file_util.h"
 #include "base/logging.h"
 #include "base/memory/singleton.h"
+#include "base/memory/weak_ptr.h"
+#include "base/metrics/histogram.h"
+#include "base/observer_list.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/time.h"
@@ -72,6 +75,9 @@ class StatisticsProviderImpl : public StatisticsProvider {
   virtual bool GetMachineStatistic(const std::string& name,
                                    std::string* result) OVERRIDE;
 
+  virtual void AddObserver(Observer* observer) OVERRIDE;
+  virtual void RemoveObserver(Observer* observer) OVERRIDE;
+
   static StatisticsProviderImpl* GetInstance();
 
  private:
@@ -82,19 +88,27 @@ class StatisticsProviderImpl : public StatisticsProvider {
   // Starts loading the machine statistcs.
   void StartLoadingMachineStatistics();
 
-  // Loads the machine statistcs by examining the system.
-  void LoadMachineStatistics();
+  // Loads the machine statistics by examining the system.
+  // This is executed on the blocking pool, and replies by posting a task to
+  // the UI thread using |weak_this|.
+  void LoadMachineStatistics(base::WeakPtr<StatisticsProviderImpl> weak_this,
+                             base::Time start_time);
+
+  // Posted to UI from LoadMachineStatistics() once the info has been read.
+  void OnStatisticsLoaded(base::Time start_time);
 
   NameValuePairsParser::NameValueMap machine_info_;
   base::WaitableEvent on_statistics_loaded_;
+  base::WeakPtrFactory<StatisticsProviderImpl> weak_factory_;
+  ObserverList<Observer, true> observer_list_;
 
   DISALLOW_COPY_AND_ASSIGN(StatisticsProviderImpl);
 };
 
-bool StatisticsProviderImpl::GetMachineStatistic(
-    const std::string& name, std::string* result) {
+bool StatisticsProviderImpl::GetMachineStatistic(const std::string& name,
+                                                 std::string* result) {
   VLOG(1) << "Statistic is requested for " << name;
-  // Block if the statistics are not loaded yet. Per LOG(WARNING) below,
+  // Block if the statistics are not loaded yet. Per DLOG(WARNING) below,
   // the statistics are loaded before requested as of now. For regular
   // sessions (i.e. not OOBE), statistics are first requested when the
   // user is logging in so we have plenty of time to load the data
@@ -105,15 +119,16 @@ bool StatisticsProviderImpl::GetMachineStatistic(
   // very early stage of the browser startup. The statistic name should be
   // helpful to identify the caller.
   if (!on_statistics_loaded_.IsSignaled()) {
-    LOG(WARNING) << "Waiting to load statistics. Requested statistic: "
-                 << name;
+    DLOG(WARNING) << "Waiting to load statistics. Requested statistic: "
+                  << name;
     // http://crbug.com/125385
     base::ThreadRestrictions::ScopedAllowWait allow_wait;
     on_statistics_loaded_.TimedWait(base::TimeDelta::FromSeconds(kTimeoutSecs));
 
     if (!on_statistics_loaded_.IsSignaled()) {
-      LOG(ERROR) << "Statistics weren't loaded after waiting! "
-                 << "Requested statistic: " << name;
+      DLOG(ERROR) << "Statistics weren't loaded after waiting! "
+                  << "Requested statistic: " << name;
+      UMA_HISTOGRAM_BOOLEAN("Cros.StatisticsProviderTimedOut", true);
       return false;
     }
   }
@@ -126,10 +141,24 @@ bool StatisticsProviderImpl::GetMachineStatistic(
   return false;
 }
 
+void StatisticsProviderImpl::AddObserver(Observer* observer) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (on_statistics_loaded_.IsSignaled()) {

[jar (doing other things), 2012/05/21 18:10:02]

This confuses me a bit, as we're doing signalling, and setting up an observer. 
I'd expect to see one or the other.

[Joao da Silva, 2012/05/22 14:55:25]

Removing the signal would require refactoring all the current clients to use the
async interface, which isn't feasible in one go.

+    observer->OnMachineStatisticsReady();
+  } else {
+    observer_list_.AddObserver(observer);

[jar (doing other things), 2012/05/21 18:10:02]

I see an observer optionally being added... but I don't see the corresponding
call to remove the observer, presumably when the callback is made.  Note that if
you make the callback on line 147, you effectively remove (actually, avoid
adding) the observer.

[Joao da Silva, 2012/05/22 14:55:25]

Right, this is a bit confusing. Removing an observer that wasn't added is OK,
therefore this code. But observers that go away before the signal is ready
should be able to unregister themselves.

This is a one-shot event, and adding an observer after the signal is ready won't
do anything, since it won't be notified again. I think the callback version was
clearer for this; "Observers" could just invalidate the callback (using
CancellableCallback or invalidating a WeakPtr), and wouldn't have to worry about
unregistering.

I see two options now:

(1) automatically remove observers right after notifying them. RemoveObserver()
is still required to remove observers that go away before the statistics are
ready.

(2) go back to the callback version :-)

I prefer (2); what's your opinion?

[Joao da Silva, 2012/05/22 19:10:09]

I'd like your take on this matter before proceeding further, please comment.
Thanks!

[jar (doing other things), 2012/05/22 20:09:30]

The "callback" version was re-inventing the wheel... and probably re-inventing
all the evolutionary bugs.  I think the current direction is better.

To maintain symmetry, I'd suggest you remove the observer after you notify it,
so that the end result is the same for both immediate notification, or async
notification.

You are right, you still have to clean up, just in case.


On 2012/05/22 14:55:25, Joao da Silva wrote:

+  }
+}
+
+void StatisticsProviderImpl::RemoveObserver(Observer* observer) {
+  observer_list_.RemoveObserver(observer);
+}
+
 // manual_reset needs to be true, as we want to keep the signaled state.
 StatisticsProviderImpl::StatisticsProviderImpl()
     : on_statistics_loaded_(true  /* manual_reset */,
-                            false /* initially_signaled */) {
+                            false /* initially_signaled */),
+      ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)) {
   StartLoadingMachineStatistics();
 }
 
@@ -138,18 +167,23 @@ void StatisticsProviderImpl::StartLoadingMachineStatistics() {
   BrowserThread::PostBlockingPoolTask(
       FROM_HERE,
       base::Bind(&StatisticsProviderImpl::LoadMachineStatistics,
-                 base::Unretained(this)));
+                 base::Unretained(this),
+                 weak_factory_.GetWeakPtr(),
+                 base::Time::Now()));
 }
 
-void StatisticsProviderImpl::LoadMachineStatistics() {
+void StatisticsProviderImpl::LoadMachineStatistics(
+    base::WeakPtr<StatisticsProviderImpl> weak_this,
+    base::Time start_time) {
   NameValuePairsParser parser(&machine_info_);
 
   // Parse all of the key/value pairs from the crossystem tool.
   if (!parser.ParseNameValuePairsFromTool(
           arraysize(kCrosSystemTool), kCrosSystemTool, kCrosSystemEq,
           kCrosSystemDelim, kCrosSystemCommentDelim)) {
-    LOG(WARNING) << "There were errors parsing the output of "
-                 << kCrosSystemTool << ".";
+    DLOG(WARNING) << "There were errors parsing the output of "
+                  << kCrosSystemTool << ".";
+    UMA_HISTOGRAM_BOOLEAN("Cros.CrosSystemParsingFailed", true);
   }
 
   // Ensure that the hardware class key is present with the expected
@@ -175,6 +209,12 @@ void StatisticsProviderImpl::LoadMachineStatistics() {
   on_statistics_loaded_.Signal();
   VLOG(1) << "Finished loading statistics";
 
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&StatisticsProviderImpl::OnStatisticsLoaded,
+                 weak_this,
+                 start_time));
+
 #if defined(GOOGLE_CHROME_BUILD)
   // TODO(kochi): This is for providing a channel information to
   // chrome::VersionInfo::GetChannel()/GetVersionStringModifier(),
@@ -199,6 +239,13 @@ void StatisticsProviderImpl::LoadMachineStatistics() {
 #endif
 }
 
+void StatisticsProviderImpl::OnStatisticsLoaded(base::Time start_time) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  base::TimeDelta elapsed = base::Time::Now() - start_time;
+  UMA_HISTOGRAM_LONG_TIMES("Cros.StatisticsProviderReadyDelay", elapsed);
+  FOR_EACH_OBSERVER(Observer, observer_list_, OnMachineStatisticsReady());

[jar (doing other things), 2012/05/21 18:10:02]

This, for instance, would be a great place to remove the observers (after we've
done the callbacks).

[jar (doing other things), 2012/05/22 20:09:30]

Note that it would get messy if you tried to use async registration from other
threads while you were iterating here.  Again, single thread registration and
notification seems like a much clearer picture.

+}
+
 StatisticsProviderImpl* StatisticsProviderImpl::GetInstance() {
   return Singleton<StatisticsProviderImpl,
                    DefaultSingletonTraits<StatisticsProviderImpl> >::get();
@@ -213,6 +260,13 @@ class StatisticsProviderStubImpl : public StatisticsProvider {
     return false;
   }
 
+  virtual void AddObserver(Observer* observer) OVERRIDE {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    observer->OnMachineStatisticsReady();
+  }
+
+  virtual void RemoveObserver(Observer* observer) OVERRIDE {}
+
   static StatisticsProviderStubImpl* GetInstance() {
     return Singleton<StatisticsProviderStubImpl,
         DefaultSingletonTraits<StatisticsProviderStubImpl> >::get();