From 994fe36960efd251216bb091803c7cb824f35b07 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Wed, 17 Oct 2018 16:38:34 +0900 Subject: [PATCH] refactor: remove WebContents::CreateFrom --- atom/browser/api/atom_api_app.cc | 15 ++-- .../atom_api_render_process_preferences.cc | 2 +- atom/browser/api/atom_api_web_contents.cc | 82 +++++++++++-------- atom/browser/api/atom_api_web_contents.h | 31 +++++-- atom/browser/atom_navigation_throttle.cc | 4 +- .../content_converter.cc | 2 +- 6 files changed, 85 insertions(+), 51 deletions(-) diff --git a/atom/browser/api/atom_api_app.cc b/atom/browser/api/atom_api_app.cc index dafde725c47d8..17c053dd569bb 100644 --- a/atom/browser/api/atom_api_app.cc +++ b/atom/browser/api/atom_api_app.cc @@ -683,7 +683,7 @@ void App::OnLogin(scoped_refptr 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)); diff --git a/atom/browser/api/atom_api_render_process_preferences.cc b/atom/browser/api/atom_api_render_process_preferences.cc index 98c6cca2f0e6c..d0344261dc948 100644 --- a/atom/browser/api/atom_api_render_process_preferences.cc +++ b/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; diff --git a/atom/browser/api/atom_api_web_contents.cc b/atom/browser/api/atom_api_web_contents.cc index b441f25b78545..80cf594058aa8 100644 --- a/atom/browser/api/atom_api_web_contents.cc +++ b/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::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(existing)); - - // Otherwise create a new WebContents wrapper object. - return mate::CreateHandle(isolate, - new WebContents(isolate, web_contents, REMOTE)); +mate::Handle WebContents::Create(v8::Isolate* isolate, + const mate::Dictionary& options) { + return mate::CreateHandle(isolate, new WebContents(isolate, options)); } -mate::Handle WebContents::CreateFrom( +// static +mate::Handle 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::Create(v8::Isolate* isolate, - const mate::Dictionary& options) { - return mate::CreateHandle(isolate, new WebContents(isolate, options)); +mate::Handle WebContents::From( + v8::Isolate* isolate, + content::WebContents* web_contents) { + auto* existing = TrackableObject::FromWrappedClass(isolate, web_contents); + if (existing) + return mate::CreateHandle(isolate, static_cast(existing)); + else + return mate::Handle(); +} + +// static +mate::Handle 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 diff --git a/atom/browser/api/atom_api_web_contents.h b/atom/browser/api/atom_api_web_contents.h index dfe4e4c678ee2..40014665cb487 100644 --- a/atom/browser/api/atom_api_web_contents.h +++ b/atom/browser/api/atom_api_web_contents.h @@ -82,18 +82,29 @@ class WebContents : public mate::TrackableObject, using PrintToPDFCallback = base::Callback, v8::Local)>; - // Create from an existing WebContents. - static mate::Handle CreateFrom( - v8::Isolate* isolate, - content::WebContents* web_contents); - static mate::Handle CreateFrom( + // Create a new WebContents and return the V8 wrapper of it. + static mate::Handle 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 CreateAndTake( v8::Isolate* isolate, content::WebContents* web_contents, Type type); - // Create a new WebContents. - static mate::Handle Create(v8::Isolate* isolate, - const mate::Dictionary& options); + // Get the V8 wrapper of |web_content|, return empty handle if not wrapped. + static mate::Handle 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 FromOrCreate( + v8::Isolate* isolate, + content::WebContents* web_contents); static void BuildPrototype(v8::Isolate* isolate, v8::Local prototype); @@ -281,9 +292,13 @@ class WebContents : public mate::TrackableObject, 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; diff --git a/atom/browser/atom_navigation_throttle.cc b/atom/browser/atom_navigation_throttle.cc index bcb0dfe56118b..714bf32e4c6a2 100644 --- a/atom/browser/atom_navigation_throttle.cc +++ b/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; } diff --git a/atom/common/native_mate_converters/content_converter.cc b/atom/common/native_mate_converters/content_converter.cc index 2684aa7dda07a..25eec92c9f33b 100644 --- a/atom/common/native_mate_converters/content_converter.cc +++ b/atom/common/native_mate_converters/content_converter.cc @@ -206,7 +206,7 @@ v8::Local Converter::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