Added asynchronous notification of readiness to the StatisticsProvider, and

chrome/browser/chromeos/system/statistics_provider.h

 // Copyright (c) 2011 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.
 
 #ifndef CHROME_BROWSER_CHROMEOS_SYSTEM_STATISTICS_PROVIDER_H_
 #define CHROME_BROWSER_CHROMEOS_SYSTEM_STATISTICS_PROVIDER_H_
 #pragma once
 
 #include <string>
 
+#include "base/callback.h"
+
 namespace chromeos {
 namespace system {
 
 // This interface provides access to Chrome OS statistics.
 class StatisticsProvider {
  public:
   // Retrieve the named machine statistic (e.g. "hardware_class").
-  // This does not update the statistcs. If the |name| is not set, |result|
-  // preserves old value.
+  // This does not update the statistcs. |result| is not updated if |name| is
+  // not set.
+  // This call may block if the statistics aren't ready yet, up to a timeout;
+  // it may also fail because the statistics weren't ready before the timeout.
+  // To make sure this won't block nor fail, invoke GetMachineStatistic() after
+  // WhenReady() has invoked its callback.
   virtual bool GetMachineStatistic(const std::string& name,

[jar (doing other things), 2012/04/24 18:37:40]

nit: I had to read the comment a few times to parse it and possibly undrestand
it.  For instance, I still don't know the significance of "This does not update
the statistics.".  Plausibly better comment (for current API) would be:

/* Get the machine statistic with a key of |name| (e.g., "hardware_class"). 
Returns true if the key was found, and data was returned in |result|.  Returns
false otherwise, and the contents of |result| are undefined.  This call may
block for an arbitrary length of time (waiting for statistics to load, or a
timeout to take place).  If this function is called anytime after WhenReady()
has invoked any callback, then this function is guaranteed not to block. 
*/

Reading that description, it is IMO clear that the interface *should* specify
this timeout, and that would perchance be more valuable than providing an async
interface in this case.  With a timeout provided in this call, a polling loop
could easily get the data.  Given that this data is *not* something that a user
needs to wait for, and I'm *guessing* that that the call is cheap and rarely
used, a polling loop (which keeps posting delayed tasks to retry) sure sounds
like a nice solution to avoid this whole problem.

[Joao da Silva, 2012/04/27 09:47:48]

Thanks, that's a much better wording :-)
It seems to me that both approaches would be similar from the client's
perspective; in both cases, the client needs to have a method that either
handles the callback task or that retries polling. The callback interface
doesn't require rewriting clients that don't care about the timeout though, and
that's why it was the first choice. wdyt?

                                    std::string* result) = 0;
 
+  // Invokes |callback| on the UI loop once the statistics have been loaded.
+  // |callback| is immediately posted to the UI loop if the statistics are
+  // already available.
+  virtual void WhenReady(const base::Closure& callback) = 0;

[jar (doing other things), 2012/04/24 18:37:40]

You should probably use a more standard pattern, and just register an observer
that will be notified.

[Joao da Silva, 2012/04/27 09:47:48]

I've been encouraged in other CLs to use callbacks when possible. Observers are
more appropriate when there is more than 1 callback method, or when there's a
need to unregister them. Callbacks work well for one-shot notifications, as is
the case here. Do you strongly prefer an Observer for this case?

[jar (doing other things), 2012/04/27 18:00:38]

I like callbacks better, as you noted, when there is only one callback method. 
In this case, you implemented a stack of callbacks, and a push, which suggested
you wanted to have a multitude of callbacks.  I'd prefer to see exactly one
callback... with a CHECK if we ever had more than one.

If you want support for a multitude of callbacks, I'd rather see you use the
observer pattern.

[Joao da Silva, 2012/05/15 13:52:47]

Added an observer as suggested.

+
   static StatisticsProvider* GetInstance();
 
  protected:
   virtual ~StatisticsProvider() {}
 };
 
 }  // namespace system
 }  // namespace chromeos
 
 #endif  // CHROME_BROWSER_CHROMEOS_SYSTEM_STATISTICS_PROVIDER_H_