Run parsing JSON into ResourceList on blocking pool.

chrome/browser/chromeos/drive/drive_api_service.cc

Index: chrome/browser/chromeos/drive/drive_api_service.cc
diff --git a/chrome/browser/chromeos/drive/drive_api_service.cc b/chrome/browser/chromeos/drive/drive_api_service.cc
index 18348fa2efd921fed6a9219ce22d96079c73fc53..711530d6e6ac87211cda7c3bef218af10d4da237 100644
--- a/chrome/browser/chromeos/drive/drive_api_service.cc
+++ b/chrome/browser/chromeos/drive/drive_api_service.cc
@@ -9,6 +9,7 @@
 
 #include "base/bind.h"
 #include "base/message_loop_proxy.h"
+#include "base/threading/sequenced_worker_pool.h"
 #include "base/values.h"
 #include "chrome/browser/google_apis/drive_api_operations.h"
 #include "chrome/browser/google_apis/drive_api_parser.h"
@@ -30,35 +31,67 @@ const char kDriveScope[] = "https://www.googleapis.com/auth/drive";
 const char kDriveAppsReadonlyScope[] =
     "https://www.googleapis.com/auth/drive.apps.readonly";
 
-// Parses the JSON value into ResourceList and runs |callback|.
-void ParseResourceListAndRun(
-    const google_apis::GetResourceListCallback& callback,
-    google_apis::GDataErrorCode error,
-    scoped_ptr<base::Value> value) {
+struct OnParseResourceListCompletedParams {
+  google_apis::GDataErrorCode error;
+  scoped_ptr<google_apis::ResourceList> resource_list;
+};
+
+void ParseResourceListOnBlockingPool(
+    scoped_ptr<base::Value> value,
+    OnParseResourceListCompletedParams* params) {
   if (!value) {
-    callback.Run(error, scoped_ptr<google_apis::ResourceList>());
+    // JSON value is not available.
     return;
   }
 
-  // TODO(satorux): Parse the JSON value on a blocking pool. crbug.com/165088
+  // Parse the value into ResourceList via ChangeList.
+  // If failed, set (i.e. overwrite) the error flag and return immediately.
   scoped_ptr<google_apis::ChangeList> change_list(
       google_apis::ChangeList::CreateFrom(*value));
   if (!change_list) {
-    callback.Run(google_apis::GDATA_PARSE_ERROR,
-                 scoped_ptr<google_apis::ResourceList>());
+    params->error = google_apis::GDATA_PARSE_ERROR;
     return;
   }
 
-  // TODO(satorux): Do the conversion on a blocking pool too. crbug.com/165088
   scoped_ptr<google_apis::ResourceList> resource_list =
       google_apis::ResourceList::CreateFromChangeList(*change_list);
   if (!resource_list) {
-    callback.Run(google_apis::GDATA_PARSE_ERROR,
-                 scoped_ptr<google_apis::ResourceList>());
+    params->error = google_apis::GDATA_PARSE_ERROR;
     return;
   }
 
-  callback.Run(error, resource_list.Pass());
+  // Pass the result to the params, so that OnParseResourceListCompleted
+  // defined below can process it.
+  params->resource_list = resource_list.Pass();
+}
+
+// Callback invoked when the parsing of resource list is completed,
+// regardless whether it is succeeded or not.
+void OnParseResourceListCompleted(

[satorux1, 2013/01/09 00:25:20]

Please rename this to DidParseResourceListOnBlockingPool.

This naming convention, just adding "Did" is easier.

[hidehiko, 2013/01/09 05:21:51]

Done.

+    const google_apis::GetResourceListCallback& callback,
+    OnParseResourceListCompletedParams* params) {
+  callback.Run(params->error, params->resource_list.Pass());
+}
+
+// Sends a task to parse the JSON value into ResourceList on blocking pool,
+// with a callback which is called when the task is done.
+void ParseResourceListOnBlockingPoolAndRun(
+    const google_apis::GetResourceListCallback& callback,
+    google_apis::GDataErrorCode error,
+    scoped_ptr<base::Value> value) {
+  OnParseResourceListCompletedParams* params =
+      new OnParseResourceListCompletedParams;

[satorux1, 2013/01/09 00:25:20]

I think you don't need a struct.

[hidehiko, 2013/01/09 05:21:51]

At the moment we need, IMHO, though there are some alternatives:
1) Run callback if value is NULL here immediately. (though, handling the
callback would be distributed in this method and
DidParseResourceListOnBlockingPool).
2) Overwrite the error code with GDATA_PARSE_ERROR if value is NULL. Then we can
remove error code handling in ParseResourceListOnBlockingPool so that we can
remove params here. (I'm not very sure if we can overwrite it or not, though.)
3) Check if error code is HTTP_SUCCESS or not in
DidParseResourceListOnBlockingPool. If so, and the parsing is failed (i.e.
ParseResourceListOnBlockingPool returns NULL), overwrite error code with
GDATA_PARSE_ERROR.
4) Pass the ownership of |value| to DidParseResourceListOnBlockingPool, instead
give ParseResourceListOnBlockingPool the raw pointer to |value|. So that
DidParseResourceListOnBlockingPool can know if the parsing is failed, or JSON
value was empty (though the handling code may be less clear).
5) Use new for google_apis::GDataErrorCode and 
scoped_ptr<google_apis::ResourceList> individually, here. Then pass their
ownership to the DidParseResourceListOnBlockingPool.
6) Keep the current structure (use the struct).
7) something else?

Which is better in this project?

[satorux1, 2013/01/09 05:34:15]

I thought we could do something like:

  google_apis::GDataErrorCode* error = new
google_apis::GDataErrorCode(in_error);
  base::PostTaskAndReplyWithResult(
      BrowserThread::GetBlockingPool(),
      FROM_HERE,
      base::Bind(&ParseResourceListOnBlockingPool, base::Passed(&value), error),
      base::Bind(&DidParseResourceListOnBlockingPool, callback,
base::Owned(error)));

scoped_ptr<ResourceList> ParseResourceListOnBlockingPool(
    scoped_ptr<base::Value> value,
    GDataErrorCode* error) {
  ...

  return resource_list.Pass();
}


void DidParseResourceListOnBlockingPool(
    const google_apis::GetResourceListCallback& callback,
    GDataErrorCode* error,
    scoped_ptr<ResourceList> resource_list)
  callback.Run(*error, resource_list.Pass());
}

[hidehiko, 2013/01/09 07:34:07]

I see. Done.

+  // The error is initialized here to the given |error|, but may be
+  // overwritten in ParseResourceListOnBlockingPool before used in
+  // OnParseResourceListCompleted.
+  params->error = error;
+
+  BrowserThread::GetBlockingPool()->PostTaskAndReply(
+      FROM_HERE,
+      base::Bind(&ParseResourceListOnBlockingPool,
+                 base::Passed(&value), params),
+      base::Bind(&OnParseResourceListCompleted,
+                 callback, base::Owned(params)));
 }
 
 // Parses the JSON value to ResourceEntry runs |callback|.
@@ -221,7 +254,7 @@ void DriveAPIService::GetFilelist(
           url_generator_,
           url,
           search_query,
-          base::Bind(&ParseResourceListAndRun, callback)));
+          base::Bind(&ParseResourceListOnBlockingPoolAndRun, callback)));
 }
 
 void DriveAPIService::GetChangelist(
@@ -238,7 +271,7 @@ void DriveAPIService::GetChangelist(
           url_generator_,
           url,
           start_changestamp,
-          base::Bind(&ParseResourceListAndRun, callback)));
+          base::Bind(&ParseResourceListOnBlockingPoolAndRun, callback)));
 }
 
 void DriveAPIService::GetResourceEntry(