Introduce TraceLog::AsyncEnabledStateObserver

base/trace_event/trace_event_unittest.cc

Index: base/trace_event/trace_event_unittest.cc
diff --git a/base/trace_event/trace_event_unittest.cc b/base/trace_event/trace_event_unittest.cc
index e626a779ed5e33cc5b1eb8dedac1435c0984b282..89811bdd2f63f8a6072c0b7216d009014f0084b4 100644
--- a/base/trace_event/trace_event_unittest.cc
+++ b/base/trace_event/trace_event_unittest.cc
@@ -1165,6 +1165,207 @@ TEST_F(TraceEventTestFixture, SelfRemovingObserver) {
   EXPECT_EQ(0u, TraceLog::GetInstance()->GetObserverCountForTest());
 }
 
+// A helper class for synchronous manipulation of an AsyncObserver object in
+// another thread.
+// AsyncObserver must be a subclass of TraceLog::AsyncEnabledStateObserver
+// with a GetWeakPtr() member function returning a WeakPtr of itself.
+template <typename AsyncObserver>
+class AsyncTraceEventHelper {
+ public:
+  AsyncTraceEventHelper()
+      : thread_("Owner of observer"),
+        observer_(nullptr),
+        async_indicator_(false),
+        cv_(&lock_) {
+    CHECK(thread_.Start());
+    CHECK(thread_.message_loop());
+    PerformRemoteTask(&AsyncTraceEventHelper::CreateObserver);
+  }
+
+  ~AsyncTraceEventHelper() {
+    PerformRemoteTask(&AsyncTraceEventHelper::DestroyObserver);
+  }
+
+  void PerformRemoteTask(void (AsyncTraceEventHelper::*task)(void)) {
+    AutoLock lock(lock_);
+    async_indicator_ = false;
+    thread_.task_runner()->PostTask(
+        FROM_HERE, Bind(&AsyncTraceEventHelper::RemoteTaskWrapper,
+                        Unretained(this), task));
+    while (!async_indicator_)

[Primiano Tucci (use gerrit), 2016/05/10 14:20:26]

hmm don't do this, I think that if this is not volatile some compilers with /o2
might optimize this as a while(true). And we don't want to start putting
volatile code.
I'd just simplify this test. I think you are overshooting with this test :)
Given that you are introductin a rather simple feature, the only think we should
cover is whether the observer is called and whether is called on the right
thread and that basic unregristration works. Which should be doable using no
subtle atomic / wait conditions etc.
I think this is just risking to introduce a flaky test

+      cv_.Wait();
+  }
+
+  void RemoteTaskWrapper(void (AsyncTraceEventHelper::*task)(void)) {
+    AutoLock lock(lock_);
+    (this->*task)();
+    async_indicator_ = true;
+    cv_.Signal();
+  }
+
+  AsyncObserver* observer() { return observer_; }
+
+  void Nop() {}
+  void CreateObserver() { observer_ = new AsyncObserver; }
+  void DestroyObserver() { delete observer_; }
+  void AddObserverToTraceLog() {
+    TraceLog::GetInstance()->AddAsyncEnabledStateObserver(
+        observer_->GetWeakPtr());
+  }
+  void RemoveObserverFromTraceLog() {
+    TraceLog::GetInstance()->RemoveAsyncEnabledStateObserver(observer_);
+  }
+
+ private:
+  Thread thread_;
+  AsyncObserver* observer_;  // Owned, managed remotely and manually in thread_.
+
+  bool async_indicator_;
+  Lock lock_;
+  ConditionVariable cv_;
+};
+
+class MockAsyncEnabledStateChangedObserver
+    : public TraceLog::AsyncEnabledStateObserver {
+ public:
+  MOCK_METHOD0(OnTraceLogEnabled, void());
+  MOCK_METHOD0(OnTraceLogDisabled, void());
+  MockAsyncEnabledStateChangedObserver() : weak_factory_(this) {}
+  WeakPtr<MockAsyncEnabledStateChangedObserver> GetWeakPtr() {
+    return weak_factory_.GetWeakPtr();
+  }
+
+ private:
+  WeakPtrFactory<MockAsyncEnabledStateChangedObserver> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(MockAsyncEnabledStateChangedObserver);
+};
+
+TEST_F(TraceEventTestFixture, AsyncEnabledObserverFiresOnEnable) {
+  typedef AsyncTraceEventHelper<MockAsyncEnabledStateChangedObserver> Helper;
+  Helper helper;
+  helper.PerformRemoteTask(&Helper::AddObserverToTraceLog);
+
+  EXPECT_CALL(*helper.observer(), OnTraceLogEnabled()).Times(1);
+  TraceLog::GetInstance()->SetEnabled(TraceConfig(kRecordAllCategoryFilter, ""),
+                                      TraceLog::RECORDING_MODE);
+  helper.PerformRemoteTask(&Helper::Nop);
+  testing::Mock::VerifyAndClear(helper.observer());
+  EXPECT_TRUE(TraceLog::GetInstance()->IsEnabled());
+
+  // Cleanup.
+  helper.PerformRemoteTask(&Helper::RemoveObserverFromTraceLog);
+  TraceLog::GetInstance()->SetDisabled();
+}
+
+TEST_F(TraceEventTestFixture, AsyncEnabledObserverDoesntFireOnSecondEnable) {
+  TraceLog::GetInstance()->SetEnabled(TraceConfig(kRecordAllCategoryFilter, ""),
+                                      TraceLog::RECORDING_MODE);
+
+  typedef AsyncTraceEventHelper<
+      testing::StrictMock<MockAsyncEnabledStateChangedObserver>>
+      Helper;
+  Helper helper;
+  helper.PerformRemoteTask(&Helper::AddObserverToTraceLog);
+
+  EXPECT_CALL(*helper.observer(), OnTraceLogEnabled()).Times(0);
+  EXPECT_CALL(*helper.observer(), OnTraceLogDisabled()).Times(0);
+  TraceLog::GetInstance()->SetEnabled(TraceConfig(kRecordAllCategoryFilter, ""),
+                                      TraceLog::RECORDING_MODE);
+  helper.PerformRemoteTask(&Helper::Nop);
+  testing::Mock::VerifyAndClear(helper.observer());
+  EXPECT_TRUE(TraceLog::GetInstance()->IsEnabled());
+
+  // Cleanup.
+  helper.PerformRemoteTask(&Helper::RemoveObserverFromTraceLog);
+  TraceLog::GetInstance()->SetDisabled();
+  TraceLog::GetInstance()->SetDisabled();
+}
+
+TEST_F(TraceEventTestFixture, AsyncEnabledObserverFiresOnFirstDisable) {
+  TraceConfig tc_inc_all("*", "");
+  TraceLog::GetInstance()->SetEnabled(tc_inc_all, TraceLog::RECORDING_MODE);
+  TraceLog::GetInstance()->SetEnabled(tc_inc_all, TraceLog::RECORDING_MODE);
+
+  typedef AsyncTraceEventHelper<
+      testing::StrictMock<MockAsyncEnabledStateChangedObserver>>
+      Helper;
+  Helper helper;
+  helper.PerformRemoteTask(&Helper::AddObserverToTraceLog);
+
+  EXPECT_CALL(*helper.observer(), OnTraceLogEnabled()).Times(0);
+  EXPECT_CALL(*helper.observer(), OnTraceLogDisabled()).Times(1);
+  TraceLog::GetInstance()->SetDisabled();
+  helper.PerformRemoteTask(&Helper::Nop);
+  testing::Mock::VerifyAndClear(helper.observer());
+  EXPECT_FALSE(TraceLog::GetInstance()->IsEnabled());
+
+  // Cleanup.
+  helper.PerformRemoteTask(&Helper::RemoveObserverFromTraceLog);
+  TraceLog::GetInstance()->SetDisabled();
+}
+
+TEST_F(TraceEventTestFixture, AsyncEnabledObserverFiresOnDisable) {
+  TraceLog::GetInstance()->SetEnabled(TraceConfig(kRecordAllCategoryFilter, ""),
+                                      TraceLog::RECORDING_MODE);
+
+  typedef AsyncTraceEventHelper<MockAsyncEnabledStateChangedObserver> Helper;
+  Helper helper;
+  helper.PerformRemoteTask(&Helper::AddObserverToTraceLog);
+
+  EXPECT_CALL(*helper.observer(), OnTraceLogDisabled()).Times(1);
+  TraceLog::GetInstance()->SetDisabled();
+  helper.PerformRemoteTask(&Helper::Nop);
+  testing::Mock::VerifyAndClear(helper.observer());
+  EXPECT_FALSE(TraceLog::GetInstance()->IsEnabled());
+
+  // Cleanup.
+  helper.PerformRemoteTask(&Helper::RemoveObserverFromTraceLog);
+}
+
+// No test for the IsEnabled() state of TraceLog at the time when callbacks of
+// AsyncEnabledStateObserver are run. Such tests do not make much sense due to
+// the async nature of the class.
+
+// Tests that a state observer can remove itself during a callback.
+class SelfRemovingAsyncEnabledStateObserver
+    : public TraceLog::AsyncEnabledStateObserver {
+ public:
+  SelfRemovingAsyncEnabledStateObserver() : weak_factory_(this) {}
+  ~SelfRemovingAsyncEnabledStateObserver() override {}
+  WeakPtr<SelfRemovingAsyncEnabledStateObserver> GetWeakPtr() {
+    return weak_factory_.GetWeakPtr();
+  }
+
+  // TraceLog::AsyncEnabledStateObserver overrides:
+  void OnTraceLogEnabled() override {}
+  void OnTraceLogDisabled() override {
+    TraceLog::GetInstance()->RemoveAsyncEnabledStateObserver(this);
+  }
+
+ private:
+  WeakPtrFactory<SelfRemovingAsyncEnabledStateObserver> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(SelfRemovingAsyncEnabledStateObserver);
+};
+
+TEST_F(TraceEventTestFixture, SelfRemovingAsyncObserver) {
+  typedef AsyncTraceEventHelper<SelfRemovingAsyncEnabledStateObserver> Helper;
+  Helper helper;
+  helper.PerformRemoteTask(&Helper::AddObserverToTraceLog);
+  EXPECT_TRUE(
+      TraceLog::GetInstance()->HasAsyncEnabledStateObserver(helper.observer()));
+
+  TraceLog::GetInstance()->SetEnabled(TraceConfig(kRecordAllCategoryFilter, ""),
+                                      TraceLog::RECORDING_MODE);
+  TraceLog::GetInstance()->SetDisabled();
+  helper.PerformRemoteTask(&Helper::Nop);
+
+  // The observer removed itself on disable.
+  EXPECT_FALSE(
+      TraceLog::GetInstance()->HasAsyncEnabledStateObserver(helper.observer()));
+}
+
 bool IsNewTrace() {
   bool is_new_trace;
   TRACE_EVENT_IS_NEW_TRACE(&is_new_trace);