[Android] Use FeatureMap to generalize ContentFeatureList

This reduces code duplication in ContentFeatureList.java and
content_feature_list.cc, reusing the code in base
FeatureMap.java and feature_map.cc.

ContentFeatureList is kept for convenience static methods to
access ContentFeatureMap, but we can inline and update callers in
a follow-up if desired.

Also add a TODO to ContentFeatureList since I noticed the constants
there with feature names largely overlap with the constants in the
generated ContentFeatures.

Using this opportunity to update documentation
(since ContentFeatureList is used as an example) with the new
FeatureMap API.

Change-Id: I2360830bfbceff20714fc65db4dce5ef743ab95a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4544343
Reviewed-by: Mark Schillaci <mschillaci@google.com>
Reviewed-by: Jinsuk Kim <jinsukkim@chromium.org>
Commit-Queue: Henrique Nakashima <hnakashima@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1151372}

components/browser_ui/accessibility/android/java/src/org/chromium/components/browser_ui/accessibility/PageZoomUtilsUnitTest.java

@@ -4,7 +4,6 @@
 
 package org.chromium.components.browser_ui.accessibility;
 
-import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
@@ -21,8 +20,6 @@ import org.mockito.MockitoAnnotations;
 
 import org.chromium.base.test.BaseRobolectricTestRunner;
 import org.chromium.base.test.util.JniMocker;
-import org.chromium.content.browser.ContentFeatureListImpl;
-import org.chromium.content.browser.ContentFeatureListImplJni;
 import org.chromium.content.browser.HostZoomMapImpl;
 import org.chromium.content.browser.HostZoomMapImplJni;
 import org.chromium.content_public.browser.BrowserContextHandle;
@@ -67,9 +64,6 @@ public class PageZoomUtilsUnitTest {
     @Mock
     private HostZoomMapImpl.Natives mHostZoomMapMock;
 
-    @Mock
-    private ContentFeatureListImpl.Natives mContentFeatureListMock;
-
     @Mock
     private BrowserContextHandle mContextMock;
 
@@ -80,7 +74,6 @@ public class PageZoomUtilsUnitTest {
         MockitoAnnotations.initMocks(this);
 
         mJniMocker.mock(HostZoomMapImplJni.TEST_HOOKS, mHostZoomMapMock);
-        mJniMocker.mock(ContentFeatureListImplJni.TEST_HOOKS, mContentFeatureListMock);
     }
 
     @Test
@@ -142,8 +135,7 @@ public class PageZoomUtilsUnitTest {
     }
 
     @Test
-    public void testShouldAlwaysShowZoomMenuItem_featureFlagOff() {
-        when(mContentFeatureListMock.isEnabled(any())).thenReturn(false);
+    public void testShouldAlwaysShowZoomMenuItem_defaultIsFalse() {
         Assert.assertEquals(SHOULD_SHOW_ZOOM_MENU_ITEM_FAILURE_EXPECTED_FALSE, false,
                 PageZoomUtils.shouldAlwaysShowZoomMenuItem());
     }

content/browser/BUILD.gn

@@ -2985,7 +2985,7 @@ source_set("browser") {
       "android/browser_startup_controller.cc",
       "android/browser_startup_controller.h",
       "android/client_data_json_android.cc",
-      "android/content_feature_list.cc",
+      "android/content_feature_map.cc",
       "android/content_startup_flags.cc",
       "android/content_startup_flags.h",
       "android/content_ui_event_handler.cc",

content/browser/android/content_feature_list.cc

@@ -1,91 +0,0 @@
-// Copyright 2017 The Chromium Authors
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "base/android/jni_string.h"
-#include "base/feature_list.h"
-#include "base/notreached.h"
-#include "content/common/features.h"
-#include "content/public/android/content_jni_headers/ContentFeatureListImpl_jni.h"
-#include "content/public/common/content_features.h"
-#include "third_party/blink/public/common/features.h"
-#include "ui/accessibility/accessibility_features.h"
-
-using base::android::ConvertJavaStringToUTF8;
-using base::android::JavaParamRef;
-
-namespace content {
-namespace android {
-
-namespace {
-
-// Array of features exposed through the Java ContentFeatureList API. Entries in
-// this array may either refer to features defined in the header of this file or
-// in other locations in the code base (e.g. content_features.h).
-const base::Feature* const kFeaturesExposedToJava[] = {
-    &blink::features::kStylusPointerAdjustment,
-    &blink::features::kStylusRichGestures,
-    &features::kAccessibilityPageZoom,
-    &features::kAccessibilityPerformanceFiltering,
-    &features::kAutoDisableAccessibilityV2,
-    &features::kBackgroundMediaRendererHasModerateBinding,
-    &features::kFedCm,
-    &features::kOnDemandAccessibilityEvents,
-    &kOptimizeImmHideCalls,
-    &features::kProcessSharingWithStrictSiteInstances,
-    &features::kReduceGpuPriorityOnBackground,
-    &features::kRequestDesktopSiteAdditions,
-    &features::kRequestDesktopSiteExceptions,
-    &features::kTouchDragAndContextMenu,
-    &features::kWebAuthnTouchToFillCredentialSelection,
-    &features::kWebBluetoothNewPermissionsBackend,
-    &features::kWebNfc,
-};
-
-const base::Feature* FindFeatureExposedToJava(const std::string& feature_name) {
-  for (const base::Feature* feature : kFeaturesExposedToJava) {
-    if (feature->name == feature_name) {
-      return feature;
-    }
-  }
-  NOTREACHED() << "Queried feature cannot be found in ContentFeatureList: "
-               << feature_name;
-  return nullptr;
-}
-
-}  // namespace
-
-static jboolean JNI_ContentFeatureListImpl_IsEnabled(
-    JNIEnv* env,
-    const JavaParamRef<jstring>& jfeature_name) {
-  const base::Feature* feature =
-      FindFeatureExposedToJava(ConvertJavaStringToUTF8(env, jfeature_name));
-  return base::FeatureList::IsEnabled(*feature);
-}
-
-static jint JNI_ContentFeatureListImpl_GetFieldTrialParamByFeatureAsInt(
-    JNIEnv* env,
-    const JavaParamRef<jstring>& jfeature_name,
-    const JavaParamRef<jstring>& jparam_name,
-    const jint jdefault_value) {
-  const base::Feature* feature =
-      FindFeatureExposedToJava(ConvertJavaStringToUTF8(env, jfeature_name));
-  const std::string& param_name = ConvertJavaStringToUTF8(env, jparam_name);
-  return base::GetFieldTrialParamByFeatureAsInt(*feature, param_name,
-                                                jdefault_value);
-}
-
-static jboolean JNI_ContentFeatureListImpl_GetFieldTrialParamByFeatureAsBoolean(
-    JNIEnv* env,
-    const JavaParamRef<jstring>& jfeature_name,
-    const JavaParamRef<jstring>& jparam_name,
-    const jboolean jdefault_value) {
-  const base::Feature* feature =
-      FindFeatureExposedToJava(ConvertJavaStringToUTF8(env, jfeature_name));
-  const std::string& param_name = ConvertJavaStringToUTF8(env, jparam_name);
-  return base::GetFieldTrialParamByFeatureAsBool(*feature, param_name,
-                                                 jdefault_value);
-}
-
-}  // namespace android
-}  // namespace content

content/browser/android/content_feature_map.cc

@@ -0,0 +1,54 @@
+// Copyright 2023 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/android/feature_map.h"
+#include "base/feature_list.h"
+#include "base/no_destructor.h"
+#include "content/common/features.h"
+#include "content/public/android/content_jni_headers/ContentFeatureMap_jni.h"
+#include "content/public/common/content_features.h"
+#include "third_party/blink/public/common/features.h"
+#include "ui/accessibility/accessibility_features.h"
+
+namespace content::android {
+
+namespace {
+
+// Array of features exposed through the Java ContentFeatureMap API. Entries in
+// this array may either refer to features defined in the header of this file or
+// in other locations in the code base (e.g. content_features.h).
+const base::Feature* const kFeaturesExposedToJava[] = {
+    &blink::features::kStylusPointerAdjustment,
+    &blink::features::kStylusRichGestures,
+    &features::kAccessibilityPageZoom,
+    &features::kAccessibilityPerformanceFiltering,
+    &features::kAutoDisableAccessibilityV2,
+    &features::kBackgroundMediaRendererHasModerateBinding,
+    &features::kFedCm,
+    &features::kOnDemandAccessibilityEvents,
+    &kOptimizeImmHideCalls,
+    &features::kProcessSharingWithStrictSiteInstances,
+    &features::kReduceGpuPriorityOnBackground,
+    &features::kRequestDesktopSiteAdditions,
+    &features::kRequestDesktopSiteExceptions,
+    &features::kTouchDragAndContextMenu,
+    &features::kWebAuthnTouchToFillCredentialSelection,
+    &features::kWebBluetoothNewPermissionsBackend,
+    &features::kWebNfc,
+};
+
+// static
+base::android::FeatureMap* GetFeatureMap() {
+  static base::NoDestructor<base::android::FeatureMap> kFeatureMap(std::vector(
+      std::begin(kFeaturesExposedToJava), std::end(kFeaturesExposedToJava)));
+  return kFeatureMap.get();
+}
+
+}  // namespace
+
+static jlong JNI_ContentFeatureMap_GetNativeMap(JNIEnv* env) {
+  return reinterpret_cast<jlong>(GetFeatureMap());
+}
+
+}  // namespace content::android

content/public/android/BUILD.gn

@@ -211,7 +211,7 @@ android_library("content_full_java") {
     "java/src/org/chromium/content/browser/ClientDataJsonImpl.java",
     "java/src/org/chromium/content/browser/ContactsDialogHost.java",
     "java/src/org/chromium/content/browser/ContentClassFactory.java",
-    "java/src/org/chromium/content/browser/ContentFeatureListImpl.java",
+    "java/src/org/chromium/content/browser/ContentFeatureMap.java",
     "java/src/org/chromium/content/browser/ContentNfcDelegate.java",
     "java/src/org/chromium/content/browser/ContentUiEventHandler.java",
     "java/src/org/chromium/content/browser/ContentViewStaticsImpl.java",
@@ -440,7 +440,7 @@ generate_jni("content_jni_headers") {
     "java/src/org/chromium/content/browser/ChildProcessLauncherHelperImpl.java",
     "java/src/org/chromium/content/browser/ClientDataJsonImpl.java",
     "java/src/org/chromium/content/browser/ContactsDialogHost.java",
-    "java/src/org/chromium/content/browser/ContentFeatureListImpl.java",
+    "java/src/org/chromium/content/browser/ContentFeatureMap.java",
     "java/src/org/chromium/content/browser/ContentNfcDelegate.java",
     "java/src/org/chromium/content/browser/ContentUiEventHandler.java",
     "java/src/org/chromium/content/browser/ContentViewStaticsImpl.java",

content/public/android/java/src/org/chromium/content/browser/ContentFeatureListImpl.java

@@ -1,63 +0,0 @@
-// Copyright 2019 The Chromium Authors
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-package org.chromium.content.browser;
-
-import org.chromium.base.annotations.JNINamespace;
-import org.chromium.base.annotations.NativeMethods;
-
-/**
- * Implementation of {@link ContentFeatureList}.
- * Java accessor for base/feature_list.h state.
- */
-@JNINamespace("content::android")
-public class ContentFeatureListImpl {
-    /**
-     * Returns whether the specified feature is enabled or not.
-     *
-     * Note: Features queried through this API must be added to the array
-     * |kFeaturesExposedToJava| in content/browser/android/content_feature_list.cc
-     *
-     * @param featureName The name of the feature to query.
-     * @return Whether the feature is enabled or not.
-     */
-    public static boolean isEnabled(String featureName) {
-        return ContentFeatureListImplJni.get().isEnabled(featureName);
-    }
-
-    /**
-     * Returns a field trial param as an int for the specified feature.
-     * {@see ContentFeatureList#getFieldTrialParamByFeatureAsInt}
-     *
-     * Note: Features queried through this API must be added to the array
-     * |kFeaturesExposedToJava| in content/browser/android/content_feature_list.cc
-     */
-    public static int getFieldTrialParamByFeatureAsInt(
-            String featureName, String paramName, int defaultValue) {
-        return ContentFeatureListImplJni.get().getFieldTrialParamByFeatureAsInt(
-                featureName, paramName, defaultValue);
-    }
-
-    /**
-     * Returns a field trial param as a boolean for the specified feature.
-     * {@see ContentFeatureList#getFieldTrialParamByFeatureAsBoolean}
-     *
-     * Note: Features queried through this API must be added to the array
-     * |kFeaturesExposedToJava| in content/browser/android/content_feature_list.cc
-     */
-    public static boolean getFieldTrialParamByFeatureAsBoolean(
-            String featureName, String paramName, boolean defaultValue) {
-        return ContentFeatureListImplJni.get().getFieldTrialParamByFeatureAsBoolean(
-                featureName, paramName, defaultValue);
-    }
-
-    @NativeMethods
-    public interface Natives {
-        boolean isEnabled(String featureName);
-        int getFieldTrialParamByFeatureAsInt(
-                String featureName, String paramName, int defaultValue);
-        boolean getFieldTrialParamByFeatureAsBoolean(
-                String featureName, String paramName, boolean defaultValue);
-    }
-}

content/public/android/java/src/org/chromium/content/browser/ContentFeatureMap.java

@@ -0,0 +1,37 @@
+// Copyright 2023 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.content.browser;
+
+import org.chromium.base.FeatureMap;
+import org.chromium.base.annotations.JNINamespace;
+import org.chromium.base.annotations.NativeMethods;
+
+/**
+ * Java accessor for base::Features listed in content/browser/android/content_feature_map.cc.
+ */
+@JNINamespace("content::android")
+public class ContentFeatureMap extends FeatureMap {
+    private static final ContentFeatureMap sInstance = new ContentFeatureMap();
+
+    // Do not instantiate this class.
+    private ContentFeatureMap() {}
+
+    /**
+     * @return the singleton {@link ContentFeatureMap}
+     */
+    public static ContentFeatureMap getInstance() {
+        return sInstance;
+    }
+
+    @Override
+    protected long getNativeMap() {
+        return ContentFeatureMapJni.get().getNativeMap();
+    }
+
+    @NativeMethods
+    public interface Natives {
+        long getNativeMap();
+    }
+}

content/public/android/java/src/org/chromium/content_public/browser/ContentFeatureList.java

@@ -4,11 +4,10 @@
 
 package org.chromium.content_public.browser;
 
-import org.chromium.base.FeatureList;
-import org.chromium.content.browser.ContentFeatureListImpl;
+import org.chromium.content.browser.ContentFeatureMap;
 
 /**
- * Static public methods for ContentFeatureList.
+ * Convenience static methods to access {@link ContentFeatureMap}.
  */
 public class ContentFeatureList {
     private ContentFeatureList() {}
@@ -20,9 +19,7 @@ public class ContentFeatureList {
      * @return Whether the feature is enabled or not.
      */
     public static boolean isEnabled(String featureName) {
-        Boolean testValue = FeatureList.getTestValueForFeature(featureName);
-        if (testValue != null) return testValue;
-        return ContentFeatureListImpl.isEnabled(featureName);
+        return ContentFeatureMap.getInstance().isEnabled(featureName);
     }
 
     /**
@@ -36,11 +33,7 @@ public class ContentFeatureList {
      */
     public static int getFieldTrialParamByFeatureAsInt(
             String featureName, String paramName, int defaultValue) {
-        String testValue = FeatureList.getTestValueForFieldTrialParam(featureName, paramName);
-        if (testValue != null) return Integer.valueOf(testValue);
-        if (FeatureList.hasTestFeatures()) return defaultValue;
-        assert FeatureList.isInitialized();
-        return ContentFeatureListImpl.getFieldTrialParamByFeatureAsInt(
+        return ContentFeatureMap.getInstance().getFieldTrialParamByFeatureAsInt(
                 featureName, paramName, defaultValue);
     }
 
@@ -55,14 +48,13 @@ public class ContentFeatureList {
      */
     public static boolean getFieldTrialParamByFeatureAsBoolean(
             String featureName, String paramName, boolean defaultValue) {
-        String testValue = FeatureList.getTestValueForFieldTrialParam(featureName, paramName);
-        if (testValue != null) return Boolean.valueOf(testValue);
-        if (FeatureList.hasTestFeatures()) return defaultValue;
-        assert FeatureList.isInitialized();
-        return ContentFeatureListImpl.getFieldTrialParamByFeatureAsBoolean(
+        return ContentFeatureMap.getInstance().getFieldTrialParamByFeatureAsBoolean(
                 featureName, paramName, defaultValue);
     }
 
+    // TODO(crbug.com/1447098): Use generated constants in ContentFeatures and other generated
+    // Features files, then remove the constants below.
+
     // Alphabetical:
     public static final String ACCESSIBILITY_PAGE_ZOOM = "AccessibilityPageZoom";
 

docs/android_accessing_cpp_features_in_java.md

@@ -2,7 +2,42 @@
 
 [TOC]
 
-## Introduction
+# Checking if a Feature is enabled
+
+In C++, add your `base::Feature` to an existing `base::android::FeatureMap` in the appropriate layer/component. Then, you can check the
+enabled state like so:
+
+```java
+// FooFeatureMap can check FooFeatures.MY_FEATURE as long as foo_feature_map.cc
+// adds `kMyFeature` to its `base::android::FeatureMap`.
+if (FooFeatureMap.getInstance().isEnabled(FooFeatures.MY_FEATURE)) {
+    // ...
+}
+```
+
+If the components or layer does not have a FeatureMap, create a new one:
+
+1. In C++, create a new `foo_feature_map.cc` (ex.
+[`content_feature_map`](/content/browser/android/content_feature_map.cc)) with:
+    * `kFeaturesExposedToJava` array with a pointer to your `base::Feature`.
+    * `GetFeatureMap` with a static `base::android::FeatureMap` initialized
+      with `kFeaturesExposedToJava`.
+    * `JNI_FooFeatureList_GetNativeMap` simply calling `GetFeatureMap`.
+2. In Java, create a `FooFeatureMap.java` class extending `FeatureMap.java`
+   (ex. [`ContentFeatureMap`](/content/public/android/java/src/org/chromium/content/browser/ContentFeatureMap.java)) with:
+    * A `getInstance()` that returns the singleton instance.
+    * A single `long getNativeMap()` as @NativeMethods.
+    * An `@Override` for the abstract `getNativeMap()` simply calling
+      `FooFeatureMapJni.get().getNativeMap()`.
+3. Still in Java, `FooFeatures.java` with the String constants with the feature
+   names needs to be generated or created.
+    * Auto-generate it by writing a `FooFeatures.java.tmpl`. [See instructions
+      below]((#generating-foo-feature-list-java)).
+    * If `FooFeatures` cannot be auto-generated, keep the list of String
+      constants with the feature names in a `FooFeatures` or `FooFeatureList`
+      separate from the pure boilerplate `FooFeatureMap`.
+
+# Auto-generating FooFeatureList.java {#generating-foo-feature-list-java}
 
 Accessing C++ `base::Features` in Java is implemented via a Python script which
 analyzes the `*_features.cc` file and generates the corresponding Java class,
@@ -123,7 +158,7 @@ org/chromium/foo/FooFeatures.java:41: error: duplicate declaration of field: MY_
     public static final String MY_FEATURE = "MyFeature";
 ```
 
-This can happen if you've re-declared a feature for mutually-exclsuive build
+This can happen if you've re-declared a feature for mutually-exclusive build
 configs (ex. the feature is enabled-by-default for one config, but
 disabled-by-default for another). Example:
 
@@ -152,28 +187,6 @@ BASE_FEATURE(kMyFeature,
 
 ```
 
-## Checking if a Feature is enabled
-
-The standard pattern is to create a `FooFeatureList.java` class with an
-`isEnabled()` method (ex.
-[`ContentFeatureList`](/content/public/android/java/src/org/chromium/content_public/browser/ContentFeatureList.java)).
-This should call into C++ (ex.
-[`content_feature_list`](/content/browser/android/content_feature_list.cc)),
-where a subset of features are exposed via the `kFeaturesExposedToJava` array.
-You can either add your `base::Feature` to an existing `feature_list` or create
-a new `FeatureList` class if no existing one is suitable. Then you can check the
-enabled state like so:
-
-```java
-// It's OK if ContentFeatureList checks FooFeatures.*, so long as
-// content_feature_list.cc exposes `kMyFeature`.
-if (ContentFeatureList.isEnabled(FooFeatures.MY_FEATURE)) {
-    // ...
-}
-```
-
-At the moment, `base::Features` must be explicitly exposed to Java this way, in
-whichever layer needs to access their state. See https://crbug.com/1060097.
 
 ## See also
 * [Accessing C++ Enums In Java](android_accessing_cpp_enums_in_java.md)