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"

[mmenke, 2014/08/21 15:35:40]

nit:  Probably safe to leave this out, since header includes trace_event_impl.

[xunjieli, 2014/08/22 17:20:09]

Done. Thanks!

+#include "base/json/json_writer.h"
+#include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
+
+namespace net {
+
+namespace {
+
+class TracedValue : public base::debug::ConvertableToTraceFormat {
+ public:
+  explicit TracedValue(scoped_ptr<base::Value> value) : value_(value.Pass()) {}
+
+ protected:

[mmenke, 2014/08/21 15:35:39]

Suggest either making these private or public - protected means subclasses can't
call them, but since they're overriding public functions in the parent class,
the can still be called when using an instance as a member of the parent class.

[xunjieli, 2014/08/22 17:20:09]

Done. I see. Thank you! 

On 2014/08/21 15:35:39, mmenke wrote:

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

[mmenke, 2014/08/21 15:35:39]

nit:  Blank line above end of namespace.

[xunjieli, 2014/08/22 17:20:10]

Done. Thanks! I will keep this in mind next time!

+
+TraceNetLogObserver::TraceNetLogObserver():
+    log_level_(NetLog::LOG_STRIP_PRIVATE_DATA) {

[mmenke, 2014/08/21 15:35:40]

Initialize net_log_to_watch_ to NULL.

[xunjieli, 2014/08/22 17:20:09]

Done.

+}
+
+TraceNetLogObserver::~TraceNetLogObserver() {

[mmenke, 2014/08/21 15:35:39]

Maybe:

DCHECK(!net_log_to_watch_);
DCHECK(!net_log());

I suggest a change below that makes the first one always the case.

[xunjieli, 2014/08/22 17:20:09]

Done.

+}
+
+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) {
+  scoped_ptr<base::Value> value(entry.ToValue());
+  switch (entry.phase()) {
+    case(NetLog::PHASE_BEGIN):

[mmenke, 2014/08/21 15:35:40]

nit:  case NetLog::PHASE_BEGIN:

(Same goes for the other cases as well)

[xunjieli, 2014/08/22 17:20:09]

Done.Thanks!

+      TRACE_EVENT_NET_LOG_BEGIN(

[mmenke, 2014/08/21 15:35:39]

If this is only for NetLog events, then the macro can include "netlog" and
"value" baked into it, unless there's a reason not to do so.

[xunjieli, 2014/08/22 17:20:09]

Done.

+          "netlog", NetLog::EventTypeToString(entry.type()), entry.source().id,
+          "value", scoped_refptr<base::debug::ConvertableToTraceFormat>(
+              new TracedValue(value.Pass())));

[mmenke, 2014/08/21 15:35:39]

Optional:  This is a bit redundant.  Value includes phase, event type, and
source id.

The other things it contains are time (Which tracing already has), source type,
and parameters.  To reduce redundancy (And size of the JSON data), may make more
sense to make source type arg1, and make the entry.ParametersToValue() arg2.

[xunjieli, 2014/08/22 17:20:10]

Done. This is a good idea! Thanks!
On 2014/08/21 15:35:39, mmenke wrote:

+      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:
+      NOTREACHED();

[mmenke, 2014/08/21 15:35:39]

Get rid of default path.  Clang is configured to emit a compiler failure when a
switch statement uses an enum, there's no default branch, and some values of the
enum don't have a case.  So without the default, we get a compile failure,
instead of a debug-build-only runtime check.

[xunjieli, 2014/08/22 17:20:09]

Do you mean not doing anything in the default branch? 

On 2014/08/21 15:35:39, mmenke wrote:

+  }
+}
+
+void TraceNetLogObserver::WatchForTraceStart(NetLog* net_log) {
+  net_log_to_watch_ = net_log;
+  base::debug::TraceLog::GetInstance()->AddEnabledStateObserver(this);
+}
+
+void TraceNetLogObserver::StopWatchForTraceStart() {
+  // Should only stop if is currently watching.
+  DCHECK(net_log_to_watch_);
+  base::debug::TraceLog::GetInstance()->RemoveEnabledStateObserver(this);
+  if (net_log())
+    net_log()->RemoveThreadSafeObserver(this);
+}

[mmenke, 2014/08/21 15:35:39]

net_log_to_watch_ = NULL?

[xunjieli, 2014/08/22 17:20:09]

Done.

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