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

chrome/browser/android/chrome_feature_list.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/android/chrome_feature_list.h"
 
 #include <stddef.h>
 
 #include <string>
 
 #include "base/android/jni_string.h"
 #include "base/feature_list.h"
 #include "base/macros.h"
 #include "base/metrics/field_trial_params.h"
 #include "chrome/common/chrome_features.h"
 #include "components/autofill/core/browser/autofill_experiments.h"
 #include "components/ntp_snippets/features.h"
 #include "components/offline_pages/core/offline_page_feature.h"
 #include "components/password_manager/core/common/password_manager_features.h"
 #include "content/public/common/content_features.h"
 #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 {
 
 namespace {
 
 // Array of features exposed through the Java ChromeFeatureList 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. chrome/, components/, etc).
 const base::Feature* kFeaturesExposedToJava[] = {
@@ skipping 30 matching lines @@
     &ntp_snippets::kIncreasedVisibility,
     &ntp_snippets::kForeignSessionsSuggestionsFeature,
     &ntp_snippets::kOfflineBadgeFeature,
     &ntp_snippets::kSaveToOfflineFeature,
     &offline_pages::kBackgroundLoaderForDownloadsFeature,
     &offline_pages::kOfflinePagesCTFeature,  // See crbug.com/620421.
     &offline_pages::kOfflinePagesSharingFeature,
     &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:
 const base::Feature kAndroidPayIntegrationV1{"AndroidPayIntegrationV1",
                                              base::FEATURE_ENABLED_BY_DEFAULT};
 
 const base::Feature kAndroidPayIntegrationV2{"AndroidPayIntegrationV2",
                                              base::FEATURE_DISABLED_BY_DEFAULT};
 
 const base::Feature kAndroidPaymentApps{"AndroidPaymentApps",
@@ skipping 59 matching lines @@
 
 const base::Feature kWebPaymentsSingleAppUiSkip{
     "WebPaymentsSingleAppUiSkip", base::FEATURE_ENABLED_BY_DEFAULT};
 
 const base::Feature kWebVRCardboardSupport{
     "WebVRCardboardSupport", base::FEATURE_ENABLED_BY_DEFAULT};
 
 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(
     JNIEnv* env,
     const JavaParamRef<jclass>& clazz,
     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) {
   return RegisterNativesImpl(env);
 }
 
 }  // namespace android
 }  // namespace chrome