Remove the is_loading_ field from WebContentsImpl

content/browser/web_contents/web_contents_impl_browsertest.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/macros.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "build/build_config.h"
 #include "content/browser/frame_host/navigation_entry_impl.h"
 #include "content/browser/renderer_host/render_widget_host_impl.h"
@@ skipping 90 matching lines @@
         shell_(shell),
         url_(url),
         done_(false) {
   }
 
   // WebContentsObserver:
   void NavigationEntryCommitted(
       const LoadCommittedDetails& load_details) override {
     if (!done_) {
       done_ = true;
+      shell_->LoadURL(url_);
+
+      // There should be a pending entry.
+      CHECK(shell_->web_contents()->GetController().GetPendingEntry());
+
+      // Now that there is a pending entry, stop the load.
       shell_->Stop();
-      shell_->LoadURL(url_);
     }
   }
 
   Shell* shell_;
   GURL url_;
   bool done_;
 };
 
 class RenderViewSizeDelegate : public WebContentsDelegate {
  public:
@@ skipping 59 matching lines @@
   int loadingStateChangedCount() const { return loadingStateChangedCount_; }
   int loadingStateToDifferentDocumentCount() const {
     return loadingStateToDifferentDocumentCount_;
   }
 
  private:
   int loadingStateChangedCount_;
   int loadingStateToDifferentDocumentCount_;
 };
 
-// See: http://crbug.com/298193
-#if defined(OS_WIN) || defined(OS_LINUX)
-#define MAYBE_DidStopLoadingDetails DISABLED_DidStopLoadingDetails
-#else
-#define MAYBE_DidStopLoadingDetails DidStopLoadingDetails
-#endif
-
 // Test that DidStopLoading includes the correct URL in the details.
-IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest,
-                       MAYBE_DidStopLoadingDetails) {
+IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest, DidStopLoadingDetails) {

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

It appears that the patch fixed the flakyness on that one (I got 100 runs to
succeed locally).

   ASSERT_TRUE(embedded_test_server()->Start());
 
   LoadStopNotificationObserver load_observer(
       &shell()->web_contents()->GetController());
   NavigateToURL(shell(), embedded_test_server()->GetURL("/title1.html"));
   load_observer.Wait();
 
   EXPECT_EQ("/title1.html", load_observer.url_.path());
   EXPECT_EQ(0, load_observer.session_index_);
   EXPECT_EQ(&shell()->web_contents()->GetController(),
             load_observer.controller_);
 }
 
-// See: http://crbug.com/298193
-#if defined(OS_WIN) || defined(OS_LINUX)
-#define MAYBE_DidStopLoadingDetailsWithPending \
-  DISABLED_DidStopLoadingDetailsWithPending
-#else
-#define MAYBE_DidStopLoadingDetailsWithPending DidStopLoadingDetailsWithPending
-#endif
-
 // Test that DidStopLoading includes the correct URL in the details when a
 // pending entry is present.
 IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest,
-                       MAYBE_DidStopLoadingDetailsWithPending) {
+                       DidStopLoadingDetailsWithPending) {

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

Same here, but I had to rewrite part of the test to get it to work (otherwise it
was failing consistently). The url were changed, because the navigation has to
be same-site. Otherwise, we can't properly stop it due to crbug.com/581024, and
the pending entry gets committed, leading to the failure of the test.

[nasko, 2016/01/26 01:34:27]

Should there be a TODO to cover this case or do you plan to write a different
test case when you fix 581024?

[clamy, 2016/01/28 13:52:21]

I added a TODO not to forget about it.

   ASSERT_TRUE(embedded_test_server()->Start());
-  GURL url("data:text/html,<div>test</div>");
+  GURL url1 = embedded_test_server()->GetURL("/title1.html");
+  GURL url2 = embedded_test_server()->GetURL("/title2.html");
 
   // Listen for the first load to stop.
   LoadStopNotificationObserver load_observer(
       &shell()->web_contents()->GetController());
   // Start a new pending navigation as soon as the first load commits.
   // We will hear a DidStopLoading from the first load as the new load
   // is started.
   NavigateOnCommitObserver commit_observer(
-      shell(), embedded_test_server()->GetURL("/title2.html"));
-  NavigateToURL(shell(), url);
+      shell(), url2);
+  NavigateToURL(shell(), url1);
   load_observer.Wait();
 
-  EXPECT_EQ(url, load_observer.url_);
+  EXPECT_EQ(url1, load_observer.url_);
   EXPECT_EQ(0, load_observer.session_index_);
   EXPECT_EQ(&shell()->web_contents()->GetController(),
             load_observer.controller_);
 }
 // Test that a renderer-initiated navigation to an invalid URL does not leave
 // around a pending entry that could be used in a URL spoof.  We test this in
 // a browser test because our unit test framework incorrectly calls
 // DidStartProvisionalLoadForFrame for in-page navigations.
 // See http://crbug.com/280512.
 IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest,
@@ skipping 560 matching lines @@
     Shell* new_shell = new_shell_observer.GetShell();
     WaitForLoadStop(new_shell->web_contents());
 
     EXPECT_EQ("foo",
               static_cast<WebContentsImpl*>(new_shell->web_contents())
                   ->GetFrameTree()->root()->frame_name());
   }
 }
 
 }  // namespace content