SF: cleanup FlagManager

We currently use FlagManager just for boolean flags.
Remove the templated functions in favot of simpler ones.
Add macros for code reuse.

Test: presubmit
Change-Id: Iba892d981960b653466f55a52ed74a16138da75c
diff --git a/services/surfaceflinger/FlagManager.cpp b/services/surfaceflinger/FlagManager.cpp
index 303714c..919ea42 100644
--- a/services/surfaceflinger/FlagManager.cpp
+++ b/services/surfaceflinger/FlagManager.cpp
@@ -28,7 +28,6 @@
 
 namespace android {
 static constexpr const char* kExperimentNamespace = "surface_flinger_native_boot";
-static constexpr const int64_t kDemoFlag = -1;
 
 std::unique_ptr<FlagManager> FlagManager::mInstance;
 std::once_flag FlagManager::mOnce;
@@ -36,40 +35,8 @@
 FlagManager::FlagManager(ConstructorTag) {}
 FlagManager::~FlagManager() = default;
 
-FlagManager& FlagManager::getInstance() {
-    std::call_once(mOnce, [&] {
-        LOG_ALWAYS_FATAL_IF(mInstance, "Instance already created");
-        mInstance = std::make_unique<FlagManager>(ConstructorTag{});
-    });
-
-    return *mInstance;
-}
-
-void FlagManager::dump(std::string& result) const {
-    base::StringAppendF(&result, "FlagManager values: \n");
-    base::StringAppendF(&result, "demo_flag: %" PRId64 "\n", demo_flag());
-    base::StringAppendF(&result, "use_adpf_cpu_hint: %s\n", use_adpf_cpu_hint() ? "true" : "false");
-    base::StringAppendF(&result, "use_skia_tracing: %s\n", use_skia_tracing() ? "true" : "false");
-}
-
 namespace {
-template <typename T>
-std::optional<T> doParse(const char* str);
-
-template <>
-[[maybe_unused]] std::optional<int32_t> doParse(const char* str) {
-    int32_t ret;
-    return base::ParseInt(str, &ret) ? std::make_optional(ret) : std::nullopt;
-}
-
-template <>
-[[maybe_unused]] std::optional<int64_t> doParse(const char* str) {
-    int64_t ret;
-    return base::ParseInt(str, &ret) ? std::make_optional(ret) : std::nullopt;
-}
-
-template <>
-[[maybe_unused]] std::optional<bool> doParse(const char* str) {
+std::optional<bool> parseBool(const char* str) {
     base::ParseBoolResult parseResult = base::ParseBool(str);
     switch (parseResult) {
         case base::ParseBoolResult::kTrue:
@@ -80,44 +47,67 @@
             return std::nullopt;
     }
 }
+
+void dumpFlag(std::string& result, const char* name, std::function<bool()> getter) {
+    base::StringAppendF(&result, "%s: %s\n", name, getter() ? "true" : "false");
+}
+
 } // namespace
 
-std::string FlagManager::getServerConfigurableFlag(const std::string& experimentFlagName) const {
-    return server_configurable_flags::GetServerConfigurableFlag(kExperimentNamespace,
-                                                                experimentFlagName, "");
+const FlagManager& FlagManager::getInstance() {
+    return getMutableInstance();
 }
 
-template int32_t FlagManager::getValue<int32_t>(const std::string&, std::optional<int32_t>,
-                                                int32_t) const;
-template int64_t FlagManager::getValue<int64_t>(const std::string&, std::optional<int64_t>,
-                                                int64_t) const;
-template bool FlagManager::getValue<bool>(const std::string&, std::optional<bool>, bool) const;
-template <typename T>
-T FlagManager::getValue(const std::string& experimentFlagName, std::optional<T> systemPropertyOpt,
-                        T defaultValue) const {
-    // System property takes precedence over the experiment config server value.
-    if (systemPropertyOpt.has_value()) {
-        return *systemPropertyOpt;
+FlagManager& FlagManager::getMutableInstance() {
+    std::call_once(mOnce, [&] {
+        LOG_ALWAYS_FATAL_IF(mInstance, "Instance already created");
+        mInstance = std::make_unique<FlagManager>(ConstructorTag{});
+    });
+
+    return *mInstance;
+}
+
+void FlagManager::markBootCompleted() {
+    mBootCompleted = true;
+}
+
+void FlagManager::dump(std::string& result) const {
+#define DUMP_FLAG(name) dumpFlag(result, #name, std::bind(&FlagManager::name, this));
+
+    base::StringAppendF(&result, "FlagManager values: \n");
+    DUMP_FLAG(use_adpf_cpu_hint);
+    DUMP_FLAG(use_skia_tracing);
+
+#undef DUMP_FLAG
+}
+
+std::optional<bool> FlagManager::getBoolProperty(const char* property) const {
+    return parseBool(base::GetProperty(property, "").c_str());
+}
+
+bool FlagManager::getServerConfigurableFlag(const char* experimentFlagName) const {
+    const auto value = server_configurable_flags::GetServerConfigurableFlag(kExperimentNamespace,
+                                                                            experimentFlagName, "");
+    const auto res = parseBool(value.c_str());
+    return res.has_value() && res.value();
+}
+
+#define FLAG_MANAGER_SERVER_FLAG(name, syspropOverride, serverFlagName)                     \
+    bool FlagManager::name() const {                                                        \
+        LOG_ALWAYS_FATAL_IF(!mBootCompleted,                                                \
+                            "Can't read %s before boot completed as it is server writable", \
+                            __func__);                                                      \
+        const auto debugOverride = getBoolProperty(syspropOverride);                        \
+        if (debugOverride.has_value()) return debugOverride.value();                        \
+        return getServerConfigurableFlag(serverFlagName);                                   \
     }
-    std::string str = getServerConfigurableFlag(experimentFlagName);
-    return str.empty() ? defaultValue : doParse<T>(str.c_str()).value_or(defaultValue);
-}
 
-int64_t FlagManager::demo_flag() const {
-    std::optional<int64_t> sysPropVal = std::nullopt;
-    return getValue("DemoFeature__demo_flag", sysPropVal, kDemoFlag);
-}
+FLAG_MANAGER_SERVER_FLAG(test_flag, "", "")
+FLAG_MANAGER_SERVER_FLAG(use_adpf_cpu_hint, "debug.sf.enable_adpf_cpu_hint",
+                         "AdpfFeature__adpf_cpu_hint")
+FLAG_MANAGER_SERVER_FLAG(use_skia_tracing, PROPERTY_SKIA_ATRACE_ENABLED,
+                         "SkiaTracingFeature__use_skia_tracing")
 
-bool FlagManager::use_adpf_cpu_hint() const {
-    std::optional<bool> sysPropVal =
-            doParse<bool>(base::GetProperty("debug.sf.enable_adpf_cpu_hint", "").c_str());
-    return getValue("AdpfFeature__adpf_cpu_hint", sysPropVal, false);
-}
-
-bool FlagManager::use_skia_tracing() const {
-    std::optional<bool> sysPropVal =
-            doParse<bool>(base::GetProperty(PROPERTY_SKIA_ATRACE_ENABLED, "").c_str());
-    return getValue("SkiaTracingFeature__use_skia_tracing", sysPropVal, false);
-}
+#undef FLAG_MANAGER_SERVER_FLAG
 
 } // namespace android
diff --git a/services/surfaceflinger/FlagManager.h b/services/surfaceflinger/FlagManager.h
index 8af0b4f..facf6c3 100644
--- a/services/surfaceflinger/FlagManager.h
+++ b/services/surfaceflinger/FlagManager.h
@@ -30,30 +30,30 @@
     struct ConstructorTag {};
 
 public:
-    static FlagManager& getInstance();
+    static const FlagManager& getInstance();
+    static FlagManager& getMutableInstance();
 
     FlagManager(ConstructorTag);
-
     virtual ~FlagManager();
+
+    void markBootCompleted();
     void dump(std::string& result) const;
 
-    int64_t demo_flag() const;
-
+    bool test_flag() const;
     bool use_adpf_cpu_hint() const;
-
     bool use_skia_tracing() const;
 
+protected:
+    // overridden for unit tests
+    virtual std::optional<bool> getBoolProperty(const char*) const;
+    virtual bool getServerConfigurableFlag(const char*) const;
+
 private:
-    friend class FlagManagerTest;
+    friend class TestableFlagManager;
 
     FlagManager(const FlagManager&) = delete;
 
-    // Wrapper for mocking in test.
-    virtual std::string getServerConfigurableFlag(const std::string& experimentFlagName) const;
-
-    template <typename T>
-    T getValue(const std::string& experimentFlagName, std::optional<T> systemPropertyOpt,
-               T defaultValue) const;
+    std::atomic_bool mBootCompleted;
 
     static std::unique_ptr<FlagManager> mInstance;
     static std::once_flag mOnce;
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index a36def7..3050520 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -689,6 +689,7 @@
         return;
     }
     mBootFinished = true;
+    FlagManager::getMutableInstance().markBootCompleted();
     if (mStartPropertySetThread->join() != NO_ERROR) {
         ALOGE("Join StartPropertySetThread failed!");
     }
diff --git a/services/surfaceflinger/tests/unittests/FlagManagerTest.cpp b/services/surfaceflinger/tests/unittests/FlagManagerTest.cpp
index 0905cd1..32c4d05 100644
--- a/services/surfaceflinger/tests/unittests/FlagManagerTest.cpp
+++ b/services/surfaceflinger/tests/unittests/FlagManagerTest.cpp
@@ -14,130 +14,71 @@
  * limitations under the License.
  */
 
-#include <cstdint>
 #undef LOG_TAG
 #define LOG_TAG "FlagManagerTest"
 
 #include "FlagManager.h"
 
-#include <android-base/properties.h>
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
 #include <log/log.h>
-#include <server_configurable_flags/get_flags.h>
-#include <optional>
 
 namespace android {
 
 using testing::Return;
 
-class MockFlagManager : public FlagManager {
+class TestableFlagManager : public FlagManager {
 public:
-    MockFlagManager() = default;
-    ~MockFlagManager() = default;
+    TestableFlagManager() : FlagManager(ConstructorTag{}) { markBootCompleted(); }
+    ~TestableFlagManager() = default;
 
-    MOCK_METHOD(std::string, getServerConfigurableFlag, (const std::string& experimentFlagName),
-                (const, override));
+    MOCK_METHOD(std::optional<bool>, getBoolProperty, (const char*), (const, override));
+    MOCK_METHOD(bool, getServerConfigurableFlag, (const char*), (const, override));
+
+    void markBootIncomplete() { mBootCompleted = false; }
 };
 
 class FlagManagerTest : public testing::Test {
 public:
-    FlagManagerTest();
-    ~FlagManagerTest() override;
-    std::unique_ptr<MockFlagManager> mFlagManager;
+    FlagManagerTest() {
+        const ::testing::TestInfo* const test_info =
+                ::testing::UnitTest::GetInstance()->current_test_info();
+        ALOGD("**** Setting up for %s.%s\n", test_info->test_case_name(), test_info->name());
+    }
+    ~FlagManagerTest() override {
+        const ::testing::TestInfo* const test_info =
+                ::testing::UnitTest::GetInstance()->current_test_info();
+        ALOGD("**** Tearing down after %s.%s\n", test_info->test_case_name(), test_info->name());
+    }
 
-    template <typename T>
-    T getValue(const std::string& experimentFlagName, std::optional<T> systemPropertyOpt,
-               T defaultValue);
+    TestableFlagManager mFlagManager;
 };
 
-FlagManagerTest::FlagManagerTest() {
-    const ::testing::TestInfo* const test_info =
-            ::testing::UnitTest::GetInstance()->current_test_info();
-    ALOGD("**** Setting up for %s.%s\n", test_info->test_case_name(), test_info->name());
-    mFlagManager = std::make_unique<MockFlagManager>();
+TEST_F(FlagManagerTest, isSingleton) {
+    EXPECT_EQ(&FlagManager::getInstance(), &FlagManager::getInstance());
 }
 
-FlagManagerTest::~FlagManagerTest() {
-    const ::testing::TestInfo* const test_info =
-            ::testing::UnitTest::GetInstance()->current_test_info();
-    ALOGD("**** Tearing down after %s.%s\n", test_info->test_case_name(), test_info->name());
+TEST_F(FlagManagerTest, creashesIfQueriedBeforeBoot) {
+    mFlagManager.markBootIncomplete();
+    EXPECT_DEATH(FlagManager::getInstance().use_adpf_cpu_hint(), "");
 }
 
-template <typename T>
-T FlagManagerTest::getValue(const std::string& experimentFlagName,
-                            std::optional<T> systemPropertyOpt, T defaultValue) {
-    return mFlagManager->getValue(experimentFlagName, systemPropertyOpt, defaultValue);
+TEST_F(FlagManagerTest, returnsOverride) {
+    EXPECT_CALL(mFlagManager, getBoolProperty).WillOnce(Return(true));
+    EXPECT_EQ(true, mFlagManager.test_flag());
+
+    EXPECT_CALL(mFlagManager, getBoolProperty).WillOnce(Return(false));
+    EXPECT_EQ(false, mFlagManager.test_flag());
 }
 
-namespace {
-TEST_F(FlagManagerTest, getValue_bool_default) {
-    EXPECT_CALL(*mFlagManager, getServerConfigurableFlag).Times(1).WillOnce(Return(""));
-    const bool defaultValue = false;
-    std::optional<bool> systemPropertyValue = std::nullopt;
-    const bool result = FlagManagerTest::getValue("test_flag", systemPropertyValue, defaultValue);
-    ASSERT_EQ(result, defaultValue);
+TEST_F(FlagManagerTest, returnsValue) {
+    EXPECT_CALL(mFlagManager, getBoolProperty).WillRepeatedly(Return(std::nullopt));
+
+    EXPECT_CALL(mFlagManager, getServerConfigurableFlag).WillOnce(Return(true));
+    EXPECT_EQ(true, mFlagManager.test_flag());
+
+    EXPECT_CALL(mFlagManager, getServerConfigurableFlag).WillOnce(Return(false));
+    EXPECT_EQ(false, mFlagManager.test_flag());
 }
 
-TEST_F(FlagManagerTest, getValue_bool_sysprop) {
-    const bool defaultValue = false;
-    std::optional<bool> systemPropertyValue = std::make_optional(true);
-    const bool result = FlagManagerTest::getValue("test_flag", systemPropertyValue, defaultValue);
-    ASSERT_EQ(result, true);
-}
-
-TEST_F(FlagManagerTest, getValue_bool_experiment) {
-    EXPECT_CALL(*mFlagManager, getServerConfigurableFlag).Times(1).WillOnce(Return("1"));
-    const bool defaultValue = false;
-    std::optional<bool> systemPropertyValue = std::nullopt;
-    const bool result = FlagManagerTest::getValue("test_flag", systemPropertyValue, defaultValue);
-    ASSERT_EQ(result, true);
-}
-
-TEST_F(FlagManagerTest, getValue_int32_default) {
-    EXPECT_CALL(*mFlagManager, getServerConfigurableFlag).Times(1).WillOnce(Return(""));
-    int32_t defaultValue = 30;
-    std::optional<int32_t> systemPropertyValue = std::nullopt;
-    int32_t result = FlagManagerTest::getValue("test_flag", systemPropertyValue, defaultValue);
-    ASSERT_EQ(result, defaultValue);
-}
-
-TEST_F(FlagManagerTest, getValue_int32_sysprop) {
-    int32_t defaultValue = 30;
-    std::optional<int32_t> systemPropertyValue = std::make_optional(10);
-    int32_t result = FlagManagerTest::getValue("test_flag", systemPropertyValue, defaultValue);
-    ASSERT_EQ(result, 10);
-}
-
-TEST_F(FlagManagerTest, getValue_int32_experiment) {
-    EXPECT_CALL(*mFlagManager, getServerConfigurableFlag).Times(1).WillOnce(Return("50"));
-    std::int32_t defaultValue = 30;
-    std::optional<std::int32_t> systemPropertyValue = std::nullopt;
-    std::int32_t result = FlagManagerTest::getValue("test_flag", systemPropertyValue, defaultValue);
-    ASSERT_EQ(result, 50);
-}
-
-TEST_F(FlagManagerTest, getValue_int64_default) {
-    EXPECT_CALL(*mFlagManager, getServerConfigurableFlag).Times(1).WillOnce(Return(""));
-    int64_t defaultValue = 30;
-    std::optional<int64_t> systemPropertyValue = std::nullopt;
-    int64_t result = getValue("flag_name", systemPropertyValue, defaultValue);
-    ASSERT_EQ(result, defaultValue);
-}
-
-TEST_F(FlagManagerTest, getValue_int64_sysprop) {
-    int64_t defaultValue = 30;
-    std::optional<int64_t> systemPropertyValue = std::make_optional(10);
-    int64_t result = getValue("flag_name", systemPropertyValue, defaultValue);
-    ASSERT_EQ(result, 10);
-}
-
-TEST_F(FlagManagerTest, getValue_int64_experiment) {
-    EXPECT_CALL(*mFlagManager, getServerConfigurableFlag).Times(1).WillOnce(Return("50"));
-    int64_t defaultValue = 30;
-    std::optional<int64_t> systemPropertyValue = std::nullopt;
-    int64_t result = getValue("flag_name", systemPropertyValue, defaultValue);
-    ASSERT_EQ(result, 50);
-}
-} // namespace
 } // namespace android