Prepare for per frame did{Start,Stop}Loading calls

Prepare for per frame did{Start,Stop}Loading calls

This teaches LoadProgressTracker about multiple frames loading at once and has it calculate progress as the product of all the individual frames' progresses.

RenderViewImpl has didStartLoading()/didStopLoading() shims still in place, which call the new RenderFrameImpl versions, which in turn call permanent versions in RenderViewImpl. RenderViewImpl's shims are marked as deprecated and will be deleted once blink calls RenderFrameImpl directly in https://codereview.chromium.org/183793002/.

BUG=347643
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256477

[Charlie Reis, 2014-02-27 21:52:07]

[+site-isolation-reviews]

The plan for moving the code sounds good to me.

The approach for computing overall progress doesn't seem right.  Is there a way
we can write a test for what we expect here?

https://codereview.chromium.org/180113003/diff/40001/content/renderer/load_pr...
File content/renderer/load_progress_tracker.cc (right):

https://codereview.chromium.org/180113003/diff/40001/content/renderer/load_pr...
content/renderer/load_progress_tracker.cc:29: progresses_[frame->GetRoutingID()]
= 0.1;
Is 0.1 the standard starting point from somewhere else in the code?  Maybe it
should be a kInitialProgress constant above?

https://codereview.chromium.org/180113003/diff/40001/content/renderer/load_pr...
content/renderer/load_progress_tracker.cc:38: // browser that load is complete.
nit: that load is complete -> that loading has progressed?
(It's not necessarily complete.)

https://codereview.chromium.org/180113003/diff/40001/content/renderer/load_pr...
content/renderer/load_progress_tracker.cc:64: ResetStates();
nit: 2 space indent.

https://codereview.chromium.org/180113003/diff/40001/content/renderer/load_pr...
content/renderer/load_progress_tracker.cc:83: progress *= it->second;
This doesn't make sense to me.  Consider a page with 3 frames, two of which are
complete and one is 50% complete.  That should be 5/6 complete, right?  But this
appears to give 1 * 1 * 0.5 = 0.5.  Am I missing something?

https://codereview.chromium.org/180113003/diff/40001/content/renderer/load_pr...
File content/renderer/load_progress_tracker.h (right):

https://codereview.chromium.org/180113003/diff/40001/content/renderer/load_pr...
content/renderer/load_progress_tracker.h:34: typedef base::hash_map<int, double>
ProgressMap;
Can you add a comment saying what the int and double represent?  (The double is
easy to guess, but would be useful to document the key.)

[Nate Chapin, 2014-02-27 21:58:38]

As far as I can tell, this code is completely untested currently. I observed it
manually 3 different ways: on trunk, with this patch, and with both this and the
blink side. I couldn't tell the difference visually.

I think it's feasible to test the chromium-side LoadProgressTracker in
isolation, but it would be very tricky to test the whole system reliably.

https://codereview.chromium.org/180113003/diff/40001/content/renderer/load_pr...
File content/renderer/load_progress_tracker.cc (right):

https://codereview.chromium.org/180113003/diff/40001/content/renderer/load_pr...
content/renderer/load_progress_tracker.cc:83: progress *= it->second;
On 2014/02/27 21:52:07, Charlie Reis wrote:
> This doesn't make sense to me.  Consider a page with 3 frames, two of which
are
> complete and one is 50% complete.  That should be 5/6 complete, right?  But
this
> appears to give 1 * 1 * 0.5 = 0.5.  Am I missing something?

You're right. There's a defense against decreasing the progress below, but we
shouldn't remove a frame from the progress map when it completes, only in
ResetStates.

[Nate Chapin, 2014-02-28 00:07:57]

On 2014/02/27 21:58:38, Nate Chapin wrote:
> As far as I can tell, this code is completely untested currently. I observed
it
> manually 3 different ways: on trunk, with this patch, and with both this and
the
> blink side. I couldn't tell the difference visually.
> 
> I think it's feasible to test the chromium-side LoadProgressTracker in
> isolation, but it would be very tricky to test the whole system reliably.
> 
>
https://codereview.chromium.org/180113003/diff/40001/content/renderer/load_pr...
> File content/renderer/load_progress_tracker.cc (right):
> 
>
https://codereview.chromium.org/180113003/diff/40001/content/renderer/load_pr...
> content/renderer/load_progress_tracker.cc:83: progress *= it->second;
> On 2014/02/27 21:52:07, Charlie Reis wrote:
> > This doesn't make sense to me.  Consider a page with 3 frames, two of which
> are
> > complete and one is 50% complete.  That should be 5/6 complete, right?  But
> this
> > appears to give 1 * 1 * 0.5 = 0.5.  Am I missing something?
> 
> You're right. There's a defense against decreasing the progress below, but we
> shouldn't remove a frame from the progress map when it completes, only in
> ResetStates.

Ok uploaded a new version with comments addressed.

I can look into add tests for LoadProgressTracker when I get back from vacation
on 3/10.

[Charlie Reis, 2014-02-28 02:02:38]

https://codereview.chromium.org/180113003/diff/40001/content/renderer/load_pr...
File content/renderer/load_progress_tracker.cc (right):

https://codereview.chromium.org/180113003/diff/40001/content/renderer/load_pr...
content/renderer/load_progress_tracker.cc:83: progress *= it->second;
On 2014/02/27 21:58:39, Nate Chapin wrote:
> On 2014/02/27 21:52:07, Charlie Reis wrote:
> > This doesn't make sense to me.  Consider a page with 3 frames, two of which
> are
> > complete and one is 50% complete.  That should be 5/6 complete, right?  But
> this
> > appears to give 1 * 1 * 0.5 = 0.5.  Am I missing something?
> 
> You're right. There's a defense against decreasing the progress below, but we
> shouldn't remove a frame from the progress map when it completes, only in
> ResetStates.

Oh, I even missed that in my earlier analysis.  That means that in the scenario
I described, we used to just see 0.5 for the one frame, but in the latest patch
we compute 1 * 1 * 0.5, which is still 0.5.

Don't we want 1 + 1 + 0.5 / 3?

[Nate Chapin, 2014-03-11 21:22:44]

On 2014/02/28 02:02:38, Charlie Reis wrote:
>
https://codereview.chromium.org/180113003/diff/40001/content/renderer/load_pr...
> File content/renderer/load_progress_tracker.cc (right):
> 
>
https://codereview.chromium.org/180113003/diff/40001/content/renderer/load_pr...
> content/renderer/load_progress_tracker.cc:83: progress *= it->second;
> On 2014/02/27 21:58:39, Nate Chapin wrote:
> > On 2014/02/27 21:52:07, Charlie Reis wrote:
> > > This doesn't make sense to me.  Consider a page with 3 frames, two of
which
> > are
> > > complete and one is 50% complete.  That should be 5/6 complete, right? 
But
> > this
> > > appears to give 1 * 1 * 0.5 = 0.5.  Am I missing something?
> > 
> > You're right. There's a defense against decreasing the progress below, but
we
> > shouldn't remove a frame from the progress map when it completes, only in
> > ResetStates.
> 
> Oh, I even missed that in my earlier analysis.  That means that in the
scenario
> I described, we used to just see 0.5 for the one frame, but in the latest
patch
> we compute 1 * 1 * 0.5, which is still 0.5.
> 
> Don't we want 1 + 1 + 0.5 / 3?

Yeah, that occurred to me while on vacation. Done.

Also, added a test. PTAL.

[Charlie Reis, 2014-03-11 21:51:11]

Just to be sure I understand, there's an extra step to the overall plan to move
LoadProgressTracker to the browser process, right?  That's the only place we'll
know about all the frames.

If so, this LGTM with nits.  (And please add a guard for the divide by zero.)

https://codereview.chromium.org/180113003/diff/100001/content/renderer/load_p...
File content/renderer/load_progress_tracker.cc (right):

https://codereview.chromium.org/180113003/diff/100001/content/renderer/load_p...
content/renderer/load_progress_tracker.cc:18: // minmum progress value.
nit: minimum

https://codereview.chromium.org/180113003/diff/100001/content/renderer/load_p...
content/renderer/load_progress_tracker.cc:62: min_delay) {
nit: I know this isn't yours, but can you add 4 spaces to this indent, since
it's an unfinished expression?  Right now it looks like another line of the
conditional.

https://codereview.chromium.org/180113003/diff/100001/content/renderer/load_p...
content/renderer/load_progress_tracker.cc:90: progress /= frameCount;
This is crying out to divide by zero if progresses_ is ever empty.  :)

https://codereview.chromium.org/180113003/diff/100001/content/renderer/render...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/180113003/diff/100001/content/renderer/render...
content/renderer/render_view_browsertest.cc:2234: // update withe minimum
progress.
nit: with the

[Nate Chapin, 2014-03-11 21:56:37]

On 2014/03/11 21:51:11, Charlie Reis wrote:
> Just to be sure I understand, there's an extra step to the overall plan to
move
> LoadProgressTracker to the browser process, right?  That's the only place
we'll
> know about all the frames.

Correct. This is just getting the logic over the repo boundary.

> 
> If so, this LGTM with nits.  (And please add a guard for the divide by zero.)
> 
>
https://codereview.chromium.org/180113003/diff/100001/content/renderer/load_p...
> File content/renderer/load_progress_tracker.cc (right):
> 
>
https://codereview.chromium.org/180113003/diff/100001/content/renderer/load_p...
> content/renderer/load_progress_tracker.cc:18: // minmum progress value.
> nit: minimum
> 
>
https://codereview.chromium.org/180113003/diff/100001/content/renderer/load_p...
> content/renderer/load_progress_tracker.cc:62: min_delay) {
> nit: I know this isn't yours, but can you add 4 spaces to this indent, since
> it's an unfinished expression?  Right now it looks like another line of the
> conditional.
> 
>
https://codereview.chromium.org/180113003/diff/100001/content/renderer/load_p...
> content/renderer/load_progress_tracker.cc:90: progress /= frameCount;
> This is crying out to divide by zero if progresses_ is ever empty.  :)
> 
>
https://codereview.chromium.org/180113003/diff/100001/content/renderer/render...
> File content/renderer/render_view_browsertest.cc (right):
> 
>
https://codereview.chromium.org/180113003/diff/100001/content/renderer/render...
> content/renderer/render_view_browsertest.cc:2234: // update withe minimum
> progress.
> nit: with the

[Nate Chapin, 2014-03-11 22:09:59]

https://codereview.chromium.org/180113003/diff/100001/content/renderer/load_p...
File content/renderer/load_progress_tracker.cc (right):

https://codereview.chromium.org/180113003/diff/100001/content/renderer/load_p...
content/renderer/load_progress_tracker.cc:62: min_delay) {
On 2014/03/11 21:51:12, Charlie Reis wrote:
> nit: I know this isn't yours, but can you add 4 spaces to this indent, since
> it's an unfinished expression?  Right now it looks like another line of the
> conditional.

I don't see why there's an extra line there, min_delay fits on the previous
line.

https://codereview.chromium.org/180113003/diff/100001/content/renderer/load_p...
content/renderer/load_progress_tracker.cc:90: progress /= frameCount;
On 2014/03/11 21:51:12, Charlie Reis wrote:
> This is crying out to divide by zero if progresses_ is ever empty.  :)

That shouldn't be possible, but...yeah, let's just add an early exit.

[Nate Chapin, 2014-03-11 22:12:11]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2014-03-11 22:35:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/japhet@chromium.org/180113003/120001

[commit-bot: I haz the power, 2014-03-12 07:58:49]

Change committed as 256477