refactor: ginify BrowserView (#23578)

docs/api/browser-view.md

@@ -28,25 +28,6 @@ view.webContents.loadURL('https://electronjs.org')
 * `options` Object (optional)
   * `webPreferences` Object (optional) - See [BrowserWindow](browser-window.md).
 
-### Static Methods
-
-#### `BrowserView.getAllViews()`
-
-Returns `BrowserView[]` - An array of all opened BrowserViews.
-
-#### `BrowserView.fromWebContents(webContents)`
-
-* `webContents` [WebContents](web-contents.md)
-
-Returns `BrowserView | null` - The BrowserView that owns the given `webContents`
-or `null` if the contents are not owned by a BrowserView.
-
-#### `BrowserView.fromId(id)`
-
-* `id` Integer
-
-Returns `BrowserView` - The view with the given `id`.
-
 ### Instance Properties
 
 Objects created with `new BrowserView` have the following properties:
@@ -55,10 +36,6 @@ Objects created with `new BrowserView` have the following properties:
 
 A [`WebContents`](web-contents.md) object owned by this view.
 
-#### `view.id` _Experimental_
-
-A `Integer` representing the unique ID of the view.
-
 ### Instance Methods
 
 Objects created with `new BrowserView` have the following instance methods:

lib/browser/api/browser-view.ts

@@ -1,15 +1,3 @@
-import { EventEmitter } from 'events';
-
 const { BrowserView } = process._linkedBinding('electron_browser_browser_view');
 
-Object.setPrototypeOf(BrowserView.prototype, EventEmitter.prototype);
-
-BrowserView.fromWebContents = (webContents: Electron.WebContents) => {
-  for (const view of BrowserView.getAllViews()) {
-    if (view.webContents.equal(webContents)) return view;
-  }
-
-  return null;
-};
-
 export default BrowserView;

shell/browser/api/electron_api_base_window.cc

@@ -738,11 +738,11 @@ void BaseWindow::AddBrowserView(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->weak_map_id());
+    auto get_that_view = browser_views_.find(browser_view->ID());
     if (get_that_view == browser_views_.end()) {
       window_->AddBrowserView(browser_view->view());
       browser_view->web_contents()->SetOwnerWindow(window_.get());
-      browser_views_[browser_view->weak_map_id()].Reset(isolate(), value);
+      browser_views_[browser_view->ID()].Reset(isolate(), value);
     }
   }
 }
@@ -751,7 +751,7 @@ 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->weak_map_id());
+    auto get_that_view = browser_views_.find(browser_view->ID());
     if (get_that_view != browser_views_.end()) {
       window_->RemoveBrowserView(browser_view->view());
       browser_view->web_contents()->SetOwnerWindow(nullptr);

shell/browser/api/electron_api_browser_view.cc

@@ -52,12 +52,24 @@ struct Converter<electron::AutoResizeFlags> {
 
 }  // namespace gin
 
+namespace {
+
+int32_t GetNextId() {
+  static int32_t next_id = 1;
+  return next_id++;
+}
+
+}  // namespace
+
 namespace electron {
 
 namespace api {
 
+gin::WrapperInfo BrowserView::kWrapperInfo = {gin::kEmbedderNativeGin};
+
 BrowserView::BrowserView(gin::Arguments* args,
-                         const gin_helper::Dictionary& options) {
+                         const gin_helper::Dictionary& options)
+    : id_(GetNextId()) {
   v8::Isolate* isolate = args->isolate();
   gin_helper::Dictionary web_preferences =
       gin::Dictionary::CreateEmpty(isolate);
@@ -72,8 +84,6 @@ BrowserView::BrowserView(gin::Arguments* args,
 
   view_.reset(
       NativeBrowserView::Create(api_web_contents_->managed_web_contents()));
-
-  InitWithArgs(args);
 }
 
 BrowserView::~BrowserView() {
@@ -87,24 +97,24 @@ BrowserView::~BrowserView() {
 void BrowserView::WebContentsDestroyed() {
   api_web_contents_ = nullptr;
   web_contents_.Reset();
+  Unpin();
 }
 
 // static
-gin_helper::WrappableBase* BrowserView::New(gin_helper::ErrorThrower thrower,
-                                            gin::Arguments* args) {
+gin::Handle<BrowserView> BrowserView::New(gin_helper::ErrorThrower thrower,
+                                          gin::Arguments* args) {
   if (!Browser::Get()->is_ready()) {
     thrower.ThrowError("Cannot create BrowserView before app is ready");
-    return nullptr;
+    return gin::Handle<BrowserView>();
   }
 
   gin::Dictionary options = gin::Dictionary::CreateEmpty(args->isolate());
   args->GetNext(&options);
 
-  return new BrowserView(args, options);
-}
-
-int32_t BrowserView::ID() const {
-  return weak_map_id();
+  auto handle =
+      gin::CreateHandle(args->isolate(), new BrowserView(args, options));
+  handle->Pin(args->isolate());
+  return handle;
 }
 
 void BrowserView::SetAutoResize(AutoResizeFlags flags) {
@@ -123,26 +133,25 @@ void BrowserView::SetBackgroundColor(const std::string& color_name) {
   view_->SetBackgroundColor(ParseHexColor(color_name));
 }
 
-v8::Local<v8::Value> BrowserView::GetWebContents() {
+v8::Local<v8::Value> BrowserView::GetWebContents(v8::Isolate* isolate) {
   if (web_contents_.IsEmpty()) {
-    return v8::Null(isolate());
+    return v8::Null(isolate);
   }
 
-  return v8::Local<v8::Value>::New(isolate(), web_contents_);
+  return v8::Local<v8::Value>::New(isolate, web_contents_);
 }
 
 // static
-void BrowserView::BuildPrototype(v8::Isolate* isolate,
-                                 v8::Local<v8::FunctionTemplate> prototype) {
-  prototype->SetClassName(gin::StringToV8(isolate, "BrowserView"));
-  gin_helper::Destroyable::MakeDestroyable(isolate, prototype);
-  gin_helper::ObjectTemplateBuilder(isolate, prototype->PrototypeTemplate())
+v8::Local<v8::ObjectTemplate> BrowserView::FillObjectTemplate(
+    v8::Isolate* isolate,
+    v8::Local<v8::ObjectTemplate> templ) {
+  return gin::ObjectTemplateBuilder(isolate, "BrowserView", templ)
       .SetMethod("setAutoResize", &BrowserView::SetAutoResize)
       .SetMethod("setBounds", &BrowserView::SetBounds)
       .SetMethod("getBounds", &BrowserView::GetBounds)
       .SetMethod("setBackgroundColor", &BrowserView::SetBackgroundColor)
       .SetProperty("webContents", &BrowserView::GetWebContents)
-      .SetProperty("id", &BrowserView::ID);
+      .Build();
 }
 
 }  // namespace api
@@ -158,16 +167,9 @@ void Initialize(v8::Local<v8::Object> exports,
                 v8::Local<v8::Context> context,
                 void* priv) {
   v8::Isolate* isolate = context->GetIsolate();
-  BrowserView::SetConstructor(isolate, base::BindRepeating(&BrowserView::New));
 
-  gin_helper::Dictionary browser_view(isolate,
-                                      BrowserView::GetConstructor(isolate)
-                                          ->GetFunction(context)
-                                          .ToLocalChecked());
-  browser_view.SetMethod("fromId", &BrowserView::FromWeakMapID);
-  browser_view.SetMethod("getAllViews", &BrowserView::GetAll);
   gin_helper::Dictionary dict(isolate, exports);
-  dict.Set("BrowserView", browser_view);
+  dict.Set("BrowserView", BrowserView::GetConstructor(context));
 }
 
 }  // namespace

shell/browser/api/electron_api_browser_view.h

@@ -10,9 +10,11 @@
 
 #include "content/public/browser/web_contents_observer.h"
 #include "gin/handle.h"
+#include "gin/wrappable.h"
 #include "shell/browser/native_browser_view.h"
+#include "shell/common/gin_helper/constructible.h"
 #include "shell/common/gin_helper/error_thrower.h"
-#include "shell/common/gin_helper/trackable_object.h"
+#include "shell/common/gin_helper/pinnable.h"
 
 namespace gfx {
 class Rect;
@@ -30,19 +32,25 @@ namespace api {
 
 class WebContents;
 
-class BrowserView : public gin_helper::TrackableObject<BrowserView>,
+class BrowserView : public gin::Wrappable<BrowserView>,
+                    public gin_helper::Constructible<BrowserView>,
+                    public gin_helper::Pinnable<BrowserView>,
                     public content::WebContentsObserver {
  public:
-  static gin_helper::WrappableBase* New(gin_helper::ErrorThrower thrower,
-                                        gin::Arguments* args);
+  // gin_helper::Constructible
+  static gin::Handle<BrowserView> New(gin_helper::ErrorThrower thrower,
+                                      gin::Arguments* args);
+  static v8::Local<v8::ObjectTemplate> FillObjectTemplate(
+      v8::Isolate*,
+      v8::Local<v8::ObjectTemplate>);
 
-  static void BuildPrototype(v8::Isolate* isolate,
-                             v8::Local<v8::FunctionTemplate> prototype);
+  // gin::Wrappable
+  static gin::WrapperInfo kWrapperInfo;
 
   WebContents* web_contents() const { return api_web_contents_; }
   NativeBrowserView* view() const { return view_.get(); }
 
-  int32_t ID() const;
+  int32_t ID() const { return id_; }
 
  protected:
   BrowserView(gin::Arguments* args, const gin_helper::Dictionary& options);
@@ -56,13 +64,15 @@ class BrowserView : public gin_helper::TrackableObject<BrowserView>,
   void SetBounds(const gfx::Rect& bounds);
   gfx::Rect GetBounds();
   void SetBackgroundColor(const std::string& color_name);
-  v8::Local<v8::Value> GetWebContents();
+  v8::Local<v8::Value> GetWebContents(v8::Isolate*);
 
   v8::Global<v8::Value> web_contents_;
   class WebContents* api_web_contents_ = nullptr;
 
   std::unique_ptr<NativeBrowserView> view_;
 
+  int32_t id_;
+
   DISALLOW_COPY_AND_ASSIGN(BrowserView);
 };
 

shell/browser/api/electron_api_browser_window.cc

@@ -276,6 +276,10 @@ void BrowserWindow::OnCloseButtonClicked(bool* prevent_default) {
 }
 
 void BrowserWindow::OnWindowClosed() {
+  // We need to reset the browser views before we call Cleanup, because the
+  // browser views are child views of the main web contents view, which will be
+  // deleted by Cleanup.
+  BaseWindow::ResetBrowserViews();
   Cleanup();
   // See BaseWindow::OnWindowClosed on why calling InvalidateWeakPtrs.
   weak_factory_.InvalidateWeakPtrs();

shell/common/gin_helper/constructible.h

@@ -5,7 +5,9 @@
 #ifndef SHELL_COMMON_GIN_HELPER_CONSTRUCTIBLE_H_
 #define SHELL_COMMON_GIN_HELPER_CONSTRUCTIBLE_H_
 
+#include "gin/per_isolate_data.h"
 #include "gin/wrappable.h"
+#include "shell/browser/event_emitter_mixin.h"
 #include "shell/common/gin_helper/function_template_extensions.h"
 
 namespace gin_helper {

spec-main/api-browser-view-spec.ts

@@ -1,9 +1,9 @@
 import { expect } from 'chai';
-import * as ChildProcess from 'child_process';
 import * as path from 'path';
 import { emittedOnce } from './events-helpers';
-import { BrowserView, BrowserWindow } from 'electron/main';
+import { BrowserView, BrowserWindow, webContents } from 'electron/main';
 import { closeWindow } from './window-helpers';
+import { defer, startRemoteControlApp } from './spec-helpers';
 
 describe('BrowserView module', () => {
   const fixtures = path.resolve(__dirname, '..', 'spec', 'fixtures');
@@ -12,6 +12,7 @@ describe('BrowserView module', () => {
   let view: BrowserView;
 
   beforeEach(() => {
+    expect(webContents.getAllWebContents()).to.have.length(0);
     w = new BrowserWindow({
       show: false,
       width: 400,
@@ -23,27 +24,19 @@ describe('BrowserView module', () => {
   });
 
   afterEach(async () => {
+    const p = emittedOnce(w.webContents, 'destroyed');
     await closeWindow(w);
+    w = null as any;
+    await p;
 
     if (view) {
-      view.destroy();
+      const p = emittedOnce(view.webContents, 'destroyed');
+      (view.webContents as any).destroy();
+      view = null as any;
+      await p;
     }
-  });
 
-  describe('BrowserView.destroy()', () => {
-    it('does not throw', () => {
-      view = new BrowserView();
-      view.destroy();
-    });
-  });
-
-  describe('BrowserView.isDestroyed()', () => {
-    it('returns correct value', () => {
-      view = new BrowserView();
-      expect(view.isDestroyed()).to.be.false('view is destroyed');
-      view.destroy();
-      expect(view.isDestroyed()).to.be.true('view is destroyed');
-    });
+    expect(webContents.getAllWebContents()).to.have.length(0);
   });
 
   describe('BrowserView.setBackgroundColor()', () => {
@@ -119,7 +112,6 @@ describe('BrowserView module', () => {
     it('returns the set view', () => {
       view = new BrowserView();
       w.setBrowserView(view);
-      expect(view.id).to.not.be.null('view id');
 
       const view2 = w.getBrowserView();
       expect(view2!.webContents.id).to.equal(view.webContents.id);
@@ -134,11 +126,13 @@ describe('BrowserView module', () => {
   describe('BrowserWindow.addBrowserView()', () => {
     it('does not throw for valid args', () => {
       const view1 = new BrowserView();
+      defer(() => (view1.webContents as any).destroy());
       w.addBrowserView(view1);
+      defer(() => w.removeBrowserView(view1));
       const view2 = new BrowserView();
+      defer(() => (view2.webContents as any).destroy());
       w.addBrowserView(view2);
-      view1.destroy();
-      view2.destroy();
+      defer(() => w.removeBrowserView(view2));
     });
     it('does not throw if called multiple times with same view', () => {
       view = new BrowserView();
@@ -160,18 +154,18 @@ describe('BrowserView module', () => {
   describe('BrowserWindow.getBrowserViews()', () => {
     it('returns same views as was added', () => {
       const view1 = new BrowserView();
+      defer(() => (view1.webContents as any).destroy());
       w.addBrowserView(view1);
+      defer(() => w.removeBrowserView(view1));
       const view2 = new BrowserView();
+      defer(() => (view2.webContents as any).destroy());
       w.addBrowserView(view2);
+      defer(() => w.removeBrowserView(view2));
 
-      expect(view1.id).to.be.not.null('view id');
       const views = w.getBrowserViews();
       expect(views).to.have.lengthOf(2);
       expect(views[0].webContents.id).to.equal(view1.webContents.id);
       expect(views[1].webContents.id).to.equal(view2.webContents.id);
-
-      view1.destroy();
-      view2.destroy();
     });
   });
 
@@ -188,46 +182,33 @@ describe('BrowserView module', () => {
     });
   });
 
-  describe('BrowserView.fromId()', () => {
-    it('returns the view with given id', () => {
-      view = new BrowserView();
-      w.setBrowserView(view);
-      expect(view.id).to.not.be.null('view id');
-
-      const view2 = BrowserView.fromId(view.id);
-      expect(view2.webContents.id).to.equal(view.webContents.id);
-    });
-  });
-
-  describe('BrowserView.fromWebContents()', () => {
-    it('returns the view with given id', () => {
-      view = new BrowserView();
-      w.setBrowserView(view);
-      expect(view.id).to.not.be.null('view id');
-
-      const view2 = BrowserView.fromWebContents(view.webContents);
-      expect(view2!.webContents.id).to.equal(view.webContents.id);
-    });
-  });
-
-  describe('BrowserView.getAllViews()', () => {
-    it('returns all views', () => {
-      view = new BrowserView();
-      w.setBrowserView(view);
-      expect(view.id).to.not.be.null('view id');
-
-      const views = BrowserView.getAllViews();
-      expect(views).to.be.an('array').that.has.lengthOf(1);
-      expect(views[0].webContents.id).to.equal(view.webContents.id);
-    });
-  });
-
-  describe('new BrowserView()', () => {
+  describe('shutdown behavior', () => {
     it('does not crash on exit', async () => {
-      const appPath = path.join(__dirname, 'fixtures', 'api', 'leak-exit-browserview.js');
-      const electronPath = process.execPath;
-      const appProcess = ChildProcess.spawn(electronPath, [appPath]);
-      const [code] = await emittedOnce(appProcess, 'exit');
+      const rc = await startRemoteControlApp();
+      await rc.remotely(() => {
+        const { BrowserView, app } = require('electron');
+        new BrowserView({})  // eslint-disable-line
+        setTimeout(() => {
+          app.quit();
+        });
+      });
+      const [code] = await emittedOnce(rc.process, 'exit');
+      expect(code).to.equal(0);
+    });
+
+    it('does not crash on exit if added to a browser window', async () => {
+      const rc = await startRemoteControlApp();
+      await rc.remotely(() => {
+        const { app, BrowserView, BrowserWindow } = require('electron');
+        const bv = new BrowserView();
+        bv.webContents.loadURL('about:blank');
+        const bw = new BrowserWindow({ show: false });
+        bw.addBrowserView(bv);
+        setTimeout(() => {
+          app.quit();
+        });
+      });
+      const [code] = await emittedOnce(rc.process, 'exit');
       expect(code).to.equal(0);
     });
   });

spec-main/api-browser-window-spec.ts

@@ -9,7 +9,7 @@ import { AddressInfo } from 'net';
 import { app, BrowserWindow, BrowserView, ipcMain, OnBeforeSendHeadersListenerDetails, protocol, screen, webContents, session, WebContents } from 'electron/main';
 
 import { emittedOnce, emittedUntil } from './events-helpers';
-import { ifit, ifdescribe, delay } from './spec-helpers';
+import { ifit, ifdescribe, defer, delay } from './spec-helpers';
 import { closeWindow, closeAllWindows } from './window-helpers';
 
 const features = process._linkedBinding('electron_common_features');
@@ -1518,16 +1518,19 @@ describe('BrowserWindow module', () => {
       const w = new BrowserWindow({ show: false });
       const bv = new BrowserView();
       w.setBrowserView(bv);
+      defer(() => {
+        w.removeBrowserView(bv);
+        (bv.webContents as any).destroy();
+      });
       expect(BrowserWindow.fromBrowserView(bv)!.id).to.equal(w.id);
-      // if BrowserView isn't explicitly destroyed, it will crash in GC later
-      bv.destroy();
     });
 
     it('returns undefined if not attached', () => {
       const bv = new BrowserView();
+      defer(() => {
+        (bv.webContents as any).destroy();
+      });
       expect(BrowserWindow.fromBrowserView(bv)).to.be.null('BrowserWindow associated with bv');
-      // if BrowserView isn't explicitly destroyed, it will crash in GC later
-      bv.destroy();
     });
   });
 

spec-main/fixtures/api/leak-exit-browserview.js

@@ -1,6 +0,0 @@
-const { BrowserView, app } = require('electron');
-app.whenReady().then(function () {
-  new BrowserView({})  // eslint-disable-line
-
-  app.quit();
-});

spec-main/fixtures/apps/remote-control/main.js

@@ -1,22 +1,25 @@
+const { app } = require('electron');
 const http = require('http');
 const v8 = require('v8');
 
-const server = http.createServer((req, res) => {
-  const chunks = [];
-  req.on('data', chunk => { chunks.push(chunk); });
-  req.on('end', () => {
-    const js = Buffer.concat(chunks).toString('utf8');
-    (async () => {
-      try {
-        const result = await Promise.resolve(eval(js)); // eslint-disable-line no-eval
-        res.end(v8.serialize({ result }));
-      } catch (e) {
-        res.end(v8.serialize({ error: e.stack }));
-      }
-    })();
+app.whenReady().then(() => {
+  const server = http.createServer((req, res) => {
+    const chunks = [];
+    req.on('data', chunk => { chunks.push(chunk); });
+    req.on('end', () => {
+      const js = Buffer.concat(chunks).toString('utf8');
+      (async () => {
+        try {
+          const result = await Promise.resolve(eval(js)); // eslint-disable-line no-eval
+          res.end(v8.serialize({ result }));
+        } catch (e) {
+          res.end(v8.serialize({ error: e.stack }));
+        }
+      })();
+    });
+  }).listen(0, '127.0.0.1', () => {
+    process.stdout.write(`Listening: ${server.address().port}\n`);
   });
-}).listen(0, '127.0.0.1', () => {
-  process.stdout.write(`Listening: ${server.address().port}\n`);
 });
 
 setTimeout(() => {

spec-main/spec-helpers.ts

@@ -53,7 +53,6 @@ class RemoteControlApp {
       req.end();
     });
   }
-
   remotely = (script: Function, ...args: any[]): Promise<any> => {
     return this.remoteEval(`(${script})(...${JSON.stringify(args)})`);
   }