refactor: remove WebContents::CreateFrom (#15241)

atom/browser/api/atom_api_app.cc

@@ -683,7 +683,7 @@ void App::OnLogin(scoped_refptr<LoginHandler> login_handler,
   content::WebContents* web_contents = login_handler->GetWebContents();
   if (web_contents) {
     prevent_default = Emit(
-        "login", WebContents::CreateFrom(isolate(), web_contents),
+        "login", WebContents::FromOrCreate(isolate(), web_contents),
         request_details, login_handler->auth_info(),
         base::Bind(&PassLoginInformation, base::RetainedRef(login_handler)));
   }
@@ -714,9 +714,12 @@ bool App::CanCreateWindow(
   content::WebContents* web_contents =
       content::WebContents::FromRenderFrameHost(opener);
   if (web_contents) {
-    auto api_web_contents = WebContents::CreateFrom(isolate(), web_contents);
-    api_web_contents->OnCreateWindow(target_url, referrer, frame_name,
-                                     disposition, additional_features, body);
+    auto api_web_contents = WebContents::From(isolate(), web_contents);
+    // No need to emit any event if the WebContents is not available in JS.
+    if (!api_web_contents.IsEmpty()) {
+      api_web_contents->OnCreateWindow(target_url, referrer, frame_name,
+                                       disposition, additional_features, body);
+    }
   }
 
   return false;
@@ -735,7 +738,7 @@ void App::AllowCertificateError(
   v8::Locker locker(isolate());
   v8::HandleScope handle_scope(isolate());
   bool prevent_default = Emit(
-      "certificate-error", WebContents::CreateFrom(isolate(), web_contents),
+      "certificate-error", WebContents::FromOrCreate(isolate(), web_contents),
       request_url, net::ErrorToString(cert_error), ssl_info.cert, callback);
 
   // Deny the certificate by default.
@@ -762,7 +765,7 @@ void App::SelectClientCertificate(
 
   bool prevent_default =
       Emit("select-client-certificate",
-           WebContents::CreateFrom(isolate(), web_contents),
+           WebContents::FromOrCreate(isolate(), web_contents),
            cert_request_info->host_and_port.ToString(), std::move(client_certs),
            base::Bind(&OnClientCertificateSelected, isolate(), shared_delegate,
                       shared_identities));

atom/browser/api/atom_api_render_process_preferences.cc

@@ -26,7 +26,7 @@ bool IsWebContents(v8::Isolate* isolate, content::RenderProcessHost* process) {
   if (!web_contents)
     return false;
 
-  auto api_web_contents = WebContents::CreateFrom(isolate, web_contents);
+  auto api_web_contents = WebContents::FromOrCreate(isolate, web_contents);
   auto type = api_web_contents->GetType();
   return type == WebContents::Type::BROWSER_WINDOW ||
          type == WebContents::Type::WEB_VIEW;

atom/browser/api/atom_api_web_contents.cc

@@ -309,22 +309,25 @@ struct WebContents::FrameDispatchHelper {
   }
 };
 
+WebContents::WebContents(v8::Isolate* isolate,
+                         content::WebContents* web_contents)
+    : content::WebContentsObserver(web_contents), type_(REMOTE) {
+  web_contents->SetUserAgentOverride(GetBrowserContext()->GetUserAgent(),
+                                     false);
+  Init(isolate);
+  AttachAsUserData(web_contents);
+  InitZoomController(web_contents, mate::Dictionary::CreateEmpty(isolate));
+}
+
 WebContents::WebContents(v8::Isolate* isolate,
                          content::WebContents* web_contents,
                          Type type)
     : content::WebContentsObserver(web_contents), type_(type) {
-  const mate::Dictionary options = mate::Dictionary::CreateEmpty(isolate);
-  if (type == REMOTE) {
-    web_contents->SetUserAgentOverride(GetBrowserContext()->GetUserAgent(),
-                                       false);
-    Init(isolate);
-    AttachAsUserData(web_contents);
-    InitZoomController(web_contents, options);
-  } else {
-    auto session = Session::CreateFrom(isolate, GetBrowserContext());
-    session_.Reset(isolate, session.ToV8());
-    InitWithSessionAndOptions(isolate, web_contents, session, options);
-  }
+  DCHECK(type != REMOTE) << "Can't take ownership of a remote WebContents";
+  auto session = Session::CreateFrom(isolate, GetBrowserContext());
+  session_.Reset(isolate, session.ToV8());
+  InitWithSessionAndOptions(isolate, web_contents, session,
+                            mate::Dictionary::CreateEmpty(isolate));
 }
 
 WebContents::WebContents(v8::Isolate* isolate,
@@ -538,8 +541,9 @@ void WebContents::WebContentsCreated(content::WebContents* source_contents,
                                      content::WebContents* new_contents) {
   v8::Locker locker(isolate());
   v8::HandleScope handle_scope(isolate());
-  auto api_web_contents = CreateFrom(isolate(), new_contents, BROWSER_WINDOW);
-  Emit("-web-contents-created", api_web_contents, target_url, frame_name);
+  // Create V8 wrapper for the |new_contents|.
+  auto wrapper = CreateAndTake(isolate(), new_contents, BROWSER_WINDOW);
+  Emit("-web-contents-created", wrapper, target_url, frame_name);
 }
 
 void WebContents::AddNewContents(
@@ -552,7 +556,11 @@ void WebContents::AddNewContents(
   new ChildWebContentsTracker(new_contents.get());
   v8::Locker locker(isolate());
   v8::HandleScope handle_scope(isolate());
-  auto api_web_contents = CreateFrom(isolate(), new_contents.release());
+  // Note that the ownership of |new_contents| has already been claimed by
+  // the WebContentsCreated method, the release call here completes
+  // the ownership transfer.
+  auto api_web_contents = From(isolate(), new_contents.release());
+  DCHECK(!api_web_contents.IsEmpty());
   if (Emit("-add-new-contents", api_web_contents, disposition, user_gesture,
            initial_rect.x(), initial_rect.y(), initial_rect.width(),
            initial_rect.height())) {
@@ -979,8 +987,8 @@ void WebContents::DevToolsFocused() {
 void WebContents::DevToolsOpened() {
   v8::Locker locker(isolate());
   v8::HandleScope handle_scope(isolate());
-  auto handle = WebContents::CreateFrom(
-      isolate(), managed_web_contents()->GetDevToolsWebContents());
+  auto handle =
+      FromOrCreate(isolate(), managed_web_contents()->GetDevToolsWebContents());
   devtools_web_contents_.Reset(isolate(), handle.ToV8());
 
   // Set inspected tabID.
@@ -2180,32 +2188,40 @@ void WebContents::OnRendererMessageTo(content::RenderFrameHost* frame_host,
 }
 
 // static
-mate::Handle<WebContents> WebContents::CreateFrom(
-    v8::Isolate* isolate,
-    content::WebContents* web_contents) {
-  // We have an existing WebContents object in JS.
-  auto* existing = TrackableObject::FromWrappedClass(isolate, web_contents);
-  if (existing)
-    return mate::CreateHandle(isolate, static_cast<WebContents*>(existing));
-
-  // Otherwise create a new WebContents wrapper object.
-  return mate::CreateHandle(isolate,
-                            new WebContents(isolate, web_contents, REMOTE));
+mate::Handle<WebContents> WebContents::Create(v8::Isolate* isolate,
+                                              const mate::Dictionary& options) {
+  return mate::CreateHandle(isolate, new WebContents(isolate, options));
 }
 
-mate::Handle<WebContents> WebContents::CreateFrom(
+// static
+mate::Handle<WebContents> WebContents::CreateAndTake(
     v8::Isolate* isolate,
     content::WebContents* web_contents,
     Type type) {
-  // Otherwise create a new WebContents wrapper object.
   return mate::CreateHandle(isolate,
                             new WebContents(isolate, web_contents, type));
 }
 
 // static
-mate::Handle<WebContents> WebContents::Create(v8::Isolate* isolate,
-                                              const mate::Dictionary& options) {
-  return mate::CreateHandle(isolate, new WebContents(isolate, options));
+mate::Handle<WebContents> WebContents::From(
+    v8::Isolate* isolate,
+    content::WebContents* web_contents) {
+  auto* existing = TrackableObject::FromWrappedClass(isolate, web_contents);
+  if (existing)
+    return mate::CreateHandle(isolate, static_cast<WebContents*>(existing));
+  else
+    return mate::Handle<WebContents>();
+}
+
+// static
+mate::Handle<WebContents> WebContents::FromOrCreate(
+    v8::Isolate* isolate,
+    content::WebContents* web_contents) {
+  auto existing = From(isolate, web_contents);
+  if (!existing.IsEmpty())
+    return existing;
+  else
+    return mate::CreateHandle(isolate, new WebContents(isolate, web_contents));
 }
 
 }  // namespace api

atom/browser/api/atom_api_web_contents.h

@@ -82,18 +82,29 @@ class WebContents : public mate::TrackableObject<WebContents>,
   using PrintToPDFCallback =
       base::Callback<void(v8::Local<v8::Value>, v8::Local<v8::Value>)>;
 
-  // Create from an existing WebContents.
-  static mate::Handle<WebContents> CreateFrom(
-      v8::Isolate* isolate,
-      content::WebContents* web_contents);
-  static mate::Handle<WebContents> CreateFrom(
+  // Create a new WebContents and return the V8 wrapper of it.
+  static mate::Handle<WebContents> Create(v8::Isolate* isolate,
+                                          const mate::Dictionary& options);
+
+  // Create a new V8 wrapper for an existing |web_content|.
+  //
+  // The lifetime of |web_contents| will be managed by this class.
+  static mate::Handle<WebContents> CreateAndTake(
       v8::Isolate* isolate,
       content::WebContents* web_contents,
       Type type);
 
-  // Create a new WebContents.
-  static mate::Handle<WebContents> Create(v8::Isolate* isolate,
-                                          const mate::Dictionary& options);
+  // Get the V8 wrapper of |web_content|, return empty handle if not wrapped.
+  static mate::Handle<WebContents> From(v8::Isolate* isolate,
+                                        content::WebContents* web_content);
+
+  // Get the V8 wrapper of the |web_contents|, or create one if not existed.
+  //
+  // The lifetime of |web_contents| is NOT managed by this class, and the type
+  // of this wrapper is always REMOTE.
+  static mate::Handle<WebContents> FromOrCreate(
+      v8::Isolate* isolate,
+      content::WebContents* web_contents);
 
   static void BuildPrototype(v8::Isolate* isolate,
                              v8::Local<v8::FunctionTemplate> prototype);
@@ -281,9 +292,13 @@ class WebContents : public mate::TrackableObject<WebContents>,
                            content::NavigationHandle* navigation_handle);
 
  protected:
+  // Does not manage lifetime of |web_contents|.
+  WebContents(v8::Isolate* isolate, content::WebContents* web_contents);
+  // Takes over ownership of |web_contents|.
   WebContents(v8::Isolate* isolate,
               content::WebContents* web_contents,
               Type type);
+  // Creates a new content::WebContents.
   WebContents(v8::Isolate* isolate, const mate::Dictionary& options);
   ~WebContents() override;
 

atom/browser/atom_navigation_throttle.cc

@@ -29,9 +29,9 @@ AtomNavigationThrottle::WillRedirectRequest() {
   }
 
   auto api_contents =
-      atom::api::WebContents::CreateFrom(v8::Isolate::GetCurrent(), contents);
+      atom::api::WebContents::From(v8::Isolate::GetCurrent(), contents);
   if (api_contents.IsEmpty()) {
-    NOTREACHED();
+    // No need to emit any event if the WebContents is not available in JS.
     return PROCEED;
   }
 

atom/common/native_mate_converters/content_converter.cc

@@ -206,7 +206,7 @@ v8::Local<v8::Value> Converter<content::WebContents*>::ToV8(
     content::WebContents* val) {
   if (!val)
     return v8::Null(isolate);
-  return atom::api::WebContents::CreateFrom(isolate, val).ToV8();
+  return atom::api::WebContents::FromOrCreate(isolate, val).ToV8();
 }
 
 // static