From 67663431736ea446869d2582c2c931786920c1ff Mon Sep 17 00:00:00 2001 From: Charles Kerr Date: Mon, 26 Jan 2026 05:28:57 -0600 Subject: [PATCH] refactor: simplify `NativeWindow`-to-`BaseWindow` lookup (#49520) refactor: simplify native window to base window lookup --- shell/browser/api/electron_api_base_window.cc | 5 +-- .../api/electron_api_browser_window.cc | 11 +++--- shell/browser/native_window.cc | 8 +++- shell/browser/native_window.h | 14 +++++-- shell/browser/native_window_mac.h | 4 +- shell/browser/native_window_mac.mm | 9 +++-- shell/browser/native_window_views.cc | 8 ++-- shell/browser/native_window_views.h | 3 +- shell/common/gin_helper/trackable_object.cc | 37 ------------------- shell/common/gin_helper/trackable_object.h | 19 ---------- 10 files changed, 40 insertions(+), 78 deletions(-) diff --git a/shell/browser/api/electron_api_base_window.cc b/shell/browser/api/electron_api_base_window.cc index fff85198ea..acd6642ad6 100644 --- a/shell/browser/api/electron_api_base_window.cc +++ b/shell/browser/api/electron_api_base_window.cc @@ -111,8 +111,8 @@ BaseWindow::BaseWindow(v8::Isolate* isolate, } // Creates NativeWindow. - window_ = NativeWindow::Create( - options, parent.IsEmpty() ? nullptr : parent->window_.get()); + NativeWindow* parent_native = parent.IsEmpty() ? nullptr : parent->window(); + window_ = NativeWindow::Create(GetID(), options, parent_native); window_->AddObserver(this); SetContentView(View::Create(isolate)); @@ -146,7 +146,6 @@ BaseWindow::~BaseWindow() { } void BaseWindow::InitWith(v8::Isolate* isolate, v8::Local wrapper) { - AttachAsUserData(window_.get()); gin_helper::TrackableObject::InitWith(isolate, wrapper); // We can only append this window to parent window's child windows after this diff --git a/shell/browser/api/electron_api_browser_window.cc b/shell/browser/api/electron_api_browser_window.cc index ae94c5da90..4cb2031099 100644 --- a/shell/browser/api/electron_api_browser_window.cc +++ b/shell/browser/api/electron_api_browser_window.cc @@ -336,11 +336,12 @@ void BrowserWindow::BuildPrototype(v8::Isolate* isolate, // static v8::Local BrowserWindow::From(v8::Isolate* isolate, NativeWindow* native_window) { - auto* existing = TrackableObject::FromWrappedClass(isolate, native_window); - if (existing) - return existing->GetWrapper(); - else - return v8::Null(isolate); + if (native_window != nullptr) { + if (auto* base = FromWeakMapID(isolate, native_window->base_window_id())) + return base->GetWrapper(); + } + + return v8::Null(isolate); } } // namespace electron::api diff --git a/shell/browser/native_window.cc b/shell/browser/native_window.cc index 1e0310a4da..9b5615760e 100644 --- a/shell/browser/native_window.cc +++ b/shell/browser/native_window.cc @@ -96,9 +96,11 @@ gfx::Size GetExpandedWindowSize(const NativeWindow* window, } // namespace -NativeWindow::NativeWindow(const gin_helper::Dictionary& options, +NativeWindow::NativeWindow(const int32_t base_window_id, + const gin_helper::Dictionary& options, NativeWindow* parent) - : title_bar_style_{options.ValueOrDefault(options::kTitleBarStyle, + : base_window_id_{base_window_id}, + title_bar_style_{options.ValueOrDefault(options::kTitleBarStyle, TitleBarStyle::kNormal)}, transparent_{options.ValueOrDefault(options::kTransparent, false)}, enable_larger_than_screen_{ @@ -108,6 +110,8 @@ NativeWindow::NativeWindow(const gin_helper::Dictionary& options, has_frame_{options.ValueOrDefault(options::kFrame, true) && title_bar_style_ == TitleBarStyle::kNormal}, parent_{parent} { + DCHECK_NE(base_window_id_, 0); + #if BUILDFLAG(IS_WIN) options.Get(options::kBackgroundMaterial, &background_material_); #elif BUILDFLAG(IS_MAC) diff --git a/shell/browser/native_window.h b/shell/browser/native_window.h index 34c89bef3a..64e0594264 100644 --- a/shell/browser/native_window.h +++ b/shell/browser/native_window.h @@ -17,7 +17,6 @@ #include "base/memory/weak_ptr.h" #include "base/observer_list.h" #include "base/strings/cstring_view.h" -#include "base/supports_user_data.h" #include "content/public/browser/desktop_media_id.h" #include "content/public/browser/web_contents_user_data.h" #include "extensions/browser/app_window/size_constraints.h" @@ -56,8 +55,7 @@ using NativeWindowHandle = gfx::NativeView; using NativeWindowHandle = gfx::AcceleratedWidget; #endif -class NativeWindow : public base::SupportsUserData, - public views::WidgetDelegate { +class NativeWindow : public views::WidgetDelegate { public: ~NativeWindow() override; @@ -68,6 +66,7 @@ class NativeWindow : public base::SupportsUserData, // Create window with existing WebContents, the caller is responsible for // managing the window's live. static std::unique_ptr Create( + int32_t base_window_id, const gin_helper::Dictionary& options, NativeWindow* parent = nullptr); @@ -429,8 +428,12 @@ class NativeWindow : public base::SupportsUserData, // throttling, then throttling in the `ui::Compositor` will be disabled. void UpdateBackgroundThrottlingState(); + [[nodiscard]] auto base_window_id() const { return base_window_id_; } + protected: - NativeWindow(const gin_helper::Dictionary& options, NativeWindow* parent); + NativeWindow(int32_t base_window_id, + const gin_helper::Dictionary& options, + NativeWindow* parent); void set_titlebar_overlay_height(int height) { titlebar_overlay_height_ = height; @@ -488,6 +491,9 @@ class NativeWindow : public base::SupportsUserData, static inline int32_t next_id_ = 0; const int32_t window_id_ = ++next_id_; + // ID of the api::BaseWindow that owns this NativeWindow. + const int32_t base_window_id_; + // The "titleBarStyle" option. const TitleBarStyle title_bar_style_; diff --git a/shell/browser/native_window_mac.h b/shell/browser/native_window_mac.h index 66b647ff10..987f5546b3 100644 --- a/shell/browser/native_window_mac.h +++ b/shell/browser/native_window_mac.h @@ -34,7 +34,9 @@ class NativeWindowMac : public NativeWindow, public ui::NativeThemeObserver, public display::DisplayObserver { public: - NativeWindowMac(const gin_helper::Dictionary& options, NativeWindow* parent); + NativeWindowMac(int32_t base_window_id, + const gin_helper::Dictionary& options, + NativeWindow* parent); ~NativeWindowMac() override; // NativeWindow: diff --git a/shell/browser/native_window_mac.mm b/shell/browser/native_window_mac.mm index 32c37d9946..34ea14cf86 100644 --- a/shell/browser/native_window_mac.mm +++ b/shell/browser/native_window_mac.mm @@ -151,9 +151,11 @@ class NativeAppWindowFrameViewMacClient NativeWindowMac::~NativeWindowMac() = default; -NativeWindowMac::NativeWindowMac(const gin_helper::Dictionary& options, +NativeWindowMac::NativeWindowMac(const int32_t base_window_id, + const gin_helper::Dictionary& options, NativeWindow* parent) - : NativeWindow(options, parent), root_view_(new RootViewMac(this)) { + : NativeWindow{base_window_id, options, parent}, + root_view_(new RootViewMac(this)) { ui::NativeTheme::GetInstanceForNativeUi()->AddObserver(this); display::Screen::Get()->AddObserver(this); @@ -1876,9 +1878,10 @@ void NativeWindowMac::OnWidgetInitialized() { // static std::unique_ptr NativeWindow::Create( + const int32_t base_window_id, const gin_helper::Dictionary& options, NativeWindow* parent) { - return std::make_unique(options, parent); + return std::make_unique(base_window_id, options, parent); } } // namespace electron diff --git a/shell/browser/native_window_views.cc b/shell/browser/native_window_views.cc index 1e37d646fa..68b2483657 100644 --- a/shell/browser/native_window_views.cc +++ b/shell/browser/native_window_views.cc @@ -193,9 +193,10 @@ class NativeWindowClientView : public views::ClientView { } // namespace -NativeWindowViews::NativeWindowViews(const gin_helper::Dictionary& options, +NativeWindowViews::NativeWindowViews(const int32_t base_window_id, + const gin_helper::Dictionary& options, NativeWindow* parent) - : NativeWindow(options, parent) { + : NativeWindow{base_window_id, options, parent} { if (std::string val; options.Get(options::kTitle, &val)) SetTitle(val); @@ -1952,9 +1953,10 @@ void NativeWindowViews::MoveBehindTaskBarIfNeeded() { // static std::unique_ptr NativeWindow::Create( + const int32_t base_window_id, const gin_helper::Dictionary& options, NativeWindow* parent) { - return std::make_unique(options, parent); + return std::make_unique(base_window_id, options, parent); } } // namespace electron diff --git a/shell/browser/native_window_views.h b/shell/browser/native_window_views.h index 874a9fda86..e3847e19fa 100644 --- a/shell/browser/native_window_views.h +++ b/shell/browser/native_window_views.h @@ -48,7 +48,8 @@ class NativeWindowViews : public NativeWindow, private views::WidgetObserver, private ui::EventHandler { public: - NativeWindowViews(const gin_helper::Dictionary& options, + NativeWindowViews(int32_t base_window_id, + const gin_helper::Dictionary& options, NativeWindow* parent); ~NativeWindowViews() override; diff --git a/shell/common/gin_helper/trackable_object.cc b/shell/common/gin_helper/trackable_object.cc index 2f5784402c..fe65ce8953 100644 --- a/shell/common/gin_helper/trackable_object.cc +++ b/shell/common/gin_helper/trackable_object.cc @@ -4,31 +4,11 @@ #include "shell/common/gin_helper/trackable_object.h" -#include - #include "base/functional/bind.h" -#include "base/supports_user_data.h" -#include "shell/browser/electron_browser_main_parts.h" #include "shell/common/process_util.h" namespace gin_helper { -namespace { - -const char kTrackedObjectKey[] = "TrackedObjectKey"; - -class IDUserData : public base::SupportsUserData::Data { - public: - explicit IDUserData(int32_t id) : id_(id) {} - - [[nodiscard]] auto id() { return id_; } - - private: - int32_t id_; -}; - -} // namespace - TrackableObjectBase::TrackableObjectBase() { // TODO(zcbenz): Make TrackedObject work in renderer process. DCHECK(electron::IsBrowserProcess()) @@ -46,21 +26,4 @@ void TrackableObjectBase::Destroy() { delete this; } -void TrackableObjectBase::AttachAsUserData(base::SupportsUserData* wrapped) { - wrapped->SetUserData(kTrackedObjectKey, - std::make_unique(weak_map_id_)); -} - -// static -int32_t TrackableObjectBase::GetIDFromWrappedClass( - base::SupportsUserData* wrapped) { - if (wrapped) { - auto* id = - static_cast(wrapped->GetUserData(kTrackedObjectKey)); - if (id) - return id->id(); - } - return 0; -} - } // namespace gin_helper diff --git a/shell/common/gin_helper/trackable_object.h b/shell/common/gin_helper/trackable_object.h index 41e5406327..9a1e949061 100644 --- a/shell/common/gin_helper/trackable_object.h +++ b/shell/common/gin_helper/trackable_object.h @@ -12,10 +12,6 @@ #include "shell/common/gin_helper/event_emitter.h" #include "shell/common/key_weak_map.h" -namespace base { -class SupportsUserData; -} - namespace gin_helper { // Users should use TrackableObject instead. @@ -30,12 +26,6 @@ class TrackableObjectBase : public CleanedUpAtExit { // The ID in weak map. [[nodiscard]] constexpr int32_t weak_map_id() const { return weak_map_id_; } - // Wrap TrackableObject into a class that SupportsUserData. - void AttachAsUserData(base::SupportsUserData* wrapped); - - // Get the weak_map_id from SupportsUserData. - static int32_t GetIDFromWrappedClass(base::SupportsUserData* wrapped); - protected: ~TrackableObjectBase() override; @@ -90,15 +80,6 @@ class TrackableObject : public TrackableObjectBase, public EventEmitter { return self; } - // Finds out the TrackableObject from the class it wraps. - static T* FromWrappedClass(v8::Isolate* isolate, - base::SupportsUserData* wrapped) { - int32_t id = GetIDFromWrappedClass(wrapped); - if (!id) - return nullptr; - return FromWeakMapID(isolate, id); - } - // Returns all objects in this class's weak map. static std::vector> GetAll(v8::Isolate* isolate) { if (weak_map_)