Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: ginify session.netLog #22732

Merged
merged 1 commit into from
Mar 18, 2020
Merged

refactor: ginify session.netLog #22732

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 2 additions & 6 deletions docs/api/net-log.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,16 +40,12 @@ Starts recording network events to `path`.

### `netLog.stopLogging()`

Returns `Promise<String>` - resolves with a file path to which network logs were recorded.
Returns `Promise<void>` - resolves when the net log has been flushed to disk.

Stops recording network events. If not called, net logging will automatically end when app quits.

## Properties

### `netLog.currentlyLogging` _Readonly_

A `Boolean` property that indicates whether network logs are recorded.

### `netLog.currentlyLoggingPath` _Readonly_ _Deprecated_

A `String` property that returns the path to the current log file.
A `Boolean` property that indicates whether network logs are currently being recorded.
31 changes: 1 addition & 30 deletions lib/browser/api/session.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

const { EventEmitter } = require('events')
const { app, deprecate } = require('electron')
const { fromPartition, Session, Cookies, NetLog, Protocol, ServiceWorkerContext } = process.electronBinding('session')
const { fromPartition, Session, Cookies, Protocol, ServiceWorkerContext } = process.electronBinding('session')

// Public API.
Object.defineProperties(exports, {
Expand All @@ -23,32 +23,3 @@ Object.setPrototypeOf(Session.prototype, EventEmitter.prototype)
Session.prototype._init = function () {
app.emit('session-created', this)
}

const _originalStartLogging = NetLog.prototype.startLogging
NetLog.prototype.startLogging = function (path, ...args) {
this._currentlyLoggingPath = path
try {
return _originalStartLogging.call(this, path, ...args)
} catch (e) {
this._currentlyLoggingPath = null
throw e
}
}

const _originalStopLogging = NetLog.prototype.stopLogging
NetLog.prototype.stopLogging = function () {
const logPath = this._currentlyLoggingPath
this._currentlyLoggingPath = null
return _originalStopLogging.call(this).then(() => logPath)
}

const currentlyLoggingPathDeprecated = deprecate.warnOnce('currentlyLoggingPath')
Object.defineProperties(NetLog.prototype, {
currentlyLoggingPath: {
enumerable: true,
get () {
currentlyLoggingPathDeprecated()
return this._currentlyLoggingPath == null ? '' : this._currentlyLoggingPath
}
}
})
43 changes: 23 additions & 20 deletions shell/browser/api/electron_api_net_log.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,11 @@
#include "components/net_log/chrome_net_log.h"
#include "content/public/browser/storage_partition.h"
#include "electron/electron_version.h"
#include "gin/object_template_builder.h"
#include "shell/browser/electron_browser_context.h"
#include "shell/browser/net/system_network_context_manager.h"
#include "shell/common/gin_converters/file_path_converter.h"
#include "shell/common/gin_helper/dictionary.h"
#include "shell/common/gin_helper/object_template_builder.h"
#include "shell/common/node_includes.h"

namespace gin {
Expand Down Expand Up @@ -76,18 +76,19 @@ void ResolvePromiseWithNetError(gin_helper::Promise<void> promise,

namespace api {

gin::WrapperInfo NetLog::kWrapperInfo = {gin::kEmbedderNativeGin};

NetLog::NetLog(v8::Isolate* isolate, ElectronBrowserContext* browser_context)
: browser_context_(browser_context), weak_ptr_factory_(this) {
Init(isolate);
file_task_runner_ = CreateFileTaskRunner();
}

NetLog::~NetLog() = default;

v8::Local<v8::Promise> NetLog::StartLogging(base::FilePath log_path,
gin_helper::Arguments* args) {
gin::Arguments* args) {
if (log_path.empty()) {
args->ThrowError("The first parameter must be a valid string");
args->ThrowTypeError("The first parameter must be a valid string");
return v8::Local<v8::Promise>();
}

Expand All @@ -100,27 +101,27 @@ v8::Local<v8::Promise> NetLog::StartLogging(base::FilePath log_path,
if (dict.Get("captureMode", &capture_mode_v8)) {
if (!gin::ConvertFromV8(args->isolate(), capture_mode_v8,
&capture_mode)) {
args->ThrowError("Invalid value for captureMode");
args->ThrowTypeError("Invalid value for captureMode");
return v8::Local<v8::Promise>();
}
}
v8::Local<v8::Value> max_file_size_v8;
if (dict.Get("maxFileSize", &max_file_size_v8)) {
if (!gin::ConvertFromV8(args->isolate(), max_file_size_v8,
&max_file_size)) {
args->ThrowError("Invalid value for maxFileSize");
args->ThrowTypeError("Invalid value for maxFileSize");
return v8::Local<v8::Promise>();
}
}
}

if (net_log_exporter_) {
args->ThrowError("There is already a net log running");
args->ThrowTypeError("There is already a net log running");
return v8::Local<v8::Promise>();
}

pending_start_promise_ =
base::make_optional<gin_helper::Promise<void>>(isolate());
base::make_optional<gin_helper::Promise<void>>(args->isolate());
v8::Local<v8::Promise> handle = pending_start_promise_->GetHandle();

auto command_line_string =
Expand Down Expand Up @@ -189,8 +190,8 @@ bool NetLog::IsCurrentlyLogging() const {
return !!net_log_exporter_;
}

v8::Local<v8::Promise> NetLog::StopLogging(gin_helper::Arguments* args) {
gin_helper::Promise<void> promise(isolate());
v8::Local<v8::Promise> NetLog::StopLogging(gin::Arguments* args) {
gin_helper::Promise<void> promise(args->isolate());
v8::Local<v8::Promise> handle = promise.GetHandle();

if (net_log_exporter_) {
Expand All @@ -212,22 +213,24 @@ v8::Local<v8::Promise> NetLog::StopLogging(gin_helper::Arguments* args) {
return handle;
}

gin::ObjectTemplateBuilder NetLog::GetObjectTemplateBuilder(
v8::Isolate* isolate) {
return gin::Wrappable<NetLog>::GetObjectTemplateBuilder(isolate)
.SetProperty("currentlyLogging", &NetLog::IsCurrentlyLogging)
.SetMethod("startLogging", &NetLog::StartLogging)
.SetMethod("stopLogging", &NetLog::StopLogging);
}

const char* NetLog::GetTypeName() {
return "NetLog";
}

// static
gin::Handle<NetLog> NetLog::Create(v8::Isolate* isolate,
ElectronBrowserContext* browser_context) {
return gin::CreateHandle(isolate, new NetLog(isolate, browser_context));
}

// static
void NetLog::BuildPrototype(v8::Isolate* isolate,
v8::Local<v8::FunctionTemplate> prototype) {
prototype->SetClassName(gin::StringToV8(isolate, "NetLog"));
gin_helper::ObjectTemplateBuilder(isolate, prototype->PrototypeTemplate())
.SetProperty("currentlyLogging", &NetLog::IsCurrentlyLogging)
.SetMethod("startLogging", &NetLog::StartLogging)
.SetMethod("stopLogging", &NetLog::StopLogging);
}

} // namespace api

} // namespace electron
21 changes: 14 additions & 7 deletions shell/browser/api/electron_api_net_log.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,29 +13,36 @@
#include "base/optional.h"
#include "base/values.h"
#include "gin/handle.h"
#include "gin/wrappable.h"
#include "services/network/public/mojom/net_log.mojom.h"
#include "shell/common/gin_helper/promise.h"
#include "shell/common/gin_helper/trackable_object.h"

namespace gin {
class Arguments;
}

namespace electron {

class ElectronBrowserContext;

namespace api {

class NetLog : public gin_helper::TrackableObject<NetLog> {
class NetLog : public gin::Wrappable<NetLog> {
public:
static gin::Handle<NetLog> Create(v8::Isolate* isolate,
ElectronBrowserContext* browser_context);

static void BuildPrototype(v8::Isolate* isolate,
v8::Local<v8::FunctionTemplate> prototype);

v8::Local<v8::Promise> StartLogging(base::FilePath log_path,
gin_helper::Arguments* args);
v8::Local<v8::Promise> StopLogging(gin_helper::Arguments* args);
gin::Arguments* args);
v8::Local<v8::Promise> StopLogging(gin::Arguments* args);
bool IsCurrentlyLogging() const;

// gin::Wrappable
static gin::WrapperInfo kWrapperInfo;
gin::ObjectTemplateBuilder GetObjectTemplateBuilder(
v8::Isolate* isolate) override;
const char* GetTypeName() override;

protected:
explicit NetLog(v8::Isolate* isolate,
ElectronBrowserContext* browser_context);
Expand Down
5 changes: 0 additions & 5 deletions shell/browser/api/electron_api_session.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,6 @@ Session::~Session() {
// Refs https://github.com/electron/electron/pull/12305.
DestroyGlobalHandle(isolate(), cookies_);
DestroyGlobalHandle(isolate(), protocol_);
DestroyGlobalHandle(isolate(), net_log_);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NB. this is no longer needed because the object will be GC'd by V8.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some session properties hold scoped_refptr to ElectronBrowserContext while some hold a weak-reference, we should ensure ElectronBrowserContext is released when session is destroyed and no other reference exists, otherwise bad things can happen on shutdown.

In the net_log case it is fine.

g_sessions.erase(weak_map_id());
}

Expand Down Expand Up @@ -1029,7 +1028,6 @@ void Session::BuildPrototype(v8::Isolate* isolate,
namespace {

using electron::api::Cookies;
using electron::api::NetLog;
using electron::api::Protocol;
using electron::api::ServiceWorkerContext;
using electron::api::Session;
Expand Down Expand Up @@ -1058,9 +1056,6 @@ void Initialize(v8::Local<v8::Object> exports,
dict.Set(
"Cookies",
Cookies::GetConstructor(isolate)->GetFunction(context).ToLocalChecked());
dict.Set(
"NetLog",
NetLog::GetConstructor(isolate)->GetFunction(context).ToLocalChecked());
dict.Set(
"Protocol",
Protocol::GetConstructor(isolate)->GetFunction(context).ToLocalChecked());
Expand Down
5 changes: 1 addition & 4 deletions spec-main/api-net-log-spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,7 @@ describe('netLog module', () => {

expect(testNetLog().currentlyLogging).to.be.true('currently logging')

expect(testNetLog().currentlyLoggingPath).to.equal(dumpFileDynamic)

const path = await testNetLog().stopLogging()
expect(path).to.equal(dumpFileDynamic)
await testNetLog().stopLogging()

expect(fs.existsSync(dumpFileDynamic)).to.be.true('currently logging')
})
Expand Down