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

content/renderer/load_progress_tracker.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/renderer/load_progress_tracker.h"
 
 #include "base/bind.h"
 #include "base/message_loop/message_loop.h"
 #include "content/common/view_messages.h"
 #include "content/renderer/render_view_impl.h"
 
 namespace content {
 namespace {
 
 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
   // is loaded, it might not execute a posted task in a timely manner so we make
   // sure to immediately send progress report if enough time has passed.
   base::TimeDelta min_delay =
       base::TimeDelta::FromMilliseconds(kMinimumDelayBetweenUpdatesMS);
   if (progress == 1.0 || last_time_progress_sent_.is_null() ||
       base::TimeTicks::Now() - last_time_progress_sent_ >
       min_delay) {
     // 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;
   }
 
   if (weak_factory_.HasWeakPtrs())
     return;
 
   base::MessageLoop::current()->PostDelayedTask(
       FROM_HERE,
       base::Bind(&LoadProgressTracker::SendChangeLoadProgress,
                  weak_factory_.GetWeakPtr()),
       min_delay);
 }
 
 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();
 }
 
 }  // namespace content