[PRImpl] Move updateAppModifiedTotals and simplify mModifiers

Changes:
* Move updateAppModifiedTotals into PaymentUIsManager
* Since the method include mModifiers, refactor getModifiers() to make
it less error-prone. The refactoring includes renaming it
getUnmodifiableModifiers() to make it explicit that the modifiers is
unmodifiable (although the name is funny) and implicates that it's not
just returning mModifiers itself.
* Since mModifiers now can be null and be empty, which is redundant and
thus error-prone, this CL make it not nullable.

Bug: 1102522

Change-Id: I26229494eb39e3f8c4629f711607c7d199fe3b91
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2316321
Commit-Queue: Liquan (Max) Gu <maxlg@chromium.org>
Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#792279}

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

@@ -592,7 +592,7 @@ public class AndroidPaymentAppFinder implements ManifestVerifyCallback {
                     mFactoryDelegate.getParams().getTopLevelOrigin(),
                     mFactoryDelegate.getParams().getPaymentRequestOrigin(),
                     mFactoryDelegate.getParams().getCertificateChain(),
-                    filterModifiersForApp(mFactoryDelegate.getParams().getModifiers(),
+                    filterModifiersForApp(mFactoryDelegate.getParams().getUnmodifiableModifiers(),
                             app.getInstrumentMethodNames()),
                     this::onIsReadyToPayResponse);
         }

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

@@ -163,9 +163,9 @@ public class AutofillPaymentAppFactory implements PaymentAppFactoryInterface {
             }
 
             @Override
-            public Map<String, PaymentDetailsModifier> getModifiers() {
+            public Map<String, PaymentDetailsModifier> getUnmodifiableModifiers() {
                 // AutofillPaymentAppFactory.Creator doesn't need modifiers.
-                assert false : "getModifiers() should not be called";
+                assert false : "getUnmodifiableModifiers() should not be called";
                 return null;
             }
         };

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

@@ -133,7 +133,7 @@ public class PaymentRequestImpl
                    PaymentApp.InstrumentDetailsCallback,
                    PaymentResponseHelper.PaymentResponseRequesterDelegate, FocusChangedObserver,
                    NormalizedAddressRequestDelegate, PaymentDetailsConverter.MethodChecker,
-                   PaymentHandlerUiObserver, PaymentUIsManager.Delegate {
+                   PaymentHandlerUiObserver {
     /**
      * A delegate to ask questions about the system, that allows tests to inject behaviour without
      * having to modify the entire system. This partially mirrors a similar C++
@@ -338,7 +338,7 @@ public class PaymentRequestImpl
      * items. Used to display modified totals for each payment apps, modified total in order
      * summary, and additional line items in order summary.
      */
-    private Map<String, PaymentDetailsModifier> mModifiers;
+    private Map<String, PaymentDetailsModifier> mModifiers = new ArrayMap<>();
 
     private PaymentRequestSpec mSpec;
     private String mId;
@@ -467,7 +467,7 @@ public class PaymentRequestImpl
         mSkipUiForNonUrlPaymentMethodIdentifiers = mDelegate.skipUiForBasicCard();
 
         if (sObserverForTest != null) sObserverForTest.onPaymentRequestCreated(this);
-        mPaymentUIsManager = new PaymentUIsManager(/*delegate=*/this,
+        mPaymentUIsManager = new PaymentUIsManager(
                 /*params=*/this, addressEditor, cardEditor);
         mPaymentAppComparator = new PaymentAppComparator(/*params=*/this);
     }
@@ -1457,12 +1457,11 @@ public class PaymentRequestImpl
         }
 
         if (details.modifiers != null) {
-            if (details.modifiers.length == 0 && mModifiers != null) mModifiers.clear();
+            if (details.modifiers.length == 0) mModifiers.clear();
 
             for (int i = 0; i < details.modifiers.length; i++) {
                 PaymentDetailsModifier modifier = details.modifiers[i];
                 String method = modifier.methodData.supportedMethod;
-                if (mModifiers == null) mModifiers = new ArrayMap<>();
                 mModifiers.put(method, modifier);
             }
         }
@@ -1474,7 +1473,7 @@ public class PaymentRequestImpl
             mRawShippingOptions = Collections.unmodifiableList(new ArrayList<>());
         }
 
-        updateAppModifiedTotals();
+        mPaymentUIsManager.updateAppModifiedTotals();
 
         assert mRawTotal != null;
         assert mRawLineItems != null;
@@ -1482,26 +1481,6 @@ public class PaymentRequestImpl
         return true;
     }
 
-    // Implement PaymentUIsManager.Delegate:
-    @Override
-    public void updateAppModifiedTotals() {
-        if (!PaymentFeatureList.isEnabled(PaymentFeatureList.WEB_PAYMENTS_MODIFIERS)) return;
-        if (mModifiers == null) return;
-        if (mPaymentUIsManager.getPaymentMethodsSection() == null) return;
-
-        for (int i = 0; i < mPaymentUIsManager.getPaymentMethodsSection().getSize(); i++) {
-            PaymentApp app = (PaymentApp) mPaymentUIsManager.getPaymentMethodsSection().getItem(i);
-            PaymentDetailsModifier modifier = mPaymentUIsManager.getModifier(app);
-            app.setModifiedTotal(modifier == null || modifier.total == null
-                            ? null
-                            : mPaymentUIsManager.getOrCreateCurrencyFormatter(modifier.total.amount)
-                                      .format(modifier.total.amount.value));
-        }
-
-        mPaymentUIsManager.updateOrderSummary(
-                (PaymentApp) mPaymentUIsManager.getPaymentMethodsSection().getSelectedItem());
-    }
-
     /**
      * Called to retrieve the data to show in the initial PaymentRequest UI.
      */
@@ -1855,7 +1834,7 @@ public class PaymentRequestImpl
                 // to take (if another card was selected prior to the add flow, it will stay
                 // selected).
 
-                updateAppModifiedTotals();
+                mPaymentUIsManager.updateAppModifiedTotals();
                 mPaymentUIsManager.getPaymentRequestUI().updateSection(
                         PaymentRequestUI.DataType.PAYMENT_METHODS,
                         mPaymentUIsManager.getPaymentMethodsSection());
@@ -1898,7 +1877,7 @@ public class PaymentRequestImpl
             if (mMethodData.containsKey(paymentMethodName)) {
                 methodData.put(paymentMethodName, mMethodData.get(paymentMethodName));
             }
-            if (mModifiers != null && mModifiers.containsKey(paymentMethodName)) {
+            if (mModifiers.containsKey(paymentMethodName)) {
                 modifiers.put(paymentMethodName, mModifiers.get(paymentMethodName));
             }
             if (paymentMethodName.equals(MethodStrings.ANDROID_PAY)
@@ -2318,8 +2297,8 @@ public class PaymentRequestImpl
 
     // PaymentAppFactoryParams implementation.
     @Override
-    public Map<String, PaymentDetailsModifier> getModifiers() {
-        return mModifiers == null ? new HashMap<>() : Collections.unmodifiableMap(mModifiers);
+    public Map<String, PaymentDetailsModifier> getUnmodifiableModifiers() {
+        return Collections.unmodifiableMap(mModifiers);
     }
 
     // PaymentAppFactoryParams implementation.
@@ -2499,7 +2478,7 @@ public class PaymentRequestImpl
 
         mPendingApps.clear();
 
-        updateAppModifiedTotals();
+        mPaymentUIsManager.updateAppModifiedTotals();
 
         SettingsAutofillAndPaymentsObserver.getInstance().registerObserver(mPaymentUIsManager);
 

chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentUIsManager.java

@@ -45,7 +45,6 @@ public class PaymentUIsManager
         implements SettingsAutofillAndPaymentsObserver.Observer, PaymentRequestLifecycleObserver {
     private SectionInformation mUiShippingOptions;
     private final Map<String, CurrencyFormatter> mCurrencyFormatterMap;
-    private final Delegate mDelegate;
     private final AddressEditor mAddressEditor;
     private final CardEditor mCardEditor;
     private final PaymentUisShowStateReconciler mPaymentUisShowStateReconciler;
@@ -62,12 +61,6 @@ public class PaymentUIsManager
 
     private Boolean mCanUserAddCreditCard;
 
-    /** The delegate of this class. */
-    public interface Delegate {
-        /** Updates the modifiers for payment apps and order summary. */
-        void updateAppModifiedTotals();
-    }
-
     /**
      * This class is to coordinate the show state of a bottom sheet UI (either expandable payment
      * handler or minimal UI) and the Payment Request UI so that these visibility rules are
@@ -124,7 +117,6 @@ public class PaymentUIsManager
 
     /**
      * Create PaymentUIsManager.
-     * @param delegate The delegate of this class.
      * @param params The parameters of the payment request specified by the merchant.
      * @param addressEditor The AddressEditor of the PaymentRequest UI.
      * @param cardEditor The CardEditor of the PaymentRequest UI.
@@ -132,9 +124,8 @@ public class PaymentUIsManager
     // TODO(crbug.com/1107102): AddressEditor and CardEditor should be initialized in this
     // constructor instead of the caller of the constructor, once CardEditor's "ForTest" symbols
     // have been removed from the production code.
-    public PaymentUIsManager(Delegate delegate, PaymentRequestParams params,
-            AddressEditor addressEditor, CardEditor cardEditor) {
-        mDelegate = delegate;
+    public PaymentUIsManager(
+            PaymentRequestParams params, AddressEditor addressEditor, CardEditor cardEditor) {
         mParams = params;
         mAddressEditor = addressEditor;
         mCardEditor = cardEditor;
@@ -298,7 +289,7 @@ public class PaymentUIsManager
 
         mPaymentMethodsSection.addAndSelectOrUpdateItem(updatedAutofillCard);
 
-        mDelegate.updateAppModifiedTotals();
+        updateAppModifiedTotals();
 
         if (mPaymentRequestUI != null) {
             mPaymentRequestUI.updateSection(
@@ -314,7 +305,7 @@ public class PaymentUIsManager
 
         mPaymentMethodsSection.removeAndUnselectItem(guid);
 
-        mDelegate.updateAppModifiedTotals();
+        updateAppModifiedTotals();
 
         if (mPaymentRequestUI != null) {
             mPaymentRequestUI.updateSection(
@@ -364,15 +355,16 @@ public class PaymentUIsManager
 
     /** @return The first modifier that matches the given app, or null. */
     @Nullable
-    public PaymentDetailsModifier getModifier(@Nullable PaymentApp app) {
-        if (mParams.getModifiers() == null || app == null) return null;
+    private PaymentDetailsModifier getModifier(@Nullable PaymentApp app) {
+        Map<String, PaymentDetailsModifier> modifiers = mParams.getUnmodifiableModifiers();
+        if (modifiers.isEmpty() || app == null) return null;
         // Makes a copy to ensure it is modifiable.
         Set<String> methodNames = new HashSet<>(app.getInstrumentMethodNames());
-        methodNames.retainAll(mParams.getModifiers().keySet());
+        methodNames.retainAll(modifiers.keySet());
         if (methodNames.isEmpty()) return null;
 
         for (String methodName : methodNames) {
-            PaymentDetailsModifier modifier = mParams.getModifiers().get(methodName);
+            PaymentDetailsModifier modifier = modifiers.get(methodName);
             if (app.isValidForPaymentMethodData(methodName, modifier.methodData)) {
                 return modifier;
             }
@@ -381,6 +373,24 @@ public class PaymentUIsManager
         return null;
     }
 
+    /** Updates the modifiers for payment apps and order summary. */
+    public void updateAppModifiedTotals() {
+        if (!PaymentFeatureList.isEnabled(PaymentFeatureList.WEB_PAYMENTS_MODIFIERS)) return;
+        if (mParams.getMethodData().isEmpty()) return;
+        if (mPaymentMethodsSection == null) return;
+
+        for (int i = 0; i < mPaymentMethodsSection.getSize(); i++) {
+            PaymentApp app = (PaymentApp) mPaymentMethodsSection.getItem(i);
+            PaymentDetailsModifier modifier = getModifier(app);
+            app.setModifiedTotal(modifier == null || modifier.total == null
+                            ? null
+                            : getOrCreateCurrencyFormatter(modifier.total.amount)
+                                      .format(modifier.total.amount.value));
+        }
+
+        updateOrderSummary((PaymentApp) mPaymentMethodsSection.getSelectedItem());
+    }
+
     /**
      * Gets currency formatter for a given PaymentCurrencyAmount,
      * creates one if no existing instance is found.

chrome/android/javatests/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFinderTest.java

@@ -147,7 +147,7 @@ public class AndroidPaymentAppFinderTest
 
     // PaymentAppFactoryParams implementation.
     @Override
-    public Map<String, PaymentDetailsModifier> getModifiers() {
+    public Map<String, PaymentDetailsModifier> getUnmodifiableModifiers() {
         return Collections.unmodifiableMap(new HashMap<String, PaymentDetailsModifier>());
     }
 

chrome/android/native_java_unittests/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFinderUnitTest.java

@@ -83,7 +83,7 @@ public class AndroidPaymentAppFinderUnitTest {
         Mockito.when(params.getTopLevelOrigin()).thenReturn("https://chromium.org");
         Mockito.when(params.getPaymentRequestOrigin()).thenReturn("https://chromium.org");
         Mockito.when(params.getCertificateChain()).thenReturn(null);
-        Mockito.when(params.getModifiers())
+        Mockito.when(params.getUnmodifiableModifiers())
                 .thenReturn(new HashMap<String, PaymentDetailsModifier>());
         Mockito.when(params.getMayCrawl()).thenReturn(false);
         PaymentAppFactoryDelegate delegate = Mockito.mock(PaymentAppFactoryDelegate.class);

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

@@ -23,7 +23,7 @@ public interface PaymentRequestParams {
      * and additional line items. Used to display modified totals for each payment app, modified
      * total in order summary, and additional line items in order summary. Should not be null.
      */
-    Map<String, PaymentDetailsModifier> getModifiers();
+    Map<String, PaymentDetailsModifier> getUnmodifiableModifiers();
 
     /**
      * @return The unmodifiable mapping of payment method identifier to the method-specific data in