From 61278f9aceaca9454c7aaa1d4d78222f17f64de7 Mon Sep 17 00:00:00 2001 From: "ali.ibrahim" Date: Tue, 25 Oct 2016 12:41:01 +0200 Subject: [PATCH] Fixing code review issues. --- atom/browser/api/atom_api_net.cc | 11 +-- atom/browser/api/atom_api_net.h | 3 +- atom/browser/api/atom_api_url_request.cc | 75 +++++++++++++------ atom/browser/api/atom_api_url_request.h | 52 +++---------- atom/browser/api/atom_api_web_contents.cc | 4 +- atom/browser/net/atom_url_request.cc | 2 +- atom/browser/net/atom_url_request.h | 2 +- atom/common/api/atom_api_v8_util.cc | 7 ++ .../native_mate_converters/net_converter.cc | 5 +- .../native_mate_converters/net_converter.h | 9 +-- docs/api/net.md | 12 ++- filenames.gypi | 4 +- lib/browser/api/net.js | 23 +++--- spec/api-net-spec.js | 9 ++- 14 files changed, 106 insertions(+), 112 deletions(-) diff --git a/atom/browser/api/atom_api_net.cc b/atom/browser/api/atom_api_net.cc index dac8d9ee146..24008ed7aee 100644 --- a/atom/browser/api/atom_api_net.cc +++ b/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 prototype) { prototype->SetClassName(mate::StringToV8(isolate, "Net")); mate::ObjectTemplateBuilder(isolate, prototype->PrototypeTemplate()) - .SetProperty("URLRequest", &Net::URLRequest) - .SetMethod("_RequestGarbageCollectionForTesting", - &Net::RequestGarbageCollectionForTesting); + .SetProperty("URLRequest", &Net::URLRequest); } v8::Local Net::URLRequest(v8::Isolate* isolate) { return URLRequest::GetConstructor(isolate)->GetFunction(); } -void Net::RequestGarbageCollectionForTesting() { - isolate()->RequestGarbageCollectionForTesting( - v8::Isolate::GarbageCollectionType::kFullGarbageCollection); -} - } // namespace api } // namespace atom diff --git a/atom/browser/api/atom_api_net.h b/atom/browser/api/atom_api_net.h index 0b86931978a..2a0fa4140cc 100644 --- a/atom/browser/api/atom_api_net.h +++ b/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() override; private: - void RequestGarbageCollectionForTesting(); DISALLOW_COPY_AND_ASSIGN(Net); }; diff --git a/atom/browser/api/atom_api_url_request.cc b/atom/browser/api/atom_api_url_request.cc index 42f08c8f05e..fc5c0536485 100644 --- a/atom/browser/api/atom_api_url_request.cc +++ b/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> { return false; } - auto io_buffer = new net::IOBufferWithSize(size); - if (!io_buffer) { - // Assuming allocation failed. - return false; - } - + *out = new net::IOBufferWithSize(size); // 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); - *out = io_buffer; + memcpy((*out)->data(), data, size); return true; } }; } // namespace mate +namespace { + +template +std::array, sizeof...(ArgTypes)> BuildArgsArray( + v8::Isolate* isolate, + ArgTypes... args) { + std::array, sizeof...(ArgTypes)> result = { + {mate::ConvertToV8(isolate, args)...}}; + return result; +} + +template +void EmitRequestEvent(v8::Isolate* isolate, + v8::Local object, + ArgTypes... args) { + v8::HandleScope handle_scope(isolate); + auto arguments = BuildArgsArray(isolate, args...); + v8::Local _emitRequestEvent; + if (mate::Dictionary(isolate, object) + .Get("_emitRequestEvent", &_emitRequestEvent)) + _emitRequestEvent->Call(object, arguments.size(), arguments.data()); +} + +template +void EmitResponseEvent(v8::Isolate* isolate, + v8::Local object, + ArgTypes... args) { + v8::HandleScope handle_scope(isolate); + auto arguments = BuildArgsArray(isolate, args...); + v8::Local _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 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 URLRequest::RawResponseHeaders() const { - return response_headers_; +net::HttpResponseHeaders* URLRequest::RawResponseHeaders() const { + 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(); } diff --git a/atom/browser/api/atom_api_url_request.h b/atom/browser/api/atom_api_url_request.h index f59f8bc72f4..935b9df3afe 100644 --- a/atom/browser/api/atom_api_url_request.h +++ b/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 { int StatusCode() const; std::string StatusMessage() const; - scoped_refptr RawResponseHeaders() const; + net::HttpResponseHeaders* RawResponseHeaders() const; uint32_t ResponseHttpVersionMajor() const; uint32_t ResponseHttpVersionMinor() const; - template - std::array, sizeof...(ArgTypes)> BuildArgsArray( - ArgTypes... args) const; + // template + // std::array, sizeof...(ArgTypes)> BuildArgsArray( + // ArgTypes... args) const; - template - void EmitRequestEvent(ArgTypes... args); + // template + // void EmitRequestEvent(ArgTypes... args); - template - void EmitResponseEvent(ArgTypes... args); + // template + // void EmitResponseEvent(ArgTypes... args); void Close(); - void pin(); - void unpin(); + void Pin(); + void Unpin(); scoped_refptr atom_request_; RequestState request_state_; @@ -205,36 +205,6 @@ class URLRequest : public mate::EventEmitter { DISALLOW_COPY_AND_ASSIGN(URLRequest); }; -template -std::array, sizeof...(ArgTypes)> -URLRequest::BuildArgsArray(ArgTypes... args) const { - std::array, sizeof...(ArgTypes)> result = { - {mate::ConvertToV8(isolate(), args)...}}; - return result; -} - -template -void URLRequest::EmitRequestEvent(ArgTypes... args) { - v8::HandleScope handle_scope(isolate()); - auto arguments = BuildArgsArray(args...); - v8::Local _emitRequestEvent; - auto wrapper = GetWrapper(); - if (mate::Dictionary(isolate(), wrapper) - .Get("_emitRequestEvent", &_emitRequestEvent)) - _emitRequestEvent->Call(wrapper, arguments.size(), arguments.data()); -} - -template -void URLRequest::EmitResponseEvent(ArgTypes... args) { - v8::HandleScope handle_scope(isolate()); - auto arguments = BuildArgsArray(args...); - v8::Local _emitResponseEvent; - auto wrapper = GetWrapper(); - if (mate::Dictionary(isolate(), wrapper) - .Get("_emitResponseEvent", &_emitResponseEvent)) - _emitResponseEvent->Call(wrapper, arguments.size(), arguments.data()); -} - } // namespace api } // namespace atom diff --git a/atom/browser/api/atom_api_web_contents.cc b/atom/browser/api/atom_api_web_contents.cc index bd7de5a22a2..a1dc5203b68 100644 --- a/atom/browser/api/atom_api_web_contents.cc +++ b/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( diff --git a/atom/browser/net/atom_url_request.cc b/atom/browser/net/atom_url_request.cc index 576036819f8..5d46beac808 100644 --- a/atom/browser/net/atom_url_request.cc +++ b/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. diff --git a/atom/browser/net/atom_url_request.h b/atom/browser/net/atom_url_request.h index 3efccae50f0..27cdbb5b3c3 100644 --- a/atom/browser/net/atom_url_request.h +++ b/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. diff --git a/atom/common/api/atom_api_v8_util.cc b/atom/common/api/atom_api_v8_util.cc index ddacbb08089..a587cd772a0 100644 --- a/atom/common/api/atom_api_v8_util.cc +++ b/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 exports, v8::Local unused, v8::Local context, void* priv) { mate::Dictionary dict(context->GetIsolate(), exports); @@ -105,6 +110,8 @@ void Initialize(v8::Local exports, v8::Local unused, dict.SetMethod("createIDWeakMap", &atom::api::KeyWeakMap::Create); dict.SetMethod("createDoubleIDWeakMap", &atom::api::KeyWeakMap>::Create); + dict.SetMethod("requestGarbageCollectionForTesting", + &RequestGarbageCollectionForTesting); } } // namespace diff --git a/atom/common/native_mate_converters/net_converter.cc b/atom/common/native_mate_converters/net_converter.cc index 89120c94ab5..00a06566a29 100644 --- a/atom/common/native_mate_converters/net_converter.cc +++ b/atom/common/native_mate_converters/net_converter.cc @@ -60,10 +60,9 @@ v8::Local Converter>::ToV8( } // static -v8::Local -Converter>::ToV8( +v8::Local Converter::ToV8( v8::Isolate* isolate, - scoped_refptr headers) { + net::HttpResponseHeaders* headers) { base::DictionaryValue response_headers; if (headers) { size_t iter = 0; diff --git a/atom/common/native_mate_converters/net_converter.h b/atom/common/native_mate_converters/net_converter.h index e3ba6b60cc9..16013e34f98 100644 --- a/atom/common/native_mate_converters/net_converter.h +++ b/atom/common/native_mate_converters/net_converter.h @@ -34,11 +34,10 @@ struct Converter> { const scoped_refptr& val); }; -template<> -struct Converter> { - static v8::Local ToV8( - v8::Isolate* isolate, - scoped_refptr headers); +template <> +struct Converter { + static v8::Local ToV8(v8::Isolate* isolate, + net::HttpResponseHeaders* headers); }; } // namespace mate diff --git a/docs/api/net.md b/docs/api/net.md index 0fab893c567..cf7a456e5a0 100644 --- a/docs/api/net.md +++ b/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. - - - - - - - diff --git a/filenames.gypi b/filenames.gypi index 20375c6f4e5..1834a9949ea 100644 --- a/filenames.gypi +++ b/filenames.gypi @@ -23,7 +23,7 @@ 'lib/browser/api/menu-item.js', 'lib/browser/api/menu-item-roles.js', 'lib/browser/api/navigation-controller.js', - 'lib/browser/api/net.js', + 'lib/browser/api/net.js', 'lib/browser/api/power-monitor.js', 'lib/browser/api/power-save-blocker.js', 'lib/browser/api/protocol.js', @@ -238,7 +238,7 @@ 'atom/browser/net/atom_network_delegate.h', 'atom/browser/net/atom_ssl_config_service.cc', 'atom/browser/net/atom_ssl_config_service.h', - 'atom/browser/net/atom_url_request.cc', + 'atom/browser/net/atom_url_request.cc', 'atom/browser/net/atom_url_request.h', 'atom/browser/net/atom_url_request_job_factory.cc', 'atom/browser/net/atom_url_request_job_factory.h', diff --git a/lib/browser/api/net.js b/lib/browser/api/net.js index 7fb2c61495e..1cb285184ae 100644 --- a/lib/browser/api/net.js +++ b/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 binding = process.atomBinding('net') -const {net, Net} = binding -const {URLRequest} = net +const {Readable} = require('stream') 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() -kSupportedProtocols.add('http:') -kSupportedProtocols.add('https:') +const kSupportedProtocols = new Set(['http:', 'https:']) class IncomingMessage extends Readable { constructor (urlRequest) { @@ -82,8 +79,8 @@ class IncomingMessage extends Readable { } -URLRequest.prototype._emitRequestEvent = function (async, ...rest) { - if (async) { +URLRequest.prototype._emitRequestEvent = function (isAsync, ...rest) { + 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) { - if (async) { +URLRequest.prototype._emitResponseEvent = function (isAsync, ...rest) { + 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 i = 0, l = keys.length; i < l; i++) { - const key = keys[i] + for (let key in options.headers) { this.setHeader(key, options.headers[key]) } } diff --git a/spec/api-net-spec.js b/spec/api-net-spec.js index fa73d7722cc..08ec337ecb1 100644 --- a/spec/api-net-spec.js +++ b/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') }) })