Remove the is_loading_ field from WebContentsImpl

content/browser/web_contents/web_contents_impl.cc

Index: content/browser/web_contents/web_contents_impl.cc
diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc
index 40727c5ae0662d8b72733b4431c763a86cb73ce9..6d56113650b887f5d3cace3cc123c5dd39b13d34 100644
--- a/content/browser/web_contents/web_contents_impl.cc
+++ b/content/browser/web_contents/web_contents_impl.cc
@@ -229,6 +229,11 @@ void ResetAccessibility(RenderFrameHost* rfh) {
   static_cast<RenderFrameHostImpl*>(rfh)->AccessibilityReset();
 }
 
+bool NotifyRenderViewTerminated(RenderViewHostImpl* rvh, FrameTreeNode* ftn) {
+  ftn->RenderViewTerminated(rvh);
+  return true;
+}
+
 }  // namespace
 
 WebContents* WebContents::Create(const WebContents::CreateParams& params) {
@@ -373,7 +378,6 @@ WebContentsImpl::WebContentsImpl(BrowserContext* browser_context)
                   this,
                   this,
                   this),
-      is_loading_(false),
       is_load_to_different_document_(false),
       crashed_status_(base::TERMINATION_STATUS_STILL_RUNNING),
       crashed_error_code_(0),
@@ -382,6 +386,7 @@ WebContentsImpl::WebContentsImpl(BrowserContext* browser_context)
       upload_size_(0),
       upload_position_(0),
       is_resume_pending_(false),
+      pause_throbber_for_interstitial_(false),
       displayed_insecure_content_(false),
       has_accessed_initial_document_(false),
       theme_color_(SK_ColorTRANSPARENT),
@@ -406,8 +411,8 @@ WebContentsImpl::WebContentsImpl(BrowserContext* browser_context)
       force_disable_overscroll_content_(false),
       last_dialog_suppressed_(false),
       geolocation_service_context_(new GeolocationServiceContext()),
-      accessibility_mode_(
-          BrowserAccessibilityStateImpl::GetInstance()->accessibility_mode()),
+      accessibility_mode_(BrowserAccessibilityStateImpl::GetInstance()
+                              ->accessibility_mode()),
       audio_stream_monitor_(this),
       virtual_keyboard_requested_(false),
       page_scale_factor_is_one_(true),
@@ -893,7 +898,7 @@ void WebContentsImpl::SetUserAgentOverride(const std::string& override) {
   // Reload the page if a load is currently in progress to avoid having
   // different parts of the page loaded using different user agents.
   NavigationEntry* entry = controller_.GetVisibleEntry();
-  if (is_loading_ && entry != NULL && entry->GetIsOverridingUserAgent())
+  if (IsLoading() && entry != NULL && entry->GetIsOverridingUserAgent())
     controller_.ReloadIgnoringCache(true);
 
   FOR_EACH_OBSERVER(WebContentsObserver, observers_,
@@ -1031,11 +1036,11 @@ SiteInstanceImpl* WebContentsImpl::GetPendingSiteInstance() const {
 }
 
 bool WebContentsImpl::IsLoading() const {
-  return is_loading_;
+  return frame_tree_.IsLoading() && !pause_throbber_for_interstitial_;

[nasko, 2016/01/15 01:19:16]

I still think the page is in loading state, even if it is displaying an
interstitial page. A document has been requested and it hasn't been fully loaded
yet. The fact that it is paused because we chose to ask the user something
doesn't mean that it stopped loading.

[clamy, 2016/01/15 16:47:44]

I'm just matching the current behavior here. When we show the interstitial, we
set is_loading_ in the WebContents to false, so WebContentsImpl::IsLoading will
return false. If I remember correctly, not doing it caused issues with some
browser tests.

 }
 
 bool WebContentsImpl::IsLoadingToDifferentDocument() const {
-  return is_loading_ && is_load_to_different_document_;
+  return IsLoading() && is_load_to_different_document_;
 }
 
 bool WebContentsImpl::IsWaitingForResponse() const {
@@ -2294,6 +2299,21 @@ void WebContentsImpl::AttachInterstitialPage(
 
   FOR_EACH_OBSERVER(WebContentsObserver, observers_,
                     DidAttachInterstitialPage());
+
+  // Stop the throbber if needed while the interstitial page is shown.
+  if (frame_tree_.IsLoading()) {

[nasko, 2016/01/15 01:19:16]

What if it is already paused?

[clamy, 2016/01/15 16:47:44]

Done.

+    LoadingStateChanged(false, true, NULL);

[nasko, 2016/01/15 01:19:16]

nullptr

[clamy, 2016/01/15 16:47:44]

Done.

+  }
+  pause_throbber_for_interstitial_ = true;

[nasko, 2016/01/15 01:19:16]

If this is state, then it should be "paused".

[clamy, 2016/01/15 16:47:44]

Done.

+}
+
+void WebContentsImpl::DidProceedOnIntertitial() {
+  // Restart the throbber now that the interstitial page is going away.
+  if (pause_throbber_for_interstitial_) {
+    pause_throbber_for_interstitial_ = false;
+    if (frame_tree_.IsLoading())
+      LoadingStateChanged(true, true, NULL);
+  }
 }
 
 void WebContentsImpl::DetachInterstitialPage() {
@@ -2301,6 +2321,12 @@ void WebContentsImpl::DetachInterstitialPage() {
     GetRenderManager()->remove_interstitial_page();
   FOR_EACH_OBSERVER(WebContentsObserver, observers_,
                     DidDetachInterstitialPage());
+  // Restart the throbber now that the interstitial page is going away.
+  if (pause_throbber_for_interstitial_) {

[nasko, 2016/01/15 01:19:16]

Looking at the usage here, isn't this equivalent to the value of
ShowingInterstitialPage()?

[clamy, 2016/01/15 16:47:44]

No if the user starts a new navigation, DidProceedOnInterstitial gets called,
the throbber is restarted but the interstitial is still showing. I tried to just
use ShowingInterstitialPage(), but it did not work due to this particular case.

+    pause_throbber_for_interstitial_ = false;
+    if (frame_tree_.IsLoading())
+      LoadingStateChanged(true, true, NULL);
+  }
 }
 
 void WebContentsImpl::SetHistoryOffsetAndLength(int history_offset,
@@ -3575,10 +3601,11 @@ bool WebContentsImpl::HasAccessedInitialDocument() {
 
 // Notifies the RenderWidgetHost instance about the fact that the page is
 // loading, or done loading.
-void WebContentsImpl::SetIsLoading(bool is_loading,
-                                   bool to_different_document,
-                                   LoadNotificationDetails* details) {
-  if (is_loading == is_loading_)
+void WebContentsImpl::LoadingStateChanged(bool is_loading,
+                                          bool to_different_document,
+                                          LoadNotificationDetails* details) {
+  // Do not send notifications about loading while the interstitial is showing.
+  if (pause_throbber_for_interstitial_)
     return;
 
   if (!is_loading) {
@@ -3591,7 +3618,6 @@ void WebContentsImpl::SetIsLoading(bool is_loading,
 
   GetRenderManager()->SetIsLoading(is_loading);
 
-  is_loading_ = is_loading;
   waiting_for_response_ = is_loading;
   is_load_to_different_document_ = to_different_document;
 
@@ -3944,16 +3970,11 @@ void WebContentsImpl::RenderViewTerminated(RenderViewHost* rvh,
   if (delegate_)
     delegate_->HideValidationMessage(this);
 
-  SetIsLoading(false, true, nullptr);
+  frame_tree_.ForEach(base::Bind(&NotifyRenderViewTerminated,
+                                 static_cast<RenderViewHostImpl*>(rvh)));

[nasko, 2016/01/15 01:19:16]

This method (RenderViewTerminated) is called when the process has died. There
are already enough hooks in RFHM and elsewhere that is notified of this event,
so I don't think we need yet more ways to notify hosts of the process dying.

[clamy, 2016/01/15 16:47:44]

Done.

   NotifyDisconnected();
   SetIsCrashed(status, error_code);
 
-  // Reset the loading progress. TODO(avi): What does it mean to have a
-  // "renderer crash" when there is more than one renderer process serving a
-  // webpage? Once this function is called at a more granular frame level, we
-  // probably will need to more granularly reset the state here.
-  ResetLoadProgressState();
-
   FOR_EACH_OBSERVER(WebContentsObserver,
                     observers_,
                     RenderProcessGone(GetCrashedStatus()));
@@ -4046,7 +4067,7 @@ void WebContentsImpl::RequestMove(const gfx::Rect& new_bounds) {
 
 void WebContentsImpl::DidStartLoading(FrameTreeNode* frame_tree_node,
                                       bool to_different_document) {
-  SetIsLoading(true, to_different_document, nullptr);
+  LoadingStateChanged(true, to_different_document, nullptr);
 
   // Notify accessibility that the user is navigating away from the
   // current document.
@@ -4080,7 +4101,7 @@ void WebContentsImpl::DidStopLoading() {
         controller_.GetCurrentEntryIndex()));
   }
 
-  SetIsLoading(false, true, details.get());
+  LoadingStateChanged(false, true, details.get());
 }
 
 void WebContentsImpl::DidChangeLoadProgress() {
@@ -4617,10 +4638,8 @@ void WebContentsImpl::OnDialogClosed(int render_process_id,
   last_dialog_suppressed_ = dialog_was_suppressed;
 
   if (is_showing_before_unload_dialog_ && !success) {
-    // If a beforeunload dialog is canceled, we need to stop the throbber from
-    // spinning, since we forced it to start spinning in Navigate.
     if (rfh)
-      DidStopLoading();
+      rfh->frame_tree_node()->BeforeUnloadCanceled();
     controller_.DiscardNonCommittedEntries();
 
     FOR_EACH_OBSERVER(WebContentsObserver, observers_,