Optimize sending sys info to the feedback app

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

 // Copyright 2013 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/extensions/api/feedback_private/feedback_private_api.h"
 
 #include <memory>
 #include <utility>
 
 #include "base/lazy_instance.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/statistics_recorder.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/extensions/api/feedback_private/feedback_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/browser/ui/simple_message_box.h"
+#include "chrome/common/extensions/api/feedback_private.h"
 #include "chrome/common/extensions/extension_constants.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/feedback/tracing_manager.h"
 #include "components/signin/core/browser/signin_manager.h"
 #include "components/strings/grit/components_strings.h"
 #include "content/public/browser/user_metrics.h"
 #include "extensions/browser/event_router.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/webui/web_ui_util.h"
 #include "url/url_util.h"
@@ skipping 32 matching lines @@
 }  // namespace
 
 namespace extensions {
 
 namespace feedback_private = api::feedback_private;
 
 using feedback_private::SystemInformation;
 using feedback_private::FeedbackInfo;
 using feedback_private::FeedbackFlow;
 
+using SystemInformationList =
+    std::vector<api::feedback_private::SystemInformation>;
+
 static base::LazyInstance<BrowserContextKeyedAPIFactory<FeedbackPrivateAPI> >
     g_factory = LAZY_INSTANCE_INITIALIZER;
 
 // static
 BrowserContextKeyedAPIFactory<FeedbackPrivateAPI>*
 FeedbackPrivateAPI::GetFactoryInstance() {
   return g_factory.Pointer();
 }
 
 FeedbackPrivateAPI::FeedbackPrivateAPI(content::BrowserContext* context)
@@ skipping 127 matching lines @@
 }
 
 ExtensionFunction::ResponseAction FeedbackPrivateGetUserEmailFunction::Run() {
   SigninManagerBase* signin_manager = SigninManagerFactory::GetForProfile(
       Profile::FromBrowserContext(browser_context()));
   return RespondNow(OneArgument(base::MakeUnique<base::StringValue>(
       signin_manager ? signin_manager->GetAuthenticatedAccountInfo().email
                      : std::string())));
 }
 
-bool FeedbackPrivateGetSystemInformationFunction::RunAsync() {
-  FeedbackService* service =
-      FeedbackPrivateAPI::GetFactoryInstance()->Get(GetProfile())->GetService();
+ExtensionFunction::ResponseAction
+FeedbackPrivateGetSystemInformationFunction::Run() {
+  FeedbackService* service = FeedbackPrivateAPI::GetFactoryInstance()
+                                 ->Get(browser_context())
+                                 ->GetService();
   DCHECK(service);
   service->GetSystemInformation(
       base::Bind(
           &FeedbackPrivateGetSystemInformationFunction::OnCompleted, this));

[xiyuan, 2017/01/17 18:38:14]

Is this Bind safe? |service| and |this| are not bound with each other.

[afakhry, 2017/01/17 18:51:13]

The ExtensionFunctions are RefCountedThreadSafe, so it should be safe.

[xiyuan, 2017/01/17 19:15:11]

Acknowledged. Forgot that. :p

-  return true;
+  return RespondLater();
 }
 
 void FeedbackPrivateGetSystemInformationFunction::OnCompleted(
-    const SystemInformationList& sys_info) {
-  results_ = feedback_private::GetSystemInformation::Results::Create(
-      sys_info);
-  SendResponse(true);
+    std::unique_ptr<system_logs::SystemLogsResponse> sys_info) {
+  SystemInformationList sys_info_list;
+  if (sys_info) {
+    sys_info_list.reserve(sys_info->size());
+    for (auto& itr : *sys_info) {
+      SystemInformation sys_info;

[xiyuan, 2017/01/17 18:38:14]

Can you rename this or |sys_info| arg of the function? It is a bit confusing to
read when two vars have the same name.

[afakhry, 2017/01/17 18:51:13]

Oops, that was a copy-paste mistake! :)

Done.

+      sys_info.key = std::move(itr.first);
+      sys_info.value = std::move(itr.second);
+      sys_info_list.emplace_back(std::move(sys_info));
+    }
+  }
+
+  Respond(ArgumentList(
+      feedback_private::GetSystemInformation::Results::Create(sys_info_list)));
 }
 
 bool FeedbackPrivateSendFeedbackFunction::RunAsync() {
   std::unique_ptr<feedback_private::SendFeedback::Params> params(
       feedback_private::SendFeedback::Params::Create(*args_));
   EXTENSION_FUNCTION_VALIDATE(params);
 
   const FeedbackInfo &feedback_info = params->feedback;
 
   // Populate feedback data.
@@ skipping 90 matching lines @@
       content::RecordAction(
           base::UserMetricsAction("Feedback.SrtPromptClosed"));
       break;
     default:
       return RespondNow(Error("Invalid arugment."));
   }
   return RespondNow(NoArguments());
 }
 
 }  // namespace extensions