[Offline pages] Refactor URL conversions from TabAndroid

chrome/browser/android/offline_pages/offline_page_utils_unittest.cc

+// Copyright 2015 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/offline_page_utils.h"
+
+#include "base/callback.h"
+#include "base/command_line.h"
+#include "base/files/file.h"
+#include "base/files/file_path.h"
+#include "base/files/file_util.h"
+#include "base/test/test_simple_task_runner.h"
+#include "base/thread_task_runner_handle.h"
+#include "chrome/browser/android/offline_pages/offline_page_model_factory.h"
+#include "chrome/browser/android/offline_pages/test_offline_page_model_builder.h"
+#include "chrome/common/chrome_constants.h"
+#include "chrome/test/base/testing_profile.h"
+#include "components/offline_pages/offline_page_model.h"
+#include "components/offline_pages/offline_page_switches.h"
+#include "components/offline_pages/offline_page_test_store.h"
+#include "net/base/filename_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
+
+namespace offline_pages {
+namespace android {
+
+namespace {

[jianli, 2015/12/15 01:45:24]

nit: empty line

[fgorski, 2015/12/15 16:33:02]

Done.

+const char kTestPage1Url[] = "http://test.org/page1";
+const char kTestPage2Url[] = "http://test.org/page2";
+const char kTestPage3Url[] = "http://test.org/page3";
+int64 kTestPage1BookmarkId = 1234;
+int64 kTestPage2BookmarkId = 5678;
+const base::FilePath::CharType kTestPage1FilePath[] =
+    FILE_PATH_LITERAL("test_page1.mhtml");
+const base::FilePath::CharType kTestPage2FilePath[] =
+    FILE_PATH_LITERAL("test_page2.mhtml");
+// const base::FilePath::CharType kTestPage3FilePath[] =
+//    FILE_PATH_LITERAL("/offline_pages/test_org_page3.mhtml");
+int64 kTestPage1FileSize = 654321;
+int64 kTestPage2FileSize = 765432;
+
+}  // namespace
+
+class OfflinePageUtilsBasicTest : public testing::Test {
+ public:
+  OfflinePageUtilsBasicTest();
+  ~OfflinePageUtilsBasicTest() override;
+
+  void SetUp() override;
+
+  void FactorySetup();
+  void DisableOfflinePagesFeature();
+  void EnableOfflinePagesFeature();
+  void PumpLoop();
+
+  TestingProfile* profile() { return &profile_; }
+
+ private:
+  base::FilePath CreateArchiveFile(const base::FilePath& file_name);
+
+  scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
+  base::ThreadTaskRunnerHandle task_runner_handle_;
+
+  TestingProfile profile_;
+};
+
+class OfflinePageUtilsTest : public OfflinePageUtilsBasicTest {

[jianli, 2015/12/15 01:45:24]

This class should be grouped with all its tests. 

[fgorski, 2015/12/15 16:33:02]

Done.

+ public:
+  ~OfflinePageUtilsTest() override;
+
+  void SetUp() override;
+
+  void PreloadStore();
+
+  OfflinePageTestStore* store() { return store_; }
+  OfflinePageModel* model() { return model_; }
+
+  const base::FilePath& test_file_path_1() { return test_file_path_1_; }
+  const base::FilePath& test_file_path_2() { return test_file_path_2_; }
+  const base::FilePath& test_file_path_3() { return test_file_path_3_; }
+
+ private:
+  base::FilePath CreateArchiveFile(const base::FilePath& file_name);
+
+  OfflinePageModel* model_;
+  OfflinePageTestStore* store_;
+
+  base::FilePath test_file_path_1_;
+  base::FilePath test_file_path_2_;
+  base::FilePath test_file_path_3_;
+};
+
+OfflinePageUtilsBasicTest::OfflinePageUtilsBasicTest()
+    : task_runner_(new base::TestSimpleTaskRunner),
+      task_runner_handle_(task_runner_) {}
+
+OfflinePageUtilsBasicTest::~OfflinePageUtilsBasicTest() {}
+
+void OfflinePageUtilsBasicTest::SetUp() {
+  FactorySetup();
+  PumpLoop();
+}
+
+void OfflinePageUtilsBasicTest::FactorySetup() {
+  OfflinePageModelFactory::GetInstance()->SetTestingFactoryAndUse(
+      &profile_, BuildTestOfflinePageModel);
+}
+
+void OfflinePageUtilsBasicTest::DisableOfflinePagesFeature() {
+  base::CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kDisableOfflinePages);
+}
+
+void OfflinePageUtilsBasicTest::EnableOfflinePagesFeature() {
+  base::CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kEnableOfflinePages);
+}
+
+void OfflinePageUtilsBasicTest::PumpLoop() {
+  task_runner_->RunUntilIdle();
+}
+
+OfflinePageUtilsTest::~OfflinePageUtilsTest() {}
+
+void OfflinePageUtilsTest::SetUp() {
+  EnableOfflinePagesFeature();
+
+  base::FilePath archives_dir =
+      profile()->GetPath().Append(chrome::kOfflinePageArchviesDirname);
+  base::CreateDirectory(archives_dir);
+  test_file_path_1_ = CreateArchiveFile(base::FilePath(kTestPage1FilePath));
+  test_file_path_2_ = CreateArchiveFile(base::FilePath(kTestPage2FilePath));
+  test_file_path_3_ =
+      archives_dir.Append(FILE_PATH_LITERAL("missing_file.mhtml"));
+
+  FactorySetup();
+  model_ = OfflinePageModelFactory::GetForBrowserContext(profile());
+  store_ = static_cast<OfflinePageTestStore*>(model()->GetStoreForTesting());
+  // Make sure the store contains the right offline pages before the load
+  // happens.
+  PreloadStore();

[jianli, 2015/12/15 01:45:24]

We're really not interested in testing loading logic. So the simplest thing to
do is to just create the empty store and call AddOrUpdateOfflinePage a couple
times.

[fgorski, 2015/12/15 16:33:02]

Moved the offline page creation related code to a separate method and removed
setstorestate.

+  // Load from store happens here.
+  PumpLoop();
+}
+
+void OfflinePageUtilsTest::PreloadStore() {
+  std::vector<OfflinePageItem> offline_pages;
+  offline_pages.push_back(
+      OfflinePageItem(GURL(kTestPage1Url), kTestPage1BookmarkId,
+                      test_file_path_1(), kTestPage1FileSize));
+  offline_pages.push_back(
+      OfflinePageItem(GURL(kTestPage2Url), kTestPage2BookmarkId,
+                      test_file_path_2(), kTestPage2FileSize));
+  store()->SetStoreState(offline_pages);
+}
+
+base::FilePath OfflinePageUtilsTest::CreateArchiveFile(
+    const base::FilePath& file_name) {
+  base::FilePath archives_dir =
+      profile()->GetPath().Append(chrome::kOfflinePageArchviesDirname);
+  base::FilePath file_path(archives_dir.Append(file_name));
+  base::File file(file_path, base::File::FLAG_OPEN_ALWAYS);
+  return file_path;
+}
+
+// Simple test for checking if offline page has pages when the feature is
+// disabled.
+TEST_F(OfflinePageUtilsBasicTest, HasOfflinePagesDisabled) {
+  DisableOfflinePagesFeature();

[jianli, 2015/12/15 01:45:24]

Adding command line switch should normally be done in SetUp stage, instead of
after that.

[fgorski, 2015/12/15 16:33:02]

Notice that I explicitly extracted the methods and a whole class to make
exception to that rule.

+  EXPECT_FALSE(HasOfflinePages(profile()));
+}
+
+// Simple test for offline page model having none pages loaded.
+TEST_F(OfflinePageUtilsBasicTest, HasOfflinePagesEmpty) {
+  EnableOfflinePagesFeature();
+  EXPECT_FALSE(HasOfflinePages(profile()));

[jianli, 2015/12/15 01:45:24]

What's the difference between HasOfflinePagesDisabled and HasOfflinePagesEmpty?

[fgorski, 2015/12/15 16:33:02]

Store is empty in the latter case and the feature is disabled in the former.

[jianli, 2015/12/15 23:53:34]

Both tests only check if the store is empty. Thus we couldn't tell if enabling
or disabling the feature make any difference.

+}
+
+// Simple test for offline page model having any pages loaded.
+TEST_F(OfflinePageUtilsTest, HasOfflinePages) {
+  EXPECT_TRUE(HasOfflinePages(profile()));
+}
+
+TEST_F(OfflinePageUtilsTest, MightBeOfflineURL) {
+  // URL is invalid.
+  EXPECT_FALSE(MightBeOfflineURL(GURL("/test.mhtml")));
+  // Scheme is not file.
+  EXPECT_FALSE(MightBeOfflineURL(GURL("http://test.com/")));
+  // Does not end with .mhtml.
+  EXPECT_FALSE(MightBeOfflineURL(GURL("file:///test.txt")));
+  // Might still be an offline page.
+  EXPECT_TRUE(MightBeOfflineURL(GURL("file:///test.mhtml")));
+}
+
+TEST_F(OfflinePageUtilsTest, GetOfflineURLByOnlineURL) {
+  EXPECT_EQ(net::FilePathToFileURL(test_file_path_1()),
+            GetOfflineURLByOnlineURL(profile(), GURL(kTestPage1Url)));
+  EXPECT_EQ(net::FilePathToFileURL(test_file_path_2()),
+            GetOfflineURLByOnlineURL(profile(), GURL(kTestPage2Url)));
+  EXPECT_EQ(GURL(), GetOfflineURLByOnlineURL(profile(), GURL(kTestPage3Url)));
+}
+
+TEST_F(OfflinePageUtilsTest, GetOnlineURLByOfflineURL) {
+  EXPECT_EQ(GURL(kTestPage1Url),
+            GetOnlineURLByOfflineURL(
+                profile(), net::FilePathToFileURL(test_file_path_1())));
+  EXPECT_EQ(GURL(kTestPage2Url),
+            GetOnlineURLByOfflineURL(
+                profile(), net::FilePathToFileURL(test_file_path_2())));
+  EXPECT_EQ(GURL(), GetOfflineURLByOnlineURL(
+                        profile(), net::FilePathToFileURL(test_file_path_3())));
+}
+
+TEST_F(OfflinePageUtilsTest, GetBookmarkIdByOfflineURL) {
+  EXPECT_EQ(kTestPage1BookmarkId,
+            GetBookmarkIdByOfflineURL(
+                profile(), net::FilePathToFileURL(test_file_path_1())));
+  EXPECT_EQ(kTestPage2BookmarkId,
+            GetBookmarkIdByOfflineURL(
+                profile(), net::FilePathToFileURL(test_file_path_2())));
+  EXPECT_EQ(-1, GetBookmarkIdByOfflineURL(
+                    profile(), net::FilePathToFileURL(test_file_path_3())));
+}
+
+TEST_F(OfflinePageUtilsTest, IsOfflinePage) {
+  EXPECT_TRUE(
+      IsOfflinePage(profile(), net::FilePathToFileURL(test_file_path_1())));
+  EXPECT_TRUE(
+      IsOfflinePage(profile(), net::FilePathToFileURL(test_file_path_2())));
+  EXPECT_FALSE(
+      IsOfflinePage(profile(), net::FilePathToFileURL(test_file_path_3())));
+  EXPECT_FALSE(IsOfflinePage(profile(), GURL(kTestPage1Url)));
+  EXPECT_FALSE(IsOfflinePage(profile(), GURL(kTestPage2Url)));
+}
+
+TEST_F(OfflinePageUtilsTest, HasOfflinePageForOnlineURL) {
+  EXPECT_TRUE(HasOfflinePageForOnlineURL(profile(), GURL(kTestPage1Url)));
+  EXPECT_TRUE(HasOfflinePageForOnlineURL(profile(), GURL(kTestPage2Url)));
+  EXPECT_FALSE(HasOfflinePageForOnlineURL(profile(), GURL(kTestPage3Url)));
+}
+
+}  // namespace android
+}  // namespace offline_pages