Use NESTABLE_ASYNC APIs to get NetLog data into Tracing

net/base/trace_net_log_observer.cc

+// Copyright 2014 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 "net/base/trace_net_log_observer.h"
+
+#include <stdio.h>
+
+#include "base/debug/trace_event.h"
+#include "base/json/json_writer.h"
+#include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
+
+

[mmenke, 2014/08/15 20:42:54]

nit:  Remove extra blank line.

[xunjieli, 2014/08/15 21:55:15]

Done. Oops. Thanks!

+namespace net {
+
+namespace {
+
+class TracedValue : public base::debug::ConvertableToTraceFormat {
+ public:
+  TracedValue(scoped_ptr<base::Value> value) : value_(value.Pass()) {}
+ protected:

[mmenke, 2014/08/15 20:42:53]

nit:  Blank line before protected (And private, same goes for all your files)

[xunjieli, 2014/08/15 21:55:16]

Done for all my files. Thanks!

+  virtual void AppendAsTraceFormat(std::string* out) const OVERRIDE {
+    if (value_) {
+      std::string tmp;
+      base::JSONWriter::Write(value_.get(), &tmp);
+      *out += tmp;
+    } else {
+      *out += "\"\"";
+    }
+  }
+
+  virtual ~TracedValue() {}

[xunjieli, 2014/08/15 20:01:18]

Not very familiar with ref counted pointers, should I do anything in the
destructor?

[mmenke, 2014/08/15 20:42:53]

Don't think so, just make it protected, as you're doing.

[mmenke, 2014/08/15 20:42:53]

nit:  Destructor and constructor should go before other methods, given their
scope.

[xunjieli, 2014/08/15 21:55:16]

Done. Thanks!

+ private:
+  scoped_ptr<base::Value> value_;
+};
+} // namespace

[mmenke, 2014/08/15 20:42:53]

nit:  Two spaces between comment and code.

[mmenke, 2014/08/15 20:42:53]

nit:  Blank line before closing the namespace.

[xunjieli, 2014/08/15 21:55:15]

Done.

+
+TraceNetLogObserver::TraceNetLogObserver()
+    : log_level_(NetLog::LOG_STRIP_PRIVATE_DATA) {
+}
+
+TraceNetLogObserver::~TraceNetLogObserver() {
+}
+
+void TraceNetLogObserver::set_log_level(NetLog::LogLevel log_level) {
+  DCHECK(!net_log());

[mmenke, 2014/08/15 20:42:53]

This behavior should be mentioned in the header file.

[xunjieli, 2014/08/15 21:55:15]

Acknowledged. I have removed this setter since it is not used.

+  log_level_ = log_level;
+}
+
+void TraceNetLogObserver::StartObserving(NetLog* net_log) {
+  net_log->AddThreadSafeObserver(this, log_level_);
+}
+
+void TraceNetLogObserver::StopObserving() {
+  net_log()->RemoveThreadSafeObserver(this);
+}
+
+void TraceNetLogObserver::OnAddEntry(const NetLog::Entry& entry) {
+  if (log_level_ == NetLog::LOG_NONE) {

[mmenke, 2014/08/15 20:42:53]

not needed.  The NetLog is responsible for dealing with this.

[xunjieli, 2014/08/15 21:55:15]

Done.

+    return;
+  }
+  scoped_ptr<base::Value> value(entry.ToValue());
+  switch(entry.phase()) {
+    case (NetLog::PHASE_BEGIN):
+      TRACE_EVENT_NET_LOG_BEGIN(
+          "netlog", NetLog::EventTypeToString(entry.type()), entry.source().id,

[xunjieli, 2014/08/15 20:10:03]

Nat, if I understand correctly, the tracing category here and below will now be
"netlog,disabled-by-default-netlog.strip-private-data", right? If so, I will
update it in the next patch set.

+          "value", scoped_refptr<base::debug::ConvertableToTraceFormat>(
+              new TracedValue(value.Pass())));
+      break;
+    case (NetLog::PHASE_END):
+      TRACE_EVENT_NET_LOG_END(
+          "netlog", NetLog::EventTypeToString(entry.type()), entry.source().id,
+          "value", scoped_refptr<base::debug::ConvertableToTraceFormat>(
+              new TracedValue(value.Pass())));
+      break;
+    case (NetLog::PHASE_NONE):
+      TRACE_EVENT_NET_LOG_INSTANT(
+          "netlog", NetLog::EventTypeToString(entry.type()), entry.source().id,
+          "value", scoped_refptr<base::debug::ConvertableToTraceFormat>(
+              new TracedValue(value.Pass())));
+      break;
+    default:

[mmenke, 2014/08/15 20:42:53]

NOTREACHED()?

[xunjieli, 2014/08/15 21:55:15]

Done.

+      break;
+  }
+}
+
+void TraceNetLogObserver::WatchForTraceStart(NetLog* net_log) {
+  net_log_ = net_log;
+  base::debug::TraceLog::GetInstance()->AddEnabledStateObserver(this);
+}
+
+void TraceNetLogObserver::StopWatchForTraceStart() {
+  if (!net_log_) {
+    // We should only stop if we are currently watching.

[mmenke, 2014/08/15 20:42:53]

nit:  Don't use we in comment, because it's ambiguous.

[xunjieli, 2014/08/15 21:55:15]

Done. Thanks!

+    DCHECK(false);
+  }

[mmenke, 2014/08/15 20:42:53]

This should just be DCHECK(net_log_)

[xunjieli, 2014/08/15 21:55:15]

Done. I see. That's convenient.

+  base::debug::TraceLog::GetInstance()->RemoveEnabledStateObserver(this);
+  net_log()->RemoveThreadSafeObserver(this);

[mmenke, 2014/08/15 20:42:53]

This crashes if we're not watching a net_log.  Should be:
if (net_log())
  net_log()->...

[mmenke, 2014/08/15 20:42:54]

net_log() vs net_log_ can be confusing, maybe rename net_log_? 
(net_log_to_watch_ or something?  Open to other ideas)

[xunjieli, 2014/08/15 21:55:15]

Done.Yes, that's indeed very confusing. Thanks for pointing that out!

+}
+
+void TraceNetLogObserver::OnTraceLogEnabled() {
+  StartObserving(net_log_);
+}
+
+void TraceNetLogObserver::OnTraceLogDisabled() {
+  StopObserving();
+}
+
+}  // namespace net