From 49f127860183bbc4fa9a95ceb7b2e3d08c3ec996 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Mon, 30 May 2016 20:31:00 +0900 Subject: [PATCH] Refactor the URLRequestFetchJob code This makes the read end and write end of the pipe have same logic, so it is more easy to maintain. --- atom/browser/net/url_request_fetch_job.cc | 93 +++++++++++------------ atom/browser/net/url_request_fetch_job.h | 25 +++--- spec/api-protocol-spec.js | 2 +- 3 files changed, 61 insertions(+), 59 deletions(-) diff --git a/atom/browser/net/url_request_fetch_job.cc b/atom/browser/net/url_request_fetch_job.cc index 388b9f9550b3..0f293c22fcf6 100644 --- a/atom/browser/net/url_request_fetch_job.cc +++ b/atom/browser/net/url_request_fetch_job.cc @@ -77,7 +77,8 @@ class ResponsePiper : public net::URLFetcherResponseWriter { URLRequestFetchJob::URLRequestFetchJob( net::URLRequest* request, net::NetworkDelegate* network_delegate) : JsAsker(request, network_delegate), - pending_buffer_size_(0) { + pending_buffer_size_(0), + write_num_bytes_(0) { } void URLRequestFetchJob::BeforeStartInUI( @@ -169,22 +170,21 @@ void URLRequestFetchJob::HeadersCompleted() { int URLRequestFetchJob::DataAvailable(net::IOBuffer* buffer, int num_bytes, const net::CompletionCallback& callback) { - // Store the data in a drainable IO buffer if pending_buffer_ is empty, i.e. - // there's no ReadRawData() operation waiting for IO completion. + // When pending_buffer_ is empty, there's no ReadRawData() operation waiting + // for IO completion, we have to save the parameters until the request is + // ready to read data. if (!pending_buffer_.get()) { - response_piper_callback_ = callback; - drainable_buffer_ = new net::DrainableIOBuffer(buffer, num_bytes); + write_buffer_ = buffer; + write_num_bytes_ = num_bytes; + write_callback_ = callback; return net::ERR_IO_PENDING; } - // pending_buffer_ is set to the IOBuffer instance provided to ReadRawData() - // by URLRequestJob. - int bytes_read = std::min(num_bytes, pending_buffer_size_); - memcpy(pending_buffer_->data(), buffer->data(), bytes_read); - - ClearPendingBuffer(); - + // Write data to the pending buffer and clear them after the writing. + int bytes_read = BufferCopy(buffer, num_bytes, + pending_buffer_.get(), pending_buffer_size_); ReadRawDataComplete(bytes_read); + ClearPendingBuffer(); return bytes_read; } @@ -193,46 +193,26 @@ void URLRequestFetchJob::Kill() { fetcher_.reset(); } -void URLRequestFetchJob::StartReading() { - if (drainable_buffer_.get()) { - auto num_bytes = drainable_buffer_->BytesRemaining(); - if (num_bytes <= 0 && !response_piper_callback_.is_null()) { - int size = drainable_buffer_->BytesConsumed(); - drainable_buffer_ = nullptr; - response_piper_callback_.Run(size); - response_piper_callback_.Reset(); - return; - } - - int bytes_read = std::min(num_bytes, pending_buffer_size_); - memcpy(pending_buffer_->data(), drainable_buffer_->data(), bytes_read); - - drainable_buffer_->DidConsume(bytes_read); - - ClearPendingBuffer(); - - ReadRawDataComplete(bytes_read); - } -} - -void URLRequestFetchJob::ClearPendingBuffer() { - // Clear the buffers before notifying the read is complete, so that it is - // safe for the observer to read. - pending_buffer_ = nullptr; - pending_buffer_size_ = 0; -} - int URLRequestFetchJob::ReadRawData(net::IOBuffer* dest, int dest_size) { if (GetResponseCode() == 204) { request()->set_received_response_content_length(prefilter_bytes_read()); return net::OK; } - pending_buffer_ = dest; - pending_buffer_size_ = dest_size; - // Read from drainable_buffer_ if available, i.e. response data became - // available before ReadRawData was called. - StartReading(); - return net::ERR_IO_PENDING; + + // When write_buffer_ is empty, there is no data valable yet, we have to save + // the dest buffer util DataAvailable. + if (!write_buffer_.get()) { + pending_buffer_ = dest; + pending_buffer_size_ = dest_size; + return net::ERR_IO_PENDING; + } + + // Read from the write buffer and clear them after reading. + int bytes_read = BufferCopy(write_buffer_.get(), write_num_bytes_, + dest, dest_size); + write_callback_.Run(bytes_read); + ClearWriteBuffer(); + return bytes_read; } bool URLRequestFetchJob::GetMimeType(std::string* mime_type) const { @@ -264,6 +244,7 @@ void URLRequestFetchJob::OnURLFetchComplete(const net::URLFetcher* source) { } ClearPendingBuffer(); + ClearWriteBuffer(); if (fetcher_->GetStatus().is_success()) ReadRawDataComplete(0); @@ -271,4 +252,22 @@ void URLRequestFetchJob::OnURLFetchComplete(const net::URLFetcher* source) { NotifyStartError(fetcher_->GetStatus()); } +int URLRequestFetchJob::BufferCopy(net::IOBuffer* source, int num_bytes, + net::IOBuffer* target, int target_size) { + int bytes_written = std::min(num_bytes, target_size); + memcpy(target->data(), source->data(), bytes_written); + return bytes_written; +} + +void URLRequestFetchJob::ClearPendingBuffer() { + pending_buffer_ = nullptr; + pending_buffer_size_ = 0; +} + +void URLRequestFetchJob::ClearWriteBuffer() { + write_buffer_ = nullptr; + write_num_bytes_ = 0; + write_callback_.Reset(); +} + } // namespace atom diff --git a/atom/browser/net/url_request_fetch_job.h b/atom/browser/net/url_request_fetch_job.h index da1c1678e58a..906ba68d3965 100644 --- a/atom/browser/net/url_request_fetch_job.h +++ b/atom/browser/net/url_request_fetch_job.h @@ -9,14 +9,10 @@ #include "atom/browser/net/js_asker.h" #include "browser/url_request_context_getter.h" -#include "net/url_request/url_request_context_getter.h" #include "net/url_request/url_fetcher_delegate.h" -#include "net/url_request/url_request_job.h" namespace atom { -class AtomBrowserContext; - class URLRequestFetchJob : public JsAsker, public net::URLFetcherDelegate, public brightray::URLRequestContextGetter::Delegate { @@ -30,9 +26,6 @@ class URLRequestFetchJob : public JsAsker, const net::CompletionCallback& callback); protected: - void StartReading(); - void ClearPendingBuffer(); - // JsAsker: void BeforeStartInUI(v8::Isolate*, v8::Local) override; void StartAsync(std::unique_ptr options) override; @@ -48,13 +41,23 @@ class URLRequestFetchJob : public JsAsker, void OnURLFetchComplete(const net::URLFetcher* source) override; private: + int BufferCopy(net::IOBuffer* source, int num_bytes, + net::IOBuffer* target, int target_size); + void ClearPendingBuffer(); + void ClearWriteBuffer(); + scoped_refptr url_request_context_getter_; std::unique_ptr fetcher_; - scoped_refptr pending_buffer_; - scoped_refptr drainable_buffer_; - int pending_buffer_size_; std::unique_ptr response_info_; - net::CompletionCallback response_piper_callback_; + + // Saved arguments passed to ReadRawData. + scoped_refptr pending_buffer_; + int pending_buffer_size_; + + // Saved arguments passed to DataAvailable. + scoped_refptr write_buffer_; + int write_num_bytes_; + net::CompletionCallback write_callback_; DISALLOW_COPY_AND_ASSIGN(URLRequestFetchJob); }; diff --git a/spec/api-protocol-spec.js b/spec/api-protocol-spec.js index 1b46d586a41a..803511917104 100644 --- a/spec/api-protocol-spec.js +++ b/spec/api-protocol-spec.js @@ -511,7 +511,7 @@ describe('protocol module', function () { }) }) - it('loads resource for webContents', function (done) { + it('works when target URL redirects', function (done) { var contents = null var server = http.createServer(function (req, res) { if (req.url == '/serverRedirect') {