[SystemInfo API] Finish TODOs in SystemInfoProvider

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 1fc5b3532d1007dec84a94267c0b181234f36388..2afe3e7e2364742a7982b5fc610db7056fb47e9d 100644
--- a/chrome/browser/extensions/api/system_info/system_info_provider.h
+++ b/chrome/browser/extensions/api/system_info/system_info_provider.h
@@ -44,14 +44,11 @@ class SystemInfoProvider
   // 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 base::Callback<void(bool)> QueryInfoCompletionCallback;
   typedef std::queue<QueryInfoCompletionCallback> CallbackQueue;
 
   SystemInfoProvider()
-    : is_waiting_for_completion_(false) {
-    worker_pool_token_ =
-      content::BrowserThread::GetBlockingPool()->GetSequenceToken();
-  }
+    : is_waiting_for_completion_(false) {}
 
   virtual ~SystemInfoProvider() {}
 
@@ -79,55 +76,34 @@ class SystemInfoProvider
   }
 
  protected:
-  // Default implementation of querying system information.
-  //
-  // While overriding, there are two things need to do:
-  //   1). Bind custom callback function for query system information.
-  //   2). Post the custom task to blocking pool.
+  // We can override this method with our own custom query info Callback

[Greg Billock, 2013/07/08 18:05:06]

I think you can just get rid of this comment.

[Jeffrey Yasskin, 2013/07/08 19:09:49]

If this function stays virtual, it does need a comment saying what guarantees an
overriding implementation has to provide. I agree that this comment isn't enough
to know how to override the function.

[Haojian Wu, 2013/07/09 13:45:04]

Done. Update the comments.

+  // function which type is bool().
   virtual void StartQueryInfoImpl() {
-    base::Closure callback =
-        base::Bind(&SystemInfoProvider<T>::QueryOnWorkerPool, this);
-    PostQueryTaskToBlockingPool(FROM_HERE, callback);
+    base::Callback<bool()> query_info_callback =

[Greg Billock, 2013/07/08 18:05:06]

You can put this in the PostQueryTaskToBlockingPool call directly and avoid the
temporary.

[Haojian Wu, 2013/07/09 13:45:04]

Done.

+        base::Bind(&SystemInfoProvider<T>::QueryInfo, this);
+    PostQueryTaskToBlockingPool(query_info_callback);
   }
 
-  // Post a task to blocking pool for information querying.
-  //
-  // The parameter query_callback should invoke QueryInfo directly or indirectly
-  // to query the system information and return to UI thread when the query is
-  // completed.
-  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, query_callback,
-          base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN);
+  // Post the query info task to blocking pool.

[Greg Billock, 2013/07/08 18:05:06]

Your method name says this. Delete comment or replace with one indicating that
OnQueryCompleted will be the next thing we see called.

[Haojian Wu, 2013/07/09 13:45:04]

Done.

+  void PostQueryTaskToBlockingPool(
+      const base::Callback<bool()> & query_info_callback) {
+    base::SequencedWorkerPool* pool = content::BrowserThread::GetBlockingPool();
+    scoped_refptr<base::SequencedTaskRunner> blocking_task_runner_ =
+        pool->GetSequencedTaskRunnerWithShutdownBehavior(
+            pool->GetSequenceToken(),

[Jeffrey Yasskin, 2013/07/08 19:09:49]

Although this is one reading of what I suggested, it's not the reading that
preserves the sequencing behavior in your original code. Can you read the
documentation
(https://code.google.com/p/chromium/codesearch/#chromium/src/base/threading/se...
and
https://code.google.com/p/chromium/codesearch/#chromium/src/base/threading/se...)
and modify this so it does guarantee that all tasks are executed in sequence?

[Haojian Wu, 2013/07/09 13:45:04]

Done.

+            base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN);
+    // Post the custom query info task to blocking pool for information querying
+    // and reply with OnQueryCompleted.
+    base::PostTaskAndReplyWithResult(
+        blocking_task_runner_,
+        FROM_HERE,
+        query_info_callback,
+        base::Bind(&SystemInfoProvider<T>::OnQueryCompleted, this));
   }
 
-  // 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, replacing with void
-  // QueryInfo().
-  virtual bool QueryInfo(T* info) = 0;
-
-  // Called on UI thread. The |success| parameter means whether it succeeds
-  // to get the information.
-  void OnQueryCompleted(bool success) {
-    DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
-
-    while (!callbacks_.empty()) {
-      QueryInfoCompletionCallback callback = callbacks_.front();
-      callback.Run(info_, success);
-      callbacks_.pop();
-    }
-
-    is_waiting_for_completion_ = false;
-  }
+  // Query the system information synchronously and put the result into |info_|.
+  // Return true if no error occurs.
+  virtual bool QueryInfo() = 0;
 
   // Template function for creating the single shared provider instance.
   // Template paramter I is the type of SystemInfoProvider implementation.
@@ -144,12 +120,18 @@ class SystemInfoProvider
   T info_;

[Greg Billock, 2013/07/08 18:05:06]

This change is most of the way to de-templating the design. |info_| is basically
no longer used here, and there are direct accessors on subclasses. Lets get this
in and hold off on separating out the templating stuff. I think we'll be able to
nicely use subclasses after this.

[Haojian Wu, 2013/07/09 13:45:04]

Yes, I think so. I will separate a new CL to do it after this patch gets landed
since it also requires a big change of SystemInfo API.

 
  private:
-  // TODO(Haojian): Use PostBlockingPoolTaskAndReply to avoid unnecessary
-  // trampolines trip.
-  void QueryOnWorkerPool() {
-    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.
+  void OnQueryCompleted(bool success) {
+    DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+    while (!callbacks_.empty()) {
+      QueryInfoCompletionCallback callback = callbacks_.front();
+      callback.Run(success);
+      callbacks_.pop();
+    }
+
+    is_waiting_for_completion_ = false;

[Jeffrey Yasskin, 2013/07/08 19:09:49]

This is a bit subtle. Could you add a comment to StartQueryInfo(callback) saying
that if the callback itself calls StartQueryInfo(callback2), callback2 will be
called immediately rather than triggering another call to the system?

[Haojian Wu, 2013/07/09 13:45:04]

Done.

   }
 
   // The single shared provider instance. We create it only when needed.
@@ -163,10 +145,6 @@ class SystemInfoProvider
   // Indicates if it is waiting for the querying completion.
   bool is_waiting_for_completion_;
 
-  // Unqiue sequence token so that the operation of querying inforation can
-  // be executed in order.
-  base::SequencedWorkerPool::SequenceToken worker_pool_token_;
-
   DISALLOW_COPY_AND_ASSIGN(SystemInfoProvider<T>);
 };