Remove the is_loading_ field from WebContentsImpl

Remove the is_loading_ field from WebContentsImpl

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

Committed: https://crrev.com/db73eb6cf1e59e753fb53e8c744620b00606ccd5
Cr-Commit-Position: refs/heads/master@{#374651}

[clamy, 2016-01-14 17:35:14]

Description was changed from

==========
Remove the is_loading_ field from WebContentsImpl

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
==========

to

==========
Remove the is_loading_ field from WebContentsImpl

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
==========

[clamy, 2016-01-14 17:35:14]

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

[clamy, 2016-01-14 17:36:56]

@nasko: PTAL. This is the continuation of the DidStart/StopLoading refactoring.
I'm a bit unhappy that I could not fully remove booleans related to the loading
state from WebContents, but I had to leave one that relates to whether an
interstitial is showing or not.

[nasko, 2016-01-15 01:19:15]

nasko@chromium.org changed reviewers:
+ avi@chromium.org

[nasko, 2016-01-15 01:19:17]

Adding avi@, since an IPC he added is being removed.

Overall I think there is some room for simplifying this CL, since RenderView
isn't associated with any loading state anymore.

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

https://codereview.chromium.org/1545973002/diff/90001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:405: if
(current_frame_host->render_view_host() == render_view_host)
Loading state is no longer associated with RenderViewHost. Why do we need this?

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

https://codereview.chromium.org/1545973002/diff/90001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:242: void
RenderViewTerminated(RenderViewHostImpl* render_view_host);
Why do we need this method?

https://codereview.chromium.org/1545973002/diff/90001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:244: // Called when the user closed
the modal dialogue for BeforeUnload. This
Let's add to the comment that it is called only when the user cancelled the
navigation.

https://codereview.chromium.org/1545973002/diff/90001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1545973002/diff/90001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1039: return
frame_tree_.IsLoading() && !pause_throbber_for_interstitial_;
I still think the page is in loading state, even if it is displaying an
interstitial page. A document has been requested and it hasn't been fully loaded
yet. The fact that it is paused because we chose to ask the user something
doesn't mean that it stopped loading.

https://codereview.chromium.org/1545973002/diff/90001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2304: if
(frame_tree_.IsLoading()) {
What if it is already paused?

https://codereview.chromium.org/1545973002/diff/90001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2305:
LoadingStateChanged(false, true, NULL);
nullptr

https://codereview.chromium.org/1545973002/diff/90001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2307:
pause_throbber_for_interstitial_ = true;
If this is state, then it should be "paused".

https://codereview.chromium.org/1545973002/diff/90001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2325: if
(pause_throbber_for_interstitial_) {
Looking at the usage here, isn't this equivalent to the value of
ShowingInterstitialPage()?

https://codereview.chromium.org/1545973002/diff/90001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3974:
static_cast<RenderViewHostImpl*>(rvh)));
This method (RenderViewTerminated) is called when the process has died. There
are already enough hooks in RFHM and elsewhere that is notified of this event,
so I don't think we need yet more ways to notify hosts of the process dying.

https://codereview.chromium.org/1545973002/diff/90001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1545973002/diff/90001/content/renderer/render...
content/renderer/render_frame_impl.cc:4581: NOTREACHED();
Shouldn't this be CHECK for a short period of time to ensure we don't hit this
in the wild? I think avi@ needs to weigh in on this one.

https://codereview.chromium.org/1545973002/diff/90001/content/renderer/render...
content/renderer/render_frame_impl.cc:5130: // The browser expects the frame to
be loading this navigation. Informs it
Inform

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

https://codereview.chromium.org/1545973002/diff/90001/content/test/test_web_c...
content/test/test_web_contents.cc:216: if (value)
The else clause has {}, the if clause should too.

https://codereview.chromium.org/1545973002/diff/90001/content/test/web_conten...
File content/test/web_contents_observer_sanity_checker.cc (right):

https://codereview.chromium.org/1545973002/diff/90001/content/test/web_conten...
content/test/web_contents_observer_sanity_checker.cc:348: is_loading_(false),
What about window.open() with an URL? Does it also start from non-loading state?

[Avi (use Gerrit), 2016-01-15 04:43:06]

> Adding avi@, since an IPC he added is being removed.

I'm actually quite happy about this. I haven't liked this IPC; it's a remnant of
all the dropping navs.

[Avi (use Gerrit), 2016-01-15 04:48:00]

https://codereview.chromium.org/1545973002/diff/90001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1545973002/diff/90001/content/renderer/render...
content/renderer/render_frame_impl.cc:4581: NOTREACHED();
On 2016/01/15 01:19:16, nasko wrote:
> Shouldn't this be CHECK for a short period of time to ensure we don't hit this
> in the wild? I think avi@ needs to weigh in on this one.

In the comment when I said that we "shouldn't ever be dropping navigations", I
meant that in a philosophical sense of "it's bad design to drop navigations,
since we only cause weird corner cases, so we should re-design and re-work
Chrome to not do that." Perhaps my comment wasn't as clear as it could be.

In any case, I did not mean NOTREACHED. ShouldDisplayErrorPageForFailedLoad is
going to return |false| (in fact, the whole point of that function is to
sometimes return true and sometimes false) so to explode it in that case is bad
form.

[clamy, 2016-01-15 16:47:44]

Thanks!

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

https://codereview.chromium.org/1545973002/diff/90001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:405: if
(current_frame_host->render_view_host() == render_view_host)
On 2016/01/15 01:19:16, nasko wrote:
> Loading state is no longer associated with RenderViewHost. Why do we need
this?

Done.

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

https://codereview.chromium.org/1545973002/diff/90001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:242: void
RenderViewTerminated(RenderViewHostImpl* render_view_host);
On 2016/01/15 01:19:16, nasko wrote:
> Why do we need this method?

Method removed. The loading state is now reset in RFHM::RenderProcessGone.

https://codereview.chromium.org/1545973002/diff/90001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:244: // Called when the user closed
the modal dialogue for BeforeUnload. This
On 2016/01/15 01:19:16, nasko wrote:
> Let's add to the comment that it is called only when the user cancelled the
> navigation.

Done.

https://codereview.chromium.org/1545973002/diff/90001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1545973002/diff/90001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1039: return
frame_tree_.IsLoading() && !pause_throbber_for_interstitial_;
On 2016/01/15 01:19:16, nasko wrote:
> I still think the page is in loading state, even if it is displaying an
> interstitial page. A document has been requested and it hasn't been fully
loaded
> yet. The fact that it is paused because we chose to ask the user something
> doesn't mean that it stopped loading.

I'm just matching the current behavior here. When we show the interstitial, we
set is_loading_ in the WebContents to false, so WebContentsImpl::IsLoading will
return false. If I remember correctly, not doing it caused issues with some
browser tests.

https://codereview.chromium.org/1545973002/diff/90001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2304: if
(frame_tree_.IsLoading()) {
On 2016/01/15 01:19:16, nasko wrote:
> What if it is already paused?

Done.

https://codereview.chromium.org/1545973002/diff/90001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2305:
LoadingStateChanged(false, true, NULL);
On 2016/01/15 01:19:16, nasko wrote:
> nullptr

Done.

https://codereview.chromium.org/1545973002/diff/90001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2307:
pause_throbber_for_interstitial_ = true;
On 2016/01/15 01:19:16, nasko wrote:
> If this is state, then it should be "paused".

Done.

https://codereview.chromium.org/1545973002/diff/90001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2325: if
(pause_throbber_for_interstitial_) {
On 2016/01/15 01:19:16, nasko wrote:
> Looking at the usage here, isn't this equivalent to the value of
> ShowingInterstitialPage()?

No if the user starts a new navigation, DidProceedOnInterstitial gets called,
the throbber is restarted but the interstitial is still showing. I tried to just
use ShowingInterstitialPage(), but it did not work due to this particular case.

https://codereview.chromium.org/1545973002/diff/90001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3974:
static_cast<RenderViewHostImpl*>(rvh)));
On 2016/01/15 01:19:16, nasko wrote:
> This method (RenderViewTerminated) is called when the process has died. There
> are already enough hooks in RFHM and elsewhere that is notified of this event,
> so I don't think we need yet more ways to notify hosts of the process dying.

Done.

https://codereview.chromium.org/1545973002/diff/90001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1545973002/diff/90001/content/renderer/render...
content/renderer/render_frame_impl.cc:4581: NOTREACHED();
On 2016/01/15 04:48:00, Avi wrote:
> On 2016/01/15 01:19:16, nasko wrote:
> > Shouldn't this be CHECK for a short period of time to ensure we don't hit
this
> > in the wild? I think avi@ needs to weigh in on this one.
> 
> In the comment when I said that we "shouldn't ever be dropping navigations", I
> meant that in a philosophical sense of "it's bad design to drop navigations,
> since we only cause weird corner cases, so we should re-design and re-work
> Chrome to not do that." Perhaps my comment wasn't as clear as it could be.
> 
> In any case, I did not mean NOTREACHED. ShouldDisplayErrorPageForFailedLoad is
> going to return |false| (in fact, the whole point of that function is to
> sometimes return true and sometimes false) so to explode it in that case is
bad
> form.

This is is a PlzNavigate only method, and I realize now that it is not properly
indicated. In that case the filtering should be done on the browser side so it
really should be NOTREACHED. I added the proper markings of a PlzNavigate method
to make it clearer.

https://codereview.chromium.org/1545973002/diff/90001/content/renderer/render...
content/renderer/render_frame_impl.cc:5130: // The browser expects the frame to
be loading this navigation. Informs it
On 2016/01/15 01:19:16, nasko wrote:
> Inform

Done.

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

https://codereview.chromium.org/1545973002/diff/90001/content/test/test_web_c...
content/test/test_web_contents.cc:216: if (value)
On 2016/01/15 01:19:16, nasko wrote:
> The else clause has {}, the if clause should too.

Done.

https://codereview.chromium.org/1545973002/diff/90001/content/test/web_conten...
File content/test/web_contents_observer_sanity_checker.cc (right):

https://codereview.chromium.org/1545973002/diff/90001/content/test/web_conten...
content/test/web_contents_observer_sanity_checker.cc:348: is_loading_(false),
On 2016/01/15 01:19:16, nasko wrote:
> What about window.open() with an URL? Does it also start from non-loading
state?

I think so. You first create the WebContents then ask it to navigate. In any
case, the WebContentsObserverSanityChecker was not triggered in any of our
browser tests, so I think it should be fine.

[carlosk, 2016-01-15 17:24:03]

carlosk@chromium.org changed reviewers:
+ carlosk@chromium.org

[carlosk, 2016-01-15 17:24:04]

LGTM with a nit.

https://codereview.chromium.org/1545973002/diff/130001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1545973002/diff/130001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:1044: if
(IsBrowserSideNavigationEnabled() && speculative_render_frame_host_ &&
nit: why not do an if-else on the IsBrowserSideNavigationEnabled() check? I
think this helps others understand that in "vanilla" there is a pending and
there is not a speculative, while in PlzNavigate it's vice versa.

https://codereview.chromium.org/1545973002/diff/130001/content/test/test_web_...
File content/test/test_web_contents.cc (right):

https://codereview.chromium.org/1545973002/diff/130001/content/test/test_web_...
content/test/test_web_contents.cc:42: if (IsBrowserSideNavigationEnabled()) {
nit: same as the other case.

[Avi (use Gerrit), 2016-01-15 17:29:58]

avi@chromium.org changed reviewers:
- avi@chromium.org

[Avi (use Gerrit), 2016-01-15 17:29:59]

Nasko brought me in to double-check a few points and I'm happy there, so I'm
withdrawing.

[nasko, 2016-01-16 00:13:51]

A few more comments.

https://codereview.chromium.org/1545973002/diff/130001/content/browser/frame_...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/1545973002/diff/130001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.cc:413:
pending_frame_host->ResetLoadingState();
Do we need to touch the pending and speculative RFHs? If beforeunload was
canceled, then the navigation will be cancelled and those RFHs should disappear.

https://codereview.chromium.org/1545973002/diff/130001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1545973002/diff/130001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:1044: if
(IsBrowserSideNavigationEnabled() && speculative_render_frame_host_ &&
On 2016/01/15 17:24:04, carlosk wrote:
> nit: why not do an if-else on the IsBrowserSideNavigationEnabled() check? I
> think this helps others understand that in "vanilla" there is a pending and
> there is not a speculative, while in PlzNavigate it's vice versa.

Agreed. Having a clear switch with exclusive use of pending and speculative is
much easier to read and reason about.

https://codereview.chromium.org/1545973002/diff/130001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1545973002/diff/130001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:2304: if (IsLoading()) {
No need for {} in one line if statements.

https://codereview.chromium.org/1545973002/diff/130001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:2315:
LoadingStateChanged(true, true, NULL);
nullptr in all new code.

https://codereview.chromium.org/1545973002/diff/130001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:2328:
LoadingStateChanged(true, true, NULL);
nullptr in all new code.

https://codereview.chromium.org/1545973002/diff/130001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:4640:
rfh->frame_tree_node()->BeforeUnloadCanceled();
I wonder if this is the right place to dispatch this from. The IPC that
BeforeUnload has been completed is handled by RFH itself. Can't we just reset
the state there instead of having to come all the way out to WebContents and go
down back to the RFH?

[clamy, 2016-01-19 13:31:19]

Thanks!

https://codereview.chromium.org/1545973002/diff/130001/content/browser/frame_...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/1545973002/diff/130001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.cc:413:
pending_frame_host->ResetLoadingState();
On 2016/01/16 00:13:51, nasko wrote:
> Do we need to touch the pending and speculative RFHs? If beforeunload was
> canceled, then the navigation will be cancelled and those RFHs should
disappear.

BrowserTest.CancelBeforeUnloadResetsURL is failing if we don't do so. It expects
the throbber to stop immediately when we close the dialog, not after an IPC
round trip.

https://codereview.chromium.org/1545973002/diff/130001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1545973002/diff/130001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:1044: if
(IsBrowserSideNavigationEnabled() && speculative_render_frame_host_ &&
On 2016/01/16 00:13:51, nasko wrote:
> On 2016/01/15 17:24:04, carlosk wrote:
> > nit: why not do an if-else on the IsBrowserSideNavigationEnabled() check? I
> > think this helps others understand that in "vanilla" there is a pending and
> > there is not a speculative, while in PlzNavigate it's vice versa.
> 
> Agreed. Having a clear switch with exclusive use of pending and speculative is
> much easier to read and reason about.

Done.

https://codereview.chromium.org/1545973002/diff/130001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1545973002/diff/130001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:2304: if (IsLoading()) {
On 2016/01/16 00:13:51, nasko wrote:
> No need for {} in one line if statements.

Done.

https://codereview.chromium.org/1545973002/diff/130001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:2315:
LoadingStateChanged(true, true, NULL);
On 2016/01/16 00:13:51, nasko wrote:
> nullptr in all new code.

Done.

https://codereview.chromium.org/1545973002/diff/130001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:2328:
LoadingStateChanged(true, true, NULL);
On 2016/01/16 00:13:51, nasko wrote:
> nullptr in all new code.

Done.

https://codereview.chromium.org/1545973002/diff/130001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:4640:
rfh->frame_tree_node()->BeforeUnloadCanceled();
On 2016/01/16 00:13:51, nasko wrote:
> I wonder if this is the right place to dispatch this from. The IPC that
> BeforeUnload has been completed is handled by RFH itself. Can't we just reset
> the state there instead of having to come all the way out to WebContents and
go
> down back to the RFH?

As explained in the other comments about BeforeUnload canceled, there are test
that check explicitly for that behavior. Do we want to modify it? (it would be
user-visible)

https://codereview.chromium.org/1545973002/diff/130001/content/test/test_web_...
File content/test/test_web_contents.cc (right):

https://codereview.chromium.org/1545973002/diff/130001/content/test/test_web_...
content/test/test_web_contents.cc:42: if (IsBrowserSideNavigationEnabled()) {
On 2016/01/15 17:24:04, carlosk wrote:
> nit: same as the other case.

Done.

[clamy, 2016-01-19 13:38:12]

Patchset #9 (id:150001) has been deleted

[clamy, 2016-01-20 13:23:45]

FYI I had to do a significant rebase in the latest patch set.

https://codereview.chromium.org/1545973002/diff/210001/content/browser/frame_...
File content/browser/frame_host/frame_tree.h (right):

https://codereview.chromium.org/1545973002/diff/210001/content/browser/frame_...
content/browser/frame_host/frame_tree.h:81: class CONTENT_EXPORT
ConstNodeIterator {
FrameTree::IsLoading needs to iterate over the tree in a const fashion to be
called from WebContentsImpl::IsLoading. Since dcheng's patch removed ForEach
without providing a mean to iterate over FrameTreeNodes in a const fashion I had
to write the const iterator here.

https://codereview.chromium.org/1545973002/diff/210001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1545973002/diff/210001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:100: public
SiteInstanceImpl::Observer {
I tried to get RenderFrameHostManager to properly listen to SiteInstance
notifications about RenderProcessGone for its RenderFrameHosts. Then I realized
that it would be easier to just have the RenderFrameHost listens to its
SiteInstance and reset its load state when the RenderProcess is gone. Wdyt?

[carlosk, 2016-01-20 14:05:15]

Good changes.

https://codereview.chromium.org/1545973002/diff/210001/content/browser/frame_...
File content/browser/frame_host/frame_tree.h (right):

https://codereview.chromium.org/1545973002/diff/210001/content/browser/frame_...
content/browser/frame_host/frame_tree.h:81: class CONTENT_EXPORT
ConstNodeIterator {
On 2016/01/20 13:23:45, clamy wrote:
> FrameTree::IsLoading needs to iterate over the tree in a const fashion to be
> called from WebContentsImpl::IsLoading. Since dcheng's patch removed ForEach
> without providing a mean to iterate over FrameTreeNodes in a const fashion I
had
> to write the const iterator here.

Might be better to land this separately but I don't have a strong opinion on
this.

https://codereview.chromium.org/1545973002/diff/210001/content/browser/frame_...
content/browser/frame_host/frame_tree.h:81: class CONTENT_EXPORT
ConstNodeIterator {
On 2016/01/20 13:23:45, clamy wrote:
> FrameTree::IsLoading needs to iterate over the tree in a const fashion to be
> called from WebContentsImpl::IsLoading. Since dcheng's patch removed ForEach
> without providing a mean to iterate over FrameTreeNodes in a const fashion I
had
> to write the const iterator here.

I hate the duplication C++ requires for this kind of thing but indeed this is
what's needed.

https://codereview.chromium.org/1545973002/diff/210001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1545973002/diff/210001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:100: public
SiteInstanceImpl::Observer {
On 2016/01/20 13:23:45, clamy wrote:
> I tried to get RenderFrameHostManager to properly listen to SiteInstance
> notifications about RenderProcessGone for its RenderFrameHosts. Then I
realized
> that it would be easier to just have the RenderFrameHost listens to its
> SiteInstance and reset its load state when the RenderProcess is gone. Wdyt?

Looks better this way.

Note to other reviewers: it's easier to review this change by diffing patch sets
9 and 10.

[nasko, 2016-01-20 20:58:35]

Few more comments.

https://codereview.chromium.org/1545973002/diff/230001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1545973002/diff/230001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:698: ResetLoadingState();
Let's not add implementation of SiteInstanceImpl::Observer here, since it
requires adding an empty method. RFHM is already an observer and is already
changing to support calling ResetLoadingState() on its RFH. Let's just move this
code to its implementation of RenderProcessGone.

https://codereview.chromium.org/1545973002/diff/230001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:2247: is_loading_ = false;
Shouldn't this always be set, regardless of state? Also, swapped out is now
disabled, so we can consider it gone, so let's avoid using it in comments. It
can be rephrased as "when pending deletion."

https://codereview.chromium.org/1545973002/diff/230001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1545973002/diff/230001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:923:
frame_tree_node_->DidStopLoading();
Shouldn't loading state be independent of speculative RFH in PlzNavigate? It
should key off of the NavigationRequest, shouldn't it? In general, the
speculative RFH is an optimization and PlzNavigate should function without it
just fine.

https://codereview.chromium.org/1545973002/diff/230001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (left):

https://codereview.chromium.org/1545973002/diff/230001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:3944:
ResetLoadProgressState();
Why was this removed?

https://codereview.chromium.org/1545973002/diff/230001/content/test/web_conte...
File content/test/web_contents_observer_sanity_checker.cc (right):

https://codereview.chromium.org/1545973002/diff/230001/content/test/web_conte...
content/test/web_contents_observer_sanity_checker.cc:337: is_loading_ = true;
Now that we have the loading state, we should assert its value in all of the
navigation observer APIs. Feel free to do this in a separate CL if you wish.

[ncarter (slow), 2016-01-21 00:26:39]

nick@chromium.org changed reviewers:
+ nick@chromium.org

[ncarter (slow), 2016-01-21 00:26:40]

https://codereview.chromium.org/1545973002/diff/230001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1545973002/diff/230001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:694: void
RenderFrameHostImpl::RenderProcessGone(SiteInstanceImpl* site_instance) {
I'm glad you've done this. I also have envisioned making RenderFrameHostImpl be
a SiteInstanceImpl::Observer, as a way of replacing
FrameHostMsg_RenderProcessGone. 

Note this bug: https://code.google.com/p/chromium/issues/detail?id=575400

https://codereview.chromium.org/1545973002/diff/230001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:698: ResetLoadingState();
On 2016/01/20 20:58:34, nasko wrote:
> Let's not add implementation of SiteInstanceImpl::Observer here, since it
> requires adding an empty method. RFHM is already an observer and is already
> changing to support calling ResetLoadingState() on its RFH. Let's just move
this
> code to its implementation of RenderProcessGone.

I'm not sure I agree with nasko. I think it's more natural for the RFH to be the
SiteInstanceImpl::Observer.

RFHM currently observes the siteinstance on behalf of its proxies, and you can't
observe the same siteinstance twice, it seems troublesome for the RFHM to avoid
double-observation if it has to observe on behalf of its RFH's as well. We could
potentially wind up with a proxy in the same siteinstance as a pending delete
host, say -- so you'd need some kind of refcounting scheme.

Moreover, if RFH (instead of RFHM) observes the SiteInstance, then we get RAII
guarantees that AddObserver/RemoveObserver calls are balanced. If we do it in
the RFHM then it's yet another thing we have to manage manually for all the
various speculative/pending/ subcases.

[clamy, 2016-01-22 13:08:15]

Thanks!

https://codereview.chromium.org/1545973002/diff/230001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1545973002/diff/230001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:698: ResetLoadingState();
On 2016/01/21 00:26:40, ncarter wrote:
> On 2016/01/20 20:58:34, nasko wrote:
> > Let's not add implementation of SiteInstanceImpl::Observer here, since it
> > requires adding an empty method. RFHM is already an observer and is already
> > changing to support calling ResetLoadingState() on its RFH. Let's just move
> this
> > code to its implementation of RenderProcessGone.
> 
> I'm not sure I agree with nasko. I think it's more natural for the RFH to be
the
> SiteInstanceImpl::Observer.
> 
> RFHM currently observes the siteinstance on behalf of its proxies, and you
can't
> observe the same siteinstance twice, it seems troublesome for the RFHM to
avoid
> double-observation if it has to observe on behalf of its RFH's as well. We
could
> potentially wind up with a proxy in the same siteinstance as a pending delete
> host, say -- so you'd need some kind of refcounting scheme.
> 
> Moreover, if RFH (instead of RFHM) observes the SiteInstance, then we get
RAII
> guarantees that AddObserver/RemoveObserver calls are balanced. If we do it in
> the RFHM then it's yet another thing we have to manage manually for all the
> various speculative/pending/ subcases.

I initially tried to make things work in RFHM::RenderProcessGone, but getting to
observe the right SiteInstances was somewhat complex. In particular, I was
worried about some edge cases (such as when we create a pending RFH for a
SiteInstance that was a before a RFHP and that pending RFH later gets destroyed
and we recreate a RFHP for it).

I think it is much easier conceptually to have RFH be the SiteInstanceObserver:
you start observing the SiteInstance at creation of the RFH and stop at
deletion, and just reset your own loading state when it is gone. As for the
empty method, if we go on the route of RFH being an observer, can we just change
the SiteInstanceObserver::ActiveFrameCountIsZero from pure virtual to just
virtual?

https://codereview.chromium.org/1545973002/diff/230001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:2247: is_loading_ = false;
On 2016/01/20 20:58:34, nasko wrote:
> Shouldn't this always be set, regardless of state? Also, swapped out is now
> disabled, so we can consider it gone, so let's avoid using it in comments. It
> can be rephrased as "when pending deletion."

OnDidStopLoading will set it. If it is set beforehand, then OnDidStopLoading
just returns early without informing the tree of the change in load state.

I have updated the comment to make it clearer.

https://codereview.chromium.org/1545973002/diff/230001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1545973002/diff/230001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:923:
frame_tree_node_->DidStopLoading();
On 2016/01/20 20:58:34, nasko wrote:
> Shouldn't loading state be independent of speculative RFH in PlzNavigate? It
> should key off of the NavigationRequest, shouldn't it? In general, the
> speculative RFH is an optimization and PlzNavigate should function without it
> just fine.

It has to be dependent of it during the short time where we are committing the
navigation. The NavigationRequest no longer exist, so we rely on the fact that
the speculative RFH is loading to know that we are still loading a page.

https://codereview.chromium.org/1545973002/diff/230001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (left):

https://codereview.chromium.org/1545973002/diff/230001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:3944:
ResetLoadProgressState();
On 2016/01/20 20:58:34, nasko wrote:
> Why was this removed?
Brought it back.

https://codereview.chromium.org/1545973002/diff/230001/content/test/web_conte...
File content/test/web_contents_observer_sanity_checker.cc (right):

https://codereview.chromium.org/1545973002/diff/230001/content/test/web_conte...
content/test/web_contents_observer_sanity_checker.cc:337: is_loading_ = true;
On 2016/01/20 20:58:34, nasko wrote:
> Now that we have the loading state, we should assert its value in all of the
> navigation observer APIs. Feel free to do this in a separate CL if you wish.

Will do that in a separate CL. Added a TODO for it.

[nasko, 2016-01-22 17:52:59]

https://codereview.chromium.org/1545973002/diff/230001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1545973002/diff/230001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:698: ResetLoadingState();
On 2016/01/22 13:08:15, clamy wrote:
> On 2016/01/21 00:26:40, ncarter wrote:
> > On 2016/01/20 20:58:34, nasko wrote:
> > > Let's not add implementation of SiteInstanceImpl::Observer here, since it
> > > requires adding an empty method. RFHM is already an observer and is
already
> > > changing to support calling ResetLoadingState() on its RFH. Let's just
move
> > this
> > > code to its implementation of RenderProcessGone.
> > 
> > I'm not sure I agree with nasko. I think it's more natural for the RFH to be
> the
> > SiteInstanceImpl::Observer.
> > 
> > RFHM currently observes the siteinstance on behalf of its proxies, and you
> can't
> > observe the same siteinstance twice, it seems troublesome for the RFHM to
> avoid
> > double-observation if it has to observe on behalf of its RFH's as well. We
> could
> > potentially wind up with a proxy in the same siteinstance as a pending
delete
> > host, say -- so you'd need some kind of refcounting scheme.
> > 
> > Moreover, if RFH (instead of RFHM) observes the SiteInstance, then we get
RAII
> > guarantees that AddObserver/RemoveObserver calls are balanced. If we do it
in
> > the RFHM then it's yet another thing we have to manage manually for all the
> > various speculative/pending/ subcases.
> 
> I initially tried to make things work in RFHM::RenderProcessGone, but getting
to
> observe the right SiteInstances was somewhat complex. In particular, I was
> worried about some edge cases (such as when we create a pending RFH for a
> SiteInstance that was a before a RFHP and that pending RFH later gets
destroyed
> and we recreate a RFHP for it).
> 
> I think it is much easier conceptually to have RFH be the
SiteInstanceObserver:
> you start observing the SiteInstance at creation of the RFH and stop at
> deletion, and just reset your own loading state when it is gone. As for the
> empty method, if we go on the route of RFH being an observer, can we just
change
> the SiteInstanceObserver::ActiveFrameCountIsZero from pure virtual to just
> virtual?

I would prefer not to have the empty method here.

https://codereview.chromium.org/1545973002/diff/230001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1545973002/diff/230001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:923:
frame_tree_node_->DidStopLoading();
On 2016/01/22 13:08:15, clamy wrote:
> On 2016/01/20 20:58:34, nasko wrote:
> > Shouldn't loading state be independent of speculative RFH in PlzNavigate? It
> > should key off of the NavigationRequest, shouldn't it? In general, the
> > speculative RFH is an optimization and PlzNavigate should function without
it
> > just fine.
> 
> It has to be dependent of it during the short time where we are committing the
> navigation. The NavigationRequest no longer exist, so we rely on the fact that
> the speculative RFH is loading to know that we are still loading a page.

Looking at the callers of CleanUpNavigation, it seems that this one might be
problematic:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr....
In the case where same process navigation commits while having a cross-process
one, cancelling the cross-process navigation should not reset the loading state.
As it is, it will, wouldn't it?

We should have a browser test for this type of navigation, let's make sure we
add checks for the loading state if possible. Otherwise it might be good to have
an explicit test for it to avoid regressing it.

[nasko, 2016-01-22 17:58:27]

On 2016/01/22 17:52:59, nasko wrote:
>
https://codereview.chromium.org/1545973002/diff/230001/content/browser/frame_...
> File content/browser/frame_host/render_frame_host_impl.cc (right):
> 
>
https://codereview.chromium.org/1545973002/diff/230001/content/browser/frame_...
> content/browser/frame_host/render_frame_host_impl.cc:698: ResetLoadingState();
> On 2016/01/22 13:08:15, clamy wrote:
> > On 2016/01/21 00:26:40, ncarter wrote:
> > > On 2016/01/20 20:58:34, nasko wrote:
> > > > Let's not add implementation of SiteInstanceImpl::Observer here, since
it
> > > > requires adding an empty method. RFHM is already an observer and is
> already
> > > > changing to support calling ResetLoadingState() on its RFH. Let's just
> move
> > > this
> > > > code to its implementation of RenderProcessGone.
> > > 
> > > I'm not sure I agree with nasko. I think it's more natural for the RFH to
be
> > the
> > > SiteInstanceImpl::Observer.
> > > 
> > > RFHM currently observes the siteinstance on behalf of its proxies, and you
> > can't
> > > observe the same siteinstance twice, it seems troublesome for the RFHM to
> > avoid
> > > double-observation if it has to observe on behalf of its RFH's as well. We
> > could
> > > potentially wind up with a proxy in the same siteinstance as a pending
> delete
> > > host, say -- so you'd need some kind of refcounting scheme.
> > > 
> > > Moreover, if RFH (instead of RFHM) observes the SiteInstance, then we get
> RAII
> > > guarantees that AddObserver/RemoveObserver calls are balanced. If we do it
> in
> > > the RFHM then it's yet another thing we have to manage manually for all
the
> > > various speculative/pending/ subcases.
> > 
> > I initially tried to make things work in RFHM::RenderProcessGone, but
getting
> to
> > observe the right SiteInstances was somewhat complex. In particular, I was
> > worried about some edge cases (such as when we create a pending RFH for a
> > SiteInstance that was a before a RFHP and that pending RFH later gets
> destroyed
> > and we recreate a RFHP for it).
> > 
> > I think it is much easier conceptually to have RFH be the
> SiteInstanceObserver:
> > you start observing the SiteInstance at creation of the RFH and stop at
> > deletion, and just reset your own loading state when it is gone. As for the
> > empty method, if we go on the route of RFH being an observer, can we just
> change
> > the SiteInstanceObserver::ActiveFrameCountIsZero from pure virtual to just
> > virtual?
> 
> I would prefer not to have the empty method here.
> 
>
https://codereview.chromium.org/1545973002/diff/230001/content/browser/frame_...
> File content/browser/frame_host/render_frame_host_manager.cc (right):
> 
>
https://codereview.chromium.org/1545973002/diff/230001/content/browser/frame_...
> content/browser/frame_host/render_frame_host_manager.cc:923:
> frame_tree_node_->DidStopLoading();
> On 2016/01/22 13:08:15, clamy wrote:
> > On 2016/01/20 20:58:34, nasko wrote:
> > > Shouldn't loading state be independent of speculative RFH in PlzNavigate?
It
> > > should key off of the NavigationRequest, shouldn't it? In general, the
> > > speculative RFH is an optimization and PlzNavigate should function without
> it
> > > just fine.
> > 
> > It has to be dependent of it during the short time where we are committing
the
> > navigation. The NavigationRequest no longer exist, so we rely on the fact
that
> > the speculative RFH is loading to know that we are still loading a page.
> 
> Looking at the callers of CleanUpNavigation, it seems that this one might be
> problematic:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr....
> In the case where same process navigation commits while having a cross-process
> one, cancelling the cross-process navigation should not reset the loading
state.
> As it is, it will, wouldn't it?
> 
> We should have a browser test for this type of navigation, let's make sure we
> add checks for the loading state if possible. Otherwise it might be good to
have
> an explicit test for it to avoid regressing it.

I think related tests are disabled because of flakiness :(
https://code.google.com/p/chromium/issues/detail?id=298193

Can you check if they succeed with your latest changes?

[clamy, 2016-01-25 13:52:50]

Description was changed from

==========
Remove the is_loading_ field from WebContentsImpl

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
==========

to

==========
Remove the is_loading_ field from WebContentsImpl

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
==========

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

Thanks!

https://codereview.chromium.org/1545973002/diff/230001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1545973002/diff/230001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:698: ResetLoadingState();
On 2016/01/22 17:52:59, nasko wrote:
> On 2016/01/22 13:08:15, clamy wrote:
> > On 2016/01/21 00:26:40, ncarter wrote:
> > > On 2016/01/20 20:58:34, nasko wrote:
> > > > Let's not add implementation of SiteInstanceImpl::Observer here, since
it
> > > > requires adding an empty method. RFHM is already an observer and is
> already
> > > > changing to support calling ResetLoadingState() on its RFH. Let's just
> move
> > > this
> > > > code to its implementation of RenderProcessGone.
> > > 
> > > I'm not sure I agree with nasko. I think it's more natural for the RFH to
be
> > the
> > > SiteInstanceImpl::Observer.
> > > 
> > > RFHM currently observes the siteinstance on behalf of its proxies, and you
> > can't
> > > observe the same siteinstance twice, it seems troublesome for the RFHM to
> > avoid
> > > double-observation if it has to observe on behalf of its RFH's as well. We
> > could
> > > potentially wind up with a proxy in the same siteinstance as a pending
> delete
> > > host, say -- so you'd need some kind of refcounting scheme.
> > > 
> > > Moreover, if RFH (instead of RFHM) observes the SiteInstance, then we get
> RAII
> > > guarantees that AddObserver/RemoveObserver calls are balanced. If we do it
> in
> > > the RFHM then it's yet another thing we have to manage manually for all
the
> > > various speculative/pending/ subcases.
> > 
> > I initially tried to make things work in RFHM::RenderProcessGone, but
getting
> to
> > observe the right SiteInstances was somewhat complex. In particular, I was
> > worried about some edge cases (such as when we create a pending RFH for a
> > SiteInstance that was a before a RFHP and that pending RFH later gets
> destroyed
> > and we recreate a RFHP for it).
> > 
> > I think it is much easier conceptually to have RFH be the
> SiteInstanceObserver:
> > you start observing the SiteInstance at creation of the RFH and stop at
> > deletion, and just reset your own loading state when it is gone. As for the
> > empty method, if we go on the route of RFH being an observer, can we just
> change
> > the SiteInstanceObserver::ActiveFrameCountIsZero from pure virtual to just
> > virtual?
> 
> I would prefer not to have the empty method here.

I made it non pure virtual in SiteInstanceImpl::Observer. Let me know what you
think.

https://codereview.chromium.org/1545973002/diff/270001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl_browsertest.cc (right):

https://codereview.chromium.org/1545973002/diff/270001/content/browser/web_co...
content/browser/web_contents/web_contents_impl_browsertest.cc:198:
IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest, DidStopLoadingDetails) {
It appears that the patch fixed the flakyness on that one (I got 100 runs to
succeed locally).

https://codereview.chromium.org/1545973002/diff/270001/content/browser/web_co...
content/browser/web_contents/web_contents_impl_browsertest.cc:215:
DidStopLoadingDetailsWithPending) {
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]

It looks good.

Nick, does the conversion away from pure virtual in the observer interface look
good to you?

https://codereview.chromium.org/1545973002/diff/270001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl_browsertest.cc (right):

https://codereview.chromium.org/1545973002/diff/270001/content/browser/web_co...
content/browser/web_contents/web_contents_impl_browsertest.cc:215:
DidStopLoadingDetailsWithPending) {
On 2016/01/25 14:01:18, clamy wrote:
> 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.

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

[ncarter (slow), 2016-01-26 23:06:53]

SiteInstanceImpl::Observer changes lgtm, I didn't review the full CL

[clamy, 2016-01-28 13:51:22]

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

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

@rockot: PTAL at the changes in chrome/browser/extensions

https://codereview.chromium.org/1545973002/diff/270001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl_browsertest.cc (right):

https://codereview.chromium.org/1545973002/diff/270001/content/browser/web_co...
content/browser/web_contents/web_contents_impl_browsertest.cc:215:
DidStopLoadingDetailsWithPending) {
On 2016/01/26 01:34:27, nasko wrote:
> On 2016/01/25 14:01:18, clamy wrote:
> > 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.
> 
> Should there be a TODO to cover this case or do you plan to write a different
> test case when you fix 581024?

I added a TODO not to forget about it.

[Ken Rockot(use gerrit already), 2016-01-28 21:27:49]

https://codereview.chromium.org/1545973002/diff/290001/chrome/browser/extensi...
File chrome/browser/extensions/extension_resource_request_policy_apitest.cc
(right):

https://codereview.chromium.org/1545973002/diff/290001/chrome/browser/extensi...
chrome/browser/extensions/extension_resource_request_policy_apitest.cc:230:
browser(), accessible_newtab_override, 1);
I'm curious why these had to change from 2 to 1?

[clamy, 2016-01-29 10:44:55]

https://codereview.chromium.org/1545973002/diff/290001/chrome/browser/extensi...
File chrome/browser/extensions/extension_resource_request_policy_apitest.cc
(right):

https://codereview.chromium.org/1545973002/diff/290001/chrome/browser/extensi...
chrome/browser/extensions/extension_resource_request_policy_apitest.cc:230:
browser(), accessible_newtab_override, 1);
On 2016/01/28 21:27:49, Ken Rockot wrote:
> I'm curious why these had to change from 2 to 1?

Before the CL, there was a case where starting a navigation before a
browser-initiated pending one has committed could lead to DidStopLoading being
fired. The CL removes that case, and now you don't get a DidStartLoading if you
launch a new navigation in the middle of an ongoing one.

[Ken Rockot(use gerrit already), 2016-01-29 11:47:05]

Ah OK, thanks! lgtm

[nasko, 2016-01-29 15:02:20]

LGTM

[clamy, 2016-02-01 14:20:07]

Description was changed from

==========
Remove the is_loading_ field from WebContentsImpl

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

==========
Remove the is_loading_ field from WebContentsImpl

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-01 17:00:00]

The CQ bit was checked by clamy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-01 17:00:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1545973002/320024
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1545973002/320024

[commit-bot: I haz the power, 2016-02-01 17:26:06]

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

[commit-bot: I haz the power, 2016-02-01 17:26:07]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[clamy, 2016-02-02 16:04:04]

The CQ bit was checked by clamy@chromium.org to run a CQ dry run

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

@nasko: PTAL at the latest version, which has some changes with the version
previously lgtmed to ensure that WebContents::IsLoading always returns what is
expected.

[commit-bot: I haz the power, 2016-02-02 16:07:56]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1545973002/360001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1545973002/360001

[commit-bot: I haz the power, 2016-02-02 18:43:37]

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

[commit-bot: I haz the power, 2016-02-02 18:43:38]

Dry run: 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...)

[nasko, 2016-02-03 18:38:09]

There seem to be a few browser tests failing on the site isolation bot.

https://codereview.chromium.org/1545973002/diff/360001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1545973002/diff/360001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:3613: if
(paused_throbber_for_interstitial_ && pause_throbber_for_interstitial)
Why is this change needed? I don't follow based on the other changes in this
patchset.

[clamy, 2016-02-08 16:13:49]

Thanks!

I investigated the failures on the site isolation bots. They were due to an
issue where we would incorrectly fire DidStopLoading and then DidStartLoading in
the middle of a transfer navigation. I fixed the issue in the latest patch set.

https://codereview.chromium.org/1545973002/diff/360001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1545973002/diff/360001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:3613: if
(paused_throbber_for_interstitial_ && pause_throbber_for_interstitial)
On 2016/02/03 18:38:09, nasko wrote:
> Why is this change needed? I don't follow based on the other changes in this
> patchset.

It's so that the loading state is properly set when we start showing an
interstitial. When we are showing it, we don't want to send DidStart/StopLoading
notifications. So we were returning here if paused_throbber_for_interstitial_.

However in the case were we're setting it back to false, we want to emit a
DidStartLoading notification again. Hence the &&
pause_throbber_for_interstitial, since I'm now updating whether we're showing or
not the interstitial inside the method.

The updating is done inside the method to sort issues where we need to fire a
DidStopLoading notifications when we start showing the interstitial. At that
point paused_throbber_for_interstitial_ should be false so that we continue.
However, it needs to be set to true before we fire DidStopLoading otherwise
IsLoading will still return true.

[nasko, 2016-02-09 00:12:23]

It looks like a different set of tests related to site isolation are now broken.

https://codereview.chromium.org/1545973002/diff/360001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1545973002/diff/360001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:3613: if
(paused_throbber_for_interstitial_ && pause_throbber_for_interstitial)
On 2016/02/08 16:13:49, clamy wrote:
> On 2016/02/03 18:38:09, nasko wrote:
> > Why is this change needed? I don't follow based on the other changes in this
> > patchset.
> 
> It's so that the loading state is properly set when we start showing an
> interstitial. When we are showing it, we don't want to send
DidStart/StopLoading
> notifications. So we were returning here if paused_throbber_for_interstitial_.
> 
> However in the case were we're setting it back to false, we want to emit a
> DidStartLoading notification again. Hence the &&
> pause_throbber_for_interstitial, since I'm now updating whether we're showing
or
> not the interstitial inside the method.
> 
> The updating is done inside the method to sort issues where we need to fire a
> DidStopLoading notifications when we start showing the interstitial. At that
> point paused_throbber_for_interstitial_ should be false so that we continue.
> However, it needs to be set to true before we fire DidStopLoading otherwise
> IsLoading will still return true.

Acknowledged.

[clamy, 2016-02-09 17:02:58]

@nasko: PTAL. I think it should be good for now (though I'm still waiting on the
result from the site isolation bot).

[nasko, 2016-02-09 17:15:24]

Still LGTM with a couple of nits.

https://codereview.chromium.org/1545973002/diff/440001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1545973002/diff/440001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:342: // that this works both for a
transfer to a different RenderFrameHost in the
Missing an "and" after "different RenderFrameHost"? The sentence doesn't read
quite right as it is.

https://codereview.chromium.org/1545973002/diff/440001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1545973002/diff/440001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:255: // should be sent
during the transfer.
nit: Let's add a TODO() to say that this can be removed once PlzNavigate ships.
It will be easier to grep for it when we are done with the old codepath :).

[clamy, 2016-02-10 12:03:31]

Thanks!

https://codereview.chromium.org/1545973002/diff/440001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1545973002/diff/440001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:342: // that this works both for a
transfer to a different RenderFrameHost in the
On 2016/02/09 17:15:24, nasko wrote:
> Missing an "and" after "different RenderFrameHost"? The sentence doesn't read
> quite right as it is.

Done.

https://codereview.chromium.org/1545973002/diff/440001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1545973002/diff/440001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:255: // should be sent
during the transfer.
On 2016/02/09 17:15:24, nasko wrote:
> nit: Let's add a TODO() to say that this can be removed once PlzNavigate
ships.
> It will be easier to grep for it when we are done with the old codepath :).

Done.

[clamy, 2016-02-10 12:03:46]

The CQ bit was checked by clamy@chromium.org

[clamy, 2016-02-10 12:03:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
carlosk@chromium.org, rockot@chromium.org, nick@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1545973002/#ps460001
(title: "Rebase + addressed Nasko's nits")

[commit-bot: I haz the power, 2016-02-10 12:04:25]

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

[commit-bot: I haz the power, 2016-02-10 13:28:04]

Committed patchset #24 (id:460001)

[commit-bot: I haz the power, 2016-02-10 13:30:10]

Description was changed from

==========
Remove the is_loading_ field from WebContentsImpl

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

==========
Remove the is_loading_ field from WebContentsImpl

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

Committed: https://crrev.com/db73eb6cf1e59e753fb53e8c744620b00606ccd5
Cr-Commit-Position: refs/heads/master@{#374651}
==========

[commit-bot: I haz the power, 2016-02-10 13:30:11]

Patchset 24 (id:??) landed as
https://crrev.com/db73eb6cf1e59e753fb53e8c744620b00606ccd5
Cr-Commit-Position: refs/heads/master@{#374651}

[waffles, 2016-02-10 17:41:01]

A revert of this CL (patchset #24 id:460001) has been created in
https://codereview.chromium.org/1690633002/ by waffles@chromium.org.

The reason for reverting is: CHECK(is_loading_) appears to fail (flakily?) on 
Linux ChromiumOS Tests (1). More details in the bug..