Reland: Parse Drive API responses all at once in the blocking pool.

Previous implementation did the parsing of string to base::Value
on blocking pool, and that of base::Value to specific data types
either on UI thread or on yet another post to blocking pool.

The previous implementation is slightly inefficient and moreover
involves a subtle bug 284244.

BUG=284244, 401843

This CL once landed as r288017 (Patch Set 4) and reverted.
This is the second try.

Review URL: https://codereview.chromium.org/442193002

Cr-Commit-Position: refs/heads/master@{#288275}
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@288275 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/drive/drive_api_service.cc

@@ -8,10 +8,7 @@
 #include <vector>
 
 #include "base/bind.h"
-#include "base/sequenced_task_runner.h"
 #include "base/strings/stringprintf.h"
-#include "base/task_runner_util.h"
-#include "base/values.h"
 #include "chrome/browser/drive/drive_api_util.h"
 #include "content/public/browser/browser_thread.h"
 #include "google_apis/drive/auth_service.h"
@@ -149,6 +146,16 @@ void ExtractOpenUrlAndRun(const std::string& app_id,
   callback.Run(GDATA_OTHER_ERROR, GURL());
 }
 
+void ExtractShareUrlAndRun(const google_apis::GetShareUrlCallback& callback,
+                           google_apis::GDataErrorCode error,
+                           scoped_ptr<google_apis::ResourceEntry> entry) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  const google_apis::Link* share_link =
+      entry ? entry->GetLinkByType(google_apis::Link::LINK_SHARE) : NULL;
+  callback.Run(error, share_link ? share_link->href() : GURL());
+}
+
 // Ignores the |entry|, and runs the |callback|.
 void EntryActionCallbackAdapter(
     const EntryActionCallback& callback,
@@ -376,7 +383,7 @@ CancelCallback DriveAPIService::GetShareUrl(
                                   wapi_url_generator_,
                                   resource_id,
                                   embed_origin,
-                                  base::Bind(&util::ParseShareUrlAndRun,
+                                  base::Bind(&ExtractShareUrlAndRun,
                                              callback)));
 }
 

chrome/browser/drive/drive_api_util.cc

@@ -136,29 +136,6 @@ std::string CanonicalizeResourceId(const std::string& resource_id) {
   return resource_id;
 }
 
-void ParseShareUrlAndRun(const google_apis::GetShareUrlCallback& callback,
-                         google_apis::GDataErrorCode error,
-                         scoped_ptr<base::Value> value) {
-  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
-
-  if (!value) {
-    callback.Run(error, GURL());
-    return;
-  }
-
-  // Parsing ResourceEntry is cheap enough to do on UI thread.
-  scoped_ptr<google_apis::ResourceEntry> entry =
-      google_apis::ResourceEntry::ExtractAndParse(*value);
-  if (!entry) {
-    callback.Run(google_apis::GDATA_PARSE_ERROR, GURL());
-    return;
-  }
-
-  const google_apis::Link* share_link =
-      entry->GetLinkByType(google_apis::Link::LINK_SHARE);
-  callback.Run(error, share_link ? share_link->href() : GURL());
-}
-
 scoped_ptr<google_apis::ResourceEntry>
 ConvertFileResourceToResourceEntry(
     const google_apis::FileResource& file_resource) {

chrome/browser/drive/drive_api_util.h

@@ -63,12 +63,6 @@ std::string ExtractResourceIdFromUrl(const GURL& url);
 // into the new format.
 std::string CanonicalizeResourceId(const std::string& resource_id);
 
-// Extracts an url to the sharing dialog and returns it via |callback|. If
-// the share url doesn't exist, then an empty url is returned.
-void ParseShareUrlAndRun(const google_apis::GetShareUrlCallback& callback,
-                         google_apis::GDataErrorCode error,
-                         scoped_ptr<base::Value> value);
-
 // Converts FileResource to ResourceEntry.
 scoped_ptr<google_apis::ResourceEntry>
 ConvertFileResourceToResourceEntry(

google_apis/drive/base_requests.cc

@@ -43,29 +43,18 @@ const char kUploadResponseLocation[] = "location";
 const char kUploadContentRange[] = "Content-Range: bytes ";
 const char kUploadResponseRange[] = "range";
 
-// Parse JSON string to base::Value object.
-scoped_ptr<base::Value> ParseJsonInternal(const std::string& json) {
-  int error_code = -1;
-  std::string error_message;
-  scoped_ptr<base::Value> value(base::JSONReader::ReadAndReturnError(
-      json, base::JSON_PARSE_RFC, &error_code, &error_message));
-
-  if (!value.get()) {
-    std::string trimmed_json;
-    if (json.size() < 80) {
-      trimmed_json  = json;
-    } else {
-      // Take the first 50 and the last 10 bytes.
-      trimmed_json = base::StringPrintf(
-          "%s [%s bytes] %s",
-          json.substr(0, 50).c_str(),
-          base::Uint64ToString(json.size() - 60).c_str(),
-          json.substr(json.size() - 10).c_str());
-    }
-    LOG(WARNING) << "Error while parsing entry response: " << error_message
-                 << ", code: " << error_code << ", json:\n" << trimmed_json;
-  }
-  return value.Pass();
+// Parses JSON passed in |json| on |blocking_task_runner|. Runs |callback| on
+// the calling thread when finished with either success or failure.
+// The callback must not be null.
+void ParseJsonOnBlockingPool(
+    base::TaskRunner* blocking_task_runner,
+    const std::string& json,
+    const base::Callback<void(scoped_ptr<base::Value> value)>& callback) {
+  base::PostTaskAndReplyWithResult(
+      blocking_task_runner,
+      FROM_HERE,
+      base::Bind(&google_apis::ParseJson, json),
+      callback);
 }
 
 // Returns response headers as a string. Returns a warning message if
@@ -95,14 +84,28 @@ bool IsSuccessfulResponseCode(int response_code) {
 
 namespace google_apis {
 
-void ParseJson(base::TaskRunner* blocking_task_runner,
-               const std::string& json,
-               const ParseJsonCallback& callback) {
-  base::PostTaskAndReplyWithResult(
-      blocking_task_runner,
-      FROM_HERE,
-      base::Bind(&ParseJsonInternal, json),
-      callback);
+scoped_ptr<base::Value> ParseJson(const std::string& json) {
+  int error_code = -1;
+  std::string error_message;
+  scoped_ptr<base::Value> value(base::JSONReader::ReadAndReturnError(
+      json, base::JSON_PARSE_RFC, &error_code, &error_message));
+
+  if (!value.get()) {
+    std::string trimmed_json;
+    if (json.size() < 80) {
+      trimmed_json  = json;
+    } else {
+      // Take the first 50 and the last 10 bytes.
+      trimmed_json = base::StringPrintf(
+          "%s [%s bytes] %s",
+          json.substr(0, 50).c_str(),
+          base::Uint64ToString(json.size() - 60).c_str(),
+          json.substr(json.size() - 10).c_str());
+    }
+    LOG(WARNING) << "Error while parsing entry response: " << error_message
+                 << ", code: " << error_code << ", json:\n" << trimmed_json;
+  }
+  return value.Pass();
 }
 
 //=========================== ResponseWriter ==================================
@@ -359,7 +362,7 @@ void UrlFetchRequestBase::OnURLFetchComplete(const URLFetcher* source) {
     const char kErrorReasonUserRateLimitExceeded[] = "userRateLimitExceeded";
     const char kErrorReasonQuotaExceeded[] = "quotaExceeded";
 
-    scoped_ptr<base::Value> value(ParseJsonInternal(response_writer_->data()));
+    scoped_ptr<base::Value> value(ParseJson(response_writer_->data()));
     base::DictionaryValue* dictionary = NULL;
     base::DictionaryValue* error = NULL;
     if (value &&
@@ -434,62 +437,6 @@ void EntryActionRequest::RunCallbackOnPrematureFailure(GDataErrorCode code) {
   callback_.Run(code);
 }
 
-//============================== GetDataRequest ==============================
-
-GetDataRequest::GetDataRequest(RequestSender* sender,
-                               const GetDataCallback& callback)
-    : UrlFetchRequestBase(sender),
-      callback_(callback),
-      weak_ptr_factory_(this) {
-  DCHECK(!callback_.is_null());
-}
-
-GetDataRequest::~GetDataRequest() {}
-
-void GetDataRequest::ParseResponse(GDataErrorCode fetch_error_code,
-                                   const std::string& data) {
-  DCHECK(CalledOnValidThread());
-
-  VLOG(1) << "JSON received from " << GetURL().spec() << ": "
-          << data.size() << " bytes";
-  ParseJson(blocking_task_runner(),
-            data,
-            base::Bind(&GetDataRequest::OnDataParsed,
-                       weak_ptr_factory_.GetWeakPtr(),
-                       fetch_error_code));
-}
-
-void GetDataRequest::ProcessURLFetchResults(const URLFetcher* source) {
-  GDataErrorCode fetch_error_code = GetErrorCode();
-
-  switch (fetch_error_code) {
-    case HTTP_SUCCESS:
-    case HTTP_CREATED:
-      ParseResponse(fetch_error_code, response_writer()->data());
-      break;
-    default:
-      RunCallbackOnPrematureFailure(fetch_error_code);
-      OnProcessURLFetchResultsComplete();
-      break;
-  }
-}
-
-void GetDataRequest::RunCallbackOnPrematureFailure(
-    GDataErrorCode fetch_error_code) {
-  callback_.Run(fetch_error_code, scoped_ptr<base::Value>());
-}
-
-void GetDataRequest::OnDataParsed(GDataErrorCode fetch_error_code,
-                                  scoped_ptr<base::Value> value) {
-  DCHECK(CalledOnValidThread());
-
-  if (!value.get())
-    fetch_error_code = GDATA_PARSE_ERROR;
-
-  callback_.Run(fetch_error_code, value.Pass());
-  OnProcessURLFetchResultsComplete();
-}
-
 //========================= InitiateUploadRequestBase ========================
 
 InitiateUploadRequestBase::InitiateUploadRequestBase(
@@ -618,11 +565,11 @@ void UploadRangeRequestBase::ProcessURLFetchResults(
   } else if (code == HTTP_CREATED || code == HTTP_SUCCESS) {
     // The upload is successfully done. Parse the response which should be
     // the entry's metadata.
-    ParseJson(blocking_task_runner(),
-              response_writer()->data(),
-              base::Bind(&UploadRangeRequestBase::OnDataParsed,
-                         weak_ptr_factory_.GetWeakPtr(),
-                         code));
+    ParseJsonOnBlockingPool(blocking_task_runner(),
+                            response_writer()->data(),
+                            base::Bind(&UploadRangeRequestBase::OnDataParsed,
+                                       weak_ptr_factory_.GetWeakPtr(),
+                                       code));
   } else {
     // Failed to upload. Run callbacks to notify the error.
     OnRangeRequestComplete(

google_apis/drive/base_requests.h

@@ -29,10 +29,6 @@ namespace google_apis {
 
 class RequestSender;
 
-// Callback used to pass parsed JSON from ParseJson(). If parsing error occurs,
-// then the passed argument is null.
-typedef base::Callback<void(scoped_ptr<base::Value> value)> ParseJsonCallback;
-
 // Callback used for DownloadFileRequest and ResumeUploadRequestBase.
 typedef base::Callback<void(int64 progress, int64 total)> ProgressCallback;
 
@@ -41,12 +37,8 @@ typedef base::Callback<void(
     GDataErrorCode error,
     scoped_ptr<std::string> content)> GetContentCallback;
 
-// Parses JSON passed in |json| on |blocking_task_runner|. Runs |callback| on
-// the calling thread when finished with either success or failure.
-// The callback must not be null.
-void ParseJson(base::TaskRunner* blocking_task_runner,
-               const std::string& json,
-               const ParseJsonCallback& callback);
+// Parses JSON passed in |json|. Returns NULL on failure.
+scoped_ptr<base::Value> ParseJson(const std::string& json);
 
 //======================= AuthenticatedRequestInterface ======================
 
@@ -249,45 +241,6 @@ class EntryActionRequest : public UrlFetchRequestBase {
   DISALLOW_COPY_AND_ASSIGN(EntryActionRequest);
 };
 
-//============================== GetDataRequest ==============================
-
-// Callback type for requests that returns JSON data.
-typedef base::Callback<void(GDataErrorCode error,
-                            scoped_ptr<base::Value> json_data)> GetDataCallback;
-
-// This class performs the request for fetching and converting the fetched
-// content into a base::Value.
-class GetDataRequest : public UrlFetchRequestBase {
- public:
-  // |callback| is called when the request finishes either by success or by
-  // failure. On success, a JSON Value object is passed. It must not be null.
-  GetDataRequest(RequestSender* sender, const GetDataCallback& callback);
-  virtual ~GetDataRequest();
-
- protected:
-  // UrlFetchRequestBase overrides.
-  virtual void ProcessURLFetchResults(const net::URLFetcher* source) OVERRIDE;
-  virtual void RunCallbackOnPrematureFailure(
-      GDataErrorCode fetch_error_code) OVERRIDE;
-
- private:
-  // Parses JSON response.
-  void ParseResponse(GDataErrorCode fetch_error_code, const std::string& data);
-
-  // Called when ParseJsonOnBlockingPool() is completed.
-  void OnDataParsed(GDataErrorCode fetch_error_code,
-                    scoped_ptr<base::Value> value);
-
-  const GetDataCallback callback_;
-
-  // Note: This should remain the last member so it'll be destroyed and
-  // invalidate its weak pointers before any other members are destroyed.
-  base::WeakPtrFactory<GetDataRequest> weak_ptr_factory_;
-
-  DISALLOW_COPY_AND_ASSIGN(GetDataRequest);
-};
-
-
 //=========================== InitiateUploadRequestBase=======================
 
 // Callback type for DriveServiceInterface::InitiateUpload.

google_apis/drive/base_requests_unittest.cc

@@ -51,24 +51,6 @@ class FakeUrlFetchRequest : public UrlFetchRequestBase {
   GURL url_;
 };
 
-class FakeGetDataRequest : public GetDataRequest {
- public:
-  explicit FakeGetDataRequest(RequestSender* sender,
-                              const GetDataCallback& callback,
-                              const GURL& url)
-      : GetDataRequest(sender, callback),
-        url_(url) {
-  }
-
-  virtual ~FakeGetDataRequest() {
-  }
-
- protected:
-  virtual GURL GetURL() const OVERRIDE { return url_; }
-
-  GURL url_;
-};
-
 }  // namespace
 
 class BaseRequestsTest : public testing::Test {
@@ -109,11 +91,7 @@ class BaseRequestsTest : public testing::Test {
 };
 
 TEST_F(BaseRequestsTest, ParseValidJson) {
-  scoped_ptr<base::Value> json;
-  ParseJson(message_loop_.message_loop_proxy(),
-            kValidJsonString,
-            base::Bind(test_util::CreateCopyResultCallback(&json)));
-  base::RunLoop().RunUntilIdle();
+  scoped_ptr<base::Value> json(ParseJson(kValidJsonString));
 
   base::DictionaryValue* root_dict = NULL;
   ASSERT_TRUE(json);
@@ -125,14 +103,7 @@ TEST_F(BaseRequestsTest, ParseValidJson) {
 }
 
 TEST_F(BaseRequestsTest, ParseInvalidJson) {
-  // Initialize with a valid pointer to verify that null is indeed assigned.
-  scoped_ptr<base::Value> json(base::Value::CreateNullValue());
-  ParseJson(message_loop_.message_loop_proxy(),
-            kInvalidJsonString,
-            base::Bind(test_util::CreateCopyResultCallback(&json)));
-  base::RunLoop().RunUntilIdle();
-
-  EXPECT_FALSE(json);
+  EXPECT_FALSE(ParseJson(kInvalidJsonString));
 }
 
 TEST_F(BaseRequestsTest, UrlFetchRequestBaseResponseCodeOverride) {
@@ -165,40 +136,4 @@ TEST_F(BaseRequestsTest, UrlFetchRequestBaseResponseCodeOverride) {
   EXPECT_EQ(HTTP_SERVICE_UNAVAILABLE, error);
 }
 
-TEST_F(BaseRequestsTest, GetDataRequestParseValidResponse) {
-  response_body_ = kValidJsonString;
-
-  GDataErrorCode error = GDATA_OTHER_ERROR;
-  scoped_ptr<base::Value> value;
-  base::RunLoop run_loop;
-  sender_->StartRequestWithRetry(
-      new FakeGetDataRequest(
-          sender_.get(),
-          test_util::CreateQuitCallback(
-              &run_loop, test_util::CreateCopyResultCallback(&error, &value)),
-          test_server_.base_url()));
-  run_loop.Run();
-
-  EXPECT_EQ(HTTP_SUCCESS, error);
-  EXPECT_TRUE(value);
-}
-
-TEST_F(BaseRequestsTest, GetDataRequestParseInvalidResponse) {
-  response_body_ = kInvalidJsonString;
-
-  GDataErrorCode error = GDATA_OTHER_ERROR;
-  scoped_ptr<base::Value> value;
-  base::RunLoop run_loop;
-  sender_->StartRequestWithRetry(
-      new FakeGetDataRequest(
-          sender_.get(),
-          test_util::CreateQuitCallback(
-              &run_loop, test_util::CreateCopyResultCallback(&error, &value)),
-          test_server_.base_url()));
-  run_loop.Run();
-
-  EXPECT_EQ(GDATA_PARSE_ERROR, error);
-  EXPECT_FALSE(value);
-}
-
 }  // namespace google_apis

google_apis/drive/drive_api_requests.cc

@@ -11,7 +11,6 @@
 #include "base/sequenced_task_runner.h"
 #include "base/task_runner_util.h"
 #include "base/values.h"
-#include "google_apis/drive/drive_api_parser.h"
 #include "google_apis/drive/request_sender.h"
 #include "google_apis/drive/request_util.h"
 #include "google_apis/drive/time_util.h"
@@ -23,70 +22,6 @@ namespace {
 const char kContentTypeApplicationJson[] = "application/json";
 const char kParentLinkKind[] = "drive#fileLink";
 
-// Parses the JSON value to a resource typed |T| and runs |callback| on the UI
-// thread once parsing is done.
-template<typename T>
-void ParseJsonAndRun(
-    const base::Callback<void(GDataErrorCode, scoped_ptr<T>)>& callback,
-    GDataErrorCode error,
-    scoped_ptr<base::Value> value) {
-  DCHECK(!callback.is_null());
-
-  scoped_ptr<T> resource;
-  if (value) {
-    resource = T::CreateFrom(*value);
-    if (!resource) {
-      // Failed to parse the JSON value, although the JSON value is available,
-      // so let the callback know the parsing error.
-      error = GDATA_PARSE_ERROR;
-    }
-  }
-
-  callback.Run(error, resource.Pass());
-}
-
-// Thin adapter of T::CreateFrom.
-template<typename T>
-scoped_ptr<T> ParseJsonOnBlockingPool(scoped_ptr<base::Value> value) {
-  return T::CreateFrom(*value);
-}
-
-// Runs |callback| with given |error| and |value|. If |value| is null,
-// overwrites |error| to GDATA_PARSE_ERROR.
-template<typename T>
-void ParseJsonOnBlockingPoolAndRunAfterBlockingPoolTask(
-    const base::Callback<void(GDataErrorCode, scoped_ptr<T>)>& callback,
-    GDataErrorCode error, scoped_ptr<T> value) {
-  if (!value)
-    error = GDATA_PARSE_ERROR;
-  callback.Run(error, value.Pass());
-}
-
-// Parses the JSON value to a resource typed |T| and runs |callback| on
-// blocking pool, and then run on the current thread.
-// TODO(hidehiko): Move this and ParseJsonAndRun defined above into base with
-// merging the tasks running on blocking pool into one.
-template<typename T>
-void ParseJsonOnBlockingPoolAndRun(
-    scoped_refptr<base::TaskRunner> blocking_task_runner,
-    const base::Callback<void(GDataErrorCode, scoped_ptr<T>)>& callback,
-    GDataErrorCode error,
-    scoped_ptr<base::Value> value) {
-  DCHECK(!callback.is_null());
-
-  if (!value) {
-    callback.Run(error, scoped_ptr<T>());
-    return;
-  }
-
-  base::PostTaskAndReplyWithResult(
-      blocking_task_runner,
-      FROM_HERE,
-      base::Bind(&ParseJsonOnBlockingPool<T>, base::Passed(&value)),
-      base::Bind(&ParseJsonOnBlockingPoolAndRunAfterBlockingPoolTask<T>,
-                 callback, error));
-}
-
 // Parses the JSON value to FileResource instance and runs |callback| on the
 // UI thread once parsing is done.
 // This is customized version of ParseJsonAndRun defined above to adapt the
@@ -126,17 +61,16 @@ scoped_ptr<base::DictionaryValue> CreateParentValue(
 
 namespace drive {
 
-//============================ DriveApiDataRequest ===========================
+//============================ DriveApiPartialFieldRequest ====================
 
-DriveApiDataRequest::DriveApiDataRequest(RequestSender* sender,
-                                         const GetDataCallback& callback)
-    : GetDataRequest(sender, callback) {
+DriveApiPartialFieldRequest::DriveApiPartialFieldRequest(
+    RequestSender* sender) : UrlFetchRequestBase(sender) {
 }
 
-DriveApiDataRequest::~DriveApiDataRequest() {
+DriveApiPartialFieldRequest::~DriveApiPartialFieldRequest() {
 }
 
-GURL DriveApiDataRequest::GetURL() const {
+GURL DriveApiPartialFieldRequest::GetURL() const {
   GURL url = GetURLInternal();
   if (!fields_.empty())
     url = net::AppendOrReplaceQueryParameter(url, "fields", fields_);
@@ -149,9 +83,7 @@ FilesGetRequest::FilesGetRequest(
     RequestSender* sender,
     const DriveApiUrlGenerator& url_generator,
     const FileResourceCallback& callback)
-    : DriveApiDataRequest(
-          sender,
-          base::Bind(&ParseJsonAndRun<FileResource>, callback)),
+    : DriveApiDataRequest<FileResource>(sender, callback),
       url_generator_(url_generator) {
   DCHECK(!callback.is_null());
 }
@@ -168,9 +100,7 @@ FilesAuthorizeRequest::FilesAuthorizeRequest(
     RequestSender* sender,
     const DriveApiUrlGenerator& url_generator,
     const FileResourceCallback& callback)
-    : DriveApiDataRequest(
-          sender,
-          base::Bind(&ParseJsonAndRun<FileResource>, callback)),
+    : DriveApiDataRequest<FileResource>(sender, callback),
       url_generator_(url_generator) {
   DCHECK(!callback.is_null());
 }
@@ -191,9 +121,7 @@ FilesInsertRequest::FilesInsertRequest(
     RequestSender* sender,
     const DriveApiUrlGenerator& url_generator,
     const FileResourceCallback& callback)
-    : DriveApiDataRequest(
-          sender,
-          base::Bind(&ParseJsonAndRun<FileResource>, callback)),
+    : DriveApiDataRequest<FileResource>(sender, callback),
       url_generator_(url_generator) {
   DCHECK(!callback.is_null());
 }
@@ -250,9 +178,7 @@ FilesPatchRequest::FilesPatchRequest(
     RequestSender* sender,
     const DriveApiUrlGenerator& url_generator,
     const FileResourceCallback& callback)
-    : DriveApiDataRequest(
-          sender,
-          base::Bind(&ParseJsonAndRun<FileResource>, callback)),
+    : DriveApiDataRequest<FileResource>(sender, callback),
       url_generator_(url_generator),
       set_modified_date_(false),
       update_viewed_date_(true) {
@@ -320,9 +246,7 @@ FilesCopyRequest::FilesCopyRequest(
     RequestSender* sender,
     const DriveApiUrlGenerator& url_generator,
     const FileResourceCallback& callback)
-    : DriveApiDataRequest(
-          sender,
-          base::Bind(&ParseJsonAndRun<FileResource>, callback)),
+    : DriveApiDataRequest<FileResource>(sender, callback),
       url_generator_(url_generator) {
   DCHECK(!callback.is_null());
 }
@@ -375,11 +299,7 @@ FilesListRequest::FilesListRequest(
     RequestSender* sender,
     const DriveApiUrlGenerator& url_generator,
     const FileListCallback& callback)
-    : DriveApiDataRequest(
-          sender,
-          base::Bind(&ParseJsonOnBlockingPoolAndRun<FileList>,
-                     make_scoped_refptr(sender->blocking_task_runner()),
-                     callback)),
+    : DriveApiDataRequest<FileList>(sender, callback),
       url_generator_(url_generator),
       max_results_(100) {
   DCHECK(!callback.is_null());
@@ -396,11 +316,7 @@ GURL FilesListRequest::GetURLInternal() const {
 FilesListNextPageRequest::FilesListNextPageRequest(
     RequestSender* sender,
     const FileListCallback& callback)
-    : DriveApiDataRequest(
-          sender,
-          base::Bind(&ParseJsonOnBlockingPoolAndRun<FileList>,
-                     make_scoped_refptr(sender->blocking_task_runner()),
-                     callback)) {
+    : DriveApiDataRequest<FileList>(sender, callback) {
   DCHECK(!callback.is_null());
 }
 
@@ -445,9 +361,7 @@ FilesTrashRequest::FilesTrashRequest(
     RequestSender* sender,
     const DriveApiUrlGenerator& url_generator,
     const FileResourceCallback& callback)
-    : DriveApiDataRequest(
-          sender,
-          base::Bind(&ParseJsonAndRun<FileResource>, callback)),
+    : DriveApiDataRequest<FileResource>(sender, callback),
       url_generator_(url_generator) {
   DCHECK(!callback.is_null());
 }
@@ -468,9 +382,7 @@ AboutGetRequest::AboutGetRequest(
     RequestSender* sender,
     const DriveApiUrlGenerator& url_generator,
     const AboutResourceCallback& callback)
-    : DriveApiDataRequest(
-          sender,
-          base::Bind(&ParseJsonAndRun<AboutResource>, callback)),
+    : DriveApiDataRequest<AboutResource>(sender, callback),
       url_generator_(url_generator) {
   DCHECK(!callback.is_null());
 }
@@ -487,11 +399,7 @@ ChangesListRequest::ChangesListRequest(
     RequestSender* sender,
     const DriveApiUrlGenerator& url_generator,
     const ChangeListCallback& callback)
-    : DriveApiDataRequest(
-          sender,
-          base::Bind(&ParseJsonOnBlockingPoolAndRun<ChangeList>,
-                     make_scoped_refptr(sender->blocking_task_runner()),
-                     callback)),
+    : DriveApiDataRequest<ChangeList>(sender, callback),
       url_generator_(url_generator),
       include_deleted_(true),
       max_results_(100),
@@ -511,11 +419,7 @@ GURL ChangesListRequest::GetURLInternal() const {
 ChangesListNextPageRequest::ChangesListNextPageRequest(
     RequestSender* sender,
     const ChangeListCallback& callback)
-    : DriveApiDataRequest(
-          sender,
-          base::Bind(&ParseJsonOnBlockingPoolAndRun<ChangeList>,
-                     make_scoped_refptr(sender->blocking_task_runner()),
-                     callback)) {
+    : DriveApiDataRequest<ChangeList>(sender, callback) {
   DCHECK(!callback.is_null());
 }
 
@@ -533,9 +437,7 @@ AppsListRequest::AppsListRequest(
     const DriveApiUrlGenerator& url_generator,
     bool use_internal_endpoint,
     const AppListCallback& callback)
-    : DriveApiDataRequest(
-          sender,
-          base::Bind(&ParseJsonAndRun<AppList>, callback)),
+    : DriveApiDataRequest<AppList>(sender, callback),
       url_generator_(url_generator),
       use_internal_endpoint_(use_internal_endpoint) {
   DCHECK(!callback.is_null());

google_apis/drive/gdata_wapi_requests.cc

@@ -4,22 +4,37 @@
 
 #include "google_apis/drive/gdata_wapi_requests.h"
 
+#include "base/location.h"
+#include "base/sequenced_task_runner.h"
+#include "base/task_runner_util.h"
+#include "base/values.h"
+#include "google_apis/drive/gdata_wapi_parser.h"
 #include "google_apis/drive/gdata_wapi_url_generator.h"
 
 namespace google_apis {
 
-//============================ GetResourceEntryRequest =======================
+namespace {
+
+scoped_ptr<ResourceEntry> ParseResourceEntry(const std::string& json) {
+  scoped_ptr<base::Value> value = ParseJson(json);
+  return value ? ResourceEntry::ExtractAndParse(*value) :
+      scoped_ptr<ResourceEntry>();
+}
+
+}  // namespace
 
 GetResourceEntryRequest::GetResourceEntryRequest(
     RequestSender* sender,
     const GDataWapiUrlGenerator& url_generator,
     const std::string& resource_id,
     const GURL& embed_origin,
-    const GetDataCallback& callback)
-    : GetDataRequest(sender, callback),
+    const GetResourceEntryCallback& callback)
+    : UrlFetchRequestBase(sender),
       url_generator_(url_generator),
       resource_id_(resource_id),
-      embed_origin_(embed_origin) {
+      embed_origin_(embed_origin),
+      callback_(callback),
+      weak_ptr_factory_(this) {
   DCHECK(!callback.is_null());
 }
 
@@ -30,4 +45,37 @@ GURL GetResourceEntryRequest::GetURL() const {
       resource_id_, embed_origin_);
 }
 
+void GetResourceEntryRequest::ProcessURLFetchResults(
+    const net::URLFetcher* source) {
+  GDataErrorCode error = GetErrorCode();
+  switch (error) {
+    case HTTP_SUCCESS:
+    case HTTP_CREATED:
+      base::PostTaskAndReplyWithResult(
+          blocking_task_runner(),
+          FROM_HERE,
+          base::Bind(&ParseResourceEntry, response_writer()->data()),
+          base::Bind(&GetResourceEntryRequest::OnDataParsed,
+                     weak_ptr_factory_.GetWeakPtr(), error));
+      break;
+    default:
+      RunCallbackOnPrematureFailure(error);
+      OnProcessURLFetchResultsComplete();
+      break;
+  }
+}
+
+void GetResourceEntryRequest::RunCallbackOnPrematureFailure(
+    GDataErrorCode error) {
+  callback_.Run(error, scoped_ptr<ResourceEntry>());
+}
+
+void GetResourceEntryRequest::OnDataParsed(GDataErrorCode error,
+                                           scoped_ptr<ResourceEntry> entry) {
+  if (!entry)
+    error = GDATA_PARSE_ERROR;
+  callback_.Run(error, entry.Pass());
+  OnProcessURLFetchResultsComplete();
+}
+
 }  // namespace google_apis

google_apis/drive/gdata_wapi_requests.h

@@ -12,29 +12,44 @@
 
 namespace google_apis {
 
-//========================= GetResourceEntryRequest ==========================
+class ResourceEntry;
+
+// Callback type for GetResourceEntryRequest.
+typedef base::Callback<void(GDataErrorCode error,
+                            scoped_ptr<ResourceEntry> entry)>
+    GetResourceEntryCallback;
 
 // This class performs the request for fetching a single resource entry.
-class GetResourceEntryRequest : public GetDataRequest {
+class GetResourceEntryRequest : public UrlFetchRequestBase {
  public:
   // |callback| must not be null.
   GetResourceEntryRequest(RequestSender* sender,
                           const GDataWapiUrlGenerator& url_generator,
                           const std::string& resource_id,
                           const GURL& embed_origin,
-                          const GetDataCallback& callback);
+                          const GetResourceEntryCallback& callback);
   virtual ~GetResourceEntryRequest();
 
  protected:
   // UrlFetchRequestBase overrides.
+  virtual void ProcessURLFetchResults(const net::URLFetcher* source) OVERRIDE;
+  virtual void RunCallbackOnPrematureFailure(GDataErrorCode error) OVERRIDE;
   virtual GURL GetURL() const OVERRIDE;
 
  private:
+  void OnDataParsed(GDataErrorCode error, scoped_ptr<ResourceEntry> entry);
+
   const GDataWapiUrlGenerator url_generator_;
   // Resource id of the requested entry.
   const std::string resource_id_;
   // Embed origin for an url to the sharing dialog. Can be empty.
-  const GURL& embed_origin_;
+  GURL embed_origin_;
+
+  const GetResourceEntryCallback callback_;
+
+  // Note: This should remain the last member so it'll be destroyed and
+  // invalidate its weak pointers before any other members are destroyed.
+  base::WeakPtrFactory<GetResourceEntryRequest> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(GetResourceEntryRequest);
 };

google_apis/drive/gdata_wapi_requests_unittest.cc

@@ -5,7 +5,6 @@
 #include "base/bind.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
-#include "base/values.h"
 #include "google_apis/drive/dummy_auth_service.h"
 #include "google_apis/drive/gdata_wapi_parser.h"
 #include "google_apis/drive/gdata_wapi_requests.h"
@@ -99,7 +98,7 @@ class GDataWapiRequestsTest : public testing::Test {
 
 TEST_F(GDataWapiRequestsTest, GetResourceEntryRequest_ValidResourceId) {
   GDataErrorCode result_code = GDATA_OTHER_ERROR;
-  scoped_ptr<base::Value> result_data;
+  scoped_ptr<ResourceEntry> result_data;
 
   {
     base::RunLoop run_loop;
@@ -120,16 +119,14 @@ TEST_F(GDataWapiRequestsTest, GetResourceEntryRequest_ValidResourceId) {
   EXPECT_EQ("/feeds/default/private/full/file%3A2_file_resource_id"
             "?v=3&alt=json&showroot=true",
             http_request_.relative_url);
-  scoped_ptr<base::Value> expected_json =
-      test_util::LoadJSONFile("gdata/file_entry.json");
-  ASSERT_TRUE(expected_json);
   EXPECT_TRUE(result_data);
-  EXPECT_TRUE(base::Value::Equals(expected_json.get(), result_data.get()));
+  EXPECT_EQ("File 1.mp3", result_data->filename());
+  EXPECT_EQ("3b4382ebefec6e743578c76bbd0575ce", result_data->file_md5());
 }
 
 TEST_F(GDataWapiRequestsTest, GetResourceEntryRequest_InvalidResourceId) {
   GDataErrorCode result_code = GDATA_OTHER_ERROR;
-  scoped_ptr<base::Value> result_data;
+  scoped_ptr<ResourceEntry> result_data;
 
   {
     base::RunLoop run_loop;