From 5c92ea76e9a4719b2bf379669ed421e622a61835 Mon Sep 17 00:00:00 2001 From: Charles Kerr Date: Thu, 19 Sep 2024 06:01:13 -0500 Subject: [PATCH] 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 --- .../electron_api_system_preferences_win.cc | 1 + .../browser/api/electron_api_web_contents.cc | 1 + .../electron_crypto_module_delegate_nss.cc | 1 + .../serial/serial_chooser_controller.cc | 2 +- shell/browser/ui/devtools_manager_delegate.cc | 1 + shell/common/api/electron_api_clipboard.cc | 1 + shell/common/api/electron_bindings.cc | 1 + shell/common/gin_helper/promise.cc | 93 +++++++++++-------- shell/common/gin_helper/promise.h | 70 +++++--------- shell/common/node_bindings.cc | 1 + 10 files changed, 85 insertions(+), 87 deletions(-) diff --git a/shell/browser/api/electron_api_system_preferences_win.cc b/shell/browser/api/electron_api_system_preferences_win.cc index eeeb7361c277..b5097657bb5c 100644 --- a/shell/browser/api/electron_api_system_preferences_win.cc +++ b/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" diff --git a/shell/browser/api/electron_api_web_contents.cc b/shell/browser/api/electron_api_web_contents.cc index a8b10d1efe8a..4d9c5be8bb36 100644 --- a/shell/browser/api/electron_api_web_contents.cc +++ b/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" diff --git a/shell/browser/electron_crypto_module_delegate_nss.cc b/shell/browser/electron_crypto_module_delegate_nss.cc index 48fd9f1fc786..6d9b55daaa1f 100644 --- a/shell/browser/electron_crypto_module_delegate_nss.cc +++ b/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" diff --git a/shell/browser/serial/serial_chooser_controller.cc b/shell/browser/serial/serial_chooser_controller.cc index 99c7ae168101..fe93fb8112af 100644 --- a/shell/browser/serial/serial_chooser_controller.cc +++ b/shell/browser/serial/serial_chooser_controller.cc @@ -7,7 +7,7 @@ #include #include -#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" diff --git a/shell/browser/ui/devtools_manager_delegate.cc b/shell/browser/ui/devtools_manager_delegate.cc index e62729171b33..2d4547aeca40 100644 --- a/shell/browser/ui/devtools_manager_delegate.cc +++ b/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" diff --git a/shell/common/api/electron_api_clipboard.cc b/shell/common/api/electron_api_clipboard.cc index f4a20cc45efe..f8c2a84edd3e 100644 --- a/shell/common/api/electron_api_clipboard.cc +++ b/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" diff --git a/shell/common/api/electron_bindings.cc b/shell/common/api/electron_bindings.cc index acae6ba083dd..487bab7e35ea 100644 --- a/shell/common/api/electron_bindings.cc +++ b/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" diff --git a/shell/common/gin_helper/promise.cc b/shell/common/gin_helper/promise.cc index bd162c426ef6..17da3f4bb3e1 100644 --- a/shell/common/gin_helper/promise.cc +++ b/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 #include +#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 PromiseBase::Reject() { - v8::HandleScope handle_scope(isolate()); - v8::Local 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 PromiseBase::GetTaskRunner() { + if (electron::IsBrowserProcess() && + !content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)) { + return content::GetUIThreadTaskRunner({}); + } + return {}; } v8::Maybe PromiseBase::Reject(v8::Local except) { - v8::HandleScope handle_scope(isolate()); - v8::Local 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 PromiseBase::RejectWithErrorMessage( - const std::string_view message) { - v8::HandleScope handle_scope(isolate()); - v8::Local context = GetContext(); - gin_helper::MicrotasksScope microtasks_scope{ - isolate(), context->GetMicrotaskQueue(), false, - v8::MicrotasksScope::kRunMicrotasks}; - v8::Context::Scope context_scope(context); +v8::Maybe PromiseBase::Reject() { + SettleScope settle_scope{*this}; + return GetInner()->Reject(settle_scope.context_, v8::Undefined(isolate())); +} - v8::Local error = - v8::Exception::Error(gin::StringToV8(isolate(), message)); - return GetInner()->Reject(context, (error)); +v8::Maybe 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 PromiseBase::GetContext() const { @@ -76,11 +80,28 @@ v8::Local 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::ResolvePromise(Promise 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 promise) { promise.Resolve(); }, std::move(promise))); @@ -97,14 +118,8 @@ v8::Local Promise::ResolvedPromise(v8::Isolate* isolate) { } v8::Maybe Promise::Resolve() { - v8::HandleScope handle_scope(isolate()); - v8::Local 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 diff --git a/shell/common/gin_helper/promise.h b/shell/common/gin_helper/promise.h index e220bd6291e6..1fc1832f013d 100644 --- a/shell/common/gin_helper/promise.h +++ b/shell/common/gin_helper/promise.h @@ -12,12 +12,10 @@ #include #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 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 Reject(); v8::Maybe Reject(v8::Local 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 context_; + gin_helper::MicrotasksScope microtasks_scope_; + v8::Context::Scope context_scope_; + }; + v8::Local GetInner() const; + static scoped_refptr GetTaskRunner(); + private: raw_ptr isolate_; v8::Global context_; @@ -93,9 +83,8 @@ class Promise : public PromiseBase { // Helper for resolving the promise with |result|. static void ResolvePromise(Promise 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 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 Promise As() { - return Promise(isolate(), GetInner()); + return Promise{isolate(), GetInner()}; } - // Promise resolution is a microtask - // We use the MicrotasksRunner to trigger the running of pending microtasks v8::Maybe Resolve(const RT& value) { - gin_helper::Locker locker(isolate()); - v8::HandleScope handle_scope(isolate()); - v8::Local 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 @@ -142,15 +123,10 @@ class Promise : public PromiseBase { std::is_same>>(), "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 context = GetContext(); - v8::Context::Scope context_scope(context); - + SettleScope settle_scope{*this}; v8::Local value = gin::ConvertToV8(isolate(), std::move(cb)); v8::Local handler = value.As(); - - return GetHandle()->Then(context, handler); + return GetHandle()->Then(settle_scope.context_, handler); } }; diff --git a/shell/common/node_bindings.cc b/shell/common/node_bindings.cc index fd77be299004..1f5944a50cc7 100644 --- a/shell/common/node_bindings.cc +++ b/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