Convert FeedbackService to OnceCallback

The callback passed to SendFeedback is semantically meant to be called
only once (to signify the completion of SendFeedback). However, because
the feedback report depends on multiple conditions completing, this
callback is currently copied into multiple BlobReader callbacks and
invoked only once all have been completed.

This CL replaces the copies by storing the callback in a member and
having the CompleteSendFeedback method call that instead of copying it
around with various bound callback. This allows us to make it a
OnceCallback, improving readability and safety.

We also notice that SendSysLogFeedback, the only function in
feedback_util_chromeos.{h|cc}, is never used. Thus, we can remove these
files entirely.

Bug: 1152268
Change-Id: Ie6d3683562f0374833a83468fc48e98ec0158c87
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2601225Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#839760}

chrome/browser/BUILD.gn

@@ -4298,8 +4298,6 @@ static_library("browser") {
       "download/notification/download_notification_manager.h",
       "enterprise/reporting/android_app_info_generator.cc",
       "enterprise/reporting/android_app_info_generator.h",
-      "feedback/feedback_util_chromeos.cc",
-      "feedback/feedback_util_chromeos.h",
       "google/google_brand_chromeos.cc",
       "google/google_brand_chromeos.h",
       "google/google_brand_code_map_chromeos.cc",

chrome/browser/feedback/feedback_util_chromeos.cc

-// Copyright 2016 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/feedback/feedback_util_chromeos.h"
-
-#include "base/bind.h"
-#include "base/callback.h"
-#include "chrome/browser/feedback/feedback_uploader_chrome.h"
-#include "chrome/browser/feedback/feedback_uploader_factory_chrome.h"
-#include "chrome/browser/feedback/system_logs/chrome_system_logs_fetcher.h"
-#include "chrome/browser/profiles/profile.h"
-#include "components/feedback/feedback_data.h"
-#include "components/feedback/feedback_report.h"
-#include "components/feedback/system_logs/system_logs_fetcher.h"
-#include "extensions/browser/api/feedback_private/feedback_private_api.h"
-#include "extensions/browser/api/feedback_private/feedback_service.h"
-
-using feedback::FeedbackData;
-
-namespace feedback_util {
-
-namespace {
-
-extensions::FeedbackService* GetFeedbackService(Profile* profile) {
-  return extensions::FeedbackPrivateAPI::GetFactoryInstance()
-      ->Get(profile)
-      ->GetService();
-}
-
-void OnGetSystemInformation(
-    Profile* profile,
-    const std::string& description,
-    const SendSysLogFeedbackCallback& callback,
-    bool send_tab_titles,
-    std::unique_ptr<system_logs::SystemLogsResponse> sys_info) {
-  scoped_refptr<FeedbackData> feedback_data =
-      base::MakeRefCounted<FeedbackData>(
-          feedback::FeedbackUploaderFactoryChrome::GetForBrowserContext(
-              profile));
-
-  feedback_data->set_context(profile);
-  feedback_data->set_description(description);
-  if (sys_info)
-    feedback_data->AddLogs(std::move(*sys_info));
-
-  if (!send_tab_titles) {
-    feedback_data->RemoveLog(
-        feedback::FeedbackReport::kMemUsageWithTabTitlesKey);
-  }
-  feedback_data->CompressSystemInfo();
-
-  GetFeedbackService(profile)->SendFeedback(feedback_data, callback);
-}
-
-}  // namespace
-
-void SendSysLogFeedback(Profile* profile,
-                        const std::string& description,
-                        const SendSysLogFeedbackCallback& callback,
-                        bool send_tab_titles) {
-  // |fetcher| deletes itself after calling its callback.
-  system_logs::SystemLogsFetcher* fetcher =
-      system_logs::BuildChromeSystemLogsFetcher(/*scrub_data=*/true);
-  fetcher->Fetch(base::BindOnce(&OnGetSystemInformation, profile, description,
-                                callback, send_tab_titles));
-}
-
-}  // namespace feedback_util

chrome/browser/feedback/feedback_util_chromeos.h

-// Copyright 2016 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_FEEDBACK_FEEDBACK_UTIL_CHROMEOS_H_
-#define CHROME_BROWSER_FEEDBACK_FEEDBACK_UTIL_CHROMEOS_H_
-
-#include <string>
-
-#include "base/callback_forward.h"
-#include "base/memory/ref_counted.h"
-
-class Profile;
-
-namespace feedback_util {
-
-// Sends a system log feedback from the given |profile| with the
-// given |description|. |callback| will be invoked when the feedback is sent.
-// If |send_tab_titles| is true, include open tab titles in the report.
-using SendSysLogFeedbackCallback = base::RepeatingCallback<void(bool)>;
-void SendSysLogFeedback(Profile* profile,
-                        const std::string& description,
-                        const SendSysLogFeedbackCallback& callback,
-                        bool send_tab_titles);
-
-}  // namespace feedback_util
-
-#endif  // CHROME_BROWSER_FEEDBACK_FEEDBACK_UTIL_CHROMEOS_H_

extensions/browser/api/feedback_private/feedback_private_api.cc

@@ -412,8 +412,8 @@ void FeedbackPrivateSendFeedbackFunction::OnAllLogsFetched(
 
   service->SendFeedback(
       feedback_data,
-      base::Bind(&FeedbackPrivateSendFeedbackFunction::OnCompleted, this,
-                 GetLandingPageType(*feedback_data)));
+      base::BindOnce(&FeedbackPrivateSendFeedbackFunction::OnCompleted, this,
+                     GetLandingPageType(*feedback_data)));
 }
 
 #if BUILDFLAG(IS_CHROMEOS_ASH)

extensions/browser/api/feedback_private/feedback_private_api_chromeos_unittest.cc

@@ -27,6 +27,7 @@ using base::TimeDelta;
 using feedback::FeedbackData;
 using testing::_;
 using testing::DoAll;
+using testing::Invoke;
 using testing::SaveArg;
 
 // Converts |params| to a string containing a JSON dictionary within an argument
@@ -126,12 +127,11 @@ class FeedbackPrivateApiUnittest : public FeedbackPrivateApiUnittestBase {
 
     scoped_refptr<FeedbackData> actual_feedback_data;
     EXPECT_CALL(*mock, SendFeedback(_, _))
-        .WillOnce(
-            DoAll(testing::SaveArg<0>(&actual_feedback_data),
-                  [](scoped_refptr<FeedbackData> feedback_data,
-                     const FeedbackService::SendFeedbackCallback& callback) {
-                    callback.Run(true);
-                  }));
+        .WillOnce(Invoke([&](scoped_refptr<FeedbackData> feedback_data,
+                             FeedbackService::SendFeedbackCallback callback) {
+          actual_feedback_data = feedback_data;
+          std::move(callback).Run(true);
+        }));
 
     FeedbackPrivateAPI::GetFactoryInstance()
         ->Get(browser_context())

extensions/browser/api/feedback_private/feedback_service.cc

@@ -37,7 +37,8 @@ FeedbackService::FeedbackService(content::BrowserContext* browser_context)
 FeedbackService::~FeedbackService() = default;
 
 void FeedbackService::SendFeedback(scoped_refptr<FeedbackData> feedback_data,
-                                   const SendFeedbackCallback& callback) {
+                                   SendFeedbackCallback callback) {
+  send_feedback_callback_ = std::move(callback);
   feedback_data->set_locale(
       ExtensionsBrowserClient::Get()->GetApplicationLocale());
   feedback_data->set_user_agent(ExtensionsBrowserClient::Get()->GetUserAgent());
@@ -45,45 +46,42 @@ void FeedbackService::SendFeedback(scoped_refptr<FeedbackData> feedback_data,
   if (!feedback_data->attached_file_uuid().empty()) {
     BlobReader::Read(browser_context_, feedback_data->attached_file_uuid(),
                      base::BindOnce(&FeedbackService::AttachedFileCallback,
-                                    AsWeakPtr(), feedback_data, callback));
+                                    AsWeakPtr(), feedback_data));
   }
 
   if (!feedback_data->screenshot_uuid().empty()) {
     BlobReader::Read(browser_context_, feedback_data->screenshot_uuid(),
                      base::BindOnce(&FeedbackService::ScreenshotCallback,
-                                    AsWeakPtr(), feedback_data, callback));
+                                    AsWeakPtr(), feedback_data));
   }
 
-  CompleteSendFeedback(feedback_data, callback);
+  CompleteSendFeedback(feedback_data);
 }
 
 void FeedbackService::AttachedFileCallback(
     scoped_refptr<feedback::FeedbackData> feedback_data,
-    const SendFeedbackCallback& callback,
     std::unique_ptr<std::string> data,
     int64_t /* total_blob_length */) {
   feedback_data->set_attached_file_uuid(std::string());
   if (data)
     feedback_data->AttachAndCompressFileData(std::move(*data));
 
-  CompleteSendFeedback(feedback_data, callback);
+  CompleteSendFeedback(feedback_data);
 }
 
 void FeedbackService::ScreenshotCallback(
     scoped_refptr<feedback::FeedbackData> feedback_data,
-    const SendFeedbackCallback& callback,
     std::unique_ptr<std::string> data,
     int64_t /* total_blob_length */) {
   feedback_data->set_screenshot_uuid(std::string());
   if (data)
     feedback_data->set_image(std::move(*data));
 
-  CompleteSendFeedback(feedback_data, callback);
+  CompleteSendFeedback(feedback_data);
 }
 
 void FeedbackService::CompleteSendFeedback(
-    scoped_refptr<feedback::FeedbackData> feedback_data,
-    const SendFeedbackCallback& callback) {
+    scoped_refptr<feedback::FeedbackData> feedback_data) {
   // A particular data collection is considered completed if,
   // a.) The blob URL is invalid - this will either happen because we never had
   //     a URL and never needed to read this data, or that the data read failed
@@ -114,7 +112,7 @@ void FeedbackService::CompleteSendFeedback(
 
     // TODO(rkc): Change this once we have FeedbackData/Util refactored to
     // report the status of the report being sent.
-    callback.Run(result);
+    std::move(send_feedback_callback_).Run(result);
   }
 }
 

extensions/browser/api/feedback_private/feedback_service.h

@@ -29,30 +29,30 @@ class FeedbackService : public base::SupportsWeakPtr<FeedbackService> {
   // True will be passed to indicate that it is being successfully sent now,
   // and false to indicate that it will be delayed (usually due to being
   // offline).
-  using SendFeedbackCallback = base::Callback<void(bool)>;
+  using SendFeedbackCallback = base::OnceCallback<void(bool)>;
 
   explicit FeedbackService(content::BrowserContext* browser_context);
   virtual ~FeedbackService();
 
   // Sends a feedback report.
   virtual void SendFeedback(scoped_refptr<feedback::FeedbackData> feedback_data,
-                            const SendFeedbackCallback& callback);
+                            SendFeedbackCallback callback);
 
  private:
   // Callbacks to receive blob data.
   void AttachedFileCallback(scoped_refptr<feedback::FeedbackData> feedback_data,
-                            const SendFeedbackCallback& callback,
                             std::unique_ptr<std::string> data,
                             int64_t total_blob_length);
   void ScreenshotCallback(scoped_refptr<feedback::FeedbackData> feedback_data,
-                          const SendFeedbackCallback& callback,
                           std::unique_ptr<std::string> data,
                           int64_t total_blob_length);
 
   // Checks if we have read all the blobs we need to; signals the feedback
   // data object once all the requisite data has been populated.
-  void CompleteSendFeedback(scoped_refptr<feedback::FeedbackData> feedback_data,
-                            const SendFeedbackCallback& callback);
+  void CompleteSendFeedback(
+      scoped_refptr<feedback::FeedbackData> feedback_data);
+
+  SendFeedbackCallback send_feedback_callback_;
 
   content::BrowserContext* browser_context_;
 

extensions/browser/api/feedback_private/mock_feedback_service.h

@@ -17,7 +17,7 @@ class MockFeedbackService : public FeedbackService {
 
   MOCK_METHOD2(SendFeedback,
                void(scoped_refptr<feedback::FeedbackData>,
-                    const FeedbackService::SendFeedbackCallback&));
+                    FeedbackService::SendFeedbackCallback));
 };
 
 }  // namespace extensions