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

@@ -220,8 +220,7 @@ WebContents::WebContents(content::WebContents* web_contents)
       embedder_(nullptr),
       type_(REMOTE),
       request_id_(0),
-      background_throttling_(true),
+      background_throttling_(true) {
-      is_loading_main_frame_(false) {
   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),
+      background_throttling_(true) {
-      is_loading_main_frame_(false) {
   // 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 {

atom/browser/api/atom_api_web_contents.h

@@ -224,21 +224,11 @@ class WebContents : public mate::TrackableObject<WebContents>,
                    int error_code,
                    const base::string16& error_description,
                    bool was_ignored_by_handler) override;
-  void DidFailProvisionalLoad(
+  void DidFailProvisionalLoad(content::RenderFrameHost* render_frame_host,
-      content::RenderFrameHost* render_frame_host,
+                              const GURL& validated_url,
-      const GURL& validated_url,
+                              int error_code,
-      int error_code,
+                              const base::string16& error_description,
-      const base::string16& error_description,
+                              bool was_ignored_by_handler) override;
-      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 DidStartLoading() override;
   void DidStopLoading() override;
   void DidGetResourceResponseStart(
@@ -313,9 +303,6 @@ class WebContents : public mate::TrackableObject<WebContents>,
   // 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);
 };
 

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)
     })
   })