From d3e879cd7f3cc7093ebc9246eaccd2b5857faebb Mon Sep 17 00:00:00 2001 From: Rob Brackett Date: Tue, 19 Apr 2016 22:05:09 -0700 Subject: [PATCH] Change `WebContents::IsLoadingMainFrame` to compare SiteInstances (per @deepak1556's recommendation) Also updates tests to cover the situation where navigating between pages from the same potential "site" and adds generalized tests for `isLoadingMainFrame()`. --- atom/browser/api/atom_api_web_contents.cc | 34 +++------- atom/browser/api/atom_api_web_contents.h | 23 ++----- spec/api-browser-window-spec.js | 76 +++++++++++++++++------ 3 files changed, 69 insertions(+), 64 deletions(-) diff --git a/atom/browser/api/atom_api_web_contents.cc b/atom/browser/api/atom_api_web_contents.cc index 7c7e5896abc..e39fa985e26 100644 --- a/atom/browser/api/atom_api_web_contents.cc +++ b/atom/browser/api/atom_api_web_contents.cc @@ -220,8 +220,7 @@ WebContents::WebContents(content::WebContents* web_contents) embedder_(nullptr), type_(REMOTE), request_id_(0), - background_throttling_(true), - is_loading_main_frame_(false) { + background_throttling_(true) { AttachAsUserData(web_contents); web_contents->SetUserAgentOverride(GetBrowserContext()->GetUserAgent()); } @@ -230,8 +229,7 @@ WebContents::WebContents(v8::Isolate* isolate, const mate::Dictionary& options) : embedder_(nullptr), request_id_(0), - background_throttling_(true), - is_loading_main_frame_(false) { + background_throttling_(true) { // Read options. options.Get("backgroundThrottling", &background_throttling_); @@ -545,32 +543,12 @@ void WebContents::DocumentLoadedInFrame( void WebContents::DidFinishLoad(content::RenderFrameHost* render_frame_host, const GURL& validated_url) { bool is_main_frame = !render_frame_host->GetParent(); - if (is_main_frame) - is_loading_main_frame_ = false; - Emit("did-frame-finish-load", is_main_frame); if (is_main_frame) Emit("did-finish-load"); } -void WebContents::DidStartProvisionalLoadForFrame( - content::RenderFrameHost* render_frame_host, - const GURL& url, - bool is_error_page, - bool is_iframe_srcdoc) { - if (!render_frame_host->GetParent()) - is_loading_main_frame_ = true; -} - -void WebContents::DidCommitProvisionalLoadForFrame( - content::RenderFrameHost* render_frame_host, - const GURL& url, - ui::PageTransition transition_type) { - if (!render_frame_host->GetParent()) - is_loading_main_frame_ = true; -} - void WebContents::DidFailProvisionalLoad( content::RenderFrameHost* render_frame_host, const GURL& url, @@ -578,8 +556,6 @@ void WebContents::DidFailProvisionalLoad( const base::string16& description, bool was_ignored_by_handler) { bool is_main_frame = !render_frame_host->GetParent(); - if (is_main_frame) - is_loading_main_frame_ = false; Emit("did-fail-provisional-load", code, description, url, is_main_frame); Emit("did-fail-load", code, description, url, is_main_frame); } @@ -818,7 +794,11 @@ bool WebContents::IsLoading() const { } bool WebContents::IsLoadingMainFrame() const { - return is_loading_main_frame_; + // Comparing site instances works because Electron always creates a new site + // instance when navigating, regardless of origin. See AtomBrowserClient. + return (web_contents()->GetLastCommittedURL().is_empty() || + web_contents()->GetSiteInstance() != + web_contents()->GetPendingSiteInstance()) && IsLoading(); } bool WebContents::IsWaitingForResponse() const { diff --git a/atom/browser/api/atom_api_web_contents.h b/atom/browser/api/atom_api_web_contents.h index 292ec096088..bc2d4106f5f 100644 --- a/atom/browser/api/atom_api_web_contents.h +++ b/atom/browser/api/atom_api_web_contents.h @@ -224,21 +224,11 @@ class WebContents : public mate::TrackableObject, int error_code, const base::string16& error_description, bool was_ignored_by_handler) override; - void DidFailProvisionalLoad( - content::RenderFrameHost* render_frame_host, - const GURL& validated_url, - int error_code, - const base::string16& error_description, - bool was_ignored_by_handler) override; - void DidStartProvisionalLoadForFrame( - content::RenderFrameHost* render_frame_host, - const GURL& validated_url, - bool is_error_page, - bool is_iframe_srcdoc) override; - void DidCommitProvisionalLoadForFrame( - content::RenderFrameHost* render_frame_host, - const GURL& url, - ui::PageTransition transition_type) override; + void DidFailProvisionalLoad(content::RenderFrameHost* render_frame_host, + const GURL& validated_url, + int error_code, + const base::string16& error_description, + bool was_ignored_by_handler) override; void DidStartLoading() override; void DidStopLoading() override; void DidGetResourceResponseStart( @@ -313,9 +303,6 @@ class WebContents : public mate::TrackableObject, // Whether background throttling is disabled. bool background_throttling_; - // Whether the main frame (not just a sub-frame) is currently loading. - bool is_loading_main_frame_; - DISALLOW_COPY_AND_ASSIGN(WebContents); }; diff --git a/spec/api-browser-window-spec.js b/spec/api-browser-window-spec.js index 26d4ff4af35..1110269e5de 100644 --- a/spec/api-browser-window-spec.js +++ b/spec/api-browser-window-spec.js @@ -19,6 +19,23 @@ const isCI = remote.getGlobal('isCi') describe('browser-window module', function () { var fixtures = path.resolve(__dirname, 'fixtures') var w = null + var server + + before(function (done) { + server = http.createServer(function (req, res) { + function respond() { res.end(''); } + setTimeout(respond, req.url.includes('slow') ? 200 : 0) + }); + server.listen(0, '127.0.0.1', function () { + server.url = 'http://127.0.0.1:' + server.address().port + done() + }) + }) + + after(function () { + server.close() + server = null + }) beforeEach(function () { if (w != null) { @@ -635,6 +652,44 @@ describe('browser-window module', function () { assert.equal(w.isResizable(), true) }) }) + + describe('loading main frame state', function () { + it('is true when the main frame is loading', function (done) { + w.webContents.on('did-start-loading', function() { + assert.equal(w.webContents.isLoadingMainFrame(), true) + done() + }) + w.webContents.loadURL(server.url) + }) + + it('is false when only a subframe is loading', function (done) { + w.webContents.once('did-finish-load', function() { + assert.equal(w.webContents.isLoadingMainFrame(), false) + w.webContents.on('did-start-loading', function() { + assert.equal(w.webContents.isLoadingMainFrame(), false) + done() + }) + w.webContents.executeJavaScript(` + var iframe = document.createElement('iframe') + iframe.src = '${server.url}/page2' + document.body.appendChild(iframe) + `) + }) + w.webContents.loadURL(server.url) + }) + + it('is true when navigating to pages from the same origin', function (done) { + w.webContents.once('did-finish-load', function() { + assert.equal(w.webContents.isLoadingMainFrame(), false) + w.webContents.on('did-start-loading', function() { + assert.equal(w.webContents.isLoadingMainFrame(), true) + done() + }) + w.webContents.loadURL(`${server.url}/page2`) + }) + w.webContents.loadURL(server.url) + }) + }) }) describe('window states (excluding Linux)', function () { @@ -786,14 +841,6 @@ describe('browser-window module', function () { describe('window.webContents.executeJavaScript', function () { var expected = 'hello, world!' var code = '(() => "' + expected + '")()' - var server - - afterEach(function () { - if (server) { - server.close() - server = null - } - }) it('doesnt throw when no calback is provided', function () { const result = ipcRenderer.sendSync('executeJavaScript', code, false) @@ -809,21 +856,11 @@ describe('browser-window module', function () { }) it('works after page load and during subframe load', function (done) { - var url - // a slow server, guaranteeing time to execute code during loading - server = http.createServer(function (req, res) { - setTimeout(function() { res.end('') }, 200) - }); - server.listen(0, '127.0.0.1', function () { - url = 'http://127.0.0.1:' + server.address().port - w.loadURL('file://' + path.join(fixtures, 'pages', 'base-page.html')) - }) - w.webContents.once('did-finish-load', function() { // initiate a sub-frame load, then try and execute script during it w.webContents.executeJavaScript(` var iframe = document.createElement('iframe') - iframe.src = '${url}' + iframe.src = '${server.url}/slow' document.body.appendChild(iframe) `, function() { w.webContents.executeJavaScript(`console.log('hello')`, function() { @@ -831,6 +868,7 @@ describe('browser-window module', function () { }) }) }) + w.loadURL(server.url) }) })