Reland #2 Remove the is_loading_ field from WebContentsImpl

content/renderer/history_controller.h

Index: content/renderer/history_controller.h
diff --git a/content/renderer/history_controller.h b/content/renderer/history_controller.h
index c8b00f2d33fda90be84e1bf6b7c1ebde3762ab81..451501c622413b51e69e74a2e80c443f61273cc7 100644
--- a/content/renderer/history_controller.h
+++ b/content/renderer/history_controller.h
@@ -117,7 +117,9 @@ class CONTENT_EXPORT HistoryController {
     provisional_entry_ = std::move(entry);
   }
 
-  void GoToEntry(blink::WebLocalFrame* main_frame,
+  // Return true if the main frame ended up loading a request as part of the
+  // history navigation.
+  bool GoToEntry(blink::WebLocalFrame* main_frame,

[Charlie Reis, 2016/02/17 05:15:28]

Hmm, I'm not thrilled with this change, partly because this code is being
replaced by NavigationControllerImpl::NavigateToPendingEntryInternal (once
UseSubframeNavigationEntries becomes the default).  That code has a different
meaning for its return value, and it's not clear to me why this change is needed
in the old path and not the new one.

[clamy, 2016/02/17 14:42:06]

With the change, we set is_loading_ to true at the beginning of the browser
initiated navigation. So the RFH expects the RenderFrame to be loading
something. In the case of a history navigation in subframes, the request is sent
by the main frame. However, the main frame itself will not be loading anything.
So the main RFH needs to be told back that it is not actually loading. With
subframe navigation entries, we're sending the navigation to the right frame
from the start, so this is not needed.

[Charlie Reis, 2016/02/17 23:34:26]

I see, so it's just a consequence of the old design, where the main frame might
not actually load anything.

Could we avoid changing HistoryController by querying WebLocalFrame::isLoading()
in RenderFrameImpl::NavigateInternal, after the GoToEntry call?  If that has the
correct state, we wouldn't need to give GoToEntry a return value.

[clamy, 2016/02/18 17:20:27]

That's what I did initially and that caused the revert of the patch. For some
unknown reason, even when a load is scheduled in the frame sometimes blink does
not set the frame as loading, and we end up sending a DidStopLoading followed by
a DidStartLoading in the middle of the navigation, which confuses some of the
SessionHistoryTests.

Because this is very flaky (happens ~ once every 70 tests locally), I have had
issues trying to pinpoint why blink is doing that. Considering that this code
should be going away anyway, I went with this solution.

[Charlie Reis, 2016/02/19 05:24:54]

Sigh.  I would say that we should understand why this is happening in Blink, but
I agree that the code is about to go away anyway.  Perhaps it isn't worth the
time, if you're sure that this resolves the flakiness.

                  scoped_ptr<HistoryEntry> entry,
                  scoped_ptr<NavigationParams> navigation_params,
                  blink::WebURLRequest::CachePolicy cache_policy);