Adds unit tests for Offliner SavePage implementation.

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 45267eddc5a7e63e4424ffb594d9047ae4ddf0f0..28e0b4b5233b09941d39cf180f9d33ec0c79dc37 100644
--- a/chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc
+++ b/chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc
@@ -10,9 +10,11 @@
 #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 {
@@ -20,6 +22,7 @@ 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.
@@ -27,6 +30,7 @@ class MockPrerenderingLoader : public PrerenderingLoader {
  public:
   explicit MockPrerenderingLoader(content::BrowserContext* browser_context)
       : PrerenderingLoader(browser_context),
+        can_prerender_(true),
         mock_loading_(false),
         mock_loaded_(false) {}
   ~MockPrerenderingLoader() override {}
@@ -41,6 +45,8 @@ class MockPrerenderingLoader : public PrerenderingLoader {
     mock_loading_ = false;
     mock_loaded_ = false;
   }
+
+  bool CanPrerender() override { return can_prerender_; }
   bool IsIdle() override { return !mock_loading_ && !mock_loaded_; }
   bool IsLoaded() override { return mock_loaded_; }
 
@@ -54,11 +60,34 @@ class MockPrerenderingLoader : public PrerenderingLoader {
                    nullptr /* web_contents */));
   }
 
+  void CallbackForLoaded() {
+    DCHECK(mock_loading_);
+    mock_loading_ = false;

[fgorski, 2016/05/27 18:43:11]

Just a consideration:
* Might mock_loading_ and mock_loaded_ a a tri-state enum really?
* Is there ever a case when they are both true?

[dougarnett, 2016/05/27 20:39:18]

True - was hoping two bools might be simpler to see here that the state_ and
enum values which are currently private (but other way to go would be to make
them protected and use here). 

No longer exposing these outside of the mock which should help with any
confusion in test case assertions anyway.

+    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 */));
+  }
+
+  void DisablePrerendering() { can_prerender_ = false; }
   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 can_prerender_;
   bool mock_loading_;
   bool mock_loaded_;
   LoadPageCallback load_page_callback_;
@@ -66,6 +95,47 @@ class MockPrerenderingLoader : public PrerenderingLoader {
   DISALLOW_COPY_AND_ASSIGN(MockPrerenderingLoader);
 };
 
+// TODO(dougarnett): Remove this subclass once OfflinePageModel is split out

[Pete Williamson, 2016/05/27 18:32:09]

I'd prefer to delay checking in this changelist until the new interface has
landed and we can remove this inheritance.

[dougarnett, 2016/05/27 20:39:18]

I do too - hopefully that review can land soon. I'm slightly concerned its scope
might get expanded to cover more cleanups though.

[Pete Williamson, 2016/05/27 21:06:46]

OK.  Still waiting for this one.

[dougarnett, 2016/06/01 17:51:10]

Done.

+// as an interface that can be mocked (so can mock SavePage() call with it).
+class TestPrerenderingOffliner : public PrerenderingOffliner {
+ public:
+  TestPrerenderingOffliner()
+      : PrerenderingOffliner(nullptr, nullptr, nullptr), mock_saving_(false) {}
+
+  ~TestPrerenderingOffliner() override {}
+
+  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() {

[fgorski, 2016/05/27 18:43:11]

Can you consider this to be mirror for the  method above. E.g.
CallbackForSaveSucceeded()

[dougarnett, 2016/05/27 20:39:18]

Went with the later Complete* naming suggested below

+    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_; }

[fgorski, 2016/05/27 18:43:11]

I believe this method can be const.

[dougarnett, 2016/05/27 20:39:18]

Done.

+
+ protected:
+  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 mock_saving_;
+  SavePageCallback save_page_callback_;
+};
+
 void PumpLoop() {
   base::RunLoop().RunUntilIdle();
 }
@@ -79,7 +149,7 @@ 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));
@@ -87,6 +157,7 @@ class PrerenderingOfflinerTest : public testing::Test {
 
   bool loading() const { return loader_->mock_loading(); }
   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_; }
@@ -96,7 +167,7 @@ class PrerenderingOfflinerTest : public testing::Test {
                     Offliner::RequestStatus status);
 
   content::TestBrowserThreadBundle thread_bundle_;
-  std::unique_ptr<PrerenderingOffliner> offliner_;
+  std::unique_ptr<TestPrerenderingOffliner> offliner_;
   // Not owned.
   MockPrerenderingLoader* loader_;
   bool callback_called_;
@@ -113,7 +184,7 @@ PrerenderingOfflinerTest::PrerenderingOfflinerTest()
 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();
@@ -127,6 +198,21 @@ void PrerenderingOfflinerTest::OnCompletion(const SavePageRequest& request,
   request_status_ = status;
 }
 
+TEST_F(PrerenderingOfflinerTest, LoadAndSaveBadUrl) {
+  base::Time creation_time = base::Time::Now();
+  SavePageRequest request(kRequestId, kFileUrl, kClientId, creation_time);
+  EXPECT_FALSE(offliner()->LoadAndSave(request, callback()));
+  EXPECT_FALSE(loading());
+}
+
+TEST_F(PrerenderingOfflinerTest, LoadAndSavePrerenderingDisabled) {
+  base::Time creation_time = base::Time::Now();
+  SavePageRequest request(kRequestId, kHttpUrl, kClientId, creation_time);
+  loader()->DisablePrerendering();
+  EXPECT_FALSE(offliner()->LoadAndSave(request, callback()));
+  EXPECT_FALSE(loading());

[Pete Williamson, 2016/05/27 18:32:09]

Which loading() function is being called here, the one on the test or the one on
the base class?  I didn't see one on the test, and we probably shouldn't be
calling the base class this way, instead we should be getting a pointer to it
from the test class.  We'll have to make that change when we move towards an
interface, might as well make it now.  Same comment for saving().

[dougarnett, 2016/05/27 20:39:18]

Reworked to use IsIdle() and IsLoaded() methods on Loader instead.

[Pete Williamson, 2016/05/27 21:06:46]

Acknowledged.

+}
+
 TEST_F(PrerenderingOfflinerTest, LoadAndSaveLoadStartedButFails) {
   base::Time creation_time = base::Time::Now();
   SavePageRequest request(kRequestId, kHttpUrl, kClientId, creation_time);
@@ -140,6 +226,7 @@ TEST_F(PrerenderingOfflinerTest, LoadAndSaveLoadStartedButFails) {
   EXPECT_EQ(Offliner::RequestStatus::FAILED, request_status());
   EXPECT_FALSE(loading());
   EXPECT_FALSE(loaded());
+  EXPECT_FALSE(saving());
 }
 
 TEST_F(PrerenderingOfflinerTest, CancelWhenLoading) {
@@ -152,4 +239,93 @@ TEST_F(PrerenderingOfflinerTest, CancelWhenLoading) {
   EXPECT_FALSE(loading());
 }
 
+TEST_F(PrerenderingOfflinerTest, CancelWhenLoaded) {
+  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();

[fgorski, 2016/05/27 18:43:11]

I am thinking whether there is a better name here.
How does loader()->CompleteLoad[ing]WithSuccess();  // or something like that
sound?

This would apply to saving as well.

[dougarnett, 2016/05/27 20:39:18]

Done - trying new naming scheme

+  PumpLoop();
+  EXPECT_FALSE(callback_called());

[Pete Williamson, 2016/05/27 18:32:09]

I'm a bit confused that callback_called() is returning false given that you just
called CallbackForLoaded().  Perhaps it is a different callback, and naming the
callback_called() function to show which callback would help.  Maybe
"save_callback_called()"?

[dougarnett, 2016/05/27 20:39:18]

changed to completion_callback_called()

[Pete Williamson, 2016/05/27 21:06:46]

Acknowledged.

+  EXPECT_TRUE(loaded());
+  EXPECT_TRUE(saving());
+
+  offliner()->Cancel();
+  PumpLoop();
+  EXPECT_FALSE(callback_called());
+  EXPECT_FALSE(loaded());
+  EXPECT_TRUE(saving());

[fgorski, 2016/05/27 18:43:11]

Could you add a comment and explain why saving is still in progress after
cancel?

[dougarnett, 2016/05/27 20:39:18]

Done.

+
+  // Subsequent save callback causes no harm (no crash and no callback).
+  offliner()->CallbackForSaveFailed();
+  PumpLoop();
+  EXPECT_FALSE(callback_called());
+  EXPECT_FALSE(loaded());
+  EXPECT_FALSE(saving());
+}
+
+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(loaded());
+  EXPECT_FALSE(saving());
+}
+
+TEST_F(PrerenderingOfflinerTest, LoadAndSaveLoadedAndSaved) {

[fgorski, 2016/05/27 18:43:11]

How about: LoadAndSaveSuccessful

[dougarnett, 2016/05/27 20:39:18]

Done.

+  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(loaded());
+  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());

[fgorski, 2016/05/27 18:43:11]

Is there an assertion related to saving missing here?

[dougarnett, 2016/05/27 20:39:18]

Done.

+}
+
 }  // namespace offline_pages