Skip to content

Commit

Permalink
[Android] Move code from CachedFeatureFlags to CachedFlag
Browse files Browse the repository at this point in the history 
Move static methods from CachedFeatureFlags to be instance methods in
CachedFlag. This makes CachedFlag more cohesive and separates it from
CachedFeatureFlags code that concerns field trials or special-cased
flags.

This is a step to move CachedFlag to //base.

Disable-Rts: True
Bug: 1442347
Change-Id: Ic18de98a6475ada089a0d32ce7057286f7f72bf7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4529355
Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Commit-Queue: Henrique Nakashima <hnakashima@chromium.org>
Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#1146713}
  • Loading branch information
Henrique Nakashima authored and Chromium LUCI CQ committed May 19, 2023
1 parent 47978d8 commit 5a72c34
Show file tree
Hide file tree
Showing 8 changed files with 161 additions and 174 deletions.
11 changes: 0 additions & 11 deletions chrome/android/expectations/lint-baseline.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7090,17 +7090,6 @@
column="16"/>
</issue>

<issue
id="AssertionSideEffect"
message="Assertion condition has a side effect: sValuesReturned.boolValues.put(preferenceName, flag)"
errorLine1=" assert ChromeFeatureList.sInstantStart.isEnabled();"
errorLine2=" ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~">
<location
file="../../chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/pseudotab/PseudoTab.java"
line="300"
column="20"/>
</issue>

<issue
id="AssertionSideEffect"
message="Assertion condition has a side effect: sHaveAccessNetworkState =&#xA; ApiCompatibilityUtils.checkPermission(ContextUtils.getApplicationContext(),&#xA; Manifest.permission.ACCESS_NETWORK_STATE, Process.myPid(),&#xA; Process.myUid())&#xA; == PackageManager.PERMISSION_GRANTED"
Expand Down
2 changes: 1 addition & 1 deletion ...droid/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/pseudotab/PseudoTab.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,7 @@ public static void clearForTesting() {
private static @Nullable List<Tab> getRelatedTabList(
@NonNull TabModelSelector tabModelSelector, int tabId) {
if (!tabModelSelector.isTabStateInitialized()) {
assert ChromeFeatureList.sInstantStart.isEnabled();
if (!ChromeFeatureList.sInstantStart.isEnabled()) throw new IllegalStateException();
return null;
}
TabModelFilterProvider provider = tabModelSelector.getTabModelFilterProvider();
Expand Down
159 changes: 35 additions & 124 deletions .../browser/flags/android/java/src/org/chromium/chrome/browser/flags/CachedFeatureFlags.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
package org.chromium.chrome.browser.flags;

import androidx.annotation.AnyThread;
import androidx.annotation.Nullable;
import androidx.annotation.VisibleForTesting;

import org.chromium.base.FieldTrialList;
Expand Down Expand Up @@ -40,92 +39,17 @@
* value in shared preferences.
*/
public class CachedFeatureFlags {
private static ValuesReturned sValuesReturned = new ValuesReturned();
private static ValuesOverridden sValuesOverridden = new ValuesOverridden();
private static CachedFlagsSafeMode sSafeMode = new CachedFlagsSafeMode();

private static String sReachedCodeProfilerTrialGroup;

/**
* Rules from highest to lowest priority:
* 1. If the flag has been forced by @EnableFeatures/@DisableFeatures or
* {@link CachedFlag#setForTesting}, the forced value is returned.
* 2. If a value was previously returned in the same run, the same value is returned for
* consistency.
* 3. If native is loaded, the value from {@link ChromeFeatureList} is returned.
* 4. If in a previous run, the value from {@link ChromeFeatureList} was cached to SharedPrefs,
* it is returned.
* 5. The default value passed as a parameter is returned.
*
* TODO(crbug.com/1442347): Move this code to CachedFlag.
*/
@AnyThread
static boolean isEnabled(CachedFlag cachedFlag) {
sSafeMode.onFlagChecked();

String featureName = cachedFlag.getFeatureName();
boolean defaultValue = cachedFlag.getDefaultValue();
String preferenceName = cachedFlag.getSharedPreferenceKey();

Boolean flag;
synchronized (sValuesReturned.boolValues) {
flag = sValuesReturned.boolValues.get(preferenceName);
if (flag != null) {
return flag;
}

flag = sSafeMode.isEnabled(featureName, preferenceName, defaultValue);
if (flag == null) {
SharedPreferencesManager prefs = SharedPreferencesManager.getInstance();
if (prefs.contains(preferenceName)) {
flag = prefs.readBoolean(preferenceName, false);
} else {
flag = defaultValue;
}
}

sValuesReturned.boolValues.put(preferenceName, flag);
}
return flag;
}

@CalledByNative
@AnyThread
static boolean isEnabled(String featureName) {
CachedFlag cachedFlag = ChromeFeatureList.sAllCachedFlags.get(featureName);
assert cachedFlag != null : String.format("%s not found in ChromeFeatureList", featureName);

return isEnabled(cachedFlag);
}

/**
* Caches the value of a feature from {@link ChromeFeatureList} to SharedPrefs.
*
* TODO(crbug.com/1442347): Move this code to CachedFlag.
*/
static void cacheFeature(CachedFlag cachedFlag) {
boolean isEnabledInNative = ChromeFeatureList.isEnabled(cachedFlag.getFeatureName());
SharedPreferencesManager.getInstance().writeBoolean(
cachedFlag.getSharedPreferenceKey(), isEnabledInNative);
}
assert cachedFlag != null;

/**
* Forces a feature to be enabled or disabled for testing.
*
* Do not call this from tests; use @EnableFeatures/@DisableFeatures annotations or
* {@link CachedFlag#setForTesting(Boolean)} instead.
*
* @param cachedFlag the {@link CachedFlag} to set a value for
* @param value the value that {@link CachedFlag#isEnabled()} will be forced to return. If null,
* remove any values previously forced.
*
* TODO(crbug.com/1442347): Move this code to CachedFlag.
*/
static void setForTesting(CachedFlag cachedFlag, @Nullable Boolean value) {
String preferenceName = cachedFlag.getSharedPreferenceKey();
synchronized (sValuesReturned.boolValues) {
sValuesReturned.boolValues.put(preferenceName, value);
}
return cachedFlag.isEnabled();
}

/**
Expand All @@ -139,12 +63,7 @@ public static void setFeaturesForTesting(Map<String, Boolean> features) {

sValuesOverridden.enableOverrides();

for (Map.Entry<String, Boolean> entry : features.entrySet()) {
CachedFlag possibleCachedFlag = ChromeFeatureList.sAllCachedFlags.get(entry.getKey());
if (possibleCachedFlag != null) {
setForTesting(possibleCachedFlag, entry.getValue());
}
}
CachedFlag.setFeaturesForTesting(features);
}

/**
Expand All @@ -164,7 +83,7 @@ public static void cacheNativeFlags(List<CachedFlag> featuresToCache) {
*/
public static void cacheAdditionalNativeFlags() {
cacheNetworkServiceWarmUpEnabled();
sSafeMode.cacheSafeModeForCachedFlagsEnabled();
CachedFlagsSafeMode.getInstance().cacheSafeModeForCachedFlagsEnabled();
cacheReachedCodeProfilerTrialGroup();

// Propagate REACHED_CODE_PROFILER feature value to LibraryLoader. This can't be done in
Expand Down Expand Up @@ -251,148 +170,140 @@ public static String getReachedCodeProfilerTrialGroup() {
* Call when entering an initialization flow that should result in caching flags.
*/
public static void onStartOrResumeCheckpoint() {
sSafeMode.onStartOrResumeCheckpoint();
CachedFlagsSafeMode.getInstance().onStartOrResumeCheckpoint();
}

/**
* Call when aborting an initialization flow that would have resulted in caching flags.
*/
public static void onPauseCheckpoint() {
sSafeMode.onPauseCheckpoint();
CachedFlagsSafeMode.getInstance().onPauseCheckpoint();
}

/**
* Call when finishing an initialization flow with flags having been cached successfully.
*/
public static void onEndCheckpoint() {
sSafeMode.onEndCheckpoint(sValuesReturned);
CachedFlagsSafeMode.getInstance().onEndCheckpoint();
}

public static @CachedFlagsSafeMode.Behavior int getSafeModeBehaviorForTesting() {
return sSafeMode.getBehaviorForTesting();
return CachedFlagsSafeMode.getInstance().getBehaviorForTesting();
}

@AnyThread
static boolean getConsistentBooleanValue(String preferenceName, boolean defaultValue) {
sSafeMode.onFlagChecked();
CachedFlagsSafeMode.getInstance().onFlagChecked();

if (sValuesOverridden.isEnabled()) {
return sValuesOverridden.getBool(preferenceName, defaultValue);
}

Boolean value;
synchronized (sValuesReturned.boolValues) {
value = sValuesReturned.boolValues.get(preferenceName);
synchronized (ValuesReturned.sBoolValues) {
value = ValuesReturned.sBoolValues.get(preferenceName);
if (value != null) {
return value;
}

value = sSafeMode.getBooleanFieldTrialParam(preferenceName, defaultValue);
value = CachedFlagsSafeMode.getInstance().getBooleanFieldTrialParam(
preferenceName, defaultValue);
if (value == null) {
value = SharedPreferencesManager.getInstance().readBoolean(
preferenceName, defaultValue);
}

sValuesReturned.boolValues.put(preferenceName, value);
ValuesReturned.sBoolValues.put(preferenceName, value);
}
return value;
}

@AnyThread
static String getConsistentStringValue(String preferenceName, String defaultValue) {
sSafeMode.onFlagChecked();
CachedFlagsSafeMode.getInstance().onFlagChecked();

if (sValuesOverridden.isEnabled()) {
return sValuesOverridden.getString(preferenceName, defaultValue);
}

String value;
synchronized (sValuesReturned.stringValues) {
value = sValuesReturned.stringValues.get(preferenceName);
synchronized (ValuesReturned.sStringValues) {
value = ValuesReturned.sStringValues.get(preferenceName);
if (value != null) {
return value;
}

value = sSafeMode.getStringFieldTrialParam(preferenceName, defaultValue);
value = CachedFlagsSafeMode.getInstance().getStringFieldTrialParam(
preferenceName, defaultValue);
if (value == null) {
value = SharedPreferencesManager.getInstance().readString(
preferenceName, defaultValue);
}

sValuesReturned.stringValues.put(preferenceName, value);
ValuesReturned.sStringValues.put(preferenceName, value);
}
return value;
}

@AnyThread
static int getConsistentIntValue(String preferenceName, int defaultValue) {
sSafeMode.onFlagChecked();
CachedFlagsSafeMode.getInstance().onFlagChecked();

if (sValuesOverridden.isEnabled()) {
return sValuesOverridden.getInt(preferenceName, defaultValue);
}

Integer value;
synchronized (sValuesReturned.intValues) {
value = sValuesReturned.intValues.get(preferenceName);
synchronized (ValuesReturned.sIntValues) {
value = ValuesReturned.sIntValues.get(preferenceName);
if (value != null) {
return value;
}

value = sSafeMode.getIntFieldTrialParam(preferenceName, defaultValue);
value = CachedFlagsSafeMode.getInstance().getIntFieldTrialParam(
preferenceName, defaultValue);
if (value == null) {
value = SharedPreferencesManager.getInstance().readInt(
preferenceName, defaultValue);
}

sValuesReturned.intValues.put(preferenceName, value);
ValuesReturned.sIntValues.put(preferenceName, value);
}
return value;
}

@AnyThread
static double getConsistentDoubleValue(String preferenceName, double defaultValue) {
sSafeMode.onFlagChecked();
CachedFlagsSafeMode.getInstance().onFlagChecked();

if (sValuesOverridden.isEnabled()) {
return sValuesOverridden.getDouble(preferenceName, defaultValue);
}

Double value;
synchronized (sValuesReturned.doubleValues) {
value = sValuesReturned.doubleValues.get(preferenceName);
synchronized (ValuesReturned.sDoubleValues) {
value = ValuesReturned.sDoubleValues.get(preferenceName);
if (value != null) {
return value;
}

value = sSafeMode.getDoubleFieldTrialParam(preferenceName, defaultValue);
value = CachedFlagsSafeMode.getInstance().getDoubleFieldTrialParam(
preferenceName, defaultValue);
if (value == null) {
value = SharedPreferencesManager.getInstance().readDouble(
preferenceName, defaultValue);
}

sValuesReturned.doubleValues.put(preferenceName, value);
ValuesReturned.sDoubleValues.put(preferenceName, value);
}
return value;
}

@VisibleForTesting
public static void resetFlagsForTesting() {
sValuesReturned.clearForTesting();
ValuesReturned.clearForTesting();
sValuesOverridden.removeOverrides();
sSafeMode.clearMemoryForTesting();
}

@VisibleForTesting
public static void resetDiskForTesting() {
SharedPreferencesManager.getInstance().removeKeysWithPrefix(
ChromePreferenceKeys.FLAGS_CACHED);
for (Map.Entry<String, CachedFlag> e : ChromeFeatureList.sAllCachedFlags.entrySet()) {
String legacyPreferenceKey = e.getValue().getLegacySharedPreferenceKey();
if (legacyPreferenceKey != null) {
SharedPreferencesManager.getInstance().removeKey(legacyPreferenceKey);
}
}
CachedFlagsSafeMode.getInstance().clearMemoryForTesting();
}

@VisibleForTesting
Expand All @@ -402,7 +313,7 @@ static void setOverrideTestValue(String preferenceKey, String overrideValue) {

@VisibleForTesting
static void setSafeModeExperimentEnabledForTesting(Boolean value) {
sSafeMode.setExperimentEnabledForTesting(value);
CachedFlagsSafeMode.getInstance().setExperimentEnabledForTesting(value);
}

@NativeMethods
Expand Down
4 changes: 2 additions & 2 deletions ...android/java/src/org/chromium/chrome/browser/flags/CachedFieldTrialParameterUnitTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ public void setUp() {
MockitoAnnotations.initMocks(this);

CachedFeatureFlags.resetFlagsForTesting();
CachedFeatureFlags.resetDiskForTesting();
CachedFlag.resetDiskForTesting();

TestValues testValues = new TestValues();

Expand All @@ -122,7 +122,7 @@ public void setUp() {
@After
public void tearDown() {
CachedFeatureFlags.resetFlagsForTesting();
CachedFeatureFlags.resetDiskForTesting();
CachedFlag.resetDiskForTesting();
FeatureList.setTestValues(null);
}

Expand Down

0 comments on commit 5a72c34

Please sign in to comment.