Remove TraceController

content/browser/devtools/devtools_tracing_handler.cc

Index: content/browser/devtools/devtools_tracing_handler.cc
diff --git a/content/browser/devtools/devtools_tracing_handler.cc b/content/browser/devtools/devtools_tracing_handler.cc
index 0657b8e48d2a9c83cc2d171630eddcefc77913b4..701fd03cf9da4bb6b59ef7b87c0031466567e7ef 100644
--- a/content/browser/devtools/devtools_tracing_handler.cc
+++ b/content/browser/devtools/devtools_tracing_handler.cc
@@ -6,14 +6,17 @@
 
 #include "base/bind.h"
 #include "base/callback.h"
+#include "base/file_util.h"
+#include "base/json/json_reader.h"
+#include "base/json/json_writer.h"
 #include "base/location.h"
 #include "base/strings/string_split.h"
 #include "base/strings/stringprintf.h"
 #include "base/values.h"
 #include "content/browser/devtools/devtools_http_handler_impl.h"
 #include "content/browser/devtools/devtools_protocol_constants.h"
-#include "content/public/browser/trace_controller.h"
-#include "content/public/browser/trace_subscriber.h"
+#include "content/browser/tracing/tracing_controller_impl.h"
+#include "content/public/browser/browser_thread.h"
 
 namespace content {
 
@@ -26,7 +29,8 @@ const char kEnableSampling[] = "enable-sampling";
 }  // namespace
 
 DevToolsTracingHandler::DevToolsTracingHandler()
-    : is_running_(false) {
+    : is_running_(false),
+      weak_factory_(this) {
   RegisterCommandHandler(devtools::Tracing::start::kName,
                          base::Bind(&DevToolsTracingHandler::OnStart,
                                     base::Unretained(this)));
@@ -38,28 +42,66 @@ DevToolsTracingHandler::DevToolsTracingHandler()
 DevToolsTracingHandler::~DevToolsTracingHandler() {
 }
 
-void DevToolsTracingHandler::OnEndTracingComplete() {
+void DevToolsTracingHandler::ReadRecordingResult(const base::FilePath& path) {

[pfeldman, 2013/11/20 00:10:10]

Please make sure order of methods in cc matches the one in the header.

[Xianzhu, 2013/11/20 00:37:22]

Done.

+  if (!is_running_)
+    return;
+
+  std::string trace_data;
+  if (!base::ReadFileToString(path, &trace_data)) {

[pfeldman, 2013/11/20 00:10:10]

Why do we need to deal with files in the handler?

[Xianzhu, 2013/11/20 00:37:22]

This is how the new TracingController interface is defined. I didn't contribute
discussion when defining the interface, but I think the purpose is to simplify
most of the clients (single callback, avoid passing big data through IPC, etc.)

Here the code becomes more complex because we need to conform to the original
devtools protocol. My next step work for
https://code.google.com/p/trace-viewer/issues/detail?id=414) will use and change
the protocol and simplify code here.

BTW, is there already existing client of the protocol about tracing?

+    LOG(ERROR) << "Failed to read file: " << path.value();
+    OnTraceDataCollected("");
+  } else {
+    scoped_ptr<base::Value> trace_value(base::JSONReader::Read(trace_data));
+    DictionaryValue* dictionary = NULL;
+    bool ok = trace_value->GetAsDictionary(&dictionary);
+    DCHECK(ok);
+    ListValue* list = NULL;
+    ok = dictionary->GetList("traceEvents", &list);
+    DCHECK(ok);
+    std::string buffer;
+    for (size_t i = 0; i < list->GetSize(); ++i) {
+      std::string item;
+      base::Value* item_value;
+      list->Get(i, &item_value);
+      base::JSONWriter::Write(item_value, &item);
+      if (buffer.size())
+        buffer.append(",");
+      buffer.append(item);
+      if (i % 1000 == 0) {
+        OnTraceDataCollected(buffer);
+        buffer.clear();
+      }
+    }
+    if (buffer.size())
+      OnTraceDataCollected(buffer);
+  }
+  base::DeleteFile(path, false);
+
   is_running_ = false;

[pfeldman, 2013/11/20 00:10:10]

Do we still need is_running_?

[Xianzhu, 2013/11/20 00:37:22]

Seems useless (even in the original code). Removed.

   SendNotification(devtools::Tracing::tracingComplete::kName, NULL);
 }
 
 void DevToolsTracingHandler::OnTraceDataCollected(
-    const scoped_refptr<base::RefCountedString>& trace_fragment) {
-  if (is_running_) {
-    // Hand-craft protocol notification message so we can substitute JSON
-    // that we already got as string as a bare object, not a quoted string.
-    std::string message = base::StringPrintf(
-        "{ \"method\": \"%s\", \"params\": { \"%s\": [ %s ] } }",
-        devtools::Tracing::dataCollected::kName,
-        devtools::Tracing::dataCollected::kValue,
-        trace_fragment->data().c_str());
-    SendRawMessage(message);
-  }
+    const std::string& trace_fragment) {
+  // Hand-craft protocol notification message so we can substitute JSON
+  // that we already got as string as a bare object, not a quoted string.
+  std::string message = base::StringPrintf(
+      "{ \"method\": \"%s\", \"params\": { \"%s\": [ %s ] } }",
+      devtools::Tracing::dataCollected::kName,
+      devtools::Tracing::dataCollected::kValue,
+      trace_fragment.c_str());
+  SendRawMessage(message);
 }
 
-// Note, if you add more options here you also need to update:
-// base/debug/trace_event_impl:TraceOptionsFromString
-base::debug::TraceLog::Options DevToolsTracingHandler::TraceOptionsFromString(
+void DevToolsTracingHandler::BeginReadingRecordingResult(
+    const base::FilePath& path) {
+  BrowserThread::PostTask(
+      BrowserThread::FILE, FROM_HERE,
+      base::Bind(&DevToolsTracingHandler::ReadRecordingResult,
+                 weak_factory_.GetWeakPtr(), path));
+}
+
+TracingController::Options DevToolsTracingHandler::TraceOptionsFromString(
     const std::string& options) {
   std::vector<std::string> split;
   std::vector<std::string>::iterator iter;
@@ -68,18 +110,14 @@ base::debug::TraceLog::Options DevToolsTracingHandler::TraceOptionsFromString(
   base::SplitString(options, ',', &split);
   for (iter = split.begin(); iter != split.end(); ++iter) {
     if (*iter == kRecordUntilFull) {
-      ret |= base::debug::TraceLog::RECORD_UNTIL_FULL;
+      ret &= ~TracingController::RECORD_CONTINUOUSLY;

[pfeldman, 2013/11/20 00:10:10]

Is this change intentional?

[Xianzhu, 2013/11/20 00:37:22]

Yes. TracingController::Options doesn't have RECORD_UNTIL_FULL, but imply it by
default. Here just ensures the RECORD_CONTINUOUSLY bit is cleared if
kRecordUntilFull is specified (in case kRecordContinuously is specified
eariler). Can also be a no-op.

Will change the code here when changing the devtools tracing protocol.

     } else if (*iter == kRecordContinuously) {
-      ret |= base::debug::TraceLog::RECORD_CONTINUOUSLY;
+      ret |= TracingController::RECORD_CONTINUOUSLY;
     } else if (*iter == kEnableSampling) {
-      ret |= base::debug::TraceLog::ENABLE_SAMPLING;
+      ret |= TracingController::ENABLE_SAMPLING;
     }
   }
-  if (!(ret & base::debug::TraceLog::RECORD_UNTIL_FULL) &&
-      !(ret & base::debug::TraceLog::RECORD_CONTINUOUSLY))
-    ret |= base::debug::TraceLog::RECORD_UNTIL_FULL;
-
-  return static_cast<base::debug::TraceLog::Options>(ret);
+  return static_cast<TracingController::Options>(ret);
 }
 
 scoped_refptr<DevToolsProtocol::Response>
@@ -90,15 +128,16 @@ DevToolsTracingHandler::OnStart(
   if (params)
     params->GetString(devtools::Tracing::start::kCategories, &categories);
 
-  base::debug::TraceLog::Options options =
-      base::debug::TraceLog::RECORD_UNTIL_FULL;
+  TracingController::Options options = TracingController::DEFAULT_OPTIONS;
   if (params && params->HasKey(devtools::Tracing::start::kTraceOptions)) {
     std::string options_param;
     params->GetString(devtools::Tracing::start::kTraceOptions, &options_param);
     options = TraceOptionsFromString(options_param);
   }
 
-  TraceController::GetInstance()->BeginTracing(this, categories, options);
+  TracingController::GetInstance()->EnableRecording(
+      categories, options,
+      TracingController::EnableRecordingDoneCallback());
   is_running_ = true;
   return command->SuccessResponse(NULL);
 }
@@ -106,7 +145,10 @@ DevToolsTracingHandler::OnStart(
 scoped_refptr<DevToolsProtocol::Response>
 DevToolsTracingHandler::OnEnd(
     scoped_refptr<DevToolsProtocol::Command> command) {
-  TraceController::GetInstance()->EndTracingAsync(this);
+  TracingController::GetInstance()->DisableRecording(
+      base::FilePath(),
+      base::Bind(&DevToolsTracingHandler::BeginReadingRecordingResult,
+                 weak_factory_.GetWeakPtr()));
   return command->SuccessResponse(NULL);
 }