diff --git a/atom/browser/api/atom_api_view.h b/atom/browser/api/atom_api_view.h index b1fb38e4c439..4e851ef28983 100644 --- a/atom/browser/api/atom_api_view.h +++ b/atom/browser/api/atom_api_view.h @@ -7,14 +7,14 @@ #include -#include "atom/browser/api/event_emitter.h" +#include "atom/browser/api/trackable_object.h" #include "ui/views/view.h" namespace atom { namespace api { -class View : public mate::EventEmitter { +class View : public mate::TrackableObject { public: static mate::WrappableBase* New(mate::Arguments* args); diff --git a/atom/browser/api/atom_api_web_contents_view.cc b/atom/browser/api/atom_api_web_contents_view.cc index 5bfd94dfee94..de27f8eeed64 100644 --- a/atom/browser/api/atom_api_web_contents_view.cc +++ b/atom/browser/api/atom_api_web_contents_view.cc @@ -6,6 +6,7 @@ #include "atom/browser/api/atom_api_web_contents.h" #include "brightray/browser/inspectable_web_contents_view.h" +#include "content/public/browser/web_contents_user_data.h" #include "native_mate/dictionary.h" #if defined(OS_MACOSX) @@ -14,20 +15,41 @@ #include "atom/common/node_includes.h" +namespace { + +// Used to indicate whether a WebContents already has a view. +class WebContentsViewRelay + : public content::WebContentsUserData { + public: + ~WebContentsViewRelay() override {} + + private: + explicit WebContentsViewRelay(content::WebContents* contents) {} + friend class content::WebContentsUserData; + + atom::api::WebContentsView* view_ = nullptr; + + DISALLOW_COPY_AND_ASSIGN(WebContentsViewRelay); +}; + +} // namespace + +DEFINE_WEB_CONTENTS_USER_DATA_KEY(WebContentsViewRelay); + namespace atom { namespace api { -WebContentsView::WebContentsView( - v8::Isolate* isolate, - v8::Local web_contents_wrapper, - brightray::InspectableWebContents* web_contents) +WebContentsView::WebContentsView(v8::Isolate* isolate, + mate::Handle web_contents, + brightray::InspectableWebContents* iwc) #if defined(OS_MACOSX) - : View(new DelayedNativeViewHost(web_contents->GetView()->GetNativeView())), + : View(new DelayedNativeViewHost(iwc->GetView()->GetNativeView())), #else - : View(web_contents->GetView()->GetView()), + : View(iwc->GetView()->GetView()), #endif - web_contents_wrapper_(isolate, web_contents_wrapper) { + web_contents_(isolate, web_contents->GetWrapper()), + api_web_contents_(web_contents.get()) { #if defined(OS_MACOSX) // On macOS a View is created to wrap the NSView, and its lifetime is managed // by us. @@ -36,21 +58,42 @@ WebContentsView::WebContentsView( // On other platforms the View is managed by InspectableWebContents. set_delete_view(false); #endif + WebContentsViewRelay::CreateForWebContents(web_contents->web_contents()); + Observe(web_contents->web_contents()); } -WebContentsView::~WebContentsView() {} +WebContentsView::~WebContentsView() { + if (api_web_contents_) + api_web_contents_->DestroyWebContents(false /* async */); +} + +void WebContentsView::WebContentsDestroyed() { + api_web_contents_ = nullptr; + web_contents_.Reset(); +} // static mate::WrappableBase* WebContentsView::New( mate::Arguments* args, mate::Handle 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()) { const char* error = "The WebContents must be created by user"; args->isolate()->ThrowException( v8::Exception::Error(mate::StringToV8(args->isolate(), error))); return nullptr; } - auto* view = new WebContentsView(args->isolate(), web_contents->GetWrapper(), + // Check if the WebContents has already been added to a view. + if (WebContentsViewRelay::FromWebContents(web_contents->web_contents())) { + const char* error = "The WebContents has already been added to a View"; + args->isolate()->ThrowException( + v8::Exception::Error(mate::StringToV8(args->isolate(), error))); + return nullptr; + } + // Constructor call. + auto* view = new WebContentsView(args->isolate(), web_contents, web_contents->managed_web_contents()); view->InitWith(args->isolate(), args->GetThis()); return view; diff --git a/atom/browser/api/atom_api_web_contents_view.h b/atom/browser/api/atom_api_web_contents_view.h index e3cf96f99085..d502918a155a 100644 --- a/atom/browser/api/atom_api_web_contents_view.h +++ b/atom/browser/api/atom_api_web_contents_view.h @@ -6,6 +6,7 @@ #define ATOM_BROWSER_API_ATOM_API_WEB_CONTENTS_VIEW_H_ #include "atom/browser/api/atom_api_view.h" +#include "content/public/browser/web_contents_observer.h" #include "native_mate/handle.h" namespace brightray { @@ -18,7 +19,7 @@ namespace api { class WebContents; -class WebContentsView : public View { +class WebContentsView : public View, public content::WebContentsObserver { public: static mate::WrappableBase* New(mate::Arguments* args, mate::Handle web_contents); @@ -28,13 +29,17 @@ class WebContentsView : public View { protected: WebContentsView(v8::Isolate* isolate, - v8::Local web_contents_wrapper, - brightray::InspectableWebContents* web_contents); + mate::Handle web_contents, + brightray::InspectableWebContents* iwc); ~WebContentsView() override; + // content::WebContentsObserver: + void WebContentsDestroyed() override; + private: // Keep a reference to v8 wrapper. - v8::Global web_contents_wrapper_; + v8::Global web_contents_; + api::WebContents* api_web_contents_; DISALLOW_COPY_AND_ASSIGN(WebContentsView); }; diff --git a/atom/browser/atom_browser_main_parts.cc b/atom/browser/atom_browser_main_parts.cc index 8cccac09931f..6c071a318b5f 100644 --- a/atom/browser/atom_browser_main_parts.cc +++ b/atom/browser/atom_browser_main_parts.cc @@ -121,7 +121,10 @@ int AtomBrowserMainParts::GetExitCode() { void AtomBrowserMainParts::RegisterDestructionCallback( base::OnceClosure callback) { - destructors_.insert(destructors_.end(), std::move(callback)); + // The destructors should be called in reversed order, so dependencies between + // JavaScript objects can be correctly resolved. + // For example WebContentsView => WebContents => Session. + destructors_.insert(destructors_.begin(), std::move(callback)); } void AtomBrowserMainParts::PreEarlyInitialization() { diff --git a/spec/api-view-spec.js b/spec/api-view-spec.js new file mode 100644 index 000000000000..a47283b124d5 --- /dev/null +++ b/spec/api-view-spec.js @@ -0,0 +1,14 @@ +'use strict' + +const {closeWindow} = require('./window-helpers') +const {TopLevelWindow, View} = require('electron').remote + +describe('View', () => { + let w = null + afterEach(() => closeWindow(w).then(() => { w = null })) + + it('can be used as content view', () => { + w = new TopLevelWindow({show: false}) + w.setContentView(new View()) + }) +}) diff --git a/spec/api-web-contents-view-spec.js b/spec/api-web-contents-view-spec.js new file mode 100644 index 000000000000..b61ddb616386 --- /dev/null +++ b/spec/api-web-contents-view-spec.js @@ -0,0 +1,25 @@ +'use strict' + +const assert = require('assert') +const {closeWindow} = require('./window-helpers') +const {webContents, TopLevelWindow, WebContentsView} = require('electron').remote + +describe('WebContentsView', () => { + let w = null + afterEach(() => closeWindow(w).then(() => { w = null })) + + it('can be used as content view', () => { + const web = webContents.create({}) + w = new TopLevelWindow({show: false}) + w.setContentView(new WebContentsView(web)) + }) + + it('prevents adding same WebContents', () => { + const web = webContents.create({}) + w = new TopLevelWindow({show: false}) + w.setContentView(new WebContentsView(web)) + assert.throws(() => { + w.setContentView(new WebContentsView(web)) + }, /The WebContents has already been added to a View/) + }) +})