refactor: reduce code duplication in gin_helper::Promise (33-x-y) (#43778)

refactor: reduce code duplication in gin_helper::Promise (#43716)

* refactor: move scope scaffolding into SettletScope

idea stolen from SpellCheckScope

* refactor: move impl of PromiseBase::RejectPromise() to the cc file

* chore: remove unused #include

Co-authored-by: Shelley Vohr <[email protected]>

shell/browser/api/electron_api_system_preferences_win.cc

@@ -16,6 +16,7 @@
 #include "base/win/windows_types.h"
 #include "base/win/wrapped_window_proc.h"
 #include "shell/common/color_util.h"
+#include "shell/common/process_util.h"
 #include "ui/gfx/color_utils.h"
 #include "ui/gfx/win/hwnd_util.h"
 

shell/browser/api/electron_api_web_contents.cc

@@ -125,6 +125,7 @@
 #include "shell/common/gin_converters/value_converter.h"
 #include "shell/common/gin_helper/dictionary.h"
 #include "shell/common/gin_helper/error_thrower.h"
+#include "shell/common/gin_helper/locker.h"
 #include "shell/common/gin_helper/object_template_builder.h"
 #include "shell/common/gin_helper/promise.h"
 #include "shell/common/language_util.h"

shell/browser/electron_crypto_module_delegate_nss.cc

@@ -4,6 +4,7 @@
 
 #include "shell/browser/electron_crypto_module_delegate_nss.h"
 
+#include "content/public/browser/browser_thread.h"
 #include "crypto/nss_crypto_module_delegate.h"
 #include "shell/browser/api/electron_api_app.h"
 #include "shell/browser/javascript_environment.h"

shell/browser/serial/serial_chooser_controller.cc

@@ -7,7 +7,7 @@
 #include <algorithm>
 #include <utility>
 
-#include "base/files/file_path.h"
+#include "base/containers/contains.h"
 #include "base/functional/bind.h"
 #include "base/strings/stringprintf.h"
 #include "content/public/browser/web_contents.h"

shell/browser/ui/devtools_manager_delegate.cc

@@ -12,6 +12,7 @@
 #include "base/functional/bind.h"
 #include "base/path_service.h"
 #include "base/strings/string_number_conversions.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/devtools_agent_host.h"
 #include "content/public/browser/devtools_frontend_host.h"
 #include "content/public/browser/devtools_socket_factory.h"

shell/common/api/electron_api_clipboard.cc

@@ -13,6 +13,7 @@
 #include "shell/common/gin_converters/image_converter.h"
 #include "shell/common/gin_helper/dictionary.h"
 #include "shell/common/node_includes.h"
+#include "shell/common/process_util.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/base/clipboard/clipboard_format_type.h"
 #include "ui/base/clipboard/file_info.h"

shell/common/api/electron_bindings.cc

@@ -20,6 +20,7 @@
 #include "shell/common/application_info.h"
 #include "shell/common/gin_converters/file_path_converter.h"
 #include "shell/common/gin_helper/dictionary.h"
+#include "shell/common/gin_helper/locker.h"
 #include "shell/common/gin_helper/microtasks_scope.h"
 #include "shell/common/gin_helper/promise.h"
 #include "shell/common/heap_snapshot.h"

shell/common/gin_helper/promise.cc

@@ -2,13 +2,27 @@
 // Use of this source code is governed by the MIT license that can be
 // found in the LICENSE file.
 
+#include <string>
 #include <string_view>
 
+#include "content/public/browser/browser_task_traits.h"
+#include "content/public/browser/browser_thread.h"
+#include "shell/common/gin_helper/microtasks_scope.h"
 #include "shell/common/gin_helper/promise.h"
+#include "shell/common/process_util.h"
 #include "v8/include/v8-context.h"
 
 namespace gin_helper {
 
+PromiseBase::SettleScope::SettleScope(const PromiseBase& base)
+    : handle_scope_{base.isolate()},
+      context_{base.GetContext()},
+      microtasks_scope_{base.isolate(), context_->GetMicrotaskQueue(), false,
+                        v8::MicrotasksScope::kRunMicrotasks},
+      context_scope_{context_} {}
+
+PromiseBase::SettleScope::~SettleScope() = default;
+
 PromiseBase::PromiseBase(v8::Isolate* isolate)
     : PromiseBase(isolate,
                   v8::Promise::Resolver::New(isolate->GetCurrentContext())
@@ -28,40 +42,30 @@ PromiseBase::~PromiseBase() = default;
 
 PromiseBase& PromiseBase::operator=(PromiseBase&&) = default;
 
-v8::Maybe<bool> PromiseBase::Reject() {
-  v8::HandleScope handle_scope(isolate());
-  v8::Local<v8::Context> context = GetContext();
-  gin_helper::MicrotasksScope microtasks_scope{
-      isolate(), context->GetMicrotaskQueue(), false,
-      v8::MicrotasksScope::kRunMicrotasks};
-  v8::Context::Scope context_scope(context);
-
-  return GetInner()->Reject(context, v8::Undefined(isolate()));
+// static
+scoped_refptr<base::TaskRunner> PromiseBase::GetTaskRunner() {
+  if (electron::IsBrowserProcess() &&
+      !content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)) {
+    return content::GetUIThreadTaskRunner({});
+  }
+  return {};
 }
 
 v8::Maybe<bool> PromiseBase::Reject(v8::Local<v8::Value> except) {
-  v8::HandleScope handle_scope(isolate());
-  v8::Local<v8::Context> context = GetContext();
-  gin_helper::MicrotasksScope microtasks_scope{
-      isolate(), context->GetMicrotaskQueue(), false,
-      v8::MicrotasksScope::kRunMicrotasks};
-  v8::Context::Scope context_scope(context);
-
-  return GetInner()->Reject(context, except);
+  SettleScope settle_scope{*this};
+  return GetInner()->Reject(settle_scope.context_, except);
 }
 
-v8::Maybe<bool> PromiseBase::RejectWithErrorMessage(
-    const std::string_view message) {
-  v8::HandleScope handle_scope(isolate());
-  v8::Local<v8::Context> context = GetContext();
-  gin_helper::MicrotasksScope microtasks_scope{
-      isolate(), context->GetMicrotaskQueue(), false,
-      v8::MicrotasksScope::kRunMicrotasks};
-  v8::Context::Scope context_scope(context);
-
-  v8::Local<v8::Value> error =
-      v8::Exception::Error(gin::StringToV8(isolate(), message));
-  return GetInner()->Reject(context, (error));
+v8::Maybe<bool> PromiseBase::Reject() {
+  SettleScope settle_scope{*this};
+  return GetInner()->Reject(settle_scope.context_, v8::Undefined(isolate()));
+}
+
+v8::Maybe<bool> PromiseBase::RejectWithErrorMessage(std::string_view errmsg) {
+  SettleScope settle_scope{*this};
+  return GetInner()->Reject(
+      settle_scope.context_,
+      v8::Exception::Error(gin::StringToV8(isolate(), errmsg)));
 }
 
 v8::Local<v8::Context> PromiseBase::GetContext() const {
@@ -76,11 +80,28 @@ v8::Local<v8::Promise::Resolver> PromiseBase::GetInner() const {
   return resolver_.Get(isolate());
 }
 
+// static
+void PromiseBase::RejectPromise(PromiseBase&& promise,
+                                std::string_view errmsg) {
+  if (auto task_runner = GetTaskRunner()) {
+    task_runner->PostTask(
+        FROM_HERE, base::BindOnce(
+                       // Note that this callback can not take std::string_view,
+                       // as StringPiece only references string internally and
+                       // will blow when a temporary string is passed.
+                       [](PromiseBase&& promise, std::string str) {
+                         promise.RejectWithErrorMessage(str);
+                       },
+                       std::move(promise), std::string{errmsg}));
+  } else {
+    promise.RejectWithErrorMessage(errmsg);
+  }
+}
+
 // static
 void Promise<void>::ResolvePromise(Promise<void> promise) {
-  if (electron::IsBrowserProcess() &&
-      !content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)) {
-    content::GetUIThreadTaskRunner({})->PostTask(
+  if (auto task_runner = GetTaskRunner()) {
+    task_runner->PostTask(
         FROM_HERE,
         base::BindOnce([](Promise<void> promise) { promise.Resolve(); },
                        std::move(promise)));
@@ -97,14 +118,8 @@ v8::Local<v8::Promise> Promise<void>::ResolvedPromise(v8::Isolate* isolate) {
 }
 
 v8::Maybe<bool> Promise<void>::Resolve() {
-  v8::HandleScope handle_scope(isolate());
-  v8::Local<v8::Context> context = GetContext();
-  gin_helper::MicrotasksScope microtasks_scope{
-      isolate(), context->GetMicrotaskQueue(), false,
-      v8::MicrotasksScope::kRunMicrotasks};
-  v8::Context::Scope context_scope(context);
-
-  return GetInner()->Resolve(context, v8::Undefined(isolate()));
+  SettleScope settle_scope{*this};
+  return GetInner()->Resolve(settle_scope.context_, v8::Undefined(isolate()));
 }
 
 }  // namespace gin_helper

shell/common/gin_helper/promise.h

@@ -12,12 +12,10 @@
 #include <utility>
 
 #include "base/memory/raw_ptr.h"
-#include "content/public/browser/browser_task_traits.h"
-#include "content/public/browser/browser_thread.h"
+#include "base/memory/scoped_refptr.h"
+#include "base/task/task_runner.h"
 #include "shell/common/gin_converters/std_converter.h"
-#include "shell/common/gin_helper/locker.h"
 #include "shell/common/gin_helper/microtasks_scope.h"
-#include "shell/common/process_util.h"
 #include "v8/include/v8-context.h"
 
 namespace gin_helper {
@@ -45,27 +43,7 @@ class PromiseBase {
   PromiseBase& operator=(PromiseBase&&);
 
   // Helper for rejecting promise with error message.
-  //
-  // Note: The parameter type is PromiseBase&& so it can take the instances of
-  // Promise<T> type.
-  static void RejectPromise(PromiseBase&& promise,
-                            const std::string_view errmsg) {
-    if (electron::IsBrowserProcess() &&
-        !content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)) {
-      content::GetUIThreadTaskRunner({})->PostTask(
-          FROM_HERE,
-          base::BindOnce(
-              // Note that this callback can not take std::string_view,
-              // as StringPiece only references string internally and
-              // will blow when a temporary string is passed.
-              [](PromiseBase&& promise, std::string str) {
-                promise.RejectWithErrorMessage(str);
-              },
-              std::move(promise), std::string{errmsg}));
-    } else {
-      promise.RejectWithErrorMessage(errmsg);
-    }
-  }
+  static void RejectPromise(PromiseBase&& promise, std::string_view errmsg);
 
   v8::Maybe<bool> Reject();
   v8::Maybe<bool> Reject(v8::Local<v8::Value> except);
@@ -77,8 +55,20 @@ class PromiseBase {
   v8::Isolate* isolate() const { return isolate_; }
 
  protected:
+  struct SettleScope {
+    explicit SettleScope(const PromiseBase& base);
+    ~SettleScope();
+
+    v8::HandleScope handle_scope_;
+    v8::Local<v8::Context> context_;
+    gin_helper::MicrotasksScope microtasks_scope_;
+    v8::Context::Scope context_scope_;
+  };
+
   v8::Local<v8::Promise::Resolver> GetInner() const;
 
+  static scoped_refptr<base::TaskRunner> GetTaskRunner();
+
  private:
   raw_ptr<v8::Isolate> isolate_;
   v8::Global<v8::Context> context_;
@@ -93,9 +83,8 @@ class Promise : public PromiseBase {
 
   // Helper for resolving the promise with |result|.
   static void ResolvePromise(Promise<RT> promise, RT result) {
-    if (electron::IsBrowserProcess() &&
-        !content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)) {
-      content::GetUIThreadTaskRunner({})->PostTask(
+    if (auto task_runner = GetTaskRunner()) {
+      task_runner->PostTask(
           FROM_HERE, base::BindOnce([](Promise<RT> promise,
                                        RT result) { promise.Resolve(result); },
                                     std::move(promise), std::move(result)));
@@ -115,21 +104,13 @@ class Promise : public PromiseBase {
   // Convert to another type.
   template <typename NT>
   Promise<NT> As() {
-    return Promise<NT>(isolate(), GetInner());
+    return Promise<NT>{isolate(), GetInner()};
   }
 
-  // Promise resolution is a microtask
-  // We use the MicrotasksRunner to trigger the running of pending microtasks
   v8::Maybe<bool> Resolve(const RT& value) {
-    gin_helper::Locker locker(isolate());
-    v8::HandleScope handle_scope(isolate());
-    v8::Local<v8::Context> context = GetContext();
-    gin_helper::MicrotasksScope microtasks_scope{
-        isolate(), context->GetMicrotaskQueue(), false,
-        v8::MicrotasksScope::kRunMicrotasks};
-    v8::Context::Scope context_scope(context);
-
-    return GetInner()->Resolve(context, gin::ConvertToV8(isolate(), value));
+    SettleScope settle_scope{*this};
+    return GetInner()->Resolve(settle_scope.context_,
+                               gin::ConvertToV8(isolate(), value));
   }
 
   template <typename... ResolveType>
@@ -142,15 +123,10 @@ class Promise : public PromiseBase {
         std::is_same<RT, std::tuple_element_t<0, std::tuple<ResolveType...>>>(),
         "A promises's 'Then' callback must handle the same type as the "
         "promises resolve type");
-    gin_helper::Locker locker(isolate());
-    v8::HandleScope handle_scope(isolate());
-    v8::Local<v8::Context> context = GetContext();
-    v8::Context::Scope context_scope(context);
-
+    SettleScope settle_scope{*this};
     v8::Local<v8::Value> value = gin::ConvertToV8(isolate(), std::move(cb));
     v8::Local<v8::Function> handler = value.As<v8::Function>();
-
-    return GetHandle()->Then(context, handler);
+    return GetHandle()->Then(settle_scope.context_, handler);
   }
 };
 

shell/common/node_bindings.cc

@@ -37,6 +37,7 @@
 #include "shell/common/mac/main_application_bundle.h"
 #include "shell/common/node_includes.h"
 #include "shell/common/node_util.h"
+#include "shell/common/process_util.h"
 #include "shell/common/world_ids.h"
 #include "third_party/blink/public/web/web_local_frame.h"
 #include "third_party/blink/renderer/bindings/core/v8/v8_initializer.h"  // nogncheck