Add tracing AutoOpenCloseEvent.

base/trace_event/persistent_async_event.h

+// Copyright 2016 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.
+
+#ifndef BASE_TRACE_EVENT_PERSISTENT_ASYNC_H_
+#define BASE_TRACE_EVENT_PERSISTENT_ASYNC_H_
+
+#include "base/macros.h"
+#include "base/time/time.h"
+#include "base/trace_event/trace_event.h"
+
+namespace base {
+

[Primiano Tucci (use gerrit), 2016/09/14 09:38:54]

nit: remove this extra space

[alexandermont, 2016/09/14 20:14:57]

Done

+namespace trace_event {
+
+class BASE_EXPORT PersistentAsyncEvent
+    : public trace_event::TraceLog::EnabledStateObserver {

[Primiano Tucci (use gerrit), 2016/09/14 09:38:54]

Can this object be created and destroyed on different threads? I hope not, in
which case please add a ThreadChecker to this class to prevent this.

[alexandermont, 2016/09/14 20:14:56]

Done

+ public:
+  PersistentAsyncEvent(const char* category, const char* event_name);
+  ~PersistentAsyncEvent() override;
+
+  void Begin();
+  void End();
+
+  void OnTraceLogEnabled() override;

[Primiano Tucci (use gerrit), 2016/09/14 09:38:54]

nit: add // EnabledStateObserver implementation

Also you might probably want to override these in the private section, so you
remove the temptation from clients to directly call the OnTraceLog methods

[alexandermont, 2016/09/14 20:14:57]

Added comment. How could you put these in the private section? Don't these need
to be public since the EnabledStateObserver calls them?

[Primiano Tucci (use gerrit), 2016/09/15 10:08:29]

The TraceLog calls the methods on the base class, where they are public. Making
them private here would have still made it work, as they reach here by virtual
dispatching and don't need to be really public. It's just a style and bit of
paranoy thing here.
Anyways, not sure what is the recommended style with this, so fine if you leave
them where they are. Will start a thread on chromium-dev as I'm curious at this
point :)

+  void OnTraceLogDisabled() override;
+
+ private:
+  bool active_;

[Primiano Tucci (use gerrit), 2016/09/14 09:38:54]

Active seems a redundant field here. If I read the code correctly active_ ==
(start_time_ != 0)

[alexandermont, 2016/09/14 20:14:57]

Well, actually, start_time doesn't get initialized at the beginning. It only
gets initialized after Start() is called and active_ is set.

[Primiano Tucci (use gerrit), 2016/09/15 10:08:29]

It does, implicitly.
TimeTicks is a non-POD type and its ctor explicitly zero-initializes it.
https://cs.chromium.org/chromium/src/base/time/time.h?rcl=0&l=706

active_ here seems a redundant state field.

+  void* id_;
+  const char* category_;

[Primiano Tucci (use gerrit), 2016/09/14 09:38:54]

const char* const category_; 
ditto for event_name_

[alexandermont, 2016/09/14 20:14:57]

I'll put that in there, but what does it do?

[Primiano Tucci (use gerrit), 2016/09/15 10:08:29]

const char* foo -> foo is a pointer to const char. You cannot change the content
of the pointed string but you can still change the value of the pointer itself.

const char* const foo -> foo is a pointer to const char (as ABOVE) AND the
pointer itself is const.
Practically is just a paranoid documentation thing to say: we don't expect that
the category_ changes after the constructor.
On a more practical side, it may allows the compiler to make further
optimizations, because it can rely on that value to be const forever.

+  const char* event_name_;
+  base::TimeTicks start_time_;
+};

[Primiano Tucci (use gerrit), 2016/09/14 09:38:54]

DISALLOW_COPY_AND_ASSIGN

[alexandermont, 2016/09/14 20:14:57]

Done

+
+}  // namespace trace_event
+}  // namespace base
+
+#endif  // BASE_TRACE_EVENT_PERSISTENT_ASYNC_H_