From 26a473db981c9593154aaeb0a558565722105c83 Mon Sep 17 00:00:00 2001 From: Charles Kerr Date: Wed, 24 Sep 2025 19:42:22 -0500 Subject: [PATCH] refactor: make api::Menu inherit from gin::Wrappable (#48351) * refactor: make api::Menu inherit from gin::Wrappable* * refactor: make api::Menu::kWrapperInfo const * refactor: use three-arg version of GetConstructor in Menu refactor: undo branch changes to two-arg version of GetConstructor * fixup! refactor: make api::Menu inherit from gin::Wrappable* fix: return type of Menu::New * fixup! refactor: make api::Menu inherit from gin::Wrappable* make MenuMac's constructor public so that cppgc can use it * refactor: Pinnable -> SelfKeepAlive --- ...ctron_objects_to_wrappablepointertag.patch | 7 ++-- shell/browser/api/electron_api_menu.cc | 16 +++++---- shell/browser/api/electron_api_menu.h | 36 ++++++++++--------- shell/browser/api/electron_api_menu_mac.h | 4 ++- shell/browser/api/electron_api_menu_mac.mm | 15 ++++---- shell/browser/api/electron_api_menu_views.cc | 13 ++++--- shell/common/gin_helper/constructible.h | 4 +-- 7 files changed, 56 insertions(+), 39 deletions(-) diff --git a/patches/chromium/chore_add_electron_objects_to_wrappablepointertag.patch b/patches/chromium/chore_add_electron_objects_to_wrappablepointertag.patch index c91b76f1e3..d34c190f25 100644 --- a/patches/chromium/chore_add_electron_objects_to_wrappablepointertag.patch +++ b/patches/chromium/chore_add_electron_objects_to_wrappablepointertag.patch @@ -8,10 +8,10 @@ electron objects that extend gin::Wrappable and gets allocated on the cpp heap diff --git a/gin/public/wrappable_pointer_tags.h b/gin/public/wrappable_pointer_tags.h -index 80ec409efe1635390887d1324be661643818abff..2e20b63d1fca56efb43c18d0fa04b1e2b4cf339a 100644 +index 80ec409efe1635390887d1324be661643818abff..7b23fbcb16958a37a3ad4d313326c0cd37bf05d4 100644 --- a/gin/public/wrappable_pointer_tags.h +++ b/gin/public/wrappable_pointer_tags.h -@@ -66,7 +66,12 @@ enum WrappablePointerTag : uint16_t { +@@ -66,7 +66,13 @@ enum WrappablePointerTag : uint16_t { kTextInputControllerBindings, // content::TextInputControllerBindings kWebAXObjectProxy, // content::WebAXObjectProxy kWrappedExceptionHandler, // extensions::WrappedExceptionHandler @@ -19,9 +19,10 @@ index 80ec409efe1635390887d1324be661643818abff..2e20b63d1fca56efb43c18d0fa04b1e2 + kElectronApp, // electron::api::App + kElectronDebugger, // electron::api::Debugger + kElectronEvent, // gin_helper::internal::Event ++ kElectronMenu, // electron::api::Menu + kElectronNetLog, // electron::api::NetLog + kElectronSession, // electron::api::Session -+ kLastPointerTag = kElectronEvent, ++ kLastPointerTag = kElectronSession, }; static_assert(kLastPointerTag < diff --git a/shell/browser/api/electron_api_menu.cc b/shell/browser/api/electron_api_menu.cc index 817fa2ac7f..269080dba7 100644 --- a/shell/browser/api/electron_api_menu.cc +++ b/shell/browser/api/electron_api_menu.cc @@ -50,7 +50,8 @@ struct Converter { namespace electron::api { -gin::DeprecatedWrapperInfo Menu::kWrapperInfo = {gin::kEmbedderNativeGin}; +const gin::WrapperInfo Menu::kWrapperInfo = {{gin::kEmbedderNativeGin}, + gin::kElectronMenu}; Menu::Menu(gin::Arguments* args) : model_(std::make_unique(this)) { @@ -268,12 +269,11 @@ bool Menu::WorksWhenHiddenAt(int index) const { } void Menu::OnMenuWillClose() { - Unpin(); + keep_alive_.Clear(); Emit("menu-will-close"); } void Menu::OnMenuWillShow() { - Pin(JavascriptEnvironment::GetIsolate()); Emit("menu-will-show"); } @@ -311,8 +311,12 @@ void Menu::FillObjectTemplate(v8::Isolate* isolate, .Build(); } -const char* Menu::GetTypeName() { - return GetClassName(); +const gin::WrapperInfo* Menu::wrapper_info() const { + return &kWrapperInfo; +} + +const char* Menu::GetHumanReadableName() const { + return "Electron / Menu"; } } // namespace electron::api @@ -327,7 +331,7 @@ void Initialize(v8::Local exports, void* priv) { v8::Isolate* const isolate = electron::JavascriptEnvironment::GetIsolate(); gin_helper::Dictionary dict{isolate, exports}; - dict.Set("Menu", Menu::GetConstructor(isolate, context)); + dict.Set("Menu", Menu::GetConstructor(isolate, context, &Menu::kWrapperInfo)); #if BUILDFLAG(IS_MAC) dict.SetMethod("setApplicationMenu", &Menu::SetApplicationMenu); dict.SetMethod("sendActionToFirstResponder", diff --git a/shell/browser/api/electron_api_menu.h b/shell/browser/api/electron_api_menu.h index ba92e230e3..fc76a0c3df 100644 --- a/shell/browser/api/electron_api_menu.h +++ b/shell/browser/api/electron_api_menu.h @@ -9,11 +9,11 @@ #include #include "base/memory/raw_ptr.h" +#include "gin/wrappable.h" #include "shell/browser/event_emitter_mixin.h" #include "shell/browser/ui/electron_menu_model.h" #include "shell/common/gin_helper/constructible.h" -#include "shell/common/gin_helper/pinnable.h" -#include "shell/common/gin_helper/wrappable.h" +#include "shell/common/gin_helper/self_keep_alive.h" #include "ui/base/mojom/menu_source_type.mojom-forward.h" namespace gin { @@ -25,22 +25,31 @@ namespace electron::api { class BaseWindow; class WebFrameMain; -class Menu : public gin_helper::DeprecatedWrappable, +class Menu : public gin::Wrappable, public gin_helper::EventEmitterMixin, public gin_helper::Constructible, - public gin_helper::Pinnable, public ElectronMenuModel::Delegate, private ElectronMenuModel::Observer { public: + static Menu* New(gin::Arguments* args); + + // Make public for cppgc::MakeGarbageCollected. + explicit Menu(gin::Arguments* args); + ~Menu() override; + + // disable copy + Menu(const Menu&) = delete; + Menu& operator=(const Menu&) = delete; + + // gin::Wrappable + static const gin::WrapperInfo kWrapperInfo; + const gin::WrapperInfo* wrapper_info() const override; + const char* GetHumanReadableName() const override; + // gin_helper::Constructible - static gin_helper::Handle New(gin::Arguments* args); static void FillObjectTemplate(v8::Isolate*, v8::Local); static const char* GetClassName() { return "Menu"; } - // gin_helper::Wrappable - static gin::DeprecatedWrapperInfo kWrapperInfo; - const char* GetTypeName() override; - #if BUILDFLAG(IS_MAC) // Set the global menubar. static void SetApplicationMenu(Menu* menu); @@ -51,14 +60,7 @@ class Menu : public gin_helper::DeprecatedWrappable, ElectronMenuModel* model() const { return model_.get(); } - // disable copy - Menu(const Menu&) = delete; - Menu& operator=(const Menu&) = delete; - protected: - explicit Menu(gin::Arguments* args); - ~Menu() override; - // Returns a new callback which keeps references of the JS wrapper until the // passed |callback| is called. base::OnceClosure BindSelfToClosure(base::OnceClosure callback); @@ -129,6 +131,8 @@ class Menu : public gin_helper::DeprecatedWrappable, bool IsEnabledAt(int index) const; bool IsVisibleAt(int index) const; bool WorksWhenHiddenAt(int index) const; + + gin_helper::SelfKeepAlive keep_alive_{this}; }; } // namespace electron::api diff --git a/shell/browser/api/electron_api_menu_mac.h b/shell/browser/api/electron_api_menu_mac.h index f4d92f0840..750f2a01a2 100644 --- a/shell/browser/api/electron_api_menu_mac.h +++ b/shell/browser/api/electron_api_menu_mac.h @@ -18,10 +18,12 @@ class WebFrameMain; namespace api { class MenuMac : public Menu { - protected: + public: + // Make public for cppgc::MakeGarbageCollected. explicit MenuMac(gin::Arguments* args); ~MenuMac() override; + protected: // Menu void PopupAt(BaseWindow* window, std::optional frame, diff --git a/shell/browser/api/electron_api_menu_mac.mm b/shell/browser/api/electron_api_menu_mac.mm index a70725d983..b5c60b3eb5 100644 --- a/shell/browser/api/electron_api_menu_mac.mm +++ b/shell/browser/api/electron_api_menu_mac.mm @@ -20,6 +20,8 @@ #include "shell/common/keyboard_util.h" #include "shell/common/node_includes.h" #include "ui/base/cocoa/menu_utils.h" +#include "v8/include/cppgc/allocation.h" +#include "v8/include/v8-cppgc.h" namespace { @@ -47,7 +49,7 @@ ui::Accelerator GetAcceleratorFromKeyEquivalentAndModifierMask( namespace electron::api { -MenuMac::MenuMac(gin::Arguments* args) : Menu(args) {} +MenuMac::MenuMac(gin::Arguments* args) : Menu{args} {} MenuMac::~MenuMac() = default; @@ -288,11 +290,12 @@ void Menu::SendActionToFirstResponder(const std::string& action) { } // static -gin_helper::Handle Menu::New(gin::Arguments* args) { - auto handle = gin_helper::CreateHandle(args->isolate(), - static_cast(new MenuMac(args))); - gin_helper::CallMethod(args->isolate(), handle.get(), "_init"); - return handle; +Menu* Menu::New(gin::Arguments* args) { + v8::Isolate* const isolate = args->isolate(); + Menu* const menu = cppgc::MakeGarbageCollected( + isolate->GetCppHeap()->GetAllocationHandle(), args); + gin_helper::CallMethod(isolate, menu, "_init"); + return menu; } } // namespace electron::api diff --git a/shell/browser/api/electron_api_menu_views.cc b/shell/browser/api/electron_api_menu_views.cc index cc10653a4d..1fa67b3bb4 100644 --- a/shell/browser/api/electron_api_menu_views.cc +++ b/shell/browser/api/electron_api_menu_views.cc @@ -11,6 +11,8 @@ #include "shell/browser/api/electron_api_web_frame_main.h" #include "shell/browser/native_window_views.h" #include "ui/display/screen.h" +#include "v8/include/cppgc/allocation.h" +#include "v8/include/v8-cppgc.h" using views::MenuRunner; @@ -84,11 +86,12 @@ void MenuViews::OnClosed(int32_t window_id, base::OnceClosure callback) { } // static -gin_helper::Handle Menu::New(gin::Arguments* args) { - auto handle = gin_helper::CreateHandle( - args->isolate(), static_cast(new MenuViews(args))); - gin_helper::CallMethod(args->isolate(), handle.get(), "_init"); - return handle; +Menu* Menu::New(gin::Arguments* args) { + v8::Isolate* const isolate = args->isolate(); + Menu* menu = cppgc::MakeGarbageCollected( + isolate->GetCppHeap()->GetAllocationHandle(), args); + gin_helper::CallMethod(isolate, menu, "_init"); + return menu; } } // namespace electron::api diff --git a/shell/common/gin_helper/constructible.h b/shell/common/gin_helper/constructible.h index dc07046269..1f6db4bc8c 100644 --- a/shell/common/gin_helper/constructible.h +++ b/shell/common/gin_helper/constructible.h @@ -43,7 +43,7 @@ class Constructible { v8::Isolate* const isolate, v8::Local context) { gin::PerIsolateData* data = gin::PerIsolateData::From(isolate); - auto* wrapper_info = &T::kWrapperInfo; + auto* const wrapper_info = &T::kWrapperInfo; v8::Local constructor = data->DeprecatedGetFunctionTemplate(wrapper_info); if (constructor.IsEmpty()) { @@ -67,7 +67,7 @@ class Constructible { static v8::Local GetConstructor( v8::Isolate* const isolate, v8::Local context, - gin::WrapperInfo* wrapper_info) { + const gin::WrapperInfo* const wrapper_info) { gin::PerIsolateData* data = gin::PerIsolateData::From(isolate); v8::Local constructor = data->GetFunctionTemplate(wrapper_info);