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

content/renderer/load_progress_tracker.cc

Index: content/renderer/load_progress_tracker.cc
diff --git a/content/renderer/load_progress_tracker.cc b/content/renderer/load_progress_tracker.cc
index 0cc26fdcccf14ef4ac74b52f9ea36d252fe85d91..5a0d386165df5c294cdb76f72e603f111ce5e7af 100644
--- a/content/renderer/load_progress_tracker.cc
+++ b/content/renderer/load_progress_tracker.cc
@@ -18,35 +18,35 @@ const int kMinimumDelayBetweenUpdatesMS = 100;
 
 LoadProgressTracker::LoadProgressTracker(RenderViewImpl* render_view)
     : render_view_(render_view),
-      tracked_frame_(NULL),
-      progress_(0.0),
+      total_progress_(0.0),
       weak_factory_(this) {
 }
 
 LoadProgressTracker::~LoadProgressTracker() {
 }
 
-void LoadProgressTracker::DidStopLoading() {
-  if (!tracked_frame_)
+void LoadProgressTracker::DidStartLoading(RenderFrame* frame) {
+  progresses_[frame->GetRoutingID()] = 0.1;

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

Is 0.1 the standard starting point from somewhere else in the code?  Maybe it
should be a kInitialProgress constant above?

+  SendChangeLoadProgress();
+}
+
+void LoadProgressTracker::DidStopLoading(RenderFrame* frame) {
+  if (progresses_.find(frame->GetRoutingID()) == progresses_.end())
     return;
 
   // Load stopped while we were still tracking load.  Make sure we notify the
   // browser that load is complete.

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

nit: that load is complete -> that loading has progressed?
(It's not necessarily complete.)

-  progress_ = 1.0;
+  progresses_[frame->GetRoutingID()] = 1.0;
   SendChangeLoadProgress();
   // Then we clean-up our states.
-  ResetStates();
+  progresses_.erase(frame->GetRoutingID());
+  if (total_progress_ == 1.0)
+    ResetStates();
 }
 
-void LoadProgressTracker::DidChangeLoadProgress(blink::WebFrame* frame,
+void LoadProgressTracker::DidChangeLoadProgress(RenderFrame* frame,
                                                 double progress) {
-  if (tracked_frame_ && frame != tracked_frame_)
-    return;
-
-  if (!tracked_frame_)
-    tracked_frame_ = frame;
-
-  progress_ = progress;
+  progresses_[frame->GetRoutingID()] = progress;
 
   // We send the progress change to the browser immediately for the first and
   // last updates.  Also, since the message loop may be pretty busy when a page
@@ -60,8 +60,8 @@ void LoadProgressTracker::DidChangeLoadProgress(blink::WebFrame* frame,
     // If there is a pending task to send progress, it is now obsolete.
     weak_factory_.InvalidateWeakPtrs();
     SendChangeLoadProgress();
-    if (progress == 1.0)
-      ResetStates();
+    if (total_progress_ == 1.0)
+        ResetStates();

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

nit: 2 space indent.

     return;
   }
 
@@ -77,14 +77,23 @@ void LoadProgressTracker::DidChangeLoadProgress(blink::WebFrame* frame,
 
 void LoadProgressTracker::SendChangeLoadProgress() {
   last_time_progress_sent_ = base::TimeTicks::Now();
+  double progress = 1.0;
+  ProgressMap::iterator end = progresses_.end();
+  for (ProgressMap::iterator it = progresses_.begin(); it != end; ++it) {
+    progress *= it->second;

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

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?

[Nate Chapin, 2014/02/27 21:58:39]

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.

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

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?

+  }
+  if (progress < total_progress_)
+    return;
+  total_progress_ = progress;
+
   render_view_->Send(
       new ViewHostMsg_DidChangeLoadProgress(render_view_->routing_id(),
-                                            progress_));
+                                            progress));
 }
 
 void LoadProgressTracker::ResetStates() {
-  tracked_frame_ = NULL;
-  progress_ = 0.0;
+  progresses_.clear();
+  total_progress_ = 0.0;
   weak_factory_.InvalidateWeakPtrs();
   last_time_progress_sent_ = base::TimeTicks();
 }