[Field trial params] Getting field trial params by feature name in Java.

chrome/browser/android/chrome_feature_list.cc

Index: chrome/browser/android/chrome_feature_list.cc
diff --git a/chrome/browser/android/chrome_feature_list.cc b/chrome/browser/android/chrome_feature_list.cc
index 5c03d74f36903523914fa2cef46f9923c30b3efc..91e5f0c3a19d0e4a61260eff7191c578b3ae13bd 100644
--- a/chrome/browser/android/chrome_feature_list.cc
+++ b/chrome/browser/android/chrome_feature_list.cc
@@ -21,7 +21,9 @@
 #include "jni/ChromeFeatureList_jni.h"
 
 using base::android::ConvertJavaStringToUTF8;
+using base::android::ConvertUTF8ToJavaString;
 using base::android::JavaParamRef;
+using base::android::ScopedJavaLocalRef;
 
 namespace chrome {
 namespace android {
@@ -72,6 +74,16 @@ const base::Feature* kFeaturesExposedToJava[] = {
     &password_manager::features::kViewPasswords,
 };
 
+const base::Feature FindFeatureExposedToJava(const std::string& feature_name) {
+  for (size_t i = 0; i < arraysize(kFeaturesExposedToJava); ++i) {
+    if (kFeaturesExposedToJava[i]->name == feature_name)
+      return *kFeaturesExposedToJava[i];
+  }
+  NOTREACHED() << "Features queried via ChromeFeatureList must be present in "
+                  "|kFeaturesExposedToJava|.";
+  return base::Feature("", base::FEATURE_DISABLED_BY_DEFAULT);
+}
+
 }  // namespace
 
 // Alphabetical:
@@ -151,14 +163,22 @@ const base::Feature kWebVRCardboardSupport{
 static jboolean IsEnabled(JNIEnv* env,
                           const JavaParamRef<jclass>& clazz,
                           const JavaParamRef<jstring>& jfeature_name) {
-  const std::string feature_name = ConvertJavaStringToUTF8(env, jfeature_name);
-  for (size_t i = 0; i < arraysize(kFeaturesExposedToJava); ++i) {
-    if (kFeaturesExposedToJava[i]->name == feature_name)
-      return base::FeatureList::IsEnabled(*kFeaturesExposedToJava[i]);
-  }
-  // Features queried via this API must be present in |kFeaturesExposedToJava|.
-  NOTREACHED();
-  return false;
+  const base::Feature& feature =
+      FindFeatureExposedToJava(ConvertJavaStringToUTF8(env, jfeature_name));

[Bernhard Bauer, 2017/02/10 16:27:07]

You are passing the returned feature by value, but then you only keep a
reference to it... where would the actual feature object live? The old approach
with returning a pointer would work better. (And you don't actually have to care
about what happens if the method returns a nullptr, because you are explicitly
saying "this should never happen". Crashing from a null pointer dereference if
it does happen is totally fine.)

[jkrcal, 2017/02/10 16:53:44]

Damn, I meant to return by reference. 

The point of the change was to make the contract clear - FindFeature will always
return a feature (or crash) so that the caller does not need to care. Do I
misunderstand the implicit contracts about returning "const type*" and returning
"const type&"?

Do you still prefer me returning by pointer? Why?

[Bernhard Bauer, 2017/02/10 17:56:37]

I don't have a problem with returning a reference per se. The only issue is
going to be making the compiler happy in the case where the feature is not
found. If you return a reference to a temporary object on the stack, it's going
to be invalid once the method returns, and I do realize that I just said earlier
we shouldn't care about what happens if a DCHECK fails, but this would
potentially access invalid memory, which is security-critical, so avoiding that
takes precedence over style.

You could return a null reference (which can in fact exist, see
https://groups.google.com/a/chromium.org/d/msg/cxx/j5rFewBzSBQ/8I8-MJ_9AAAJ),
but that is undefined behavior, so it might trigger a warning if we're
sanitizing for that. Returning a pointer would avoid that, as we would never
have code that is guaranteed to dereference a null pointer, and given that you
control all the callers, you can just _make_ them not care :)

[jkrcal, 2017/02/13 07:57:27]

Oh, clear! Thanks for bearing with me and explaining in detail! That was really
helpful!

+  return base::FeatureList::IsEnabled(feature);
+}
+
+static ScopedJavaLocalRef<jstring> GetFieldTrialParamByFeature(
+    JNIEnv* env,
+    const JavaParamRef<jclass>& clazz,
+    const JavaParamRef<jstring>& jfeature_name,
+    const JavaParamRef<jstring>& jparam_name) {
+  const base::Feature& feature =
+      FindFeatureExposedToJava(ConvertJavaStringToUTF8(env, jfeature_name));
+  const std::string& param_name = ConvertJavaStringToUTF8(env, jparam_name);
+  const std::string& param_value =
+      base::GetFieldTrialParamValueByFeature(feature, param_name);
+  return ConvertUTF8ToJavaString(env, param_value);
 }
 
 static jint GetFieldTrialParamByFeatureAsInt(
@@ -167,18 +187,37 @@ static jint GetFieldTrialParamByFeatureAsInt(
     const JavaParamRef<jstring>& jfeature_name,
     const JavaParamRef<jstring>& jparam_name,
     const jint jdefault_value) {
-  const std::string feature_name = ConvertJavaStringToUTF8(env, jfeature_name);
-  const std::string param_name = ConvertJavaStringToUTF8(env, jparam_name);
-  int default_value = static_cast<int>(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);
+}
 
-  for (size_t i = 0; i < arraysize(kFeaturesExposedToJava); ++i) {
-    if (kFeaturesExposedToJava[i]->name == feature_name)
-      return base::GetFieldTrialParamByFeatureAsInt(
-          *kFeaturesExposedToJava[i], param_name, default_value);
-  }
-  // Features queried via this API must be present in |kFeaturesExposedToJava|.
-  NOTREACHED();
-  return jdefault_value;
+static jdouble GetFieldTrialParamByFeatureAsDouble(
+    JNIEnv* env,
+    const JavaParamRef<jclass>& clazz,
+    const JavaParamRef<jstring>& jfeature_name,
+    const JavaParamRef<jstring>& jparam_name,
+    const jdouble jdefault_value) {
+  const base::Feature& feature =
+      FindFeatureExposedToJava(ConvertJavaStringToUTF8(env, jfeature_name));
+  const std::string& param_name = ConvertJavaStringToUTF8(env, jparam_name);
+  return base::GetFieldTrialParamByFeatureAsDouble(feature, param_name,
+                                                   jdefault_value);
+}
+
+static jboolean GetFieldTrialParamByFeatureAsBoolean(
+    JNIEnv* env,
+    const JavaParamRef<jclass>& clazz,
+    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);
 }
 
 bool RegisterChromeFeatureListJni(JNIEnv* env) {