Reland #2 Remove the is_loading_ field from WebContentsImpl

content/browser/frame_host/frame_tree_node.h

Index: content/browser/frame_host/frame_tree_node.h
diff --git a/content/browser/frame_host/frame_tree_node.h b/content/browser/frame_host/frame_tree_node.h
index eac49777a663150917ab2289073e2dd3f8dc4d6b..4ea612350f35ebecf80f0d3ec8736c5212fa620a 100644
--- a/content/browser/frame_host/frame_tree_node.h
+++ b/content/browser/frame_host/frame_tree_node.h
@@ -235,7 +235,11 @@ class CONTENT_EXPORT FrameTreeNode {
   // A RenderFrameHost in this node started loading.
   // |to_different_document| will be true unless the load is a fragment
   // navigation, or triggered by history.pushState/replaceState.
-  void DidStartLoading(bool to_different_document);
+  // |was_previously_loading| is false if the FrameTree was not loading before.
+  // The caller is required to provide this boolean as the delegate should only
+  // be called if the FrameTree went from non-loading to loading state.

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

nit: s/called/notified/

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

Done.

+  // However, when it is called, the FrameTree should be in a loading state.

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

I don't understand this part.  The comment could be rephrased as
"FrameTree::IsLoading does not provide this distinction," but I'm not sure why
that's true.  We relied on it before and I don't see which part has changed.

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

Before we would call it before setting is_loading to true in the RFH, which
means that in WebContentsImpl::DidStartLoading FrameTree::IsLoading would return
false. This is a problem if we rely on FrameTree::IsLoading in
WebContentsImpl::IsLoading (in particular, Chrome on Android believes we never
start navigations in that case).

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

Acknowledged.

+  void DidStartLoading(bool to_different_document, bool was_previously_loading);
 
   // A RenderFrameHost in this node stopped loading.
   void DidStopLoading();
@@ -257,6 +261,10 @@ class CONTENT_EXPORT FrameTreeNode {
   // time and notifies observers.
   void DidFocus();
 
+  // Called when the user closed the modal dialogue for BeforeUnload and
+  // cancelled the navigation. This should stop the loads.

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

The comment is unclear: which loads?  (There's just one, right?)

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

What if there are two ongoing ones: one in the pending RFH and one in the
current RFH?

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

Actually, this turns out to be an interesting question.  :)

I was going to say that the pending RFH isn't created until after we've finished
(and approved) the beforeunload dialog, so there should only be a current RFH at
the time.

But then I checked what happens if you have a slow pending RFH (after approving
a beforeunload dialog) and then do another same-site navigation.  You get a
second beforeunload dialog, which you can cancel, and it does stop the spinner. 
However, it doesn't actually cancel the pending RFH at any point, so that ends
up committing later!

I suppose that's a separate bug (unless you have thoughts on how it would be
fixed).  For now, let's just clarify which loads the comment is referring to.

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

Actually the RFH is created before we approve the beforeunload dialog, but it's
not navigating at that time (navigations are suspended). Note that it also
creates other issues such as crbug.com/581024, where the suspended pending RFh
is not canceled when the WebContents is told to stop (and eventually commits).

Rewrote the comment, it should be clearer. Also added a TODO for the case you
mentioned.

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

Ah, excellent point.  And yes, we should clear the suspended nav params on stop
for 581024.
Thanks.  Just curious, where's the TODO for the case above?  I see ones for
subframes and a cross-process version of DidStopLoadingDetailsWithPending, but
not beforeunload deny with a separate pending RFH.  (I can file a bug if that's
easier.)

+  void BeforeUnloadCanceled();
+
  private:
   class OpenerDestroyedObserver;