Fixing code review issues.

atom/browser/api/atom_api_net.cc

@@ -1,4 +1,4 @@
-// Copyright (c) 2013 GitHub, Inc.
+// Copyright (c) 2016 GitHub, Inc.
 // Use of this source code is governed by the MIT license that can be
 // found in the LICENSE file.
 
@@ -27,20 +27,13 @@ void Net::BuildPrototype(v8::Isolate* isolate,
                          v8::Local<v8::FunctionTemplate> prototype) {
   prototype->SetClassName(mate::StringToV8(isolate, "Net"));
   mate::ObjectTemplateBuilder(isolate, prototype->PrototypeTemplate())
-      .SetProperty("URLRequest", &Net::URLRequest)
+      .SetProperty("URLRequest", &Net::URLRequest);
-      .SetMethod("_RequestGarbageCollectionForTesting",
-                 &Net::RequestGarbageCollectionForTesting);
 }
 
 v8::Local<v8::Value> Net::URLRequest(v8::Isolate* isolate) {
   return URLRequest::GetConstructor(isolate)->GetFunction();
 }
 
-void Net::RequestGarbageCollectionForTesting() {
-  isolate()->RequestGarbageCollectionForTesting(
-      v8::Isolate::GarbageCollectionType::kFullGarbageCollection);
-}
-
 }  // namespace api
 
 }  // namespace atom

atom/browser/api/atom_api_net.h

@@ -1,4 +1,4 @@
-// Copyright (c) 2013 GitHub, Inc.
+// Copyright (c) 2016 GitHub, Inc.
 // Use of this source code is governed by the MIT license that can be
 // found in the LICENSE file.
 
@@ -25,7 +25,6 @@ class Net : public mate::EventEmitter<Net> {
   ~Net() override;
 
  private:
-  void RequestGarbageCollectionForTesting();
   DISALLOW_COPY_AND_ASSIGN(Net);
 };
 

atom/browser/api/atom_api_url_request.cc

@@ -1,4 +1,4 @@
-// Copyright (c) 2013 GitHub, Inc.
+// Copyright (c) 2016 GitHub, Inc.
 // Use of this source code is governed by the MIT license that can be
 // found in the LICENSE file.
 
@@ -42,22 +42,53 @@ struct Converter<scoped_refptr<const net::IOBufferWithSize>> {
       return false;
     }
 
-    auto io_buffer = new net::IOBufferWithSize(size);
+    *out = new net::IOBufferWithSize(size);
-    if (!io_buffer) {
-      // Assuming allocation failed.
-      return false;
-    }
-
     // We do a deep copy. We could have used Buffer's internal memory
     // but that is much more complicated to be properly handled.
-    memcpy(io_buffer->data(), data, size);
+    memcpy((*out)->data(), data, size);
-    *out = io_buffer;
     return true;
   }
 };
 
 }  // namespace mate
 
+namespace {
+
+template <typename... ArgTypes>
+std::array<v8::Local<v8::Value>, sizeof...(ArgTypes)> BuildArgsArray(
+    v8::Isolate* isolate,
+    ArgTypes... args) {
+  std::array<v8::Local<v8::Value>, sizeof...(ArgTypes)> result = {
+      {mate::ConvertToV8(isolate, args)...}};
+  return result;
+}
+
+template <typename... ArgTypes>
+void EmitRequestEvent(v8::Isolate* isolate,
+                      v8::Local<v8::Object> object,
+                      ArgTypes... args) {
+  v8::HandleScope handle_scope(isolate);
+  auto arguments = BuildArgsArray(isolate, args...);
+  v8::Local<v8::Function> _emitRequestEvent;
+  if (mate::Dictionary(isolate, object)
+          .Get("_emitRequestEvent", &_emitRequestEvent))
+    _emitRequestEvent->Call(object, arguments.size(), arguments.data());
+}
+
+template <typename... ArgTypes>
+void EmitResponseEvent(v8::Isolate* isolate,
+                       v8::Local<v8::Object> object,
+                       ArgTypes... args) {
+  v8::HandleScope handle_scope(isolate);
+  auto arguments = BuildArgsArray(isolate, args...);
+  v8::Local<v8::Function> _emitResponseEvent;
+  if (mate::Dictionary(isolate, object)
+          .Get("_emitResponseEvent", &_emitResponseEvent))
+    _emitResponseEvent->Call(object, arguments.size(), arguments.data());
+}
+
+}  // namespace
+
 namespace atom {
 namespace api {
 
@@ -210,12 +241,12 @@ bool URLRequest::Write(scoped_refptr<const net::IOBufferWithSize> buffer,
   if (request_state_.NotStarted()) {
     request_state_.SetFlag(RequestStateFlags::kStarted);
     // Pin on first write.
-    pin();
+    Pin();
   }
 
   if (is_last) {
     request_state_.SetFlag(RequestStateFlags::kFinished);
-    EmitRequestEvent(true, "finish");
+    EmitRequestEvent(isolate(), GetWrapper(), true, "finish");
   }
 
   DCHECK(atom_request_);
@@ -239,10 +270,10 @@ void URLRequest::Cancel() {
     // Really cancel if it was started.
     atom_request_->Cancel();
   }
-  EmitRequestEvent(true, "abort");
+  EmitRequestEvent(isolate(), GetWrapper(), true, "abort");
 
   if (response_state_.Started() && !response_state_.Ended()) {
-    EmitResponseEvent(true, "aborted");
+    EmitResponseEvent(isolate(), GetWrapper(), true, "aborted");
   }
   Close();
 }
@@ -351,10 +382,10 @@ void URLRequest::OnError(const std::string& error, bool isRequestError) {
   auto error_object = v8::Exception::Error(mate::StringToV8(isolate(), error));
   if (isRequestError) {
     request_state_.SetFlag(RequestStateFlags::kFailed);
-    EmitRequestEvent(false, "error", error_object);
+    EmitRequestEvent(isolate(), GetWrapper(), false, "error", error_object);
   } else {
     response_state_.SetFlag(ResponseStateFlags::kFailed);
-    EmitResponseEvent(false, "error", error_object);
+    EmitResponseEvent(isolate(), GetWrapper(), false, "error", error_object);
   }
   Close();
 }
@@ -374,8 +405,8 @@ std::string URLRequest::StatusMessage() const {
   return result;
 }
 
-scoped_refptr<net::HttpResponseHeaders> URLRequest::RawResponseHeaders() const {
+net::HttpResponseHeaders* URLRequest::RawResponseHeaders() const {
-  return response_headers_;
+  return response_headers_.get();
 }
 
 uint32_t URLRequest::ResponseHttpVersionMajor() const {
@@ -397,11 +428,11 @@ void URLRequest::Close() {
     request_state_.SetFlag(RequestStateFlags::kClosed);
     if (response_state_.Started()) {
       // Emit a close event if we really have a response object.
-      EmitResponseEvent(true, "close");
+      EmitResponseEvent(isolate(), GetWrapper(), true, "close");
     }
-    EmitRequestEvent(true, "close");
+    EmitRequestEvent(isolate(), GetWrapper(), true, "close");
   }
-  unpin();
+  Unpin();
   if (atom_request_) {
     // A request has been created in JS, used and then it ended.
     // We release unneeded net resources.
@@ -410,13 +441,13 @@ void URLRequest::Close() {
   atom_request_ = nullptr;
 }
 
-void URLRequest::pin() {
+void URLRequest::Pin() {
   if (wrapper_.IsEmpty()) {
     wrapper_.Reset(isolate(), GetWrapper());
   }
 }
 
-void URLRequest::unpin() {
+void URLRequest::Unpin() {
   wrapper_.Reset();
 }
 

atom/browser/api/atom_api_url_request.h

@@ -1,4 +1,4 @@
-// Copyright (c) 2013 GitHub, Inc.
+// Copyright (c) 2016 GitHub, Inc.
 // Use of this source code is governed by the MIT license that can be
 // found in the LICENSE file.
 
@@ -175,23 +175,23 @@ class URLRequest : public mate::EventEmitter<URLRequest> {
 
   int StatusCode() const;
   std::string StatusMessage() const;
-  scoped_refptr<net::HttpResponseHeaders> RawResponseHeaders() const;
+  net::HttpResponseHeaders* RawResponseHeaders() const;
   uint32_t ResponseHttpVersionMajor() const;
   uint32_t ResponseHttpVersionMinor() const;
 
-  template <typename... ArgTypes>
+  // template <typename... ArgTypes>
-  std::array<v8::Local<v8::Value>, sizeof...(ArgTypes)> BuildArgsArray(
+  // std::array<v8::Local<v8::Value>, sizeof...(ArgTypes)> BuildArgsArray(
-      ArgTypes... args) const;
+  //    ArgTypes... args) const;
 
-  template <typename... ArgTypes>
+  // template <typename... ArgTypes>
-  void EmitRequestEvent(ArgTypes... args);
+  // void EmitRequestEvent(ArgTypes... args);
 
-  template <typename... ArgTypes>
+  // template <typename... ArgTypes>
-  void EmitResponseEvent(ArgTypes... args);
+  // void EmitResponseEvent(ArgTypes... args);
 
   void Close();
-  void pin();
+  void Pin();
-  void unpin();
+  void Unpin();
 
   scoped_refptr<AtomURLRequest> atom_request_;
   RequestState request_state_;
@@ -205,36 +205,6 @@ class URLRequest : public mate::EventEmitter<URLRequest> {
   DISALLOW_COPY_AND_ASSIGN(URLRequest);
 };
 
-template <typename... ArgTypes>
-std::array<v8::Local<v8::Value>, sizeof...(ArgTypes)>
-URLRequest::BuildArgsArray(ArgTypes... args) const {
-  std::array<v8::Local<v8::Value>, sizeof...(ArgTypes)> result = {
-      {mate::ConvertToV8(isolate(), args)...}};
-  return result;
-}
-
-template <typename... ArgTypes>
-void URLRequest::EmitRequestEvent(ArgTypes... args) {
-  v8::HandleScope handle_scope(isolate());
-  auto arguments = BuildArgsArray(args...);
-  v8::Local<v8::Function> _emitRequestEvent;
-  auto wrapper = GetWrapper();
-  if (mate::Dictionary(isolate(), wrapper)
-          .Get("_emitRequestEvent", &_emitRequestEvent))
-    _emitRequestEvent->Call(wrapper, arguments.size(), arguments.data());
-}
-
-template <typename... ArgTypes>
-void URLRequest::EmitResponseEvent(ArgTypes... args) {
-  v8::HandleScope handle_scope(isolate());
-  auto arguments = BuildArgsArray(args...);
-  v8::Local<v8::Function> _emitResponseEvent;
-  auto wrapper = GetWrapper();
-  if (mate::Dictionary(isolate(), wrapper)
-          .Get("_emitResponseEvent", &_emitResponseEvent))
-    _emitResponseEvent->Call(wrapper, arguments.size(), arguments.data());
-}
-
 }  // namespace api
 
 }  // namespace atom

atom/browser/api/atom_api_web_contents.cc

@@ -664,7 +664,7 @@ void WebContents::DidGetResourceResponseStart(
        details.http_response_code,
        details.method,
        details.referrer,
-       details.headers,
+       details.headers.get(),
        ResourceTypeToString(details.resource_type));
 }
 
@@ -678,7 +678,7 @@ void WebContents::DidGetRedirectForResourceRequest(
        details.http_response_code,
        details.method,
        details.referrer,
-       details.headers);
+       details.headers.get());
 }
 
 void WebContents::DidFinishNavigation(

atom/browser/net/atom_url_request.cc

@@ -1,4 +1,4 @@
-// Copyright (c) 2013 GitHub, Inc.
+// Copyright (c) 2016 GitHub, Inc.
 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by the MIT license that can be
 // found in the LICENSE file.

atom/browser/net/atom_url_request.h

@@ -1,4 +1,4 @@
-// Copyright (c) 2013 GitHub, Inc.
+// Copyright (c) 2016 GitHub, Inc.
 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by the MIT license that can be
 // found in the LICENSE file.

atom/common/api/atom_api_v8_util.cc

@@ -92,6 +92,11 @@ void TakeHeapSnapshot(v8::Isolate* isolate) {
   isolate->GetHeapProfiler()->TakeHeapSnapshot();
 }
 
+void RequestGarbageCollectionForTesting(v8::Isolate* isolate) {
+  isolate->RequestGarbageCollectionForTesting(
+    v8::Isolate::GarbageCollectionType::kFullGarbageCollection);
+}
+
 void Initialize(v8::Local<v8::Object> exports, v8::Local<v8::Value> unused,
                 v8::Local<v8::Context> context, void* priv) {
   mate::Dictionary dict(context->GetIsolate(), exports);
@@ -105,6 +110,8 @@ void Initialize(v8::Local<v8::Object> exports, v8::Local<v8::Value> unused,
   dict.SetMethod("createIDWeakMap", &atom::api::KeyWeakMap<int32_t>::Create);
   dict.SetMethod("createDoubleIDWeakMap",
                  &atom::api::KeyWeakMap<std::pair<int64_t, int32_t>>::Create);
+  dict.SetMethod("requestGarbageCollectionForTesting",
+                 &RequestGarbageCollectionForTesting);
 }
 
 }  // namespace

atom/common/native_mate_converters/net_converter.cc

@@ -60,10 +60,9 @@ v8::Local<v8::Value> Converter<scoped_refptr<net::X509Certificate>>::ToV8(
 }
 
 // static
-v8::Local<v8::Value>
+v8::Local<v8::Value> Converter<net::HttpResponseHeaders*>::ToV8(
-Converter<scoped_refptr<net::HttpResponseHeaders>>::ToV8(
     v8::Isolate* isolate,
-    scoped_refptr<net::HttpResponseHeaders> headers) {
+    net::HttpResponseHeaders* headers) {
   base::DictionaryValue response_headers;
   if (headers) {
     size_t iter = 0;

atom/common/native_mate_converters/net_converter.h

@@ -35,10 +35,9 @@ struct Converter<scoped_refptr<net::X509Certificate>> {
 };
 
 template <>
-struct Converter<scoped_refptr<net::HttpResponseHeaders>> {
+struct Converter<net::HttpResponseHeaders*> {
-  static v8::Local<v8::Value> ToV8(
+  static v8::Local<v8::Value> ToV8(v8::Isolate* isolate,
-      v8::Isolate* isolate,
+                                   net::HttpResponseHeaders* headers);
-      scoped_refptr<net::HttpResponseHeaders> headers);
 };
 
 }  // namespace mate

docs/api/net.md

@@ -226,6 +226,11 @@ string, it is converted into a Buffer using the specified encoding.
 * `encoding` String (optional) - Used to convert string chunks into Buffer
 objects. Defaults to 'utf-8'.
 * `callback` Function (optional) - Called after the write operation ends.
+`callback` is essentially a dummy function introduced in the purpose of keeping
+similarity with the Node.js API. It is called asynchronously in the next tick
+after `chunk` content have been delivered to the Chromium networking layer.
+Contrary to the Node.js implementation, it is not guaranteed that `chunk`
+content have been flushed on the wire before `callback` is called.
 
 Adds a chunk of data to the request body. The first write operation may cause
 the request headers to be issued on the wire. After the first write operation,
@@ -311,10 +316,3 @@ A String indicating the HTTP protocol version number. Typical values are '1.0'
 or '1.1'. Additionally `httpVersionMajor` and `httpVersionMinor` are two
 Integer-valued readable properties that return respectively the HTTP major and
 minor version numbers.
-
-
-
-
-
-
-

lib/browser/api/net.js

@@ -3,18 +3,15 @@
 const url = require('url')
 const {EventEmitter} = require('events')
 const util = require('util')
-const Readable = require('stream').Readable
+const {Readable} = require('stream')
-const binding = process.atomBinding('net')
-const {net, Net} = binding
-const {URLRequest} = net
 const {app} = require('electron')
+const {net, Net} = process.atomBinding('net')
+const {URLRequest} = net
 
 Object.setPrototypeOf(Net.prototype, EventEmitter.prototype)
 Object.setPrototypeOf(URLRequest.prototype, EventEmitter.prototype)
 
-let kSupportedProtocols = new Set()
+const kSupportedProtocols = new Set(['http:', 'https:'])
-kSupportedProtocols.add('http:')
-kSupportedProtocols.add('https:')
 
 class IncomingMessage extends Readable {
   constructor (urlRequest) {
@@ -82,8 +79,8 @@ class IncomingMessage extends Readable {
 
 }
 
-URLRequest.prototype._emitRequestEvent = function (async, ...rest) {
+URLRequest.prototype._emitRequestEvent = function (isAsync, ...rest) {
-  if (async) {
+  if (isAsync) {
     process.nextTick(() => {
       this.clientRequest.emit.apply(this.clientRequest, rest)
     })
@@ -92,8 +89,8 @@ URLRequest.prototype._emitRequestEvent = function (async, ...rest) {
   }
 }
 
-URLRequest.prototype._emitResponseEvent = function (async, ...rest) {
+URLRequest.prototype._emitResponseEvent = function (isAsync, ...rest) {
-  if (async) {
+  if (isAsync) {
     process.nextTick(() => {
       this._response.emit.apply(this._response, rest)
     })
@@ -175,9 +172,7 @@ class ClientRequest extends EventEmitter {
     this.extraHeaders = {}
 
     if (options.headers) {
-      const keys = Object.keys(options.headers)
+      for (let key in options.headers) {
-      for (let i = 0, l = keys.length; i < l; i++) {
-        const key = keys[i]
         this.setHeader(key, options.headers[key])
       }
     }

spec/api-net-spec.js

@@ -1179,7 +1179,8 @@ describe('net module', function () {
         const {net} = require('electron')
         const urlRequest = net.request('${server.url}${requestUrl}')
         process.nextTick(function () {
-          net._RequestGarbageCollectionForTesting()
+          const v8Util = process.atomBinding('v8_util')
+          v8Util.requestGarbageCollectionForTesting()
           event.sender.send('api-net-spec-done')
         })
       `)
@@ -1217,7 +1218,8 @@ describe('net module', function () {
           })
           process.nextTick(function () {
             // Trigger a garbage collection.
-            net._RequestGarbageCollectionForTesting()
+            const v8Util = process.atomBinding('v8_util')
+            v8Util.requestGarbageCollectionForTesting()
             event.sender.send('api-net-spec-resume')
           })
         })
@@ -1251,7 +1253,8 @@ describe('net module', function () {
         })
         urlRequest.on('close', function () {
           process.nextTick(function () {
-            net._RequestGarbageCollectionForTesting()
+            const v8Util = process.atomBinding('v8_util')
+            v8Util.requestGarbageCollectionForTesting()
             event.sender.send('api-net-spec-done')
           })
         })