refactor: BrowserView is owned by a BaseWindow (#35511)

shell/browser/api/electron_api_base_window.cc

@@ -753,63 +753,54 @@ void BaseWindow::SetParentWindow(v8::Local<v8::Value> value,
   }
 }
 
-void BaseWindow::SetBrowserView(v8::Local<v8::Value> value) {
+void BaseWindow::SetBrowserView(
+    absl::optional<gin::Handle<BrowserView>> browser_view) {
   ResetBrowserViews();
-  AddBrowserView(value);
+  if (browser_view)
+    AddBrowserView(*browser_view);
 }
 
-void BaseWindow::AddBrowserView(v8::Local<v8::Value> value) {
+void BaseWindow::AddBrowserView(gin::Handle<BrowserView> browser_view) {
-  gin::Handle<BrowserView> browser_view;
+  auto iter = browser_views_.find(browser_view->ID());
-  if (value->IsObject() &&
+  if (iter == browser_views_.end()) {
-      gin::ConvertFromV8(isolate(), value, &browser_view)) {
+    // If we're reparenting a BrowserView, ensure that it's detached from
-    auto get_that_view = browser_views_.find(browser_view->ID());
+    // its previous owner window.
-    if (get_that_view == browser_views_.end()) {
+    BaseWindow* owner_window = browser_view->owner_window();
-      // If we're reparenting a BrowserView, ensure that it's detached from
+    if (owner_window) {
-      // its previous owner window.
+      // iter == browser_views_.end() should imply owner_window != this.
-      auto* owner_window = browser_view->owner_window();
+      DCHECK_NE(owner_window, this);
-      if (owner_window && owner_window != window_.get()) {
+      owner_window->RemoveBrowserView(browser_view);
-        owner_window->RemoveBrowserView(browser_view->view());
-        browser_view->SetOwnerWindow(nullptr);
-      }
-
-      window_->AddBrowserView(browser_view->view());
-      browser_view->SetOwnerWindow(window_.get());
-      browser_views_[browser_view->ID()].Reset(isolate(), value);
-    }
-  }
-}
-
-void BaseWindow::RemoveBrowserView(v8::Local<v8::Value> value) {
-  gin::Handle<BrowserView> browser_view;
-  if (value->IsObject() &&
-      gin::ConvertFromV8(isolate(), value, &browser_view)) {
-    auto get_that_view = browser_views_.find(browser_view->ID());
-    if (get_that_view != browser_views_.end()) {
-      window_->RemoveBrowserView(browser_view->view());
       browser_view->SetOwnerWindow(nullptr);
-      (*get_that_view).second.Reset(isolate(), value);
-      browser_views_.erase(get_that_view);
     }
+
+    window_->AddBrowserView(browser_view->view());
+    browser_view->SetOwnerWindow(this);
+    browser_views_[browser_view->ID()].Reset(isolate(), browser_view.ToV8());
   }
 }
 
-void BaseWindow::SetTopBrowserView(v8::Local<v8::Value> value,
+void BaseWindow::RemoveBrowserView(gin::Handle<BrowserView> browser_view) {
-                                   gin_helper::Arguments* args) {
+  auto iter = browser_views_.find(browser_view->ID());
-  gin::Handle<BrowserView> browser_view;
+  if (iter != browser_views_.end()) {
-  if (value->IsObject() &&
+    window_->RemoveBrowserView(browser_view->view());
-      gin::ConvertFromV8(isolate(), value, &browser_view)) {
+    browser_view->SetOwnerWindow(nullptr);
-    auto* owner_window = browser_view->owner_window();
+    iter->second.Reset();
-    auto get_that_view = browser_views_.find(browser_view->ID());
+    browser_views_.erase(iter);
-    if (get_that_view == browser_views_.end() ||
-        (owner_window && owner_window != window_.get())) {
-      args->ThrowError("Given BrowserView is not attached to the window");
-      return;
-    }
-
-    window_->SetTopBrowserView(browser_view->view());
   }
 }
 
+void BaseWindow::SetTopBrowserView(gin::Handle<BrowserView> browser_view,
+                                   gin_helper::Arguments* args) {
+  BaseWindow* owner_window = browser_view->owner_window();
+  auto iter = browser_views_.find(browser_view->ID());
+  if (iter == browser_views_.end() || (owner_window && owner_window != this)) {
+    args->ThrowError("Given BrowserView is not attached to the window");
+    return;
+  }
+
+  window_->SetTopBrowserView(browser_view->view());
+}
+
 std::string BaseWindow::GetMediaSourceId() const {
   return window_->GetDesktopMediaID().ToString();
 }
@@ -1127,11 +1118,11 @@ void BaseWindow::ResetBrowserViews() {
         !browser_view.IsEmpty()) {
       // There's a chance that the BrowserView may have been reparented - only
       // reset if the owner window is *this* window.
-      auto* owner_window = browser_view->owner_window();
+      BaseWindow* owner_window = browser_view->owner_window();
-      if (owner_window && owner_window == window_.get()) {
+      DCHECK_EQ(owner_window, this);
-        browser_view->SetOwnerWindow(nullptr);
+      browser_view->SetOwnerWindow(nullptr);
-        owner_window->RemoveBrowserView(browser_view->view());
+      window_->RemoveBrowserView(browser_view->view());
-      }
+      browser_view->SetOwnerWindow(nullptr);
     }
 
     item.second.Reset();

shell/browser/api/electron_api_base_window.h

@@ -22,6 +22,7 @@
 namespace electron::api {
 
 class View;
+class BrowserView;
 
 class BaseWindow : public gin_helper::TrackableObject<BaseWindow>,
                    public NativeWindowObserver {
@@ -173,10 +174,11 @@ class BaseWindow : public gin_helper::TrackableObject<BaseWindow>,
   void SetMenu(v8::Isolate* isolate, v8::Local<v8::Value> menu);
   void RemoveMenu();
   void SetParentWindow(v8::Local<v8::Value> value, gin_helper::Arguments* args);
-  virtual void SetBrowserView(v8::Local<v8::Value> value);
+  virtual void SetBrowserView(
-  virtual void AddBrowserView(v8::Local<v8::Value> value);
+      absl::optional<gin::Handle<BrowserView>> browser_view);
-  virtual void RemoveBrowserView(v8::Local<v8::Value> value);
+  virtual void AddBrowserView(gin::Handle<BrowserView> browser_view);
-  virtual void SetTopBrowserView(v8::Local<v8::Value> value,
+  virtual void RemoveBrowserView(gin::Handle<BrowserView> browser_view);
+  virtual void SetTopBrowserView(gin::Handle<BrowserView> browser_view,
                                  gin_helper::Arguments* args);
   virtual std::vector<v8::Local<v8::Value>> GetBrowserViews() const;
   virtual void ResetBrowserViews();

shell/browser/api/electron_api_browser_view.cc

@@ -8,6 +8,7 @@
 
 #include "content/browser/renderer_host/render_widget_host_view_base.h"  // nogncheck
 #include "content/public/browser/render_widget_host_view.h"
+#include "shell/browser/api/electron_api_base_window.h"
 #include "shell/browser/api/electron_api_web_contents.h"
 #include "shell/browser/browser.h"
 #include "shell/browser/native_browser_view.h"
@@ -99,10 +100,10 @@ BrowserView::BrowserView(gin::Arguments* args,
       NativeBrowserView::Create(api_web_contents_->inspectable_web_contents()));
 }
 
-void BrowserView::SetOwnerWindow(NativeWindow* window) {
+void BrowserView::SetOwnerWindow(BaseWindow* window) {
   // Ensure WebContents and BrowserView owner windows are in sync.
   if (web_contents())
-    web_contents()->SetOwnerWindow(window);
+    web_contents()->SetOwnerWindow(window ? window->window() : nullptr);
 
   owner_window_ = window ? window->GetWeakPtr() : nullptr;
 }

shell/browser/api/electron_api_browser_view.h

@@ -31,6 +31,7 @@ class Dictionary;
 namespace electron::api {
 
 class WebContents;
+class BaseWindow;
 
 class BrowserView : public gin::Wrappable<BrowserView>,
                     public gin_helper::Constructible<BrowserView>,
@@ -51,9 +52,9 @@ class BrowserView : public gin::Wrappable<BrowserView>,
   WebContents* web_contents() const { return api_web_contents_; }
   NativeBrowserView* view() const { return view_.get(); }
 
-  NativeWindow* owner_window() const { return owner_window_.get(); }
+  BaseWindow* owner_window() const { return owner_window_.get(); }
 
-  void SetOwnerWindow(NativeWindow* window);
+  void SetOwnerWindow(BaseWindow* window);
 
   int32_t ID() const { return id_; }
 
@@ -83,7 +84,7 @@ class BrowserView : public gin::Wrappable<BrowserView>,
   class WebContents* api_web_contents_ = nullptr;
 
   std::unique_ptr<NativeBrowserView> view_;
-  base::WeakPtr<NativeWindow> owner_window_;
+  base::WeakPtr<BaseWindow> owner_window_;
 
   int32_t id_;
 };

shell/browser/api/electron_api_browser_window.cc

@@ -395,31 +395,33 @@ void BrowserWindow::SetBackgroundColor(const std::string& color_name) {
   }
 }
 
-void BrowserWindow::SetBrowserView(v8::Local<v8::Value> value) {
+void BrowserWindow::SetBrowserView(
+    absl::optional<gin::Handle<BrowserView>> browser_view) {
   BaseWindow::ResetBrowserViews();
-  BaseWindow::AddBrowserView(value);
+  if (browser_view)
+    BaseWindow::AddBrowserView(*browser_view);
 #if BUILDFLAG(IS_MAC)
   UpdateDraggableRegions(draggable_regions_);
 #endif
 }
 
-void BrowserWindow::AddBrowserView(v8::Local<v8::Value> value) {
+void BrowserWindow::AddBrowserView(gin::Handle<BrowserView> browser_view) {
-  BaseWindow::AddBrowserView(value);
+  BaseWindow::AddBrowserView(browser_view);
 #if BUILDFLAG(IS_MAC)
   UpdateDraggableRegions(draggable_regions_);
 #endif
 }
 
-void BrowserWindow::RemoveBrowserView(v8::Local<v8::Value> value) {
+void BrowserWindow::RemoveBrowserView(gin::Handle<BrowserView> browser_view) {
-  BaseWindow::RemoveBrowserView(value);
+  BaseWindow::RemoveBrowserView(browser_view);
 #if BUILDFLAG(IS_MAC)
   UpdateDraggableRegions(draggable_regions_);
 #endif
 }
 
-void BrowserWindow::SetTopBrowserView(v8::Local<v8::Value> value,
+void BrowserWindow::SetTopBrowserView(gin::Handle<BrowserView> browser_view,
                                       gin_helper::Arguments* args) {
-  BaseWindow::SetTopBrowserView(value, args);
+  BaseWindow::SetTopBrowserView(browser_view, args);
 #if BUILDFLAG(IS_MAC)
   UpdateDraggableRegions(draggable_regions_);
 #endif

shell/browser/api/electron_api_browser_window.h

@@ -82,10 +82,11 @@ class BrowserWindow : public BaseWindow,
   void Focus() override;
   void Blur() override;
   void SetBackgroundColor(const std::string& color_name) override;
-  void SetBrowserView(v8::Local<v8::Value> value) override;
+  void SetBrowserView(
-  void AddBrowserView(v8::Local<v8::Value> value) override;
+      absl::optional<gin::Handle<BrowserView>> browser_view) override;
-  void RemoveBrowserView(v8::Local<v8::Value> value) override;
+  void AddBrowserView(gin::Handle<BrowserView> browser_view) override;
-  void SetTopBrowserView(v8::Local<v8::Value> value,
+  void RemoveBrowserView(gin::Handle<BrowserView> browser_view) override;
+  void SetTopBrowserView(gin::Handle<BrowserView> browser_view,
                          gin_helper::Arguments* args) override;
   void ResetBrowserViews() override;
   void SetVibrancy(v8::Isolate* isolate, v8::Local<v8::Value> value) override;

spec/api-browser-view-spec.ts

@@ -30,7 +30,7 @@ describe('BrowserView module', () => {
     w = null as any;
     await p;
 
-    if (view) {
+    if (view && view.webContents) {
       const p = emittedOnce(view.webContents, 'destroyed');
       (view.webContents as any).destroy();
       view = null as any;