TracingController: Simplify public API

Cleaned up the TraceController public API a little bit by removing the metadata DictionaryValue which is now inlined in the JSON rather than treated as separate data, and hold the tracing data in a std::unique_ptr<std::string> rather than a RefCountedString as the data only has one owner at any time. Split out from and already LGTM'd (the tracing bits) in https://chromium-review.googlesource.com/c/chromium/src/+/1817207/28 TBR=ssid@chromium.org,zork@chromium.org,erikchen@chromium.org,boliu@chromium.org Bug: 839086 Change-Id: I84a717e7c6915a81348d7e91fbd6fe2ba00a4d46 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1834576 Commit-Queue: oysteine <oysteine@chromium.org> Reviewed-by: oysteine <oysteine@chromium.org> Reviewed-by: ssid <ssid@chromium.org> Reviewed-by: Avi Drissman <avi@chromium.org> Cr-Commit-Position: refs/heads/master@{#702185}
qweri0p · Oct 2, 2019 · 2386e05 · 2386e05
1 parent d31a632
commit 2386e05
Show file tree

Hide file tree

Showing 29 changed files with 205 additions and 357 deletions.
diff --git a/android_webview/browser/tracing/aw_tracing_controller.cc b/android_webview/browser/tracing/aw_tracing_controller.cc
@@ -33,21 +33,17 @@ class AwTraceDataEndpoint
  public:
   using ReceivedChunkCallback =
       base::RepeatingCallback<void(std::unique_ptr<std::string>)>;
-  using CompletedCallback =
-      base::OnceCallback<void(std::unique_ptr<const base::DictionaryValue>)>;
 
   static scoped_refptr<content::TracingController::TraceDataEndpoint> Create(
       ReceivedChunkCallback received_chunk_callback,
-      CompletedCallback completed_callback) {
+      base::OnceClosure completed_callback) {
     return new AwTraceDataEndpoint(std::move(received_chunk_callback),
                                    std::move(completed_callback));
   }
 
-  void ReceiveTraceFinalContents(
-      std::unique_ptr<const base::DictionaryValue> metadata) override {
-    base::PostTask(
-        FROM_HERE, {content::BrowserThread::UI},
-        base::BindOnce(std::move(completed_callback_), std::move(metadata)));
+  void ReceivedTraceFinalContents() override {
+    base::PostTask(FROM_HERE, {content::BrowserThread::UI},
+                   base::BindOnce(std::move(completed_callback_)));
   }
 
   void ReceiveTraceChunk(std::unique_ptr<std::string> chunk) override {
@@ -56,15 +52,15 @@ class AwTraceDataEndpoint
   }
 
   explicit AwTraceDataEndpoint(ReceivedChunkCallback received_chunk_callback,
-                               CompletedCallback completed_callback)
+                               base::OnceClosure completed_callback)
       : received_chunk_callback_(std::move(received_chunk_callback)),
         completed_callback_(std::move(completed_callback)) {}
 
  private:
   ~AwTraceDataEndpoint() override {}
 
   ReceivedChunkCallback received_chunk_callback_;
-  CompletedCallback completed_callback_;
+  base::OnceClosure completed_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(AwTraceDataEndpoint);
 };
@@ -108,8 +104,7 @@ bool AwTracingController::StopAndFlush(JNIEnv* env,
                          weak_factory_.GetWeakPtr())));
 }
 
-void AwTracingController::OnTraceDataComplete(
-    std::unique_ptr<const base::DictionaryValue> metadata) {
+void AwTracingController::OnTraceDataComplete() {
   JNIEnv* env = base::android::AttachCurrentThread();
   base::android::ScopedJavaLocalRef<jobject> obj = weak_java_object_.get(env);
   if (obj.obj()) {

diff --git a/android_webview/browser/tracing/aw_tracing_controller.h b/android_webview/browser/tracing/aw_tracing_controller.h
@@ -8,7 +8,6 @@
 #include "base/android/jni_weak_ref.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
-#include "base/values.h"
 
 namespace android_webview {
 
@@ -28,8 +27,7 @@ class AwTracingController {
   ~AwTracingController();
 
   void OnTraceDataReceived(std::unique_ptr<std::string> chunk);
-  void OnTraceDataComplete(
-      std::unique_ptr<const base::DictionaryValue> metadata);
+  void OnTraceDataComplete();
 
   JavaObjectWeakGlobalRef weak_java_object_;
   base::WeakPtrFactory<AwTracingController> weak_factory_{this};

diff --git a/chrome/browser/tracing/background_tracing_field_trial.cc b/chrome/browser/tracing/background_tracing_field_trial.cc
@@ -25,6 +25,7 @@
 #include "content/public/browser/browser_thread.h"
 #include "net/base/network_change_notifier.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
+#include "services/tracing/public/cpp/perfetto/trace_event_data_source.h"
 #include "url/gurl.h"
 
 namespace tracing {
@@ -48,8 +49,7 @@ void OnBackgroundTracingUploadComplete(
 
 void BackgroundTracingUploadCallback(
     const std::string& upload_url,
-    const scoped_refptr<base::RefCountedString>& file_contents,
-    std::unique_ptr<const base::DictionaryValue> metadata,
+    std::unique_ptr<std::string> file_contents,
     content::BackgroundTracingManager::FinishedProcessingCallback callback) {
   TraceCrashServiceUploader* uploader = new TraceCrashServiceUploader(
       g_browser_process->shared_url_loader_factory());
@@ -66,6 +66,8 @@ void BackgroundTracingUploadCallback(
     uploader->SetMaxUploadBytes(100 * 1024);
   }
 #endif
+  std::unique_ptr<base::DictionaryValue> metadata =
+      TraceEventMetadataSource::GetInstance()->GenerateLegacyMetadataDict();
 
   uploader->DoUpload(
       file_contents->data(), content::TraceUploader::UNCOMPRESSED_UPLOAD,

diff --git a/chrome/browser/tracing/background_tracing_metrics_provider_unittest.cc b/chrome/browser/tracing/background_tracing_metrics_provider_unittest.cc
@@ -44,8 +44,7 @@ class BackgroundTracingMetricsProviderTest : public testing::Test {
     ASSERT_TRUE(
         content::BackgroundTracingManager::GetInstance()->SetActiveScenario(
             std::move(config),
-            base::BindRepeating([](const scoped_refptr<base::RefCountedString>&,
-                                   std::unique_ptr<const base::DictionaryValue>,
+            base::BindRepeating([](std::unique_ptr<std::string>,
                                    content::BackgroundTracingManager::
                                        FinishedProcessingCallback) {}),
             content::BackgroundTracingManager::ANONYMIZE_DATA));

diff --git a/chrome/browser/tracing/chrome_tracing_delegate_browsertest.cc b/chrome/browser/tracing/chrome_tracing_delegate_browsertest.cc
@@ -98,8 +98,7 @@ class ChromeTracingDelegateBrowserTest : public InProcessBrowserTest {
   }
 
  private:
-  void OnUpload(const scoped_refptr<base::RefCountedString>& file_contents,
-                std::unique_ptr<const base::DictionaryValue> metadata,
+  void OnUpload(std::unique_ptr<std::string> file_contents,
                 content::BackgroundTracingManager::FinishedProcessingCallback
                     done_callback) {
     receive_count_ += 1;

diff --git a/chrome/browser/tracing/navigation_tracing.cc b/chrome/browser/tracing/navigation_tracing.cc
@@ -18,6 +18,7 @@
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/render_frame_host.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
+#include "services/tracing/public/cpp/perfetto/trace_event_data_source.h"
 
 using content::RenderFrameHost;
 
@@ -37,12 +38,14 @@ void OnNavigationTracingUploadComplete(
 }
 
 void NavigationUploadCallback(
-    const scoped_refptr<base::RefCountedString>& file_contents,
-    std::unique_ptr<const base::DictionaryValue> metadata,
+    std::unique_ptr<std::string> file_contents,
     content::BackgroundTracingManager::FinishedProcessingCallback callback) {
   TraceCrashServiceUploader* uploader = new TraceCrashServiceUploader(
       g_browser_process->shared_url_loader_factory());
 
+  std::unique_ptr<base::DictionaryValue> metadata =
+      TraceEventMetadataSource::GetInstance()->GenerateLegacyMetadataDict();
+
   uploader->DoUpload(
       file_contents->data(), content::TraceUploader::UNCOMPRESSED_UPLOAD,
       std::move(metadata), content::TraceUploader::UploadProgressCallback(),
@@ -93,7 +96,7 @@ void SetupNavigationTracing() {
   DCHECK(config);
 
   content::BackgroundTracingManager::GetInstance()->SetActiveScenario(
-      std::move(config), base::Bind(&NavigationUploadCallback),
+      std::move(config), base::BindRepeating(&NavigationUploadCallback),
       content::BackgroundTracingManager::NO_DATA_FILTERING);
 }
 

diff --git a/chrome/browser/ui/webui/chromeos/arc_graphics_tracing/arc_graphics_tracing_handler.cc b/chrome/browser/ui/webui/chromeos/arc_graphics_tracing/arc_graphics_tracing_handler.cc
@@ -441,8 +441,8 @@ void ArcGraphicsTracingHandler::StopTracing() {
     return;
 
   controller->StopTracing(content::TracingController::CreateStringEndpoint(
-      base::BindRepeating(&ArcGraphicsTracingHandler::OnTracingStopped,
-                          weak_ptr_factory_.GetWeakPtr())));
+      base::BindOnce(&ArcGraphicsTracingHandler::OnTracingStopped,
+                     weak_ptr_factory_.GetWeakPtr())));
 }
 
 void ArcGraphicsTracingHandler::SetStatus(const std::string& status) {
@@ -459,10 +459,9 @@ void ArcGraphicsTracingHandler::OnTracingStarted() {
 }
 
 void ArcGraphicsTracingHandler::OnTracingStopped(
-    std::unique_ptr<const base::DictionaryValue> metadata,
-    base::RefCountedString* trace_data) {
+    std::unique_ptr<std::string> trace_data) {
   std::string string_data;
-  string_data.swap(trace_data->data());
+  string_data.swap(*trace_data);
   base::PostTaskAndReplyWithResult(
       FROM_HERE,
       {base::ThreadPool(), base::MayBlock(), base::TaskPriority::BEST_EFFORT},

diff --git a/chrome/browser/ui/webui/chromeos/arc_graphics_tracing/arc_graphics_tracing_handler.h b/chrome/browser/ui/webui/chromeos/arc_graphics_tracing/arc_graphics_tracing_handler.h
@@ -24,7 +24,6 @@ class ArcSystemStatCollector;
 
 namespace base {
 class ListValue;
-class RefCountedString;
 }  // namespace base
 
 namespace exo {
@@ -71,8 +70,7 @@ class ArcGraphicsTracingHandler : public content::WebUIMessageHandler,
   void SetStatus(const std::string& status);
 
   void OnTracingStarted();
-  void OnTracingStopped(std::unique_ptr<const base::DictionaryValue> metadata,
-                        base::RefCountedString* trace_data);
+  void OnTracingStopped(std::unique_ptr<std::string> trace_data);
 
   // Called when graphics model is built or load. Extra string parameter
   // contains a status. In case model cannot be built/load empty |base::Value|

diff --git a/chrome/test/base/tracing.cc b/chrome/test/base/tracing.cc
@@ -36,8 +36,7 @@ class StringTraceEndpoint
     *result_ += *chunk;
   }
 
-  void ReceiveTraceFinalContents(
-      std::unique_ptr<const base::DictionaryValue> metadata) override {
+  void ReceivedTraceFinalContents() override {
     if (!result_->empty())
       *result_ += "]";
     completion_callback_.Run();

diff --git a/components/feedback/tracing_manager.cc b/components/feedback/tracing_manager.cc
@@ -43,8 +43,8 @@ int TracingManager::RequestTrace() {
   ++g_next_trace_id;
   content::TracingController::GetInstance()->StopTracing(
       content::TracingController::CreateStringEndpoint(
-          base::Bind(&TracingManager::OnTraceDataCollected,
-                     weak_ptr_factory_.GetWeakPtr())));
+          base::BindOnce(&TracingManager::OnTraceDataCollected,
+                         weak_ptr_factory_.GetWeakPtr())));
   return current_trace_id_;
 }
 
@@ -93,14 +93,13 @@ void TracingManager::StartTracing() {
 }
 
 void TracingManager::OnTraceDataCollected(
-    std::unique_ptr<const base::DictionaryValue> metadata,
-    base::RefCountedString* trace_data) {
+    std::unique_ptr<std::string> trace_data) {
   if (!current_trace_id_)
     return;
 
   std::string output_val;
-  feedback_util::ZipString(
-      base::FilePath(kTracingFilename), trace_data->data(), &output_val);
+  feedback_util::ZipString(base::FilePath(kTracingFilename), *trace_data,
+                           &output_val);
 
   scoped_refptr<base::RefCountedString> output(
       base::RefCountedString::TakeString(&output_val));

diff --git a/components/feedback/tracing_manager.h b/components/feedback/tracing_manager.h
@@ -58,9 +58,7 @@ class TracingManager {
   TracingManager();
 
   void StartTracing();
-  void OnTraceDataCollected(
-      std::unique_ptr<const base::DictionaryValue> metadata,
-      base::RefCountedString* data);
+  void OnTraceDataCollected(std::unique_ptr<std::string> data);
 
   // ID of the trace that is being collected.
   int current_trace_id_;

diff --git a/components/heap_profiling/supervisor.cc b/components/heap_profiling/supervisor.cc
@@ -127,12 +127,11 @@ void Supervisor::RequestTraceWithHeapDump(TraceFinishedCallback callback,
   auto finished_dump_callback = base::BindOnce(
       [](TraceFinishedCallback callback, bool success, uint64_t dump_guid) {
         // Once the trace has stopped, run |callback| on the UI thread.
-        auto finish_sink_callback = base::Bind(
+        auto finish_sink_callback = base::BindOnce(
             [](TraceFinishedCallback callback,
-               std::unique_ptr<const base::DictionaryValue> metadata,
-               base::RefCountedString* in) {
+               std::unique_ptr<std::string> in) {
               std::string result;
-              result.swap(in->data());
+              result.swap(*in);
               base::CreateSingleThreadTaskRunner({content::BrowserThread::UI})
                   ->PostTask(FROM_HERE,
                              base::BindOnce(std::move(callback), true,

diff --git a/content/browser/devtools/protocol/tracing_handler.cc b/content/browser/devtools/protocol/tracing_handler.cc
@@ -122,8 +122,7 @@ class DevToolsTraceEndpointProxy : public TracingController::TraceDataEndpoint {
     if (TracingHandler* h = tracing_handler_.get())
       h->OnTraceDataCollected(std::move(chunk));
   }
-  void ReceiveTraceFinalContents(
-      std::unique_ptr<const base::DictionaryValue> metadata) override {
+  void ReceivedTraceFinalContents() override {
     if (TracingHandler* h = tracing_handler_.get())
       h->OnTraceComplete();
   }
@@ -152,13 +151,12 @@ class DevToolsStreamEndpoint : public TracingController::TraceDataEndpoint {
     stream_->Append(std::move(chunk));
   }
 
-  void ReceiveTraceFinalContents(
-      std::unique_ptr<const base::DictionaryValue> metadata) override {
+  void ReceivedTraceFinalContents() override {
     if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
       base::PostTask(
           FROM_HERE, {BrowserThread::UI},
-          base::BindOnce(&DevToolsStreamEndpoint::ReceiveTraceFinalContents,
-                         this, std::move(metadata)));
+          base::BindOnce(&DevToolsStreamEndpoint::ReceivedTraceFinalContents,
+                         this));
       return;
     }
     if (TracingHandler* h = tracing_handler_.get())
@@ -324,7 +322,7 @@ class TracingHandler::PerfettoTracingSession
     if (!tracing_session_host_) {
       if (endpoint_) {
         // Will delete |this|.
-        endpoint_->ReceiveTraceFinalContents(nullptr);
+        endpoint_->ReceivedTraceFinalContents();
       }
       return;
     }
@@ -445,7 +443,7 @@ class TracingHandler::PerfettoTracingSession
     if (endpoint_) {
       // TODO(oysteine): Signal to the client that tracing failed.
       // Will delete |this|.
-      endpoint_->ReceiveTraceFinalContents(nullptr);
+      endpoint_->ReceivedTraceFinalContents();
     }
   }
 
@@ -492,7 +490,7 @@ class TracingHandler::PerfettoTracingSession
     if (!endpoint_)
       return;
     // Will delete |this|.
-    endpoint_->ReceiveTraceFinalContents(nullptr);
+    endpoint_->ReceivedTraceFinalContents();
   }
 
   mojo::Binding<tracing::mojom::TracingSessionClient> binding_{this};
@@ -517,11 +515,11 @@ class TracingHandler::PerfettoTracingSession
 #endif
 };
 
-TracingHandler::TracingHandler(FrameTreeNode* frame_tree_node_,
+TracingHandler::TracingHandler(FrameTreeNode* frame_tree_node,
                                DevToolsIOContext* io_context)
     : DevToolsDomainHandler(Tracing::Metainfo::domainName),
       io_context_(io_context),
-      frame_tree_node_(frame_tree_node_),
+      frame_tree_node_(frame_tree_node),
       did_initiate_recording_(false),
       return_as_stream_(false),
       gzip_compression_(false),

diff --git a/content/browser/renderer_host/input/mouse_latency_browsertest.cc b/content/browser/renderer_host/input/mouse_latency_browsertest.cc
@@ -165,11 +165,9 @@ class MouseLatencyBrowserTest : public ContentBrowserTest {
     runner_->Quit();
   }
 
-  void OnTraceDataCollected(
-      std::unique_ptr<const base::DictionaryValue> metadata,
-      base::RefCountedString* trace_data_string) {
+  void OnTraceDataCollected(std::unique_ptr<std::string> trace_data_string) {
     std::unique_ptr<base::Value> trace_data =
-        base::JSONReader::ReadDeprecated(trace_data_string->data());
+        base::JSONReader::ReadDeprecated(*trace_data_string);
     ASSERT_TRUE(trace_data);
     trace_data_ = trace_data->Clone();
     runner_->Quit();
@@ -264,8 +262,8 @@ class MouseLatencyBrowserTest : public ContentBrowserTest {
   const base::Value& StopTracing() {
     bool success = TracingController::GetInstance()->StopTracing(
         TracingController::CreateStringEndpoint(
-            base::Bind(&MouseLatencyBrowserTest::OnTraceDataCollected,
-                       base::Unretained(this))));
+            base::BindOnce(&MouseLatencyBrowserTest::OnTraceDataCollected,
+                           base::Unretained(this))));
     EXPECT_TRUE(success);
 
     // Runs until we get the OnTraceDataCollected callback, which populates