feat: promisify debugger.sendCommand() (#16861)

* remove duplicate entry for desktopCapturer.getSources

* feat: promisify debugger.sendCommand

atom/browser/api/atom_api_debugger.cc

@@ -61,23 +61,26 @@ void Debugger::DispatchProtocolMessage(DevToolsAgentHost* agent_host,
       params.Swap(params_value);
     Emit("message", method, params);
   } else {
-    auto send_command_callback = pending_requests_[id];
-    pending_requests_.erase(id);
-    if (send_command_callback.is_null())
+    auto it = pending_requests_.find(id);
+    if (it == pending_requests_.end())
       return;
-    base::DictionaryValue* error_body = nullptr;
-    base::DictionaryValue error;
-    bool has_error;
-    if ((has_error = dict->GetDictionary("error", &error_body))) {
-      error.Swap(error_body);
-    }
 
+    scoped_refptr<atom::util::Promise> promise = it->second;
+    pending_requests_.erase(it);
+
+    base::DictionaryValue* error = nullptr;
+    if (dict->GetDictionary("error", &error)) {
+      std::string message;
+      error->GetString("message", &message);
+      promise->RejectWithErrorMessage(message);
+    } else {
       base::DictionaryValue* result_body = nullptr;
       base::DictionaryValue result;
-    if (dict->GetDictionary("result", &result_body))
+      if (dict->GetDictionary("result", &result_body)) {
         result.Swap(result_body);
-    send_command_callback.Run(has_error ? error.Clone() : base::Value(),
-                              result);
+      }
+      promise->Resolve(result);
+    }
   }
 }
 
@@ -125,23 +128,26 @@ void Debugger::Detach() {
   AgentHostClosed(agent_host_.get());
 }
 
-void Debugger::SendCommand(mate::Arguments* args) {
-  if (!agent_host_)
-    return;
+v8::Local<v8::Promise> Debugger::SendCommand(mate::Arguments* args) {
+  scoped_refptr<util::Promise> promise = new util::Promise(isolate());
+
+  if (!agent_host_) {
+    promise->RejectWithErrorMessage("No target available");
+    return promise->GetHandle();
+  }
 
   std::string method;
   if (!args->GetNext(&method)) {
-    args->ThrowError();
-    return;
+    promise->RejectWithErrorMessage("Invalid method");
+    return promise->GetHandle();
   }
+
   base::DictionaryValue command_params;
   args->GetNext(&command_params);
-  SendCommandCallback callback;
-  args->GetNext(&callback);
 
   base::DictionaryValue request;
   int request_id = ++previous_request_id_;
-  pending_requests_[request_id] = callback;
+  pending_requests_[request_id] = promise;
   request.SetInteger("id", request_id);
   request.SetString("method", method);
   if (!command_params.empty())
@@ -151,16 +157,13 @@ void Debugger::SendCommand(mate::Arguments* args) {
   std::string json_args;
   base::JSONWriter::Write(request, &json_args);
   agent_host_->DispatchProtocolMessage(this, json_args);
+
+  return promise->GetHandle();
 }
 
 void Debugger::ClearPendingRequests() {
-  if (pending_requests_.empty())
-    return;
-  base::Value error(base::Value::Type::DICTIONARY);
-  base::Value error_msg("target closed while handling command");
-  error.SetKey("message", std::move(error_msg));
   for (const auto& it : pending_requests_)
-    it.second.Run(error, base::Value());
+    it.second->RejectWithErrorMessage("target closed while handling command");
 }
 
 // static

atom/browser/api/atom_api_debugger.h

@@ -9,6 +9,7 @@
 #include <string>
 
 #include "atom/browser/api/trackable_object.h"
+#include "atom/common/promise_util.h"
 #include "base/callback.h"
 #include "base/values.h"
 #include "content/public/browser/devtools_agent_host_client.h"
@@ -32,9 +33,6 @@ class Debugger : public mate::TrackableObject<Debugger>,
                  public content::DevToolsAgentHostClient,
                  public content::WebContentsObserver {
  public:
-  using SendCommandCallback =
-      base::Callback<void(const base::Value&, const base::Value&)>;
-
   static mate::Handle<Debugger> Create(v8::Isolate* isolate,
                                        content::WebContents* web_contents);
 
@@ -56,12 +54,12 @@ class Debugger : public mate::TrackableObject<Debugger>,
                               content::RenderFrameHost* new_rfh) override;
 
  private:
-  using PendingRequestMap = std::map<int, SendCommandCallback>;
+  using PendingRequestMap = std::map<int, scoped_refptr<atom::util::Promise>>;
 
   void Attach(mate::Arguments* args);
   bool IsAttached();
   void Detach();
-  void SendCommand(mate::Arguments* args);
+  v8::Local<v8::Promise> SendCommand(mate::Arguments* args);
   void ClearPendingRequests();
 
   content::WebContents* web_contents_;  // Weak Reference.

docs/api/debugger.md

@@ -60,6 +60,20 @@ Detaches the debugger from the `webContents`.
 
 Send given command to the debugging target.
 
+**[Deprecated Soon](promisification.md)**
+
+#### `debugger.sendCommand(method[, commandParams])`
+
+* `method` String - Method name, should be one of the methods defined by the
+   [remote debugging protocol][rdp].
+* `commandParams` Object (optional) - JSON object with request parameters.
+
+Returns `Promise<any>` - A promise that resolves with the response defined by
+the 'returns' attribute of the command description in the remote debugging protocol
+or is rejected indicating the failure of the command.
+
+Send given command to the debugging target.
+
 ### Instance Events
 
 #### Event: 'detach'

docs/api/promisification.md

@@ -10,7 +10,6 @@ When a majority of affected functions are migrated, this flag will be enabled by
 
 - [app.importCertificate(options, callback)](https://github.com/electron/electron/blob/master/docs/api/app.md#importCertificate)
 - [contentTracing.getTraceBufferUsage(callback)](https://github.com/electron/electron/blob/master/docs/api/content-tracing.md#getTraceBufferUsage)
-- [debugger.sendCommand(method[, commandParams, callback])](https://github.com/electron/electron/blob/master/docs/api/debugger.md#sendCommand)
 - [dialog.showOpenDialog([browserWindow, ]options[, callback])](https://github.com/electron/electron/blob/master/docs/api/dialog.md#showOpenDialog)
 - [dialog.showSaveDialog([browserWindow, ]options[, callback])](https://github.com/electron/electron/blob/master/docs/api/dialog.md#showSaveDialog)
 - [dialog.showMessageBox([browserWindow, ]options[, callback])](https://github.com/electron/electron/blob/master/docs/api/dialog.md#showMessageBox)
@@ -45,10 +44,10 @@ When a majority of affected functions are migrated, this flag will be enabled by
 - [cookies.get(filter, callback)](https://github.com/electron/electron/blob/master/docs/api/cookies.md#get)
 - [cookies.remove(url, name, callback)](https://github.com/electron/electron/blob/master/docs/api/cookies.md#remove)
 - [cookies.set(details, callback)](https://github.com/electron/electron/blob/master/docs/api/cookies.md#set)
+- [debugger.sendCommand(method[, commandParams, callback])](https://github.com/electron/electron/blob/master/docs/api/debugger.md#sendCommand)
 - [desktopCapturer.getSources(options, callback)](https://github.com/electron/electron/blob/master/docs/api/desktop-capturer.md#getSources)
 - [protocol.isProtocolHandled(scheme, callback)](https://github.com/electron/electron/blob/master/docs/api/protocol.md#isProtocolHandled)
 - [shell.openExternal(url[, options, callback])](https://github.com/electron/electron/blob/master/docs/api/shell.md#openExternal)
 - [webviewTag.capturePage([rect, ]callback)](https://github.com/electron/electron/blob/master/docs/api/webview-tag.md#capturePage)
 - [webviewTag.printToPDF(options, callback)](https://github.com/electron/electron/blob/master/docs/api/webview-tag.md#printToPDF)
 - [win.capturePage([rect, ]callback)](https://github.com/electron/electron/blob/master/docs/api/browser-window.md#capturePage)
-- [desktopCapturer.getSources(options, callback)](https://github.com/electron/electron/blob/master/docs/api/desktop-capturer.md#getSources)

lib/browser/api/web-contents.js

@@ -483,6 +483,8 @@ WebContents.prototype._init = function () {
 // JavaScript wrapper of Debugger.
 const { Debugger } = process.atomBinding('debugger')
 
+Debugger.prototype.sendCommand = deprecate.promisify(Debugger.prototype.sendCommand)
+
 Object.setPrototypeOf(Debugger.prototype, EventEmitter.prototype)
 
 // Public APIs.

spec/api-debugger-spec.js

@@ -2,6 +2,7 @@ const chai = require('chai')
 const dirtyChai = require('dirty-chai')
 const http = require('http')
 const path = require('path')
+const { emittedOnce } = require('./events-helpers')
 const { closeWindow } = require('./window-helpers')
 const { BrowserWindow } = require('electron').remote
 
@@ -102,7 +103,21 @@ describe('debugger module', () => {
       }
     })
 
-    it('returns response', done => {
+    it('returns response', async () => {
+      w.webContents.loadURL('about:blank')
+      w.webContents.debugger.attach()
+
+      const params = { 'expression': '4+2' }
+      const res = await w.webContents.debugger.sendCommand('Runtime.evaluate', params)
+
+      expect(res.wasThrown).to.be.undefined()
+      expect(res.result.value).to.equal(6)
+
+      w.webContents.debugger.detach()
+    })
+
+    // TODO(miniak): remove when promisification is complete
+    it('returns response (callback)', done => {
       w.webContents.loadURL('about:blank')
       try {
         w.webContents.debugger.attach()
@@ -123,7 +138,24 @@ describe('debugger module', () => {
       w.webContents.debugger.sendCommand('Runtime.evaluate', params, callback)
     })
 
-    it('returns response when devtools is opened', done => {
+    it('returns response when devtools is opened', async () => {
+      w.webContents.loadURL('about:blank')
+      w.webContents.debugger.attach()
+
+      w.webContents.openDevTools()
+      await emittedOnce(w.webContents, 'devtools-opened')
+
+      const params = { 'expression': '4+2' }
+      const res = await w.webContents.debugger.sendCommand('Runtime.evaluate', params)
+
+      expect(res.wasThrown).to.be.undefined()
+      expect(res.result.value).to.equal(6)
+
+      w.webContents.debugger.detach()
+    })
+
+    // TODO(miniak): remove when promisification is complete
+    it('returns response when devtools is opened (callback)', done => {
       w.webContents.loadURL('about:blank')
       try {
         w.webContents.debugger.attach()
@@ -169,7 +201,18 @@ describe('debugger module', () => {
       w.webContents.debugger.sendCommand('Console.enable')
     })
 
-    it('returns error message when command fails', done => {
+    it('returns error message when command fails', async () => {
+      w.webContents.loadURL('about:blank')
+      w.webContents.debugger.attach()
+
+      const promise = w.webContents.debugger.sendCommand('Test')
+      await expect(promise).to.be.eventually.rejectedWith(Error, "'Test' wasn't found")
+
+      w.webContents.debugger.detach()
+    })
+
+    // TODO(miniak): remove when promisification is complete
+    it('returns error message when command fails (callback)', done => {
       w.webContents.loadURL('about:blank')
       try {
         w.webContents.debugger.attach()
@@ -177,9 +220,8 @@ describe('debugger module', () => {
         done(`unexpected error : ${err}`)
       }
 
-      w.webContents.debugger.sendCommand('Test', err => {
-        expect(err).to.not.be.null()
-        expect(err.message).to.equal("'Test' wasn't found")
+      w.webContents.debugger.sendCommand('Test', (err, res) => {
+        expect(err).to.be.an.instanceOf(Error).with.property('message', "'Test' wasn't found")
         w.webContents.debugger.detach()
         done()
       })

spec/chromium-spec.js

@@ -1515,11 +1515,7 @@ describe('font fallback', () => {
     try {
       await w.loadURL(`data:text/html,${html}`)
       w.webContents.debugger.attach()
-      const sendCommand = (...args) => new Promise((resolve, reject) => {
-        w.webContents.debugger.sendCommand(...args, (e, r) => {
-          if (e) { reject(e) } else { resolve(r) }
-        })
-      })
+      const sendCommand = (...args) => w.webContents.debugger.sendCommand(...args)
       const { nodeId } = (await sendCommand('DOM.getDocument')).root.children[0]
       await sendCommand('CSS.enable')
       const { fonts } = await sendCommand('CSS.getPlatformFontsForNode', { nodeId })