From b5cd9ce0b31acc02b4c92f3704570de79a76f25b Mon Sep 17 00:00:00 2001 From: Jeremy Rose Date: Thu, 30 Jul 2020 09:17:57 -0700 Subject: [PATCH] refactor: ginify WebContents (#24651) --- docs/api/structures/keyboard-event.md | 2 +- lib/browser/api/web-contents.ts | 3 - .../api/electron_api_browser_window.cc | 2 +- .../browser/api/electron_api_web_contents.cc | 149 +++++++++++++----- shell/browser/api/electron_api_web_contents.h | 44 +++++- shell/browser/event_emitter_mixin.h | 4 +- shell/common/gin_helper/constructor.h | 2 +- shell/common/gin_helper/event_emitter.h | 17 -- 8 files changed, 156 insertions(+), 67 deletions(-) diff --git a/docs/api/structures/keyboard-event.md b/docs/api/structures/keyboard-event.md index 95442ee0e574..209cdefebecf 100644 --- a/docs/api/structures/keyboard-event.md +++ b/docs/api/structures/keyboard-event.md @@ -1,4 +1,4 @@ -# KeyboardEvent Object extends `Event` +# KeyboardEvent Object * `ctrlKey` Boolean (optional) - whether the Control key was used in an accelerator to trigger the Event * `metaKey` Boolean (optional) - whether a meta key was used in an accelerator to trigger the Event diff --git a/lib/browser/api/web-contents.ts b/lib/browser/api/web-contents.ts index 404e513632b3..f4c30bfc7ac4 100644 --- a/lib/browser/api/web-contents.ts +++ b/lib/browser/api/web-contents.ts @@ -9,7 +9,6 @@ import { ipcMainInternal } from '@electron/internal/browser/ipc-main-internal'; import * as ipcMainUtils from '@electron/internal/browser/ipc-main-internal-utils'; import { parseFeatures } from '@electron/internal/common/parse-features-string'; import { MessagePortMain } from '@electron/internal/browser/message-port-main'; -import { EventEmitter } from 'events'; // session is not used here, the purpose is to make sure session is initalized // before the webContents module. @@ -124,8 +123,6 @@ const defaultPrintingSetting = { const binding = process._linkedBinding('electron_browser_web_contents'); const { WebContents } = binding as { WebContents: { prototype: Electron.WebContentsInternal } }; -Object.setPrototypeOf(WebContents.prototype, EventEmitter.prototype); - WebContents.prototype.send = function (channel, ...args) { if (typeof channel !== 'string') { throw new Error('Missing required channel argument'); diff --git a/shell/browser/api/electron_api_browser_window.cc b/shell/browser/api/electron_api_browser_window.cc index 703f9c639489..4ba184104895 100644 --- a/shell/browser/api/electron_api_browser_window.cc +++ b/shell/browser/api/electron_api_browser_window.cc @@ -78,7 +78,7 @@ BrowserWindow::BrowserWindow(gin::Arguments* args, Observe(api_web_contents_->web_contents()); // Keep a copy of the options for later use. - gin_helper::Dictionary(isolate, web_contents->GetWrapper()) + gin_helper::Dictionary(isolate, web_contents.ToV8().As()) .Set("browserWindowOptions", options); // Associate with BrowserWindow. diff --git a/shell/browser/api/electron_api_web_contents.cc b/shell/browser/api/electron_api_web_contents.cc index 25fa409e6f0e..ccc389c5aca2 100644 --- a/shell/browser/api/electron_api_web_contents.cc +++ b/shell/browser/api/electron_api_web_contents.cc @@ -12,6 +12,7 @@ #include #include +#include "base/containers/id_map.h" #include "base/no_destructor.h" #include "base/optional.h" #include "base/strings/utf_string_conversions.h" @@ -332,6 +333,11 @@ namespace api { namespace { +base::IDMap& GetAllWebContents() { + static base::NoDestructor> s_all_web_contents; + return *s_all_web_contents; +} + // Called when CapturePage is done. void OnCapturePageDone(gin_helper::Promise promise, const SkBitmap& bitmap) { @@ -389,12 +395,21 @@ base::string16 GetDefaultPrinterAsync() { } #endif +struct UserDataLink : public base::SupportsUserData::Data { + explicit UserDataLink(base::WeakPtr contents) + : web_contents(contents) {} + + base::WeakPtr web_contents; +}; +const void* kElectronApiWebContentsKey = &kElectronApiWebContentsKey; + } // namespace WebContents::WebContents(v8::Isolate* isolate, content::WebContents* web_contents) : content::WebContentsObserver(web_contents), type_(Type::REMOTE), + id_(GetAllWebContents().Add(this)), weak_factory_(this) { auto session = Session::CreateFrom(isolate, GetBrowserContext()); session_.Reset(isolate, session.ToV8()); @@ -402,8 +417,8 @@ WebContents::WebContents(v8::Isolate* isolate, web_contents->SetUserAgentOverride(blink::UserAgentOverride::UserAgentOnly( GetBrowserContext()->GetUserAgent()), false); - Init(isolate); - AttachAsUserData(web_contents); + web_contents->SetUserData(kElectronApiWebContentsKey, + std::make_unique(GetWeakPtr())); InitZoomController(web_contents, gin::Dictionary::CreateEmpty(isolate)); #if BUILDFLAG(ENABLE_ELECTRON_EXTENSIONS) extensions::ElectronExtensionWebContentsObserver::CreateForWebContents( @@ -421,6 +436,7 @@ WebContents::WebContents(v8::Isolate* isolate, Type type) : content::WebContentsObserver(web_contents.get()), type_(type), + id_(GetAllWebContents().Add(this)), weak_factory_(this) { DCHECK(type != Type::REMOTE) << "Can't take ownership of a remote WebContents"; @@ -432,7 +448,7 @@ WebContents::WebContents(v8::Isolate* isolate, WebContents::WebContents(v8::Isolate* isolate, const gin_helper::Dictionary& options) - : weak_factory_(this) { + : id_(GetAllWebContents().Add(this)), weak_factory_(this) { // Read options. options.Get("backgroundThrottling", &background_throttling_); @@ -610,11 +626,12 @@ void WebContents::InitWithSessionAndOptions( SetOwnerWindow(owner_window); } - Init(isolate); - AttachAsUserData(web_contents()); + web_contents()->SetUserData(kElectronApiWebContentsKey, + std::make_unique(GetWeakPtr())); } WebContents::~WebContents() { + MarkDestroyed(); // The destroy() is called. if (managed_web_contents()) { managed_web_contents()->GetView()->SetDelegate(nullptr); @@ -740,19 +757,19 @@ void WebContents::AddNewContents( content::WebContents* WebContents::OpenURLFromTab( content::WebContents* source, const content::OpenURLParams& params) { + auto weak_this = GetWeakPtr(); if (params.disposition != WindowOpenDisposition::CURRENT_TAB) { Emit("-new-window", params.url, "", params.disposition, "", params.referrer, params.post_data); return nullptr; } + if (!weak_this) + return nullptr; // Give user a chance to cancel navigation. if (Emit("will-navigate", params.url)) return nullptr; - - // Don't load the URL if the web contents was marked as destroyed from a - // will-navigate event listener - if (IsDestroyed()) + if (!weak_this) return nullptr; return CommonWebContentsDelegate::OpenURLFromTab(source, params); @@ -1217,9 +1234,7 @@ void WebContents::MessageTo(bool internal, const std::string& channel, blink::CloneableMessage arguments) { TRACE_EVENT1("electron", "WebContents::MessageTo", "channel", channel); - v8::Isolate* isolate = JavascriptEnvironment::GetIsolate(); - auto* web_contents = gin_helper::TrackableObject::FromWeakMapID( - isolate, web_contents_id); + auto* web_contents = FromID(web_contents_id); if (web_contents) { web_contents->SendIPCMessageWithSender(internal, send_to_all, channel, @@ -1398,6 +1413,17 @@ bool WebContents::OnMessageReceived(const IPC::Message& message) { return handled; } +void WebContents::MarkDestroyed() { + if (GetAllWebContents().Lookup(id_)) + GetAllWebContents().Remove(id_); + v8::Isolate* isolate = JavascriptEnvironment::GetIsolate(); + v8::HandleScope scope(isolate); + v8::Local wrapper; + if (!GetWrapper(isolate).ToLocal(&wrapper)) + return; + wrapper->SetAlignedPointerInInternalField(0, nullptr); +} + // There are three ways of destroying a webContents: // 1. call webContents.destroy(); // 2. garbage collection; @@ -1419,7 +1445,6 @@ void WebContents::WebContentsDestroyed() { guest_delegate_->WillDestroy(); // Cleanup relationships with other parts. - RemoveFromWeakMap(); // We can not call Destroy here because we need to call Emit first, but we // also do not want any method to be used, so just mark as destroyed here. @@ -1435,7 +1460,13 @@ void WebContents::WebContentsDestroyed() { } // Destroy the native class in next tick. - base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, GetDestroyClosure()); + base::ThreadTaskRunnerHandle::Get()->PostTask( + FROM_HERE, base::BindOnce( + [](base::WeakPtr wc) { + if (wc) + delete wc.get(); + }, + GetWeakPtr())); } void WebContents::NavigationEntryCommitted( @@ -2632,10 +2663,6 @@ v8::Local WebContents::GetOwnerBrowserWindow( return v8::Null(isolate); } -int32_t WebContents::ID() const { - return weak_map_id(); -} - v8::Local WebContents::Session(v8::Isolate* isolate) { return v8::Local::New(isolate, session_); } @@ -2754,11 +2781,28 @@ v8::Local WebContents::TakeHeapSnapshot( } // static -void WebContents::BuildPrototype(v8::Isolate* isolate, - v8::Local prototype) { - prototype->SetClassName(gin::StringToV8(isolate, "WebContents")); - gin_helper::Destroyable::MakeDestroyable(isolate, prototype); - gin_helper::ObjectTemplateBuilder(isolate, prototype->PrototypeTemplate()) +v8::Local WebContents::FillObjectTemplate( + v8::Isolate* isolate, + v8::Local templ) { + gin::InvokerOptions options; + options.holder_is_first_argument = true; + options.holder_type = "WebContents"; + templ->Set( + gin::StringToSymbol(isolate, "isDestroyed"), + gin::CreateFunctionTemplate( + isolate, base::BindRepeating(&gin_helper::Destroyable::IsDestroyed), + options)); + templ->Set( + gin::StringToSymbol(isolate, "destroy"), + gin::CreateFunctionTemplate( + isolate, base::BindRepeating([](gin::Handle handle) { + delete handle.get(); + }), + options)); + // We use gin_helper::ObjectTemplateBuilder instead of + // gin::ObjectTemplateBuilder here to handle the fact that WebContents is + // destroyable. + return gin_helper::ObjectTemplateBuilder(isolate, templ) .SetMethod("getBackgroundThrottling", &WebContents::GetBackgroundThrottling) .SetMethod("setBackgroundThrottling", @@ -2870,7 +2914,12 @@ void WebContents::BuildPrototype(v8::Isolate* isolate, .SetProperty("session", &WebContents::Session) .SetProperty("hostWebContents", &WebContents::HostWebContents) .SetProperty("devToolsWebContents", &WebContents::DevToolsWebContents) - .SetProperty("debugger", &WebContents::Debugger); + .SetProperty("debugger", &WebContents::Debugger) + .Build(); +} + +const char* WebContents::GetTypeName() { + return "WebContents"; } ElectronBrowserContext* WebContents::GetBrowserContext() const { @@ -2882,7 +2931,17 @@ ElectronBrowserContext* WebContents::GetBrowserContext() const { gin::Handle WebContents::Create( v8::Isolate* isolate, const gin_helper::Dictionary& options) { - return gin::CreateHandle(isolate, new WebContents(isolate, options)); + gin::Handle handle = + gin::CreateHandle(isolate, new WebContents(isolate, options)); + gin_helper::CallMethod(isolate, handle.get(), "_init"); + return handle; +} + +// static +gin::Handle WebContents::New( + v8::Isolate* isolate, + const gin_helper::Dictionary& options) { + return Create(isolate, options); } // static @@ -2890,15 +2949,19 @@ gin::Handle WebContents::CreateAndTake( v8::Isolate* isolate, std::unique_ptr web_contents, Type type) { - return gin::CreateHandle( + gin::Handle handle = gin::CreateHandle( isolate, new WebContents(isolate, std::move(web_contents), type)); + gin_helper::CallMethod(isolate, handle.get(), "_init"); + return handle; } // static WebContents* WebContents::From(content::WebContents* web_contents) { - v8::Isolate* isolate = JavascriptEnvironment::GetIsolate(); - auto* existing = TrackableObject::FromWrappedClass(isolate, web_contents); - return static_cast(existing); + if (!web_contents) + return nullptr; + UserDataLink* data = static_cast( + web_contents->GetUserData(kElectronApiWebContentsKey)); + return data ? data->web_contents.get() : nullptr; } // static @@ -2906,36 +2969,50 @@ gin::Handle WebContents::FromOrCreate( v8::Isolate* isolate, content::WebContents* web_contents) { WebContents* api_web_contents = From(web_contents); - if (!api_web_contents) + if (!api_web_contents) { api_web_contents = new WebContents(isolate, web_contents); + gin_helper::CallMethod(isolate, api_web_contents, "_init"); + } return gin::CreateHandle(isolate, api_web_contents); } // static WebContents* WebContents::FromID(int32_t id) { - return FromWeakMapID(JavascriptEnvironment::GetIsolate(), id); + return GetAllWebContents().Lookup(id); } +// static +gin::WrapperInfo WebContents::kWrapperInfo = {gin::kEmbedderNativeGin}; + } // namespace api } // namespace electron namespace { +using electron::api::GetAllWebContents; using electron::api::WebContents; +std::vector> GetAllWebContentsAsV8( + v8::Isolate* isolate) { + std::vector> list; + for (auto iter = base::IDMap::iterator(&GetAllWebContents()); + !iter.IsAtEnd(); iter.Advance()) { + list.push_back(gin::CreateHandle(isolate, iter.GetCurrentValue())); + } + return list; +} + void Initialize(v8::Local exports, v8::Local unused, v8::Local context, void* priv) { v8::Isolate* isolate = context->GetIsolate(); gin_helper::Dictionary dict(isolate, exports); - dict.Set("WebContents", WebContents::GetConstructor(isolate) - ->GetFunction(context) - .ToLocalChecked()); + dict.Set("WebContents", WebContents::GetConstructor(context)); dict.SetMethod("create", &WebContents::Create); - dict.SetMethod("fromId", &WebContents::FromWeakMapID); - dict.SetMethod("getAllWebContents", &WebContents::GetAll); + dict.SetMethod("fromId", &WebContents::FromID); + dict.SetMethod("getAllWebContents", &GetAllWebContentsAsV8); } } // namespace diff --git a/shell/browser/api/electron_api_web_contents.h b/shell/browser/api/electron_api_web_contents.h index 9823d6798d4c..77ffec27985e 100644 --- a/shell/browser/api/electron_api_web_contents.h +++ b/shell/browser/api/electron_api_web_contents.h @@ -8,6 +8,7 @@ #include #include #include +#include #include #include "base/observer_list.h" @@ -22,14 +23,17 @@ #include "electron/buildflags/buildflags.h" #include "electron/shell/common/api/api.mojom.h" #include "gin/handle.h" +#include "gin/wrappable.h" #include "mojo/public/cpp/bindings/receiver_set.h" #include "printing/buildflags/buildflags.h" #include "services/service_manager/public/cpp/binder_registry.h" #include "shell/browser/api/frame_subscriber.h" #include "shell/browser/api/save_page_handler.h" #include "shell/browser/common_web_contents_delegate.h" +#include "shell/browser/event_emitter_mixin.h" +#include "shell/common/gin_helper/cleaned_up_at_exit.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 "ui/gfx/image/image.h" #if BUILDFLAG(ENABLE_PRINTING) @@ -131,7 +135,10 @@ class ExtendedWebContentsObserver : public base::CheckedObserver { }; // Wrapper around the content::WebContents. -class WebContents : public gin_helper::TrackableObject, +class WebContents : public gin::Wrappable, + public gin_helper::EventEmitterMixin, + public gin_helper::Constructible, + public gin_helper::CleanedUpAtExit, public CommonWebContentsDelegate, public content::WebContentsObserver, public mojom::ElectronBrowser { @@ -148,6 +155,8 @@ class WebContents : public gin_helper::TrackableObject, // Create a new WebContents and return the V8 wrapper of it. static gin::Handle Create(v8::Isolate* isolate, const gin_helper::Dictionary& options); + static gin::Handle New(v8::Isolate* isolate, + const gin_helper::Dictionary& options); // Create a new V8 wrapper for an existing |web_content|. // @@ -170,8 +179,12 @@ class WebContents : public gin_helper::TrackableObject, v8::Isolate* isolate, content::WebContents* web_contents); - static void BuildPrototype(v8::Isolate* isolate, - v8::Local prototype); + // gin::Wrappable + static gin::WrapperInfo kWrapperInfo; + static v8::Local FillObjectTemplate( + v8::Isolate*, + v8::Local); + const char* GetTypeName() override; base::WeakPtr GetWeakPtr() { return weak_factory_.GetWeakPtr(); } @@ -375,7 +388,7 @@ class WebContents : public gin_helper::TrackableObject, const base::FilePath& file_path); // Properties. - int32_t ID() const; + int32_t ID() const { return id_; } v8::Local Session(v8::Isolate* isolate); content::WebContents* HostWebContents() const; v8::Local DevToolsWebContents(v8::Isolate* isolate); @@ -395,6 +408,25 @@ class WebContents : public gin_helper::TrackableObject, bool EmitNavigationEvent(const std::string& event, content::NavigationHandle* navigation_handle); + // this.emit(name, new Event(sender, message), args...); + template + bool EmitWithSender(base::StringPiece name, + content::RenderFrameHost* sender, + electron::mojom::ElectronBrowser::InvokeCallback callback, + Args&&... args) { + DCHECK_CURRENTLY_ON(content::BrowserThread::UI); + v8::Isolate* isolate = JavascriptEnvironment::GetIsolate(); + v8::HandleScope handle_scope(isolate); + v8::Local wrapper; + if (!GetWrapper(isolate).ToLocal(&wrapper)) + return false; + v8::Local event = gin_helper::internal::CreateNativeEvent( + isolate, wrapper, sender, std::move(callback)); + return EmitCustomEvent(name, event, std::forward(args)...); + } + + void MarkDestroyed(); + WebContents* embedder() { return embedder_; } #if BUILDFLAG(ENABLE_ELECTRON_EXTENSIONS) @@ -637,6 +669,8 @@ class WebContents : public gin_helper::TrackableObject, // The type of current WebContents. Type type_ = Type::BROWSER_WINDOW; + int32_t id_; + // Request id used for findInPage request. uint32_t request_id_ = 0; diff --git a/shell/browser/event_emitter_mixin.h b/shell/browser/event_emitter_mixin.h index f003e4265a38..2270dfdc1284 100644 --- a/shell/browser/event_emitter_mixin.h +++ b/shell/browser/event_emitter_mixin.h @@ -44,9 +44,7 @@ class EventEmitterMixin { v8::Local wrapper; if (!static_cast(this)->GetWrapper(isolate).ToLocal(&wrapper)) return false; - v8::Local event = - internal::CreateEvent(isolate, wrapper, custom_event); - return EmitWithEvent(isolate, wrapper, name, event, + return EmitWithEvent(isolate, wrapper, name, custom_event, std::forward(args)...); } diff --git a/shell/common/gin_helper/constructor.h b/shell/common/gin_helper/constructor.h index b4e6e3487b80..73b5dbed1ca4 100644 --- a/shell/common/gin_helper/constructor.h +++ b/shell/common/gin_helper/constructor.h @@ -157,7 +157,7 @@ v8::Local CreateConstructor( CHECK(!called) << "CreateConstructor can only be called for one type once"; called = true; #endif - v8::Local templ = CreateFunctionTemplate( + v8::Local templ = gin_helper::CreateFunctionTemplate( isolate, base::BindRepeating(&internal::InvokeNew, func)); templ->InstanceTemplate()->SetInternalFieldCount(1); T::BuildPrototype(isolate, templ); diff --git a/shell/common/gin_helper/event_emitter.h b/shell/common/gin_helper/event_emitter.h index 9b0d5aaf82d0..92be7098e911 100644 --- a/shell/common/gin_helper/event_emitter.h +++ b/shell/common/gin_helper/event_emitter.h @@ -70,23 +70,6 @@ class EventEmitter : public gin_helper::Wrappable { return EmitWithEvent(name, event, std::forward(args)...); } - // this.emit(name, new Event(sender, message), args...); - template - bool EmitWithSender(base::StringPiece name, - content::RenderFrameHost* sender, - electron::mojom::ElectronBrowser::InvokeCallback callback, - Args&&... args) { - DCHECK_CURRENTLY_ON(content::BrowserThread::UI); - v8::Locker locker(isolate()); - v8::HandleScope handle_scope(isolate()); - v8::Local wrapper = GetWrapper(); - if (wrapper.IsEmpty()) - return false; - v8::Local event = internal::CreateNativeEvent( - isolate(), wrapper, sender, std::move(callback)); - return EmitWithEvent(name, event, std::forward(args)...); - } - protected: EventEmitter() {}