chore: make WebContentsView take webPreferences as parameter (#22997)

* chore: add WebContentsView.webContents helper

* chore: no need to handle webContents option

* chore: Create WebContentsView in C++

* chore: make WebContentsView accept web_preferences

* fix: nativeWindowOpen still passes WebContents to BrowserWindow

* chore: no more need of WebContentsViewRelay

* test: WebContentsView now takes options

* fix: avoid creating 2 constructors

lib/browser/api/browser-window.js

@@ -1,7 +1,7 @@
 'use strict';
 
 const electron = require('electron');
-const { WebContentsView, TopLevelWindow, deprecate } = electron;
+const { TopLevelWindow, deprecate } = electron;
 const { BrowserWindow } = process.electronBinding('window');
 
 Object.setPrototypeOf(BrowserWindow.prototype, TopLevelWindow.prototype);
@@ -13,9 +13,6 @@ BrowserWindow.prototype._init = function () {
   // Avoid recursive require.
   const { app } = electron;
 
-  // Create WebContentsView.
-  this.setContentView(new WebContentsView(this.webContents));
-
   const nativeSetBounds = this.setBounds;
   this.setBounds = (bounds, ...opts) => {
     bounds = {

shell/browser/api/electron_api_browser_window.cc

@@ -12,12 +12,12 @@
 #include "content/browser/web_contents/web_contents_impl.h"  // nogncheck
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
+#include "shell/browser/api/electron_api_web_contents_view.h"
 #include "shell/browser/browser.h"
 #include "shell/browser/unresponsive_suppressor.h"
 #include "shell/browser/web_contents_preferences.h"
 #include "shell/browser/window_list.h"
 #include "shell/common/color_util.h"
-#include "shell/common/gin_converters/value_converter.h"
 #include "shell/common/gin_helper/constructor.h"
 #include "shell/common/gin_helper/dictionary.h"
 #include "shell/common/gin_helper/object_template_builder.h"
@@ -32,8 +32,6 @@ namespace api {
 BrowserWindow::BrowserWindow(gin::Arguments* args,
                              const gin_helper::Dictionary& options)
     : TopLevelWindow(args->isolate(), options), weak_factory_(this) {
-  gin::Handle<class WebContents> web_contents;
-
   // Use options.webPreferences in WebContents.
   v8::Isolate* isolate = args->isolate();
   gin_helper::Dictionary web_preferences =
@@ -60,23 +58,20 @@ BrowserWindow::BrowserWindow(gin::Arguments* args,
     web_preferences.Set(options::kShow, show);
   }
 
-  if (options.Get("webContents", &web_contents) && !web_contents.IsEmpty()) {
-    // Set webPreferences from options if using an existing webContents.
-    // These preferences will be used when the webContent launches new
-    // render processes.
-    auto* existing_preferences =
-        WebContentsPreferences::From(web_contents->web_contents());
-    base::DictionaryValue web_preferences_dict;
-    if (gin::ConvertFromV8(isolate, web_preferences.GetHandle(),
-                           &web_preferences_dict)) {
-      existing_preferences->Clear();
-      existing_preferences->Merge(web_preferences_dict);
-    }
-  } else {
-    // Creates the WebContents used by BrowserWindow.
-    web_contents = WebContents::Create(isolate, web_preferences);
+  // Copy the webContents option to webPreferences. This is only used internally
+  // to implement nativeWindowOpen option.
+  if (options.Get("webContents", &value)) {
+    web_preferences.SetHidden("webContents", value);
   }
 
+  // Creates the WebContentsView.
+  gin::Handle<WebContentsView> web_contents_view =
+      WebContentsView::Create(isolate, web_preferences);
+  DCHECK(web_contents_view.get());
+
+  // Save a reference of the WebContents.
+  gin::Handle<WebContents> web_contents =
+      web_contents_view->GetWebContents(isolate);
   web_contents_.Reset(isolate, web_contents.ToV8());
   api_web_contents_ = web_contents->GetWeakPtr();
   api_web_contents_->AddObserver(this);
@@ -95,6 +90,9 @@ BrowserWindow::BrowserWindow(gin::Arguments* args,
 
   InitWithArgs(args);
 
+  // Install the content view after TopLevelWindow's JS code is initialized.
+  SetContentView(gin::CreateHandle<View>(isolate, web_contents_view.get()));
+
 #if defined(OS_MACOSX)
   OverrideNSWindowContentView(web_contents->managed_web_contents());
 #endif

shell/browser/api/electron_api_web_contents_view.cc

@@ -4,52 +4,32 @@
 
 #include "shell/browser/api/electron_api_web_contents_view.h"
 
-#include "content/public/browser/web_contents_user_data.h"
+#include "base/no_destructor.h"
 #include "shell/browser/api/electron_api_web_contents.h"
 #include "shell/browser/browser.h"
 #include "shell/browser/ui/inspectable_web_contents_view.h"
+#include "shell/browser/web_contents_preferences.h"
+#include "shell/common/gin_converters/value_converter.h"
 #include "shell/common/gin_helper/constructor.h"
 #include "shell/common/gin_helper/dictionary.h"
+#include "shell/common/gin_helper/object_template_builder.h"
 #include "shell/common/node_includes.h"
 
 #if defined(OS_MACOSX)
 #include "shell/browser/ui/cocoa/delayed_native_view_host.h"
 #endif
 
-namespace {
-
-// Used to indicate whether a WebContents already has a view.
-class WebContentsViewRelay
-    : public content::WebContentsUserData<WebContentsViewRelay> {
- public:
-  ~WebContentsViewRelay() override = default;
-
- private:
-  explicit WebContentsViewRelay(content::WebContents* contents) {}
-  friend class content::WebContentsUserData<WebContentsViewRelay>;
-
-  electron::api::WebContentsView* view_ = nullptr;
-
-  WEB_CONTENTS_USER_DATA_KEY_DECL();
-
-  DISALLOW_COPY_AND_ASSIGN(WebContentsViewRelay);
-};
-
-WEB_CONTENTS_USER_DATA_KEY_IMPL(WebContentsViewRelay)
-
-}  // namespace
-
 namespace electron {
 
 namespace api {
 
 WebContentsView::WebContentsView(v8::Isolate* isolate,
-                                 gin::Handle<WebContents> web_contents,
-                                 InspectableWebContents* iwc)
+                                 gin::Handle<WebContents> web_contents)
 #if defined(OS_MACOSX)
-    : View(new DelayedNativeViewHost(iwc->GetView()->GetNativeView())),
+    : View(new DelayedNativeViewHost(
+          web_contents->managed_web_contents()->GetView()->GetNativeView())),
 #else
-    : View(iwc->GetView()->GetView()),
+    : View(web_contents->managed_web_contents()->GetView()->GetView()),
 #endif
       web_contents_(isolate, web_contents->GetWrapper()),
       api_web_contents_(web_contents.get()) {
@@ -59,7 +39,6 @@ WebContentsView::WebContentsView(v8::Isolate* isolate,
   // managed by InspectableWebContents.
   set_delete_view(false);
 #endif
-  WebContentsViewRelay::CreateForWebContents(web_contents->web_contents());
   Observe(web_contents->web_contents());
 }
 
@@ -78,30 +57,66 @@ WebContentsView::~WebContentsView() {
   }
 }
 
+gin::Handle<WebContents> WebContentsView::GetWebContents(v8::Isolate* isolate) {
+  return gin::CreateHandle(isolate, api_web_contents_);
+}
+
 void WebContentsView::WebContentsDestroyed() {
   api_web_contents_ = nullptr;
   web_contents_.Reset();
 }
 
+// static
+gin::Handle<WebContentsView> WebContentsView::Create(
+    v8::Isolate* isolate,
+    const gin_helper::Dictionary& web_preferences) {
+  v8::Local<v8::Context> context = isolate->GetCurrentContext();
+  v8::Local<v8::Value> arg = gin::ConvertToV8(isolate, web_preferences);
+  v8::Local<v8::Object> obj;
+  if (GetConstructor(isolate)->NewInstance(context, 1, &arg).ToLocal(&obj)) {
+    gin::Handle<WebContentsView> web_contents_view;
+    if (gin::ConvertFromV8(isolate, obj, &web_contents_view))
+      return web_contents_view;
+  }
+  return gin::Handle<WebContentsView>();
+}
+
+// static
+v8::Local<v8::Function> WebContentsView::GetConstructor(v8::Isolate* isolate) {
+  static base::NoDestructor<v8::Global<v8::Function>> constructor;
+  if (constructor.get()->IsEmpty()) {
+    constructor->Reset(
+        isolate, gin_helper::CreateConstructor<WebContentsView>(
+                     isolate, base::BindRepeating(&WebContentsView::New)));
+  }
+  return v8::Local<v8::Function>::New(isolate, *constructor.get());
+}
+
 // static
 gin_helper::WrappableBase* WebContentsView::New(
     gin_helper::Arguments* args,
-    gin::Handle<WebContents> web_contents) {
-  // Currently we only support InspectableWebContents, e.g. the WebContents
-  // created by users directly. To support devToolsWebContents we need to create
-  // a wrapper view.
-  if (!web_contents->managed_web_contents()) {
-    args->ThrowError("The WebContents must be created by user");
-    return nullptr;
-  }
-  // Check if the WebContents has already been added to a view.
-  if (WebContentsViewRelay::FromWebContents(web_contents->web_contents())) {
-    args->ThrowError("The WebContents has already been added to a View");
-    return nullptr;
+    const gin_helper::Dictionary& web_preferences) {
+  // Check if BrowserWindow has passend |webContents| option to us.
+  gin::Handle<WebContents> web_contents;
+  if (web_preferences.GetHidden("webContents", &web_contents) &&
+      !web_contents.IsEmpty()) {
+    // Set webPreferences from options if using an existing webContents.
+    // These preferences will be used when the webContent launches new
+    // render processes.
+    auto* existing_preferences =
+        WebContentsPreferences::From(web_contents->web_contents());
+    base::DictionaryValue web_preferences_dict;
+    if (gin::ConvertFromV8(args->isolate(), web_preferences.GetHandle(),
+                           &web_preferences_dict)) {
+      existing_preferences->Clear();
+      existing_preferences->Merge(web_preferences_dict);
+    }
+  } else {
+    // Create one if not.
+    web_contents = WebContents::Create(args->isolate(), web_preferences);
   }
   // Constructor call.
-  auto* view = new WebContentsView(args->isolate(), web_contents,
-                                   web_contents->managed_web_contents());
+  auto* view = new WebContentsView(args->isolate(), web_contents);
   view->InitWithArgs(args);
   return view;
 }
@@ -109,7 +124,11 @@ gin_helper::WrappableBase* WebContentsView::New(
 // static
 void WebContentsView::BuildPrototype(
     v8::Isolate* isolate,
-    v8::Local<v8::FunctionTemplate> prototype) {}
+    v8::Local<v8::FunctionTemplate> prototype) {
+  prototype->SetClassName(gin::StringToV8(isolate, "WebContentsView"));
+  gin_helper::ObjectTemplateBuilder(isolate, prototype->PrototypeTemplate())
+      .SetProperty("webContents", &WebContentsView::GetWebContents);
+}
 
 }  // namespace api
 
@@ -125,9 +144,7 @@ void Initialize(v8::Local<v8::Object> exports,
                 void* priv) {
   v8::Isolate* isolate = context->GetIsolate();
   gin_helper::Dictionary dict(isolate, exports);
-  dict.Set("WebContentsView",
-           gin_helper::CreateConstructor<WebContentsView>(
-               isolate, base::BindRepeating(&WebContentsView::New)));
+  dict.Set("WebContentsView", WebContentsView::GetConstructor(isolate));
 }
 
 }  // namespace

shell/browser/api/electron_api_web_contents_view.h

@@ -8,6 +8,10 @@
 #include "content/public/browser/web_contents_observer.h"
 #include "shell/browser/api/electron_api_view.h"
 
+namespace gin_helper {
+class Dictionary;
+}
+
 namespace electron {
 
 class InspectableWebContents;
@@ -18,22 +22,34 @@ class WebContents;
 
 class WebContentsView : public View, public content::WebContentsObserver {
  public:
-  static gin_helper::WrappableBase* New(gin_helper::Arguments* args,
-                                        gin::Handle<WebContents> web_contents);
+  // Create a new instance of WebContentsView.
+  static gin::Handle<WebContentsView> Create(
+      v8::Isolate* isolate,
+      const gin_helper::Dictionary& web_preferences);
+
+  // Return the cached constructor function.
+  static v8::Local<v8::Function> GetConstructor(v8::Isolate* isolate);
 
+  // gin_helper::Wrappable
   static void BuildPrototype(v8::Isolate* isolate,
                              v8::Local<v8::FunctionTemplate> prototype);
 
+  // Public APIs.
+  gin::Handle<WebContents> GetWebContents(v8::Isolate* isolate);
+
  protected:
-  WebContentsView(v8::Isolate* isolate,
-                  gin::Handle<WebContents> web_contents,
-                  InspectableWebContents* iwc);
+  // Takes an existing WebContents.
+  WebContentsView(v8::Isolate* isolate, gin::Handle<WebContents> web_contents);
   ~WebContentsView() override;
 
   // content::WebContentsObserver:
   void WebContentsDestroyed() override;
 
  private:
+  static gin_helper::WrappableBase* New(
+      gin_helper::Arguments* args,
+      const gin_helper::Dictionary& web_preferences);
+
   // Keep a reference to v8 wrapper.
   v8::Global<v8::Value> web_contents_;
   api::WebContents* api_web_contents_;

spec-main/ambient.d.ts

@@ -27,7 +27,7 @@ declare namespace Electron {
   }
   class View {}
   class WebContentsView {
-    constructor(webContents: WebContents)
+    constructor(options: BrowserWindowConstructorOptions)
   }
 
   namespace Main {

spec-main/api-web-contents-view-spec.ts

@@ -4,25 +4,15 @@ import * as path from 'path';
 import { emittedOnce } from './events-helpers';
 import { closeWindow } from './window-helpers';
 
-import { webContents, TopLevelWindow, WebContentsView } from 'electron/main';
+import { TopLevelWindow, WebContentsView } from 'electron/main';
 
 describe('WebContentsView', () => {
   let w: TopLevelWindow;
   afterEach(() => closeWindow(w as any).then(() => { w = null as unknown as TopLevelWindow; }));
 
   it('can be used as content view', () => {
-    const web = (webContents as any).create({});
     w = new TopLevelWindow({ show: false });
-    w.setContentView(new WebContentsView(web));
-  });
-
-  it('prevents adding same WebContents', () => {
-    const web = (webContents as any).create({});
-    w = new TopLevelWindow({ show: false });
-    w.setContentView(new WebContentsView(web));
-    expect(() => {
-      w.setContentView(new WebContentsView(web));
-    }).to.throw('The WebContents has already been added to a View');
+    w.setContentView(new WebContentsView({}));
   });
 
   describe('new WebContentsView()', () => {
@@ -50,9 +40,8 @@ describe('WebContentsView', () => {
   }
 
   it('doesn\'t crash when GCed during allocation', (done) => {
-    const web = (webContents as any).create({});
     // eslint-disable-next-line no-new
-    new WebContentsView(web);
+    new WebContentsView({});
     setTimeout(() => {
       // NB. the crash we're testing for is the lack of a current `v8::Context`
       // when emitting an event in WebContents's destructor. V8 is inconsistent

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

@@ -1,7 +1,6 @@
-const { WebContentsView, app, webContents } = require('electron');
+const { WebContentsView, app } = require('electron');
 app.whenReady().then(function () {
-  const web = webContents.create({});
-  new WebContentsView(web)  // eslint-disable-line
+  new WebContentsView({})  // eslint-disable-line
 
   app.quit();
 });