Reland #2 Remove the is_loading_ field from WebContentsImpl

Reland #2 Remove the is_loading_ field from WebContentsImpl

The original CL was reverted because it introduced flakyness on some
SessionHistoryTest on the Cast Linux bot.

Original commit message:
This CL removes the is_loading_ field from WebContents, in favor of only
tracking the loading state through the FrameTree. Currently the loading state
is tracked in both, leading to more edge cases in the code. These edge cases
are removed by this CL, which allows the current architecture and PlzNavigate
to have the same behavior. It also removes some flakyness with
browser-initiated navigations and reenables 2 content_browsertests.

BUG=571887, 298193
TBR=nasko@chromium.org,rockot@chromium.org,nick@chromium.org,carlosk@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/44e84cef5a598bbee63ba83ad1cd5534e7295abd
Cr-Commit-Position: refs/heads/master@{#376732}

[clamy, 2016-02-15 12:42:46]

Description was changed from

==========
Reland #2 Remove the is_loading_ field from WebContentsImpl

The original CL was reverted because it introduced flakyness on some
SessionHistoryTest on the Cast Linux bot.

Original commit message:
This CL removes the is_loading_ field from WebContents, in favor of only
tracking the loading state through the FrameTree. Currently the loading state
is tracked in both, leading to more complexity in the code.

BUG=571887, 298193
==========

to

==========
Reland #2 Remove the is_loading_ field from WebContentsImpl

The original CL was reverted because it introduced flakyness on some
SessionHistoryTest on the Cast Linux bot.

Original commit message:
This CL removes the is_loading_ field from WebContents, in favor of only
tracking the loading state through the FrameTree. Currently the loading state
is tracked in both, leading to more complexity in the code.

BUG=571887, 298193
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[clamy, 2016-02-16 15:56:22]

Description was changed from

==========
Reland #2 Remove the is_loading_ field from WebContentsImpl

The original CL was reverted because it introduced flakyness on some
SessionHistoryTest on the Cast Linux bot.

Original commit message:
This CL removes the is_loading_ field from WebContents, in favor of only
tracking the loading state through the FrameTree. Currently the loading state
is tracked in both, leading to more complexity in the code.

BUG=571887, 298193
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Reland #2 Remove the is_loading_ field from WebContentsImpl

The original CL was reverted because it introduced flakyness on some
SessionHistoryTest on the Cast Linux bot.

Original commit message:
This CL removes the is_loading_ field from WebContents, in favor of only
tracking the loading state through the FrameTree. Currently the loading state
is tracked in both, leading to more complexity in the code.

BUG=571887, 298193
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[clamy, 2016-02-16 15:56:22]

clamy@chromium.org changed reviewers:
+ creis@chromium.org

[clamy, 2016-02-16 16:01:03]

@creis: PTAL. Since Nasko is out, I'm sending this to you.

The goal of the patch is to remove the is_loading field from WebContents, in
favor of just using the state of the FrameTree.

The patch was reverted because of the following issue:
When we do a browser-initiated navigation, we mark the RFH as loading and send a
DidStartLoading IPC. Then if the renderer decides not to actually load the
navigation/not load it in this frame, it send a DidStopLoading (if it's not
currently loading anything). However, in the case of history navigations, it's
possible that a navigation is started in the frame but by the time we get to the
check of whether the frame is loading or not, it is not. Therefore, we send an
extraneous DidStopLoading message. To avoid that, I'm not sending it if a load
was for the main frame was found in the HistoryItem.

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

Sorry, but I'm having trouble understanding the improvement here.  This seems
like a substantial amount of extra complexity, rather than a simplification.

Can you help explain what the benefits are?  I'm wondering if it started out
simpler and might have grown to where we should decide if it's still worthwhile.

https://codereview.chromium.org/1693353002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_resource_request_policy_apitest.cc
(right):

https://codereview.chromium.org/1693353002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_resource_request_policy_apitest.cc:230:
browser(), accessible_newtab_override, 1);
What made this change?

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:471: for (const FrameTreeNode* node :
ConstNodes()) {
This seems like a lot of extra code just to add the const label to IsLoading. 
Does it get us any specific benefits here?  On the surface it doesn't seem
worthwhile.

It's also potentially unrelated to the rest of the CL, and could land separately
if we decide we want to keep it.

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:412: if (!IsMainFrame())
Add TODO to support beforeunload in subframes.

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:240: // be called if the FrameTree
went from non-loading to loading state.
nit: s/called/notified/

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:241: // However, when it is called,
the FrameTree should be in a loading state.
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.

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:265: // cancelled the navigation.
This should stop the loads.
The comment is unclear: which loads?  (There's just one, right?)

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/interstitial_page_impl.cc (left):

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
content/browser/frame_host/interstitial_page_impl.cc:522: // Notify the tab we
are not loading so the throbber is stopped. It also
What stops the throbber now in this case?

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:349: // without emitting a
spurrious DidStartLoading notification at the
nit: spurious

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:385: // Also sets the
RenderFrameHost back to a loading state again.
nit: set

Where does that happen?  I don't see any changes to ResumeDeferredNavigation.

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (left):

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1704: // Any main frame
load to a new document should reset the load since it will
I'm concerned about removing these old corner cases, because they were really
subtle and had bad implications to get wrong (e.g., spinner never stopped).  I
don't fully understand why the new code makes these checks unnecessary, which
makes me worry about possible regressions.

Is there a good way to reason about why this change is safe?

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:696: DCHECK(site_instance
== site_instance_.get());
Does DCHECK_EQ work here?

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:698: // The render process
is gone, this frame can no longer be loading.
nit: renderer process
nit: so this

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1319: ResetLoadingState();
I think this line is unreachable now.  We don't keep RFH's in STATE_SWAPPED_OUT
anymore, and Nasko is preparing to remove the old code once we branch for M51.

Is this ResetLoadingState call needed otherwise, or just in the old
STATE_SWAPPED_OUT case?

https://codereview.chromium.org/1693353002/diff/20001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/1693353002/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:1118: bool
paused_throbber_for_interstitial_;
Again, I'm not sure if this is an improvement.  is_loading_ seems like a more
straightforward concept than this one.

https://codereview.chromium.org/1693353002/diff/20001/content/renderer/histor...
File content/renderer/history_controller.h (right):

https://codereview.chromium.org/1693353002/diff/20001/content/renderer/histor...
content/renderer/history_controller.h:122: bool GoToEntry(blink::WebLocalFrame*
main_frame,
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]

The goal of the patch is to make WebContentsImpl::IsLoading depends uniquely on
FrameTree::IsLoading (and whether an interstitial is present or not). This
allows to ensure that as long as FrameTree::IsLoading is properly set, then the
behavior of the browser is the same. Without that, I can't have the same
behavior in PlzNavigate as in the current architecture, because I simply can't
match some of the edge cases of what the current architecture is doing. In
particular, this is preventing me from landing
https://codereview.chromium.org/1608283002/, which has PlzNavigate use
WebNavigationPolicy::HandledByClient. This change is needed because without it
the load event may be fired to early with PlzNavigate. This would also allow me
to land https://codereview.chromium.org/1446253002/, which fixes an issue with
forms in PlzNavigate.

Apart from that, it also fixes some flakyness in browser-initiated navigations,
and allows to re-enable 3 loading related tests.

I reckon that the patch is a bit big (it grew during review). It would be
possible to cut it in several pieces, making it easier to reason about it.

https://codereview.chromium.org/1693353002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_resource_request_policy_apitest.cc
(right):

https://codereview.chromium.org/1693353002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_resource_request_policy_apitest.cc:230:
browser(), accessible_newtab_override, 1);
On 2016/02/17 05:15:28, Charlie Reis (catching up) wrote:
> What made this change?

If I remember correctly, there was a race involving a browser-initiated
navigation to a pending renderer starting and the current one stopping just
after that then another browser-initiated navigation starting, which resulted in
DidStopLoading and DidStartLoading being fired in the middle of that sequence.
Therefore they had to wait for 2 navigations. Since this case was fixed by the
patch, they only have to wait for one.

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:471: for (const FrameTreeNode* node :
ConstNodes()) {
On 2016/02/17 05:15:28, Charlie Reis (catching up) wrote:
> This seems like a lot of extra code just to add the const label to IsLoading. 
> Does it get us any specific benefits here?  On the surface it doesn't seem
> worthwhile.
> 
> It's also potentially unrelated to the rest of the CL, and could land
separately
> if we decide we want to keep it.

We cannot use it in WebContentsImpl::IsLoading otherwise, since that function is
const. I would not be making the change otherwise.

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:241: // However, when it is called,
the FrameTree should be in a loading state.
On 2016/02/17 05:15:28, Charlie Reis (catching up) wrote:
> 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.

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).

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:265: // cancelled the navigation.
This should stop the loads.
On 2016/02/17 05:15:28, Charlie Reis (catching up) wrote:
> The comment is unclear: which loads?  (There's just one, right?)

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

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/interstitial_page_impl.cc (left):

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
content/browser/frame_host/interstitial_page_impl.cc:522: // Notify the tab we
are not loading so the throbber is stopped. It also
On 2016/02/17 05:15:28, Charlie Reis (catching up) wrote:
> What stops the throbber now in this case?

The function that shows the interstitial in WebContents stops it.

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:385: // Also sets the
RenderFrameHost back to a loading state again.
On 2016/02/17 05:15:28, Charlie Reis (catching up) wrote:
> nit: set
> 
> Where does that happen?  I don't see any changes to ResumeDeferredNavigation.

This remains from a previous version of the patch, my mistake. Will delete it.

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (left):

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1704: // Any main frame
load to a new document should reset the load since it will
On 2016/02/17 05:15:28, Charlie Reis (catching up) wrote:
> I'm concerned about removing these old corner cases, because they were really
> subtle and had bad implications to get wrong (e.g., spinner never stopped).  I
> don't fully understand why the new code makes these checks unnecessary, which
> makes me worry about possible regressions.
> 
> Is there a good way to reason about why this change is safe?

A lot of the corner cases were caused by the old architecture where is_loading_
would sometimes be out of phase with whether the RFH was actually loading or
not. This was partly alleviated by the fact that we had a boolean in WC
is_loading_, which would also get out-of-phase with what is happening at the
FrameTree level.

Here I'm trying to ensure that is_loading_ matches the loading state at all
time. So there should be less edge cases in the long run. I also added new
checks to the WebContentsObserverSanityChecker for loading state. And it seems
that all our tests are fine with the change. There may be some regressions that
are not tested, however, the change also allows to re-enable tests that were
disabled due to flakyness on the current architecture.

https://codereview.chromium.org/1693353002/diff/20001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/1693353002/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:1118: bool
paused_throbber_for_interstitial_;
On 2016/02/17 05:15:28, Charlie Reis (catching up) wrote:
> Again, I'm not sure if this is an improvement.  is_loading_ seems like a more
> straightforward concept than this one.

Now that I think about it, it should be possible to move this to the
InterstitialPage itself. Then whether the WebContents is loading would be:
FrameTree::IsLoading && !InterstitialPage::PauseThrobber.

https://codereview.chromium.org/1693353002/diff/20001/content/renderer/histor...
File content/renderer/history_controller.h (right):

https://codereview.chromium.org/1693353002/diff/20001/content/renderer/histor...
content/renderer/history_controller.h:122: bool GoToEntry(blink::WebLocalFrame*
main_frame,
On 2016/02/17 05:15:28, Charlie Reis (catching up) wrote:
> 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.

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]

On 2016/02/17 14:42:06, clamy wrote:
> The goal of the patch is to make WebContentsImpl::IsLoading depends uniquely
on
> FrameTree::IsLoading (and whether an interstitial is present or not). This
> allows to ensure that as long as FrameTree::IsLoading is properly set, then
the
> behavior of the browser is the same. Without that, I can't have the same
> behavior in PlzNavigate as in the current architecture, because I simply can't
> match some of the edge cases of what the current architecture is doing. In
> particular, this is preventing me from landing
> https://codereview.chromium.org/1608283002/, which has PlzNavigate use
> WebNavigationPolicy::HandledByClient. This change is needed because without it
> the load event may be fired to early with PlzNavigate. This would also allow
me
> to land https://codereview.chromium.org/1446253002/, which fixes an issue with
> forms in PlzNavigate.
> 
> Apart from that, it also fixes some flakyness in browser-initiated
navigations,
> and allows to re-enable 3 loading related tests.
> 
> I reckon that the patch is a bit big (it grew during review). It would be
> possible to cut it in several pieces, making it easier to reason about it.

Thanks, that clarifies things a lot.  I'd suggest updating the CL description to
indicate that this will make it possible to preserve the existing loading
behavior in PlzNavigate, and that it will allow re-enabling some flaky tests. 
(I wasn't aware of those benefits before, so I was evaluating whether it was
reducing complexity or not.)

One question about the HistoryController change and a few followups below.

https://codereview.chromium.org/1693353002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_resource_request_policy_apitest.cc
(right):

https://codereview.chromium.org/1693353002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_resource_request_policy_apitest.cc:230:
browser(), accessible_newtab_override, 1);
On 2016/02/17 14:42:06, clamy wrote:
> On 2016/02/17 05:15:28, Charlie Reis (catching up) wrote:
> > What made this change?
> 
> If I remember correctly, there was a race involving a browser-initiated
> navigation to a pending renderer starting and the current one stopping just
> after that then another browser-initiated navigation starting, which resulted
in
> DidStopLoading and DidStartLoading being fired in the middle of that sequence.
> Therefore they had to wait for 2 navigations. Since this case was fixed by the
> patch, they only have to wait for one.

Acknowledged.

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:471: for (const FrameTreeNode* node :
ConstNodes()) {
On 2016/02/17 14:42:06, clamy wrote:
> On 2016/02/17 05:15:28, Charlie Reis (catching up) wrote:
> > This seems like a lot of extra code just to add the const label to
IsLoading. 
> > Does it get us any specific benefits here?  On the surface it doesn't seem
> > worthwhile.
> > 
> > It's also potentially unrelated to the rest of the CL, and could land
> separately
> > if we decide we want to keep it.
> 
> We cannot use it in WebContentsImpl::IsLoading otherwise, since that function
is
> const. I would not be making the change otherwise.

Ok, I see it's necessary for that.

(Sigh-- the interesting part is that it's technically illegal for
WebContents::IsLoading to be const in the first place, according to the Content
API ("don't add the const identifier to interfaces").  That was legacy, though,
and it doesn't seem worthwhile to remove the const at this point.  Let's keep
what you have.)

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:241: // However, when it is called,
the FrameTree should be in a loading state.
On 2016/02/17 14:42:06, clamy wrote:
> On 2016/02/17 05:15:28, Charlie Reis (catching up) wrote:
> > 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.
> 
> 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).

Acknowledged.

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:265: // cancelled the navigation.
This should stop the loads.
On 2016/02/17 14:42:06, clamy wrote:
> On 2016/02/17 05:15:28, Charlie Reis (catching up) wrote:
> > The comment is unclear: which loads?  (There's just one, right?)
> 
> What if there are two ongoing ones: one in the pending RFH and one in the
> current RFH?

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.

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/interstitial_page_impl.cc (left):

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
content/browser/frame_host/interstitial_page_impl.cc:522: // Notify the tab we
are not loading so the throbber is stopped. It also
On 2016/02/17 14:42:06, clamy wrote:
> On 2016/02/17 05:15:28, Charlie Reis (catching up) wrote:
> > What stops the throbber now in this case?
> 
> The function that shows the interstitial in WebContents stops it.

Acknowledged.

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (left):

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1704: // Any main frame
load to a new document should reset the load since it will
On 2016/02/17 14:42:06, clamy wrote:
> On 2016/02/17 05:15:28, Charlie Reis (catching up) wrote:
> > I'm concerned about removing these old corner cases, because they were
really
> > subtle and had bad implications to get wrong (e.g., spinner never stopped). 
I
> > don't fully understand why the new code makes these checks unnecessary,
which
> > makes me worry about possible regressions.
> > 
> > Is there a good way to reason about why this change is safe?
> 
> A lot of the corner cases were caused by the old architecture where
is_loading_
> would sometimes be out of phase with whether the RFH was actually loading or
> not. This was partly alleviated by the fact that we had a boolean in WC
> is_loading_, which would also get out-of-phase with what is happening at the
> FrameTree level.
> 
> Here I'm trying to ensure that is_loading_ matches the loading state at all
> time. So there should be less edge cases in the long run. I also added new
> checks to the WebContentsObserverSanityChecker for loading state. And it seems
> that all our tests are fine with the change. There may be some regressions
that
> are not tested, however, the change also allows to re-enable tests that were
> disabled due to flakyness on the current architecture.

Ok, that seems worthwhile.  Thanks for explaining.

https://codereview.chromium.org/1693353002/diff/20001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/1693353002/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:1118: bool
paused_throbber_for_interstitial_;
On 2016/02/17 14:42:06, clamy wrote:
> On 2016/02/17 05:15:28, Charlie Reis (catching up) wrote:
> > Again, I'm not sure if this is an improvement.  is_loading_ seems like a
more
> > straightforward concept than this one.
> 
> Now that I think about it, it should be possible to move this to the
> InterstitialPage itself. Then whether the WebContents is loading would be:
> FrameTree::IsLoading && !InterstitialPage::PauseThrobber.

Yes, that sounds a bit nicer.

https://codereview.chromium.org/1693353002/diff/20001/content/renderer/histor...
File content/renderer/history_controller.h (right):

https://codereview.chromium.org/1693353002/diff/20001/content/renderer/histor...
content/renderer/history_controller.h:122: bool GoToEntry(blink::WebLocalFrame*
main_frame,
On 2016/02/17 14:42:06, clamy wrote:
> On 2016/02/17 05:15:28, Charlie Reis (catching up) wrote:
> > 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.
> 
> 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.

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:16:22]

Patchset #3 (id:40001) has been deleted

[clamy, 2016-02-18 17:19:54]

Description was changed from

==========
Reland #2 Remove the is_loading_ field from WebContentsImpl

The original CL was reverted because it introduced flakyness on some
SessionHistoryTest on the Cast Linux bot.

Original commit message:
This CL removes the is_loading_ field from WebContents, in favor of only
tracking the loading state through the FrameTree. Currently the loading state
is tracked in both, leading to more complexity in the code.

BUG=571887, 298193
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Reland #2 Remove the is_loading_ field from WebContentsImpl

The original CL was reverted because it introduced flakyness on some
SessionHistoryTest on the Cast Linux bot.

Original commit message:
This CL removes the is_loading_ field from WebContents, in favor of only
tracking the loading state through the FrameTree. Currently the loading state
is tracked in both, leading to more edge cases in the code. These edge cases
are removed by this CL, which allows the current architecture and PlzNavigate
to have the same behavior. It also removes some flakyness with
browser-initiated navigations and reenables 2 content_browsertests.

BUG=571887, 298193
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

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

Thanks! Updated the CL description.

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:412: if (!IsMainFrame())
On 2016/02/17 05:15:28, Charlie Reis wrote:
> Add TODO to support beforeunload in subframes.

Done.

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:240: // be called if the FrameTree
went from non-loading to loading state.
On 2016/02/17 05:15:28, Charlie Reis wrote:
> nit: s/called/notified/

Done.

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:265: // cancelled the navigation.
This should stop the loads.
On 2016/02/17 23:34:26, Charlie Reis wrote:
> On 2016/02/17 14:42:06, clamy wrote:
> > On 2016/02/17 05:15:28, Charlie Reis (catching up) wrote:
> > > The comment is unclear: which loads?  (There's just one, right?)
> > 
> > What if there are two ongoing ones: one in the pending RFH and one in the
> > current RFH?
> 
> 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.

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.

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:349: // without emitting a
spurrious DidStartLoading notification at the
On 2016/02/17 05:15:28, Charlie Reis wrote:
> nit: spurious

Done.

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:696: DCHECK(site_instance
== site_instance_.get());
On 2016/02/17 05:15:28, Charlie Reis wrote:
> Does DCHECK_EQ work here?

Done.

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:698: // The render process
is gone, this frame can no longer be loading.
On 2016/02/17 05:15:28, Charlie Reis wrote:
> nit: renderer process
> nit: so this

Done.

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1319: ResetLoadingState();
On 2016/02/17 05:15:28, Charlie Reis wrote:
> I think this line is unreachable now.  We don't keep RFH's in
STATE_SWAPPED_OUT
> anymore, and Nasko is preparing to remove the old code once we branch for M51.
> 
> Is this ResetLoadingState call needed otherwise, or just in the old
> STATE_SWAPPED_OUT case?

It seems it's not necessary (we reset the loading state when swapping in the
pending RFH or when destroying the pending RFH). Removed it.

https://codereview.chromium.org/1693353002/diff/20001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/1693353002/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:1118: bool
paused_throbber_for_interstitial_;
On 2016/02/17 23:34:26, Charlie Reis wrote:
> On 2016/02/17 14:42:06, clamy wrote:
> > On 2016/02/17 05:15:28, Charlie Reis (catching up) wrote:
> > > Again, I'm not sure if this is an improvement.  is_loading_ seems like a
> more
> > > straightforward concept than this one.
> > 
> > Now that I think about it, it should be possible to move this to the
> > InterstitialPage itself. Then whether the WebContents is loading would be:
> > FrameTree::IsLoading && !InterstitialPage::PauseThrobber.
> 
> Yes, that sounds a bit nicer.

Done.

https://codereview.chromium.org/1693353002/diff/20001/content/renderer/histor...
File content/renderer/history_controller.h (right):

https://codereview.chromium.org/1693353002/diff/20001/content/renderer/histor...
content/renderer/history_controller.h:122: bool GoToEntry(blink::WebLocalFrame*
main_frame,
On 2016/02/17 23:34:26, Charlie Reis wrote:
> On 2016/02/17 14:42:06, clamy wrote:
> > On 2016/02/17 05:15:28, Charlie Reis (catching up) wrote:
> > > 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.
> > 
> > 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.
> 
> 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.

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:55]

Thanks for explaining.  LGTM with nits.

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/1693353002/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:265: // cancelled the navigation.
This should stop the loads.
On 2016/02/18 17:20:26, clamy wrote:
> On 2016/02/17 23:34:26, Charlie Reis wrote:
> > On 2016/02/17 14:42:06, clamy wrote:
> > > On 2016/02/17 05:15:28, Charlie Reis (catching up) wrote:
> > > > The comment is unclear: which loads?  (There's just one, right?)
> > > 
> > > What if there are two ongoing ones: one in the pending RFH and one in the
> > > current RFH?
> > 
> > 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.
> 
> 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).

Ah, excellent point.  And yes, we should clear the suspended nav params on stop
for 581024.


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

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.)

https://codereview.chromium.org/1693353002/diff/20001/content/renderer/histor...
File content/renderer/history_controller.h (right):

https://codereview.chromium.org/1693353002/diff/20001/content/renderer/histor...
content/renderer/history_controller.h:122: bool GoToEntry(blink::WebLocalFrame*
main_frame,
On 2016/02/18 17:20:27, clamy wrote:
> On 2016/02/17 23:34:26, Charlie Reis wrote:
> > On 2016/02/17 14:42:06, clamy wrote:
> > > On 2016/02/17 05:15:28, Charlie Reis (catching up) wrote:
> > > > 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.
> > > 
> > > 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.
> > 
> > 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.
> 
> 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.

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.

https://codereview.chromium.org/1693353002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/1693353002/diff/60001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:412: // TODO(clamy); Support
BeforeUnload in subframes.
nit: Colon rather than semicolon.

https://codereview.chromium.org/1693353002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/interstitial_page_impl.cc (right):

https://codereview.chromium.org/1693353002/diff/60001/content/browser/frame_h...
content/browser/frame_host/interstitial_page_impl.cc:164: this,
Curious, is this a git cl format thing?  In general, you don't need each
parameter on its own line for a call site.

https://codereview.chromium.org/1693353002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/interstitial_page_impl.h (right):

https://codereview.chromium.org/1693353002/diff/60001/content/browser/frame_h...
content/browser/frame_host/interstitial_page_impl.h:294: // user choses to
proceed.
nit: chooses

https://codereview.chromium.org/1693353002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1693353002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:440:
DCHECK(frame_tree_node_->IsLoading());
Is there a race where this could fail, where the UI thread sets IsLoading to
false just before the IO thread posts the task to OnCrossSiteResponse?  (Not
sure, just checking.)

https://codereview.chromium.org/1693353002/diff/60001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/1693353002/diff/60001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:1021: void
LoadingStateChanged(bool to_different_document,
Maybe this belongs in the "Tracking loading progress" section rather than the
"Misc non-view stuff" section?

https://codereview.chromium.org/1693353002/diff/60001/content/test/test_web_c...
File content/test/test_web_contents.cc (right):

https://codereview.chromium.org/1693353002/diff/60001/content/test/test_web_c...
content/test/test_web_contents.cc:192: if (value)
Style nit: This needs braces (since the else block has braces).

https://codereview.chromium.org/1693353002/diff/60001/content/test/test_web_c...
content/test/test_web_contents.cc:201: RenderFrameHostImpl* pending_frame_host =
nit: Why not declare this inside the else branch below?

[clamy, 2016-02-19 12:02:05]

Thanks!

https://codereview.chromium.org/1693353002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/1693353002/diff/60001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:412: // TODO(clamy); Support
BeforeUnload in subframes.
On 2016/02/19 05:24:54, Charlie Reis (slow til Mar 7) wrote:
> nit: Colon rather than semicolon.

Done.

https://codereview.chromium.org/1693353002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/interstitial_page_impl.cc (right):

https://codereview.chromium.org/1693353002/diff/60001/content/browser/frame_h...
content/browser/frame_host/interstitial_page_impl.cc:164: this,
On 2016/02/19 05:24:54, Charlie Reis (slow til Mar 7) wrote:
> Curious, is this a git cl format thing?  In general, you don't need each
> parameter on its own line for a call site.

Yes it's a git cl format change: maybe it's different in an initialization list?

https://codereview.chromium.org/1693353002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/interstitial_page_impl.h (right):

https://codereview.chromium.org/1693353002/diff/60001/content/browser/frame_h...
content/browser/frame_host/interstitial_page_impl.h:294: // user choses to
proceed.
On 2016/02/19 05:24:54, Charlie Reis (slow til Mar 7) wrote:
> nit: chooses

Done.

https://codereview.chromium.org/1693353002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1693353002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:440:
DCHECK(frame_tree_node_->IsLoading());
On 2016/02/19 05:24:54, Charlie Reis (slow til Mar 7) wrote:
> Is there a race where this could fail, where the UI thread sets IsLoading to
> false just before the IO thread posts the task to OnCrossSiteResponse?  (Not
> sure, just checking.)

That would mean that we set is_loading to false not in response to an IPC (they
go through the IO thread) or an IO event. The only possible case I see is when
the user closes the BeforeUnload dialog and we reset the load state. I modified
the code so that it is now a boolean that we use to fire or not the
DidStopLoading at the end of the function.

https://codereview.chromium.org/1693353002/diff/60001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/1693353002/diff/60001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:1021: void
LoadingStateChanged(bool to_different_document,
On 2016/02/19 05:24:54, Charlie Reis (slow til Mar 7) wrote:
> Maybe this belongs in the "Tracking loading progress" section rather than the
> "Misc non-view stuff" section?

Done.

https://codereview.chromium.org/1693353002/diff/60001/content/test/test_web_c...
File content/test/test_web_contents.cc (right):

https://codereview.chromium.org/1693353002/diff/60001/content/test/test_web_c...
content/test/test_web_contents.cc:192: if (value)
On 2016/02/19 05:24:54, Charlie Reis (slow til Mar 7) wrote:
> Style nit: This needs braces (since the else block has braces).

Done.

https://codereview.chromium.org/1693353002/diff/60001/content/test/test_web_c...
content/test/test_web_contents.cc:201: RenderFrameHostImpl* pending_frame_host =
On 2016/02/19 05:24:54, Charlie Reis (slow til Mar 7) wrote:
> nit: Why not declare this inside the else branch below?

Done.

[clamy, 2016-02-19 12:10:08]

Description was changed from

==========
Reland #2 Remove the is_loading_ field from WebContentsImpl

The original CL was reverted because it introduced flakyness on some
SessionHistoryTest on the Cast Linux bot.

Original commit message:
This CL removes the is_loading_ field from WebContents, in favor of only
tracking the loading state through the FrameTree. Currently the loading state
is tracked in both, leading to more edge cases in the code. These edge cases
are removed by this CL, which allows the current architecture and PlzNavigate
to have the same behavior. It also removes some flakyness with
browser-initiated navigations and reenables 2 content_browsertests.

BUG=571887, 298193
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Reland #2 Remove the is_loading_ field from WebContentsImpl

The original CL was reverted because it introduced flakyness on some
SessionHistoryTest on the Cast Linux bot.

Original commit message:
This CL removes the is_loading_ field from WebContents, in favor of only
tracking the loading state through the FrameTree. Currently the loading state
is tracked in both, leading to more edge cases in the code. These edge cases
are removed by this CL, which allows the current architecture and PlzNavigate
to have the same behavior. It also removes some flakyness with
browser-initiated navigations and reenables 2 content_browsertests.

BUG=571887, 298193
TBR=nasko@chromium.org,rockot@chromium.org,nick@chromium.org,carlosk@chromium...
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[clamy, 2016-02-19 12:10:27]

The CQ bit was checked by clamy@chromium.org

[clamy, 2016-02-19 12:10:27]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1693353002/#ps80001
(title: "Addressed nits + rebase")

[commit-bot: I haz the power, 2016-02-19 12:12:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1693353002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1693353002/80001

[commit-bot: I haz the power, 2016-02-19 14:13:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-19 14:14:00]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[clamy, 2016-02-19 16:14:05]

The CQ bit was checked by clamy@chromium.org

[commit-bot: I haz the power, 2016-02-19 16:15:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1693353002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1693353002/80001

[commit-bot: I haz the power, 2016-02-19 17:27:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-19 17:27:47]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[clamy, 2016-02-22 10:24:10]

The CQ bit was checked by clamy@chromium.org

[commit-bot: I haz the power, 2016-02-22 10:24:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1693353002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1693353002/80001

[commit-bot: I haz the power, 2016-02-22 14:09:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-22 14:09:04]

Try jobs failed on following builders:
  linux_site_isolation on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)

[clamy, 2016-02-22 14:29:09]

The CQ bit was checked by clamy@chromium.org

[commit-bot: I haz the power, 2016-02-22 14:29:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1693353002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1693353002/80001

[commit-bot: I haz the power, 2016-02-22 15:38:32]

Description was changed from

==========
Reland #2 Remove the is_loading_ field from WebContentsImpl

The original CL was reverted because it introduced flakyness on some
SessionHistoryTest on the Cast Linux bot.

Original commit message:
This CL removes the is_loading_ field from WebContents, in favor of only
tracking the loading state through the FrameTree. Currently the loading state
is tracked in both, leading to more edge cases in the code. These edge cases
are removed by this CL, which allows the current architecture and PlzNavigate
to have the same behavior. It also removes some flakyness with
browser-initiated navigations and reenables 2 content_browsertests.

BUG=571887, 298193
TBR=nasko@chromium.org,rockot@chromium.org,nick@chromium.org,carlosk@chromium...
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Reland #2 Remove the is_loading_ field from WebContentsImpl

The original CL was reverted because it introduced flakyness on some
SessionHistoryTest on the Cast Linux bot.

Original commit message:
This CL removes the is_loading_ field from WebContents, in favor of only
tracking the loading state through the FrameTree. Currently the loading state
is tracked in both, leading to more edge cases in the code. These edge cases
are removed by this CL, which allows the current architecture and PlzNavigate
to have the same behavior. It also removes some flakyness with
browser-initiated navigations and reenables 2 content_browsertests.

BUG=571887, 298193
TBR=nasko@chromium.org,rockot@chromium.org,nick@chromium.org,carlosk@chromium...
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-02-22 15:38:33]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-02-22 15:39:21]

Description was changed from

==========
Reland #2 Remove the is_loading_ field from WebContentsImpl

The original CL was reverted because it introduced flakyness on some
SessionHistoryTest on the Cast Linux bot.

Original commit message:
This CL removes the is_loading_ field from WebContents, in favor of only
tracking the loading state through the FrameTree. Currently the loading state
is tracked in both, leading to more edge cases in the code. These edge cases
are removed by this CL, which allows the current architecture and PlzNavigate
to have the same behavior. It also removes some flakyness with
browser-initiated navigations and reenables 2 content_browsertests.

BUG=571887, 298193
TBR=nasko@chromium.org,rockot@chromium.org,nick@chromium.org,carlosk@chromium...
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Reland #2 Remove the is_loading_ field from WebContentsImpl

The original CL was reverted because it introduced flakyness on some
SessionHistoryTest on the Cast Linux bot.

Original commit message:
This CL removes the is_loading_ field from WebContents, in favor of only
tracking the loading state through the FrameTree. Currently the loading state
is tracked in both, leading to more edge cases in the code. These edge cases
are removed by this CL, which allows the current architecture and PlzNavigate
to have the same behavior. It also removes some flakyness with
browser-initiated navigations and reenables 2 content_browsertests.

BUG=571887, 298193
TBR=nasko@chromium.org,rockot@chromium.org,nick@chromium.org,carlosk@chromium...
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/44e84cef5a598bbee63ba83ad1cd5534e7295abd
Cr-Commit-Position: refs/heads/master@{#376732}
==========

[commit-bot: I haz the power, 2016-02-22 15:39:22]

Patchset 4 (id:??) landed as
https://crrev.com/44e84cef5a598bbee63ba83ad1cd5534e7295abd
Cr-Commit-Position: refs/heads/master@{#376732}