Run parsing JSON into ResourceList on blocking pool.

chrome/browser/chromeos/drive/drive_api_service.cc

 // Copyright (c) 2012 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.
 
 #include "chrome/browser/chromeos/drive/drive_api_service.h"
 
 #include <string>
 #include <vector>
 
 #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"
 #include "chrome/browser/google_apis/gdata_wapi_parser.h"
 #include "chrome/browser/google_apis/operation_runner.h"
 #include "chrome/browser/google_apis/time_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/net/url_util.h"
 #include "content/public/browser/browser_thread.h"
 
 using content::BrowserThread;
 
 namespace drive {
 
 namespace {
 
 // OAuth2 scopes for Drive API.
 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(
+struct OnParseResourceListCompletedParams {
+  google_apis::GDataErrorCode error;
+  scoped_ptr<google_apis::ResourceList> resource_list;
+};
+
+void ParseResourceListOnBlockingPool(
+    scoped_ptr<base::Value> value,
+    OnParseResourceListCompletedParams* params) {
+  if (!value) {
+    // JSON value is not available.
+    return;
+  }
+
+  // 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) {
+    params->error = google_apis::GDATA_PARSE_ERROR;
+    return;
+  }
+
+  scoped_ptr<google_apis::ResourceList> resource_list =
+      google_apis::ResourceList::CreateFromChangeList(*change_list);
+  if (!resource_list) {
+    params->error = google_apis::GDATA_PARSE_ERROR;
+    return;
+  }
+
+  // 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) {
-  if (!value) {
-    callback.Run(error, scoped_ptr<google_apis::ResourceList>());
-    return;
-  }
+  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;
 
-  // TODO(satorux): Parse the JSON value on a blocking pool. crbug.com/165088
-  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>());
-    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>());
-    return;
-  }
-
-  callback.Run(error, resource_list.Pass());
+  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|.
 void ParseResourceEntryAndRun(
     const google_apis::GetResourceEntryCallback& callback,
     google_apis::GDataErrorCode error,
     scoped_ptr<base::Value> value) {
   DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   if (!value) {
@@ skipping 142 matching lines @@
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(!callback.is_null());
 
   runner_->StartOperationWithRetry(
       new google_apis::GetFilelistOperation(
           operation_registry(),
           url_request_context_getter_,
           url_generator_,
           url,
           search_query,
-          base::Bind(&ParseResourceListAndRun, callback)));
+          base::Bind(&ParseResourceListOnBlockingPoolAndRun, callback)));
 }
 
 void DriveAPIService::GetChangelist(
     const GURL& url,
     int64 start_changestamp,
     const google_apis::GetResourceListCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(!callback.is_null());
 
   runner_->StartOperationWithRetry(
       new google_apis::GetChangelistOperation(
           operation_registry(),
           url_request_context_getter_,
           url_generator_,
           url,
           start_changestamp,
-          base::Bind(&ParseResourceListAndRun, callback)));
+          base::Bind(&ParseResourceListOnBlockingPoolAndRun, callback)));
 }
 
 void DriveAPIService::GetResourceEntry(
     const std::string& resource_id,
     const google_apis::GetResourceEntryCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(!callback.is_null());
 
   runner_->StartOperationWithRetry(new google_apis::GetFileOperation(
       operation_registry(),
@@ skipping 185 matching lines @@
 
 void DriveAPIService::OnAuthenticationFailed(
     google_apis::GDataErrorCode error) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   FOR_EACH_OBSERVER(
       google_apis::DriveServiceObserver, observers_,
       OnAuthenticationFailed(error));
 }
 
 }  // namespace drive