AW UMA: componentize GetAppPackageNameInternal

No change to logic. This moves GetAppPackageNameInternal into the shared
component, reducing code duplication and making it an implementation
detail of the class.

Bug: 1015655
Test: run_components_unittests -f AndroidMetricsServiceClientTest.*
Test: run_android_webview_unittests -f AwMetricsServiceClientTest.*
Change-Id: I6612046d07dd75e781d9a5464a66ec8c0f16f625
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2067657Reviewed-by: Tobias Sargeant <tobiasjs@chromium.org>
Commit-Queue: Nate Fischer <ntfschr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#745729}

android_webview/BUILD.gn

@@ -463,6 +463,7 @@ android_library("browser_java") {
     "//components/crash/android:handler_java",
     "//components/crash/android:java",
     "//components/embedder_support/android:web_contents_delegate_java",
+    "//components/embedder_support/android/metrics:java",
     "//components/minidump_uploader:minidump_uploader_java",
     "//components/navigation_interception/android:navigation_interception_java",
     "//components/policy/android:policy_java",

android_webview/browser/metrics/aw_metrics_service_client.cc

@@ -12,7 +12,6 @@
 #include "android_webview/browser_jni_headers/AwMetricsServiceClient_jni.h"
 #include "android_webview/common/aw_features.h"
 #include "base/android/jni_android.h"
-#include "base/android/jni_string.h"
 #include "base/feature_list.h"
 #include "base/metrics/histogram_functions.h"
 #include "base/no_destructor.h"
@@ -181,22 +180,6 @@ void AwMetricsServiceClient::RegisterAdditionalMetricsProviders(
           pref_service()));
 }
 
-std::string AwMetricsServiceClient::GetAppPackageNameInternal() {
-  JNIEnv* env = base::android::AttachCurrentThread();
-  base::android::ScopedJavaLocalRef<jstring> j_app_name =
-      Java_AwMetricsServiceClient_getAppPackageName(env);
-  if (j_app_name)
-    return ConvertJavaStringToUTF8(env, j_app_name);
-  return std::string();
-}
-
-bool AwMetricsServiceClient::CanRecordPackageNameForAppType() {
-  // Check with Java side, to see if it's OK to log the package name for this
-  // type of app (see Java side for the specific requirements).
-  JNIEnv* env = base::android::AttachCurrentThread();
-  return Java_AwMetricsServiceClient_canRecordPackageNameForAppType(env);
-}
-
 // static
 void JNI_AwMetricsServiceClient_SetHaveMetricsConsent(JNIEnv* env,
                                                       jboolean user_consent,

android_webview/browser/metrics/aw_metrics_service_client.h

@@ -113,8 +113,6 @@ class AwMetricsServiceClient : public ::metrics::AndroidMetricsServiceClient,
   bool ShouldWakeMetricsService() override;
   void RegisterAdditionalMetricsProviders(
       metrics::MetricsService* service) override;
-  bool CanRecordPackageNameForAppType() override;
-  std::string GetAppPackageNameInternal() override;
 
  private:
   bool app_in_foreground_ = false;

android_webview/browser/metrics/aw_metrics_service_client_unittest.cc

@@ -108,14 +108,6 @@ TEST_F(AwMetricsServiceClientTest, TestBackfillEnabledDateIfMissing) {
       prefs->HasPrefPath(metrics::prefs::kMetricsReportingEnabledTimestamp));
 }
 
-TEST_F(AwMetricsServiceClientTest, TestGetPackageNameInternal) {
-  auto prefs = CreateTestPrefs();
-  prefs->SetString(metrics::prefs::kMetricsClientID, kTestClientId);
-  auto client = CreateAndInitTestClient(prefs.get());
-  // Make sure GetPackageNameInternal returns a non-empty string.
-  EXPECT_FALSE(client->GetAppPackageNameInternal().empty());
-}
-
 // TODO(https://crbug.com/1012025): remove this when the kInstallDate pref has
 // been persisted for one or two milestones.
 TEST_F(AwMetricsServiceClientTest, TestPreferPersistedInstallDate) {

android_webview/java/src/org/chromium/android_webview/metrics/AwMetricsServiceClient.java

@@ -29,8 +29,6 @@ public class AwMetricsServiceClient {
     // reporting. See https://developer.android.com/reference/android/webkit/WebView.html
     private static final String OPT_OUT_META_DATA_STR = "android.webkit.WebView.MetricsOptOut";
 
-    private static final String PLAY_STORE_PACKAGE_NAME = "com.android.vending";
-
     private static boolean isAppOptedOut(Context ctx) {
         try {
             ApplicationInfo info = ctx.getPackageManager().getApplicationInfo(
@@ -49,16 +47,6 @@ public class AwMetricsServiceClient {
         }
     }
 
-    @CalledByNative
-    private static boolean canRecordPackageNameForAppType() {
-        // Only record if it's a system app or it was installed from Play Store.
-        Context ctx = ContextUtils.getApplicationContext();
-        String packageName = ctx.getPackageName();
-        String installerPackageName = ctx.getPackageManager().getInstallerPackageName(packageName);
-        return (ctx.getApplicationInfo().flags & ApplicationInfo.FLAG_SYSTEM) != 0
-                || (PLAY_STORE_PACKAGE_NAME.equals(installerPackageName));
-    }
-
     public static void setConsentSetting(Context ctx, boolean userConsent) {
         ThreadUtils.assertOnUiThread();
         AwMetricsServiceClientJni.get().setHaveMetricsConsent(userConsent, !isAppOptedOut(ctx));
@@ -74,14 +62,6 @@ public class AwMetricsServiceClient {
         AwMetricsServiceClientJni.get().setUploadIntervalForTesting(uploadIntervalMs);
     }
 
-    @CalledByNative
-    private static String getAppPackageName() {
-        // Return this unconditionally; let native code enforce whether or not it's OK to include
-        // this in the logs.
-        Context ctx = ContextUtils.getApplicationContext();
-        return ctx.getPackageName();
-    }
-
     /**
      * Gets a long representing the install time of the embedder application. Units are in seconds,
      * as this is the resolution used by the metrics service. Returns {@code -1} upon failure.

components/embedder_support/android/metrics/BUILD.gn

@@ -35,6 +35,7 @@ static_library("metrics") {
 android_library("java") {
   sources = [
     "java/src/org/chromium/components/metrics/AndroidMetricsLogUploader.java",
+    "java/src/org/chromium/components/metrics/AndroidMetricsServiceClient.java",
   ]
 
   deps = [
@@ -47,6 +48,7 @@ android_library("java") {
 generate_jni("jni") {
   sources = [
     "java/src/org/chromium/components/metrics/AndroidMetricsLogUploader.java",
+    "java/src/org/chromium/components/metrics/AndroidMetricsServiceClient.java",
   ]
 }
 
@@ -67,5 +69,8 @@ source_set("unit_tests") {
 # target.
 java_group("test_support_java") {
   testonly = true
-  deps = [ "//components/version_info/android:version_constants_java" ]
+  deps = [
+    ":java",
+    "//components/version_info/android:version_constants_java",
+  ]
 }

components/embedder_support/android/metrics/android_metrics_service_client.cc

@@ -4,12 +4,16 @@
 
 #include "components/embedder_support/android/metrics/android_metrics_service_client.h"
 
+#include <jni.h>
 #include <cstdint>
 
+#include "base/android/jni_android.h"
+#include "base/android/jni_string.h"
 #include "base/base_paths_android.h"
 #include "base/i18n/rtl.h"
 #include "build/build_config.h"
 #include "components/embedder_support/android/metrics/android_metrics_log_uploader.h"
+#include "components/embedder_support/android/metrics/jni/AndroidMetricsServiceClient_jni.h"
 #include "components/metrics/android_metrics_provider.h"
 #include "components/metrics/call_stack_profile_metrics_provider.h"
 #include "components/metrics/cpu_metrics_provider.h"
@@ -288,6 +292,13 @@ bool AndroidMetricsServiceClient::IsInSample() {
   return GetSampleBucketValue() < GetSampleRatePerMille();
 }
 
+bool AndroidMetricsServiceClient::CanRecordPackageNameForAppType() {
+  // Check with Java side, to see if it's OK to log the package name for this
+  // type of app (see Java side for the specific requirements).
+  JNIEnv* env = base::android::AttachCurrentThread();
+  return Java_AndroidMetricsServiceClient_canRecordPackageNameForAppType(env);
+}
+
 bool AndroidMetricsServiceClient::IsInPackageNameSample() {
   // Check if this client falls within the group for which it's acceptable to
   // log package name. This guarantees we enforce the privacy requirement
@@ -307,4 +318,13 @@ std::string AndroidMetricsServiceClient::GetAppPackageName() {
   return std::string();
 }
 
+std::string AndroidMetricsServiceClient::GetAppPackageNameInternal() {
+  JNIEnv* env = base::android::AttachCurrentThread();
+  base::android::ScopedJavaLocalRef<jstring> j_app_name =
+      Java_AndroidMetricsServiceClient_getAppPackageName(env);
+  if (j_app_name)
+    return ConvertJavaStringToUTF8(env, j_app_name);
+  return std::string();
+}
+
 }  // namespace metrics

components/embedder_support/android/metrics/android_metrics_service_client.h

@@ -162,8 +162,8 @@ class AndroidMetricsServiceClient : public MetricsServiceClient,
 
   // Determines if the embedder app is the type of app for which we may log the
   // package name. If this returns false, GetAppPackageName() must return empty
-  // string.
-  virtual bool CanRecordPackageNameForAppType() = 0;
+  // string. Virtual for testing.
+  virtual bool CanRecordPackageNameForAppType();
 
   // Determines if this client falls within the group for which it's acceptable
   // to include the embedding app's package name. If this returns false,
@@ -185,8 +185,9 @@ class AndroidMetricsServiceClient : public MetricsServiceClient,
   // MetricsProviders. Does nothing by default.
   virtual void RegisterAdditionalMetricsProviders(MetricsService* service);
 
-  // Returns the embedding application's package name.
-  virtual std::string GetAppPackageNameInternal() = 0;
+  // Returns the embedding application's package name (unconditionally). Virtual
+  // for testing.
+  virtual std::string GetAppPackageNameInternal();
 
   void EnsureOnValidSequence() const {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

components/embedder_support/android/metrics/android_metrics_service_client_unittest.cc

@@ -66,6 +66,7 @@ class TestClient : public AndroidMetricsServiceClient {
   void SetSampleBucketValue(int per_mille) { sample_bucket_value_ = per_mille; }
 
   // Expose the super class implementation for testing.
+  using AndroidMetricsServiceClient::GetAppPackageNameInternal;
   using AndroidMetricsServiceClient::IsInPackageNameSample;
   using AndroidMetricsServiceClient::IsInSample;
 
@@ -98,8 +99,6 @@ class TestClient : public AndroidMetricsServiceClient {
 
   void RegisterAdditionalMetricsProviders(MetricsService* service) override {}
 
-  std::string GetAppPackageNameInternal() override { return "TestPackage"; }
-
  private:
   int sample_bucket_value_;
   int sampled_in_rate_per_mille_;
@@ -249,6 +248,14 @@ TEST_F(AndroidMetricsServiceClientTest, TestCanUploadPackageName) {
   EXPECT_FALSE(package_name.empty());
 }
 
+TEST_F(AndroidMetricsServiceClientTest, TestGetPackageNameInternal) {
+  auto prefs = CreateTestPrefs();
+  prefs->SetString(metrics::prefs::kMetricsClientID, kTestClientId);
+  auto client = CreateAndInitTestClient(prefs.get());
+  // Make sure GetPackageNameInternal returns a non-empty string.
+  EXPECT_FALSE(client->GetAppPackageNameInternal().empty());
+}
+
 TEST_F(AndroidMetricsServiceClientTest,
        TestPackageNameLogic_SampleRateBelowPackageNameRate) {
   auto prefs = CreateTestPrefs();

components/embedder_support/android/metrics/java/src/org/chromium/components/metrics/AndroidMetricsServiceClient.java

+// Copyright 2020 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.
+
+package org.chromium.components.metrics;
+
+import android.content.Context;
+import android.content.pm.ApplicationInfo;
+
+import org.chromium.base.ContextUtils;
+import org.chromium.base.annotations.CalledByNative;
+import org.chromium.base.annotations.JNINamespace;
+
+/**
+ * Helps the native AndroidMetricsServiceClient call Android Java APIs over JNI.
+ */
+@JNINamespace("metrics")
+public class AndroidMetricsServiceClient {
+    private static final String PLAY_STORE_PACKAGE_NAME = "com.android.vending";
+
+    @CalledByNative
+    private static boolean canRecordPackageNameForAppType() {
+        // Only record if it's a system app or it was installed from Play Store.
+        Context ctx = ContextUtils.getApplicationContext();
+        String packageName = ctx.getPackageName();
+        String installerPackageName = ctx.getPackageManager().getInstallerPackageName(packageName);
+        return (ctx.getApplicationInfo().flags & ApplicationInfo.FLAG_SYSTEM) != 0
+                || (PLAY_STORE_PACKAGE_NAME.equals(installerPackageName));
+    }
+
+    @CalledByNative
+    private static String getAppPackageName() {
+        // Return this unconditionally; let native code enforce whether or not it's OK to include
+        // this in the logs.
+        Context ctx = ContextUtils.getApplicationContext();
+        return ctx.getPackageName();
+    }
+}

weblayer/browser/android/metrics/weblayer_metrics_service_client.cc

@@ -8,8 +8,6 @@
 #include <cstdint>
 #include <memory>
 
-#include "base/android/jni_android.h"
-#include "base/android/jni_string.h"
 #include "base/no_destructor.h"
 #include "components/metrics/metrics_service.h"
 #include "components/version_info/android/channel_getter.h"
@@ -54,15 +52,6 @@ int32_t WebLayerMetricsServiceClient::GetProduct() {
   return metrics::ChromeUserMetricsExtension::ANDROID_WEBLAYER;
 }
 
-std::string WebLayerMetricsServiceClient::GetAppPackageNameInternal() {
-  JNIEnv* env = base::android::AttachCurrentThread();
-  base::android::ScopedJavaLocalRef<jstring> j_app_name =
-      Java_MetricsServiceClient_getAppPackageName(env);
-  if (j_app_name)
-    return ConvertJavaStringToUTF8(env, j_app_name);
-  return std::string();
-}
-
 int WebLayerMetricsServiceClient::GetSampleRatePerMille() {
   version_info::Channel channel = version_info::android::GetChannel();
   if (channel == version_info::Channel::STABLE ||
@@ -84,13 +73,6 @@ bool WebLayerMetricsServiceClient::ShouldWakeMetricsService() {
   return true;
 }
 
-bool WebLayerMetricsServiceClient::CanRecordPackageNameForAppType() {
-  // Check with Java side, to see if it's OK to log the package name for this
-  // type of app (see Java side for the specific requirements).
-  JNIEnv* env = base::android::AttachCurrentThread();
-  return Java_MetricsServiceClient_canRecordPackageNameForAppType(env);
-}
-
 // static
 void JNI_MetricsServiceClient_SetHaveMetricsConsent(JNIEnv* env,
                                                     jboolean user_consent,

weblayer/browser/android/metrics/weblayer_metrics_service_client.h

@@ -40,8 +40,6 @@ class WebLayerMetricsServiceClient
   void OnMetricsStart() override;
   int GetPackageNameLimitRatePerMille() override;
   bool ShouldWakeMetricsService() override;
-  bool CanRecordPackageNameForAppType() override;
-  std::string GetAppPackageNameInternal() override;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(WebLayerMetricsServiceClient);

weblayer/browser/java/BUILD.gn

@@ -78,6 +78,7 @@ android_library("java") {
     "//components/embedder_support/android:application_java",
     "//components/embedder_support/android:context_menu_java",
     "//components/embedder_support/android:web_contents_delegate_java",
+    "//components/embedder_support/android/metrics:java",
     "//components/find_in_page/android:java",
     "//components/javascript_dialogs/android:java",
     "//components/metrics:metrics_java",

weblayer/browser/java/org/chromium/weblayer_private/metrics/MetricsServiceClient.java

@@ -10,7 +10,6 @@ import android.content.pm.PackageManager;
 
 import org.chromium.base.ContextUtils;
 import org.chromium.base.Log;
-import org.chromium.base.annotations.CalledByNative;
 import org.chromium.base.annotations.JNINamespace;
 import org.chromium.base.annotations.NativeMethods;
 import org.chromium.weblayer_private.GmsBridge;
@@ -25,8 +24,6 @@ import org.chromium.weblayer_private.GmsBridge;
 public class MetricsServiceClient {
     private static final String TAG = "MetricsServiceClie-";
 
-    private static final String PLAY_STORE_PACKAGE_NAME = "com.android.vending";
-
     // Individual apps can use this meta-data tag in their manifest to opt out of metrics.
     private static final String AUTO_UPLOAD_METADATA_STR = "android.WebLayer.MetricsAutoUpload";
 
@@ -55,24 +52,6 @@ public class MetricsServiceClient {
         });
     }
 
-    @CalledByNative
-    private static String getAppPackageName() {
-        // Return this unconditionally; let native code enforce whether or not it's OK to include
-        // this in the logs.
-        Context ctx = ContextUtils.getApplicationContext();
-        return ctx.getPackageName();
-    }
-
-    @CalledByNative
-    private static boolean canRecordPackageNameForAppType() {
-        // Only record if it's a system app or it was installed from Play Store.
-        Context ctx = ContextUtils.getApplicationContext();
-        String packageName = ctx.getPackageName();
-        String installerPackageName = ctx.getPackageManager().getInstallerPackageName(packageName);
-        return (ctx.getApplicationInfo().flags & ApplicationInfo.FLAG_SYSTEM) != 0
-                || (PLAY_STORE_PACKAGE_NAME.equals(installerPackageName));
-    }
-
     @NativeMethods
     interface Natives {
         void setHaveMetricsConsent(boolean userConsent, boolean appConsent);