Reland of Refactor the loading tracking logic in WebContentsImpl.

Reland of Refactor the loading tracking logic in WebContentsImpl.

Reason for reland:
Changed the DCHECK to a Warning in the log.

This removes the loading_progresses_ map and the
loading_frames_in_progress counter in WebContentsImpl.
Instead, tracking of these is done at the RFHI level.

TBR=clamy@chromium.org,carlosk@chromium.org,nasko@chromium.org,creis@chromium.org,tasak@google.com
BUG=429399
Committed: https://crrev.com/4a49ce93d4f845512f4bdeb6c22850506bf1882a
Cr-Commit-Position: refs/heads/master@{#320305}

[Fabrice (no longer in Chrome), 2015-03-06 15:15:54]

Created Revert of Revert of Refactor the loading tracking logic in
WebContentsImpl.

[Fabrice (no longer in Chrome), 2015-03-07 00:33:10]

https://codereview.chromium.org/984983002/ has landed blink-side.
This fixes the only tests that failed, let's try relanding?

[Charlie Reis, 2015-03-07 01:03:48]

On 2015/03/07 00:33:10, Fabrice wrote:
> https://codereview.chromium.org/984983002/ has landed blink-side.
> This fixes the only tests that failed, let's try relanding?

Yep.  Rubber stamp LGTM.

[Fabrice (no longer in Chrome), 2015-03-07 01:11:39]

The CQ bit was checked by fdegans@chromium.org

[commit-bot: I haz the power, 2015-03-07 01:12:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/989473003/1

[commit-bot: I haz the power, 2015-03-07 03:13:51]

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

[commit-bot: I haz the power, 2015-03-07 03:13:52]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
Timed out jobs are not retried to avoid causing additional load on the builders
with large pending queues.

[Fabrice (no longer in Chrome), 2015-03-07 10:13:50]

The CQ bit was checked by fdegans@chromium.org

[commit-bot: I haz the power, 2015-03-07 10:14:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/989473003/1

[commit-bot: I haz the power, 2015-03-07 12:14:44]

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

[commit-bot: I haz the power, 2015-03-07 12:14:45]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
Timed out jobs are not retried to avoid causing additional load on the builders
with large pending queues.

[Fabrice (no longer in Chrome), 2015-03-09 15:56:19]

So, following the revert of my attempt to fix the issue in Blink, clamy@ and I
decided to remove the DCHECK.
The underlying issue is in Blink because tracking of the LoadEventProgress is
mixed with the BeforeUnloadEvent and depending on the status of each of them, we
should or should not fire the Start|StopLoading events.
This new patch is no worse than the current situation and will still fix a bunch
of issues. Realistically, we should still have a balance of Start and Stop
Loading events.
We can - and should - revisit this at a later time.

[clamy, 2015-03-10 13:49:10]

Thanks!

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

https://codereview.chromium.org/989473003/diff/190001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2923: // Unfortunately, it can
happen if there is a BeforeUnload event while the
I thought the problem was doing a history navigation during a
beforeUnload/unload event?

[Fabrice (no longer in Chrome), 2015-03-10 14:44:19]

Thanks!

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

https://codereview.chromium.org/989473003/diff/190001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2923: // Unfortunately, it can
happen if there is a BeforeUnload event while the
On 2015/03/10 13:49:10, clamy wrote:
> I thought the problem was doing a history navigation during a
> beforeUnload/unload event?
Yes that's what happens, clarified the comment.

[nasko, 2015-03-10 18:05:33]

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

https://codereview.chromium.org/989473003/diff/210001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2924: // BeforeUnload event.
Since this also applies to the unload event, let's be comprehensive here and add
it (as well as the comment in DidStopLoading).

[Fabrice (no longer in Chrome), 2015-03-10 18:10:10]

Thanks for the review!

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

https://codereview.chromium.org/989473003/diff/210001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2924: // BeforeUnload event.
On 2015/03/10 18:05:32, nasko wrote:
> Since this also applies to the unload event, let's be comprehensive here and
add
> it (as well as the comment in DidStopLoading).

Done.

[Charlie Reis, 2015-03-10 22:25:29]

Diff from original LGTM with nit.

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

https://codereview.chromium.org/989473003/diff/230001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2926: // refactored in Blink.
nit: Please list a bug number for this work, here and below.

[Fabrice (no longer in Chrome), 2015-03-11 15:30:37]

fdegans@chromium.org changed reviewers:
+ erikchen@chromium.org

[Fabrice (no longer in Chrome), 2015-03-11 15:30:38]

I rebased following https://codereview.chromium.org/996843002 +erikchen to make
sure we're still tracking the right things. I modified your ScopedTrackers a
bit, I think this will track what you actually want.

The bug inspired me to change one thing - and that is the only functional change
in the latest patch set - we'll only update the loading progress in
OnDidStopLoading if the loading progress is not "NotStarted". This way we won't
track the loading progress of "discarded" frames following a call to
ResetLoadProgressState, as we should. WDYT?

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

https://codereview.chromium.org/989473003/diff/230001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2926: // refactored in Blink.
On 2015/03/10 22:25:28, Charlie Reis wrote:
> nit: Please list a bug number for this work, here and below.

Done.

[erikchen, 2015-03-11 16:55:14]

The changes to ScopedTracker seem reasonable.

[Charlie Reis, 2015-03-11 23:20:52]

Sorry, not LGTM unless we clarify this last change.

On 2015/03/11 15:30:38, Fabrice wrote:
> I rebased following https://codereview.chromium.org/996843002 +erikchen to
make
> sure we're still tracking the right things. I modified your ScopedTrackers a
> bit, I think this will track what you actually want.
> 
> The bug inspired me to change one thing - and that is the only functional
change
> in the latest patch set - we'll only update the loading progress in
> OnDidStopLoading if the loading progress is not "NotStarted". This way we
won't
> track the loading progress of "discarded" frames following a call to
> ResetLoadProgressState, as we should. WDYT?

I don't understand this, and it's making things harder to reason about.

What do you mean by discarded frames?  Can you add a test case that shows why
this matters?

It seems like this change has the potential to suppress events that we may have
expected (e.g., WebContentsDelete::LoadProgressChanged), affect
loading_total_progress_, leave the RFH in the wrong state (i.e., 0 instead of
1), or have other unexpected behavior.  If it is correct, it's for very subtle
reasons that could be easy to regress (e.g., we know loading_total_progress_
can't change because the frame count used within SendLoadProgressChanged doesn't
happen to include frames with kLoadingProgressNotStarted).

I don't think this new change is worth the risk of bugs, unless there's a
concrete case it improves.

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

https://codereview.chromium.org/989473003/diff/270001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2991: if
(rfh->loading_progress() !=
I don't see why this change is safe, as explained above.

https://codereview.chromium.org/989473003/diff/270001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2999:
rfh->set_loading_progress(RenderFrameHostImpl::kLoadingProgressDone);
nit: Blank line above, to separate from the ScopedTracker.  Also below, as
before.

https://codereview.chromium.org/989473003/diff/270001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3002: // Then clean-up the
states.
nit: Blank line above, as before.

[Fabrice (no longer in Chrome), 2015-03-12 14:35:11]

As discussed offline last night, I reverted that change.

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

https://codereview.chromium.org/989473003/diff/270001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2991: if
(rfh->loading_progress() !=
On 2015/03/11 23:20:52, Charlie Reis wrote:
> I don't see why this change is safe, as explained above.

Done.

https://codereview.chromium.org/989473003/diff/270001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2999:
rfh->set_loading_progress(RenderFrameHostImpl::kLoadingProgressDone);
On 2015/03/11 23:20:52, Charlie Reis wrote:
> nit: Blank line above, to separate from the ScopedTracker.  Also below, as
> before.

Done.

https://codereview.chromium.org/989473003/diff/270001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3002: // Then clean-up the
states.
On 2015/03/11 23:20:52, Charlie Reis wrote:
> nit: Blank line above, as before.

Done.

[Charlie Reis, 2015-03-12 16:56:42]

Thanks!  LGTM again.

[Fabrice (no longer in Chrome), 2015-03-12 17:04:13]

The CQ bit was checked by fdegans@chromium.org

[commit-bot: I haz the power, 2015-03-12 17:05:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/989473003/290001

[commit-bot: I haz the power, 2015-03-12 17:11:45]

Committed patchset #7 (id:290001)

[commit-bot: I haz the power, 2015-03-12 17:12:33]

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