From 2bd3a9fe6555c1fb8c5f882dbb182d07701d9a92 Mon Sep 17 00:00:00 2001
From: John Kleinschmidt <jkleinsc@electronjs.org>
Date: Fri, 15 Nov 2024 07:09:47 -0500
Subject: [PATCH] fix: `WebContentsView` removal should compare directly
 (#44673)

fix: `WebContentsView` removal should compare directly (#44656)

* fix: WebContentsView removal should compare directly

* fixup view comparision

* chore: use erase_if

* Apply review suggestions

---------

Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
---
 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 465c2980fae2..037f2f384502 100644
--- a/shell/browser/api/electron_api_view.cc
+++ b/shell/browser/api/electron_api_view.cc
@@ -122,15 +122,6 @@ struct Converter<views::FlexAllocationOrder> {
   }
 };
 
-template <>
-struct Converter<electron::api::View> {
-  static bool FromV8(v8::Isolate* isolate,
-                     v8::Local<v8::Value> val,
-                     electron::api::View* out) {
-    return gin::ConvertFromV8(isolate, val, &out);
-  }
-};
-
 template <>
 struct Converter<views::SizeBound> {
   static v8::Local<v8::Value> ToV8(v8::Isolate* isolate,
@@ -193,7 +184,10 @@ View::~View() {
 void View::ReorderChildView(gin::Handle<View> 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.
@@ -237,8 +231,9 @@ void View::AddChildViewAt(gin::Handle<View> 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<v8::Object>(isolate(), child->GetWrapper()));
 #if BUILDFLAG(IS_MAC)
   // Disable the implicit CALayer animations that happen by default when adding
   // or removing sublayers.
@@ -258,7 +253,10 @@ void View::RemoveChildView(gin::Handle<View> 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;
@@ -336,8 +334,8 @@ std::vector<v8::Local<v8::Value>> 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;
 }
@@ -364,16 +362,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<v8::Object>& child_view) {
-        View current_view;
-        gin::ConvertFromV8(isolate, child_view.Get(isolate), &current_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 4364aca112b5..8d1c71501a94 100644
--- a/shell/browser/api/electron_api_view.h
+++ b/shell/browser/api/electron_api_view.h
@@ -17,6 +17,8 @@
 
 namespace electron::api {
 
+using ChildPair = std::pair<raw_ptr<views::View>, v8::Global<v8::Object>>;
+
 class View : public gin_helper::EventEmitter<View>, public views::ViewObserver {
  public:
   static gin_helper::WrappableBase* New(gin::Arguments* args);
@@ -61,7 +63,7 @@ class View : public gin_helper::EventEmitter<View>, public views::ViewObserver {
  private:
   void ReorderChildView(gin::Handle<View> child, size_t index);
 
-  std::vector<v8::Global<v8::Object>> child_views_;
+  std::vector<ChildPair> child_views_;
 
   bool delete_view_ = true;
   raw_ptr<views::View> view_ = nullptr;
diff --git a/spec/api-web-contents-view-spec.ts b/spec/api-web-contents-view-spec.ts
index 7e256c24853b..bf8282ca2361 100644
--- a/spec/api-web-contents-view-spec.ts
+++ b/spec/api-web-contents-view-spec.ts
@@ -100,6 +100,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++) {