From 900400e442ffc0054eec48da8d6d013a9629b60e Mon Sep 17 00:00:00 2001 From: "trop[bot]" <37223003+trop[bot]@users.noreply.github.com> Date: Fri, 15 Nov 2024 13:09:34 +0100 Subject: [PATCH] fix: `WebContentsView` removal should compare directly (#44670) * fix: WebContentsView removal should compare directly Co-authored-by: Shelley Vohr * fixup view comparision Co-authored-by: John Kleinschmidt * chore: use erase_if Co-authored-by: John Kleinschmidt * Apply review suggestions Co-authored-by: John Kleinschmidt --------- Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr Co-authored-by: John Kleinschmidt --- shell/browser/api/electron_api_view.cc | 41 ++++++++++---------------- shell/browser/api/electron_api_view.h | 4 ++- spec/api-web-contents-view-spec.ts | 24 +++++++++++++++ 3 files changed, 43 insertions(+), 26 deletions(-) diff --git a/shell/browser/api/electron_api_view.cc b/shell/browser/api/electron_api_view.cc index c65f80b164fe..1f337426a055 100644 --- a/shell/browser/api/electron_api_view.cc +++ b/shell/browser/api/electron_api_view.cc @@ -124,15 +124,6 @@ struct Converter { } }; -template <> -struct Converter { - static bool FromV8(v8::Isolate* isolate, - v8::Local val, - electron::api::View* out) { - return gin::ConvertFromV8(isolate, val, &out); - } -}; - template <> struct Converter { static v8::Local ToV8(v8::Isolate* isolate, @@ -196,7 +187,10 @@ View::~View() { void View::ReorderChildView(gin::Handle child, size_t index) { view_->ReorderChildView(child->view(), index); - const auto i = base::ranges::find(child_views_, child.ToV8()); + const auto i = + std::ranges::find_if(child_views_, [&](const ChildPair& child_view) { + return child_view.first == child->view(); + }); DCHECK(i != child_views_.end()); // If |view| is already at the desired position, there's nothing to do. @@ -240,8 +234,9 @@ void View::AddChildViewAt(gin::Handle child, return; } - child_views_.emplace(child_views_.begin() + index, // index - isolate(), child->GetWrapper()); // v8::Global(args...) + child_views_.emplace(child_views_.begin() + index, // index + child->view(), + v8::Global(isolate(), child->GetWrapper())); #if BUILDFLAG(IS_MAC) // Disable the implicit CALayer animations that happen by default when adding // or removing sublayers. @@ -261,7 +256,10 @@ void View::RemoveChildView(gin::Handle child) { if (!view_) return; - const auto it = base::ranges::find(child_views_, child.ToV8()); + const auto it = + std::ranges::find_if(child_views_, [&](const ChildPair& child_view) { + return child_view.first == child->view(); + }); if (it != child_views_.end()) { #if BUILDFLAG(IS_MAC) ScopedCAActionDisabler disable_animations; @@ -339,8 +337,8 @@ std::vector> View::GetChildren() { v8::Isolate* isolate = JavascriptEnvironment::GetIsolate(); - for (auto& child_view : child_views_) - ret.push_back(child_view.Get(isolate)); + for (auto& [view, global] : child_views_) + ret.push_back(global.Get(isolate)); return ret; } @@ -400,16 +398,9 @@ void View::OnViewIsDeleting(views::View* observed_view) { } void View::OnChildViewRemoved(views::View* observed_view, views::View* child) { - v8::Isolate* isolate = JavascriptEnvironment::GetIsolate(); - auto it = std::ranges::find_if( - child_views_, [&](const v8::Global& child_view) { - View current_view; - gin::ConvertFromV8(isolate, child_view.Get(isolate), ¤t_view); - return current_view.view()->GetID() == child->GetID(); - }); - if (it != child_views_.end()) { - child_views_.erase(it); - } + std::erase_if(child_views_, [child](const ChildPair& child_view) { + return child_view.first == child; + }); } // static diff --git a/shell/browser/api/electron_api_view.h b/shell/browser/api/electron_api_view.h index 1b0a3f815a6f..9ae61655eb7f 100644 --- a/shell/browser/api/electron_api_view.h +++ b/shell/browser/api/electron_api_view.h @@ -21,6 +21,8 @@ class Handle; namespace electron::api { +using ChildPair = std::pair, v8::Global>; + class View : public gin_helper::EventEmitter, private views::ViewObserver { public: @@ -69,7 +71,7 @@ class View : public gin_helper::EventEmitter, void ApplyBorderRadius(); void ReorderChildView(gin::Handle child, size_t index); - std::vector> child_views_; + std::vector child_views_; std::optional border_radius_; bool delete_view_ = true; diff --git a/spec/api-web-contents-view-spec.ts b/spec/api-web-contents-view-spec.ts index 958775067e03..f98bdd20a9c0 100644 --- a/spec/api-web-contents-view-spec.ts +++ b/spec/api-web-contents-view-spec.ts @@ -101,6 +101,30 @@ describe('WebContentsView', () => { expect(w.contentView.children).to.deep.equal([wcv3, wcv1, wcv2]); }); + it('handle removal and re-addition of children', () => { + const w = new BaseWindow({ show: false }); + const cv = new View(); + w.setContentView(cv); + + const wcv1 = new WebContentsView(); + const wcv2 = new WebContentsView(); + + expect(w.contentView.children).to.deep.equal([]); + + w.contentView.addChildView(wcv1); + w.contentView.addChildView(wcv2); + + expect(w.contentView.children).to.deep.equal([wcv1, wcv2]); + + w.contentView.removeChildView(wcv1); + + expect(w.contentView.children).to.deep.equal([wcv2]); + + w.contentView.addChildView(wcv1); + + expect(w.contentView.children).to.deep.equal([wcv2, wcv1]); + }); + function triggerGCByAllocation () { const arr = []; for (let i = 0; i < 1000000; i++) {