Introduce TraceLog::AsyncEnabledStateObserver

base/trace_event/trace_log.h

 // Copyright 2015 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_TRACE_LOG_H_
 #define BASE_TRACE_EVENT_TRACE_LOG_H_
 
 #include <stddef.h>
 #include <stdint.h>
 
@@ skipping 85 matching lines @@
 
 #if defined(OS_ANDROID)
   void StartATrace();
   void StopATrace();
   void AddClockSyncMetadataEvent();
 #endif
 
   // Enabled state listeners give a callback when tracing is enabled or
   // disabled. This can be used to tie into other library's tracing systems
   // on-demand.
+  // TODO(xiaochengh): Deprecate this class. Migrate all its subclasses under

[Primiano Tucci (use gerrit), 2016/05/09 17:18:29]

I will be happy if we can remove all callers to this, not 100% sure you will be
able to do that.
I wonder if there are cases where we register observers before we have a message
loop (I am thinking specifically to the chain
TracingControllerImpl::GetInstance() -> MemoryDumpManager->Initialize() ->
AddEnabledStateObserver).

The truth is that most of the observer out there are indefinitely lived
(MemoryDumpManager,TraceNetLog, TraceEventSystemStatsMonitor,
android/trace_event_binding.cc) and don't really need this.
I think this is really just for BlameContext, V8 sampling profiler. 

What I am saying is: Makes sense what you are introducing here, not sure you
will be able to deprecated this. Try, if you succeed, good. If you don't, remove
the todo :)

[Xiaocheng, 2016/05/10 09:33:35]

Yeah, a deprecation of this class seems too much.

A better idea seems to be having two observer interfaces, synchronous and
asynchronous. Only some of the existing observer classes need to be migrated to
the async interface.

+  // ThreadSafeEnabledStateObserver.
   class BASE_EXPORT EnabledStateObserver {
    public:
     virtual ~EnabledStateObserver() = default;
 
     // Called just after the tracing system becomes enabled, outside of the
     // |lock_|. TraceLog::IsEnabled() is true at this point.
     virtual void OnTraceLogEnabled() = 0;
 
     // Called just after the tracing system disables, outside of the |lock_|.
     // TraceLog::IsEnabled() is false at this point.
     virtual void OnTraceLogDisabled() = 0;
   };
   void AddEnabledStateObserver(EnabledStateObserver* listener);
   void RemoveEnabledStateObserver(EnabledStateObserver* listener);
   bool HasEnabledStateObserver(EnabledStateObserver* listener) const;
 
+  // Enabled state listeners give a callback when tracing is enabled or
+  // disabled. This can be used to tie into other library's tracing systems
+  // on-demand.

[Primiano Tucci (use gerrit), 2016/05/09 17:18:29]

I think the important information to convey to this comment is:
All observer calls are posted on the task runner where AddEnabledStateObserver
was called

[Xiaocheng, 2016/05/10 09:33:34]

Done.

+  class BASE_EXPORT ThreadSafeEnabledStateObserver {
+   public:
+    virtual ~ThreadSafeEnabledStateObserver() = default;
+
+    // Called just after the tracing system becomes enabled.
+    // TraceLog::IsEnabled() is true at this point.
+    virtual void OnTraceLogEnabled() = 0;
+
+    // Called just after the tracing system becomes disabled.
+    // TraceLog::IsEnabled() is false at this point.
+    virtual void OnTraceLogDisabled() = 0;
+  };
+  // Assigns a unique id to the listener, and returns the id to the caller.
+  int AddThreadSafeEnabledStateObserver(

[Primiano Tucci (use gerrit), 2016/05/09 17:18:29]

instea of returning an int, can we just make the TSESO* pointer the actual key
(i.e. map<TSESO*, RegisteredTSO>). So that:
when you register you can just deref the weakptr to get the key (and this is
fine as you will be on the right thread).
To unregister you just ask the class to pass you "this".

[Xiaocheng, 2016/05/10 09:33:35]

That's neat. Done.

+      WeakPtr<ThreadSafeEnabledStateObserver> listener);
+  void RemoveThreadSafeEnabledStateObserver(int listener_id);
+  bool HasThreadSafeEnabledStateObserver(int listener_id) const;
+  size_t GetThreadSafeObserverCountForTest() const;

[Primiano Tucci (use gerrit), 2016/05/09 17:18:29]

this doesn't seem to be used in the current CL

[Xiaocheng, 2016/05/10 09:33:35]

Removed. It's not even useful in test.

+
   TraceLogStatus GetStatus() const;
   bool BufferIsFull() const;
 
   // Computes an estimate of the size of the TraceLog including all the retained
   // objects.
   void EstimateTraceMemoryOverhead(TraceEventMemoryOverhead* overhead);
 
   // Not using base::Callback because of its limited by 7 parameters.
   // Also, using primitive type allows directly passing callback from WebCore.
   // WARNING: It is possible for the previously set callback to be called
@@ skipping 226 matching lines @@
 
   // Configure synthetic delays based on the values set in the current
   // trace config.
   void UpdateSyntheticDelaysFromTraceConfig();
 
   InternalTraceOptions GetInternalOptionsFromTraceConfig(
       const TraceConfig& config);
 
   class ThreadLocalEventBuffer;
   class OptionalAutoLock;
+  struct RegisteredThreadSafeObserver;
 
   TraceLog();
   ~TraceLog() override;
   const unsigned char* GetCategoryGroupEnabledInternal(const char* name);
   void AddMetadataEventsWhileLocked();
 
   InternalTraceOptions trace_options() const {
     return static_cast<InternalTraceOptions>(
         subtle::NoBarrier_Load(&trace_options_));
   }
@@ skipping 58 matching lines @@
   // and thread_colors_.
   Lock thread_info_lock_;
   Mode mode_;
   int num_traces_recorded_;
   std::unique_ptr<TraceBuffer> logged_events_;
   std::vector<std::unique_ptr<TraceEvent>> metadata_events_;
   subtle::AtomicWord /* EventCallback */ event_callback_;
   bool dispatching_to_observer_list_;
   std::vector<EnabledStateObserver*> enabled_state_observer_list_;
 
+  std::map<int, RegisteredThreadSafeObserver> thread_safe_observers_;
+
   std::string process_name_;
   base::hash_map<int, std::string> process_labels_;
   int process_sort_index_;
   base::hash_map<int, int> thread_sort_indices_;
   base::hash_map<int, std::string> thread_names_;
 
   // The following two maps are used only when ECHO_TO_CONSOLE.
   base::hash_map<int, std::stack<TimeTicks>> thread_event_start_times_;
   base::hash_map<std::string, int> thread_colors_;
 
@@ skipping 41 matching lines @@
   subtle::AtomicWord generation_;
   bool use_worker_thread_;
 
   DISALLOW_COPY_AND_ASSIGN(TraceLog);
 };
 
 }  // namespace trace_event
 }  // namespace base
 
 #endif  // BASE_TRACE_EVENT_TRACE_LOG_H_