Change meaning of "IsLoadingToDifferentDocument" in WebContents to only

apply to loads in the main frame.

The distinction between "IsLoading" and "IsLoadingToDifferentDocument"
appears to only be relevant in UI contexts (e.g. TabRendererData, which
controls whether to show the throbber in the tabstrip on desktop
Chrome). I think the distinction only makes sense in the context of the
main frame and a load in a subframe should always be lumped in with
non-top-level document loads. For example, on Android it's sometimes
used to conditionally call ToolbarLayout.onNavigatedToDifferentPage(),
which from other callsites definitely appears to only care about main
frame navigations.

Making this change means the throbber won't flicker when content is
loaded in an iframe right after the main frame finishes loading, which
happens a lot (see bug).

With this change, we can probably remove the timer added in
89603210 which attempts to smooth out favicon/throbber
flickering.

Bug: 734104
Change-Id: Ie14b1c54956479021fd9aa6c4c8aa690be52f193
Reviewed-on: https://chromium-review.googlesource.com/569060Reviewed-by: Pavel Feldman <pfeldman@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486854}

chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc

@@ -56,8 +56,7 @@ namespace {
 
 TabRendererData::NetworkState TabContentsNetworkState(
     WebContents* contents) {
-  if (!contents)
-    return TabRendererData::NETWORK_STATE_NONE;
+  DCHECK(contents);
 
   if (!contents->IsLoadingToDifferentDocument()) {
     content::NavigationEntry* entry =
@@ -500,7 +499,6 @@ void BrowserTabStripController::SetTabRendererDataFromModel(
   data->network_state = TabContentsNetworkState(contents);
   data->title = contents->GetTitle();
   data->url = contents->GetURL();
-  data->loading = contents->IsLoading();
   data->crashed_status = contents->GetCrashedStatus();
   data->incognito = contents->GetBrowserContext()->IsOffTheRecord();
   data->pinned = model_->IsTabPinned(model_index);

chrome/browser/ui/views/tabs/tab.cc

@@ -540,9 +540,9 @@ void Tab::SetData(const TabRendererData& data) {
 
   base::string16 title = data_.title;
   if (title.empty()) {
-    title = data_.loading ?
-        l10n_util::GetStringUTF16(IDS_TAB_LOADING_TITLE) :
-        CoreTabHelper::GetDefaultTitle();
+    title = ShouldShowThrobber(data_.network_state)
+                ? l10n_util::GetStringUTF16(IDS_TAB_LOADING_TITLE)
+                : CoreTabHelper::GetDefaultTitle();
   } else {
     Browser::FormatTitleForDisplay(&title);
   }

chrome/browser/ui/views/tabs/tab_renderer_data.cc

@@ -9,7 +9,6 @@
 
 TabRendererData::TabRendererData()
     : network_state(NETWORK_STATE_NONE),
-      loading(false),
       crashed_status(base::TERMINATION_STATUS_STILL_RUNNING),
       incognito(false),
       show_icon(true),
@@ -39,7 +38,6 @@ bool TabRendererData::Equals(const TabRendererData& data) {
       network_state == data.network_state &&
       title == data.title &&
       url == data.url &&
-      loading == data.loading &&
       crashed_status == data.crashed_status &&
       incognito == data.incognito &&
       show_icon == data.show_icon &&

chrome/browser/ui/views/tabs/tab_renderer_data.h

@@ -39,7 +39,6 @@ struct CHROME_VIEWS_EXPORT TabRendererData {
   NetworkState network_state;
   base::string16 title;
   GURL url;
-  bool loading;
   base::TerminationStatus crashed_status;
   bool incognito;
   bool show_icon;

content/browser/web_contents/web_contents_impl.cc

@@ -4812,7 +4812,8 @@ void WebContentsImpl::RequestMove(const gfx::Rect& new_bounds) {
 
 void WebContentsImpl::DidStartLoading(FrameTreeNode* frame_tree_node,
                                       bool to_different_document) {
-  LoadingStateChanged(to_different_document, false, nullptr);
+  LoadingStateChanged(frame_tree_node->IsMainFrame() && to_different_document,
+                      false, nullptr);
 
   // Notify accessibility that the user is navigating away from the
   // current document.

content/browser/web_contents/web_contents_impl.h

@@ -1361,7 +1361,7 @@ class CONTENT_EXPORT WebContentsImpl
   // Data for loading state ----------------------------------------------------
 
   // Indicates whether the current load is to a different document. Only valid
-  // if is_loading_ is true.
+  // if |is_loading_| is true and only tracks loads in the main frame.
   bool is_load_to_different_document_;
 
   // Indicates if the tab is considered crashed.

content/browser/web_contents/web_contents_impl_unittest.cc

@@ -3245,6 +3245,58 @@ TEST_F(WebContentsImplTestWithSiteIsolation, StartStopEventsBalance) {
   EXPECT_FALSE(observer.is_loading());
 }
 
+// Tests that WebContentsImpl::IsLoadingToDifferentDocument only reports main
+// frame loads. Browser-initiated navigation of subframes is only possible in
+// --site-per-process mode within unit tests.
+TEST_F(WebContentsImplTestWithSiteIsolation, IsLoadingToDifferentDocument) {
+  const GURL main_url("http://www.chromium.org");
+  TestRenderFrameHost* orig_rfh = main_test_rfh();
+
+  // Navigate the main RenderFrame, simulate the DidStartLoading, and commit.
+  // The frame should still be loading.
+  controller().LoadURL(main_url, Referrer(), ui::PAGE_TRANSITION_TYPED,
+                       std::string());
+  int entry_id = controller().GetPendingEntry()->GetUniqueID();
+
+  // PlzNavigate: the RenderFrameHost does not expect to receive
+  // DidStartLoading IPCs for navigations to different documents.
+  if (!IsBrowserSideNavigationEnabled()) {
+    orig_rfh->OnMessageReceived(
+        FrameHostMsg_DidStartLoading(orig_rfh->GetRoutingID(), false));
+  }
+  main_test_rfh()->PrepareForCommit();
+  contents()->TestDidNavigate(orig_rfh, entry_id, true, main_url,
+                              ui::PAGE_TRANSITION_TYPED);
+  EXPECT_FALSE(contents()->CrossProcessNavigationPending());
+  EXPECT_EQ(orig_rfh, main_test_rfh());
+  EXPECT_TRUE(contents()->IsLoading());
+  EXPECT_TRUE(contents()->IsLoadingToDifferentDocument());
+
+  // Send the DidStopLoading for the main frame and ensure it isn't loading
+  // anymore.
+  orig_rfh->OnMessageReceived(
+      FrameHostMsg_DidStopLoading(orig_rfh->GetRoutingID()));
+  EXPECT_FALSE(contents()->IsLoading());
+  EXPECT_FALSE(contents()->IsLoadingToDifferentDocument());
+
+  // Create a child frame to navigate.
+  TestRenderFrameHost* subframe = orig_rfh->AppendChild("subframe");
+
+  // Navigate the child frame to about:blank, make sure the web contents is
+  // marked as "loading" but not "loading to different document".
+  if (!IsBrowserSideNavigationEnabled()) {
+    subframe->OnMessageReceived(
+        FrameHostMsg_DidStartLoading(subframe->GetRoutingID(), true));
+  }
+  subframe->SendNavigateWithTransition(0, false, GURL("about:blank"),
+                                       ui::PAGE_TRANSITION_AUTO_SUBFRAME);
+  EXPECT_TRUE(contents()->IsLoading());
+  EXPECT_FALSE(contents()->IsLoadingToDifferentDocument());
+  subframe->OnMessageReceived(
+      FrameHostMsg_DidStopLoading(subframe->GetRoutingID()));
+  EXPECT_FALSE(contents()->IsLoading());
+}
+
 // Ensure that WebContentsImpl does not stop loading too early when there still
 // is a pending renderer. This can happen if a same-process non user-initiated
 // navigation completes while there is an ongoing cross-process navigation.

content/public/browser/web_contents.h

@@ -373,7 +373,8 @@ class WebContents : public PageNavigator,
 
   // Returns whether this WebContents is loading and and the load is to a
   // different top-level document (rather than being a navigation within the
-  // same document). This being true implies that IsLoading() is also true.
+  // same document) in the main frame. This being true implies that IsLoading()
+  // is also true.
   virtual bool IsLoadingToDifferentDocument() const = 0;
 
   // Returns whether this WebContents is waiting for a first-response for the