Reland #2 Remove the is_loading_ field from WebContentsImpl

content/renderer/history_controller.h

 // Copyright 2014 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.
 
 /*
  * Copyright (C) 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.
  * Copyright (C) 2008, 2009 Torch Mobile Inc. All rights reserved.
  *     (http://www.torchmobile.com/)
  *
  * Redistribution and use in source and binary forms, with or without
@@ skipping 99 matching lines @@
 //
 class CONTENT_EXPORT HistoryController {
  public:
   explicit HistoryController(RenderViewImpl* render_view);
   ~HistoryController();
 
   void set_provisional_entry(scoped_ptr<HistoryEntry> entry) {
     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);
 
   void UpdateForCommit(RenderFrameImpl* frame,
                        const blink::WebHistoryItem& item,
                        blink::WebHistoryCommitType commit_type,
                        bool navigation_within_page);
 
   HistoryEntry* GetCurrentEntry();
@@ skipping 26 matching lines @@
   // passed into frames created after the commit that resulted from the
   // navigation in GetItemForNewChildFrame().
   scoped_ptr<NavigationParams> navigation_params_;
 
   DISALLOW_COPY_AND_ASSIGN(HistoryController);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_RENDERER_HISTORY_CONTROLLER_H_