[SystemInfo API] Rewrite storage info provider using storage monitor impl.

chrome/browser/extensions/api/system_info/system_info_provider.h

Index: chrome/browser/extensions/api/system_info/system_info_provider.h
diff --git a/chrome/browser/extensions/api/system_info/system_info_provider.h b/chrome/browser/extensions/api/system_info/system_info_provider.h
index 574e62925ace61d2cb8d60c36e767eae41599438..4b8ad9ae97e2aa9a165cb964bfc8b3d37b8e800f 100644
--- a/chrome/browser/extensions/api/system_info/system_info_provider.h
+++ b/chrome/browser/extensions/api/system_info/system_info_provider.h
@@ -29,6 +29,9 @@ namespace extensions {
 //
 // Template parameter T is the system information type. It could be the
 // structure type generated by IDL parser.
+//
+// The class member info_ is accessed on multiple threads, but that the whole
+// class is being guarded by SystemInfoProvider.

[Jeffrey Yasskin, 2013/07/02 23:00:50]

What does this mean? Normally I'd expect a mutex to guard an object accessed
from multiple threads, but SystemInfoProvider isn't a mutex.

[Hongbo Min, 2013/07/03 04:59:40]

You might miss some background of this class implementation.

We try to avoid using mutex to keep thread-safe to access |info_|. You might
notice that the StartQueryInfo will put a new callback into a queue, once the
information is queried and put into |info_|, then the callbacks in queue will be
called one by one in UI thread.

The bool |is_waiting_for_completion_| serves as such a purpose. 

[Haojian Wu, 2013/07/03 16:23:49]

Please see hongbo's explanation at
here:https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc#newcode74

I have updated the comment for more details.

 template<class T>
 class SystemInfoProvider
     : public base::RefCountedThreadSafe<SystemInfoProvider<T> > {
@@ -68,24 +71,39 @@ class SystemInfoProvider
 
     is_waiting_for_completion_ = true;
 
+    StartQueryInfoImpl();
+  }
+
+ protected:
+  // Default implementation of querying system information.

[Jeffrey Yasskin, 2013/07/02 23:00:50]

Virtual functions need a comment saying how to override them, not just what the
base class implementation does.

[Haojian Wu, 2013/07/03 16:23:49]

Done.

+  virtual void StartQueryInfoImpl() {
+    base::Closure callback =
+        base::Bind(&SystemInfoProvider<T>::QueryOnWorkerPool, this);
+    PostQueryTaskToBlockingPool(FROM_HERE, callback);
+  }
+
+  // Post a task to blocking pool for information querying.

[Jeffrey Yasskin, 2013/07/02 23:00:50]

"for information querying" is too vague. What does the query_callback need to
do?

[Haojian Wu, 2013/07/03 16:23:49]

Done.

+  void PostQueryTaskToBlockingPool(const tracked_objects::Location& from_here,
+                                   const base::Closure& query_callback) {
+    DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
     base::SequencedWorkerPool* worker_pool =
         content::BrowserThread::GetBlockingPool();
     // The query task posted to the worker pool won't block shutdown, and any
     // running query task at shutdown time will be ignored.
     worker_pool->PostSequencedWorkerTaskWithShutdownBehavior(
-          worker_pool_token_,
-          FROM_HERE,
-          base::Bind(&SystemInfoProvider<T>::QueryOnWorkerPool, this),
+          worker_pool_token_, from_here, query_callback,
           base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN);
   }
 
- protected:
   // Query the system information synchronously and output the result to the
   // |info| parameter. The |info| contents MUST be reset firstly in its
   // platform specific implementation. Return true if it succeeds, otherwise
   // false is returned.
+  // TODO(Haojian): Remove the parameter T-typed pointer.

[Jeffrey Yasskin, 2013/07/02 23:00:50]

And replace it with what?

[Haojian Wu, 2013/07/03 16:23:49]

Replace with bool QueryInfo() instead. Update the comment.

[Jeffrey Yasskin, 2013/07/03 21:44:48]

No, this signature gives the subclass access to a T*. Where will the subclass
get that T* once you've removed the parameter?

[Haojian Wu, 2013/07/04 00:41:17]

Actually the subclass can access T-typed info_ directly since info_ is a
protected member in SystemInfoProvider(see system_info_provider.h line 139).

   virtual bool QueryInfo(T* info) = 0;
 
+  // TODO(Haojian): Use PostBlockingPoolTaskAndReply to avoid unnecessary

[Jeffrey Yasskin, 2013/07/02 23:00:50]

Why can't you do this now?

[Hongbo Min, 2013/07/03 04:59:40]

After discussion with Billock@ and Lei@, we agree to do that in a separate CL.
This CL we only focus on StorageMonitor reusing work.

[Haojian Wu, 2013/07/03 16:23:49]

Since this work will make a big change of SystemInfo API code. It will be better
to separate a new CL to do this. Please see this CL:
https://codereview.chromium.org/18290002/

[Jeffrey Yasskin, 2013/07/03 21:44:48]

In https://codereview.chromium.org/18290002/ I see only 2 uses of
PostBlockingPoolTaskAndReply, both of which replace code you're adding in this
CL. I see lots of *other* changes, which make sense to split out, but the
PostBlockingPoolTaskAndReply change doesn't look like it implies changes
anywhere else.

Actually, you should use
content::BrowserThread::GetBlockingPool()->GetSequencedTaskRunnerWithShutdownBehavior(
    content::BrowserThread::GetBlockingPool()->GetSequenceToken(),
CONTINUE_ON_SHUTDOWN)
to initialize a scoped_refptr<SequencedTaskRunner> (replacing
|worker_pool_token_|) and then use base::PostTaskAndReplyWithResult() to get
|success| passed from the worker thread back to the UI thread.

[Haojian Wu, 2013/07/04 00:41:17]

Thanks for the useful suggestions. I will update that CL soon.

+  // trampolines trip.
   virtual void QueryOnWorkerPool() {

[Jeffrey Yasskin, 2013/07/02 23:00:50]

This virtual function also needs a description of how to override it.

[Haojian Wu, 2013/07/03 16:23:49]

This function can be non-virtual and replace it with non-virtual.
Also update the comment.

     bool success = QueryInfo(&info_);
     content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,