Skip to content

Commit

Permalink
Revert 288017 "Parse Drive API responses all at once in the bloc..."
Browse files Browse the repository at this point in the history 
It broke Drive sync completely.

> 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
> 
> Review URL: https://codereview.chromium.org/442193002

TBR=kinaba@chromium.org
BUG=401843

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

Cr-Commit-Position: refs/heads/master@{#288216}
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@288216 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
kinaba@chromium.org committed Aug 8, 2014
1 parent 39cfeda commit d6907a5
Show file tree
Hide file tree
Showing 11 changed files with 393 additions and 234 deletions.
15 changes: 4 additions & 11 deletions chrome/browser/drive/drive_api_service.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@
#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"
Expand Down Expand Up @@ -146,16 +149,6 @@ 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,
Expand Down Expand Up @@ -383,7 +376,7 @@ CancelCallback DriveAPIService::GetShareUrl(
wapi_url_generator_,
resource_id,
embed_origin,
base::Bind(&ExtractShareUrlAndRun,
base::Bind(&util::ParseShareUrlAndRun,
callback)));
}

Expand Down
23 changes: 23 additions & 0 deletions chrome/browser/drive/drive_api_util.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,29 @@ 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) {
Expand Down
6 changes: 6 additions & 0 deletions chrome/browser/drive/drive_api_util.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,12 @@ 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(
Expand Down
133 changes: 93 additions & 40 deletions google_apis/drive/base_requests.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,18 +43,29 @@ const char kUploadResponseLocation[] = "location";
const char kUploadContentRange[] = "Content-Range: bytes ";
const char kUploadResponseRange[] = "range";

// 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);
// 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();
}

// Returns response headers as a string. Returns a warning message if
Expand Down Expand Up @@ -84,28 +95,14 @@ bool IsSuccessfulResponseCode(int response_code) {

namespace google_apis {

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();
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);
}

//=========================== ResponseWriter ==================================
Expand Down Expand Up @@ -362,7 +359,7 @@ void UrlFetchRequestBase::OnURLFetchComplete(const URLFetcher* source) {
const char kErrorReasonUserRateLimitExceeded[] = "userRateLimitExceeded";
const char kErrorReasonQuotaExceeded[] = "quotaExceeded";

scoped_ptr<base::Value> value(ParseJson(response_writer_->data()));
scoped_ptr<base::Value> value(ParseJsonInternal(response_writer_->data()));
base::DictionaryValue* dictionary = NULL;
base::DictionaryValue* error = NULL;
if (value &&
Expand Down Expand Up @@ -437,6 +434,62 @@ 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(
Expand Down Expand Up @@ -565,11 +618,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.
ParseJsonOnBlockingPool(blocking_task_runner(),
response_writer()->data(),
base::Bind(&UploadRangeRequestBase::OnDataParsed,
weak_ptr_factory_.GetWeakPtr(),
code));
ParseJson(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(
Expand Down
51 changes: 49 additions & 2 deletions google_apis/drive/base_requests.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ 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;

Expand All @@ -37,8 +41,12 @@ typedef base::Callback<void(
GDataErrorCode error,
scoped_ptr<std::string> content)> GetContentCallback;

// Parses JSON passed in |json|. Returns NULL on failure.
scoped_ptr<base::Value> ParseJson(const std::string& json);
// 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);

//======================= AuthenticatedRequestInterface ======================

Expand Down Expand Up @@ -241,6 +249,45 @@ 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.
Expand Down
Loading

0 comments on commit d6907a5

Please sign in to comment.