feat: bring back optional callbacks for WebFrame#executeJavaScript* methods

docs/api/web-frame.md

@@ -122,28 +122,45 @@ by its key, which is returned from `webFrame.insertCSS(css)`.
 
 Inserts `text` to the focused element.
 
-### `webFrame.executeJavaScript(code[, userGesture])`
+### `webFrame.executeJavaScript(code[, userGesture, callback])`
 
 * `code` String
 * `userGesture` Boolean (optional) - Default is `false`.
+* `callback` Function (optional) - Called after script has been executed. Unless
+  the frame is suspended (e.g. showing a modal alert), execution will be
+  synchronous and the callback will be invoked before the method returns. For
+  compatibility with an older version of this method, the error parameter is
+  second.
+  * `result` Any
+  * `error` Error
 
-Returns `Promise<any>` - A promise that resolves with the result of the executed code
-or is rejected if the result of the code is a rejected promise.
+Returns `Promise<any>` - A promise that resolves with the result of the executed
+code or is rejected if execution throws or results in a rejected promise.
 
 Evaluates `code` in page.
 
 In the browser window some HTML APIs like `requestFullScreen` can only be
 invoked by a gesture from the user. Setting `userGesture` to `true` will remove
 this limitation.
 
-### `webFrame.executeJavaScriptInIsolatedWorld(worldId, scripts[, userGesture])`
+### `webFrame.executeJavaScriptInIsolatedWorld(worldId, scripts[, userGesture, callback])`
 
-* `worldId` Integer - The ID of the world to run the javascript in, `0` is the default world, `999` is the world used by Electrons `contextIsolation` feature.  You can provide any integer here.
+* `worldId` Integer - The ID of the world to run the javascript
+            in, `0` is the default main world (where content runs), `999` is the
+            world used by Electron's `contextIsolation` feature. Accepts values
+            in the range 1..536870911.
 * `scripts` [WebSource[]](structures/web-source.md)
 * `userGesture` Boolean (optional) - Default is `false`.
-
-Returns `Promise<any>` - A promise that resolves with the result of the executed code
-or is rejected if the result of the code is a rejected promise.
+* `callback` Function (optional) - Called after script has been executed. Unless
+  the frame is suspended (e.g. showing a modal alert), execution will be
+  synchronous and the callback will be invoked before the method returns.  For
+  compatibility with an older version of this method, the error parameter is
+  second.
+  * `result` Any
+  * `error` Error
+
+Returns `Promise<any>` - A promise that resolves with the result of the executed
+code or is rejected if execution throws or results in a rejected promise.
 
 Works like `executeJavaScript` but evaluates `scripts` in an isolated context.
 

shell/renderer/api/electron_api_web_frame.cc

@@ -15,6 +15,7 @@
 #include "services/service_manager/public/cpp/interface_provider.h"
 #include "shell/common/api/api.mojom.h"
 #include "shell/common/gin_converters/blink_converter.h"
+#include "shell/common/gin_converters/callback_converter.h"
 #include "shell/common/gin_helper/dictionary.h"
 #include "shell/common/gin_helper/promise.h"
 #include "shell/common/node_includes.h"
@@ -110,32 +111,58 @@ class RenderFrameStatus final : public content::RenderFrameObserver {
 
 class ScriptExecutionCallback : public blink::WebScriptExecutionCallback {
  public:
+  // for compatibility with the older version of this, error is after result
+  using CompletionCallback =
+      base::OnceCallback<void(const v8::Local<v8::Value>& result,
+                              const v8::Local<v8::Value>& error)>;
+
   explicit ScriptExecutionCallback(
-      gin_helper::Promise<v8::Local<v8::Value>> promise)
-      : promise_(std::move(promise)) {}
+      gin_helper::Promise<v8::Local<v8::Value>> promise,
+      CompletionCallback callback)
+      : promise_(std::move(promise)), callback_(std::move(callback)) {}
+
   ~ScriptExecutionCallback() override = default;
 
   void Completed(
       const blink::WebVector<v8::Local<v8::Value>>& result) override {
+    v8::Isolate* isolate = v8::Isolate::GetCurrent();
     if (!result.empty()) {
       if (!result[0].IsEmpty()) {
         // Right now only single results per frame is supported.
+        if (!callback_.is_null())
+          std::move(callback_).Run(result[0], v8::Undefined(isolate));
         promise_.Resolve(result[0]);
       } else {
-        promise_.RejectWithErrorMessage(
+        const char* error_message =
             "Script failed to execute, this normally means an error "
-            "was thrown. Check the renderer console for the error.");
+            "was thrown. Check the renderer console for the error.";
+        if (!callback_.is_null()) {
+          std::move(callback_).Run(
+              v8::Undefined(isolate),
+              v8::Exception::Error(
+                  v8::String::NewFromUtf8(isolate, error_message)
+                      .ToLocalChecked()));
+        }
+        promise_.RejectWithErrorMessage(error_message);
       }
     } else {
-      promise_.RejectWithErrorMessage(
+      const char* error_message =
           "WebFrame was removed before script could run. This normally means "
-          "the underlying frame was destroyed");
+          "the underlying frame was destroyed";
+      if (!callback_.is_null()) {
+        std::move(callback_).Run(
+            v8::Undefined(isolate),
+            v8::Exception::Error(v8::String::NewFromUtf8(isolate, error_message)
+                                     .ToLocalChecked()));
+      }
+      promise_.RejectWithErrorMessage(error_message);
     }
     delete this;
   }
 
  private:
   gin_helper::Promise<v8::Local<v8::Value>> promise_;
+  CompletionCallback callback_;
 
   DISALLOW_COPY_AND_ASSIGN(ScriptExecutionCallback);
 };
@@ -373,9 +400,14 @@ v8::Local<v8::Promise> ExecuteJavaScript(gin_helper::Arguments* args,
   bool has_user_gesture = false;
   args->GetNext(&has_user_gesture);
 
+  ScriptExecutionCallback::CompletionCallback completion_callback;
+  args->GetNext(&completion_callback);
+
   GetRenderFrame(window)->GetWebFrame()->RequestExecuteScriptAndReturnValue(
       blink::WebScriptSource(blink::WebString::FromUTF16(code)),
-      has_user_gesture, new ScriptExecutionCallback(std::move(promise)));
+      has_user_gesture,
+      new ScriptExecutionCallback(std::move(promise),
+                                  std::move(completion_callback)));
 
   return handle;
 }
@@ -389,6 +421,16 @@ v8::Local<v8::Promise> ExecuteJavaScriptInIsolatedWorld(
   gin_helper::Promise<v8::Local<v8::Value>> promise(isolate);
   v8::Local<v8::Promise> handle = promise.GetHandle();
 
+  bool has_user_gesture = false;
+  args->GetNext(&has_user_gesture);
+
+  blink::WebLocalFrame::ScriptExecutionType scriptExecutionType =
+      blink::WebLocalFrame::kSynchronous;
+  args->GetNext(&scriptExecutionType);
+
+  ScriptExecutionCallback::CompletionCallback completion_callback;
+  args->GetNext(&completion_callback);
+
   std::vector<blink::WebScriptSource> sources;
 
   for (const auto& script : scripts) {
@@ -399,7 +441,15 @@ v8::Local<v8::Promise> ExecuteJavaScriptInIsolatedWorld(
     script.Get("startLine", &start_line);
 
     if (!script.Get("code", &code)) {
-      promise.RejectWithErrorMessage("Invalid 'code'");
+      const char* error_message = "Invalid 'code'";
+      if (!completion_callback.is_null()) {
+        std::move(completion_callback)
+            .Run(v8::Undefined(isolate),
+                 v8::Exception::Error(
+                     v8::String::NewFromUtf8(isolate, error_message)
+                         .ToLocalChecked()));
+      }
+      promise.RejectWithErrorMessage(error_message);
       return handle;
     }
 
@@ -408,19 +458,14 @@ v8::Local<v8::Promise> ExecuteJavaScriptInIsolatedWorld(
                                blink::WebURL(GURL(url)), start_line));
   }
 
-  bool has_user_gesture = false;
-  args->GetNext(&has_user_gesture);
-
-  blink::WebLocalFrame::ScriptExecutionType scriptExecutionType =
-      blink::WebLocalFrame::kSynchronous;
-  args->GetNext(&scriptExecutionType);
-
   // Debugging tip: if you see a crash stack trace beginning from this call,
   // then it is very likely that some exception happened when executing the
   // "content_script/init.js" script.
   GetRenderFrame(window)->GetWebFrame()->RequestExecuteScriptInIsolatedWorld(
       world_id, &sources.front(), sources.size(), has_user_gesture,
-      scriptExecutionType, new ScriptExecutionCallback(std::move(promise)));
+      scriptExecutionType,
+      new ScriptExecutionCallback(std::move(promise),
+                                  std::move(completion_callback)));
 
   return handle;
 }

spec/api-web-frame-spec.js

@@ -25,4 +25,116 @@ describe('webFrame module', function () {
   it('findFrameByRoutingId() does not crash when not found', () => {
     expect(webFrame.findFrameByRoutingId(-1)).to.be.null()
   })
+
+  describe('executeJavaScript', () => {
+    let childFrameElement, childFrame
+
+    before(() => {
+      childFrameElement = document.createElement('iframe')
+      document.body.appendChild(childFrameElement)
+      childFrame = webFrame.firstChild
+    })
+
+    after(() => {
+      childFrameElement.remove()
+    })
+
+    it('executeJavaScript() yields results via a promise and a sync callback', done => {
+      let callbackResult, callbackError
+
+      childFrame
+        .executeJavaScript('1 + 1', (result, error) => {
+          callbackResult = result
+          callbackError = error
+        })
+        .then(
+          promiseResult => {
+            expect(promiseResult).to.equal(2)
+            done()
+          },
+          promiseError => {
+            done(promiseError)
+          }
+        )
+
+      expect(callbackResult).to.equal(2)
+      expect(callbackError).to.be.undefined()
+    })
+
+    it('executeJavaScriptInIsolatedWorld() yields results via a promise and a sync callback', done => {
+      let callbackResult, callbackError
+
+      childFrame
+        .executeJavaScriptInIsolatedWorld(999, [{ code: '1 + 1' }], (result, error) => {
+          callbackResult = result
+          callbackError = error
+        })
+        .then(
+          promiseResult => {
+            expect(promiseResult).to.equal(2)
+            done()
+          },
+          promiseError => {
+            done(promiseError)
+          }
+        )
+
+      expect(callbackResult).to.equal(2)
+      expect(callbackError).to.be.undefined()
+    })
+
+    it('executeJavaScript() yields errors via a promise and a sync callback', done => {
+      let callbackResult, callbackError
+
+      childFrame
+        .executeJavaScript('thisShouldProduceAnError()', (result, error) => {
+          callbackResult = result
+          callbackError = error
+        })
+        .then(
+          promiseResult => {
+            done(new Error('error is expected'))
+          },
+          promiseError => {
+            expect(promiseError).to.be.an('error')
+            done()
+          }
+        )
+
+      expect(callbackResult).to.be.undefined()
+      expect(callbackError).to.be.an('error')
+    })
+
+    // executeJavaScriptInIsolatedWorld is failing to detect exec errors and is neither
+    // rejecting nor passing the error to the callback. This predates the reintroduction
+    // of the callback so will not be fixed as part of the callback PR
+    // if/when this is fixed the test can be uncommented.
+    //
+    // it('executeJavaScriptInIsolatedWorld() yields errors via a promise and a sync callback', done => {
+    //   let callbackResult, callbackError
+    //
+    //   childFrame
+    //     .executeJavaScriptInIsolatedWorld(999, [{ code: 'thisShouldProduceAnError()' }], (result, error) => {
+    //       callbackResult = result
+    //       callbackError = error
+    //     })
+    //     .then(
+    //       promiseResult => {
+    //         done(new Error('error is expected'))
+    //       },
+    //       promiseError => {
+    //         expect(promiseError).to.be.an('error')
+    //         done()
+    //       }
+    //     )
+    //
+    //   expect(callbackResult).to.be.undefined()
+    //   expect(callbackError).to.be.an('error')
+    // })
+
+    it('executeJavaScript(InIsolatedWorld) can be used without a callback', async () => {
+      expect(await webFrame.executeJavaScript('1 + 1')).to.equal(2)
+      expect(await webFrame.executeJavaScriptInIsolatedWorld(999, [{ code: '1 + 1' }])).to.equal(2)
+    })
+  })
 })