From bde30b90e871cba8414e416528e30caa0edadfbd Mon Sep 17 00:00:00 2001 From: "ali.ibrahim" Date: Fri, 30 Sep 2016 14:17:29 +0200 Subject: [PATCH] Adding some implementation comments. Enforcing Chromium coding conventions. --- atom/browser/api/atom_api_url_request.cc | 5 +- atom/browser/api/atom_api_url_request.h | 97 ++++++++++++++++++++---- atom/browser/net/atom_url_request.cc | 12 ++- atom/browser/net/atom_url_request.h | 27 +++---- 4 files changed, 101 insertions(+), 40 deletions(-) diff --git a/atom/browser/api/atom_api_url_request.cc b/atom/browser/api/atom_api_url_request.cc index ad2d66f5fe4..016edbe0c7f 100644 --- a/atom/browser/api/atom_api_url_request.cc +++ b/atom/browser/api/atom_api_url_request.cc @@ -13,6 +13,7 @@ #include "native_mate/dictionary.h" + namespace mate { template<> @@ -83,8 +84,8 @@ namespace atom { namespace api { URLRequest::URLRequest(v8::Isolate* isolate, v8::Local wrapper) - : weak_ptr_factory_(this) { - InitWith(isolate, wrapper); + : weak_ptr_factory_(this) { + InitWith(isolate, wrapper); } URLRequest::~URLRequest() { diff --git a/atom/browser/api/atom_api_url_request.h b/atom/browser/api/atom_api_url_request.h index 89702b0d401..20eb66d3f0f 100644 --- a/atom/browser/api/atom_api_url_request.h +++ b/atom/browser/api/atom_api_url_request.h @@ -7,18 +7,88 @@ #include #include +#include "atom/browser/api/event_emitter.h" #include "atom/browser/api/trackable_object.h" +#include "base/memory/weak_ptr.h" #include "native_mate/handle.h" +#include "native_mate/wrappable_base.h" +#include "net/base/auth.h" +#include "net/base/io_buffer.h" #include "net/http/http_response_headers.h" #include "net/url_request/url_request_context.h" - namespace atom { class AtomURLRequest; namespace api { +// +// The URLRequest class implements the V8 binding between the JavaScript API +// and Chromium native net library. It is responsible for handling HTTP/HTTPS +// requests. +// +// The current class provides only the binding layer. Two other JavaScript +// classes (ClientRequest and IncomingMessage) in the net module provide the +// final API, including some state management and arguments validation. +// +// URLRequest's methods fall into two main categories: command and event methods. +// They are always executed on the Browser's UI thread. +// Command methods are called directly from JavaScript code via the API defined +// in BuildPrototype. A command method is generally implemented by forwarding +// the call to a corresponding method on AtomURLRequest which does the +// synchronization on the Browser IO thread. The latter then calls into Chromium +// net library. On the other hand, net library events originate on the IO +// thread in AtomURLRequest and are synchronized back on the UI thread, then +// forwarded to a corresponding event method in URLRequest and then to +// JavaScript via the EmitRequestEvent/EmitResponseEvent helpers. +// +// URLRequest lifetime management: we followed the Wrapper/Wrappable pattern +// defined in native_mate. However, we augment that pattern with a pin/unpin +// mechanism. The main reason is that we want the JS API to provide a similar +// lifetime guarantees as the XMLHttpRequest. +// https://xhr.spec.whatwg.org/#garbage-collection +// +// The primary motivation is to not garbage collect a URLInstance as long as the +// object is emitting network events. For instance, in the following JS code +// +// (function() { +// let request = new URLRequest(...); +// request.on('response', (response)=>{ +// response.on('data', (data) = > { +// console.log(data.toString()); +// }); +// }); +// })(); +// +// we still want data to be logged even if the response/request objects are no +// more referenced in JavaScript. +// +// Binding by simply following the native_mate Wrapper/Wrappable pattern will +// delete the URLRequest object when the corresponding JS object is collected. +// The v8 handle is a private member in WrappableBase and it is always weak, +// there is no way to make it strong without changing native_mate. +// The solution we implement consists of maintaining some kind of state that +// prevents collection of JS wrappers as long as the request is emitting network +// events. At initialization, the object is unpinned. When the request starts, +// it is pinned. When no more events would be emitted, the object is unpinned +// and lifetime is again managed by the standard native mate Wrapper/Wrappable +// pattern. +// +// pin/unpin: are implemented by constructing/reseting a V8 strong persistent +// handle. +// +// The URLRequest/AtmURLRequest interaction could have been implemented in a +// single class. However, it implies that the resulting class lifetime will be +// managed by two conflicting mechanisms: JavaScript garbage collection and +// Chromium reference counting. Reasoning about lifetime issues become much +// more complex. +// +// We chose to split the implementation into two classes linked via a strong/weak +// pointers. A URLRequest instance is deleted if it is unpinned and the +// corresponding JS wrapper object is garbage collected. On the other hand, +// an AtmURLRequest instance lifetime is totally governed by reference counting. +// class URLRequest : public mate::EventEmitter { public: static mate::WrappableBase* New(mate::Arguments* args); @@ -27,12 +97,19 @@ class URLRequest : public mate::EventEmitter { v8::Isolate* isolate, v8::Local prototype); + // Methods for reporting events into JavaScript. + void OnAuthenticationRequired( + scoped_refptr auth_info); + void OnResponseStarted(); + void OnResponseData(scoped_refptr data); + void OnResponseCompleted(); + void OnError(const std::string& error); + protected: explicit URLRequest(v8::Isolate* isolate, v8::Local wrapper); ~URLRequest() override; - private: bool Write(scoped_refptr buffer, bool is_last); @@ -41,21 +118,12 @@ class URLRequest : public mate::EventEmitter { void RemoveExtraHeader(const std::string& name); void SetChunkedUpload(bool is_chunked_upload); - friend class AtomURLRequest; - void OnAuthenticationRequired( - scoped_refptr auth_info); - void OnResponseStarted(); - void OnResponseData(scoped_refptr data); - void OnResponseCompleted(); - void OnError(const std::string& error); - int StatusCode() const; std::string StatusMessage() const; scoped_refptr RawResponseHeaders() const; uint32_t ResponseHttpVersionMajor() const; uint32_t ResponseHttpVersionMinor() const; - template std::array, sizeof...(ArgTypes)> BuildArgsArray(ArgTypes... args) const; @@ -70,7 +138,10 @@ class URLRequest : public mate::EventEmitter { void unpin(); scoped_refptr atom_request_; + + // Used to implement pin/unpin. v8::Global wrapper_; + base::WeakPtrFactory weak_ptr_factory_; DISALLOW_COPY_AND_ASSIGN(URLRequest); @@ -94,7 +165,6 @@ void URLRequest::EmitRequestEvent(ArgTypes... args) { _emitRequestEvent->Call(wrapper, arguments.size(), arguments.data()); } - template void URLRequest::EmitResponseEvent(ArgTypes... args) { auto arguments = BuildArgsArray(args...); @@ -105,9 +175,6 @@ void URLRequest::EmitResponseEvent(ArgTypes... args) { _emitResponseEvent->Call(wrapper, arguments.size(), arguments.data()); } - - - } // namespace api } // namespace atom diff --git a/atom/browser/net/atom_url_request.cc b/atom/browser/net/atom_url_request.cc index 6b07a2e3ef6..6a20dc47686 100644 --- a/atom/browser/net/atom_url_request.cc +++ b/atom/browser/net/atom_url_request.cc @@ -50,9 +50,9 @@ class UploadOwnedIOBufferElementReader : public net::UploadBytesElementReader { } // namespace internal AtomURLRequest::AtomURLRequest(base::WeakPtr delegate) - : delegate_(delegate) - , response_read_buffer_(new net::IOBuffer(kBufferSize)) - , is_chunked_upload_(false) { + : delegate_(delegate), + response_read_buffer_(new net::IOBuffer(kBufferSize)), + is_chunked_upload_(false) { } AtomURLRequest::~AtomURLRequest() { @@ -263,8 +263,7 @@ void AtomURLRequest::ReadResponse() { // completed immediately, without trying to read any data back (all we care // about is the response code and headers, which we already have). int bytes_read = 0; - if (request_->status().is_success() - /* TODO && (request_type_ != URLFetcher::HEAD)*/) + if (request_->status().is_success()) if (!request_->Read(response_read_buffer_.get(), kBufferSize, &bytes_read)) bytes_read = -1; OnReadCompleted(request_.get(), bytes_read); @@ -297,8 +296,7 @@ void AtomURLRequest::OnReadCompleted(net::URLRequest* request, kBufferSize, &bytes_read)); - if (!status.is_io_pending() - /* TODO || request_type_ == URLFetcher::HEAD*/ ) + if (!status.is_io_pending()) content::BrowserThread::PostTask( content::BrowserThread::UI, FROM_HERE, diff --git a/atom/browser/net/atom_url_request.h b/atom/browser/net/atom_url_request.h index dd6e3d11e08..f065bc5249a 100644 --- a/atom/browser/net/atom_url_request.h +++ b/atom/browser/net/atom_url_request.h @@ -8,25 +8,19 @@ #include #include +#include "atom/browser/api/atom_api_url_request.h" +#include "atom/browser/atom_browser_context.h" #include "base/memory/ref_counted.h" +#include "base/memory/weak_ptr.h" +#include "net/base/auth.h" #include "net/base/chunked_upload_data_stream.h" +#include "net/base/io_buffer.h" +#include "net/base/upload_element_reader.h" +#include "net/http/http_response_headers.h" #include "net/url_request/url_request.h" - -namespace net { -class IOBuffer; -class IOBufferWithSize; -class DrainableIOBuffer; -}; - namespace atom { -class AtomBrowserContext; - -namespace api { -class URLRequest; -} - class AtomURLRequest : public base::RefCountedThreadSafe, public net::URLRequest::Delegate { public: @@ -56,6 +50,10 @@ class AtomURLRequest : public base::RefCountedThreadSafe, private: friend class base::RefCountedThreadSafe; + + explicit AtomURLRequest(base::WeakPtr delegate); + ~AtomURLRequest()override; + void DoWriteBuffer(scoped_refptr buffer, bool is_last); void DoAbort() const; @@ -74,9 +72,6 @@ class AtomURLRequest : public base::RefCountedThreadSafe, void InformDelegateResponseCompleted() const; void InformDelegateErrorOccured(const std::string& error) const; - explicit AtomURLRequest(base::WeakPtr delegate); - virtual ~AtomURLRequest(); - base::WeakPtr delegate_; std::unique_ptr request_;