From 8f4440075845550559c09a787bf71de915d4cfc5 Mon Sep 17 00:00:00 2001 From: xunjieli Date: Mon, 9 Nov 2015 14:36:45 -0800 Subject: Revert "URLRequestJob: change ReadRawData contract" This reverts commit a505cbb45ec92d2617fe80490acf7d01ef4d3225. Revert "Disable http/tests/serviceworker/registration.html" This reverts commit e2bf349b84f10e940b5ebe5a8fcaf2cf8c8eb322. Revert "Remove incorrect DCHECK_NE in ServiceWorkerWriteToCacheJob::OnReadCompleted." This reverts commit 5b28d0b0efd8a75a9ad783bd849fa41eb9339a3b. Revert "Do not call NotifyDone in URLRequestJob::ReadRawDataComplete if ERR_IO_PENDING" This reverts commit 876da269322a66d5e2741de0892e01b24ab98065. TBR=michaeln@chromium.org,mnaganov@chromium.org,skyostil@chromium.org,eugenebut@chromium.org,davidben@chromium.org,falken@chromium.org,mtomasz@chromium.org, sky@chromium.org,jianli@chromium.org,zork@chromium.org BUG=553300 BUG=474859 Review URL: https://codereview.chromium.org/1437523002 Cr-Commit-Position: refs/heads/master@{#358686} --- storage/browser/blob/blob_url_request_job.cc | 42 ++++++++----- storage/browser/blob/blob_url_request_job.h | 2 +- .../fileapi/file_system_dir_url_request_job.cc | 19 +++--- .../fileapi/file_system_dir_url_request_job.h | 2 +- .../browser/fileapi/file_system_url_request_job.cc | 70 ++++++++++++---------- .../browser/fileapi/file_system_url_request_job.h | 5 +- 6 files changed, 83 insertions(+), 57 deletions(-) (limited to 'storage') diff --git a/storage/browser/blob/blob_url_request_job.cc b/storage/browser/blob/blob_url_request_job.cc index deea300..6ec2fe5 100644 --- a/storage/browser/blob/blob_url_request_job.cc +++ b/storage/browser/blob/blob_url_request_job.cc @@ -75,37 +75,41 @@ void BlobURLRequestJob::Kill() { weak_factory_.InvalidateWeakPtrs(); } -int BlobURLRequestJob::ReadRawData(net::IOBuffer* dest, int dest_size) { +bool BlobURLRequestJob::ReadRawData(net::IOBuffer* dest, + int dest_size, + int* bytes_read) { TRACE_EVENT_ASYNC_BEGIN1("Blob", "BlobRequest::ReadRawData", this, "uuid", blob_handle_ ? blob_handle_->uuid() : "NotFound"); DCHECK_NE(dest_size, 0); + DCHECK(bytes_read); - // Bail out immediately if we encounter an error. This happens if a previous - // ReadRawData signalled an error to its caller but the caller called - // ReadRawData again anyway. - if (error_) - return 0; + // Bail out immediately if we encounter an error. + if (error_) { + *bytes_read = 0; + return true; + } - int bytes_read = 0; BlobReader::Status read_status = - blob_reader_->Read(dest, dest_size, &bytes_read, + blob_reader_->Read(dest, dest_size, bytes_read, base::Bind(&BlobURLRequestJob::DidReadRawData, weak_factory_.GetWeakPtr())); switch (read_status) { case BlobReader::Status::NET_ERROR: + NotifyFailure(blob_reader_->net_error()); TRACE_EVENT_ASYNC_END1("Blob", "BlobRequest::ReadRawData", this, "uuid", blob_handle_ ? blob_handle_->uuid() : "NotFound"); - return blob_reader_->net_error(); + return false; case BlobReader::Status::IO_PENDING: - return net::ERR_IO_PENDING; + SetStatus(net::URLRequestStatus(net::URLRequestStatus::IO_PENDING, 0)); + return false; case BlobReader::Status::DONE: TRACE_EVENT_ASYNC_END1("Blob", "BlobRequest::ReadRawData", this, "uuid", blob_handle_ ? blob_handle_->uuid() : "NotFound"); - return bytes_read; + return true; } NOTREACHED(); - return 0; + return true; } bool BlobURLRequestJob::GetMimeType(std::string* mime_type) const { @@ -218,7 +222,13 @@ void BlobURLRequestJob::DidCalculateSize(int result) { void BlobURLRequestJob::DidReadRawData(int result) { TRACE_EVENT_ASYNC_END1("Blob", "BlobRequest::ReadRawData", this, "uuid", blob_handle_ ? blob_handle_->uuid() : "NotFound"); - ReadRawDataComplete(result); + if (result < 0) { + NotifyFailure(result); + return; + } + // Clear the IO_PENDING status + SetStatus(net::URLRequestStatus()); + NotifyReadComplete(result); } void BlobURLRequestJob::NotifyFailure(int error_code) { @@ -226,7 +236,11 @@ void BlobURLRequestJob::NotifyFailure(int error_code) { // If we already return the headers on success, we can't change the headers // now. Instead, we just error out. - DCHECK(!response_info_) << "Cannot NotifyFailure after headers."; + if (response_info_) { + NotifyDone( + net::URLRequestStatus(net::URLRequestStatus::FAILED, error_code)); + return; + } net::HttpStatusCode status_code = net::HTTP_INTERNAL_SERVER_ERROR; switch (error_code) { diff --git a/storage/browser/blob/blob_url_request_job.h b/storage/browser/blob/blob_url_request_job.h index 1f0b9fb..21baa2c 100644 --- a/storage/browser/blob/blob_url_request_job.h +++ b/storage/browser/blob/blob_url_request_job.h @@ -44,7 +44,7 @@ class STORAGE_EXPORT BlobURLRequestJob // net::URLRequestJob methods. void Start() override; void Kill() override; - int ReadRawData(net::IOBuffer* buf, int buf_size) override; + bool ReadRawData(net::IOBuffer* buf, int buf_size, int* bytes_read) override; bool GetMimeType(std::string* mime_type) const override; void GetResponseInfo(net::HttpResponseInfo* info) override; int GetResponseCode() const override; diff --git a/storage/browser/fileapi/file_system_dir_url_request_job.cc b/storage/browser/fileapi/file_system_dir_url_request_job.cc index 2a0984b7..4236fec 100644 --- a/storage/browser/fileapi/file_system_dir_url_request_job.cc +++ b/storage/browser/fileapi/file_system_dir_url_request_job.cc @@ -14,6 +14,7 @@ #include "base/time/time.h" #include "build/build_config.h" #include "net/base/io_buffer.h" +#include "net/base/net_errors.h" #include "net/base/net_util.h" #include "net/url_request/url_request.h" #include "storage/browser/fileapi/file_system_context.h" @@ -43,14 +44,16 @@ FileSystemDirURLRequestJob::FileSystemDirURLRequestJob( FileSystemDirURLRequestJob::~FileSystemDirURLRequestJob() { } -int FileSystemDirURLRequestJob::ReadRawData(net::IOBuffer* dest, - int dest_size) { - int count = std::min(dest_size, base::checked_cast(data_.size())); +bool FileSystemDirURLRequestJob::ReadRawData(net::IOBuffer* dest, + int dest_size, + int* bytes_read) { + int count = std::min(dest_size, static_cast(data_.size())); if (count > 0) { memcpy(dest->data(), data_.data(), count); data_.erase(0, count); } - return count; + *bytes_read = count; + return true; } void FileSystemDirURLRequestJob::Start() { @@ -96,7 +99,8 @@ void FileSystemDirURLRequestJob::StartAsync() { false); return; } - NotifyStartError(URLRequestStatus::FromError(net::ERR_FILE_NOT_FOUND)); + NotifyDone( + URLRequestStatus(URLRequestStatus::FAILED, net::ERR_FILE_NOT_FOUND)); return; } file_system_context_->operation_runner()->ReadDirectory( @@ -109,7 +113,8 @@ void FileSystemDirURLRequestJob::DidAttemptAutoMount(base::File::Error result) { file_system_context_->CrackURL(request_->url()).is_valid()) { StartAsync(); } else { - NotifyStartError(URLRequestStatus::FromError(net::ERR_FILE_NOT_FOUND)); + NotifyDone( + URLRequestStatus(URLRequestStatus::FAILED, net::ERR_FILE_NOT_FOUND)); } } @@ -121,7 +126,7 @@ void FileSystemDirURLRequestJob::DidReadDirectory( int rv = net::ERR_FILE_NOT_FOUND; if (result == base::File::FILE_ERROR_INVALID_URL) rv = net::ERR_INVALID_URL; - NotifyStartError(URLRequestStatus::FromError(rv)); + NotifyDone(URLRequestStatus(URLRequestStatus::FAILED, rv)); return; } diff --git a/storage/browser/fileapi/file_system_dir_url_request_job.h b/storage/browser/fileapi/file_system_dir_url_request_job.h index 67104d6..0163535 100644 --- a/storage/browser/fileapi/file_system_dir_url_request_job.h +++ b/storage/browser/fileapi/file_system_dir_url_request_job.h @@ -32,7 +32,7 @@ class STORAGE_EXPORT FileSystemDirURLRequestJob : public net::URLRequestJob { // URLRequestJob methods: void Start() override; void Kill() override; - int ReadRawData(net::IOBuffer* buf, int buf_size) override; + bool ReadRawData(net::IOBuffer* buf, int buf_size, int* bytes_read) override; bool GetCharset(std::string* charset) override; // FilterContext methods (via URLRequestJob): diff --git a/storage/browser/fileapi/file_system_url_request_job.cc b/storage/browser/fileapi/file_system_url_request_job.cc index 6c33fc8..4607dd8 100644 --- a/storage/browser/fileapi/file_system_url_request_job.cc +++ b/storage/browser/fileapi/file_system_url_request_job.cc @@ -62,7 +62,6 @@ FileSystemURLRequestJob::FileSystemURLRequestJob( file_system_context_(file_system_context), is_directory_(false), remaining_bytes_(0), - range_parse_result_(net::OK), weak_factory_(this) {} FileSystemURLRequestJob::~FileSystemURLRequestJob() {} @@ -80,28 +79,39 @@ void FileSystemURLRequestJob::Kill() { weak_factory_.InvalidateWeakPtrs(); } -int FileSystemURLRequestJob::ReadRawData(net::IOBuffer* dest, int dest_size) { +bool FileSystemURLRequestJob::ReadRawData(net::IOBuffer* dest, + int dest_size, + int* bytes_read) { DCHECK_NE(dest_size, 0); + DCHECK(bytes_read); DCHECK_GE(remaining_bytes_, 0); if (reader_.get() == NULL) - return net::ERR_FAILED; + return false; if (remaining_bytes_ < dest_size) - dest_size = remaining_bytes_; + dest_size = static_cast(remaining_bytes_); - if (!dest_size) - return 0; + if (!dest_size) { + *bytes_read = 0; + return true; + } const int rv = reader_->Read(dest, dest_size, base::Bind(&FileSystemURLRequestJob::DidRead, weak_factory_.GetWeakPtr())); if (rv >= 0) { + // Data is immediately available. + *bytes_read = rv; remaining_bytes_ -= rv; DCHECK_GE(remaining_bytes_, 0); + return true; } - - return rv; + if (rv == net::ERR_IO_PENDING) + SetStatus(URLRequestStatus(URLRequestStatus::IO_PENDING, 0)); + else + NotifyFailed(rv); + return false; } bool FileSystemURLRequestJob::GetMimeType(std::string* mime_type) const { @@ -116,12 +126,8 @@ bool FileSystemURLRequestJob::GetMimeType(std::string* mime_type) const { void FileSystemURLRequestJob::SetExtraRequestHeaders( const net::HttpRequestHeaders& headers) { std::string range_header; - // Currently this job only cares about the Range header. Note that validation - // is deferred to DidGetMetaData(), because NotifyStartError is not legal to - // call since the job has not started. if (headers.GetHeader(net::HttpRequestHeaders::kRange, &range_header)) { std::vector ranges; - if (net::HttpUtil::ParseRangeHeader(range_header, &ranges)) { if (ranges.size() == 1) { byte_range_ = ranges[0]; @@ -129,7 +135,7 @@ void FileSystemURLRequestJob::SetExtraRequestHeaders( // We don't support multiple range requests in one single URL request. // TODO(adamk): decide whether we want to support multiple range // requests. - range_parse_result_ = net::ERR_REQUEST_RANGE_NOT_SATISFIABLE; + NotifyFailed(net::ERR_REQUEST_RANGE_NOT_SATISFIABLE); } } } @@ -161,7 +167,7 @@ void FileSystemURLRequestJob::StartAsync() { } if (!file_system_context_->CanServeURLRequest(url_)) { // In incognito mode the API is not usable and there should be no data. - NotifyStartError(URLRequestStatus::FromError(net::ERR_FILE_NOT_FOUND)); + NotifyFailed(net::ERR_FILE_NOT_FOUND); return; } file_system_context_->operation_runner()->GetMetadata( @@ -175,7 +181,7 @@ void FileSystemURLRequestJob::DidAttemptAutoMount(base::File::Error result) { file_system_context_->CrackURL(request_->url()).is_valid()) { StartAsync(); } else { - NotifyStartError(URLRequestStatus::FromError(net::ERR_FILE_NOT_FOUND)); + NotifyFailed(net::ERR_FILE_NOT_FOUND); } } @@ -183,10 +189,9 @@ void FileSystemURLRequestJob::DidGetMetadata( base::File::Error error_code, const base::File::Info& file_info) { if (error_code != base::File::FILE_OK) { - NotifyStartError(URLRequestStatus::FromError( - error_code == base::File::FILE_ERROR_INVALID_URL - ? net::ERR_INVALID_URL - : net::ERR_FILE_NOT_FOUND)); + NotifyFailed(error_code == base::File::FILE_ERROR_INVALID_URL + ? net::ERR_INVALID_URL + : net::ERR_FILE_NOT_FOUND); return; } @@ -196,14 +201,8 @@ void FileSystemURLRequestJob::DidGetMetadata( is_directory_ = file_info.is_directory; - if (range_parse_result_ != net::OK) { - NotifyStartError(URLRequestStatus::FromError(range_parse_result_)); - return; - } - if (!byte_range_.ComputeBounds(file_info.size)) { - NotifyStartError( - URLRequestStatus::FromError(net::ERR_REQUEST_RANGE_NOT_SATISFIABLE)); + NotifyFailed(net::ERR_REQUEST_RANGE_NOT_SATISFIABLE); return; } @@ -227,12 +226,17 @@ void FileSystemURLRequestJob::DidGetMetadata( } void FileSystemURLRequestJob::DidRead(int result) { - if (result >= 0) { - remaining_bytes_ -= result; - DCHECK_GE(remaining_bytes_, 0); - } + if (result > 0) + SetStatus(URLRequestStatus()); // Clear the IO_PENDING status + else if (result == 0) + NotifyDone(URLRequestStatus()); + else + NotifyFailed(result); + + remaining_bytes_ -= result; + DCHECK_GE(remaining_bytes_, 0); - ReadRawDataComplete(result); + NotifyReadComplete(result); } bool FileSystemURLRequestJob::IsRedirectResponse(GURL* location, @@ -252,4 +256,8 @@ bool FileSystemURLRequestJob::IsRedirectResponse(GURL* location, return false; } +void FileSystemURLRequestJob::NotifyFailed(int rv) { + NotifyDone(URLRequestStatus(URLRequestStatus::FAILED, rv)); +} + } // namespace storage diff --git a/storage/browser/fileapi/file_system_url_request_job.h b/storage/browser/fileapi/file_system_url_request_job.h index 308b66c..e442cae 100644 --- a/storage/browser/fileapi/file_system_url_request_job.h +++ b/storage/browser/fileapi/file_system_url_request_job.h @@ -11,7 +11,6 @@ #include "base/memory/ref_counted.h" #include "base/memory/scoped_ptr.h" #include "base/memory/weak_ptr.h" -#include "net/base/net_errors.h" #include "net/http/http_byte_range.h" #include "net/url_request/url_request_job.h" #include "storage/browser/fileapi/file_system_url.h" @@ -42,7 +41,7 @@ class STORAGE_EXPORT FileSystemURLRequestJob : public net::URLRequestJob { // URLRequestJob methods: void Start() override; void Kill() override; - int ReadRawData(net::IOBuffer* buf, int buf_size) override; + bool ReadRawData(net::IOBuffer* buf, int buf_size, int* bytes_read) override; bool IsRedirectResponse(GURL* location, int* http_status_code) override; void SetExtraRequestHeaders(const net::HttpRequestHeaders& headers) override; void GetResponseInfo(net::HttpResponseInfo* info) override; @@ -61,6 +60,7 @@ class STORAGE_EXPORT FileSystemURLRequestJob : public net::URLRequestJob { void DidGetMetadata(base::File::Error error_code, const base::File::Info& file_info); void DidRead(int result); + void NotifyFailed(int rv); const std::string storage_domain_; FileSystemContext* file_system_context_; @@ -69,7 +69,6 @@ class STORAGE_EXPORT FileSystemURLRequestJob : public net::URLRequestJob { bool is_directory_; scoped_ptr response_info_; int64 remaining_bytes_; - net::Error range_parse_result_; net::HttpByteRange byte_range_; base::WeakPtrFactory weak_factory_; -- cgit v1.1