Extract the logic of generating report list

The existing ReportGenerator class contains the logic of generating
report list, which is used for ChromeDesktopReport. Now to share the
same logic with new ChromeOsUserReport, this part needs be extracted to
a new class: ProfileListReportGenerator.

Bug: 1010213
Change-Id: Id88df8924c134592628d395e9e35b19d553bbb49
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1894005
Commit-Queue: Anqing Zhao <anqing@google.com>
Reviewed-by: Owen Min <zmin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#714042}

chrome/browser/BUILD.gn

@@ -3064,6 +3064,8 @@ jumbo_static_library("browser") {
       "enterprise_reporting/profile_report_generator.h",
       "enterprise_reporting/report_generator.cc",
       "enterprise_reporting/report_generator.h",
+      "enterprise_reporting/report_request_queue_generator.cc",
+      "enterprise_reporting/report_request_queue_generator.h",
       "enterprise_reporting/report_scheduler.cc",
       "enterprise_reporting/report_scheduler.h",
       "enterprise_reporting/report_uploader.cc",

chrome/browser/enterprise_reporting/report_generator.cc

@@ -8,8 +8,6 @@
 
 #include "base/bind.h"
 #include "base/files/file_path.h"
-#include "base/metrics/histogram_functions.h"
-#include "base/path_service.h"
 #include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "components/policy/core/common/cloud/cloud_policy_util.h"
@@ -21,27 +19,8 @@
 namespace em = enterprise_management;
 
 namespace enterprise_reporting {
-namespace {
 
-const size_t kMaximumReportSize =
-    5000000;  // The report size limitation is 5mb.
-
-constexpr char kRequestCountMetricsName[] =
-    "Enterprise.CloudReportingRequestCount";
-constexpr char kRequestSizeMetricsName[] =
-    "Enterprise.CloudReportingRequestSize";
-constexpr char kBasicRequestSizeMetricsName[] =
-    "Enterprise.CloudReportingBasicRequestSize";
-
-// Because server only stores 20 profiles for each report and when report is
-// separated into requests, there is at least one profile per request. It means
-// server will truncate the report when there are more than 20 requests. Actions
-// are needed if there are many reports exceed this limitation.
-const int kRequestCountMetricMaxValue = 21;
-
-}  // namespace
-
-ReportGenerator::ReportGenerator() : maximum_report_size_(kMaximumReportSize) {}
+ReportGenerator::ReportGenerator() = default;
 
 ReportGenerator::~ReportGenerator() = default;
 
@@ -52,14 +31,17 @@ void ReportGenerator::Generate(ReportCallback callback) {
 }
 
 void ReportGenerator::SetMaximumReportSizeForTesting(size_t size) {
-  maximum_report_size_ = size;
+  report_request_queue_generator_.SetMaximumReportSizeForTesting(size);
 }
 
 void ReportGenerator::CreateBasicRequest() {
+#if !defined(OS_CHROMEOS)
   basic_request_.set_computer_name(this->GetMachineName());
   basic_request_.set_os_user_name(GetOSUserName());
   basic_request_.set_serial_number(GetSerialNumber());
   basic_request_.set_allocated_os_report(GetOSReport().release());
+#endif
+
   browser_report_generator_.Generate(base::BindOnce(
       &ReportGenerator::OnBrowserReportReady, weak_ptr_factory_.GetWeakPtr()));
 }
@@ -89,89 +71,11 @@ std::string ReportGenerator::GetSerialNumber() {
 #endif
 }
 
-void ReportGenerator::GenerateProfileReportWithIndex(int profile_index) {
-  DCHECK_LT(profile_index,
-            basic_request_.browser_report().chrome_user_profile_infos_size());
-
-  auto basic_profile_report =
-      basic_request_.browser_report().chrome_user_profile_infos(profile_index);
-  auto profile_report = profile_report_generator_.MaybeGenerate(
-      base::FilePath::FromUTF8Unsafe(basic_profile_report.id()),
-      basic_profile_report.name());
-
-  // Return if Profile is not loaded and there is no full report.
-  if (!profile_report) {
-    return;
-  }
-
-  // Use size diff to calculate estimated request size after full profile report
-  // is added. There are still few bytes difference but close enough.
-  size_t profile_report_incremental_size =
-      profile_report->ByteSizeLong() -
-      basic_request_.browser_report()
-          .chrome_user_profile_infos(profile_index)
-          .ByteSizeLong();
-  size_t current_request_size = requests_.back()->ByteSizeLong();
-
-  if (current_request_size + profile_report_incremental_size <=
-      maximum_report_size_) {
-    // The new full Profile report can be appended into the current request.
-    requests_.back()
-        ->mutable_browser_report()
-        ->mutable_chrome_user_profile_infos(profile_index)
-        ->Swap(profile_report.get());
-  } else if (basic_request_size_ + profile_report_incremental_size <=
-             maximum_report_size_) {
-    // The new full Profile report is too big to be appended into the current
-    // request, move it to the next request if possible. Record metrics for the
-    // current request's size.
-    base::UmaHistogramMemoryKB(kRequestSizeMetricsName,
-                               requests_.back()->ByteSizeLong() / 1024);
-    requests_.push(
-        std::make_unique<em::ChromeDesktopReportRequest>(basic_request_));
-    requests_.back()
-        ->mutable_browser_report()
-        ->mutable_chrome_user_profile_infos(profile_index)
-        ->Swap(profile_report.get());
-  } else {
-    // The new full Profile report is too big to be uploaded, skip this
-    // Profile report. But we still add the report size into metrics so
-    // that we could understand the situation better.
-    base::UmaHistogramMemoryKB(
-        kRequestSizeMetricsName,
-        (basic_request_size_ + profile_report_incremental_size) / 1024);
-  }
-}
-
 void ReportGenerator::OnBrowserReportReady(
     std::unique_ptr<em::BrowserReport> browser_report) {
   basic_request_.set_allocated_browser_report(browser_report.release());
-  basic_request_size_ = basic_request_.ByteSizeLong();
-  base::UmaHistogramMemoryKB(kBasicRequestSizeMetricsName,
-                             basic_request_size_ / 1024);
-  if (basic_request_size_ > maximum_report_size_) {
-    // Basic request is already too large so we can't upload any valid report.
-    // Skip all Profiles and response an empty request list.
-    Response();
-    return;
-  }
-
-  requests_.push(
-      std::make_unique<em::ChromeDesktopReportRequest>(basic_request_));
-  for (int index = 0;
-       index < basic_request_.browser_report().chrome_user_profile_infos_size();
-       index++) {
-    GenerateProfileReportWithIndex(index);
-  }
-  base::UmaHistogramMemoryKB(kRequestSizeMetricsName,
-                             requests_.back()->ByteSizeLong() / 1024);
-  Response();
-}
-
-void ReportGenerator::Response() {
-  base::UmaHistogramExactLinear(kRequestCountMetricsName, requests_.size(),
-                                kRequestCountMetricMaxValue);
-  std::move(callback_).Run(std::move(requests_));
+  Requests requests = report_request_queue_generator_.Generate(basic_request_);
+  std::move(callback_).Run(std::move(requests));
 }
 
 }  // namespace enterprise_reporting

chrome/browser/enterprise_reporting/report_generator.h

@@ -12,7 +12,7 @@
 
 #include "base/macros.h"
 #include "chrome/browser/enterprise_reporting/browser_report_generator.h"
-#include "chrome/browser/enterprise_reporting/profile_report_generator.h"
+#include "chrome/browser/enterprise_reporting/report_request_queue_generator.h"
 #include "components/policy/proto/device_management_backend.pb.h"
 
 namespace em = enterprise_management;
@@ -21,7 +21,12 @@ namespace enterprise_reporting {
 
 class ReportGenerator {
  public:
-  using Requests = std::queue<std::unique_ptr<em::ChromeDesktopReportRequest>>;
+#if defined(OS_CHROMEOS)
+  using Request = em::ChromeOsUserReportRequest;
+#else
+  using Request = em::ChromeDesktopReportRequest;
+#endif
+  using Requests = std::queue<std::unique_ptr<Request>>;
   using ReportCallback = base::OnceCallback<void(Requests)>;
 
   ReportGenerator();
@@ -50,24 +55,13 @@ class ReportGenerator {
   virtual std::string GetSerialNumber();
 
  private:
-  void GenerateProfileReportWithIndex(int profile_index);
-
   void OnBrowserReportReady(std::unique_ptr<em::BrowserReport> browser_report);
 
-  void Response();
-
-  ProfileReportGenerator profile_report_generator_;
+  ReportRequestQueueGenerator report_request_queue_generator_;
   BrowserReportGenerator browser_report_generator_;
-
   ReportCallback callback_;
-
-  Requests requests_;
-
   // Basic information that is shared among requests.
-  em::ChromeDesktopReportRequest basic_request_;
-  size_t basic_request_size_;
-
-  size_t maximum_report_size_;
+  Request basic_request_;
 
   base::WeakPtrFactory<ReportGenerator> weak_ptr_factory_{this};
 

chrome/browser/enterprise_reporting/report_request_queue_generator.cc

+// Copyright 2019 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/enterprise_reporting/report_request_queue_generator.h"
+
+#include <string>
+#include <vector>
+
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/files/file_path.h"
+#include "base/metrics/histogram_functions.h"
+
+namespace enterprise_reporting {
+namespace {
+
+const size_t kMaximumReportSize =
+    5000000;  // The report size limitation is 5mb.
+
+constexpr char kRequestCountMetricsName[] =
+    "Enterprise.CloudReportingRequestCount";
+constexpr char kRequestSizeMetricsName[] =
+    "Enterprise.CloudReportingRequestSize";
+constexpr char kBasicRequestSizeMetricsName[] =
+    "Enterprise.CloudReportingBasicRequestSize";
+
+// Because server only stores 20 profiles for each report and when report is
+// separated into requests, there is at least one profile per request. It means
+// server will truncate the report when there are more than 20 requests. Actions
+// are needed if there are many reports exceed this limitation.
+const int kRequestCountMetricMaxValue = 21;
+
+}  // namespace
+
+ReportRequestQueueGenerator::ReportRequestQueueGenerator()
+    : maximum_report_size_(kMaximumReportSize) {}
+
+ReportRequestQueueGenerator::~ReportRequestQueueGenerator() = default;
+
+size_t ReportRequestQueueGenerator::GetMaximumReportSizeForTesting() const {
+  return maximum_report_size_;
+}
+
+void ReportRequestQueueGenerator::SetMaximumReportSizeForTesting(
+    size_t maximum_report_size) {
+  maximum_report_size_ = maximum_report_size;
+}
+
+ReportRequestQueueGenerator::ReportRequests
+ReportRequestQueueGenerator::Generate(const ReportRequest& basic_request) {
+  ReportRequests requests;
+  size_t basic_request_size = basic_request.ByteSizeLong();
+  base::UmaHistogramMemoryKB(kBasicRequestSizeMetricsName,
+                             basic_request_size / 1024);
+
+  if (basic_request_size <= maximum_report_size_) {
+    requests.push(std::make_unique<ReportRequest>(basic_request));
+    int profile_infos_size =
+        basic_request.browser_report().chrome_user_profile_infos_size();
+    for (int index = 0; index < profile_infos_size; index++) {
+      GenerateProfileReportWithIndex(basic_request, index, &requests);
+    }
+
+    base::UmaHistogramMemoryKB(kRequestSizeMetricsName,
+                               requests.back()->ByteSizeLong() / 1024);
+  }
+
+  base::UmaHistogramExactLinear(kRequestCountMetricsName, requests.size(),
+                                kRequestCountMetricMaxValue);
+  return requests;
+}
+
+void ReportRequestQueueGenerator::GenerateProfileReportWithIndex(
+    const ReportRequest& basic_request,
+    int profile_index,
+    ReportRequests* requests) {
+  DCHECK_LT(profile_index,
+            basic_request.browser_report().chrome_user_profile_infos_size());
+
+  size_t basic_request_size = basic_request.ByteSizeLong();
+  auto basic_profile =
+      basic_request.browser_report().chrome_user_profile_infos(profile_index);
+  auto profile_report = profile_report_generator_.MaybeGenerate(
+      base::FilePath::FromUTF8Unsafe(basic_profile.id()), basic_profile.name());
+
+  // Return if Profile is not loaded and there is no full report.
+  if (!profile_report)
+    return;
+
+  // Use size diff to calculate estimated request size after full profile report
+  // is added. There are still few bytes difference but close enough.
+  size_t profile_report_incremental_size =
+      profile_report->ByteSizeLong() - basic_profile.ByteSizeLong();
+  size_t current_request_size = requests->back()->ByteSizeLong();
+
+  if (current_request_size + profile_report_incremental_size <=
+      maximum_report_size_) {
+    // The new full Profile report can be appended into the current request.
+    requests->back()
+        ->mutable_browser_report()
+        ->mutable_chrome_user_profile_infos(profile_index)
+        ->Swap(profile_report.get());
+  } else if (basic_request_size + profile_report_incremental_size <=
+             maximum_report_size_) {
+    // The new full Profile report is too big to be appended into the current
+    // request, move it to the next request if possible. Record metrics for the
+    // current request's size.
+    base::UmaHistogramMemoryKB(kRequestSizeMetricsName,
+                               requests->back()->ByteSizeLong() / 1024);
+    requests->push(std::make_unique<ReportRequest>(basic_request));
+    requests->back()
+        ->mutable_browser_report()
+        ->mutable_chrome_user_profile_infos(profile_index)
+        ->Swap(profile_report.get());
+  } else {
+    // The new full Profile report is too big to be uploaded, skip this
+    // Profile report. But we still add the report size into metrics so
+    // that we could understand the situation better.
+    base::UmaHistogramMemoryKB(
+        kRequestSizeMetricsName,
+        (basic_request_size + profile_report_incremental_size) / 1024);
+  }
+}
+
+}  // namespace enterprise_reporting

chrome/browser/enterprise_reporting/report_request_queue_generator.h

+// Copyright 2019 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_ENTERPRISE_REPORTING_REPORT_REQUEST_QUEUE_GENERATOR_H_
+#define CHROME_BROWSER_ENTERPRISE_REPORTING_REPORT_REQUEST_QUEUE_GENERATOR_H_
+
+#include <memory>
+#include <queue>
+
+#include "base/callback_forward.h"
+#include "base/macros.h"
+#include "build/build_config.h"
+#include "chrome/browser/enterprise_reporting/profile_report_generator.h"
+#include "components/policy/proto/device_management_backend.pb.h"
+
+namespace em = enterprise_management;
+
+namespace enterprise_reporting {
+
+// Generate a report request queue that contains full profile information. The
+// request number in the queue is decided by the maximum report size setting.
+class ReportRequestQueueGenerator {
+#if defined(OS_CHROMEOS)
+  using ReportRequest = em::ChromeOsUserReportRequest;
+#else
+  using ReportRequest = em::ChromeDesktopReportRequest;
+#endif
+
+  using ReportRequests = std::queue<std::unique_ptr<ReportRequest>>;
+  using BrowserReportGetter =
+      base::RepeatingCallback<em::BrowserReport*(ReportRequest*)>;
+
+ public:
+  explicit ReportRequestQueueGenerator();
+  ReportRequestQueueGenerator(const ReportRequestQueueGenerator&) = delete;
+  ReportRequestQueueGenerator& operator=(const ReportRequestQueueGenerator&) =
+      delete;
+  ~ReportRequestQueueGenerator();
+
+  // Get the maximum report size.
+  size_t GetMaximumReportSizeForTesting() const;
+
+  // Set the maximum report size. The full profile info will be skipped or moved
+  // to another new request if its size exceeds the limit.
+  void SetMaximumReportSizeForTesting(size_t maximum_report_size);
+
+  // Generate a queue of requests including full profile info based on given
+  // basic request.
+  ReportRequests Generate(const ReportRequest& basic_request);
+
+ private:
+  // Generate request with full profile info at |profile_index| according to
+  // |basic_request|, then store it into |requests|.
+  void GenerateProfileReportWithIndex(const ReportRequest& basic_request,
+                                      int profile_index,
+                                      ReportRequests* requests);
+
+ private:
+  size_t maximum_report_size_;
+  ProfileReportGenerator profile_report_generator_;
+};
+
+}  // namespace enterprise_reporting
+
+#endif  // CHROME_BROWSER_ENTERPRISE_REPORTING_REPORT_REQUEST_QUEUE_GENERATOR_H_

chrome/browser/enterprise_reporting/report_uploader.cc

@@ -47,10 +47,15 @@ void ReportUploader::SetRequestAndUpload(Requests requests,
 }
 
 void ReportUploader::Upload() {
-  client_->UploadChromeDesktopReport(
-      std::make_unique<em::ChromeDesktopReportRequest>(*requests_.front()),
-      base::BindRepeating(&ReportUploader::OnRequestFinished,
-                          weak_ptr_factory_.GetWeakPtr()));
+  auto request = std::make_unique<Request>(*requests_.front());
+  auto callback = base::BindRepeating(&ReportUploader::OnRequestFinished,
+                                      weak_ptr_factory_.GetWeakPtr());
+
+#if defined(OS_CHROMEOS)
+  // Pending for another change list for cloud_policy_client.
+#else
+  client_->UploadChromeDesktopReport(std::move(request), std::move(callback));
+#endif
 }
 
 void ReportUploader::OnRequestFinished(bool status) {

chrome/browser/enterprise_reporting/report_uploader.h

@@ -18,7 +18,11 @@ class OneShotTimer;
 }  // namespace base
 
 namespace enterprise_management {
+#if defined(OS_CHROMEOS)
+class ChromeOsUserReportRequest;
+#else
 class ChromeDesktopReportRequest;
+#endif
 }  // namespace enterprise_management
 
 namespace policy {
@@ -48,7 +52,13 @@ class ReportUploader {
                        // invalid dm token.
   };
 
-  using Requests = std::queue<std::unique_ptr<em::ChromeDesktopReportRequest>>;
+#if defined(OS_CHROMEOS)
+  using Request = em::ChromeOsUserReportRequest;
+#else
+  using Request = em::ChromeDesktopReportRequest;
+#endif
+
+  using Requests = std::queue<std::unique_ptr<Request>>;
   // A callback to notify the upload result.
   using ReportCallback = base::OnceCallback<void(ReportStatus status)>;
 

chrome/test/BUILD.gn

@@ -3696,8 +3696,7 @@ test("unit_tests") {
       "../browser/enterprise_reporting/policy_info_unittest.cc",
       "../browser/enterprise_reporting/profile_report_generator_unittest.cc",
       "../browser/enterprise_reporting/report_generator_unittest.cc",
-      "../browser/enterprise_reporting/report_scheduler_unittest.cc",
-      "../browser/enterprise_reporting/report_uploader_unittest.cc",
+      "../browser/enterprise_reporting/report_request_queue_generator_unittest.cc",
       "../browser/enterprise_reporting/request_timer_unittest.cc",
       "../browser/first_run/first_run_unittest.cc",
       "../browser/font_family_cache_unittest.cc",
@@ -3985,6 +3984,15 @@ test("unit_tests") {
     if (is_mac) {
       deps += [ ":firefox_importer_interface" ]
     }
+
+    # TODO(anqing): finally these two set of unit test will be suitable to
+    # other platforms, rather than Chrome OS only.
+    if (!is_chromeos) {
+      sources += [
+        "../browser/enterprise_reporting/report_scheduler_unittest.cc",
+        "../browser/enterprise_reporting/report_uploader_unittest.cc",
+      ]
+    }
   }
 
   if (!is_android) {