Reduce unnecessary download interruptions due to parallel requests

content/browser/download/download_file_impl.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/browser/download/download_file_impl.h"
 
 #include <string>
 #include <utility>
 
 #include "base/bind.h"
@@ skipping 423 matching lines @@
               AddNewSlice(source_stream->offset(), bytes_to_write);
             } else {
               received_slices_[source_stream->index()].received_bytes +=
                   bytes_to_write;
             }
           }
         }
         break;
       case ByteStreamReader::STREAM_COMPLETE:
         {
-          reason = static_cast<DownloadInterruptReason>(
+        reason = static_cast<DownloadInterruptReason>(
             source_stream->stream_reader()->GetStatus());
-          SendUpdate();
+        if (source_stream->length() == DownloadSaveInfo::kLengthFullContent &&
+            reason == DownloadInterruptReason::
+                          DOWNLOAD_INTERRUPT_REASON_SERVER_NO_RANGE) {

[xingliu, 2017/05/10 01:40:17]

Are errors for non-parallel download also affected by this change?

And do we plan to handle DOWNLOAD_INTERRUPT_REASON_SERVER_FAILED in this CL?
that's the error we've seen higher in the metric.

If we handle DOWNLOAD_INTERRUPT_REASON_SERVER_FAILED here, does it also affect
non-parallel downloads? 

In the long run, shall we assess the way we send errors around in
content/download to see if it makes sense to centralize the error piping around
the byte stream?

[qinmin, 2017/05/10 17:47:24]

For non-parallel download, if we receive
DOWNLOAD_INTERRUPT_REASON_SERVER_NO_RANGE, the current behavior is to restart
from the beginning. This is different from the parallel download behavior
because the parallel request can start from the end. Making the if statement
more restricted so it only applies to the request after the last received slice.

We don't need to special handle DOWNLOAD_INTERRUPT_REASON_SERVER_FAILED. that's
only for UMA purpose, I don't think it makes too much sense to single out a
specific error reason for special handing while it can be treated the same way
as the remaining errors.
There are several different errors: file error, server error and connection
error. DownloadFileImpl reports file errors, while the other errors need to be
passed down to here through thread hoppings. So if we want to centralize all
errors, we need to figure out which thread will be responsible for handling
this. and how are we going to pass those errors around. I don't think this is a
top priority thing to do, but maybe for long term refactoring purpose.

+          // We are probably reaching the end of the stream, don't treat this
+          // as an error.
+          reason = DOWNLOAD_INTERRUPT_REASON_NONE;
+        }
+        SendUpdate();
         }
         break;
       default:
         NOTREACHED();
         break;
     }
     now = base::TimeTicks::Now();
   } while (state == ByteStreamReader::STREAM_HAS_DATA &&
            reason == DOWNLOAD_INTERRUPT_REASON_NONE && now - start <= delta &&
            !should_terminate);
@@ skipping 259 matching lines @@
     const base::FilePath& new_path,
     const RenameCompletionCallback& completion_callback)
     : option(option),
       new_path(new_path),
       retries_left(kMaxRenameRetries),
       completion_callback(completion_callback) {}
 
 DownloadFileImpl::RenameParameters::~RenameParameters() {}
 
 }  // namespace content