Refactor Storate configuration.

The purpose is to make max_total_files_size, max_total_memory_size,
single_file_size and max_record common parameters for all queues,
and have them available in StorageQueue.
The next step would be to manage queues based on them.

Also, parameterize StorageTest (like StorageQueueTest was).


Bug:b:157943192
Bug:b:159361496

Change-Id: I073ba6f3a4d3dd4e9c92ef263a3f569a9343c3c0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2548101Reviewed-by: Zach Trudo <zatrudo@google.com>
Commit-Queue: Leonid Baraz <lbaraz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#829266}

chrome/browser/BUILD.gn

@@ -1222,6 +1222,7 @@ static_library("browser") {
     "policy/messaging_layer/public/report_queue_configuration.h",
     "policy/messaging_layer/storage/storage.cc",
     "policy/messaging_layer/storage/storage.h",
+    "policy/messaging_layer/storage/storage_configuration.h",
     "policy/messaging_layer/storage/storage_module.cc",
     "policy/messaging_layer/storage/storage_module.h",
     "policy/messaging_layer/storage/storage_queue.cc",

chrome/browser/policy/messaging_layer/public/report_client.cc

@@ -21,6 +21,7 @@
 #include "chrome/browser/net/system_network_context_manager.h"
 #include "chrome/browser/policy/messaging_layer/public/report_queue.h"
 #include "chrome/browser/policy/messaging_layer/public/report_queue_configuration.h"
+#include "chrome/browser/policy/messaging_layer/storage/storage_configuration.h"
 #include "chrome/browser/policy/messaging_layer/storage/storage_module.h"
 #include "chrome/browser/policy/messaging_layer/util/status.h"
 #include "chrome/browser/policy/messaging_layer/util/status_macros.h"
@@ -442,7 +443,7 @@ void ReportingClient::InitializingContext::ConfigureStorageModule() {
 
   base::FilePath reporting_path = user_data_dir.Append(kReportingDirectory);
   StorageModule::Create(
-      Storage::Options().set_directory(reporting_path),
+      StorageOptions().set_directory(reporting_path),
       std::move(start_upload_cb_), base::MakeRefCounted<EncryptionModule>(),
       base::BindOnce(
           &ReportingClient::InitializingContext::OnStorageModuleConfigured,

chrome/browser/policy/messaging_layer/storage/storage.cc

@@ -16,6 +16,7 @@
 #include "base/threading/thread.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/policy/messaging_layer/encryption/encryption_module.h"
+#include "chrome/browser/policy/messaging_layer/storage/storage_configuration.h"
 #include "chrome/browser/policy/messaging_layer/storage/storage_queue.h"
 #include "chrome/browser/policy/messaging_layer/util/status_macros.h"
 #include "chrome/browser/policy/messaging_layer/util/task_runner_context.h"
@@ -32,13 +33,11 @@ constexpr base::FilePath::CharType immediate_queue_subdir[] =
     FILE_PATH_LITERAL("Immediate");
 constexpr base::FilePath::CharType immediate_queue_prefix[] =
     FILE_PATH_LITERAL("P_Immediate");
-const uint64_t immediate_queue_total = 4 * 1024LL;
 
 constexpr base::FilePath::CharType fast_batch_queue_subdir[] =
     FILE_PATH_LITERAL("FastBatch");
 constexpr base::FilePath::CharType fast_batch_queue_prefix[] =
     FILE_PATH_LITERAL("P_FastBatch");
-const uint64_t fast_batch_queue_total = 64 * 1024LL;
 constexpr base::TimeDelta fast_batch_upload_period =
     base::TimeDelta::FromSeconds(1);
 
@@ -46,7 +45,6 @@ constexpr base::FilePath::CharType slow_batch_queue_subdir[] =
     FILE_PATH_LITERAL("SlowBatch");
 constexpr base::FilePath::CharType slow_batch_queue_prefix[] =
     FILE_PATH_LITERAL("P_SlowBatch");
-const uint64_t slow_batch_queue_total = 16 * 1024LL * 1024LL;
 constexpr base::TimeDelta slow_batch_upload_period =
     base::TimeDelta::FromSeconds(20);
 
@@ -54,7 +52,6 @@ constexpr base::FilePath::CharType background_queue_subdir[] =
     FILE_PATH_LITERAL("Background");
 constexpr base::FilePath::CharType background_queue_prefix[] =
     FILE_PATH_LITERAL("P_Background");
-const uint64_t background_queue_total = 64 * 1024LL * 1024LL;
 constexpr base::TimeDelta background_upload_period =
     base::TimeDelta::FromMinutes(1);
 
@@ -62,47 +59,36 @@ constexpr base::FilePath::CharType manual_queue_subdir[] =
     FILE_PATH_LITERAL("Manual");
 constexpr base::FilePath::CharType manual_queue_prefix[] =
     FILE_PATH_LITERAL("P_Manual");
-const uint64_t manual_queue_total = 64 * 1024LL * 1024LL;
 constexpr base::TimeDelta manual_upload_period = base::TimeDelta::Max();
 
 // Returns vector of <priority, queue_options> for all expected queues in
 // Storage. Queues are all located under the given root directory.
-std::vector<std::pair<Priority, StorageQueue::Options>> ExpectedQueues(
-    const base::FilePath& root_directory) {
+std::vector<std::pair<Priority, QueueOptions>> ExpectedQueues(
+    const StorageOptions& options) {
   return {
-      std::make_pair(IMMEDIATE, StorageQueue::Options()
-                                    .set_directory(root_directory.Append(
-                                        immediate_queue_subdir))
-                                    .set_file_prefix(immediate_queue_prefix)
-                                    .set_total_size(immediate_queue_total)),
-      std::make_pair(
-          FAST_BATCH,
-          StorageQueue::Options()
-              .set_directory(root_directory.Append(fast_batch_queue_subdir))
-              .set_file_prefix(fast_batch_queue_prefix)
-              .set_total_size(fast_batch_queue_total)
-              .set_upload_period(fast_batch_upload_period)),
-      std::make_pair(
-          SLOW_BATCH,
-          StorageQueue::Options()
-              .set_directory(root_directory.Append(slow_batch_queue_subdir))
-              .set_file_prefix(slow_batch_queue_prefix)
-              .set_total_size(slow_batch_queue_total)
-              .set_upload_period(slow_batch_upload_period)),
-      std::make_pair(
-          BACKGROUND_BATCH,
-          StorageQueue::Options()
-              .set_directory(root_directory.Append(background_queue_subdir))
-              .set_file_prefix(background_queue_prefix)
-              .set_total_size(background_queue_total)
-              .set_upload_period(background_upload_period)),
-      std::make_pair(
-          MANUAL_BATCH,
-          StorageQueue::Options()
-              .set_directory(root_directory.Append(manual_queue_subdir))
-              .set_file_prefix(manual_queue_prefix)
-              .set_total_size(manual_queue_total)
-              .set_upload_period(manual_upload_period)),
+      std::make_pair(IMMEDIATE, QueueOptions(options)
+                                    .set_subdirectory(immediate_queue_subdir)
+                                    .set_file_prefix(immediate_queue_prefix)),
+      std::make_pair(FAST_BATCH,
+                     QueueOptions(options)
+                         .set_subdirectory(fast_batch_queue_subdir)
+                         .set_file_prefix(fast_batch_queue_prefix)
+                         .set_upload_period(fast_batch_upload_period)),
+      std::make_pair(SLOW_BATCH,
+                     QueueOptions(options)
+                         .set_subdirectory(slow_batch_queue_subdir)
+                         .set_file_prefix(slow_batch_queue_prefix)
+                         .set_upload_period(slow_batch_upload_period)),
+      std::make_pair(BACKGROUND_BATCH,
+                     QueueOptions(options)
+                         .set_subdirectory(background_queue_subdir)
+                         .set_file_prefix(background_queue_prefix)
+                         .set_upload_period(background_upload_period)),
+      std::make_pair(MANUAL_BATCH,
+                     QueueOptions(options)
+                         .set_subdirectory(manual_queue_subdir)
+                         .set_file_prefix(manual_queue_prefix)
+                         .set_upload_period(manual_upload_period)),
   };
 }
 
@@ -154,7 +140,7 @@ class Storage::QueueUploaderInterface : public StorageQueue::UploaderInterface {
 };
 
 void Storage::Create(
-    const Options& options,
+    const StorageOptions& options,
     StartUploadCb start_upload_cb,
     scoped_refptr<EncryptionModule> encryption_module,
     base::OnceCallback<void(StatusOr<scoped_refptr<Storage>>)> completion_cb) {
@@ -163,8 +149,7 @@ void Storage::Create(
       : public TaskRunnerContext<StatusOr<scoped_refptr<Storage>>> {
    public:
     StorageInitContext(
-        const std::vector<std::pair<Priority, StorageQueue::Options>>&
-            queues_options,
+        const std::vector<std::pair<Priority, QueueOptions>>& queues_options,
         scoped_refptr<EncryptionModule> encryption_module,
         scoped_refptr<Storage> storage,
         base::OnceCallback<void(StatusOr<scoped_refptr<Storage>>)> callback)
@@ -228,8 +213,7 @@ void Storage::Create(
       Response(std::move(storage_));
     }
 
-    const std::vector<std::pair<Priority, StorageQueue::Options>>
-        queues_options_;
+    const std::vector<std::pair<Priority, QueueOptions>> queues_options_;
     scoped_refptr<EncryptionModule> encryption_module_;
     scoped_refptr<Storage> storage_;
     int32_t count_;
@@ -242,12 +226,12 @@ void Storage::Create(
       base::WrapRefCounted(new Storage(options, std::move(start_upload_cb)));
 
   // Asynchronously run initialization.
-  Start<StorageInitContext>(ExpectedQueues(storage->options_.directory()),
+  Start<StorageInitContext>(ExpectedQueues(storage->options_),
                             encryption_module, std::move(storage),
                             std::move(completion_cb));
 }
 
-Storage::Storage(const Options& options, StartUploadCb start_upload_cb)
+Storage::Storage(const StorageOptions& options, StartUploadCb start_upload_cb)
     : options_(options), start_upload_cb_(std::move(start_upload_cb)) {}
 
 Storage::~Storage() = default;

chrome/browser/policy/messaging_layer/storage/storage.h

@@ -16,6 +16,7 @@
 #include "base/memory/scoped_refptr.h"
 #include "base/strings/string_piece.h"
 #include "chrome/browser/policy/messaging_layer/encryption/encryption_module.h"
+#include "chrome/browser/policy/messaging_layer/storage/storage_configuration.h"
 #include "chrome/browser/policy/messaging_layer/storage/storage_queue.h"
 #include "chrome/browser/policy/messaging_layer/util/status.h"
 #include "chrome/browser/policy/messaging_layer/util/statusor.h"
@@ -36,29 +37,9 @@ class Storage : public base::RefCountedThreadSafe<Storage> {
       base::RepeatingCallback<StatusOr<std::unique_ptr<UploaderInterface>>(
           Priority priority)>;
 
-  // Options class allowing to set parameters individually, e.g.:
-  // Storage::Create(Options()
-  //                     .set_directory("/var/cache/reporting"),
-  //                 callback);
-  class Options {
-   public:
-    Options() = default;
-    Options(const Options& options) = default;
-    Options& operator=(const Options& options) = default;
-    Options& set_directory(const base::FilePath& directory) {
-      directory_ = directory;
-      return *this;
-    }
-    const base::FilePath& directory() const { return directory_; }
-
-   private:
-    // Subdirectory of the location assigned for this Storage.
-    base::FilePath directory_;
-  };
-
   // Creates Storage instance, and returns it with the completion callback.
   static void Create(
-      const Options& options,
+      const StorageOptions& options,
       StartUploadCb start_upload_cb,
       scoped_refptr<EncryptionModule> encryption_module,
       base::OnceCallback<void(StatusOr<scoped_refptr<Storage>>)> completion_cb);
@@ -98,7 +79,7 @@ class Storage : public base::RefCountedThreadSafe<Storage> {
 
   // Private constructor, to be called by Create factory method only.
   // Queues need to be added afterwards.
-  Storage(const Options& options, StartUploadCb start_upload_cb);
+  Storage(const StorageOptions& options, StartUploadCb start_upload_cb);
 
   // Initializes the object by adding all queues for all priorities.
   // Must be called once and only once after construction.
@@ -111,7 +92,7 @@ class Storage : public base::RefCountedThreadSafe<Storage> {
   // need to protect or serialize access to it.
   StatusOr<scoped_refptr<StorageQueue>> GetQueue(Priority priority);
 
-  const Options options_;
+  const StorageOptions options_;
 
   // Map priority->StorageQueue.
   base::flat_map<Priority, scoped_refptr<StorageQueue>> queues_;

chrome/browser/policy/messaging_layer/storage/storage_configuration.h

+// Copyright 2020 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_POLICY_MESSAGING_LAYER_STORAGE_STORAGE_CONFIGURATION_H_
+#define CHROME_BROWSER_POLICY_MESSAGING_LAYER_STORAGE_STORAGE_CONFIGURATION_H_
+
+namespace reporting {
+
+// Storage options class allowing to set parameters individually, e.g.:
+// Storage::Create(Options()
+//                     .set_directory("/var/cache/reporting")
+//                     .set_max_record_size(4 * 1024u)
+//                     .set_max_total_files_size(64 * 1024u * 1024u)
+//                     .set_max_total_memory_size(256 * 1024u),
+//                 callback);
+class StorageOptions {
+ public:
+  StorageOptions() = default;
+  StorageOptions(const StorageOptions& options) = default;
+  StorageOptions& operator=(const StorageOptions& options) = default;
+  StorageOptions& set_directory(const base::FilePath& directory) {
+    directory_ = directory;
+    return *this;
+  }
+  StorageOptions& set_max_record_size(size_t max_record_size) {
+    max_record_size_ = max_record_size;
+    return *this;
+  }
+  StorageOptions& set_max_total_files_size(uint64_t max_total_files_size) {
+    max_total_files_size_ = max_total_files_size;
+    return *this;
+  }
+  StorageOptions& set_max_total_memory_size(uint64_t max_total_memory_size) {
+    max_total_memory_size_ = max_total_memory_size;
+    return *this;
+  }
+  StorageOptions& set_single_file_size(uint64_t single_file_size) {
+    single_file_size_ = single_file_size;
+    return *this;
+  }
+  const base::FilePath& directory() const { return directory_; }
+  size_t max_record_size() const { return max_record_size_; }
+  uint64_t max_total_files_size() const { return max_total_files_size_; }
+  uint64_t max_total_memory_size() const { return max_total_memory_size_; }
+  uint64_t single_file_size() const { return single_file_size_; }
+
+ private:
+  // Subdirectory of the location assigned for this Storage.
+  base::FilePath directory_;
+
+  // Maximum record size.
+  size_t max_record_size_ = 1 * 1024LL * 1024LL;  // 1 MiB
+
+  // Maximum total size of all files in all queues.
+  uint64_t max_total_files_size_ = 64 * 1024LL * 1024LL;  // 64 MiB
+
+  // Maximum memory usage (reading buffers).
+  uint64_t max_total_memory_size_ = 4 * 1024LL * 1024LL;  // 4 MiB
+
+  // Cut-off size of an individual file in all queues.
+  // When file exceeds this size, the new file is created
+  // for further records. Note that each file must have at least
+  // one record before it is closed, regardless of that record size.
+  uint64_t single_file_size_ = 1 * 1024LL * 1024LL;  // 1 MiB
+};
+
+// Single queue options class allowing to set parameters individually, e.g.:
+// StorageQueue::Create(QueueOptions(storage_options)
+//                  .set_subdirectory("reporting")
+//                  .set_file_prefix(FILE_PATH_LITERAL("p00000001")),
+//                 callback);
+// storage_options must outlive QueueOptions.
+class QueueOptions {
+ public:
+  explicit QueueOptions(const StorageOptions& storage_options)
+      : storage_options_(storage_options) {}
+  QueueOptions(const QueueOptions& options) = default;
+  //   QueueOptions& operator=(const QueueOptions& options) = default;
+  QueueOptions& set_subdirectory(
+      const base::FilePath::StringType& subdirectory) {
+    directory_ = storage_options_.directory().Append(subdirectory);
+    return *this;
+  }
+  QueueOptions& set_file_prefix(const base::FilePath::StringType& file_prefix) {
+    file_prefix_ = file_prefix;
+    return *this;
+  }
+  QueueOptions& set_upload_period(base::TimeDelta upload_period) {
+    upload_period_ = upload_period;
+    return *this;
+  }
+  const base::FilePath& directory() const { return directory_; }
+  const base::FilePath::StringType& file_prefix() const { return file_prefix_; }
+  size_t max_record_size() const { return storage_options_.max_record_size(); }
+  size_t max_total_files_size() const {
+    return storage_options_.max_total_files_size();
+  }
+  size_t max_total_memory_size() const {
+    return storage_options_.max_total_memory_size();
+  }
+  uint64_t single_file_size() const {
+    return storage_options_.single_file_size();
+  }
+  base::TimeDelta upload_period() const { return upload_period_; }
+
+ private:
+  // Whole storage options, which this queue options are based on.
+  const StorageOptions& storage_options_;
+
+  // Subdirectory of the Storage location assigned for this StorageQueue.
+  base::FilePath directory_;
+  // Prefix of data files assigned for this StorageQueue.
+  base::FilePath::StringType file_prefix_;
+  // Time period the data is uploaded with.
+  // If 0, uploaded immediately after a new record is stored
+  // (this setting is intended for the immediate priority).
+  // Can be set to infinity - in that case Flush() is expected to be
+  // called from time to time.
+  base::TimeDelta upload_period_;
+};
+
+}  // namespace reporting
+
+#endif  // CHROME_BROWSER_POLICY_MESSAGING_LAYER_STORAGE_STORAGE_CONFIGURATION_H_

chrome/browser/policy/messaging_layer/storage/storage_module.cc

@@ -11,6 +11,7 @@
 #include "base/memory/ptr_util.h"
 #include "chrome/browser/policy/messaging_layer/encryption/encryption_module.h"
 #include "chrome/browser/policy/messaging_layer/storage/storage.h"
+#include "chrome/browser/policy/messaging_layer/storage/storage_configuration.h"
 #include "chrome/browser/policy/messaging_layer/storage/storage_module.h"
 #include "chrome/browser/policy/messaging_layer/util/status.h"
 #include "chrome/browser/policy/messaging_layer/util/statusor.h"
@@ -42,7 +43,7 @@ void StorageModule::ReportSuccess(
 
 // static
 void StorageModule::Create(
-    const Storage::Options& options,
+    const StorageOptions& options,
     Storage::StartUploadCb start_upload_cb,
     scoped_refptr<EncryptionModule> encryption_module,
     base::OnceCallback<void(StatusOr<scoped_refptr<StorageModule>>)> callback) {

chrome/browser/policy/messaging_layer/storage/storage_module.h

@@ -12,6 +12,7 @@
 #include "base/memory/scoped_refptr.h"
 #include "chrome/browser/policy/messaging_layer/encryption/encryption_module.h"
 #include "chrome/browser/policy/messaging_layer/storage/storage.h"
+#include "chrome/browser/policy/messaging_layer/storage/storage_configuration.h"
 #include "chrome/browser/policy/messaging_layer/util/status.h"
 #include "chrome/browser/policy/messaging_layer/util/statusor.h"
 #include "components/policy/proto/record.pb.h"
@@ -23,7 +24,7 @@ class StorageModule : public base::RefCountedThreadSafe<StorageModule> {
  public:
   // Factory method creates |StorageModule| object.
   static void Create(
-      const Storage::Options& options,
+      const StorageOptions& options,
       Storage::StartUploadCb start_upload_cb,
       scoped_refptr<EncryptionModule> encryption_module,
       base::OnceCallback<void(StatusOr<scoped_refptr<StorageModule>>)>

chrome/browser/policy/messaging_layer/storage/storage_queue.cc

@@ -33,6 +33,7 @@
 #include "base/task/post_task.h"
 #include "base/task_runner.h"
 #include "chrome/browser/policy/messaging_layer/encryption/encryption_module.h"
+#include "chrome/browser/policy/messaging_layer/storage/storage_configuration.h"
 #include "chrome/browser/policy/messaging_layer/util/status.h"
 #include "chrome/browser/policy/messaging_layer/util/status_macros.h"
 #include "chrome/browser/policy/messaging_layer/util/statusor.h"
@@ -77,7 +78,7 @@ struct RecordHeader {
 
 // static
 void StorageQueue::Create(
-    const Options& options,
+    const QueueOptions& options,
     StartUploadCb start_upload_cb,
     scoped_refptr<EncryptionModule> encryption_module,
     base::OnceCallback<void(StatusOr<scoped_refptr<StorageQueue>>)>
@@ -124,7 +125,7 @@ void StorageQueue::Create(
                                  std::move(completion_cb));
 }
 
-StorageQueue::StorageQueue(const Options& options,
+StorageQueue::StorageQueue(const QueueOptions& options,
                            StartUploadCb start_upload_cb,
                            scoped_refptr<EncryptionModule> encryption_module)
     : options_(options),
@@ -384,7 +385,7 @@ StatusOr<scoped_refptr<StorageQueue::SingleFile>> StorageQueue::AssignLastFile(
             /*size=*/0));
     DCHECK(insert_result.second);
   }
-  if (size > options_.total_size()) {
+  if (size > options_.max_record_size()) {
     return Status(error::OUT_OF_RANGE, "Too much data to be recorded at once");
   }
   scoped_refptr<SingleFile> last_file = files_.rbegin()->second;

chrome/browser/policy/messaging_layer/storage/storage_queue.h

@@ -24,6 +24,7 @@
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/timer/timer.h"
 #include "chrome/browser/policy/messaging_layer/encryption/encryption_module.h"
+#include "chrome/browser/policy/messaging_layer/storage/storage_configuration.h"
 #include "chrome/browser/policy/messaging_layer/util/status.h"
 #include "chrome/browser/policy/messaging_layer/util/statusor.h"
 #include "components/policy/proto/record.pb.h"
@@ -36,67 +37,6 @@ namespace reporting {
 // sequencing number to be eliminated.
 class StorageQueue : public base::RefCountedThreadSafe<StorageQueue> {
  public:
-  // Options class allowing to set parameters individually, e.g.:
-  // StorageQueue::Create(Options()
-  //                  .set_directory("/var/cache/reporting")
-  //                  .set_file_prefix(FILE_PATH_LITERAL("p00000001"))
-  //                  .set_total_size(128 * 1024LL * 1024LL),
-  //                 callback);
-  class Options {
-   public:
-    Options() = default;
-    Options(const Options& options) = default;
-    Options& operator=(const Options& options) = default;
-    Options& set_directory(const base::FilePath& directory) {
-      directory_ = directory;
-      return *this;
-    }
-    Options& set_file_prefix(const base::FilePath::StringType& file_prefix) {
-      file_prefix_ = file_prefix;
-      return *this;
-    }
-    Options& set_single_file_size(uint64_t single_file_size) {
-      single_file_size_ = single_file_size;
-      return *this;
-    }
-    Options& set_total_size(uint64_t total_size) {
-      total_size_ = total_size;
-      return *this;
-    }
-    Options& set_upload_period(base::TimeDelta upload_period) {
-      upload_period_ = upload_period;
-      return *this;
-    }
-    const base::FilePath& directory() const { return directory_; }
-    const base::FilePath::StringType& file_prefix() const {
-      return file_prefix_;
-    }
-    uint64_t single_file_size() const { return single_file_size_; }
-    uint64_t total_size() const { return total_size_; }
-    base::TimeDelta upload_period() const { return upload_period_; }
-
-   private:
-    // Subdirectory of the Storage location assigned for this StorageQueue.
-    base::FilePath directory_;
-    // Prefix of data files assigned for this StorageQueue.
-    base::FilePath::StringType file_prefix_;
-    // Cut-off size of an individual file in the set.
-    // When file exceeds this size, the new file is created
-    // for further records. Note that each file must have at least
-    // one record before it is closed, regardless of that record size.
-    uint64_t single_file_size_ = 1 * 1024LL * 1024LL;  // 1 MiB
-    // Cut-off total size of all files in the set.
-    // When the storage queue exceeds this size, oldest records can be
-    // dropped.
-    uint64_t total_size_ = 256 * 256LL * 256LL;  // 256 MiB
-    // Time period the data is uploaded with.
-    // If 0, uploaded immediately after a new record is stored
-    // (this setting is intended for the immediate priority).
-    // Can be set to infinity - in that case Flush() is expected to be
-    // called from time to time.
-    base::TimeDelta upload_period_;
-  };
-
   // Interface for Upload, which must be implemented by an object returned by
   // |StartUpload| callback (see below).
   // Every time StorageQueue starts an upload (by timer or immediately after
@@ -137,7 +77,7 @@ class StorageQueue : public base::RefCountedThreadSafe<StorageQueue> {
   // records - periodically or immediately after Write (and in the near future -
   // upon explicit Flush request).
   static void Create(
-      const Options& options,
+      const QueueOptions& options,
       StartUploadCb start_upload_cb,
       scoped_refptr<EncryptionModule> encryption_module,
       base::OnceCallback<void(StatusOr<scoped_refptr<StorageQueue>>)>
@@ -186,7 +126,7 @@ class StorageQueue : public base::RefCountedThreadSafe<StorageQueue> {
   void TestInjectBlockReadErrors(std::initializer_list<uint64_t> seq_numbers);
 
   // Access queue options.
-  const Options& options() const { return options_; }
+  const QueueOptions& options() const { return options_; }
 
   StorageQueue(const StorageQueue& other) = delete;
   StorageQueue& operator=(const StorageQueue& other) = delete;
@@ -256,7 +196,7 @@ class StorageQueue : public base::RefCountedThreadSafe<StorageQueue> {
   };
 
   // Private constructor, to be called by Create factory method only.
-  StorageQueue(const Options& options,
+  StorageQueue(const QueueOptions& options,
                StartUploadCb start_upload_cb,
                scoped_refptr<EncryptionModule> encryption_module);
 
@@ -345,7 +285,7 @@ class StorageQueue : public base::RefCountedThreadSafe<StorageQueue> {
   Status RemoveConfirmedData(uint64_t seq_number);
 
   // Immutable options, stored at the time of creation.
-  const Options options_;
+  const QueueOptions options_;
 
   // Current generation id, unique per device and queue.
   // Set up once during initialization by reading from the 'gen_id.NNNN' file

chrome/browser/policy/messaging_layer/storage/storage_queue_unittest.cc

@@ -18,6 +18,7 @@
 #include "base/synchronization/waitable_event.h"
 #include "base/test/task_environment.h"
 #include "chrome/browser/policy/messaging_layer/encryption/test_encryption_module.h"
+#include "chrome/browser/policy/messaging_layer/storage/storage_configuration.h"
 #include "chrome/browser/policy/messaging_layer/util/status.h"
 #include "chrome/browser/policy/messaging_layer/util/statusor.h"
 #include "components/policy/proto/record.pb.h"
@@ -261,9 +262,13 @@ class MockUploadClient : public StorageQueue::UploaderInterface {
 
 class StorageQueueTest : public ::testing::TestWithParam<size_t> {
  protected:
-  void SetUp() override { ASSERT_TRUE(location_.CreateUniqueTempDir()); }
+  void SetUp() override {
+    ASSERT_TRUE(location_.CreateUniqueTempDir());
+    options_.set_directory(base::FilePath(location_.GetPath()))
+        .set_single_file_size(GetParam());
+  }
 
-  void CreateStorageQueueOrDie(const StorageQueue::Options& options) {
+  void CreateStorageQueueOrDie(const QueueOptions& options) {
     ASSERT_FALSE(storage_queue_) << "StorageQueue already assigned";
     test_encryption_module_ =
         base::MakeRefCounted<test::TestEncryptionModule>();
@@ -283,20 +288,18 @@ class StorageQueueTest : public ::testing::TestWithParam<size_t> {
     storage_queue_->TestInjectBlockReadErrors(seq_numbers);
   }
 
-  StorageQueue::Options BuildStorageQueueOptionsImmediate() const {
-    return StorageQueue::Options()
-        .set_directory(
-            base::FilePath(location_.GetPath()).Append(FILE_PATH_LITERAL("D1")))
-        .set_file_prefix(FILE_PATH_LITERAL("F0001"))
-        .set_single_file_size(GetParam());
+  QueueOptions BuildStorageQueueOptionsImmediate() const {
+    return QueueOptions(options_)
+        .set_subdirectory(FILE_PATH_LITERAL("D1"))
+        .set_file_prefix(FILE_PATH_LITERAL("F0001"));
   }
 
-  StorageQueue::Options BuildStorageQueueOptionsPeriodic(
+  QueueOptions BuildStorageQueueOptionsPeriodic(
       base::TimeDelta upload_period = base::TimeDelta::FromSeconds(1)) const {
     return BuildStorageQueueOptionsImmediate().set_upload_period(upload_period);
   }
 
-  StorageQueue::Options BuildStorageQueueOptionsOnlyManual() const {
+  QueueOptions BuildStorageQueueOptionsOnlyManual() const {
     return BuildStorageQueueOptionsPeriodic(base::TimeDelta::Max());
   }
 
@@ -332,6 +335,7 @@ class StorageQueueTest : public ::testing::TestWithParam<size_t> {
   }
 
   base::ScopedTempDir location_;
+  StorageOptions options_;
   scoped_refptr<test::TestEncryptionModule> test_encryption_module_;
   scoped_refptr<StorageQueue> storage_queue_;
 
@@ -455,7 +459,7 @@ TEST_P(StorageQueueTest,
   WriteStringOrDie(data[2]);
 
   // Save copy of options.
-  const StorageQueue::Options options = storage_queue_->options();
+  const QueueOptions options = storage_queue_->options();
 
   storage_queue_.reset();
 
@@ -496,7 +500,7 @@ TEST_P(StorageQueueTest,
   WriteStringOrDie(data[2]);
 
   // Save copy of options.
-  const StorageQueue::Options options = storage_queue_->options();
+  const QueueOptions options = storage_queue_->options();
 
   storage_queue_.reset();
 

chrome/browser/policy/messaging_layer/storage/storage_unittest.cc

@@ -15,6 +15,7 @@
 #include "base/synchronization/waitable_event.h"
 #include "base/test/task_environment.h"
 #include "chrome/browser/policy/messaging_layer/encryption/test_encryption_module.h"
+#include "chrome/browser/policy/messaging_layer/storage/storage_configuration.h"
 #include "chrome/browser/policy/messaging_layer/util/status.h"
 #include "chrome/browser/policy/messaging_layer/util/statusor.h"
 #include "components/policy/proto/record.pb.h"
@@ -231,11 +232,11 @@ class MockUploadClient : public Storage::UploaderInterface {
   Sequence test_upload_sequence_;
 };
 
-class StorageTest : public ::testing::Test {
+class StorageTest : public ::testing::TestWithParam<size_t> {
  protected:
   void SetUp() override { ASSERT_TRUE(location_.CreateUniqueTempDir()); }
 
-  void CreateStorageTestOrDie(const Storage::Options& options) {
+  void CreateStorageTestOrDie(const StorageOptions& options) {
     ASSERT_FALSE(storage_) << "StorageTest already assigned";
     test_encryption_module_ =
         base::MakeRefCounted<test::TestEncryptionModule>();
@@ -250,9 +251,10 @@ class StorageTest : public ::testing::Test {
     storage_ = std::move(storage_result.ValueOrDie());
   }
 
-  Storage::Options BuildStorageOptions() const {
-    return Storage::Options().set_directory(
-        base::FilePath(location_.GetPath()));
+  StorageOptions BuildStorageOptions() const {
+    return StorageOptions()
+        .set_directory(base::FilePath(location_.GetPath()))
+        .set_single_file_size(GetParam());
   }
 
   StatusOr<std::unique_ptr<Storage::UploaderInterface>> BuildMockUploader(
@@ -305,7 +307,7 @@ constexpr std::array<const char*, 3> data = {"Rec1111", "Rec222", "Rec33"};
 constexpr std::array<const char*, 3> more_data = {"More1111", "More222",
                                                   "More33"};
 
-TEST_F(StorageTest, WriteIntoNewStorageAndReopen) {
+TEST_P(StorageTest, WriteIntoNewStorageAndReopen) {
   EXPECT_CALL(set_mock_uploader_expectations_, Call(_, NotNull())).Times(0);
   CreateStorageTestOrDie(BuildStorageOptions());
   WriteStringOrDie(FAST_BATCH, data[0]);
@@ -317,7 +319,7 @@ TEST_F(StorageTest, WriteIntoNewStorageAndReopen) {
   CreateStorageTestOrDie(BuildStorageOptions());
 }
 
-TEST_F(StorageTest, WriteIntoNewStorageReopenAndWriteMore) {
+TEST_P(StorageTest, WriteIntoNewStorageReopenAndWriteMore) {
   EXPECT_CALL(set_mock_uploader_expectations_, Call(_, NotNull())).Times(0);
   CreateStorageTestOrDie(BuildStorageOptions());
   WriteStringOrDie(FAST_BATCH, data[0]);
@@ -332,7 +334,7 @@ TEST_F(StorageTest, WriteIntoNewStorageReopenAndWriteMore) {
   WriteStringOrDie(FAST_BATCH, more_data[2]);
 }
 
-TEST_F(StorageTest, WriteIntoNewStorageAndUpload) {
+TEST_P(StorageTest, WriteIntoNewStorageAndUpload) {
   CreateStorageTestOrDie(BuildStorageOptions());
   WriteStringOrDie(FAST_BATCH, data[0]);
   WriteStringOrDie(FAST_BATCH, data[1]);
@@ -352,7 +354,7 @@ TEST_F(StorageTest, WriteIntoNewStorageAndUpload) {
   task_environment_.FastForwardBy(base::TimeDelta::FromSeconds(1));
 }
 
-TEST_F(StorageTest, WriteIntoNewStorageReopenWriteMoreAndUpload) {
+TEST_P(StorageTest, WriteIntoNewStorageReopenWriteMoreAndUpload) {
   CreateStorageTestOrDie(BuildStorageOptions());
   WriteStringOrDie(FAST_BATCH, data[0]);
   WriteStringOrDie(FAST_BATCH, data[1]);
@@ -382,7 +384,7 @@ TEST_F(StorageTest, WriteIntoNewStorageReopenWriteMoreAndUpload) {
   task_environment_.FastForwardBy(base::TimeDelta::FromSeconds(1));
 }
 
-TEST_F(StorageTest, WriteIntoNewStorageAndFlush) {
+TEST_P(StorageTest, WriteIntoNewStorageAndFlush) {
   CreateStorageTestOrDie(BuildStorageOptions());
   WriteStringOrDie(MANUAL_BATCH, data[0]);
   WriteStringOrDie(MANUAL_BATCH, data[1]);
@@ -403,7 +405,7 @@ TEST_F(StorageTest, WriteIntoNewStorageAndFlush) {
   EXPECT_OK(storage_->Flush(MANUAL_BATCH));
 }
 
-TEST_F(StorageTest, WriteIntoNewStorageReopenWriteMoreAndFlush) {
+TEST_P(StorageTest, WriteIntoNewStorageReopenWriteMoreAndFlush) {
   CreateStorageTestOrDie(BuildStorageOptions());
   WriteStringOrDie(MANUAL_BATCH, data[0]);
   WriteStringOrDie(MANUAL_BATCH, data[1]);
@@ -434,7 +436,7 @@ TEST_F(StorageTest, WriteIntoNewStorageReopenWriteMoreAndFlush) {
   EXPECT_OK(storage_->Flush(MANUAL_BATCH));
 }
 
-TEST_F(StorageTest, WriteAndRepeatedlyUploadWithConfirmations) {
+TEST_P(StorageTest, WriteAndRepeatedlyUploadWithConfirmations) {
   CreateStorageTestOrDie(BuildStorageOptions());
 
   WriteStringOrDie(FAST_BATCH, data[0]);
@@ -511,7 +513,7 @@ TEST_F(StorageTest, WriteAndRepeatedlyUploadWithConfirmations) {
   task_environment_.FastForwardBy(base::TimeDelta::FromSeconds(1));
 }
 
-TEST_F(StorageTest, WriteAndRepeatedlyImmediateUpload) {
+TEST_P(StorageTest, WriteAndRepeatedlyImmediateUpload) {
   CreateStorageTestOrDie(BuildStorageOptions());
 
   // Upload is initiated asynchronously, so it may happen after the next
@@ -549,7 +551,7 @@ TEST_F(StorageTest, WriteAndRepeatedlyImmediateUpload) {
                    data[2]);  // Immediately uploads and verifies.
 }
 
-TEST_F(StorageTest, WriteAndRepeatedlyImmediateUploadWithConfirmations) {
+TEST_P(StorageTest, WriteAndRepeatedlyImmediateUploadWithConfirmations) {
   CreateStorageTestOrDie(BuildStorageOptions());
 
   // Upload is initiated asynchronously, so it may happen after the next
@@ -623,7 +625,7 @@ TEST_F(StorageTest, WriteAndRepeatedlyImmediateUploadWithConfirmations) {
   WriteStringOrDie(IMMEDIATE, more_data[2]);
 }
 
-TEST_F(StorageTest, WriteAndRepeatedlyUploadMultipleQueues) {
+TEST_P(StorageTest, WriteAndRepeatedlyUploadMultipleQueues) {
   CreateStorageTestOrDie(BuildStorageOptions());
 
   // Upload is initiated asynchronously, so it may happen after the next
@@ -699,7 +701,7 @@ TEST_F(StorageTest, WriteAndRepeatedlyUploadMultipleQueues) {
   task_environment_.FastForwardBy(base::TimeDelta::FromSeconds(20));
 }
 
-TEST_F(StorageTest, WriteEncryptFailure) {
+TEST_P(StorageTest, WriteEncryptFailure) {
   CreateStorageTestOrDie(BuildStorageOptions());
   DCHECK(test_encryption_module_);
   EXPECT_CALL(*test_encryption_module_, EncryptRecord(_, _))
@@ -712,5 +714,11 @@ TEST_F(StorageTest, WriteEncryptFailure) {
   EXPECT_EQ(result.error_code(), error::UNKNOWN);
 }
 
+INSTANTIATE_TEST_SUITE_P(VaryingFileSize,
+                         StorageTest,
+                         testing::Values(128 * 1024LL * 1024LL,
+                                         256 /* two records in file */,
+                                         1 /* single record in file */));
+
 }  // namespace
 }  // namespace reporting