Fixing code review issues: function call formatting, renaming JS member variables, refactoring response headers conversion.

atom/browser/api/atom_api_url_request.cc

@@ -16,24 +16,6 @@
 
 namespace mate {
 
-template<>
-struct Converter<scoped_refptr<const net::HttpResponseHeaders>> {
-  static v8::Local<v8::Value> ToV8(
-    v8::Isolate* isolate,
-    scoped_refptr<const net::HttpResponseHeaders> val) {
-    mate::Dictionary dict = mate::Dictionary::CreateEmpty(isolate);
-    if (val) {
-      size_t iter = 0;
-      std::string name;
-      std::string value;
-      while (val->EnumerateHeaderLines(&iter, &name, &value)) {
-        dict.Set(name, value);
-      }
-    }
-    return dict.GetHandle();
-  }
-};
-
 template<>
 struct Converter<scoped_refptr<const net::IOBufferWithSize>> {
   static v8::Local<v8::Value> ToV8(
@@ -179,10 +161,7 @@ mate::WrappableBase* URLRequest::New(mate::Arguments* args) {
   auto browser_context = session->browser_context();
   auto api_url_request = new URLRequest(args->isolate(), args->GetThis());
   auto weak_ptr = api_url_request->weak_ptr_factory_.GetWeakPtr();
-  auto atom_url_request = AtomURLRequest::Create(
+  auto atom_url_request = AtomURLRequest::Create(browser_context, method, url,
-    browser_context,
-    method,
-    url,
                                                  weak_ptr);
 
   api_url_request->atom_request_ = atom_url_request;

atom/browser/api/atom_api_web_contents.cc

@@ -35,10 +35,10 @@
 #include "atom/common/native_mate_converters/gfx_converter.h"
 #include "atom/common/native_mate_converters/gurl_converter.h"
 #include "atom/common/native_mate_converters/image_converter.h"
+#include "atom/common/native_mate_converters/net_converter.h"
 #include "atom/common/native_mate_converters/string16_converter.h"
 #include "atom/common/native_mate_converters/value_converter.h"
 #include "atom/common/options_switches.h"
-#include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "brightray/browser/inspectable_web_contents.h"
 #include "brightray/browser/inspectable_web_contents_view.h"
@@ -66,7 +66,6 @@
 #include "content/public/common/context_menu_params.h"
 #include "native_mate/dictionary.h"
 #include "native_mate/object_template_builder.h"
-#include "net/http/http_response_headers.h"
 #include "net/url_request/url_request_context.h"
 #include "third_party/WebKit/public/web/WebFindOptions.h"
 #include "third_party/WebKit/public/web/WebInputEvent.h"
@@ -141,32 +140,6 @@ struct Converter<WindowOpenDisposition> {
   }
 };
 
-template<>
-struct Converter<net::HttpResponseHeaders*> {
-  static v8::Local<v8::Value> ToV8(v8::Isolate* isolate,
-                                   net::HttpResponseHeaders* headers) {
-    base::DictionaryValue response_headers;
-    if (headers) {
-      size_t iter = 0;
-      std::string key;
-      std::string value;
-      while (headers->EnumerateHeaderLines(&iter, &key, &value)) {
-        key = base::ToLowerASCII(key);
-        if (response_headers.HasKey(key)) {
-          base::ListValue* values = nullptr;
-          if (response_headers.GetList(key, &values))
-            values->AppendString(value);
-        } else {
-          std::unique_ptr<base::ListValue> values(new base::ListValue());
-          values->AppendString(value);
-          response_headers.Set(key, std::move(values));
-        }
-      }
-    }
-    return ConvertToV8(isolate, response_headers);
-  }
-};
-
 template<>
 struct Converter<content::SavePageType> {
   static bool FromV8(v8::Isolate* isolate, v8::Local<v8::Value> val,
@@ -691,7 +664,7 @@ void WebContents::DidGetResourceResponseStart(
        details.http_response_code,
        details.method,
        details.referrer,
-       details.headers.get(),
+       details.headers,
        ResourceTypeToString(details.resource_type));
 }
 
@@ -705,7 +678,7 @@ void WebContents::DidGetRedirectForResourceRequest(
        details.http_response_code,
        details.method,
        details.referrer,
-       details.headers.get());
+       details.headers);
 }
 
 void WebContents::DidFinishNavigation(

atom/browser/net/atom_url_request.cc

@@ -64,29 +64,42 @@ scoped_refptr<AtomURLRequest> AtomURLRequest::Create(
     const std::string& url,
     base::WeakPtr<api::URLRequest> delegate) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
   DCHECK(browser_context);
   DCHECK(!url.empty());
+  if (!browser_context || url.empty()) {
+    return nullptr;
+  }
 
   auto request_context_getter = browser_context->url_request_context_getter();
+  scoped_refptr<AtomURLRequest> atom_url_request(new AtomURLRequest(delegate));
+  if (content::BrowserThread::PostTask(
+      content::BrowserThread::IO,
+      FROM_HERE,
+      base::Bind(&AtomURLRequest::DoInitialize, atom_url_request,
+                 request_context_getter, method, url))) {
+    return atom_url_request;
+  }
+  return nullptr;
+}
 
+void AtomURLRequest::DoInitialize(
+    scoped_refptr<net::URLRequestContextGetter> request_context_getter,
+    const std::string& method,
+    const std::string& url) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
   DCHECK(request_context_getter);
 
   auto context = request_context_getter->GetURLRequestContext();
 
   DCHECK(context);
-
+  request_ = context->CreateRequest(GURL(url),
-  scoped_refptr<AtomURLRequest> atom_url_request =
-    new AtomURLRequest(delegate);
-
-  atom_url_request->request_ = context->CreateRequest(GURL(url),
                                     net::RequestPriority::DEFAULT_PRIORITY,
-    atom_url_request.get());
+                                    this);
 
-  atom_url_request->request_->set_method(method);
+  request_->set_method(method);
-  return atom_url_request;
 }
 
-
 bool AtomURLRequest::Write(
     scoped_refptr<const net::IOBufferWithSize> buffer,
     bool is_last) {
@@ -105,7 +118,6 @@ void AtomURLRequest::SetChunkedUpload(bool is_chunked_upload) {
   is_chunked_upload_ = is_chunked_upload;
 }
 
-
 void AtomURLRequest::Cancel() const {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   content::BrowserThread::PostTask(
@@ -117,29 +129,34 @@ void AtomURLRequest::Cancel() const {
 void AtomURLRequest::SetExtraHeader(const std::string& name,
                                const std::string& value) const {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  request_->SetExtraRequestHeaderByName(name, value, true);
+  content::BrowserThread::PostTask(
+      content::BrowserThread::IO,
+      FROM_HERE,
+      base::Bind(&AtomURLRequest::DoSetExtraHeader, this, name, value));
 }
 
-
 void AtomURLRequest::RemoveExtraHeader(const std::string& name) const {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  request_->RemoveRequestHeaderByName(name);
+  content::BrowserThread::PostTask(
+      content::BrowserThread::IO,
+      FROM_HERE,
+      base::Bind(&AtomURLRequest::DoRemoveExtraHeader, this, name));
 }
 
 void AtomURLRequest::PassLoginInformation(const base::string16& username,
   const base::string16& password) const {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  if (username.empty() || password.empty())
+  if (username.empty() || password.empty()) {
     content::BrowserThread::PostTask(
         content::BrowserThread::IO, FROM_HERE,
         base::Bind(&AtomURLRequest::DoCancelAuth, this));
-  else
+  } else {
     content::BrowserThread::PostTask(
         content::BrowserThread::IO, FROM_HERE,
         base::Bind(&AtomURLRequest::DoSetAuth, this, username, password));
+  }
 }
 
-
 void AtomURLRequest::DoWriteBuffer(
   scoped_refptr<const net::IOBufferWithSize> buffer,
   bool is_last) {
@@ -168,8 +185,9 @@ void AtomURLRequest::DoWriteBuffer(
                                          0,
                                          true);
 
-    if (first_call)
+    if (first_call) {
       request_->Start();
+    }
   } else {
     if (buffer) {
       // Handling potential empty buffers.
@@ -182,8 +200,7 @@ void AtomURLRequest::DoWriteBuffer(
 
     if (is_last) {
       auto elements_upload_data_stream = new net::ElementsUploadDataStream(
-         std::move(upload_element_readers_),
+          std::move(upload_element_readers_), 0);
-         0);
       request_->set_upload(
           std::unique_ptr<net::UploadDataStream>(elements_upload_data_stream));
       request_->Start();
@@ -196,6 +213,16 @@ void AtomURLRequest::DoCancel() const {
   request_->Cancel();
 }
 
+void AtomURLRequest::DoSetExtraHeader(const std::string& name,
+                                      const std::string& value) const {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  request_->SetExtraRequestHeaderByName(name, value, true);
+}
+void AtomURLRequest::DoRemoveExtraHeader(const std::string& name) const {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  request_->RemoveRequestHeaderByName(name);
+}
+
 void AtomURLRequest::DoSetAuth(const base::string16& username,
   const base::string16& password) const {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);

atom/browser/net/atom_url_request.h

@@ -53,9 +53,15 @@ class AtomURLRequest : public base::RefCountedThreadSafe<AtomURLRequest>,
   explicit AtomURLRequest(base::WeakPtr<api::URLRequest> delegate);
   ~AtomURLRequest()override;
 
+  void DoInitialize(scoped_refptr<net::URLRequestContextGetter>,
+                    const std::string& method,
+                    const std::string& url);
   void DoWriteBuffer(scoped_refptr<const net::IOBufferWithSize> buffer,
                      bool is_last);
   void DoCancel() const;
+  void DoSetExtraHeader(const std::string& name,
+                        const std::string& value) const;
+  void DoRemoveExtraHeader(const std::string& name) const;
   void DoSetAuth(const base::string16& username,
     const base::string16& password) const;
   void DoCancelAuth() const;

atom/common/native_mate_converters/net_converter.cc

@@ -10,6 +10,7 @@
 #include "atom/common/native_mate_converters/gurl_converter.h"
 #include "atom/common/native_mate_converters/value_converter.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/strings/string_util.h"
 #include "base/values.h"
 #include "native_mate/dictionary.h"
 #include "net/base/upload_bytes_element_reader.h"
@@ -58,6 +59,32 @@ v8::Local<v8::Value> Converter<scoped_refptr<net::X509Certificate>>::ToV8(
   return dict.GetHandle();
 }
 
+// static
+v8::Local<v8::Value>
+Converter<scoped_refptr<const net::HttpResponseHeaders>>::ToV8(
+    v8::Isolate* isolate,
+    scoped_refptr<const net::HttpResponseHeaders> headers) {
+  base::DictionaryValue response_headers;
+  if (headers) {
+    size_t iter = 0;
+    std::string key;
+    std::string value;
+    while (headers->EnumerateHeaderLines(&iter, &key, &value)) {
+      key = base::ToLowerASCII(key);
+      if (response_headers.HasKey(key)) {
+        base::ListValue* values = nullptr;
+        if (response_headers.GetList(key, &values))
+          values->AppendString(value);
+      } else {
+        std::unique_ptr<base::ListValue> values(new base::ListValue());
+        values->AppendString(value);
+        response_headers.Set(key, std::move(values));
+      }
+    }
+  }
+  return ConvertToV8(isolate, response_headers);
+}
+
 }  // namespace mate
 
 namespace atom {

atom/common/native_mate_converters/net_converter.h

@@ -17,6 +17,7 @@ namespace net {
 class AuthChallengeInfo;
 class URLRequest;
 class X509Certificate;
+class HttpResponseHeaders;
 }
 
 namespace mate {
@@ -33,6 +34,12 @@ struct Converter<scoped_refptr<net::X509Certificate>> {
       const scoped_refptr<net::X509Certificate>& val);
 };
 
+template<>
+struct Converter<scoped_refptr<const net::HttpResponseHeaders>> {
+  static v8::Local<v8::Value> ToV8(v8::Isolate* isolate,
+    scoped_refptr<const net::HttpResponseHeaders> headers);
+};
+
 }  // namespace mate
 
 namespace atom {

lib/browser/api/net.js

@@ -18,29 +18,29 @@ kSupportedProtocols.add('https:')
 class IncomingMessage extends Readable {
   constructor (urlRequest) {
     super()
-    this._url_request = urlRequest
+    this.urlRequest = urlRequest
-    this._shouldPush = false
+    this.shouldPush = false
-    this._data = []
+    this.data = []
-    this._url_request.on('data', (event, chunk) => {
+    this.urlRequest.on('data', (event, chunk) => {
       this._storeInternalData(chunk)
       this._pushInternalData()
     })
-    this._url_request.on('end', () => {
+    this.urlRequest.on('end', () => {
       this._storeInternalData(null)
       this._pushInternalData()
     })
   }
 
   get statusCode () {
-    return this._url_request.statusCode
+    return this.urlRequest.statusCode
   }
 
   get statusMessage () {
-    return this._url_request.statusMessage
+    return this.urlRequest.statusMessage
   }
 
   get headers () {
-    return this._url_request.rawResponseHeaders
+    return this.urlRequest.rawResponseHeaders
   }
 
   get httpVersion () {
@@ -48,15 +48,11 @@ class IncomingMessage extends Readable {
   }
 
   get httpVersionMajor () {
-    return this._url_request.httpVersionMajor
+    return this.urlRequest.httpVersionMajor
   }
 
   get httpVersionMinor () {
-    return this._url_request.httpVersionMinor
+    return this.urlRequest.httpVersionMinor
-  }
-
-  get rawHeaders () {
-    return this._url_request.rawResponseHeaders
   }
 
   get rawTrailers () {
@@ -68,18 +64,18 @@ class IncomingMessage extends Readable {
   }
 
   _storeInternalData (chunk) {
-    this._data.push(chunk)
+    this.data.push(chunk)
   }
 
   _pushInternalData () {
-    while (this._shouldPush && this._data.length > 0) {
+    while (this.shouldPush && this.data.length > 0) {
-      const chunk = this._data.shift()
+      const chunk = this.data.shift()
-      this._shouldPush = this.push(chunk)
+      this.shouldPush = this.push(chunk)
     }
   }
 
   _read () {
-    this._shouldPush = true
+    this.shouldPush = true
     this._pushInternalData()
   }
 
@@ -88,17 +84,17 @@ class IncomingMessage extends Readable {
 URLRequest.prototype._emitRequestEvent = function (async, ...rest) {
   if (async) {
     process.nextTick(() => {
-      this._request.emit.apply(this._request, rest)
+      this.clientRequest.emit.apply(this.clientRequest, rest)
     })
   } else {
-    this._request.emit.apply(this._request, rest)
+    this.clientRequest.emit.apply(this.clientRequest, rest)
   }
 }
 
 URLRequest.prototype._emitResponseEvent = function (async, ...rest) {
   if (async) {
     process.nextTick(() => {
-      this._request.emit.apply(this._response, rest)
+      this.clientRequest.emit.apply(this._response, rest)
     })
   } else {
     this._response.emit.apply(this._response, rest)
@@ -165,13 +161,13 @@ class ClientRequest extends EventEmitter {
     })
 
     // Set back and forward links.
-    this._url_request = urlRequest
+    this.urlRequest = urlRequest
-    urlRequest._request = this
+    urlRequest.clientRequest = this
 
     // This is a copy of the extra headers structure held by the native
     // net::URLRequest. The main reason is to keep the getHeader API synchronous
     // after the request starts.
-    this._extra_headers = {}
+    this.extraHeaders = {}
 
     if (options.headers) {
       const keys = Object.keys(options.headers)
@@ -183,7 +179,7 @@ class ClientRequest extends EventEmitter {
 
     // Set when the request uses chunked encoding. Can be switched
     // to true only once and never set back to false.
-    this._chunkedEncoding = false
+    this.chunkedEncodingEnabled = false
 
     urlRequest.on('response', () => {
       const response = new IncomingMessage(urlRequest)
@@ -197,14 +193,14 @@ class ClientRequest extends EventEmitter {
   }
 
   get chunkedEncoding () {
-    return this._chunkedEncoding
+    return this.chunkedEncodingEnabled
   }
 
   set chunkedEncoding (value) {
-    if (!this._url_request.notStarted) {
+    if (!this.urlRequest.notStarted) {
       throw new Error('Can\'t set the transfer encoding, headers have been sent.')
     }
-    this._chunkedEncoding = value
+    this.chunkedEncodingEnabled = value
   }
 
   setHeader (name, value) {
@@ -214,13 +210,13 @@ class ClientRequest extends EventEmitter {
     if (value === undefined) {
       throw new Error('`value` required in setHeader("' + name + '", value).')
     }
-    if (!this._url_request.notStarted) {
+    if (!this.urlRequest.notStarted) {
       throw new Error('Can\'t set headers after they are sent.')
     }
 
     const key = name.toLowerCase()
-    this._extra_headers[key] = value
+    this.extraHeaders[key] = value
-    this._url_request.setExtraHeader(name, value)
+    this.urlRequest.setExtraHeader(name, value)
   }
 
   getHeader (name) {
@@ -228,12 +224,12 @@ class ClientRequest extends EventEmitter {
       throw new Error('`name` is required for getHeader(name).')
     }
 
-    if (!this._extra_headers) {
+    if (!this.extraHeaders) {
       return
     }
 
     const key = name.toLowerCase()
-    return this._extra_headers[key]
+    return this.extraHeaders[key]
   }
 
   removeHeader (name) {
@@ -241,13 +237,13 @@ class ClientRequest extends EventEmitter {
       throw new Error('`name` is required for removeHeader(name).')
     }
 
-    if (!this._url_request.notStarted) {
+    if (!this.urlRequest.notStarted) {
       throw new Error('Can\'t remove headers after they are sent.')
     }
 
     const key = name.toLowerCase()
-    delete this._extra_headers[key]
+    delete this.extraHeaders[key]
-    this._url_request.removeExtraHeader(name)
+    this.urlRequest.removeExtraHeader(name)
   }
 
   _write (chunk, encoding, callback, isLast) {
@@ -265,13 +261,13 @@ class ClientRequest extends EventEmitter {
     // Since writing to the network is asynchronous, we conservatively
     // assume that request headers are written after delivering the first
     // buffer to the network IO thread.
-    if (this._url_request.notStarted) {
+    if (this.urlRequest.notStarted) {
-      this._url_request.setChunkedUpload(this.chunkedEncoding)
+      this.urlRequest.setChunkedUpload(this.chunkedEncoding)
     }
 
     // Headers are assumed to be sent on first call to _writeBuffer,
     // i.e. after the first call to write or end.
-    let result = this._url_request.write(chunk, isLast)
+    let result = this.urlRequest.write(chunk, isLast)
 
     // The write callback is fired asynchronously to mimic Node.js.
     if (callback) {
@@ -282,7 +278,7 @@ class ClientRequest extends EventEmitter {
   }
 
   write (data, encoding, callback) {
-    if (this._url_request.finished) {
+    if (this.urlRequest.finished) {
       let error = new Error('Write after end.')
       process.nextTick(writeAfterEndNT, this, error, callback)
       return true
@@ -292,7 +288,7 @@ class ClientRequest extends EventEmitter {
   }
 
   end (data, encoding, callback) {
-    if (this._url_request.finished) {
+    if (this.urlRequest.finished) {
       return false
     }
 
@@ -311,7 +307,7 @@ class ClientRequest extends EventEmitter {
   }
 
   abort () {
-    this._url_request.cancel()
+    this.urlRequest.cancel()
   }
 
 }

spec/api-net-spec.js

@@ -949,8 +949,28 @@ describe('net module', function () {
       nodeRequest.end()
     })
 
-    it.skip('should emit error event on server socket close', function (done) {
+    it('should emit error event on server socket close', function (done) {
+      const requestUrl = '/requestUrl'
+      server.on('request', function (request, response) {
+        switch (request.url) {
+          case requestUrl:
+            request.socket.destroy()
+            break
+          default:
             assert(false)
+        }
+      })
+      let requestErrorEventEmitted = false
+      const urlRequest = net.request(`${server.url}${requestUrl}`)
+      urlRequest.on('error', function (error) {
+        assert(error)
+        requestErrorEventEmitted = true
+      })
+      urlRequest.on('close', function () {
+        assert(requestErrorEventEmitted)
+        done()
+      })
+      urlRequest.end()
     })
   })
   describe('IncomingMessage API', function () {
@@ -995,10 +1015,10 @@ describe('net module', function () {
         const statusMessage = response.statusMessage
         assert(typeof statusMessage === 'string')
         assert.equal(statusMessage, 'OK')
-        const rawHeaders = response.rawHeaders
+        const headers = response.headers
-        assert(typeof rawHeaders === 'object')
+        assert(typeof headers === 'object')
-        assert(rawHeaders[customHeaderName] ===
+        assert.deepEqual(headers[customHeaderName.toLowerCase()],
-          customHeaderValue)
+          [customHeaderValue])
         const httpVersion = response.httpVersion
         assert(typeof httpVersion === 'string')
         assert(httpVersion.length > 0)
@@ -1078,8 +1098,60 @@ describe('net module', function () {
       netRequest.end()
     })
 
-    it.skip('should not emit any event after close', function () {
+    it('should not emit any event after close', function (done) {
+      const requestUrl = '/requestUrl'
+      let bodyData = randomString(kOneKiloByte)
+      server.on('request', function (request, response) {
+        switch (request.url) {
+          case requestUrl:
+            response.statusCode = 200
+            response.statusMessage = 'OK'
+            response.write(bodyData)
+            response.end()
+            break
+          default:
             assert(false)
+        }
+      })
+      let requestCloseEventEmitted = false
+      const urlRequest = net.request({
+        method: 'GET',
+        url: `${server.url}${requestUrl}`
+      })
+      urlRequest.on('response', function (response) {
+        assert(!requestCloseEventEmitted)
+        const statusCode = response.statusCode
+        assert.equal(statusCode, 200)
+        response.pause()
+        response.on('data', function () {
+        })
+        response.on('end', function () {
+        })
+        response.resume()
+        response.on('error', function () {
+          assert(!requestCloseEventEmitted)
+        })
+        response.on('aborted', function () {
+          assert(!requestCloseEventEmitted)
+        })
+      })
+      urlRequest.on('finish', function () {
+        assert(!requestCloseEventEmitted)
+      })
+      urlRequest.on('error', function () {
+        assert(!requestCloseEventEmitted)
+      })
+      urlRequest.on('abort', function () {
+        assert(!requestCloseEventEmitted)
+      })
+      urlRequest.on('close', function () {
+        requestCloseEventEmitted = true
+        // Wait so that all async events get scheduled.
+        setTimeout(function () {
+          done()
+        }, 100)
+      })
+      urlRequest.end()
     })
   })
 })