Adds PrerenderingOffliner implementation for handling Loader callbacks and then performing SavePage…

chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc

Index: chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc
diff --git a/chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc b/chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc
index 71f66b6c277ebd73af90c9c51f9ba2d4057a7671..f83ca8f61287eb36a07292d7a06f650b58030841 100644
--- a/chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc
+++ b/chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc
@@ -7,10 +7,14 @@
 #include <memory>
 
 #include "base/bind.h"
+#include "base/run_loop.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/android/offline_pages/prerendering_loader.h"
+#include "chrome/test/base/testing_profile.h"
 #include "components/offline_pages/background/offliner.h"
 #include "components/offline_pages/background/save_page_request.h"
 #include "content/public/test/test_browser_thread_bundle.h"
+#include "content/public/test/web_contents_tester.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace offline_pages {
@@ -18,7 +22,6 @@ namespace offline_pages {
 namespace {
 const int64_t kRequestId = 7;
 const GURL kHttpUrl("http://tunafish.com");
-const GURL kFileUrl("file://sailfish.png");
 const ClientId kClientId("AsyncLoading", "88");
 
 // Mock Loader for testing the Offliner calls.
@@ -32,6 +35,7 @@ class MockPrerenderingLoader : public PrerenderingLoader {
 
   bool LoadPage(const GURL& url, const LoadPageCallback& callback) override {
     mock_loading_ = true;
+    load_page_callback_ = callback;
     return mock_loading_;
   }
 
@@ -42,15 +46,98 @@ class MockPrerenderingLoader : public PrerenderingLoader {
   bool IsIdle() override { return !mock_loading_ && !mock_loaded_; }
   bool IsLoaded() override { return mock_loaded_; }
 
+  void CallbackForLoadFailed() {
+    DCHECK(mock_loading_);
+    mock_loading_ = false;
+    mock_loaded_ = false;
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(load_page_callback_, Offliner::RequestStatus::FAILED,
+                   nullptr /* web_contents */));
+  }
+
+  void CallbackForLoaded() {
+    DCHECK(mock_loading_);
+    mock_loading_ = false;
+    mock_loaded_ = true;
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(load_page_callback_, Offliner::RequestStatus::LOADED,
+                   content::WebContentsTester::CreateTestWebContents(
+                       new TestingProfile(), NULL)));
+  }
+
+  void CallbackForLoadCanceledAfterLoaded() {
+    DCHECK(mock_loaded_);
+    mock_loading_ = false;
+    mock_loaded_ = false;
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(load_page_callback_, Offliner::RequestStatus::CANCELED,
+                   nullptr /* web_contents */));
+  }
+
   bool mock_loading() const { return mock_loading_; }
+  bool mock_loaded() const { return mock_loaded_; }
+  const LoadPageCallback& load_page_callback() { return load_page_callback_; }
 
  private:
   bool mock_loading_;
   bool mock_loaded_;
+  LoadPageCallback load_page_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(MockPrerenderingLoader);
 };
 
+class TestPrerenderingOffliner : public PrerenderingOffliner {

[Pete Williamson, 2016/05/24 00:40:07]

This is not the testing strategy I normally see.  I normally see the real class
being created, and passing in factories to create dependencies (or passing in
the dependencies themselves).  I dislike that this approach is modifying the
class under test while testing, and I am concerned that it might make tests pass
that shouldn't.

[dougarnett, 2016/05/24 19:07:27]

I'm not really happy with it either (I have TODO's with their declarations). I
was surprised the OfflinePageModel is not an interface and wonder if we should
make it one (but not sure about tackling that in this CL). The other approach I
considered was defining some other interface (Snapshotter, PageSaver) that is
mockable and the actual implementation uses OPM.

[Pete Williamson, 2016/05/24 21:18:42]

We discussed breaking out the unit tests to a new changelist, and seeing what
our other options are for solving this, including making OfflinePageModel
mockable, or making an interface for it, or inheriting a test OfflinePageModel
from it.

[dougarnett, 2016/05/25 18:04:19]

Acknowledged.

+ public:
+  TestPrerenderingOffliner()
+      : PrerenderingOffliner(nullptr, nullptr, nullptr),
+        can_save_(true),
+        mock_saving_(false) {}
+
+  ~TestPrerenderingOffliner() override {}
+
+  void SetCanSaveFalse() { can_save_ = false; }
+
+  void CallbackForSaveFailed() {
+    DCHECK(mock_saving_);
+    mock_saving_ = false;
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::Bind(save_page_callback_,
+                              SavePageResult::ARCHIVE_CREATION_FAILED, 0));
+  }
+
+  void CallbackForSaved() {
+    DCHECK(mock_saving_);
+    mock_saving_ = false;
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(save_page_callback_, SavePageResult::SUCCESS, 123456));
+  }
+
+  bool mock_saving() { return mock_saving_; }
+
+ protected:
+  bool CanSavePage(const GURL& url) override { return can_save_; }
+  void SavePage(const GURL& url,
+                const ClientId& client_id,
+                std::unique_ptr<OfflinePageArchiver> archiver,
+                const SavePageCallback& callback) override {
+    mock_saving_ = true;
+    save_page_callback_ = callback;
+  }
+
+ private:
+  bool can_save_;
+  bool mock_saving_;
+  SavePageCallback save_page_callback_;
+};
+
+void PumpLoop() {
+  base::RunLoop().RunUntilIdle();
+}
+
 }  // namespace
 
 class PrerenderingOfflinerTest : public testing::Test {
@@ -60,35 +147,42 @@ class PrerenderingOfflinerTest : public testing::Test {
 
   void SetUp() override;
 
-  PrerenderingOffliner* offliner() const { return offliner_.get(); }
+  TestPrerenderingOffliner* offliner() const { return offliner_.get(); }
   Offliner::CompletionCallback const callback() {
     return base::Bind(&PrerenderingOfflinerTest::OnCompletion,
                       base::Unretained(this));
   }
 
   bool loading() const { return loader_->mock_loading(); }
-  Offliner::RequestStatus completion_status() { return completion_status_; }
+  bool loaded() const { return loader_->mock_loaded(); }
+  bool saving() const { return offliner_->mock_saving(); }
+  MockPrerenderingLoader* loader() { return loader_; }
+  bool callback_called() { return callback_called_; }
+  Offliner::RequestStatus request_status() { return request_status_; }
 
  private:
   void OnCompletion(const SavePageRequest& request,
                     Offliner::RequestStatus status);
 
   content::TestBrowserThreadBundle thread_bundle_;
-  std::unique_ptr<PrerenderingOffliner> offliner_;
+  std::unique_ptr<TestPrerenderingOffliner> offliner_;
   // Not owned.
   MockPrerenderingLoader* loader_;
-  Offliner::RequestStatus completion_status_;
+  bool callback_called_;
+  Offliner::RequestStatus request_status_;
 
   DISALLOW_COPY_AND_ASSIGN(PrerenderingOfflinerTest);
 };
 
 PrerenderingOfflinerTest::PrerenderingOfflinerTest()
-    : thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP) {}
+    : thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP),
+      callback_called_(false),
+      request_status_(Offliner::RequestStatus::UNKNOWN) {}
 
 PrerenderingOfflinerTest::~PrerenderingOfflinerTest() {}
 
 void PrerenderingOfflinerTest::SetUp() {
-  offliner_.reset(new PrerenderingOffliner(nullptr, nullptr, nullptr));
+  offliner_.reset(new TestPrerenderingOffliner());
   std::unique_ptr<MockPrerenderingLoader> mock_loader(
       new MockPrerenderingLoader(nullptr));
   loader_ = mock_loader.get();
@@ -97,26 +191,34 @@ void PrerenderingOfflinerTest::SetUp() {
 
 void PrerenderingOfflinerTest::OnCompletion(const SavePageRequest& request,
                                             Offliner::RequestStatus status) {
-  completion_status_ = status;
+  DCHECK(!callback_called_);  // Only expect single callback per request.
+  callback_called_ = true;
+  request_status_ = status;
 }
 
-// Tests initiate loading.
-TEST_F(PrerenderingOfflinerTest, LoadAndSaveBadUrl) {
+TEST_F(PrerenderingOfflinerTest, LoadAndSaveCanSaveFalse) {
   base::Time creation_time = base::Time::Now();
-  SavePageRequest request(kRequestId, kFileUrl, kClientId, creation_time);
+  SavePageRequest request(kRequestId, kHttpUrl, kClientId, creation_time);
+  offliner()->SetCanSaveFalse();
   EXPECT_FALSE(offliner()->LoadAndSave(request, callback()));
   EXPECT_FALSE(loading());
 }
 
-// Tests initiate loading.
-TEST_F(PrerenderingOfflinerTest, LoadAndSaveStartsLoading) {
+TEST_F(PrerenderingOfflinerTest, LoadAndSaveLoadStartedButFails) {
   base::Time creation_time = base::Time::Now();
   SavePageRequest request(kRequestId, kHttpUrl, kClientId, creation_time);
   EXPECT_TRUE(offliner()->LoadAndSave(request, callback()));
   EXPECT_TRUE(loading());
+  EXPECT_EQ(Offliner::RequestStatus::UNKNOWN, request_status());
+
+  loader()->CallbackForLoadFailed();
+  PumpLoop();
+  EXPECT_TRUE(callback_called());
+  EXPECT_EQ(Offliner::RequestStatus::FAILED, request_status());
+  EXPECT_FALSE(loading());
+  EXPECT_FALSE(saving());
 }
 
-// Tests cancels loading.
 TEST_F(PrerenderingOfflinerTest, CancelLoading) {
   base::Time creation_time = base::Time::Now();
   SavePageRequest request(kRequestId, kHttpUrl, kClientId, creation_time);
@@ -127,4 +229,64 @@ TEST_F(PrerenderingOfflinerTest, CancelLoading) {
   EXPECT_FALSE(loading());
 }
 
+TEST_F(PrerenderingOfflinerTest, LoadAndSaveLoadedButSaveFails) {
+  base::Time creation_time = base::Time::Now();
+  SavePageRequest request(kRequestId, kHttpUrl, kClientId, creation_time);
+  EXPECT_TRUE(offliner()->LoadAndSave(request, callback()));
+  EXPECT_TRUE(loading());
+  EXPECT_EQ(Offliner::RequestStatus::UNKNOWN, request_status());
+
+  loader()->CallbackForLoaded();
+  PumpLoop();
+  EXPECT_FALSE(callback_called());
+  EXPECT_TRUE(loaded());
+  EXPECT_TRUE(saving());
+
+  offliner()->CallbackForSaveFailed();
+  PumpLoop();
+  EXPECT_TRUE(callback_called());
+  EXPECT_EQ(Offliner::RequestStatus::FAILED_SAVE, request_status());
+  EXPECT_FALSE(saving());
+}
+
+TEST_F(PrerenderingOfflinerTest, LoadAndSaveLoadedAndSaved) {
+  base::Time creation_time = base::Time::Now();
+  SavePageRequest request(kRequestId, kHttpUrl, kClientId, creation_time);
+  EXPECT_TRUE(offliner()->LoadAndSave(request, callback()));
+  EXPECT_TRUE(loading());
+  EXPECT_EQ(Offliner::RequestStatus::UNKNOWN, request_status());
+
+  loader()->CallbackForLoaded();
+  PumpLoop();
+  EXPECT_FALSE(callback_called());
+  EXPECT_TRUE(loaded());
+  EXPECT_TRUE(saving());
+
+  offliner()->CallbackForSaved();
+  PumpLoop();
+  EXPECT_TRUE(callback_called());
+  EXPECT_EQ(Offliner::RequestStatus::SAVED, request_status());
+  EXPECT_FALSE(saving());
+}
+
+TEST_F(PrerenderingOfflinerTest, LoadAndSaveLoadedButThenCanceledFromLoader) {
+  base::Time creation_time = base::Time::Now();
+  SavePageRequest request(kRequestId, kHttpUrl, kClientId, creation_time);
+  EXPECT_TRUE(offliner()->LoadAndSave(request, callback()));
+  EXPECT_TRUE(loading());
+  EXPECT_EQ(Offliner::RequestStatus::UNKNOWN, request_status());
+
+  loader()->CallbackForLoaded();
+  PumpLoop();
+  EXPECT_FALSE(callback_called());
+  EXPECT_TRUE(loaded());
+  EXPECT_TRUE(saving());
+
+  loader()->CallbackForLoadCanceledAfterLoaded();
+  PumpLoop();
+  EXPECT_TRUE(callback_called());
+  EXPECT_EQ(Offliner::RequestStatus::CANCELED, request_status());
+  EXPECT_FALSE(loaded());
+}
+
 }  // namespace offline_pages