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 eee0310328aeb465cbe8a1ada81a11d5dc10ec82..9c91412088bb7d7a11def8a569dd6efd32b413aa 100644
--- a/content/browser/web_contents/web_contents_impl.cc
+++ b/content/browser/web_contents/web_contents_impl.cc
@@ -374,7 +374,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),
@@ -383,6 +382,7 @@ WebContentsImpl::WebContentsImpl(BrowserContext* browser_context)
       upload_size_(0),
       upload_position_(0),
       is_resume_pending_(false),
+      paused_throbber_for_interstitial_(false),
       displayed_insecure_content_(false),
       has_accessed_initial_document_(false),
       theme_color_(SK_ColorTRANSPARENT),
@@ -407,8 +407,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),
@@ -898,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_,
@@ -1036,11 +1036,11 @@ SiteInstanceImpl* WebContentsImpl::GetPendingSiteInstance() const {
 }
 
 bool WebContentsImpl::IsLoading() const {
-  return is_loading_;
+  return frame_tree_.IsLoading() && !paused_throbber_for_interstitial_;
 }
 
 bool WebContentsImpl::IsLoadingToDifferentDocument() const {
-  return is_loading_ && is_load_to_different_document_;
+  return IsLoading() && is_load_to_different_document_;
 }
 
 bool WebContentsImpl::IsWaitingForResponse() const {
@@ -2299,6 +2299,21 @@ void WebContentsImpl::AttachInterstitialPage(
 
   FOR_EACH_OBSERVER(WebContentsObserver, observers_,
                     DidAttachInterstitialPage());
+
+  // Stop the throbber if needed while the interstitial page is shown.
+  if (IsLoading()) {

[nasko, 2016/01/16 00:13:51]

No need for {} in one line if statements.

[clamy, 2016/01/19 13:31:18]

Done.

+    LoadingStateChanged(false, true, nullptr);
+  }
+  paused_throbber_for_interstitial_ = true;
+}
+
+void WebContentsImpl::DidProceedOnInterstitial() {
+  // Restart the throbber now that the interstitial page is going away.
+  if (paused_throbber_for_interstitial_) {
+    paused_throbber_for_interstitial_ = false;
+    if (frame_tree_.IsLoading())
+      LoadingStateChanged(true, true, NULL);

[nasko, 2016/01/16 00:13:51]

nullptr in all new code.

[clamy, 2016/01/19 13:31:18]

Done.

+  }
 }
 
 void WebContentsImpl::DetachInterstitialPage() {
@@ -2306,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 (paused_throbber_for_interstitial_) {
+    paused_throbber_for_interstitial_ = false;
+    if (frame_tree_.IsLoading())
+      LoadingStateChanged(true, true, NULL);

[nasko, 2016/01/16 00:13:51]

nullptr in all new code.

[clamy, 2016/01/19 13:31:18]

Done.

+  }
 }
 
 void WebContentsImpl::SetHistoryOffsetAndLength(int history_offset,
@@ -3580,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 (paused_throbber_for_interstitial_)
     return;
 
   if (!is_loading) {
@@ -3596,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;
 
@@ -3949,16 +3970,9 @@ void WebContentsImpl::RenderViewTerminated(RenderViewHost* rvh,
   if (delegate_)
     delegate_->HideValidationMessage(this);
 
-  SetIsLoading(false, true, nullptr);
   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()));
@@ -4051,7 +4065,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.
@@ -4085,7 +4099,7 @@ void WebContentsImpl::DidStopLoading() {
         controller_.GetCurrentEntryIndex()));
   }
 
-  SetIsLoading(false, true, details.get());
+  LoadingStateChanged(false, true, details.get());
 }
 
 void WebContentsImpl::DidChangeLoadProgress() {
@@ -4622,10 +4636,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();

[nasko, 2016/01/16 00:13:51]

I wonder if this is the right place to dispatch this from. The IPC that
BeforeUnload has been completed is handled by RFH itself. Can't we just reset
the state there instead of having to come all the way out to WebContents and go
down back to the RFH?

[clamy, 2016/01/19 13:31:18]

As explained in the other comments about BeforeUnload canceled, there are test
that check explicitly for that behavior. Do we want to modify it? (it would be
user-visible)

     controller_.DiscardNonCommittedEntries();
 
     FOR_EACH_OBSERVER(WebContentsObserver, observers_,