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
electron · Jan 31, 2019 · 1b8feaf · 1b8feaf
1 parent d6612d2
commit 1b8feaf
Show file tree

Hide file tree

Showing 5 changed files with 158 additions and 21 deletions.
diff --git a/atom/browser/api/atom_api_content_tracing.cc b/atom/browser/api/atom_api_content_tracing.cc
@@ -65,10 +65,19 @@ scoped_refptr<TracingController::TraceDataEndpoint> GetTraceDataEndpoint(
       result_file_path, base::Bind(callback, result_file_path));
 }
 
-void StopRecording(const base::FilePath& path,
-                   const CompletionCallback& callback) {
+void OnRecordingStopped(scoped_refptr<atom::util::Promise> promise,
+                        const base::FilePath& path) {
+  promise->Resolve(path);
+}
+
+v8::Local<v8::Promise> StopRecording(v8::Isolate* isolate,
+                                     const base::FilePath& path) {
+  scoped_refptr<atom::util::Promise> 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 +87,22 @@ bool GetCategories(
       base::BindOnce(callback));
 }
 
-bool StartTracing(const base::trace_event::TraceConfig& trace_config,
-                  const base::RepeatingCallback<void()>& callback) {
-  return TracingController::GetInstance()->StartTracing(
-      trace_config, base::BindOnce(callback));
+void OnTracingStarted(scoped_refptr<atom::util::Promise> promise) {
+  promise->Resolve();
+}
+
+v8::Local<v8::Promise> StartTracing(
+    v8::Isolate* isolate,
+    const base::trace_event::TraceConfig& trace_config) {
+  scoped_refptr<atom::util::Promise> 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
@@ -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<void>` - 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<String>` - 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
@@ -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)
diff --git 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
@@ -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 */ '')