Remove the is_loading_ field from WebContentsImpl

content/browser/frame_host/render_frame_host_impl.cc

Index: content/browser/frame_host/render_frame_host_impl.cc
diff --git a/content/browser/frame_host/render_frame_host_impl.cc b/content/browser/frame_host/render_frame_host_impl.cc
index e57bde68e4829cdaf80a02109f7ab186fcc6b4f7..a3f5a78c0876168eb0091f5bad37c39fe13df55c 100644
--- a/content/browser/frame_host/render_frame_host_impl.cc
+++ b/content/browser/frame_host/render_frame_host_impl.cc
@@ -213,6 +213,7 @@ RenderFrameHostImpl::RenderFrameHostImpl(SiteInstance* site_instance,
   g_routing_id_frame_map.Get().insert(std::make_pair(
       RenderFrameHostID(GetProcess()->GetID(), routing_id_),
       this));
+  site_instance_->AddObserver(this);
 
   if (is_swapped_out) {
     rfh_state_ = STATE_SWAPPED_OUT;
@@ -258,6 +259,8 @@ RenderFrameHostImpl::~RenderFrameHostImpl() {
   g_routing_id_frame_map.Get().erase(
       RenderFrameHostID(GetProcess()->GetID(), routing_id_));
 
+  site_instance_->RemoveObserver(this);
+
   if (delegate_ && render_frame_created_)
     delegate_->RenderFrameDeleted(this);
 
@@ -513,7 +516,6 @@ bool RenderFrameHostImpl::OnMessageReceived(const IPC::Message &msg) {
                         OnDidFailLoadWithError)
     IPC_MESSAGE_HANDLER_GENERIC(FrameHostMsg_DidCommitProvisionalLoad,
                                 OnDidCommitProvisionalLoad(msg))
-    IPC_MESSAGE_HANDLER(FrameHostMsg_DidDropNavigation, OnDidDropNavigation)
     IPC_MESSAGE_HANDLER(FrameHostMsg_UpdateState, OnUpdateState)
     IPC_MESSAGE_HANDLER(FrameHostMsg_OpenURL, OnOpenURL)
     IPC_MESSAGE_HANDLER(FrameHostMsg_DocumentOnLoadCompleted,
@@ -686,6 +688,16 @@ gfx::NativeViewAccessible
   return NULL;
 }
 
+void RenderFrameHostImpl::ActiveFrameCountIsZero(
+    SiteInstanceImpl* site_instance) {}
+
+void RenderFrameHostImpl::RenderProcessGone(SiteInstanceImpl* site_instance) {

[ncarter (slow), 2016/01/21 00:26:40]

I'm glad you've done this. I also have envisioned making RenderFrameHostImpl be
a SiteInstanceImpl::Observer, as a way of replacing
FrameHostMsg_RenderProcessGone. 

Note this bug: https://code.google.com/p/chromium/issues/detail?id=575400

+  DCHECK(site_instance == site_instance_.get());
+
+  // The render process is gone, this frame can no longer be loading.
+  ResetLoadingState();

[nasko, 2016/01/20 20:58:34]

Let's not add implementation of SiteInstanceImpl::Observer here, since it
requires adding an empty method. RFHM is already an observer and is already
changing to support calling ResetLoadingState() on its RFH. Let's just move this
code to its implementation of RenderProcessGone.

[ncarter (slow), 2016/01/21 00:26:40]

I'm not sure I agree with nasko. I think it's more natural for the RFH to be the
SiteInstanceImpl::Observer.

RFHM currently observes the siteinstance on behalf of its proxies, and you can't
observe the same siteinstance twice, it seems troublesome for the RFHM to avoid
double-observation if it has to observe on behalf of its RFH's as well. We could
potentially wind up with a proxy in the same siteinstance as a pending delete
host, say -- so you'd need some kind of refcounting scheme.

Moreover, if RFH (instead of RFHM) observes the SiteInstance, then we get RAII
guarantees that AddObserver/RemoveObserver calls are balanced. If we do it in
the RFHM then it's yet another thing we have to manage manually for all the
various speculative/pending/ subcases.

[clamy, 2016/01/22 13:08:15]

I initially tried to make things work in RFHM::RenderProcessGone, but getting to
observe the right SiteInstances was somewhat complex. In particular, I was
worried about some edge cases (such as when we create a pending RFH for a
SiteInstance that was a before a RFHP and that pending RFH later gets destroyed
and we recreate a RFHP for it).

I think it is much easier conceptually to have RFH be the SiteInstanceObserver:
you start observing the SiteInstance at creation of the RFH and stop at
deletion, and just reset your own loading state when it is gone. As for the
empty method, if we go on the route of RFH being an observer, can we just change
the SiteInstanceObserver::ActiveFrameCountIsZero from pure virtual to just
virtual?

[nasko, 2016/01/22 17:52:59]

I would prefer not to have the empty method here.

[clamy, 2016/01/25 14:01:18]

I made it non pure virtual in SiteInstanceImpl::Observer. Let me know what you
think.

+}
+
 bool RenderFrameHostImpl::CreateRenderFrame(int proxy_routing_id,
                                             int opener_routing_id,
                                             int parent_routing_id,
@@ -1037,15 +1049,6 @@ void RenderFrameHostImpl::OnDidCommitProvisionalLoad(const IPC::Message& msg) {
     pending_commit_ = false;
 }
 
-void RenderFrameHostImpl::OnDidDropNavigation() {
-  // At the end of Navigate(), the FrameTreeNode's DidStartLoading is called to
-  // force the spinner to start, even if the renderer didn't yet begin the load.
-  // If it turns out that the renderer dropped the navigation, the spinner needs
-  // to be turned off.
-  frame_tree_node_->DidStopLoading();
-  navigation_handle_.reset();
-}
-
 void RenderFrameHostImpl::OnUpdateState(const PageState& state) {
   // TODO(creis): Verify the state's ISN matches the last committed FNE.
 
@@ -1307,6 +1310,8 @@ void RenderFrameHostImpl::OnSwappedOut() {
     return;
   }
 
+  ResetLoadingState();
+
   // If this RFH wasn't pending deletion, then it is now swapped out.
   SetState(RenderFrameHostImpl::STATE_SWAPPED_OUT);
 }
@@ -1692,21 +1697,6 @@ void RenderFrameHostImpl::OnToggleFullscreen(bool enter_fullscreen) {
 }
 
 void RenderFrameHostImpl::OnDidStartLoading(bool to_different_document) {
-  // Any main frame load to a new document should reset the load since it will
-  // replace the current page and any frames.
-  if (to_different_document && !GetParent())
-    is_loading_ = false;
-
-  // This method should never be called when the frame is loading.
-  // Unfortunately, it can happen if a history navigation happens during a
-  // BeforeUnload or Unload event.
-  // TODO(fdegans): Change this to a DCHECK after LoadEventProgress has been
-  // refactored in Blink. See crbug.com/466089
-  if (is_loading_) {
-    LOG(WARNING) << "OnDidStartLoading was called twice.";
-    return;
-  }
-
   frame_tree_node_->DidStartLoading(to_different_document);
   is_loading_ = true;
 }
@@ -1903,8 +1893,10 @@ void RenderFrameHostImpl::Navigate(
   //
   // Blink doesn't send throb notifications for JavaScript URLs, so it is not
   // done here either.
-  if (!common_params.url.SchemeIs(url::kJavaScriptScheme))
+  if (!common_params.url.SchemeIs(url::kJavaScriptScheme)) {
     frame_tree_node_->DidStartLoading(true);
+    is_loading_ = true;
+  }
 }
 
 void RenderFrameHostImpl::NavigateToInterstitialURL(const GURL& data_url) {
@@ -2245,6 +2237,17 @@ RenderFrameHostImpl::GetMojoImageDownloader() {
   return mojo_image_downloader_;
 }
 
+void RenderFrameHostImpl::ResetLoadingState() {
+  if (is_loading()) {
+    // Do not send notifications about load state when swapped out or pending
+    // swap out.
+    if (rfh_state_ == STATE_DEFAULT)
+      OnDidStopLoading();
+    else
+      is_loading_ = false;

[nasko, 2016/01/20 20:58:34]

Shouldn't this always be set, regardless of state? Also, swapped out is now
disabled, so we can consider it gone, so let's avoid using it in comments. It
can be rephrased as "when pending deletion."

[clamy, 2016/01/22 13:08:15]

OnDidStopLoading will set it. If it is set beforehand, then OnDidStopLoading
just returns early without informing the tree of the change in load state.

I have updated the comment to make it clearer.

+  }
+}
+
 bool RenderFrameHostImpl::IsSameSiteInstance(
     RenderFrameHostImpl* other_render_frame_host) {
   // As a sanity check, make sure the frame belongs to the same BrowserContext.