Refactor feedback code

List of changes:
* restructure FeedbackPrivateDelegate to have a more generic interface
* remove unecessary use of unique_ptrs
* change instances of RepeatingCallback to OnceCallback
* factor common DebugDaemonLogSource callback functionality into one callback
* use move semantics to avoid large string copies

BUG=None
TEST=Ensure feedback report upload works

Change-Id: I5b33ab60c727e2a92a08a463e9ad874e5e72b3d6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1560516Reviewed-by: Chris Morin <cmtm@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: Michael Giuffrida <michaelpg@chromium.org>
Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
Commit-Queue: Chris Morin <cmtm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#650420}

chrome/browser/chromeos/system_logs/debug_daemon_log_source.cc

@@ -89,25 +89,31 @@ void DebugDaemonLogSource::Fetch(SysLogsSourceCallback callback) {
 
   client->GetRoutes(true,   // Numeric
                     false,  // No IPv6
-                    base::Bind(&DebugDaemonLogSource::OnGetRoutes,
-                               weak_ptr_factory_.GetWeakPtr()));
+                    base::BindOnce(&DebugDaemonLogSource::OnGetRoutes,
+                                   weak_ptr_factory_.GetWeakPtr()));
   ++num_pending_requests_;
-  client->GetNetworkStatus(base::Bind(&DebugDaemonLogSource::OnGetNetworkStatus,
-                                      weak_ptr_factory_.GetWeakPtr()));
+
+  client->GetNetworkStatus(base::BindOnce(&DebugDaemonLogSource::OnGetOneLog,
+                                          weak_ptr_factory_.GetWeakPtr(),
+                                          kNetworkStatusKeyName));
   ++num_pending_requests_;
-  client->GetModemStatus(base::Bind(&DebugDaemonLogSource::OnGetModemStatus,
-                                    weak_ptr_factory_.GetWeakPtr()));
+
+  client->GetModemStatus(base::BindOnce(&DebugDaemonLogSource::OnGetOneLog,
+                                        weak_ptr_factory_.GetWeakPtr(),
+                                        kModemStatusKeyName));
   ++num_pending_requests_;
-  client->GetWiMaxStatus(base::Bind(&DebugDaemonLogSource::OnGetWiMaxStatus,
-                                    weak_ptr_factory_.GetWeakPtr()));
+
+  client->GetWiMaxStatus(base::BindOnce(&DebugDaemonLogSource::OnGetOneLog,
+                                        weak_ptr_factory_.GetWeakPtr(),
+                                        kWiMaxStatusKeyName));
   ++num_pending_requests_;
 
   if (scrub_) {
-    client->GetScrubbedBigLogs(base::Bind(&DebugDaemonLogSource::OnGetLogs,
-                                          weak_ptr_factory_.GetWeakPtr()));
+    client->GetScrubbedBigLogs(base::BindOnce(&DebugDaemonLogSource::OnGetLogs,
+                                              weak_ptr_factory_.GetWeakPtr()));
   } else {
-    client->GetAllLogs(base::Bind(&DebugDaemonLogSource::OnGetLogs,
-                                  weak_ptr_factory_.GetWeakPtr()));
+    client->GetAllLogs(base::BindOnce(&DebugDaemonLogSource::OnGetLogs,
+                                      weak_ptr_factory_.GetWeakPtr()));
   }
   ++num_pending_requests_;
 }
@@ -122,28 +128,11 @@ void DebugDaemonLogSource::OnGetRoutes(
   RequestCompleted();
 }
 
-void DebugDaemonLogSource::OnGetNetworkStatus(
-    base::Optional<std::string> status) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-
-  (*response_)[kNetworkStatusKeyName] =
-      std::move(status).value_or(kNotAvailable);
-  RequestCompleted();
-}
-
-void DebugDaemonLogSource::OnGetModemStatus(
-    base::Optional<std::string> status) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-
-  (*response_)[kModemStatusKeyName] = std::move(status).value_or(kNotAvailable);
-  RequestCompleted();
-}
-
-void DebugDaemonLogSource::OnGetWiMaxStatus(
-    base::Optional<std::string> status) {
+void DebugDaemonLogSource::OnGetOneLog(std::string key,
+                                       base::Optional<std::string> status) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  (*response_)[kWiMaxStatusKeyName] = std::move(status).value_or(kNotAvailable);
+  (*response_)[std::move(key)] = std::move(status).value_or(kNotAvailable);
   RequestCompleted();
 }
 

chrome/browser/chromeos/system_logs/debug_daemon_log_source.h

@@ -33,11 +33,9 @@ class DebugDaemonLogSource : public SystemLogsSource {
  private:
   typedef std::map<std::string, std::string> KeyValueMap;
 
-  // Callbacks for the 5 different dbus calls to debugd.
+  // Callbacks for the dbus calls to debugd.
   void OnGetRoutes(base::Optional<std::vector<std::string>> routes);
-  void OnGetNetworkStatus(base::Optional<std::string> status);
-  void OnGetModemStatus(base::Optional<std::string> status);
-  void OnGetWiMaxStatus(base::Optional<std::string> status);
+  void OnGetOneLog(std::string key, base::Optional<std::string> status);
   void OnGetLogs(bool succeeded,
                  const KeyValueMap& logs);
 

chrome/browser/chromeos/system_logs/iwlwifi_dump_log_source.cc

@@ -15,7 +15,6 @@ namespace system_logs {
 
 namespace {
 
-constexpr char kIwlwifiDumpKey[] = "iwlwifi_dump";
 constexpr char kIwlwifiDumpLocation[] = "/var/log/last_iwlwifi_dump";
 
 std::unique_ptr<SystemLogsResponse> CheckExistenceOnBlockingTaskRunner() {
@@ -70,20 +69,8 @@ void IwlwifiDumpLogSource::Fetch(SysLogsSourceCallback callback) {
       base::BindOnce(std::move(callback)));
 }
 
-bool ContainsIwlwifiLogs(FeedbackCommon::SystemLogsMap* sys_logs) {
+bool ContainsIwlwifiLogs(const FeedbackCommon::SystemLogsMap* sys_logs) {
   return sys_logs->count(kIwlwifiDumpKey);
 }
 
-void MergeIwlwifiLogs(
-    std::unique_ptr<FeedbackCommon::SystemLogsMap> original_sys_logs,
-    system_logs::SysLogsFetcherCallback callback,
-    std::unique_ptr<system_logs::SystemLogsResponse> fetched_iwlwifi_response) {
-  if (fetched_iwlwifi_response->count(kIwlwifiDumpKey)) {
-    (*original_sys_logs)[kIwlwifiDumpKey] =
-        std::move(fetched_iwlwifi_response->at(kIwlwifiDumpKey));
-  }
-
-  std::move(callback).Run(std::move(original_sys_logs));
-}
-
 }  // namespace system_logs

chrome/browser/chromeos/system_logs/iwlwifi_dump_log_source.h

@@ -14,6 +14,8 @@
 
 namespace system_logs {
 
+constexpr char kIwlwifiDumpKey[] = "iwlwifi_dump";
+
 // The classes here are used to attach debug dump information from
 // Intel Wi-Fi NICs that will be produced when those NICs have issues
 // such as firmware crashes. This information will be used to help
@@ -49,16 +51,7 @@ class IwlwifiDumpLogSource : public SystemLogsSource {
 };
 
 // Checks to see if |sys_logs| contains the iwlwifi logs key.
-bool ContainsIwlwifiLogs(FeedbackCommon::SystemLogsMap* sys_logs);
-
-// This should be passed as a callback to the fetcher that will fetch logs
-// from the IwlwifiDumpLogSource above. It will merge the
-// |fetched_iwlwifi_response| into the |original_sys_logs| and call the
-// |callback| with that result.
-void MergeIwlwifiLogs(
-    std::unique_ptr<FeedbackCommon::SystemLogsMap> original_sys_logs,
-    system_logs::SysLogsFetcherCallback callback,
-    std::unique_ptr<system_logs::SystemLogsResponse> fetched_iwlwifi_response);
+bool ContainsIwlwifiLogs(const FeedbackCommon::SystemLogsMap* sys_logs);
 
 }  // namespace system_logs
 

chrome/browser/extensions/api/feedback_private/chrome_feedback_private_delegate.cc

@@ -177,24 +177,34 @@ ChromeFeedbackPrivateDelegate::CreateSingleLogSource(
   }
 }
 
-void ChromeFeedbackPrivateDelegate::FetchAndMergeIwlwifiDumpLogsIfPresent(
-    std::unique_ptr<FeedbackCommon::SystemLogsMap> original_sys_logs,
-    content::BrowserContext* context,
-    system_logs::SysLogsFetcherCallback callback) const {
-  if (!original_sys_logs ||
-      !system_logs::ContainsIwlwifiLogs(original_sys_logs.get())) {
-    VLOG(1) << "WiFi dump logs are not present.";
-    std::move(callback).Run(std::move(original_sys_logs));
-    return;
+void OnFetchedExtraLogs(
+    scoped_refptr<feedback::FeedbackData> feedback_data,
+    FetchExtraLogsCallback callback,
+    std::unique_ptr<system_logs::SystemLogsResponse> response) {
+  using system_logs::kIwlwifiDumpKey;
+  if (response && response->count(kIwlwifiDumpKey)) {
+    feedback_data->AddLog(kIwlwifiDumpKey,
+                          std::move(response->at(kIwlwifiDumpKey)));
   }
+  std::move(callback).Run(feedback_data);
+}
 
-  VLOG(1) << "Fetching WiFi dump logs.";
-  system_logs::SystemLogsFetcher* fetcher =
-      new system_logs::SystemLogsFetcher(true /* scrub_data */);
-  fetcher->AddSource(std::make_unique<system_logs::IwlwifiDumpLogSource>());
-  fetcher->Fetch(base::BindOnce(&system_logs::MergeIwlwifiLogs,
-                                std::move(original_sys_logs),
-                                std::move(callback)));
+void ChromeFeedbackPrivateDelegate::FetchExtraLogs(
+    scoped_refptr<feedback::FeedbackData> feedback_data,
+    FetchExtraLogsCallback callback) const {
+  // Anonymize data.
+  constexpr bool scrub = true;
+
+  if (system_logs::ContainsIwlwifiLogs(feedback_data->sys_info())) {
+    VLOG(1) << "Fetching WiFi dump logs.";
+    system_logs::SystemLogsFetcher* fetcher =
+        new system_logs::SystemLogsFetcher(scrub);
+    fetcher->AddSource(std::make_unique<system_logs::IwlwifiDumpLogSource>());
+    fetcher->Fetch(base::BindOnce(&OnFetchedExtraLogs, feedback_data,
+                                  std::move(callback)));
+  } else {
+    VLOG(1) << "WiFi dump logs are not present.";
+  }
 }
 
 void ChromeFeedbackPrivateDelegate::UnloadFeedbackExtension(

chrome/browser/extensions/api/feedback_private/chrome_feedback_private_delegate.h

@@ -5,7 +5,7 @@
 #ifndef CHROME_BROWSER_EXTENSIONS_API_FEEDBACK_PRIVATE_CHROME_FEEDBACK_PRIVATE_DELEGATE_H_
 #define CHROME_BROWSER_EXTENSIONS_API_FEEDBACK_PRIVATE_CHROME_FEEDBACK_PRIVATE_DELEGATE_H_
 
-#include "components/feedback/feedback_common.h"
+#include "components/feedback/feedback_data.h"
 #include "extensions/browser/api/feedback_private/feedback_private_delegate.h"
 
 #include "base/macros.h"
@@ -26,10 +26,8 @@ class ChromeFeedbackPrivateDelegate : public FeedbackPrivateDelegate {
 #if defined(OS_CHROMEOS)
   std::unique_ptr<system_logs::SystemLogsSource> CreateSingleLogSource(
       api::feedback_private::LogSource source_type) const override;
-  void FetchAndMergeIwlwifiDumpLogsIfPresent(
-      std::unique_ptr<FeedbackCommon::SystemLogsMap> original_sys_logs,
-      content::BrowserContext* context,
-      system_logs::SysLogsFetcherCallback callback) const override;
+  void FetchExtraLogs(scoped_refptr<feedback::FeedbackData> feedback_data,
+                      FetchExtraLogsCallback callback) const override;
   void UnloadFeedbackExtension(content::BrowserContext* context) const override;
 #endif  // defined(OS_CHROMEOS)
   std::string GetSignedInUserEmail(

chrome/browser/feedback/feedback_util_chromeos.cc

@@ -39,7 +39,9 @@ void OnGetSystemInformation(
 
   feedback_data->set_context(profile);
   feedback_data->set_description(description);
-  feedback_data->SetAndCompressSystemInfo(std::move(sys_info));
+  if (sys_info)
+    feedback_data->AddLogs(std::move(*sys_info));
+  feedback_data->CompressSystemInfo();
 
   GetFeedbackService(profile)->SendFeedback(feedback_data, callback);
 }

chromeos/dbus/debug_daemon_client.cc

@@ -50,11 +50,11 @@ const int kBigLogsDBusTimeoutMS = 120 * 1000;
 // the requester.
 class PipeReaderWrapper : public base::SupportsWeakPtr<PipeReaderWrapper> {
  public:
-  explicit PipeReaderWrapper(const DebugDaemonClient::GetLogsCallback& callback)
+  explicit PipeReaderWrapper(DebugDaemonClient::GetLogsCallback callback)
       : pipe_reader_(base::CreateTaskRunnerWithTraits(
             {base::MayBlock(),
              base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN})),
-        callback_(callback) {}
+        callback_(std::move(callback)) {}
 
   base::ScopedFD Initialize() {
     return pipe_reader_.StartIO(
@@ -92,9 +92,9 @@ class PipeReaderWrapper : public base::SupportsWeakPtr<PipeReaderWrapper> {
   void RunCallbackAndDestroy(
       base::Optional<std::map<std::string, std::string>> result) {
     if (result.has_value()) {
-      callback_.Run(true, result.value());
+      std::move(callback_).Run(true, std::move(result.value()));
     } else {
-      callback_.Run(false, std::map<std::string, std::string>());
+      std::move(callback_).Run(false, std::map<std::string, std::string>());
     }
     delete this;
   }
@@ -236,22 +236,22 @@ class DebugDaemonClientImpl : public DebugDaemonClient {
                        weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
   }
 
-  void GetScrubbedLogs(const GetLogsCallback& callback) override {
+  void GetScrubbedLogs(GetLogsCallback callback) override {
     dbus::MethodCall method_call(debugd::kDebugdInterface,
                                  debugd::kGetFeedbackLogs);
     debugdaemon_proxy_->CallMethod(
         &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
         base::BindOnce(&DebugDaemonClientImpl::OnGetAllLogs,
-                       weak_ptr_factory_.GetWeakPtr(), callback));
+                       weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
   }
 
-  void GetScrubbedBigLogs(const GetLogsCallback& callback) override {
+  void GetScrubbedBigLogs(GetLogsCallback callback) override {
     // The PipeReaderWrapper is a self-deleting object; we don't have to worry
     // about ownership or lifetime. We need to create a new one for each Big
     // Logs requests in order to queue these requests. One request can take a
     // long time to be processed and a new request should never be ignored nor
     // cancels the on-going one.
-    PipeReaderWrapper* pipe_reader = new PipeReaderWrapper(callback);
+    PipeReaderWrapper* pipe_reader = new PipeReaderWrapper(std::move(callback));
     base::ScopedFD pipe_write_end = pipe_reader->Initialize();
 
     dbus::MethodCall method_call(debugd::kDebugdInterface,
@@ -267,22 +267,22 @@ class DebugDaemonClientImpl : public DebugDaemonClient {
                        pipe_reader->AsWeakPtr()));
   }
 
-  void GetAllLogs(const GetLogsCallback& callback) override {
+  void GetAllLogs(GetLogsCallback callback) override {
     dbus::MethodCall method_call(debugd::kDebugdInterface,
                                  debugd::kGetAllLogs);
     debugdaemon_proxy_->CallMethod(
         &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
         base::BindOnce(&DebugDaemonClientImpl::OnGetAllLogs,
-                       weak_ptr_factory_.GetWeakPtr(), callback));
+                       weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
   }
 
-  void GetUserLogFiles(const GetLogsCallback& callback) override {
+  void GetUserLogFiles(GetLogsCallback callback) override {
     dbus::MethodCall method_call(debugd::kDebugdInterface,
                                  debugd::kGetUserLogFiles);
     debugdaemon_proxy_->CallMethod(
         &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
         base::BindOnce(&DebugDaemonClientImpl::OnGetUserLogFiles,
-                       weak_ptr_factory_.GetWeakPtr(), callback));
+                       weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
   }
 
   void GetLog(const std::string& log_name,
@@ -605,13 +605,12 @@ class DebugDaemonClientImpl : public DebugDaemonClient {
     std::move(callback).Run(std::move(routes));
   }
 
-  void OnGetAllLogs(const GetLogsCallback& callback,
-                    dbus::Response* response) {
+  void OnGetAllLogs(GetLogsCallback callback, dbus::Response* response) {
     std::map<std::string, std::string> logs;
     bool broken = false;  // did we see a broken (k,v) pair?
     dbus::MessageReader sub_reader(NULL);
     if (!response || !dbus::MessageReader(response).PopArray(&sub_reader)) {
-      callback.Run(false, logs);
+      std::move(callback).Run(false, logs);
       return;
     }
     while (sub_reader.HasMoreData()) {
@@ -625,12 +624,11 @@ class DebugDaemonClientImpl : public DebugDaemonClient {
       }
       logs[key] = value;
     }
-    callback.Run(!sub_reader.HasMoreData() && !broken, logs);
+    std::move(callback).Run(!sub_reader.HasMoreData() && !broken, logs);
   }
 
-  void OnGetUserLogFiles(const GetLogsCallback& callback,
-                         dbus::Response* response) {
-    return OnGetAllLogs(callback, response);
+  void OnGetUserLogFiles(GetLogsCallback callback, dbus::Response* response) {
+    return OnGetAllLogs(std::move(callback), response);
   }
 
   void OnBigFeedbackLogsResponse(base::WeakPtr<PipeReaderWrapper> pipe_reader,

chromeos/dbus/debug_daemon_client.h

@@ -90,22 +90,22 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS) DebugDaemonClient
 
   // Callback type for GetScrubbedLogs(), GetAllLogs() or GetUserLogFiles().
   using GetLogsCallback =
-      base::Callback<void(bool succeeded,
-                          const std::map<std::string, std::string>& logs)>;
+      base::OnceCallback<void(bool succeeded,
+                              const std::map<std::string, std::string>& logs)>;
 
   // Gets scrubbed logs from debugd.
-  virtual void GetScrubbedLogs(const GetLogsCallback& callback) = 0;
+  virtual void GetScrubbedLogs(GetLogsCallback callback) = 0;
 
   // Gets the scrubbed logs from debugd that are very large and cannot be
   // returned directly from D-Bus. These logs will include ARC and cheets
   // system information.
-  virtual void GetScrubbedBigLogs(const GetLogsCallback& callback) = 0;
+  virtual void GetScrubbedBigLogs(GetLogsCallback callback) = 0;
 
   // Gets all logs collected by debugd.
-  virtual void GetAllLogs(const GetLogsCallback& callback) = 0;
+  virtual void GetAllLogs(GetLogsCallback callback) = 0;
 
   // Gets list of user log files that must be read by Chrome.
-  virtual void GetUserLogFiles(const GetLogsCallback& callback) = 0;
+  virtual void GetUserLogFiles(GetLogsCallback callback) = 0;
 
   // Gets an individual log source provided by debugd.
   virtual void GetLog(const std::string& log_name,

chromeos/dbus/fake_debug_daemon_client.cc

@@ -114,34 +114,33 @@ void FakeDebugDaemonClient::GetPerfOutput(
     int file_descriptor,
     DBusMethodCallback<uint64_t> error_callback) {}
 
-void FakeDebugDaemonClient::GetScrubbedLogs(const GetLogsCallback& callback) {
+void FakeDebugDaemonClient::GetScrubbedLogs(GetLogsCallback callback) {
   std::map<std::string, std::string> sample;
   sample["Sample Scrubbed Log"] = "Your email address is xxxxxxxx";
   base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindOnce(callback, false, sample));
+      FROM_HERE, base::BindOnce(std::move(callback), false, sample));
 }
 
-void FakeDebugDaemonClient::GetScrubbedBigLogs(
-    const GetLogsCallback& callback) {
+void FakeDebugDaemonClient::GetScrubbedBigLogs(GetLogsCallback callback) {
   std::map<std::string, std::string> sample;
   sample["Sample Scrubbed Big Log"] = "Your email address is xxxxxxxx";
   base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindOnce(callback, false, sample));
+      FROM_HERE, base::BindOnce(std::move(callback), false, sample));
 }
 
-void FakeDebugDaemonClient::GetAllLogs(const GetLogsCallback& callback) {
+void FakeDebugDaemonClient::GetAllLogs(GetLogsCallback callback) {
   std::map<std::string, std::string> sample;
   sample["Sample Log"] = "Your email address is abc@abc.com";
   base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindOnce(callback, false, sample));
+      FROM_HERE, base::BindOnce(std::move(callback), false, sample));
 }
 
-void FakeDebugDaemonClient::GetUserLogFiles(const GetLogsCallback& callback) {
+void FakeDebugDaemonClient::GetUserLogFiles(GetLogsCallback callback) {
   std::map<std::string, std::string> user_logs;
   user_logs["preferences"] = "Preferences";
   user_logs["invalid_file"] = "Invalid File";
   base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindOnce(callback, true, user_logs));
+      FROM_HERE, base::BindOnce(std::move(callback), true, user_logs));
 }
 
 void FakeDebugDaemonClient::GetLog(const std::string& log_name,

chromeos/dbus/fake_debug_daemon_client.h

@@ -53,12 +53,12 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS) FakeDebugDaemonClient
                      const std::vector<std::string>& perf_args,
                      int file_descriptor,
                      DBusMethodCallback<uint64_t> callback) override;
-  void GetScrubbedLogs(const GetLogsCallback& callback) override;
-  void GetScrubbedBigLogs(const GetLogsCallback& callback) override;
-  void GetAllLogs(const GetLogsCallback& callback) override;
+  void GetScrubbedLogs(GetLogsCallback callback) override;
+  void GetScrubbedBigLogs(GetLogsCallback callback) override;
+  void GetAllLogs(GetLogsCallback callback) override;
   void GetLog(const std::string& log_name,
               DBusMethodCallback<std::string> callback) override;
-  void GetUserLogFiles(const GetLogsCallback& callback) override;
+  void GetUserLogFiles(GetLogsCallback callback) override;
   void TestICMP(const std::string& ip_address,
                 const TestICMPCallback& callback) override;
   void TestICMPWithOptions(const std::string& ip_address,

components/feedback/feedback_common.cc

@@ -131,8 +131,8 @@ void FeedbackCommon::AddFile(const std::string& filename, std::string data) {
   attachments_.emplace_back(filename, std::move(data));
 }
 
-void FeedbackCommon::AddLog(const std::string& name, const std::string& value) {
-  logs_[name] = value;
+void FeedbackCommon::AddLog(std::string name, std::string value) {
+  logs_[std::move(name)] = std::move(value);
 }
 
 void FeedbackCommon::AddLogs(SystemLogsMap logs) {

components/feedback/feedback_common.h

@@ -40,7 +40,7 @@ class FeedbackCommon : public base::RefCountedThreadSafe<FeedbackCommon> {
 
   void AddFile(const std::string& filename, std::string data);
 
-  void AddLog(const std::string& name, const std::string& value);
+  void AddLog(std::string name, std::string value);
   void AddLogs(SystemLogsMap logs);
 
   // Fill in |feedback_data| with all the data that we have collected.

components/feedback/feedback_data.cc

@@ -53,8 +53,7 @@ void FeedbackData::OnFeedbackPageDataComplete() {
   SendReport();
 }
 
-void FeedbackData::SetAndCompressSystemInfo(
-    std::unique_ptr<FeedbackData::SystemLogsMap> sys_info) {
+void FeedbackData::CompressSystemInfo() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   if (trace_id_ != 0) {
@@ -69,14 +68,11 @@ void FeedbackData::SetAndCompressSystemInfo(
     }
   }
 
-  if (sys_info) {
-    ++pending_op_count_;
-    AddLogs(std::move(*sys_info));
-    base::PostTaskWithTraitsAndReply(
-        FROM_HERE, {base::MayBlock(), base::TaskPriority::BEST_EFFORT},
-        base::BindOnce(&FeedbackData::CompressLogs, this),
-        base::BindOnce(&FeedbackData::OnCompressComplete, this));
-  }
+  ++pending_op_count_;
+  base::PostTaskWithTraitsAndReply(
+      FROM_HERE, {base::MayBlock(), base::TaskPriority::BEST_EFFORT},
+      base::BindOnce(&FeedbackData::CompressLogs, this),
+      base::BindOnce(&FeedbackData::OnCompressComplete, this));
 }
 
 void FeedbackData::SetAndCompressHistograms(std::string histograms) {

components/feedback/feedback_data.h

@@ -30,9 +30,8 @@ class FeedbackData : public FeedbackCommon {
   // Called once we've updated all the data from the feedback page.
   void OnFeedbackPageDataComplete();
 
-  // Sets the system information for this instance and kicks off its
-  // compression.
-  void SetAndCompressSystemInfo(std::unique_ptr<SystemLogsMap> sys_info);
+  // Kicks off compression of the system information for this instance.
+  void CompressSystemInfo();
 
   // Sets the histograms for this instance and kicks off its
   // compression.

extensions/browser/api/feedback_private/feedback_private_api.cc

@@ -316,13 +316,10 @@ ExtensionFunction::ResponseAction FeedbackPrivateSendFeedbackFunction::Run() {
     feedback_data->set_screenshot_uuid(*feedback_info.screenshot_blob_uuid);
   }
 
-  auto sys_logs = std::make_unique<FeedbackData::SystemLogsMap>();
-  const SystemInformationList* sys_info =
-      feedback_info.system_information.get();
-  if (sys_info) {
-    for (const SystemInformation& info : *sys_info)
-      sys_logs->emplace(info.key, info.value);
-  }
+  const bool send_histograms =
+      feedback_info.send_histograms && *feedback_info.send_histograms;
+  const bool send_bluetooth_logs =
+      feedback_info.send_bluetooth_logs && *feedback_info.send_bluetooth_logs;
 
 #if defined(OS_CHROMEOS)
   feedback_data->set_from_assistant(feedback_info.from_assistant &&
@@ -330,31 +327,32 @@ ExtensionFunction::ResponseAction FeedbackPrivateSendFeedbackFunction::Run() {
   feedback_data->set_assistant_debug_info_allowed(
       feedback_info.assistant_debug_info_allowed &&
       *feedback_info.assistant_debug_info_allowed);
+#endif  // defined(OS_CHROMEOS)
 
-  delegate->FetchAndMergeIwlwifiDumpLogsIfPresent(
-      std::move(sys_logs), browser_context(),
-      base::BindOnce(
-          &FeedbackPrivateSendFeedbackFunction::OnAllLogsFetched, this,
-          feedback_data,
-          feedback_info.send_histograms && *feedback_info.send_histograms,
-          feedback_info.send_bluetooth_logs &&
-              *feedback_info.send_bluetooth_logs));
-#else
-  OnAllLogsFetched(feedback_data, false /* send_histograms */,
-                   false /* send_bluetooth_logs */, std::move(sys_logs));
+  if (params->feedback.system_information) {
+    for (SystemInformation& info : *params->feedback.system_information)
+      feedback_data->AddLog(std::move(info.key), std::move(info.value));
+#if defined(OS_CHROMEOS)
+    delegate->FetchExtraLogs(
+        feedback_data,
+        base::BindOnce(&FeedbackPrivateSendFeedbackFunction::OnAllLogsFetched,
+                       this, send_histograms, send_bluetooth_logs));
+    return RespondLater();
 #endif  // defined(OS_CHROMEOS)
+  }
+
+  OnAllLogsFetched(send_histograms, send_bluetooth_logs, feedback_data);
 
   return RespondLater();
 }
 
 void FeedbackPrivateSendFeedbackFunction::OnAllLogsFetched(
-    scoped_refptr<FeedbackData> feedback_data,
     bool send_histograms,
     bool send_bluetooth_logs,
-    std::unique_ptr<system_logs::SystemLogsResponse> sys_logs) {
+    scoped_refptr<feedback::FeedbackData> feedback_data) {
   VLOG(1) << "All logs have been fetched. Proceeding with sending the report.";
 
-  feedback_data->SetAndCompressSystemInfo(std::move(sys_logs));
+  feedback_data->CompressSystemInfo();
 
   if (send_histograms) {
     std::string histograms =

extensions/browser/api/feedback_private/feedback_private_api.h

@@ -140,11 +140,9 @@ class FeedbackPrivateSendFeedbackFunction : public UIThreadExtensionFunction {
   ResponseAction Run() override;
 
  private:
-  void OnAllLogsFetched(
-      scoped_refptr<feedback::FeedbackData> feedback_data,
-      bool send_histograms,
-      bool send_bluetooth_logs,
-      std::unique_ptr<FeedbackCommon::SystemLogsMap> sys_logs);
+  void OnAllLogsFetched(bool send_histograms,
+                        bool send_bluetooth_logs,
+                        scoped_refptr<feedback::FeedbackData> feedback_data);
   void OnCompleted(api::feedback_private::LandingPageType type, bool success);
 };
 

extensions/browser/api/feedback_private/feedback_private_delegate.h

@@ -5,7 +5,8 @@
 #ifndef EXTENSIONS_BROWSER_API_FEEDBACK_PRIVATE_FEEDBACK_PRIVATE_DELEGATE_H_
 #define EXTENSIONS_BROWSER_API_FEEDBACK_PRIVATE_FEEDBACK_PRIVATE_DELEGATE_H_
 
-#include "components/feedback/feedback_common.h"
+#include "base/callback.h"
+#include "components/feedback/feedback_data.h"
 #include "components/feedback/system_logs/system_logs_fetcher.h"
 #include "extensions/common/api/feedback_private.h"
 
@@ -31,6 +32,9 @@ class SystemLogsSource;
 
 namespace extensions {
 
+using FetchExtraLogsCallback =
+    base::OnceCallback<void(scoped_refptr<feedback::FeedbackData>)>;
+
 // Delegate class for embedder-specific chrome.feedbackPrivate behavior.
 class FeedbackPrivateDelegate {
  public:
@@ -53,12 +57,13 @@ class FeedbackPrivateDelegate {
   virtual std::unique_ptr<system_logs::SystemLogsSource> CreateSingleLogSource(
       api::feedback_private::LogSource source_type) const = 0;
 
-  // Takes ownership of |original_sys_logs|, merges Intel Wi-Fi debug logs if
-  // they exist, and passes the log map back via |callback|.
-  virtual void FetchAndMergeIwlwifiDumpLogsIfPresent(
-      std::unique_ptr<FeedbackCommon::SystemLogsMap> original_sys_logs,
-      content::BrowserContext* context,
-      system_logs::SysLogsFetcherCallback callback) const = 0;
+  // Gets logs that aren't passed to the sendFeedback function, but should be
+  // included in the feedback report. These currently consist of the Intel Wi-Fi
+  // debug logs (if they exist).
+  // Modifies |feedback_data| and passes it on to |callback|.
+  virtual void FetchExtraLogs(
+      scoped_refptr<feedback::FeedbackData> feedback_data,
+      FetchExtraLogsCallback callback) const = 0;
 
   // Unloads the feedback extension from the current profile, should only be
   // called when feedback is complete for the login profile.

extensions/shell/browser/api/feedback_private/shell_feedback_private_delegate.cc

@@ -41,12 +41,11 @@ ShellFeedbackPrivateDelegate::CreateSingleLogSource(
   return nullptr;
 }
 
-void ShellFeedbackPrivateDelegate::FetchAndMergeIwlwifiDumpLogsIfPresent(
-    std::unique_ptr<FeedbackCommon::SystemLogsMap> original_sys_logs,
-    content::BrowserContext* context,
-    system_logs::SysLogsFetcherCallback callback) const {
+void ShellFeedbackPrivateDelegate::FetchExtraLogs(
+    scoped_refptr<feedback::FeedbackData> feedback_data,
+    FetchExtraLogsCallback callback) const {
   NOTIMPLEMENTED();
-  std::move(callback).Run(std::move(original_sys_logs));
+  std::move(callback).Run(feedback_data);
 }
 
 void ShellFeedbackPrivateDelegate::UnloadFeedbackExtension(

extensions/shell/browser/api/feedback_private/shell_feedback_private_delegate.h

@@ -5,7 +5,7 @@
 #ifndef EXTENSIONS_SHELL_BROWSER_API_FEEDBACK_PRIVATE_SHELL_FEEDBACK_PRIVATE_DELEGATE_H_
 #define EXTENSIONS_SHELL_BROWSER_API_FEEDBACK_PRIVATE_SHELL_FEEDBACK_PRIVATE_DELEGATE_H_
 
-#include "components/feedback/feedback_common.h"
+#include "components/feedback/feedback_data.h"
 #include "extensions/browser/api/feedback_private/feedback_private_delegate.h"
 
 #include "base/macros.h"
@@ -26,10 +26,8 @@ class ShellFeedbackPrivateDelegate : public FeedbackPrivateDelegate {
 #if defined(OS_CHROMEOS)
   std::unique_ptr<system_logs::SystemLogsSource> CreateSingleLogSource(
       api::feedback_private::LogSource source_type) const override;
-  void FetchAndMergeIwlwifiDumpLogsIfPresent(
-      std::unique_ptr<FeedbackCommon::SystemLogsMap> original_sys_logs,
-      content::BrowserContext* context,
-      system_logs::SysLogsFetcherCallback callback) const override;
+  void FetchExtraLogs(scoped_refptr<feedback::FeedbackData> feedback_data,
+                      FetchExtraLogsCallback callback) const override;
   void UnloadFeedbackExtension(content::BrowserContext* context) const override;
 #endif  // defined(OS_CHROMEOS)
   std::string GetSignedInUserEmail(