gdrive: Get JSON feeds parsing off the UI thread

chrome/browser/chromeos/gdata/gdata_operations.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/gdata/gdata_operations.h"
 
 #include "base/bind.h"
 #include "base/json/json_reader.h"
 #include "base/metrics/histogram.h"
 #include "base/string_number_conversions.h"
@@ skipping 142 matching lines @@
 // URL for fetching documents in a particular directory.
 GURL FormatDocumentListURL(const std::string& directory_resource_id) {
   if (directory_resource_id.empty())
     return GURL(kGetDocumentListURLForAllDocuments);
 
   return GURL(base::StringPrintf(kGetDocumentListURLForDirectoryFormat,
                                  net::EscapePath(
                                      directory_resource_id).c_str()));
 }
 
+// Parse JSON string to base::Value object.
+void ParseJsonOnBlockingPool(const std::string& data,
+                             scoped_ptr<base::Value>* value) {
+  DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  int error_code = -1;
+  std::string error_message;
+  value->reset(base::JSONReader::ReadAndReturnError(data,
+                                                    base::JSON_PARSE_RFC,
+                                                    &error_code,
+                                                    &error_message));
+
+  if (!value->get()) {
+    LOG(ERROR) << "Error while parsing entry response: "
+               << error_message
+               << ", code: "
+               << error_code
+               << ", data:\n"
+               << data;
+  }
+}
+
 }  // namespace
 
 namespace gdata {
 
 //================================ AuthOperation ===============================
 
 AuthOperation::AuthOperation(GDataOperationRegistry* registry,
                              Profile* profile,
                              const AuthStatusCallback& callback,
                              const std::string& refresh_token)
@@ skipping 276 matching lines @@
                                    Profile* profile,
                                    const GetDataCallback& callback)
     : UrlFetchOperationBase(registry, profile), callback_(callback) {
 }
 
 GetDataOperation::~GetDataOperation() {}
 
 bool GetDataOperation::ProcessURLFetchResults(const net::URLFetcher* source) {
   std::string data;
   source->GetResponseAsString(&data);
-  scoped_ptr<base::Value> root_value;
   GDataErrorCode code = GetErrorCode(source);
+  bool ret = false;
 
   switch (code) {
     case HTTP_SUCCESS:
-    case HTTP_CREATED: {
-      root_value.reset(ParseResponse(data));
-      if (!root_value.get())
-        code = GDATA_PARSE_ERROR;
-
+    case HTTP_CREATED:
+      ret = ParseResponse(code, data);
       break;
-    }
     default:

[hashimoto, 2012/07/10 03:48:39]

Shouldn't we call the callback on error cases?

[yoshiki, 2012/07/11 00:47:14]

Done.

       break;
   }
 
-  if (!callback_.is_null())
-    callback_.Run(code, root_value.Pass());
-  return root_value.get() != NULL;
+  return ret;
 }
 
 void GetDataOperation::RunCallbackOnPrematureFailure(GDataErrorCode code) {
   if (!callback_.is_null()) {
     scoped_ptr<base::Value> root_value;
     callback_.Run(code, root_value.Pass());
   }
 }
 
-base::Value* GetDataOperation::ParseResponse(const std::string& data) {
-  int error_code = -1;
-  std::string error_message;
-  scoped_ptr<base::Value> root_value(base::JSONReader::ReadAndReturnError(
-      data, base::JSON_PARSE_RFC, &error_code, &error_message));
-  if (!root_value.get()) {
-    LOG(ERROR) << "Error while parsing entry response: "
-               << error_message
-               << ", code: "
-               << error_code
-               << ", data:\n"
-               << data;
-    return NULL;
-  }
-  return root_value.release();
+bool GetDataOperation::ParseResponse(GDataErrorCode code,

[hashimoto, 2012/07/10 03:48:39]

Please rename |code| to more informative name, see the comment on
gdata_operations.h.

[yoshiki, 2012/07/11 00:47:14]

Done.

+                                     const std::string& data) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  // This pointer of scped_ptr is to ensure a deletion of the parsed json value
+  // object, even when OnDataParsed() is not called.
+  scoped_ptr<base::Value>* parsed_value = new scoped_ptr<base::Value>();

[hashimoto, 2012/07/10 03:48:39]

I've once tried to introduce this pointer-to-scoped_ptr pattern, but got strong
opposition from reviewers (See
http://codereview.chromium.org/10535145/diff/3001/chrome/browser/chromeos/gda...
if you like).
Can't we use base::Value::DeepCopy() to avoid this pattern?
Maybe it will not cost so much.

[yoshiki, 2012/07/11 00:47:14]

As I added the comment, sometimes json might be so big that deep-copy takes a
lot of time. So I think it is better to keep this way.

On 2012/07/10 03:48:39, hashimoto wrote:

+
+  return BrowserThread::PostBlockingPoolTaskAndReply(
+      FROM_HERE,
+      base::Bind(&ParseJsonOnBlockingPool,
+                 data,
+                 parsed_value),
+      base::Bind(&GetDataOperation::OnDataParsed,
+                 AsWeakPtr(),
+                 code,
+                 base::Owned(parsed_value)));
+}
+
+void GetDataOperation::RunCallback(GDataErrorCode code,

[hashimoto, 2012/07/10 03:48:39]

ditto as ParseResponse().

[yoshiki, 2012/07/11 00:47:14]

Done.

+                                   scoped_ptr<base::Value> value) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (!callback_.is_null())
+    callback_.Run(code, value.Pass());
+}
+
+void GetDataOperation::OnDataParsed(GDataErrorCode code,

[hashimoto, 2012/07/10 03:48:39]

ditto as ParseResponse().

[yoshiki, 2012/07/11 00:47:14]

Done.

+                                    scoped_ptr<base::Value>* value) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (!value->get())
+    code = GDATA_PARSE_ERROR;
+
+  // The ownership of the parsed json object is transfered to RunCallBack(),
+  // keeping the ownership of the |value| here.
+  RunCallback(code, value->Pass());
+  DCHECK(!value->get());
+
+  // |value| will be deleted after return beause it is base::Owned()'d.
 }
 
 //============================ GetDocumentsOperation ===========================
 
 GetDocumentsOperation::GetDocumentsOperation(
     GDataOperationRegistry* registry,
     Profile* profile,
     int start_changestamp,
     const std::string& search_string,
     const std::string& directory_resource_id,
@@ skipping 340 matching lines @@
   xml_writer.WriteElement("docs:authorizedApp", app_id_);
 
   xml_writer.EndElement();  // Ends "entry" element.
   xml_writer.StopWriting();
   upload_content->assign(xml_writer.GetWrittenString());
   DVLOG(1) << "AuthorizeAppOperation data: " << *upload_content_type << ", ["
            << *upload_content << "]";
   return true;
 }
 
-base::Value* AuthorizeAppsOperation::ParseResponse(const std::string& data) {
+bool AuthorizeAppsOperation::ParseResponse(GDataErrorCode code,
+                                           const std::string& data) {
   // Parse entry XML.
   XmlReader xml_reader;
   scoped_ptr<DocumentEntry> entry;
   if (xml_reader.Load(data)) {
     while (xml_reader.Read()) {
       if (xml_reader.NodeName() == DocumentEntry::GetEntryNodeName()) {
         entry.reset(DocumentEntry::CreateFromXml(&xml_reader));
         break;
       }
     }
   }
 
   // From the response, we create a list of the links returned, since those
   // are the only things we are interested in.
   scoped_ptr<base::ListValue> link_list(new ListValue);
   const ScopedVector<Link>& feed_links = entry->links();
   for (ScopedVector<Link>::const_iterator iter = feed_links.begin();
        iter != feed_links.end(); ++iter) {
     if ((*iter)->type() == Link::OPEN_WITH) {
       base::DictionaryValue* link = new DictionaryValue;
       link->SetString(std::string("href"), (*iter)->href().spec());
       link->SetString(std::string("mime_type"), (*iter)->mime_type());
       link->SetString(std::string("title"), (*iter)->title());
       link->SetString(std::string("app_id"), (*iter)->app_id());
       link_list->Append(link);
     }
   }
 
-  return link_list.release();
+  RunCallback(code, link_list.PassAs<base::Value>());
+
+  return true;
 }
 
 GURL AuthorizeAppsOperation::GetURL() const {
   return document_url_;
 }
 
 //======================= AddResourceToDirectoryOperation ======================
 
 AddResourceToDirectoryOperation::AddResourceToDirectoryOperation(
     GDataOperationRegistry* registry,
@@ skipping 324 matching lines @@
   return true;
 }
 
 void ResumeUploadOperation::OnURLFetchUploadProgress(
     const net::URLFetcher* source, int64 current, int64 total) {
   // Adjust the progress values according to the range currently uploaded.
   NotifyProgress(params_.start_range + current, params_.content_length);
 }
 
 }  // namespace gdata