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

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

 // Copyright 2013 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_EXTENSIONS_API_SYSTEM_INFO_SYSTEM_INFO_PROVIDER_H_
 #define CHROME_BROWSER_EXTENSIONS_API_SYSTEM_INFO_SYSTEM_INFO_PROVIDER_H_
 
 #include <queue>
 
 #include "base/bind.h"
 #include "base/callback.h"
@@ skipping 11 matching lines @@
 // The SystemInfoProvider is designed to query system information on the worker
 // pool. It also maintains a queue of callbacks on the UI thread which are
 // waiting for the completion of querying operation. Once the query operation
 // is completed, all pending callbacks in the queue get called on the UI
 // thread. In this way, it avoids frequent querying operation in case of lots
 // of query requests, e.g. calling systemInfo.cpu.get repeatedly in an
 // extension process.
 //
 // Template parameter T is the system information type. It could be the
 // structure type generated by IDL parser.
 template<class T>

[Greg Billock, 2013/06/18 18:24:52]

I'm not a fan of templates here. Given that we need specific implementation
functions anyway to fetch each type, a code format that would be more
chromium-like would be a superclass with all the functions needed, and then if
there are platform-specific functions, there'd be subclasses to implement those
methods for each type of data.

[Haojian Wu, 2013/06/21 05:49:04]

hongbo@, how about your idea? Should we refractor the whole SystemInfo API
skeleton?

[Hongbo Min, 2013/06/22 09:52:28]

Original thought on type T is, it is a type generated by IDL parser based on a
dictionary declaration in IDL file. In this way, we won't need to build a new
class hierarchy for system information categories. 

[Greg Billock, 2013/06/24 16:55:38]

I see. Do you foresee enough specializations to make that important? I think
that you've got a pretty complete set, so that wouldn't be a big saver of
complexity.

 class SystemInfoProvider
     : public base::RefCountedThreadSafe<SystemInfoProvider<T> > {
  public:
   // Callback type for completing to get information. The callback accepts
   // two arguments. The first one is the information got already, the second
   // one indicates whether its contents are valid, for example, no error
   // occurs in querying the information.
   typedef base::Callback<void(const T&, bool)> QueryInfoCompletionCallback;
   typedef std::queue<QueryInfoCompletionCallback> CallbackQueue;
 
   SystemInfoProvider()
     : is_waiting_for_completion_(false) {
     worker_pool_token_ =
       content::BrowserThread::GetBlockingPool()->GetSequenceToken();
   }
 
   virtual ~SystemInfoProvider() {}
 
   // For testing
   static void InitializeForTesting(
       scoped_refptr<SystemInfoProvider<T> > provider) {
     DCHECK(provider.get() != NULL);
     single_shared_provider_.Get() = provider;
   }
 
   // Start to query the system information. Should be called on UI thread.
   // The |callback| will get called once the query is completed.
   void StartQueryInfo(const QueryInfoCompletionCallback& callback) {

[Greg Billock, 2013/06/18 18:24:52]

This can go in a .cc file, along with non-templated methods.

[Haojian Wu, 2013/06/21 05:49:04]

I will create a new CL to separate the declaration and definition of
SystemInfoProvider.

[Greg Billock, 2013/06/24 16:55:38]

Sounds good.

     DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
     DCHECK(!callback.is_null());
 
     callbacks_.push(callback);
 
     if (is_waiting_for_completion_)
       return;
 
     is_waiting_for_completion_ = true;
 
+    StartQueryInfoImpl();
+  }
+
+ protected:
+  // Default implementation of querying system information.
+  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.
+  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.
   virtual bool QueryInfo(T* info) = 0;
 
   virtual void QueryOnWorkerPool() {

[Greg Billock, 2013/06/18 18:24:52]

Instead of trampolines on both sides, could we use PostBlockingPoolTaskAndReply?
Or does the template parameter foul that up? Could that be avoided, since we
have a "T info_" member, and just not pass the parameter to QueryInfo()?

I think this is another argument for just having the known types of system info
explicitly referenced instead of using templates.

[Haojian Wu, 2013/06/21 05:49:04]

hongbo@, your idea?

[Hongbo Min, 2013/06/22 09:52:28]

Thanks for Billock to raise these improvements.

I spent a little time to see why QueryInfo is designed to accept a T-typed
pointer as paramter, it is mainly for querying free space for storages when
checking the free space change, but right now this condition for that is now not
valid since StorageInfoProvider::QueryUnitInfo is added. 

I agree with Billock's suggestions:
1) No need a T-typed pointer in QueryInfo methods;
2) Could use PostBlockingPoolTaskAndReply to avoid unnecessary trampolines trip.

It is OK to me if doing this in another CL since it will touch all SystemInfo
related code.

[Haojian Wu, 2013/06/23 07:59:16]

Done. Add TODO comments.

     bool success = QueryInfo(&info_);
     content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
         base::Bind(&SystemInfoProvider<T>::OnQueryCompleted, this, success));
   }
 
   // Called on UI thread. The |success| parameter means whether it succeeds
   // to get the information.
   virtual void OnQueryCompleted(bool success) {
     DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
 
@@ skipping 40 matching lines @@
 };
 
 // Static member intialization.
 template<class T>
 typename base::LazyInstance<scoped_refptr<SystemInfoProvider<T> > >
   SystemInfoProvider<T>::single_shared_provider_ = LAZY_INSTANCE_INITIALIZER;
 
 }  // namespace extensions
 
 #endif  // CHROME_BROWSER_EXTENSIONS_API_SYSTEM_INFO_SYSTEM_INFO_PROVIDER_H_