diff options
author | kinaba@chromium.org <kinaba@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-08-07 08:54:45 +0000 |
---|---|---|
committer | kinaba@chromium.org <kinaba@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-08-07 08:54:45 +0000 |
commit | f69b4289516a6427b52ca5dd344104c52606e97b (patch) | |
tree | 65384af97ab68493ea9ab01a36d94e16654d467c /google_apis | |
parent | 824f5a4d70917b5e9589dd029a4ab4784f2347d0 (diff) | |
download | chromium_src-f69b4289516a6427b52ca5dd344104c52606e97b.zip chromium_src-f69b4289516a6427b52ca5dd344104c52606e97b.tar.gz chromium_src-f69b4289516a6427b52ca5dd344104c52606e97b.tar.bz2 |
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
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@288017 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'google_apis')
-rw-r--r-- | google_apis/drive/base_requests.cc | 133 | ||||
-rw-r--r-- | google_apis/drive/base_requests.h | 51 | ||||
-rw-r--r-- | google_apis/drive/base_requests_unittest.cc | 69 | ||||
-rw-r--r-- | google_apis/drive/drive_api_requests.cc | 132 | ||||
-rw-r--r-- | google_apis/drive/drive_api_requests.h | 110 | ||||
-rw-r--r-- | google_apis/drive/gdata_wapi_requests.cc | 54 | ||||
-rw-r--r-- | google_apis/drive/gdata_wapi_requests.h | 23 | ||||
-rw-r--r-- | google_apis/drive/gdata_wapi_requests_unittest.cc | 11 |
8 files changed, 223 insertions, 360 deletions
diff --git a/google_apis/drive/base_requests.cc b/google_apis/drive/base_requests.cc index 0c1a238..0b2f5e0 100644 --- a/google_apis/drive/base_requests.cc +++ b/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 @@ -96,14 +85,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 ================================== @@ -360,7 +363,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 && @@ -435,62 +438,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( @@ -619,11 +566,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( diff --git a/google_apis/drive/base_requests.h b/google_apis/drive/base_requests.h index afe12a0..d0b5a74 100644 --- a/google_apis/drive/base_requests.h +++ b/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. diff --git a/google_apis/drive/base_requests_unittest.cc b/google_apis/drive/base_requests_unittest.cc index f3a97c1..819f3e7 100644 --- a/google_apis/drive/base_requests_unittest.cc +++ b/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 diff --git a/google_apis/drive/drive_api_requests.cc b/google_apis/drive/drive_api_requests.cc index a637571..d711d07 100644 --- a/google_apis/drive/drive_api_requests.cc +++ b/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()); diff --git a/google_apis/drive/drive_api_requests.h b/google_apis/drive/drive_api_requests.h index 59bffd6..355d37e 100644 --- a/google_apis/drive/drive_api_requests.h +++ b/google_apis/drive/drive_api_requests.h @@ -8,17 +8,18 @@ #include <string> #include "base/callback_forward.h" +#include "base/location.h" +#include "base/sequenced_task_runner.h" +#include "base/task_runner_util.h" #include "base/time/time.h" +#include "base/values.h" #include "google_apis/drive/base_requests.h" +#include "google_apis/drive/drive_api_parser.h" #include "google_apis/drive/drive_api_url_generator.h" #include "google_apis/drive/drive_common_callbacks.h" namespace google_apis { -class ChangeList; -class FileResource; -class FileList; - // Callback used for requests that the server returns FileResource data // formatted into JSON value. typedef base::Callback<void(GDataErrorCode error, @@ -37,23 +38,23 @@ typedef base::Callback<void(GDataErrorCode error, namespace drive { -//============================ DriveApiDataRequest =========================== +//============================ DriveApiPartialFieldRequest ==================== // This is base class of the Drive API related requests. All Drive API requests // support partial request (to improve the performance). The function can be // shared among the Drive API requests. // See also https://developers.google.com/drive/performance -class DriveApiDataRequest : public GetDataRequest { +class DriveApiPartialFieldRequest : public UrlFetchRequestBase { public: - DriveApiDataRequest(RequestSender* sender, const GetDataCallback& callback); - virtual ~DriveApiDataRequest(); + explicit DriveApiPartialFieldRequest(RequestSender* sender); + virtual ~DriveApiPartialFieldRequest(); // Optional parameter. const std::string& fields() const { return fields_; } void set_fields(const std::string& fields) { fields_ = fields; } protected: - // Overridden from GetDataRequest. + // UrlFetchRequestBase overrides. virtual GURL GetURL() const OVERRIDE; // Derived classes should override GetURLInternal instead of GetURL() @@ -63,6 +64,73 @@ class DriveApiDataRequest : public GetDataRequest { private: std::string fields_; + DISALLOW_COPY_AND_ASSIGN(DriveApiPartialFieldRequest); +}; + +//============================ DriveApiDataRequest =========================== + +// The base class of Drive API related requests that receive a JSON response +// representing |DataType|. +template<class DataType> +class DriveApiDataRequest : public DriveApiPartialFieldRequest { + public: + typedef base::Callback<void(GDataErrorCode error, + scoped_ptr<DataType> data)> Callback; + + // |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. + DriveApiDataRequest(RequestSender* sender, const Callback& callback) + : DriveApiPartialFieldRequest(sender), + callback_(callback), + weak_ptr_factory_(this) { + DCHECK(!callback_.is_null()); + } + virtual ~DriveApiDataRequest() {} + + protected: + // UrlFetchRequestBase overrides. + virtual void ProcessURLFetchResults(const net::URLFetcher* source) OVERRIDE { + GDataErrorCode error = GetErrorCode(); + switch (error) { + case HTTP_SUCCESS: + case HTTP_CREATED: + base::PostTaskAndReplyWithResult( + blocking_task_runner(), + FROM_HERE, + base::Bind(&DriveApiDataRequest::Parse, response_writer()->data()), + base::Bind(&DriveApiDataRequest::OnDataParsed, + weak_ptr_factory_.GetWeakPtr(), error)); + break; + default: + RunCallbackOnPrematureFailure(error); + OnProcessURLFetchResultsComplete(); + break; + } + } + + virtual void RunCallbackOnPrematureFailure(GDataErrorCode error) OVERRIDE { + callback_.Run(error, scoped_ptr<DataType>()); + } + + private: + // Parses the |json| string by using DataType::CreateFrom. + static scoped_ptr<DataType> Parse(const std::string& json) { + scoped_ptr<base::Value> value = ParseJson(json); + return value ? DataType::CreateFrom(*value) : scoped_ptr<DataType>(); + } + + // Receives the parsed result and invokes the callback. + void OnDataParsed(GDataErrorCode error, scoped_ptr<DataType> value) { + callback_.Run(value ? error : GDATA_PARSE_ERROR, value.Pass()); + OnProcessURLFetchResultsComplete(); + } + + const Callback 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<DriveApiDataRequest> weak_ptr_factory_; + DISALLOW_COPY_AND_ASSIGN(DriveApiDataRequest); }; @@ -71,7 +139,7 @@ class DriveApiDataRequest : public GetDataRequest { // This class performs the request for fetching a file. // This request is mapped to // https://developers.google.com/drive/v2/reference/files/get -class FilesGetRequest : public DriveApiDataRequest { +class FilesGetRequest : public DriveApiDataRequest<FileResource> { public: FilesGetRequest(RequestSender* sender, const DriveApiUrlGenerator& url_generator, @@ -97,7 +165,7 @@ class FilesGetRequest : public DriveApiDataRequest { // This class performs request for authorizing an app to access a file. // This request is mapped to /drive/v2internal/file/authorize internal endpoint. -class FilesAuthorizeRequest : public DriveApiDataRequest { +class FilesAuthorizeRequest : public DriveApiDataRequest<FileResource> { public: FilesAuthorizeRequest(RequestSender* sender, const DriveApiUrlGenerator& url_generator, @@ -132,7 +200,7 @@ class FilesAuthorizeRequest : public DriveApiDataRequest { // https://developers.google.com/drive/v2/reference/files/insert // See also https://developers.google.com/drive/manage-uploads and // https://developers.google.com/drive/folder -class FilesInsertRequest : public DriveApiDataRequest { +class FilesInsertRequest : public DriveApiDataRequest<FileResource> { public: FilesInsertRequest(RequestSender* sender, const DriveApiUrlGenerator& url_generator, @@ -189,7 +257,7 @@ class FilesInsertRequest : public DriveApiDataRequest { // This class performs the request for patching file metadata. // This request is mapped to // https://developers.google.com/drive/v2/reference/files/patch -class FilesPatchRequest : public DriveApiDataRequest { +class FilesPatchRequest : public DriveApiDataRequest<FileResource> { public: FilesPatchRequest(RequestSender* sender, const DriveApiUrlGenerator& url_generator, @@ -263,7 +331,7 @@ class FilesPatchRequest : public DriveApiDataRequest { // This class performs the request for copying a resource. // This request is mapped to // https://developers.google.com/drive/v2/reference/files/copy -class FilesCopyRequest : public DriveApiDataRequest { +class FilesCopyRequest : public DriveApiDataRequest<FileResource> { public: // Upon completion, |callback| will be called. |callback| must not be null. FilesCopyRequest(RequestSender* sender, @@ -315,7 +383,7 @@ class FilesCopyRequest : public DriveApiDataRequest { // or by FilesListRequest with setting page token. // This request is mapped to // https://developers.google.com/drive/v2/reference/files/list -class FilesListRequest : public DriveApiDataRequest { +class FilesListRequest : public DriveApiDataRequest<FileList> { public: FilesListRequest(RequestSender* sender, const DriveApiUrlGenerator& url_generator, @@ -353,7 +421,7 @@ class FilesListRequest : public DriveApiDataRequest { // 1) Set pageToken and all params used for the initial request. // 2) Use URL in the nextLink field in the previous response. // This class implements 2)'s request. -class FilesListNextPageRequest : public DriveApiDataRequest { +class FilesListNextPageRequest : public DriveApiDataRequest<FileList> { public: FilesListNextPageRequest(RequestSender* sender, const FileListCallback& callback); @@ -408,7 +476,7 @@ class FilesDeleteRequest : public EntryActionRequest { // This class performs the request for trashing a resource. // This request is mapped to // https://developers.google.com/drive/v2/reference/files/trash -class FilesTrashRequest : public DriveApiDataRequest { +class FilesTrashRequest : public DriveApiDataRequest<FileResource> { public: FilesTrashRequest(RequestSender* sender, const DriveApiUrlGenerator& url_generator, @@ -438,7 +506,7 @@ class FilesTrashRequest : public DriveApiDataRequest { // This class performs the request for fetching About data. // This request is mapped to // https://developers.google.com/drive/v2/reference/about/get -class AboutGetRequest : public DriveApiDataRequest { +class AboutGetRequest : public DriveApiDataRequest<AboutResource> { public: AboutGetRequest(RequestSender* sender, const DriveApiUrlGenerator& url_generator, @@ -463,7 +531,7 @@ class AboutGetRequest : public DriveApiDataRequest { // or by ChangesListRequest with setting page token. // This request is mapped to // https://developers.google.com/drive/v2/reference/changes/list -class ChangesListRequest : public DriveApiDataRequest { +class ChangesListRequest : public DriveApiDataRequest<ChangeList> { public: ChangesListRequest(RequestSender* sender, const DriveApiUrlGenerator& url_generator, @@ -509,7 +577,7 @@ class ChangesListRequest : public DriveApiDataRequest { // 1) Set pageToken and all params used for the initial request. // 2) Use URL in the nextLink field in the previous response. // This class implements 2)'s request. -class ChangesListNextPageRequest : public DriveApiDataRequest { +class ChangesListNextPageRequest : public DriveApiDataRequest<ChangeList> { public: ChangesListNextPageRequest(RequestSender* sender, const ChangeListCallback& callback); @@ -533,7 +601,7 @@ class ChangesListNextPageRequest : public DriveApiDataRequest { // This class performs the request for fetching AppList. // This request is mapped to // https://developers.google.com/drive/v2/reference/apps/list -class AppsListRequest : public DriveApiDataRequest { +class AppsListRequest : public DriveApiDataRequest<AppList> { public: AppsListRequest(RequestSender* sender, const DriveApiUrlGenerator& url_generator, diff --git a/google_apis/drive/gdata_wapi_requests.cc b/google_apis/drive/gdata_wapi_requests.cc index 4259217..58ed9c0 100644 --- a/google_apis/drive/gdata_wapi_requests.cc +++ b/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,35 @@ 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) { + callback_.Run(entry ? error : GDATA_PARSE_ERROR, entry.Pass()); + OnProcessURLFetchResultsComplete(); +} + } // namespace google_apis diff --git a/google_apis/drive/gdata_wapi_requests.h b/google_apis/drive/gdata_wapi_requests.h index 7e64906..c4e1d06b 100644 --- a/google_apis/drive/gdata_wapi_requests.h +++ b/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); }; diff --git a/google_apis/drive/gdata_wapi_requests_unittest.cc b/google_apis/drive/gdata_wapi_requests_unittest.cc index 2c39745..c992909 100644 --- a/google_apis/drive/gdata_wapi_requests_unittest.cc +++ b/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; |