Added asynchronous notification of readiness to the StatisticsProvider, and

chrome/browser/chromeos/system/statistics_provider.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/system/statistics_provider.h"
 
 #include "base/bind.h"
 #include "base/chromeos/chromeos_version.h"
 #include "base/file_path.h"
 #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"
 #include "base/chromeos/chromeos_version.h"
 #include "chrome/browser/chromeos/system/name_value_pairs_parser.h"
 #include "chrome/common/child_process_logging.h"
 #include "chrome/common/chrome_version_info.h"
 #include "content/public/browser/browser_thread.h"
 
 using content::BrowserThread;
@@ skipping 42 matching lines @@
 
 }  // namespace
 
 // The StatisticsProvider implementation used in production.
 class StatisticsProviderImpl : public StatisticsProvider {
  public:
   // StatisticsProvider implementation:
   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:
   friend struct DefaultSingletonTraits<StatisticsProviderImpl>;
 
   StatisticsProviderImpl();
 
   // 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
   // beforehand.
   //
   // If you see the warning appeared for regular sessions, it probably
   // means that there is new client code that uses the statistics in the
   // 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;
     }
   }
 
   NameValuePairsParser::NameValueMap::iterator iter = machine_info_.find(name);
   if (iter != machine_info_.end()) {
     *result = iter->second;
     return true;
   }
   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();
 }
 
 void StatisticsProviderImpl::StartLoadingMachineStatistics() {
   VLOG(1) << "Started loading statistics";
   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
   // key name, and if it couldn't be retrieved, that the value is "unknown".
   std::string hardware_class = machine_info_[kHardwareClassCrosSystemKey];
   if (hardware_class.empty() || hardware_class == kCrosSystemUnknownValue)
     machine_info_[kHardwareClassKey] = kUnknownHardwareClass;
   else
     machine_info_[kHardwareClassKey] = hardware_class;
 
   parser.GetNameValuePairsFromFile(FilePath(kMachineHardwareInfoFile),
                                    kMachineHardwareInfoEq,
                                    kMachineHardwareInfoDelim);
   parser.GetNameValuePairsFromFile(FilePath(kEchoCouponFile),
                                    kEchoCouponEq,
                                    kEchoCouponDelim);
   parser.GetNameValuePairsFromFile(FilePath(kMachineOSInfoFile),
                                    kMachineOSInfoEq,
                                    kMachineOSInfoDelim);
   parser.GetNameValuePairsFromFile(FilePath(kVpdFile), kVpdEq, kVpdDelim);
 
   // Finished loading the statistics.
   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(),
   // but this is still late for some early customers such as
   // prerender::ConfigurePrefetchAndPrerender() and
   // ThreadWatcherList::ParseCommandLine().
   // See http://crbug.com/107333 .
   const char kChromeOSReleaseTrack[] = "CHROMEOS_RELEASE_TRACK";
   std::string channel;
   if (GetMachineStatistic(kChromeOSReleaseTrack, &channel)) {
       chrome::VersionInfo::SetChannel(channel);
       // Set the product channel for crash reports.  We can't just do this in
       // ChromeBrowserMainParts::PreCreateThreads like we do for Linux because
       // the FILE thread hasn't been created yet there so we can't possibly
       // have read this yet.  Note that this string isn't exactly the same as
       // 'channel', it's been parsed to be consistent with other platforms
       // (eg. "canary-channel" becomes "canary", "testimage-channel" becomes
       // "unknown").
       child_process_logging::SetChannel(
           chrome::VersionInfo::GetVersionStringModifier());
   }
 #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();
 }
 
 // The stub StatisticsProvider implementation used on Linux desktop.
 class StatisticsProviderStubImpl : public StatisticsProvider {
  public:
   // StatisticsProvider implementation:
   virtual bool GetMachineStatistic(const std::string& name,
                                    std::string* result) OVERRIDE {
     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();
   }
 
  private:
   friend struct DefaultSingletonTraits<StatisticsProviderStubImpl>;
 
   StatisticsProviderStubImpl() {
   }
 
   DISALLOW_COPY_AND_ASSIGN(StatisticsProviderStubImpl);
 };
 
 StatisticsProvider* StatisticsProvider::GetInstance() {
   if (base::chromeos::IsRunningOnChromeOS()) {
     return StatisticsProviderImpl::GetInstance();
   } else {
     return StatisticsProviderStubImpl::GetInstance();
   }
 }
 
 }  // namespace system
 }  // namespace chromeos