From db01fae4ac2fe9ab96078399ba6ffeb11a599c22 Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Wed, 30 Jan 2019 18:53:55 -0800 Subject: [PATCH] feat: promisify contentTracing recording APIs (#16584) * feat: promisify contentTracing.startRecording() * feat: promisify contentTracing.stopRecording() * test: convert specs for new promisified apis * chore: deprecate and ensure legacy tests work --- atom/browser/api/atom_api_content_tracing.cc | 36 +++++-- docs/api/content-tracing.md | 33 ++++++- docs/api/promisification.md | 5 +- lib/browser/api/content-tracing.js | 8 +- spec/api-content-tracing-spec.js | 98 ++++++++++++++++++-- 5 files changed, 159 insertions(+), 21 deletions(-) diff --git a/atom/browser/api/atom_api_content_tracing.cc b/atom/browser/api/atom_api_content_tracing.cc index 46eb1cac2c2d6..dcabedfe60416 100644 --- a/atom/browser/api/atom_api_content_tracing.cc +++ b/atom/browser/api/atom_api_content_tracing.cc @@ -8,6 +8,7 @@ #include "atom/common/native_mate_converters/callback.h" #include "atom/common/native_mate_converters/file_path_converter.h" #include "atom/common/native_mate_converters/value_converter.h" +#include "atom/common/promise_util.h" #include "base/bind.h" #include "base/files/file_util.h" #include "content/public/browser/tracing_controller.h" @@ -65,10 +66,19 @@ scoped_refptr GetTraceDataEndpoint( result_file_path, base::Bind(callback, result_file_path)); } -void StopRecording(const base::FilePath& path, - const CompletionCallback& callback) { +void OnRecordingStopped(scoped_refptr promise, + const base::FilePath& path) { + promise->Resolve(path); +} + +v8::Local StopRecording(v8::Isolate* isolate, + const base::FilePath& path) { + scoped_refptr promise = new atom::util::Promise(isolate); + TracingController::GetInstance()->StopTracing( - GetTraceDataEndpoint(path, callback)); + GetTraceDataEndpoint(path, base::Bind(&OnRecordingStopped, promise))); + + return promise->GetHandle(); } bool GetCategories( @@ -78,10 +88,22 @@ bool GetCategories( base::BindOnce(callback)); } -bool StartTracing(const base::trace_event::TraceConfig& trace_config, - const base::RepeatingCallback& callback) { - return TracingController::GetInstance()->StartTracing( - trace_config, base::BindOnce(callback)); +void OnTracingStarted(scoped_refptr promise) { + promise->Resolve(); +} + +v8::Local StartTracing( + v8::Isolate* isolate, + const base::trace_event::TraceConfig& trace_config) { + scoped_refptr promise = new atom::util::Promise(isolate); + + bool success = TracingController::GetInstance()->StartTracing( + trace_config, base::BindOnce(&OnTracingStarted, promise)); + + if (!success) + promise->RejectWithErrorMessage("Could not start tracing"); + + return promise->GetHandle(); } bool GetTraceBufferUsage( diff --git a/docs/api/content-tracing.md b/docs/api/content-tracing.md index 6d1729689b02b..ac07711bb73fd 100644 --- a/docs/api/content-tracing.md +++ b/docs/api/content-tracing.md @@ -12,7 +12,6 @@ result. **Note:** You should not use this module until the `ready` event of the app module is emitted. - ```javascript const { app, contentTracing } = require('electron') @@ -60,6 +59,19 @@ Recording begins immediately locally and asynchronously on child processes as soon as they receive the EnableRecording request. The `callback` will be called once all child processes have acknowledged the `startRecording` request. +**[Deprecated Soon](promisification.md)** + +### `contentTracing.startRecording(options)` + +* `options` ([TraceCategoriesAndOptions](structures/trace-categories-and-options.md) | [TraceConfig](structures/trace-config.md)) + +Returns `Promise` - resolved once all child processes have acknowledged the `startRecording` request. + +Start recording on all processes. + +Recording begins immediately locally and asynchronously on child processes +as soon as they receive the EnableRecording request. + ### `contentTracing.stopRecording(resultFilePath, callback)` * `resultFilePath` String @@ -81,6 +93,25 @@ Trace data will be written into `resultFilePath` if it is not empty or into a temporary file. The actual file path will be passed to `callback` if it's not `null`. +**[Deprecated Soon](promisification.md)** + +### `contentTracing.stopRecording(resultFilePath)` + +* `resultFilePath` String + +Returns `Promise` - resolves with a file that contains the traced data once all child processes have acknowledged the `stopRecording` request + +Stop recording on all processes. + +Child processes typically cache trace data and only rarely flush and send +trace data back to the main process. This helps to minimize the runtime overhead +of tracing since sending trace data over IPC can be an expensive operation. So, +to end tracing, we must asynchronously ask all child processes to flush any +pending trace data. + +Trace data will be written into `resultFilePath` if it is not empty or into a +temporary file. + ### `contentTracing.getTraceBufferUsage(callback)` * `callback` Function diff --git a/docs/api/promisification.md b/docs/api/promisification.md index 1e79b55eb62de..c54624f23afb0 100644 --- a/docs/api/promisification.md +++ b/docs/api/promisification.md @@ -12,8 +12,6 @@ When a majority of affected functions are migrated, this flag will be enabled by - [request.write(chunk[, encoding][, callback])](https://github.com/electron/electron/blob/master/docs/api/client-request.md#write) - [request.end([chunk][, encoding][, callback])](https://github.com/electron/electron/blob/master/docs/api/client-request.md#end) - [contentTracing.getCategories(callback)](https://github.com/electron/electron/blob/master/docs/api/content-tracing.md#getCategories) -- [contentTracing.startRecording(options, callback)](https://github.com/electron/electron/blob/master/docs/api/content-tracing.md#startRecording) -- [contentTracing.stopRecording(resultFilePath, callback)](https://github.com/electron/electron/blob/master/docs/api/content-tracing.md#stopRecording) - [contentTracing.getTraceBufferUsage(callback)](https://github.com/electron/electron/blob/master/docs/api/content-tracing.md#getTraceBufferUsage) - [cookies.get(filter, callback)](https://github.com/electron/electron/blob/master/docs/api/cookies.md#get) - [cookies.set(details, callback)](https://github.com/electron/electron/blob/master/docs/api/cookies.md#set) @@ -53,6 +51,9 @@ When a majority of affected functions are migrated, this flag will be enabled by - [webviewTag.capturePage([rect, ]callback)](https://github.com/electron/electron/blob/master/docs/api/webview-tag.md#capturePage) - [contents.capturePage([rect, ]callback)](https://github.com/electron/electron/blob/master/docs/api/web-contents.md#capturePage) - [app.getFileIcon(path[, options], callback)](https://github.com/electron/electron/blob/master/docs/api/app.md#getFileIcon) +- [contentTracing.startRecording(options, callback)](https://github.com/electron/electron/blob/master/docs/api/content-tracing.md#startRecording) +- [contentTracing.stopRecording(resultFilePath, callback)](https://github.com/electron/electron/blob/master/docs/api/content-tracing.md#stopRecording) +- [desktopCapturer.getSources(options, callback)](https://github.com/electron/electron/blob/master/docs/api/desktop-capturer.md#getSources) - [shell.openExternal(url[, options, callback])](https://github.com/electron/electron/blob/master/docs/api/shell.md#openExternal) - [protocol.isProtocolHandled(scheme, callback)](https://github.com/electron/electron/blob/master/docs/api/protocol.md#isProtocolHandled) - [desktopCapturer.getSources(options, callback)](https://github.com/electron/electron/blob/master/docs/api/desktop-capturer.md#getSources) \ No newline at end of file diff --git a/lib/browser/api/content-tracing.js b/lib/browser/api/content-tracing.js index 81bd70f1ebbfb..931ec2ce30810 100644 --- a/lib/browser/api/content-tracing.js +++ b/lib/browser/api/content-tracing.js @@ -1,3 +1,9 @@ 'use strict' -module.exports = process.atomBinding('content_tracing') +const { deprecate } = require('electron') +const contentTracing = process.atomBinding('content_tracing') + +contentTracing.startRecording = deprecate.promisify(contentTracing.startRecording) +contentTracing.stopRecording = deprecate.promisify(contentTracing.stopRecording) + +module.exports = contentTracing diff --git a/spec/api-content-tracing-spec.js b/spec/api-content-tracing-spec.js index 68ff6c7a970da..0d6e65c31ecca 100644 --- a/spec/api-content-tracing-spec.js +++ b/spec/api-content-tracing-spec.js @@ -28,6 +28,28 @@ describe('contentTracing', () => { } }) + const record = async (options, outputFilePath, recordTimeInMilliseconds = 1e3) => { + await app.whenReady() + + await contentTracing.startRecording(options) + await timeout(recordTimeInMilliseconds) + const resultFilePath = await contentTracing.stopRecording(outputFilePath) + + return resultFilePath + } + + // TODO(codebytere): remove when promisification is complete + const recordCallback = async (options, outputFilePath, recordTimeInMilliseconds = 1e3) => { + await app.whenReady() + + await startRecording(options) + await timeout(recordTimeInMilliseconds) + const resultFilePath = await stopRecording(outputFilePath) + + return resultFilePath + } + + // TODO(codebytere): remove when promisification is complete const startRecording = async (options) => { return new Promise((resolve) => { contentTracing.startRecording(options, () => { @@ -36,6 +58,7 @@ describe('contentTracing', () => { }) } + // TODO(codebytere): remove when promisification is complete const stopRecording = async (filePath) => { return new Promise((resolve) => { contentTracing.stopRecording(filePath, (resultFilePath) => { @@ -44,16 +67,6 @@ describe('contentTracing', () => { }) } - const record = async (options, outputFilePath, recordTimeInMilliseconds = 1e3) => { - await app.whenReady() - - await startRecording(options) - await timeout(recordTimeInMilliseconds) - const resultFilePath = await stopRecording(outputFilePath) - - return resultFilePath - } - const outputFilePath = getPathInATempFolder('trace.json') beforeEach(() => { if (fs.existsSync(outputFilePath)) { @@ -82,6 +95,18 @@ describe('contentTracing', () => { `the trace output file is empty, check "${outputFilePath}"`) }) + // TODO(codebytere): remove when promisification is complete + it('accepts an empty config (callback)', async () => { + const config = {} + await recordCallback(config, outputFilePath) + + expect(fs.existsSync(outputFilePath)).to.be.true() + + const fileSizeInKiloBytes = getFileSizeInKiloBytes(outputFilePath) + expect(fileSizeInKiloBytes).to.be.above(0, + `the trace output file is empty, check "${outputFilePath}"`) + }) + it('accepts a trace config', async () => { // (alexeykuzmin): All categories are excluded on purpose, // so only metadata gets into the output file. @@ -104,6 +129,29 @@ describe('contentTracing', () => { check "${outputFilePath}"`) }) + // TODO(codebytere): remove when promisification is complete + it('accepts a trace config (callback)', async () => { + // (alexeykuzmin): All categories are excluded on purpose, + // so only metadata gets into the output file. + const config = { + excluded_categories: ['*'] + } + await recordCallback(config, outputFilePath) + + expect(fs.existsSync(outputFilePath)).to.be.true() + + // If the `excluded_categories` param above is not respected + // the file size will be above 50KB. + const fileSizeInKiloBytes = getFileSizeInKiloBytes(outputFilePath) + const expectedMaximumFileSize = 10 // Depends on a platform. + + expect(fileSizeInKiloBytes).to.be.above(0, + `the trace output file is empty, check "${outputFilePath}"`) + expect(fileSizeInKiloBytes).to.be.below(expectedMaximumFileSize, + `the trace output file is suspiciously large (${fileSizeInKiloBytes}KB), + check "${outputFilePath}"`) + }) + it('accepts "categoryFilter" and "traceOptions" as a config', async () => { // (alexeykuzmin): All categories are excluded on purpose, // so only metadata gets into the output file. @@ -126,6 +174,30 @@ describe('contentTracing', () => { `the trace output file is suspiciously large (${fileSizeInKiloBytes}KB), check "${outputFilePath}"`) }) + + // TODO(codebytere): remove when promisification is complete + it('accepts "categoryFilter" and "traceOptions" as a config (callback)', async () => { + // (alexeykuzmin): All categories are excluded on purpose, + // so only metadata gets into the output file. + const config = { + categoryFilter: '__ThisIsANonexistentCategory__', + traceOptions: '' + } + await recordCallback(config, outputFilePath) + + expect(fs.existsSync(outputFilePath)).to.be.true() + + // If the `categoryFilter` param above is not respected + // the file size will be above 50KB. + const fileSizeInKiloBytes = getFileSizeInKiloBytes(outputFilePath) + const expectedMaximumFileSize = 10 // Depends on a platform. + + expect(fileSizeInKiloBytes).to.be.above(0, + `the trace output file is empty, check "${outputFilePath}"`) + expect(fileSizeInKiloBytes).to.be.below(expectedMaximumFileSize, + `the trace output file is suspiciously large (${fileSizeInKiloBytes}KB), + check "${outputFilePath}"`) + }) }) describe('stopRecording', function () { @@ -136,6 +208,12 @@ describe('contentTracing', () => { expect(resultFilePath).to.be.a('string').and.be.equal(outputFilePath) }) + // TODO(codebytere): remove when promisification is complete + it('calls its callback with a result file path (callback)', async () => { + const resultFilePath = await recordCallback(/* options */ {}, outputFilePath) + expect(resultFilePath).to.be.a('string').and.be.equal(outputFilePath) + }) + // FIXME(alexeykuzmin): https://github.com/electron/electron/issues/16019 xit('creates a temporary file when an empty string is passed', async function () { const resultFilePath = await record(/* options */ {}, /* outputFilePath */ '')