Introduce TraceLog::AsyncEnabledStateObserver

base/trace_event/trace_event_unittest.cc

 // Copyright (c) 2012 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 "base/trace_event/trace_event.h"
 
 #include <math.h>
 #include <stddef.h>
 #include <stdint.h>
 
@@ skipping 1147 matching lines @@
   TraceLog::GetInstance()->AddEnabledStateObserver(&observer);
   EXPECT_EQ(1u, TraceLog::GetInstance()->GetObserverCountForTest());
 
   TraceLog::GetInstance()->SetEnabled(TraceConfig(kRecordAllCategoryFilter, ""),
                                       TraceLog::RECORDING_MODE);
   TraceLog::GetInstance()->SetDisabled();
   // The observer removed itself on disable.
   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);
   return is_new_trace;
 }
 
 TEST_F(TraceEventTestFixture, NewTraceRecording) {
   ASSERT_FALSE(IsNewTrace());
   TraceLog::GetInstance()->SetEnabled(TraceConfig(kRecordAllCategoryFilter, ""),
                                       TraceLog::RECORDING_MODE);
@@ skipping 1916 matching lines @@
 }
 
 TEST_F(TraceEventTestFixture, SyntheticDelayConfigurationToString) {
   const char filter[] = "DELAY(test.Delay;16;oneshot)";
   TraceConfig config(filter, "");
   EXPECT_EQ(filter, config.ToCategoryFilterString());
 }
 
 }  // namespace trace_event
 }  // namespace base