From b145ad5ef9c297b558b97e74240917b3260b2082 Mon Sep 17 00:00:00 2001 From: Lucas Dupin Date: Wed, 17 Feb 2021 20:18:12 -0800 Subject: [PATCH] Move FeatureFlagReader to sysprops DeviceConfigs ended up being flaky and buggy. They were also eventually reset. SystemProperties are stable and widely used across the system. Test: adb shell setprop persist.systemui.flag_monet 1 Test: atest FeatureFlagReaderTest Fixes: 180559762 Change-Id: I20b1ae82ac66a2257159048ac6f9b5cd00f677c5 --- .../systemui/flags/FeatureFlagReader.java | 41 ++--------------- .../systemui/flags/SystemPropertiesHelper.kt | 32 +++++++++++++ .../systemui/flags/FeatureFlagReaderTest.java | 46 +++---------------- 3 files changed, 44 insertions(+), 75 deletions(-) create mode 100644 packages/SystemUI/src/com/android/systemui/flags/SystemPropertiesHelper.kt diff --git a/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagReader.java b/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagReader.java index b77fcc822b880..073586e88a8bb 100644 --- a/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagReader.java +++ b/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagReader.java @@ -16,24 +16,18 @@ package com.android.systemui.flags; -import android.annotation.NonNull; import android.content.res.Resources; -import android.provider.DeviceConfig; import android.util.SparseArray; import androidx.annotation.BoolRes; import androidx.annotation.Nullable; import com.android.systemui.R; -import com.android.systemui.assist.DeviceConfigHelper; import com.android.systemui.dagger.SysUISingleton; -import com.android.systemui.dagger.qualifiers.Background; import com.android.systemui.dagger.qualifiers.Main; import com.android.systemui.statusbar.FeatureFlags; import com.android.systemui.util.wrapper.BuildInfo; -import java.util.concurrent.Executor; - import javax.inject.Inject; /** * Reads and caches feature flags for quick access @@ -60,23 +54,19 @@ import javax.inject.Inject; @SysUISingleton public class FeatureFlagReader { private final Resources mResources; - private final DeviceConfigHelper mDeviceConfig; private final boolean mAreFlagsOverrideable; - + private final SystemPropertiesHelper mSystemPropertiesHelper; private final SparseArray mCachedFlags = new SparseArray<>(); @Inject public FeatureFlagReader( @Main Resources resources, BuildInfo build, - DeviceConfigHelper deviceConfig, - @Background Executor executor) { + SystemPropertiesHelper systemPropertiesHelper) { mResources = resources; - mDeviceConfig = deviceConfig; + mSystemPropertiesHelper = systemPropertiesHelper; mAreFlagsOverrideable = build.isDebuggable() && mResources.getBoolean(R.bool.are_flags_overrideable); - - mDeviceConfig.addOnPropertiesChangedListener(executor, this::onPropertiesChanged); } /** @@ -93,7 +83,7 @@ public class FeatureFlagReader { String name = resourceIdToFlagName(resId); boolean value = mResources.getBoolean(resId); if (mAreFlagsOverrideable) { - value = mDeviceConfig.getBoolean(flagNameToStorageKey(name), value); + value = mSystemPropertiesHelper.getBoolean(flagNameToStorageKey(name), value); } cachedFlag = new CachedFlag(name, value); @@ -104,27 +94,6 @@ public class FeatureFlagReader { } } - private void onPropertiesChanged(@NonNull DeviceConfig.Properties properties) { - synchronized (mCachedFlags) { - for (String key : properties.getKeyset()) { - String flagName = storageKeyToFlagName(key); - if (flagName != null) { - clearCachedFlag(flagName); - } - } - } - } - - private void clearCachedFlag(String flagName) { - for (int i = 0; i < mCachedFlags.size(); i++) { - CachedFlag flag = mCachedFlags.valueAt(i); - if (flag.name.equals(flagName)) { - mCachedFlags.removeAt(i); - break; - } - } - } - private String resourceIdToFlagName(@BoolRes int resId) { String resName = mResources.getResourceEntryName(resId); if (resName.startsWith(RESNAME_PREFIX)) { @@ -160,6 +129,6 @@ public class FeatureFlagReader { } } - private static final String STORAGE_KEY_PREFIX = "flag_"; + private static final String STORAGE_KEY_PREFIX = "persist.systemui.flag_"; private static final String RESNAME_PREFIX = "flag_"; } diff --git a/packages/SystemUI/src/com/android/systemui/flags/SystemPropertiesHelper.kt b/packages/SystemUI/src/com/android/systemui/flags/SystemPropertiesHelper.kt new file mode 100644 index 0000000000000..28f63b07e5842 --- /dev/null +++ b/packages/SystemUI/src/com/android/systemui/flags/SystemPropertiesHelper.kt @@ -0,0 +1,32 @@ +/* + * Copyright (C) 2021 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.systemui.flags + +import android.os.SystemProperties +import com.android.systemui.dagger.SysUISingleton + +import javax.inject.Inject + +/** + * Proxy to make {@link SystemProperties} easily testable. + */ +@SysUISingleton +class SystemPropertiesHelper @Inject constructor() { + fun getBoolean(name: String, default: Boolean): Boolean { + return SystemProperties.getBoolean(name, default) + } +} \ No newline at end of file diff --git a/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagReaderTest.java b/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagReaderTest.java index c79037b761aa0..223714cfda309 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagReaderTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagReaderTest.java @@ -18,7 +18,6 @@ package com.android.systemui.flags; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; -import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; @@ -27,59 +26,44 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import android.content.res.Resources; -import android.provider.DeviceConfig; import androidx.annotation.BoolRes; import androidx.test.filters.SmallTest; import com.android.systemui.R; import com.android.systemui.SysuiTestCase; -import com.android.systemui.assist.DeviceConfigHelper; import com.android.systemui.util.wrapper.BuildInfo; import org.junit.Before; import org.junit.Test; -import org.mockito.ArgumentCaptor; -import org.mockito.Captor; import org.mockito.Mock; import org.mockito.MockitoAnnotations; -import java.util.HashMap; -import java.util.Map; -import java.util.concurrent.Executor; - @SmallTest public class FeatureFlagReaderTest extends SysuiTestCase { @Mock private Resources mResources; @Mock private BuildInfo mBuildInfo; - @Mock private DeviceConfigHelper mDeviceConfig; - @Mock private Executor mBackgroundExecutor; + @Mock private SystemPropertiesHelper mSystemPropertiesHelper; private FeatureFlagReader mReader; - @Captor private ArgumentCaptor mListenerCaptor; - private DeviceConfig.OnPropertiesChangedListener mPropChangeListener; - @Before public void setUp() { MockitoAnnotations.initMocks(this); - when(mDeviceConfig.getBoolean(anyString(), anyBoolean())) + when(mSystemPropertiesHelper.getBoolean(anyString(), anyBoolean())) .thenAnswer(invocation -> invocation.getArgument(1)); defineFlag(FLAG_RESID_0, false); defineFlag(FLAG_RESID_1, true); initialize(true, true); - - verify(mDeviceConfig).addOnPropertiesChangedListener(any(), mListenerCaptor.capture()); - mPropChangeListener = mListenerCaptor.getValue(); } private void initialize(boolean isDebuggable, boolean isOverrideable) { when(mBuildInfo.isDebuggable()).thenReturn(isDebuggable); when(mResources.getBoolean(R.bool.are_flags_overrideable)).thenReturn(isOverrideable); - mReader = new FeatureFlagReader(mResources, mBuildInfo, mDeviceConfig, mBackgroundExecutor); + mReader = new FeatureFlagReader(mResources, mBuildInfo, mSystemPropertiesHelper); } @Test @@ -136,24 +120,8 @@ public class FeatureFlagReaderTest extends SysuiTestCase { // THEN the underlying resource and override are only queried once verify(mResources, times(1)).getBoolean(FLAG_RESID_0); - verify(mDeviceConfig, times(1)).getBoolean(fakeStorageKey(FLAG_RESID_0), false); - } - - @Test - public void testCachesAreClearedAfterPropsChange() { - // GIVEN a flag whose value has already been queried - assertFalse(mReader.isEnabled(FLAG_RESID_0)); - - // WHEN the value of the flag changes - overrideFlag(FLAG_RESID_0, true); - Map changedMap = new HashMap<>(); - changedMap.put(fakeStorageKey(FLAG_RESID_0), "true"); - DeviceConfig.Properties properties = - new DeviceConfig.Properties("systemui", changedMap); - mPropChangeListener.onPropertiesChanged(properties); - - // THEN the new value is provided - assertTrue(mReader.isEnabled(FLAG_RESID_0)); + verify(mSystemPropertiesHelper, times(1)) + .getBoolean(fakeStorageKey(FLAG_RESID_0), false); } private void defineFlag(int resId, boolean value) { @@ -162,12 +130,12 @@ public class FeatureFlagReaderTest extends SysuiTestCase { } private void overrideFlag(int resId, boolean value) { - when(mDeviceConfig.getBoolean(eq(fakeStorageKey(resId)), anyBoolean())) + when(mSystemPropertiesHelper.getBoolean(eq(fakeStorageKey(resId)), anyBoolean())) .thenReturn(value); } private String fakeStorageKey(@BoolRes int resId) { - return "flag_testname_" + resId; + return "persist.systemui.flag_testname_" + resId; } private static final int FLAG_RESID_0 = 47;