[Payments] Simplify JourneyLogger.recordTransactionAmount()

Behavioural changes:
* Before the CL, JourneyLogger.recordTransactionAmount() was triggered
  only when Event.SHOWN or Event.SKIPPED_SHOW is recorded. This was
  different from the desktop's behaviour(read crbug.com/1149936#c5 for
  more context).
* After the CL, the method is triggered whenever all of the following
  conditions are met:
  (1) show() has been called,
  (2) apps have been queried,
  (3) payment details has stabilized.
  Regardless of Event.SHOWN and Event.SKIPPED_SHOW, which are recorded
  when UI is shown or skipped. The new behaviour is consistent with the
  desktop one.

Changes:
* Moved the calling of recordTransactionAmount() into the hook
  onShowCalledAndAppsQueriedAndDetailsFinalized().

Bug: 1149936
Change-Id: I4fc72e2289ab95303165622e395f9d499ebf338d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2581218
Commit-Queue: Liquan (Max) Gu <maxlg@chromium.org>
Reviewed-by: Sahel Sharify <sahel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#835672}

chrome/android/java/src/org/chromium/chrome/browser/payments/ChromePaymentRequestService.java

@@ -309,11 +309,6 @@ public class ChromePaymentRequestService
             dimBackgroundIfNotBottomSheetPaymentHandler(selectedApp);
             mDidRecordShowEvent = true;
             mJourneyLogger.setEventOccurred(Event.SKIPPED_SHOW);
-            assert mSpec.getRawTotal() != null;
-            // The total amount in details should be finalized at this point. So it is safe to
-            // record the triggered transaction amount.
-            mJourneyLogger.recordTransactionAmount(mSpec.getRawTotal().amount.currency,
-                    mSpec.getRawTotal().amount.value, false /*completed*/);
             invokePaymentApp(null /* selectedShippingAddress */, null /* selectedShippingOption */,
                     selectedApp);
         } else {
@@ -346,8 +341,6 @@ public class ChromePaymentRequestService
     }
 
     private void onMinimalUiConfirmed(PaymentApp app) {
-        mJourneyLogger.recordTransactionAmount(mSpec.getRawTotal().amount.currency,
-                mSpec.getRawTotal().amount.value, false /*completed*/);
         app.disableShowingOwnUI();
         invokePaymentApp(
                 null /* selectedShippingAddress */, null /* selectedShippingOption */, app);
@@ -419,18 +412,6 @@ public class ChromePaymentRequestService
 
         mPaymentUiService.updateDetailsOnPaymentRequestUI(mSpec.getPaymentDetails());
 
-        // Triggered transaction amount gets recorded when both of the following conditions are met:
-        // 1- Either Event.Shown or Event.SKIPPED_SHOW bits are set showing that transaction is
-        // triggered (mDidRecordShowEvent == true). 2- The total amount in details won't change
-        // (mPaymentRequestService.isShowWaitingForUpdatedDetails() == false). Record the
-        // transaction amount only when the triggered condition is already met. Otherwise it will
-        // get recorded when triggered condition becomes true.
-        if (mDidRecordShowEvent) {
-            assert mSpec.getRawTotal() != null;
-            mJourneyLogger.recordTransactionAmount(mSpec.getRawTotal().amount.currency,
-                    mSpec.getRawTotal().amount.value, /*completed=*/false);
-        }
-
         if (isFinishedQueryingPaymentApps && !mHasSkippedAppSelector) {
             boolean providedInformationToPaymentRequestUI =
                     mPaymentUiService.enableAndUpdatePaymentRequestUIWithPaymentInfo();
@@ -461,15 +442,6 @@ public class ChromePaymentRequestService
         if (mDidRecordShowEvent) return;
         mDidRecordShowEvent = true;
         mJourneyLogger.setEventOccurred(Event.SHOWN);
-        // Record the triggered transaction amount only when the total amount in details is
-        // finalized (i.e. mPaymentRequestService.isShowWaitingForUpdatedDetails() == false).
-        // Otherwise it will get recorded when the updated details become available.
-        if (mPaymentRequestService != null
-                && !mPaymentRequestService.isShowWaitingForUpdatedDetails()) {
-            assert mSpec.getRawTotal() != null;
-            mJourneyLogger.recordTransactionAmount(mSpec.getRawTotal().amount.currency,
-                    mSpec.getRawTotal().amount.value, false /*completed*/);
-        }
     }
 
     // Implements BrowserPaymentRequest:

components/payments/content/android/java/src/org/chromium/components/payments/PaymentRequestService.java

@@ -810,8 +810,7 @@ public class PaymentRequestService
         }
 
         if (mIsShowWaitingForUpdatedDetails) return null;
-        String error = mBrowserPaymentRequest.onShowCalledAndAppsQueriedAndDetailsFinalized(
-                mIsUserGestureShow);
+        String error = onShowCalledAndAppsQueriedAndDetailsFinalized();
         if (error != null) {
             return new PaymentNotShownError(
                     NotShownReason.OTHER, error, PaymentErrorReason.NOT_SUPPORTED);
@@ -820,6 +819,16 @@ public class PaymentRequestService
         return null;
     }
 
+    // Returns the error if any.
+    @Nullable
+    private String onShowCalledAndAppsQueriedAndDetailsFinalized() {
+        assert mSpec.getRawTotal() != null;
+        mJourneyLogger.recordTransactionAmount(mSpec.getRawTotal().amount.currency,
+                mSpec.getRawTotal().amount.value, false /*completed*/);
+        return mBrowserPaymentRequest.onShowCalledAndAppsQueriedAndDetailsFinalized(
+                mIsUserGestureShow);
+    }
+
     private void onShowFailed(String error) {
         onShowFailed(NotShownReason.OTHER, error, PaymentErrorReason.USER_CANCEL);
     }
@@ -1187,8 +1196,7 @@ public class PaymentRequestService
         if (error != null) return error;
 
         if (!mIsFinishedQueryingPaymentApps) return null;
-        return mBrowserPaymentRequest.onShowCalledAndAppsQueriedAndDetailsFinalized(
-                mIsUserGestureShow);
+        return onShowCalledAndAppsQueriedAndDetailsFinalized();
     }
 
     /**

components/payments/content/android/junit/src/org/chromium/components/payments/PaymentRequestServiceBuilder.java

@@ -12,6 +12,7 @@ import org.chromium.components.payments.BrowserPaymentRequest.Factory;
 import org.chromium.components.payments.PaymentRequestService.Delegate;
 import org.chromium.content_public.browser.RenderFrameHost;
 import org.chromium.content_public.browser.WebContents;
+import org.chromium.payments.mojom.PaymentCurrencyAmount;
 import org.chromium.payments.mojom.PaymentDetails;
 import org.chromium.payments.mojom.PaymentItem;
 import org.chromium.payments.mojom.PaymentMethodData;
@@ -50,13 +51,14 @@ public class PaymentRequestServiceBuilder implements PaymentRequestService.Deleg
 
     /* package */ static PaymentRequestServiceBuilder defaultBuilder(Runnable onClosedListener,
             PaymentRequestClient client, PaymentAppService appService,
-            BrowserPaymentRequest browserPaymentRequest) {
+            BrowserPaymentRequest browserPaymentRequest, JourneyLogger journeyLogger) {
         return new PaymentRequestServiceBuilder(
-                onClosedListener, client, appService, browserPaymentRequest);
+                onClosedListener, client, appService, browserPaymentRequest, journeyLogger);
     }
 
     private PaymentRequestServiceBuilder(Runnable onClosedListener, PaymentRequestClient client,
-            PaymentAppService appService, BrowserPaymentRequest browserPaymentRequest) {
+            PaymentAppService appService, BrowserPaymentRequest browserPaymentRequest,
+            JourneyLogger journeyLogger) {
         mDelegate = this;
         mWebContents = Mockito.mock(WebContents.class);
         setTopLevelOrigin("https://top.level.origin");
@@ -64,7 +66,7 @@ public class PaymentRequestServiceBuilder implements PaymentRequestService.Deleg
         setFrameOrigin("https://frame.origin");
         Origin origin = Mockito.mock(Origin.class);
         Mockito.doReturn(origin).when(mRenderFrameHost).getLastCommittedOrigin();
-        mJourneyLogger = Mockito.mock(JourneyLogger.class);
+        mJourneyLogger = journeyLogger;
         mMethodData = new PaymentMethodData[1];
         mMethodData[0] = new PaymentMethodData();
         mMethodData[0].supportedMethod = "https://www.chromium.org";
@@ -74,7 +76,12 @@ public class PaymentRequestServiceBuilder implements PaymentRequestService.Deleg
         mOptions = new PaymentOptions();
         mOptions.requestShipping = true;
         mSpec = Mockito.mock(PaymentRequestSpec.class);
-        Mockito.doReturn(new PaymentItem()).when(mSpec).getRawTotal();
+        PaymentCurrencyAmount amount = new PaymentCurrencyAmount();
+        amount.currency = "CNY";
+        amount.value = "123";
+        PaymentItem total = new PaymentItem();
+        total.amount = amount;
+        Mockito.doReturn(total).when(mSpec).getRawTotal();
         Map<String, PaymentMethodData> methodDataMap = new HashMap<>();
         Mockito.doReturn(methodDataMap).when(mSpec).getMethodData();
         mBrowserPaymentRequestFactory = (paymentRequestService) -> browserPaymentRequest;

components/payments/content/android/junit/src/org/chromium/components/payments/PaymentRequestServiceTest.java

@@ -70,6 +70,7 @@ public class PaymentRequestServiceTest implements PaymentRequestClient {
     private boolean mWaitForUpdatedDetailsDefaultValue;
     private PaymentAppService mPaymentAppService;
     private PaymentAppFactoryDelegate mPaymentAppFactoryDelegate;
+    private JourneyLogger mJourneyLogger;
 
     /** The shadow of PaymentFeatureList. Not to use outside the test. */
     @Implements(PaymentFeatureList.class)
@@ -144,6 +145,8 @@ public class PaymentRequestServiceTest implements PaymentRequestClient {
         List<PaymentApp> apps = new ArrayList();
         apps.add(app);
         Mockito.doReturn(apps).when(mBrowserPaymentRequest).getPaymentApps();
+
+        mJourneyLogger = Mockito.mock(JourneyLogger.class);
     }
 
     @Before
@@ -251,7 +254,7 @@ public class PaymentRequestServiceTest implements PaymentRequestClient {
         return PaymentRequestServiceBuilder.defaultBuilder(
                 ()
                         -> mIsOnCloseListenerInvoked = true,
-                /*client=*/this, mPaymentAppService, mBrowserPaymentRequest);
+                /*client=*/this, mPaymentAppService, mBrowserPaymentRequest, mJourneyLogger);
     }
 
     private PaymentApp createDefaultPaymentApp() {
@@ -276,6 +279,11 @@ public class PaymentRequestServiceTest implements PaymentRequestClient {
                 .showOrSkipAppSelector(Mockito.anyBoolean(), Mockito.any(), Mockito.anyBoolean());
     }
 
+    private void verifyJourneyLoggerRecordedTransactionAmount() {
+        Mockito.verify(mJourneyLogger, Mockito.times(1))
+                .recordTransactionAmount(Mockito.eq("CNY"), Mockito.eq("123"), Mockito.eq(false));
+    }
+
     @Test
     @Feature({"Payments"})
     public void testNullFrameOriginFailsCreation() {
@@ -590,6 +598,8 @@ public class PaymentRequestServiceTest implements PaymentRequestClient {
         queryPaymentApps();
         Mockito.verify(mBrowserPaymentRequest, Mockito.times(1))
                 .onShowCalledAndAppsQueriedAndDetailsFinalized(Mockito.anyBoolean());
+
+        verifyJourneyLoggerRecordedTransactionAmount();
     }
 
     @Test
@@ -605,6 +615,8 @@ public class PaymentRequestServiceTest implements PaymentRequestClient {
         updateWith(service);
         Mockito.verify(mBrowserPaymentRequest, Mockito.times(1))
                 .onShowCalledAndAppsQueriedAndDetailsFinalized(Mockito.anyBoolean());
+
+        verifyJourneyLoggerRecordedTransactionAmount();
     }
 
     @Test
@@ -620,6 +632,8 @@ public class PaymentRequestServiceTest implements PaymentRequestClient {
         queryPaymentApps();
         Mockito.verify(mBrowserPaymentRequest, Mockito.times(1))
                 .onShowCalledAndAppsQueriedAndDetailsFinalized(Mockito.anyBoolean());
+
+        verifyJourneyLoggerRecordedTransactionAmount();
     }
 
     @Test

weblayer/browser/java/org/chromium/weblayer_private/payments/WebLayerPaymentRequestService.java

@@ -128,11 +128,6 @@ public class WebLayerPaymentRequestService implements BrowserPaymentRequest {
         PaymentApp selectedPaymentApp = mAvailableApps.get(0);
         if (mShouldSkipAppSelector) {
             mJourneyLogger.setEventOccurred(Event.SKIPPED_SHOW);
-            assert mSpec.getRawTotal() != null;
-            // The total amount in details should be finalized at this point. So it is safe to
-            // record the triggered transaction amount.
-            mJourneyLogger.recordTransactionAmount(mSpec.getRawTotal().amount.currency,
-                    mSpec.getRawTotal().amount.value, false /*completed*/);
             PaymentResponseHelperInterface paymentResponseHelper = new PaymentResponseHelper(
                     selectedPaymentApp.handlesShippingAddress(), mSpec.getPaymentOptions());
             mPaymentRequestService.invokePaymentApp(selectedPaymentApp, paymentResponseHelper);