diff --git a/shell/browser/api/electron_api_base_window.cc b/shell/browser/api/electron_api_base_window.cc index 3a87a11d22f06..0a933a048766e 100644 --- a/shell/browser/api/electron_api_base_window.cc +++ b/shell/browser/api/electron_api_base_window.cc @@ -753,61 +753,52 @@ void BaseWindow::SetParentWindow(v8::Local value, } } -void BaseWindow::SetBrowserView(v8::Local value) { +void BaseWindow::SetBrowserView( + absl::optional> browser_view) { ResetBrowserViews(); - AddBrowserView(value); -} - -void BaseWindow::AddBrowserView(v8::Local value) { - gin::Handle 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()) { - // If we're reparenting a BrowserView, ensure that it's detached from - // its previous owner window. - auto* owner_window = browser_view->owner_window(); - if (owner_window && owner_window != window_.get()) { - 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); + if (browser_view) + AddBrowserView(*browser_view); +} + +void BaseWindow::AddBrowserView(gin::Handle browser_view) { + auto iter = browser_views_.find(browser_view->ID()); + if (iter == browser_views_.end()) { + // If we're reparenting a BrowserView, ensure that it's detached from + // its previous owner window. + BaseWindow* owner_window = browser_view->owner_window(); + if (owner_window) { + // iter == browser_views_.end() should imply owner_window != this. + DCHECK_NE(owner_window, this); + owner_window->RemoveBrowserView(browser_view); + browser_view->SetOwnerWindow(nullptr); } + + window_->AddBrowserView(browser_view->view()); + browser_view->SetOwnerWindow(this); + browser_views_[browser_view->ID()].Reset(isolate(), browser_view.ToV8()); } } -void BaseWindow::RemoveBrowserView(v8::Local value) { - gin::Handle 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); - } +void BaseWindow::RemoveBrowserView(gin::Handle browser_view) { + auto iter = browser_views_.find(browser_view->ID()); + if (iter != browser_views_.end()) { + window_->RemoveBrowserView(browser_view->view()); + browser_view->SetOwnerWindow(nullptr); + iter->second.Reset(); + browser_views_.erase(iter); } } -void BaseWindow::SetTopBrowserView(v8::Local value, +void BaseWindow::SetTopBrowserView(gin::Handle browser_view, gin_helper::Arguments* args) { - gin::Handle browser_view; - if (value->IsObject() && - gin::ConvertFromV8(isolate(), value, &browser_view)) { - auto* owner_window = browser_view->owner_window(); - auto get_that_view = browser_views_.find(browser_view->ID()); - 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()); + 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 { @@ -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(); - if (owner_window && owner_window == window_.get()) { - browser_view->SetOwnerWindow(nullptr); - owner_window->RemoveBrowserView(browser_view->view()); - } + BaseWindow* owner_window = browser_view->owner_window(); + DCHECK_EQ(owner_window, this); + browser_view->SetOwnerWindow(nullptr); + window_->RemoveBrowserView(browser_view->view()); + browser_view->SetOwnerWindow(nullptr); } item.second.Reset(); diff --git a/shell/browser/api/electron_api_base_window.h b/shell/browser/api/electron_api_base_window.h index 482f510f4ed40..8ec83b0947407 100644 --- a/shell/browser/api/electron_api_base_window.h +++ b/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, public NativeWindowObserver { @@ -173,10 +174,11 @@ class BaseWindow : public gin_helper::TrackableObject, void SetMenu(v8::Isolate* isolate, v8::Local menu); void RemoveMenu(); void SetParentWindow(v8::Local value, gin_helper::Arguments* args); - virtual void SetBrowserView(v8::Local value); - virtual void AddBrowserView(v8::Local value); - virtual void RemoveBrowserView(v8::Local value); - virtual void SetTopBrowserView(v8::Local value, + virtual void SetBrowserView( + absl::optional> browser_view); + virtual void AddBrowserView(gin::Handle browser_view); + virtual void RemoveBrowserView(gin::Handle browser_view); + virtual void SetTopBrowserView(gin::Handle browser_view, gin_helper::Arguments* args); virtual std::vector> GetBrowserViews() const; virtual void ResetBrowserViews(); diff --git a/shell/browser/api/electron_api_browser_view.cc b/shell/browser/api/electron_api_browser_view.cc index eb0ac8e908959..732d5ee45b18e 100644 --- a/shell/browser/api/electron_api_browser_view.cc +++ b/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; } diff --git a/shell/browser/api/electron_api_browser_view.h b/shell/browser/api/electron_api_browser_view.h index 6caacf41c5c79..7d480874cb9b8 100644 --- a/shell/browser/api/electron_api_browser_view.h +++ b/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, public gin_helper::Constructible, @@ -51,9 +52,9 @@ class BrowserView : public gin::Wrappable, 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, class WebContents* api_web_contents_ = nullptr; std::unique_ptr view_; - base::WeakPtr owner_window_; + base::WeakPtr owner_window_; int32_t id_; }; diff --git a/shell/browser/api/electron_api_browser_window.cc b/shell/browser/api/electron_api_browser_window.cc index 3d1ec969123c8..00de612cecc29 100644 --- a/shell/browser/api/electron_api_browser_window.cc +++ b/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 value) { +void BrowserWindow::SetBrowserView( + absl::optional> 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 value) { - BaseWindow::AddBrowserView(value); +void BrowserWindow::AddBrowserView(gin::Handle browser_view) { + BaseWindow::AddBrowserView(browser_view); #if BUILDFLAG(IS_MAC) UpdateDraggableRegions(draggable_regions_); #endif } -void BrowserWindow::RemoveBrowserView(v8::Local value) { - BaseWindow::RemoveBrowserView(value); +void BrowserWindow::RemoveBrowserView(gin::Handle browser_view) { + BaseWindow::RemoveBrowserView(browser_view); #if BUILDFLAG(IS_MAC) UpdateDraggableRegions(draggable_regions_); #endif } -void BrowserWindow::SetTopBrowserView(v8::Local value, +void BrowserWindow::SetTopBrowserView(gin::Handle browser_view, gin_helper::Arguments* args) { - BaseWindow::SetTopBrowserView(value, args); + BaseWindow::SetTopBrowserView(browser_view, args); #if BUILDFLAG(IS_MAC) UpdateDraggableRegions(draggable_regions_); #endif diff --git a/shell/browser/api/electron_api_browser_window.h b/shell/browser/api/electron_api_browser_window.h index 91195053495ba..6bf2591771d7a 100644 --- a/shell/browser/api/electron_api_browser_window.h +++ b/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 value) override; - void AddBrowserView(v8::Local value) override; - void RemoveBrowserView(v8::Local value) override; - void SetTopBrowserView(v8::Local value, + void SetBrowserView( + absl::optional> browser_view) override; + void AddBrowserView(gin::Handle browser_view) override; + void RemoveBrowserView(gin::Handle browser_view) override; + void SetTopBrowserView(gin::Handle browser_view, gin_helper::Arguments* args) override; void ResetBrowserViews() override; void SetVibrancy(v8::Isolate* isolate, v8::Local value) override; diff --git a/spec/api-browser-view-spec.ts b/spec/api-browser-view-spec.ts index d65fa01b6f814..e8b2c05b1beb3 100644 --- a/spec/api-browser-view-spec.ts +++ b/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;