Rewrite download unittests that uses ByteStreamReader

ByteStreamReader is in content/ and download should avoid such dependency.
Replace all the ByteStreamReader mocks with mocks of download::InputStream

Bug: 803135
Change-Id: I3650d482943c604bedc39c0db8ec7b1021656fe4
Reviewed-on: https://chromium-review.googlesource.com/957446Reviewed-by: Xing Liu <xingliu@chromium.org>
Commit-Queue: Min Qin <qinmin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542293}

components/download/internal/common/stream_handle_input_stream.cc

@@ -100,12 +100,7 @@ DownloadInterruptReason StreamHandleInputStream::GetCompletionStatus() {
 void StreamHandleInputStream::OnStreamCompleted(
     mojom::NetworkRequestStatus status) {
   // This can be called before or after data pipe is completely drained.
-  OnResponseCompleted(ConvertMojoNetworkRequestStatusToInterruptReason(status));
-}
-
-void StreamHandleInputStream::OnResponseCompleted(
-    DownloadInterruptReason status) {
-  completion_status_ = status;
+  completion_status_ = ConvertMojoNetworkRequestStatusToInterruptReason(status);
   is_response_completed_ = true;
   if (completion_callback_)
     std::move(completion_callback_).Run();

components/download/public/common/BUILD.gn

@@ -38,6 +38,7 @@ component("public") {
     "download_url_parameters.cc",
     "download_url_parameters.h",
     "download_utils.h",
+    "input_stream.cc",
     "input_stream.h",
     "rate_estimator.h",
     "resume_mode.h",

components/download/public/common/input_stream.cc

+// Copyright 2018 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 "components/download/public/common/input_stream.h"
+
+namespace download {
+
+InputStream::~InputStream() = default;
+
+void InputStream::Initialize() {}
+
+void InputStream::RegisterDataReadyCallback(
+    const mojo::SimpleWatcher::ReadyCallback& callback) {}
+
+void InputStream::ClearDataReadyCallback() {}
+
+void InputStream::RegisterCompletionCallback(base::OnceClosure callback) {}
+
+}  // namespace download

components/download/public/common/input_stream.h

@@ -24,21 +24,21 @@ class COMPONENTS_DOWNLOAD_EXPORT InputStream {
     COMPLETE,
   };
 
-  virtual ~InputStream() = default;
+  virtual ~InputStream();
 
   // Initializes the inputStream object.
-  virtual void Initialize() {}
+  virtual void Initialize();
 
   // Returns true if the input stream contains no data, or false otherwise.
   virtual bool IsEmpty() = 0;
 
   // Register/clear callbacks when data become available.
   virtual void RegisterDataReadyCallback(
-      const mojo::SimpleWatcher::ReadyCallback& callback) {}
-  virtual void ClearDataReadyCallback() {}
+      const mojo::SimpleWatcher::ReadyCallback& callback);
+  virtual void ClearDataReadyCallback();
 
   // Registers stream completion callback if needed.
-  virtual void RegisterCompletionCallback(base::OnceClosure callback) {}
+  virtual void RegisterCompletionCallback(base::OnceClosure callback);
 
   // Reads data from the stream into |data|, |length| is the number of bytes
   // returned.
@@ -47,9 +47,6 @@ class COMPONENTS_DOWNLOAD_EXPORT InputStream {
 
   // Returns the completion status.
   virtual DownloadInterruptReason GetCompletionStatus() = 0;
-
-  // Mark the InputStream as completed and set the completion status.
-  virtual void OnResponseCompleted(DownloadInterruptReason status){};
 };
 
 }  // namespace download

components/download/public/common/stream_handle_input_stream.h

@@ -32,7 +32,6 @@ class COMPONENTS_DOWNLOAD_EXPORT StreamHandleInputStream
   InputStream::StreamState Read(scoped_refptr<net::IOBuffer>* data,
                                           size_t* length) override;
   DownloadInterruptReason GetCompletionStatus() override;
-  void OnResponseCompleted(DownloadInterruptReason status) override;
 
   // mojom::DownloadStreamClient
   void OnStreamCompleted(mojom::NetworkRequestStatus status) override;

content/browser/download/byte_stream_input_stream.cc

@@ -56,9 +56,4 @@ download::DownloadInterruptReason ByteStreamInputStream::GetCompletionStatus() {
   return completion_status_;
 }
 
-void ByteStreamInputStream::OnResponseCompleted(
-    download::DownloadInterruptReason status) {
-  completion_status_ = status;
-}
-
 }  // namespace content

content/browser/download/byte_stream_input_stream.h

@@ -27,7 +27,6 @@ class CONTENT_EXPORT ByteStreamInputStream : public download::InputStream {
   download::InputStream::StreamState Read(scoped_refptr<net::IOBuffer>* data,
                                           size_t* length) override;
   download::DownloadInterruptReason GetCompletionStatus() override;
-  void OnResponseCompleted(download::DownloadInterruptReason status) override;
 
  private:
   // ByteStreamReader to read from.

content/browser/download/download_file_impl.cc

@@ -102,11 +102,6 @@ void DownloadFileImpl::SourceStream::ClearDataReadyCallback() {
   input_stream_->ClearDataReadyCallback();
 }
 
-void DownloadFileImpl::SourceStream::OnResponseCompleted(
-    download::DownloadInterruptReason reason) {
-  input_stream_->OnResponseCompleted(reason);
-}
-
 download::DownloadInterruptReason
 DownloadFileImpl::SourceStream::GetCompletionStatus() const {
   return input_stream_->GetCompletionStatus();

content/browser/download/download_file_impl.h

@@ -95,9 +95,6 @@ class CONTENT_EXPORT DownloadFileImpl : public DownloadFile {
 
     void Initialize();
 
-    // Called when response is completed.
-    void OnResponseCompleted(download::DownloadInterruptReason reason);
-
     // Called after successfully writing a buffer to disk.
     void OnWriteBytesToDisk(int64_t bytes_write);
 

content/browser/download/download_job.h

@@ -10,7 +10,6 @@
 #include "base/time/time.h"
 #include "components/download/public/common/download_interrupt_reasons.h"
 #include "components/download/public/common/download_request_handle_interface.h"
-#include "content/browser/byte_stream.h"
 #include "content/browser/download/download_file.h"
 #include "content/common/content_export.h"
 

content/browser/download/mock_input_stream.cc

+// Copyright 2018 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/mock_input_stream.h"
+
+namespace content {
+
+MockInputStream::MockInputStream() = default;
+
+MockInputStream::~MockInputStream() = default;
+
+}  // namespace content

content/browser/download/mock_input_stream.h

+// Copyright 2018 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.
+
+#ifndef CONTENT_BROWSER_DOWNLOAD_MOCK_INPUT_STREAM_H_
+#define CONTENT_BROWSER_DOWNLOAD_MOCK_INPUT_STREAM_H_
+
+#include "components/download/public/common/input_stream.h"
+#include "testing/gmock/include/gmock/gmock.h"
+
+namespace content {
+
+class MockInputStream : public download::InputStream {
+ public:
+  MockInputStream();
+  ~MockInputStream() override;
+
+  // download::InputStream functions
+  MOCK_METHOD0(IsEmpty, bool());
+  MOCK_METHOD1(RegisterDataReadyCallback,
+               void(const mojo::SimpleWatcher::ReadyCallback&));
+  MOCK_METHOD0(ClearDataReadyCallback, void());
+  MOCK_METHOD2(Read,
+               download::InputStream::StreamState(scoped_refptr<net::IOBuffer>*,
+                                                  size_t*));
+  MOCK_METHOD0(GetCompletionStatus, download::DownloadInterruptReason());
+};
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_DOWNLOAD_MOCK_INPUT_STREAM_H_

content/browser/download/parallel_download_job_unittest.cc

@@ -13,10 +13,10 @@
 #include "base/test/scoped_task_environment.h"
 #include "components/download/public/common/download_destination_observer.h"
 #include "components/download/public/common/download_task_runner.h"
-#include "content/browser/download/byte_stream_input_stream.h"
 #include "content/browser/download/download_file_impl.h"
 #include "content/browser/download/download_item_impl_delegate.h"
 #include "content/browser/download/mock_download_item_impl.h"
+#include "content/browser/download/mock_input_stream.h"
 #include "content/browser/download/parallel_download_utils.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -57,15 +57,6 @@ class MockDownloadDestinationObserver
   MOCK_METHOD2(CurrentUpdateStatus, void(int64_t, int64_t));
 };
 
-class MockByteStreamReader : public ByteStreamReader {
- public:
-  MOCK_METHOD2(Read,
-               ByteStreamReader::StreamState(scoped_refptr<net::IOBuffer>*,
-                                             size_t*));
-  MOCK_CONST_METHOD0(GetStatus, int());
-  MOCK_METHOD1(RegisterCallback, void(const base::Closure&));
-};
-
 }  // namespace
 
 class ParallelDownloadJobForTest : public ParallelDownloadJob {
@@ -182,9 +173,7 @@ class ParallelDownloadJobTest : public testing::Test {
         std::make_unique<download::DownloadCreateInfo>();
     create_info->request_handle = std::move(request_handle);
     delegate->OnUrlDownloadStarted(
-        std::move(create_info),
-        std::make_unique<ByteStreamInputStream>(
-            std::make_unique<MockByteStreamReader>()),
+        std::move(create_info), std::make_unique<MockInputStream>(),
         download::DownloadUrlParameters::OnStartedCallback());
   }
 
@@ -486,16 +475,14 @@ TEST_F(ParallelDownloadJobTest, RemainingContentWillFinishSoon) {
 // Test that parallel request is not created until download file is initialized.
 TEST_F(ParallelDownloadJobTest, ParallelRequestNotCreatedUntilFileInitialized) {
   auto save_info = std::make_unique<download::DownloadSaveInfo>();
-  StrictMock<MockByteStreamReader>* input_stream =
-      new StrictMock<MockByteStreamReader>();
+  StrictMock<MockInputStream>* input_stream = new StrictMock<MockInputStream>();
   auto observer =
       std::make_unique<StrictMock<MockDownloadDestinationObserver>>();
   base::WeakPtrFactory<download::DownloadDestinationObserver> observer_factory(
       observer.get());
   auto download_file = std::make_unique<DownloadFileImpl>(
       std::move(save_info), base::FilePath(),
-      std::make_unique<ByteStreamInputStream>(
-          std::unique_ptr<ByteStreamReader>(input_stream)),
+      std::unique_ptr<MockInputStream>(input_stream),
       download::DownloadItem::kInvalidId, observer_factory.GetWeakPtr());
   CreateParallelJob(0, 100, download::DownloadItem::ReceivedSlices(), 2, 0, 0);
   job_->Start(download_file.get(),
@@ -504,7 +491,7 @@ TEST_F(ParallelDownloadJobTest, ParallelRequestNotCreatedUntilFileInitialized) {
               download::DownloadItem::ReceivedSlices());
   EXPECT_FALSE(file_initialized_);
   EXPECT_EQ(0u, job_->workers().size());
-  EXPECT_CALL(*input_stream, RegisterCallback(_));
+  EXPECT_CALL(*input_stream, RegisterDataReadyCallback(_));
   EXPECT_CALL(*input_stream, Read(_, _));
   EXPECT_CALL(*(observer.get()), DestinationUpdate(_, _, _));
   task_environment_.RunUntilIdle();

content/browser/download/parallel_download_utils_unittest.cc

@@ -11,47 +11,44 @@
 #include "base/test/scoped_feature_list.h"
 #include "components/download/public/common/download_features.h"
 #include "components/download/public/common/download_save_info.h"
-#include "content/browser/byte_stream.h"
-#include "content/browser/download/byte_stream_input_stream.h"
+#include "content/browser/download/mock_input_stream.h"
 #include "content/public/browser/download_manager.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using ::testing::Return;
+using ::testing::StrictMock;
+
 namespace content {
 
 namespace {
 
 const int kErrorStreamOffset = 100;
 
-class MockByteStreamReader : public ByteStreamReader {
+}  // namespace
+
+class ParallelDownloadUtilsTest : public testing::Test {};
+
+class ParallelDownloadUtilsRecoverErrorTest
+    : public ::testing::TestWithParam<int64_t> {
  public:
-  MockByteStreamReader() {}
-  ~MockByteStreamReader() override {}
-
-  // ByteStream functions
-  MOCK_METHOD2(Read,
-               ByteStreamReader::StreamState(scoped_refptr<net::IOBuffer>*,
-                                             size_t*));
-  MOCK_CONST_METHOD0(GetStatus, int());
-  MOCK_METHOD1(RegisterCallback, void(const base::Closure&));
-};
+  ParallelDownloadUtilsRecoverErrorTest() : input_stream_(nullptr) {}
 
-// Creates a source stream to test.
-std::unique_ptr<DownloadFileImpl::SourceStream> CreateSourceStream(
+  // Creates a source stream to test.
+  std::unique_ptr<DownloadFileImpl::SourceStream> CreateSourceStream(
       int64_t offset,
       int64_t length) {
-  auto input_stream = std::make_unique<ByteStreamInputStream>(
-      std::make_unique<MockByteStreamReader>());
+    input_stream_ = new StrictMock<MockInputStream>();
+    EXPECT_CALL(*input_stream_, GetCompletionStatus())
+        .WillRepeatedly(Return(download::DOWNLOAD_INTERRUPT_REASON_NONE));
     return std::make_unique<DownloadFileImpl::SourceStream>(
-      offset, length, std::move(input_stream));
-}
-
-}  // namespace
-
-class ParallelDownloadUtilsTest : public testing::Test {};
+        offset, length, std::unique_ptr<MockInputStream>(input_stream_));
+  }
 
-class ParallelDownloadUtilsRecoverErrorTest
-    : public ::testing::TestWithParam<int64_t> {};
+ protected:
+  // Stream for sending data into the SourceStream.
+  StrictMock<MockInputStream>* input_stream_;
+};
 
 TEST_F(ParallelDownloadUtilsTest, FindSlicesToDownload) {
   std::vector<download::DownloadItem::ReceivedSlice> downloaded_slices;
@@ -265,6 +262,7 @@ TEST_P(ParallelDownloadUtilsRecoverErrorTest,
        RecoverErrorForHalfOpenErrorStream) {
   // Create a stream that will work on byte range "100-".
   const int kErrorStreamOffset = 100;
+
   auto error_stream = CreateSourceStream(
       kErrorStreamOffset, download::DownloadSaveInfo::kLengthFullContent);
   error_stream->set_finished(true);
@@ -287,8 +285,9 @@ TEST_P(ParallelDownloadUtilsRecoverErrorTest,
 
   // Half open finished preceding stream with error, should be treated as
   // failed.
-  preceding_stream->OnResponseCompleted(
-      download::DOWNLOAD_INTERRUPT_REASON_FILE_NO_SPACE);
+  EXPECT_CALL(*input_stream_, GetCompletionStatus())
+      .WillRepeatedly(
+          Return(download::DOWNLOAD_INTERRUPT_REASON_FILE_NO_SPACE));
   EXPECT_FALSE(CanRecoverFromError(error_stream.get(), preceding_stream.get()));
 
   // Even if it has written some data.
@@ -308,8 +307,9 @@ TEST_P(ParallelDownloadUtilsRecoverErrorTest,
 
   // Inject an error results in failure, even if data written exceeds the first
   // byte of error stream.
-  preceding_stream->OnResponseCompleted(
-      download::DOWNLOAD_INTERRUPT_REASON_FILE_NO_SPACE);
+  EXPECT_CALL(*input_stream_, GetCompletionStatus())
+      .WillRepeatedly(
+          Return(download::DOWNLOAD_INTERRUPT_REASON_FILE_NO_SPACE));
   preceding_stream->OnWriteBytesToDisk(1000u);
   EXPECT_FALSE(CanRecoverFromError(error_stream.get(), preceding_stream.get()));
 
@@ -383,8 +383,9 @@ TEST_P(ParallelDownloadUtilsRecoverErrorTest,
 
   // Even if inject an error, since data written has cover the upper bound of
   // the error stream, it should succeed.
-  preceding_stream->OnResponseCompleted(
-      download::DOWNLOAD_INTERRUPT_REASON_FILE_NO_SPACE);
+  EXPECT_CALL(*input_stream_, GetCompletionStatus())
+      .WillRepeatedly(
+          Return(download::DOWNLOAD_INTERRUPT_REASON_FILE_NO_SPACE));
   EXPECT_TRUE(CanRecoverFromError(error_stream.get(), preceding_stream.get()));
 
   // Preceding stream that never download data won't recover the error stream.

content/test/BUILD.gn

@@ -50,6 +50,8 @@ jumbo_static_library("test_support") {
     "../browser/download/mock_download_item_impl.h",
     "../browser/download/mock_download_job.cc",
     "../browser/download/mock_download_job.h",
+    "../browser/download/mock_input_stream.cc",
+    "../browser/download/mock_input_stream.h",
     "../browser/media/session/mock_media_session_observer.cc",
     "../browser/media/session/mock_media_session_observer.h",
     "../browser/service_worker/embedded_worker_test_helper.cc",