[Offline pages] Create offliner that uses background loader

chrome/browser/android/offline_pages/background_loader_offliner_unittest.cc

Index: chrome/browser/android/offline_pages/background_loader_offliner_unittest.cc
diff --git a/chrome/browser/android/offline_pages/background_loader_offliner_unittest.cc b/chrome/browser/android/offline_pages/background_loader_offliner_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..7c2a55484e368b94d00f92c26e5d82b46e938960
--- /dev/null
+++ b/chrome/browser/android/offline_pages/background_loader_offliner_unittest.cc
@@ -0,0 +1,236 @@
+// Copyright 2016 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 "chrome/browser/android/offline_pages/background_loader_offliner.h"
+
+#include "base/bind.h"
+#include "base/run_loop.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "chrome/test/base/testing_profile.h"
+#include "components/offline_pages/content/background_loader/background_loader_contents_stub.h"
+#include "components/offline_pages/core/background/offliner.h"
+#include "components/offline_pages/core/background/save_page_request.h"
+#include "components/offline_pages/core/stub_offline_page_model.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 {
+
+namespace {
+
+const int64_t kRequestId = 7;
+const GURL kHttpUrl("http://www.tunafish.com");
+const GURL kFileUrl("file://salmon.png");
+const ClientId kClientId("AsyncLoading", "88");
+const bool kUserRequested = true;
+
+// Mock OfflinePageModel for testing the SavePage calls
+class MockOfflinePageModel : public StubOfflinePageModel {

[Pete Williamson, 2016/12/10 01:52:54]

I wonder if this could be split out into a separate class, and re-used by the
prerendering offliner unit tests.  (Perhaps it was copied from there
originally?).

+ public:
+  MockOfflinePageModel() : mock_saving_(false) {}
+  ~MockOfflinePageModel() override {}
+
+  void SavePage(const SavePageParams& save_page_params,
+                std::unique_ptr<OfflinePageArchiver> archiver,
+                const SavePageCallback& callback) override {
+    mock_saving_ = true;
+    save_page_callback_ = callback;
+  }
+
+  void CompleteSavingAsArchiveCreationFailed() {
+    DCHECK(mock_saving_);
+    mock_saving_ = false;
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::Bind(save_page_callback_,
+                              SavePageResult::ARCHIVE_CREATION_FAILED, 0));
+  }
+
+  void CompleteSavingAsSuccess() {
+    DCHECK(mock_saving_);
+    mock_saving_ = false;
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(save_page_callback_, SavePageResult::SUCCESS, 123456));
+  }
+
+  bool mock_saving() const { return mock_saving_; }
+
+ private:
+  bool mock_saving_;
+  SavePageCallback save_page_callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(MockOfflinePageModel);
+};
+
+void PumpLoop() {
+  base::RunLoop().RunUntilIdle();
+}
+}  // namespace
+
+// A BackgroundLoader that we can run tests on.
+// Overrides the ResetState so we don't actually try to create any web contents.
+// This is a temporary solution to test core BackgroundLoaderOffliner
+// functionality until we straighten out assumptions made by RequestCoordinator
+// so that the ResetState method is no longer needed.
+class TestBackgroundLoaderOffliner : public BackgroundLoaderOffliner {
+ public:
+  explicit TestBackgroundLoaderOffliner(
+      content::BrowserContext* browser_context,
+      const OfflinerPolicy* policy,
+      OfflinePageModel* offline_page_model);
+  ~TestBackgroundLoaderOffliner() override;
+  content::WebContentsTester* web_contents() {
+    return content::WebContentsTester::For(stub_->web_contents());
+  }
+
+  bool is_loading() { return stub_->is_loading(); }
+
+ protected:
+  void ResetState() override;
+
+ private:
+  background_loader::BackgroundLoaderContentsStub* stub_;
+};
+
+TestBackgroundLoaderOffliner::TestBackgroundLoaderOffliner(
+    content::BrowserContext* browser_context,
+    const OfflinerPolicy* policy,
+    OfflinePageModel* offline_page_model)
+    : BackgroundLoaderOffliner(browser_context, policy, offline_page_model) {}
+
+TestBackgroundLoaderOffliner::~TestBackgroundLoaderOffliner() {}
+
+void TestBackgroundLoaderOffliner::ResetState() {
+  stub_ = new background_loader::BackgroundLoaderContentsStub(browser_context_);
+  loader_.reset(stub_);
+  content::WebContentsObserver::Observe(stub_->web_contents());
+}
+
+class BackgroundLoaderOfflinerTest : public testing::Test {
+ public:
+  BackgroundLoaderOfflinerTest();
+  ~BackgroundLoaderOfflinerTest() override;
+
+  void SetUp() override;
+
+  TestBackgroundLoaderOffliner* offliner() const { return offliner_.get(); }

[Pete Williamson, 2016/12/10 01:52:54]

Can this return the BackgroundLoaderOffliner instead?  If so, that will make
life easier when we get rid of the TestBackgroundLoaderOffliner.  It looks like
the call to reset is done on the "offliner" member variable, so maybe we don't
need to access any of the special test members from the unit tests themselves.

[chili, 2016/12/15 10:34:39]

The Test has methods to call is_loading() (returns whether a loading call has
been called) on the stub contents for testing purposes. I can imagine that going
away once we don't need to reset state, though it's still useful right now.

+  Offliner::CompletionCallback const callback() {
+    return base::Bind(&BackgroundLoaderOfflinerTest::OnCompletion,
+                      base::Unretained(this));
+  }
+  Profile* profile() { return &profile_; }
+  bool completion_callback_called() { return completion_callback_called_; }
+  Offliner::RequestStatus request_status() { return request_status_; }
+  bool SaveInProgress() const { return model_->mock_saving(); }
+  MockOfflinePageModel* model() const { return model_; }
+
+  void CompleteLoading() {
+    offliner()->web_contents()->TestSetIsLoading(false);
+  }
+
+ private:
+  void OnCompletion(const SavePageRequest& request,
+                    Offliner::RequestStatus status);
+  content::TestBrowserThreadBundle thread_bundle_;
+  TestingProfile profile_;
+  std::unique_ptr<TestBackgroundLoaderOffliner> offliner_;
+  MockOfflinePageModel* model_;
+  bool completion_callback_called_;
+  Offliner::RequestStatus request_status_;
+
+  DISALLOW_COPY_AND_ASSIGN(BackgroundLoaderOfflinerTest);
+};
+
+BackgroundLoaderOfflinerTest::BackgroundLoaderOfflinerTest()
+    : thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP),

[Pete Williamson, 2016/12/10 01:52:54]

Why use TestBrowserThreadBundle instead of ThreadTaskRunnerHandle?  (I'm not
sure of the difference, I just know that most of our tests seem to use the other
thread runner instead of a thread bundle.)

[dougarnett, 2016/12/12 19:24:17]

Not sure but I have faint recollection of needing to do something different to
make the Prerenderer code happy (ie, get past some thread DCHECKs) - but that
might only apply now to PrerenderingAdapter tests at this point.

[chili, 2016/12/15 10:34:39]

thread_task_runner_handle.cc fails with a "Must be called on Chrome_UIThread;
actually called on Unknown Thread" error when i used ThreadTaskRunnerHandle

+      completion_callback_called_(false),
+      request_status_(Offliner::RequestStatus::UNKNOWN) {}
+
+BackgroundLoaderOfflinerTest::~BackgroundLoaderOfflinerTest() {}
+
+void BackgroundLoaderOfflinerTest::SetUp() {
+  model_ = new MockOfflinePageModel();
+  offliner_.reset(new TestBackgroundLoaderOffliner(profile(), nullptr, model_));
+}
+
+void BackgroundLoaderOfflinerTest::OnCompletion(
+    const SavePageRequest& request,
+    Offliner::RequestStatus status) {
+  DCHECK(!completion_callback_called_);  // Expect 1 callback per request.
+  completion_callback_called_ = true;
+  request_status_ = status;
+}
+
+TEST_F(BackgroundLoaderOfflinerTest, CancelWhenLoading) {
+  base::Time creation_time = base::Time::Now();
+  SavePageRequest request(kRequestId, kHttpUrl, kClientId, creation_time,
+                          kUserRequested);
+  EXPECT_TRUE(offliner()->LoadAndSave(request, callback()));
+  EXPECT_TRUE(offliner()->is_loading());
+
+  offliner()->Cancel();
+  EXPECT_FALSE(offliner()->is_loading());
+}
+
+TEST_F(BackgroundLoaderOfflinerTest, CancelWhenLoaded) {
+  base::Time creation_time = base::Time::Now();
+  SavePageRequest request(kRequestId, kHttpUrl, kClientId, creation_time,
+                          kUserRequested);
+  EXPECT_TRUE(offliner()->LoadAndSave(request, callback()));
+  EXPECT_TRUE(offliner()->is_loading());
+
+  CompleteLoading();
+  PumpLoop();
+  EXPECT_FALSE(completion_callback_called());
+  // Save still in progress because does not support canceling.

[Pete Williamson, 2016/12/10 01:52:54]

So what happens on a call to cancel when save is in progress?  Is the saved file
deleted by the offliner after the save operation returns, or is that the
responsibility of the request coordinator to check for the file and remove it? 
(If so, let's add a TODO to the RC code.)

[dougarnett, 2016/12/12 19:24:17]

This is a hole. We should have some bug opened or TODO. Ideally, we would be
able to cancel the SavePage() operation but if not, then good question on how to
clean it up. Can we have Offliner clean it up? Might be more complicated if
Offliner gets destroyed (in future iteration). For RC to clean up, we need to
pass it the proposed_offline_id I expect. How bad is it if not cleaned up? Shows
up in Downloads?

[dougarnett, 2016/12/12 19:33:28]

Actually, what happens to WebContents when ResetState() destroys current loader?
Can archiver keep using that WebContents?

[chili, 2016/12/15 10:34:39]

This is a good point.  ResetState would delete the WebContents, and we could
have some potential crashing going on.  Added a safety guard to delete the
loader & WebContents after save if cancel() was called during the middle of save

+  EXPECT_TRUE(SaveInProgress());
+  offliner()->Cancel();
+
+  // Subsequent save callback cause no crash.
+  model()->CompleteSavingAsArchiveCreationFailed();
+  PumpLoop();
+  EXPECT_FALSE(completion_callback_called());
+  EXPECT_FALSE(SaveInProgress());
+}
+
+TEST_F(BackgroundLoaderOfflinerTest, LoadedButSaveFails) {
+  base::Time creation_time = base::Time::Now();
+  SavePageRequest request(kRequestId, kHttpUrl, kClientId, creation_time,
+                          kUserRequested);
+  EXPECT_TRUE(offliner()->LoadAndSave(request, callback()));
+  EXPECT_TRUE(offliner()->is_loading());
+
+  CompleteLoading();
+  PumpLoop();
+  model()->CompleteSavingAsArchiveCreationFailed();
+  PumpLoop();
+  EXPECT_TRUE(completion_callback_called());
+  EXPECT_EQ(Offliner::RequestStatus::SAVE_FAILED, request_status());
+  EXPECT_FALSE(offliner()->is_loading());
+  EXPECT_FALSE(SaveInProgress());
+}
+
+TEST_F(BackgroundLoaderOfflinerTest, LoadAndSaveSuccess) {
+  base::Time creation_time = base::Time::Now();
+  SavePageRequest request(kRequestId, kHttpUrl, kClientId, creation_time,
+                          kUserRequested);
+  EXPECT_TRUE(offliner()->LoadAndSave(request, callback()));
+  EXPECT_TRUE(offliner()->is_loading());
+  EXPECT_EQ(Offliner::RequestStatus::UNKNOWN, request_status());
+
+  CompleteLoading();
+  PumpLoop();
+  EXPECT_FALSE(completion_callback_called());
+  EXPECT_TRUE(SaveInProgress());
+
+  model()->CompleteSavingAsSuccess();
+  PumpLoop();
+  EXPECT_TRUE(completion_callback_called());
+  EXPECT_EQ(Offliner::RequestStatus::SAVED, request_status());
+  EXPECT_FALSE(offliner()->is_loading());
+  EXPECT_FALSE(SaveInProgress());
+}
+
+}  // namespace offline_pages