🍱 Remove OriginVerifier dependency on BrowserServicesMetrics.

Change-Id: I785b071c73e7006bf68b25c3fbd6f852d470299a
Bug: 1164866
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2621062Reviewed-by: Ella Ge <eirage@chromium.org>
Commit-Queue: Peter Conn <peconn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842058}

chrome/android/java/src/org/chromium/chrome/browser/browserservices/BrowserServicesMetrics.java

@@ -6,44 +6,23 @@ package org.chromium.chrome.browser.browserservices;
 
 import android.os.SystemClock;
 
-import androidx.annotation.IntDef;
-
 import org.chromium.base.metrics.RecordHistogram;
-
-import java.lang.annotation.Retention;
-import java.lang.annotation.RetentionPolicy;
+import org.chromium.chrome.browser.browserservices.verification.OriginVerifier;
 
 /**
  * Class to contain metrics recording constants and behaviour for Browser Services.
  */
 public class BrowserServicesMetrics {
-    @IntDef({VerificationResult.ONLINE_SUCCESS, VerificationResult.ONLINE_FAILURE,
-            VerificationResult.OFFLINE_SUCCESS, VerificationResult.OFFLINE_FAILURE,
-            VerificationResult.HTTPS_FAILURE, VerificationResult.REQUEST_FAILURE,
-            VerificationResult.CACHED_SUCCESS})
-    @Retention(RetentionPolicy.SOURCE)
-    public @interface VerificationResult {
-        // Don't reuse values or reorder values. If you add something new, change NUM_ENTRIES as
-        // well.
-        int ONLINE_SUCCESS = 0;
-        int ONLINE_FAILURE = 1;
-        int OFFLINE_SUCCESS = 2;
-        int OFFLINE_FAILURE = 3;
-        int HTTPS_FAILURE = 4;
-        int REQUEST_FAILURE = 5;
-        int CACHED_SUCCESS = 6;
-        int NUM_ENTRIES = 7;
-    }
-
-    /**
-     * Records the verification result for Trusted Web Activity verification.
-     */
-    public static void recordVerificationResult(@VerificationResult int result) {
-        RecordHistogram.recordEnumeratedHistogram(
-                "BrowserServices.VerificationResult", result, VerificationResult.NUM_ENTRIES);
+    /** Implementation of {@link OriginVerifier.MetricsListener}. */
+    public static class OriginVerifierMetricsListener implements OriginVerifier.MetricsListener {
+        @Override
+        public void recordVerificationResult(@OriginVerifier.VerificationResult int result) {
+            RecordHistogram.recordEnumeratedHistogram("BrowserServices.VerificationResult", result,
+                    OriginVerifier.VerificationResult.NUM_ENTRIES);
         }
 
-    public static void recordVerificationTime(long duration, boolean online) {
+        @Override
+        public void recordVerificationTime(long duration, boolean online) {
             if (online) {
                 RecordHistogram.recordTimesHistogram(
                         "BrowserServices.VerificationTime.Online", duration);
@@ -52,6 +31,7 @@ public class BrowserServicesMetrics {
                         "BrowserServices.VerificationTime.Offline", duration);
             }
         }
+    }
 
     /**
      * Returns a {@link TimingMetric} that records the amount of time spent querying the Android

chrome/android/java/src/org/chromium/chrome/browser/browserservices/ui/controller/trustedwebactivity/TwaVerifier.java

@@ -9,6 +9,7 @@ import androidx.browser.customtabs.CustomTabsService;
 
 import org.chromium.base.Promise;
 import org.chromium.chrome.browser.browserservices.BrowserServicesIntentDataProvider;
+import org.chromium.chrome.browser.browserservices.BrowserServicesMetrics;
 import org.chromium.chrome.browser.browserservices.ui.controller.Verifier;
 import org.chromium.chrome.browser.browserservices.verification.OriginVerifier;
 import org.chromium.chrome.browser.customtabs.content.CustomTabActivityTabProvider;
@@ -62,8 +63,9 @@ public class TwaVerifier implements Verifier, Destroyable {
         // TODO(peconn): See if we can get rid of the dependency on Web Contents.
         WebContents webContents =
                 tabProvider.getTab() != null ? tabProvider.getTab().getWebContents() : null;
-        mOriginVerifier = originVerifierFactory.create(
-                clientPackageNameProvider.get(), RELATIONSHIP, webContents, externalAuthUtils);
+        mOriginVerifier = originVerifierFactory.create(clientPackageNameProvider.get(),
+                RELATIONSHIP, webContents, externalAuthUtils,
+                new BrowserServicesMetrics.OriginVerifierMetricsListener());
 
         lifecycleDispatcher.register(this);
     }

chrome/android/java/src/org/chromium/chrome/browser/browserservices/verification/DEPS

@@ -2,7 +2,6 @@ noparent = True
 
 include_rules = [
   "+base/android/java/src/org/chromium/base",
-  "!chrome/android/java/src/org/chromium/chrome/browser/browserservices",
   "+chrome/browser/preferences/android/java/src/org/chromium/chrome/browser/preferences",
   "+chrome/browser/profiles/android/java/src/org/chromium/chrome/browser/profiles",
   "+components/embedder_support/android/java/src/org/chromium/components/embedder_support/util",

chrome/android/java/src/org/chromium/chrome/browser/browserservices/verification/OriginVerifier.java

@@ -11,6 +11,7 @@ import android.net.Uri;
 import android.os.SystemClock;
 import android.text.TextUtils;
 
+import androidx.annotation.IntDef;
 import androidx.annotation.NonNull;
 import androidx.annotation.Nullable;
 import androidx.annotation.VisibleForTesting;
@@ -27,7 +28,6 @@ import org.chromium.base.annotations.CalledByNative;
 import org.chromium.base.annotations.JNINamespace;
 import org.chromium.base.annotations.NativeMethods;
 import org.chromium.base.task.PostTask;
-import org.chromium.chrome.browser.browserservices.BrowserServicesMetrics;
 import org.chromium.chrome.browser.flags.ChromeSwitches;
 import org.chromium.chrome.browser.profiles.Profile;
 import org.chromium.components.embedder_support.util.Origin;
@@ -39,6 +39,8 @@ import org.chromium.content_public.browser.WebContents;
 
 import java.io.ByteArrayInputStream;
 import java.io.InputStream;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
 import java.security.MessageDigest;
 import java.security.NoSuchAlgorithmException;
 import java.security.cert.CertificateEncodingException;
@@ -81,6 +83,7 @@ public class OriginVerifier {
     private long mNativeOriginVerifier;
     private final Map<Origin, Set<OriginVerificationListener>> mListeners = new HashMap<>();
     private long mVerificationStartTime;
+    private final MetricsListener mMetricsListener;
     @Nullable
     private WebContents mWebContents;
     @Nullable
@@ -93,6 +96,38 @@ public class OriginVerifier {
     private static final AtomicReference<Set<String>> sVerificationOverrides =
             new AtomicReference<>();
 
+    @IntDef({VerificationResult.ONLINE_SUCCESS, VerificationResult.ONLINE_FAILURE,
+            VerificationResult.OFFLINE_SUCCESS, VerificationResult.OFFLINE_FAILURE,
+            VerificationResult.HTTPS_FAILURE, VerificationResult.REQUEST_FAILURE,
+            VerificationResult.CACHED_SUCCESS})
+    @Retention(RetentionPolicy.SOURCE)
+    public @interface VerificationResult {
+        // Don't reuse values or reorder values. If you add something new, change NUM_ENTRIES as
+        // well.
+        int ONLINE_SUCCESS = 0;
+        int ONLINE_FAILURE = 1;
+        int OFFLINE_SUCCESS = 2;
+        int OFFLINE_FAILURE = 3;
+        int HTTPS_FAILURE = 4;
+        int REQUEST_FAILURE = 5;
+        int CACHED_SUCCESS = 6;
+        int NUM_ENTRIES = 7;
+    }
+
+    /**
+     * Interface for recording metrics.
+     */
+    public interface MetricsListener {
+        /** Called with the result of every verification attempt. */
+        default void recordVerificationResult(@VerificationResult int result) {}
+
+        /**
+         * Records the time verification takes. This is not recorded for HTTPS_FAILURE,
+         * HTTPS_FAILURE or CACHED_SUCCESS.
+         */
+        default void recordVerificationTime(long duration, boolean online) {}
+    }
+
     /**
      * Factory that can be injected by Dagger.
      */
@@ -101,9 +136,17 @@ public class OriginVerifier {
         @Inject
         public Factory() {}
 
+        public OriginVerifier create(String packageName, @Relation int relation,
+                @Nullable WebContents webContents, @Nullable ExternalAuthUtils externalAuthUtils,
+                MetricsListener metricsListener) {
+            return new OriginVerifier(
+                    packageName, relation, webContents, externalAuthUtils, metricsListener);
+        }
+
         public OriginVerifier create(String packageName, @Relation int relation,
                 @Nullable WebContents webContents, @Nullable ExternalAuthUtils externalAuthUtils) {
-            return new OriginVerifier(packageName, relation, webContents, externalAuthUtils);
+            return create(packageName, relation, webContents, externalAuthUtils,
+                    new MetricsListener() {});
         }
     }
 
@@ -225,12 +268,14 @@ public class OriginVerifier {
      *         verification. Can be null.
      */
     public OriginVerifier(String packageName, @Relation int relation,
-            @Nullable WebContents webContents, @Nullable ExternalAuthUtils externalAuthUtils) {
+            @Nullable WebContents webContents, @Nullable ExternalAuthUtils externalAuthUtils,
+            MetricsListener metricsListener) {
         mPackageName = packageName;
         mSignatureFingerprint = getCertificateSHA256FingerprintForPackage(mPackageName);
         mRelation = relation;
         mWebContents = webContents;
         mExternalAuthUtils = externalAuthUtils;
+        mMetricsListener = metricsListener;
     }
 
     /**
@@ -266,8 +311,7 @@ public class OriginVerifier {
         if (TextUtils.isEmpty(scheme)
                 || !UrlConstants.HTTPS_SCHEME.equals(scheme.toLowerCase(Locale.US))) {
             Log.i(TAG, "Verification failed for %s as not https.", origin);
-            BrowserServicesMetrics.recordVerificationResult(
-                    BrowserServicesMetrics.VerificationResult.HTTPS_FAILURE);
+            mMetricsListener.recordVerificationResult(VerificationResult.HTTPS_FAILURE);
             PostTask.runOrPostTask(
                     UiThreadTaskTraits.DEFAULT, new VerifiedCallback(origin, false, null));
             return;
@@ -318,8 +362,7 @@ public class OriginVerifier {
                 OriginVerifierJni.get().verifyOrigin(mNativeOriginVerifier, OriginVerifier.this,
                         mPackageName, mSignatureFingerprint, origin.toString(), relationship);
         if (!requestSent) {
-            BrowserServicesMetrics.recordVerificationResult(
-                    BrowserServicesMetrics.VerificationResult.REQUEST_FAILURE);
+            mMetricsListener.recordVerificationResult(VerificationResult.REQUEST_FAILURE);
             PostTask.runOrPostTask(
                     UiThreadTaskTraits.DEFAULT, new VerifiedCallback(origin, false, false));
         }
@@ -414,13 +457,11 @@ public class OriginVerifier {
         Origin origin = Origin.createOrThrow(originAsString);
         switch (result) {
             case RelationshipCheckResult.SUCCESS:
-                BrowserServicesMetrics.recordVerificationResult(
-                        BrowserServicesMetrics.VerificationResult.ONLINE_SUCCESS);
+                mMetricsListener.recordVerificationResult(VerificationResult.ONLINE_SUCCESS);
                 originVerified(origin, true, true);
                 break;
             case RelationshipCheckResult.FAILURE:
-                BrowserServicesMetrics.recordVerificationResult(
-                        BrowserServicesMetrics.VerificationResult.ONLINE_FAILURE);
+                mMetricsListener.recordVerificationResult(VerificationResult.ONLINE_FAILURE);
                 originVerified(origin, false, true);
                 break;
             case RelationshipCheckResult.NO_CONNECTION:
@@ -459,7 +500,7 @@ public class OriginVerifier {
 
         if (online != null) {
             long duration = SystemClock.uptimeMillis() - mVerificationStartTime;
-            BrowserServicesMetrics.recordVerificationTime(duration, online);
+            mMetricsListener.recordVerificationTime(duration, online);
         }
 
         cleanUp();
@@ -486,9 +527,9 @@ public class OriginVerifier {
             boolean verified = VerificationResultStore.isRelationshipSaved(
                     new Relationship(mPackageName, mSignatureFingerprint, origin, mRelation));
 
-            BrowserServicesMetrics.recordVerificationResult(verified
-                            ? BrowserServicesMetrics.VerificationResult.OFFLINE_SUCCESS
-                            : BrowserServicesMetrics.VerificationResult.OFFLINE_FAILURE);
+            mMetricsListener.recordVerificationResult(verified
+                            ? VerificationResult.OFFLINE_SUCCESS
+                            : VerificationResult.OFFLINE_FAILURE);
 
             originVerified(origin, verified, false);
         }

chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java

@@ -30,6 +30,7 @@ import org.chromium.base.ContextUtils;
 import org.chromium.base.metrics.RecordHistogram;
 import org.chromium.base.task.PostTask;
 import org.chromium.chrome.browser.IntentHandler;
+import org.chromium.chrome.browser.browserservices.BrowserServicesMetrics;
 import org.chromium.chrome.browser.browserservices.PostMessageHandler;
 import org.chromium.chrome.browser.browserservices.verification.OriginVerifier;
 import org.chromium.chrome.browser.browserservices.verification.OriginVerifier.OriginVerificationListener;
@@ -272,6 +273,9 @@ class ClientManager {
         }
     }
 
+    // TODO(crbug.com/1164866): Inject the Factory/Supplier.
+    private final OriginVerifier.Factory mOriginVerifierFactory = new OriginVerifier.Factory();
+
     private final Map<CustomTabsSessionToken, SessionParams> mSessionParams = new HashMap<>();
 
     private final SparseBooleanArray mUidHasCalledWarmup = new SparseBooleanArray();
@@ -477,8 +481,10 @@ class ClientManager {
             }
         };
 
-        params.originVerifier = new OriginVerifier(params.getPackageName(), relation,
-                /* webContents= */ null, /* externalAuthUtils= */ null);
+        params.originVerifier = mOriginVerifierFactory.create(params.getPackageName(), relation,
+                /* webContents= */ null, /* externalAuthUtils= */ null,
+                new BrowserServicesMetrics.OriginVerifierMetricsListener());
+
         PostTask.runOrPostTask(UiThreadTaskTraits.DEFAULT,
                 () -> { params.originVerifier.start(listener, origin); });
         if (relation == CustomTabsService.RELATION_HANDLE_ALL_URLS

chrome/android/javatests/src/org/chromium/chrome/browser/browserservices/verification/OriginVerifierTest.java

@@ -61,6 +61,8 @@ public class OriginVerifierTest {
             "19:75:B2:F1:71:77:BC:89:A5:DF:F3:1F:9E:64:A6:CA:E2:81:A5"
             + ":3D:C1:D1:D5:9B:1D:14:7F:E1:C8:2A:FA:00";
 
+    private final OriginVerifier.Factory mFactory = new OriginVerifier.Factory();
+
     private Origin mHttpsOrigin;
     private Origin mHttpOrigin;
     private TestExternalAuthUtils mExternalAuthUtils;
@@ -103,9 +105,9 @@ public class OriginVerifierTest {
         mHttpsOrigin = Origin.create("https://www.example.com");
         mHttpOrigin = Origin.create("http://www.android.com");
 
-        mHandleAllUrlsVerifier = new OriginVerifier(PACKAGE_NAME,
+        mHandleAllUrlsVerifier = mFactory.create(PACKAGE_NAME,
                 CustomTabsService.RELATION_HANDLE_ALL_URLS, new MockWebContents(), null);
-        mUseAsOriginVerifier = new OriginVerifier(PACKAGE_NAME,
+        mUseAsOriginVerifier = mFactory.create(PACKAGE_NAME,
                 CustomTabsService.RELATION_USE_AS_ORIGIN, /* webContents= */ null, null);
         mVerificationResultSemaphore = new Semaphore(0);
 
@@ -191,7 +193,7 @@ public class OriginVerifierTest {
     @Test
     @SmallTest
     public void testVerificationBypass() throws InterruptedException {
-        OriginVerifier verifier = new OriginVerifier(
+        OriginVerifier verifier = mFactory.create(
                 PACKAGE_NAME, CustomTabsService.RELATION_HANDLE_ALL_URLS, null, mExternalAuthUtils);
 
         PostTask.postTask(UiThreadTaskTraits.DEFAULT,

chrome/android/junit/src/org/chromium/chrome/browser/browserservices/ui/controller/trustedwebactivity/TwaVerifierTest.java

@@ -84,7 +84,7 @@ public class TwaVerifierTest {
         when(mIntentDataProvider.getTrustedWebActivityAdditionalOrigins())
                 .thenReturn(Collections.singletonList(ADDITIONAL_ORIGIN));
 
-        when(mOriginVerifierFactory.create(anyString(), anyInt(), any(), any()))
+        when(mOriginVerifierFactory.create(anyString(), anyInt(), any(), any(), any()))
                 .thenReturn(mOriginVerifier);
 
         when(mClientPackageNameProvider.get()).thenReturn("some.package.name");