Use the selected download directory for Offline Pages.

When the "Download Location" flag is set, we show a dialog allowing
the user to pick which directory is used for downloads.  Offline Pages
should use the directory that the user picked.  This change will get
the directory from downloads, and use that when publishing downloaded
offline pages.

Bug: 758690
Change-Id: I74ba4a1fb2c99fb986ba2273fc4b11f8fad60ba9
Reviewed-on: https://chromium-review.googlesource.com/1069423Reviewed-by: Yafei Duan <romax@chromium.org>
Reviewed-by: Xing Liu <xingliu@chromium.org>
Commit-Queue: Peter Williamson <petewil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561587}

chrome/browser/BUILD.gn

@@ -3763,6 +3763,8 @@ jumbo_split_static_library("browser") {
     sources += [
       "offline_pages/background_loader_offliner.cc",
       "offline_pages/background_loader_offliner.h",
+      "offline_pages/download_archive_manager.cc",
+      "offline_pages/download_archive_manager.h",
       "offline_pages/downloads/resource_throttle.cc",
       "offline_pages/downloads/resource_throttle.h",
       "offline_pages/fresh_offline_content_observer.cc",

chrome/browser/offline_pages/android/offline_page_model_factory.cc

@@ -15,11 +15,11 @@
 #include "chrome/browser/download/download_prefs.h"
 #include "chrome/browser/offline_pages/android/cct_origin_observer.h"
 #include "chrome/browser/offline_pages/android/offline_pages_download_manager_bridge.h"
+#include "chrome/browser/offline_pages/download_archive_manager.h"
 #include "chrome/browser/offline_pages/fresh_offline_content_observer.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/chrome_constants.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
-#include "components/offline_pages/core/archive_manager.h"
 #include "components/offline_pages/core/model/offline_page_model_taskified.h"
 #include "components/offline_pages/core/offline_page_metadata_store_sql.h"
 
@@ -62,9 +62,10 @@ KeyedService* OfflinePageModelFactory::BuildServiceInstanceFor(
     temporary_archives_dir =
         temporary_archives_dir.Append(chrome::kOfflinePageArchivesDirname);
   }
-  std::unique_ptr<ArchiveManager> archive_manager(new ArchiveManager(
+  std::unique_ptr<ArchiveManager> archive_manager(new DownloadArchiveManager(
       temporary_archives_dir, persistent_archives_dir,
-      DownloadPrefs::GetDefaultDownloadDirectory(), background_task_runner));
+      DownloadPrefs::GetDefaultDownloadDirectory(), background_task_runner,
+      profile));
   auto clock = std::make_unique<base::DefaultClock>();
 
   std::unique_ptr<SystemDownloadManager> download_manager(

chrome/browser/offline_pages/download_archive_manager.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 <string>
+
+#include "chrome/browser/offline_pages/download_archive_manager.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/common/pref_names.h"
+#include "components/prefs/pref_service.h"
+
+namespace offline_pages {
+
+DownloadArchiveManager::DownloadArchiveManager(
+    const base::FilePath& temporary_archives_dir,
+    const base::FilePath& private_archives_dir,
+    const base::FilePath& public_archives_dir,
+    const scoped_refptr<base::SequencedTaskRunner>& task_runner,
+    Profile* profile)
+    : ArchiveManager(temporary_archives_dir,
+                     private_archives_dir,
+                     public_archives_dir,
+                     task_runner),
+      profile_(profile) {}
+
+DownloadArchiveManager::~DownloadArchiveManager() {}
+
+const base::FilePath& DownloadArchiveManager::GetPublicArchivesDir() {
+  if (profile_) {
+    // Use the preference set by the download location dialog, if present.
+    std::string directory_preference =
+        profile_->GetPrefs()->GetString(prefs::kDownloadDefaultDirectory);
+    if (!directory_preference.empty()) {
+      download_archives_dir_ = base::FilePath(directory_preference);
+      // Must set the member variable so the reference will outlive the
+      // funciton call.
+      return download_archives_dir_;
+    }
+  }
+
+  return ArchiveManager::GetPublicArchivesDir();
+}
+
+}  // namespace offline_pages

chrome/browser/offline_pages/download_archive_manager.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 CHROME_BROWSER_OFFLINE_PAGES_DOWNLOAD_ARCHIVE_MANAGER_H_
+#define CHROME_BROWSER_OFFLINE_PAGES_DOWNLOAD_ARCHIVE_MANAGER_H_
+
+#include "base/callback_forward.h"
+#include "base/files/file_path.h"
+#include "base/memory/ref_counted.h"
+#include "components/offline_pages/core/archive_manager.h"
+
+class Profile;
+
+namespace base {
+class SequencedTaskRunner;
+}  // namespace base
+
+namespace offline_pages {
+
+// Manages the directories used for OfflinePages.  Dynamically get the directory
+// to use for downloads from the Download system.
+class DownloadArchiveManager : public ArchiveManager {
+ public:
+  DownloadArchiveManager(
+      const base::FilePath& temporary_archives_dir,
+      const base::FilePath& private_archives_dir,
+      const base::FilePath& public_archives_dir,
+      const scoped_refptr<base::SequencedTaskRunner>& task_runner,
+      Profile* profile);
+  ~DownloadArchiveManager() override;
+
+  const base::FilePath& GetPublicArchivesDir() override;
+
+ private:
+  Profile* profile_;
+  base::FilePath download_archives_dir_;
+
+  DISALLOW_COPY_AND_ASSIGN(DownloadArchiveManager);
+};
+
+}  // namespace offline_pages
+
+#endif  // CHROME_BROWSER_OFFLINE_PAGES_DOWNLOAD_ARCHIVE_MANAGER_H_

chrome/browser/offline_pages/download_archive_manager_unittest.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 "chrome/browser/offline_pages/download_archive_manager.h"
+
+#include "base/macros.h"
+#include "chrome/common/pref_names.h"
+#include "chrome/test/base/testing_profile.h"
+#include "components/prefs/pref_service.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace offline_pages {
+
+namespace {
+const char* kPrivateDir = "/private/";
+const char* kTemporaryDir = "/temporary/";
+const char* kPublicDir = "/public/";
+const char* kChromePublicSdCardDir =
+    "/sd-card/1234-5678/Android/data/org.chromium.chrome/files/Download";
+}  // namespace
+
+class DownloadArchiveManagerTest : public testing::Test {
+ public:
+  DownloadArchiveManagerTest() = default;
+  ~DownloadArchiveManagerTest() override = default;
+
+  void SetUp() override;
+  void TearDown() override;
+  void PumpLoop();
+
+  TestingProfile* profile() { return &profile_; }
+  DownloadArchiveManager* archive_manager() { return archive_manager_.get(); }
+
+ private:
+  content::TestBrowserThreadBundle browser_thread_bundle_;
+  TestingProfile profile_;
+  std::unique_ptr<DownloadArchiveManager> archive_manager_;
+  DISALLOW_COPY_AND_ASSIGN(DownloadArchiveManagerTest);
+};
+
+void DownloadArchiveManagerTest::SetUp() {
+  // Set up preferences to point to kChromePublicSdCardDir.
+  profile()->GetPrefs()->SetString(prefs::kDownloadDefaultDirectory,
+                                   kChromePublicSdCardDir);
+
+  // Create a DownloadArchiveManager to use.
+  archive_manager_.reset(new DownloadArchiveManager(
+      base::FilePath(kTemporaryDir), base::FilePath(kPrivateDir),
+      base::FilePath(kPublicDir), base::ThreadTaskRunnerHandle::Get(),
+      profile()));
+}
+
+void DownloadArchiveManagerTest::TearDown() {
+  archive_manager_.release();
+}
+
+TEST_F(DownloadArchiveManagerTest, UseDownloadDirFromPreferences) {
+  base::FilePath download_dir = archive_manager()->GetPublicArchivesDir();
+  ASSERT_EQ(kChromePublicSdCardDir, download_dir.AsUTF8Unsafe());
+}
+
+TEST_F(DownloadArchiveManagerTest, NullProfile) {
+  DownloadArchiveManager download_archive_manager(
+      base::FilePath(kTemporaryDir), base::FilePath(kPrivateDir),
+      base::FilePath(kPublicDir), base::ThreadTaskRunnerHandle::Get(), nullptr);
+
+  base::FilePath download_dir = download_archive_manager.GetPublicArchivesDir();
+  ASSERT_EQ(kPublicDir, download_dir.AsUTF8Unsafe());
+}
+
+}  // namespace offline_pages

chrome/test/BUILD.gn

@@ -2834,6 +2834,7 @@ test("unit_tests") {
   if (enable_offline_pages) {
     sources += [
       "../browser/offline_pages/background_loader_offliner_unittest.cc",
+      "../browser/offline_pages/download_archive_manager_unittest.cc",
       "../browser/offline_pages/offline_page_mhtml_archiver_unittest.cc",
       "../browser/offline_pages/offline_page_request_handler_unittest.cc",
       "../browser/offline_pages/offline_page_tab_helper_unittest.cc",

components/offline_pages/core/archive_manager.cc

@@ -136,7 +136,7 @@ const base::FilePath& ArchiveManager::GetPrivateArchivesDir() const {
   return private_archives_dir_;
 }
 
-const base::FilePath& ArchiveManager::GetPublicArchivesDir() const {
+const base::FilePath& ArchiveManager::GetPublicArchivesDir() {
   return public_archives_dir_;
 }
 

components/offline_pages/core/archive_manager.h

@@ -64,7 +64,7 @@ class ArchiveManager {
   // Gets the archive directories.
   const base::FilePath& GetTemporaryArchivesDir() const;
   const base::FilePath& GetPrivateArchivesDir() const;
-  const base::FilePath& GetPublicArchivesDir() const;
+  virtual const base::FilePath& GetPublicArchivesDir();
 
  protected:
   // Used for testing.