Don't share ResourceDispatcherHostImpl's timer for reporting upload progress.

content/browser/loader/resource_loader.cc

Index: content/browser/loader/resource_loader.cc
diff --git a/content/browser/loader/resource_loader.cc b/content/browser/loader/resource_loader.cc
index 6855cf61d2051bd1cb45c37d93262bb53ee6f720..dd52b15b7b2c55c39dbd4cafb74b5cfd10f3b8d6 100644
--- a/content/browser/loader/resource_loader.cc
+++ b/content/browser/loader/resource_loader.cc
@@ -40,6 +40,9 @@ using base::TimeTicks;
 namespace content {
 namespace {
 
+// The interval for calls to ResourceLoader::ReportUploadProgress.
+const int kUploadProgressIntervalMsec = 100;
+
 void PopulateResourceResponse(ResourceRequestInfoImpl* info,
                               net::URLRequest* request,
                               ResourceResponse* response) {
@@ -87,7 +90,6 @@ ResourceLoader::ResourceLoader(scoped_ptr<net::URLRequest> request,
       handler_(handler.Pass()),
       delegate_(delegate),
       last_upload_position_(0),
-      waiting_for_upload_progress_ack_(false),
       is_transferring_(false),
       weak_ptr_factory_(this) {
   request_->set_delegate(this);
@@ -139,16 +141,10 @@ void ResourceLoader::CancelWithError(int error_code) {
 }
 
 void ResourceLoader::ReportUploadProgress() {
-  if (waiting_for_upload_progress_ack_)
-    return;  // Send one progress event at a time.
-
   net::UploadProgress progress = request_->GetUploadProgress();
   if (!progress.size())
     return;  // Nothing to upload.

[mmenke, 2015/05/20 20:05:41]

BUG:  A request will return a size of 0 until it actually starts a URLRequestJob
(Which the NetworkDelegate can delay).  I think you're best off just always
running the timer, until OnResponseStarted.

[Andre, 2015/05/20 21:05:12]

Thanks. Changed to use a RepeatingTimer.

 
-  if (progress.position() == last_upload_position_)
-    return;  // No progress made since last time.
-
   const uint64 kHalfPercentIncrements = 200;
   const TimeDelta kOneSecond = TimeDelta::FromMilliseconds(1000);
 
@@ -161,13 +157,11 @@ void ResourceLoader::ReportUploadProgress() {
   bool too_much_time_passed = time_since_last > kOneSecond;
 
   if (is_finished || enough_new_progress || too_much_time_passed) {
-    ResourceRequestInfoImpl* info = GetRequestInfo();
-    if (info->is_upload_progress_enabled()) {
-      handler_->OnUploadProgress(progress.position(), progress.size());
-      waiting_for_upload_progress_ack_ = true;
-    }
+    handler_->OnUploadProgress(progress.position(), progress.size());
     last_upload_ticks_ = TimeTicks::Now();
     last_upload_position_ = progress.position();

[mmenke, 2015/05/20 20:05:41]

BUG:  In the redirect case, we'll start uploading the file again, so will need
to start the timer once more.

[Andre, 2015/05/20 21:05:11]

Now using a RepeatingTimer until OnResponseStarted, which should happen after
redirect (I think?).

+  } else {
+    ScheduleReportUploadProgress();
   }
 }
 
@@ -217,7 +211,8 @@ void ResourceLoader::ClearLoginDelegate() {
 }
 
 void ResourceLoader::OnUploadProgressACK() {
-  waiting_for_upload_progress_ack_ = false;
+  if (last_upload_position_ < request_->GetUploadProgress().size())
+    ScheduleReportUploadProgress();
 }
 
 void ResourceLoader::OnReceivedRedirect(net::URLRequest* unused,
@@ -352,12 +347,6 @@ void ResourceLoader::OnResponseStarted(net::URLRequest* unused) {
     return;
   }
 

[mmenke, 2015/05/20 20:05:41]

Why did you remove the code here?

[Andre, 2015/05/20 21:05:12]

Because the way I was posting a delayed task in OnUploadProgressACK should
guarantee the final progress message being sent without needing this.
But now I've changed it to use a RepeatingTimer, so bringing this back.

-  // We want to send a final upload progress message prior to sending the
-  // response complete message even if we're waiting for an ack to to a
-  // previous upload progress message.
-  waiting_for_upload_progress_ack_ = false;
-  ReportUploadProgress();
-
   CompleteResponseStarted();
 
   if (is_deferred())
@@ -496,6 +485,9 @@ void ResourceLoader::StartRequestInternal() {
   request_->Start();
 
   delegate_->DidStartRequest(this);
+
+  if (GetRequestInfo()->is_upload_progress_enabled())
+    ScheduleReportUploadProgress();
 }
 
 void ResourceLoader::CancelRequestInternal(int error, bool from_renderer) {
@@ -726,6 +718,14 @@ void ResourceLoader::CallDidFinishLoading() {
   delegate_->DidFinishLoading(this);
 }
 
+void ResourceLoader::ScheduleReportUploadProgress() {
+  base::MessageLoop::current()->PostDelayedTask(
+      FROM_HERE,
+      base::Bind(&ResourceLoader::ReportUploadProgress,
+                 weak_ptr_factory_.GetWeakPtr()),
+      base::TimeDelta::FromMilliseconds(kUploadProgressIntervalMsec));
+}
+
 void ResourceLoader::RecordHistograms() {
   ResourceRequestInfoImpl* info = GetRequestInfo();