Allow distinguishing successful from cancelled edits in EditorBase.

Previously, a single callback was invoked, no matter whether the user cancelled or confirmed the dialog. This made it impossible to distinguish the successful edit of a previously complete item from a cancelled edit, because in both cases, the callback was invoked with the same argument.

This CL also fixes a bug in Autofill Assistant caused by this behavior: cancelling the edit of a complete item should not auto-select that item (see b/132077559).

Note: the Chrome PR does not have the same problem, because the 'edit' icon is only available for the currently selected item. Thus, it is impossible to edit a currently unselected item. There is a similar issue for incomplete items, which is out-of-scope for this CL (see bug for details).

Bug: b/132077559
Change-Id: I3ef79397a7be902b6e616c984e7770d6ce6fc9d1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1603059
Commit-Queue: Clemens Arbesser <arbesser@google.com>
Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org>
Reviewed-by: Mathias Carlen <mcarlen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#663024}

chrome/android/features/autofill_assistant/java/src/org/chromium/chrome/browser/autofill_assistant/payment/AssistantPaymentRequestContactDetailsSection.java

@@ -50,12 +50,14 @@ public class AssistantPaymentRequestContactDetailsSection
     }
 
     @Override
-    protected void createOrEditItem(@Nullable View oldFullView, @Nullable AutofillContact oldItem) {
+    protected void createOrEditItem(@Nullable AutofillContact oldItem) {
         if (mEditor != null) {
-            mIgnoreProfileChangeNotifications = true;
-            mEditor.edit(oldItem,
-                    editedOption -> onItemCreatedOrEdited(oldItem, oldFullView, editedOption));
-            mIgnoreProfileChangeNotifications = false;
+            mEditor.edit(oldItem, newItem -> {
+                assert (newItem != null && newItem.isComplete());
+                mIgnoreProfileChangeNotifications = true;
+                addOrUpdateItem(newItem, true);
+                mIgnoreProfileChangeNotifications = false;
+            }, cancel -> {});
         }
     }
 
@@ -144,7 +146,8 @@ public class AssistantPaymentRequestContactDetailsSection
     }
 
     @Override
-    protected void onItemAddedOrUpdated(AutofillContact contact) {
+    protected void addOrUpdateItem(AutofillContact contact, boolean select) {
+        super.addOrUpdateItem(contact, select);
         addAutocompleteInformationToEditor(contact);
     }
 

chrome/android/features/autofill_assistant/java/src/org/chromium/chrome/browser/autofill_assistant/payment/AssistantPaymentRequestPaymentMethodSection.java

@@ -26,6 +26,7 @@ import java.util.List;
 public class AssistantPaymentRequestPaymentMethodSection
         extends AssistantPaymentRequestSection<AutofillPaymentInstrument> {
     private CardEditor mEditor;
+    private boolean mIgnorePaymentMethodsChangeNotifications;
 
     AssistantPaymentRequestPaymentMethodSection(Context context, ViewGroup parent) {
         super(context, parent, R.layout.autofill_assistant_payment_method_summary,
@@ -42,13 +43,16 @@ public class AssistantPaymentRequestPaymentMethodSection
     }
 
     @Override
-    protected void createOrEditItem(
-            @Nullable View oldFullView, @Nullable AutofillPaymentInstrument oldItem) {
+    protected void createOrEditItem(@Nullable AutofillPaymentInstrument oldItem) {
         if (mEditor == null) {
             return;
         }
-        mEditor.edit(
-                oldItem, editedOption -> onItemCreatedOrEdited(oldItem, oldFullView, editedOption));
+        mEditor.edit(oldItem, newItem -> {
+            assert (newItem != null && newItem.isComplete());
+            mIgnorePaymentMethodsChangeNotifications = true;
+            addOrUpdateItem(newItem, true);
+            mIgnorePaymentMethodsChangeNotifications = false;
+        }, cancel -> {});
     }
 
     @Override
@@ -93,11 +97,6 @@ public class AssistantPaymentRequestPaymentMethodSection
         methodIncompleteView.setVisibility(method.isComplete() ? View.GONE : View.VISIBLE);
     }
 
-    @Override
-    protected void onItemAddedOrUpdated(AutofillPaymentInstrument method) {
-        // Nothing to do
-    }
-
     void onProfilesChanged(List<PersonalDataManager.AutofillProfile> profiles) {
         for (PersonalDataManager.AutofillProfile profile : profiles) {
             // TODO(crbug.com/806868): replace suggested billing addresses (remove if necessary).
@@ -110,6 +109,9 @@ public class AssistantPaymentRequestPaymentMethodSection
      * the new/changed set of payment methods, while keeping the selected item if possible.
      */
     void onAvailablePaymentMethodsChanged(List<AutofillPaymentInstrument> paymentMethods) {
+        if (mIgnorePaymentMethodsChangeNotifications) {
+            return;
+        }
         AutofillPaymentInstrument previouslySelectedMethod = mSelectedOption;
         int selectedMethodIndex = -1;
         for (int i = 0; i < paymentMethods.size(); i++) {

chrome/android/features/autofill_assistant/java/src/org/chromium/chrome/browser/autofill_assistant/payment/AssistantPaymentRequestSection.java

@@ -108,9 +108,9 @@ public abstract class AssistantPaymentRequestSection<T extends EditableOption> {
         titleAddButtonLabelView.setText(titleAddButton);
 
         mTitleAddButton = mSectionExpander.findViewById(R.id.section_title_add_button);
-        mTitleAddButton.setOnClickListener(unusedView -> createOrEditItem(null, null));
+        mTitleAddButton.setOnClickListener(unusedView -> createOrEditItem(null));
 
-        mItemsView.setOnAddButtonClickedListener(() -> createOrEditItem(null, null));
+        mItemsView.setOnAddButtonClickedListener(() -> createOrEditItem(null));
         parent.addView(mSectionExpander,
                 new ViewGroup.LayoutParams(
                         ViewGroup.LayoutParams.MATCH_PARENT, ViewGroup.LayoutParams.WRAP_CONTENT));
@@ -182,6 +182,12 @@ public abstract class AssistantPaymentRequestSection<T extends EditableOption> {
         return items;
     }
 
+    /**
+     * Adds a new item to the list, or updates an item in-place if it is already in the list.
+     *
+     * @param option The item to add or update.
+     * @param select Whether to select the new/updated item or not.
+     */
     void addOrUpdateItem(@Nullable T option, boolean select) {
         if (option == null) {
             return;
@@ -204,7 +210,6 @@ public abstract class AssistantPaymentRequestSection<T extends EditableOption> {
         } else {
             updateSummaryView(mSummaryView, item.mOption);
         }
-        onItemAddedOrUpdated(option);
 
         if (select) {
             mIgnoreItemSelectedNotifications = true;
@@ -219,7 +224,7 @@ public abstract class AssistantPaymentRequestSection<T extends EditableOption> {
         updatePaddings();
     }
 
-    void updatePaddings() {
+    private void updatePaddings() {
         if (isEmpty()) {
             // Section is empty, i.e., the title is the bottom-most widget.
             mSectionExpander.setTitlePadding(mTopPadding, mBottomPadding);
@@ -273,19 +278,18 @@ public abstract class AssistantPaymentRequestSection<T extends EditableOption> {
                 // radiobutton to not render properly.
                 mSectionExpander.post(() -> mSectionExpander.setExpanded(false));
             } else {
-                createOrEditItem(item.mFullView, item.mOption);
+                createOrEditItem(item.mOption);
             }
-        }, () -> createOrEditItem(item.mFullView, item.mOption));
+        }, () -> createOrEditItem(item.mOption));
         updateVisibility();
     }
 
     /**
      * Asks the subclass to edit an item or create a new one (if |oldItem| is null). Subclasses
-     * should call |onItemCreatedOrEdited| when they are done.
-     * @param oldFullView The view associated with |oldItem|.
+     * should call |addOrUpdateItem| when they are done.
      * @param oldItem The item to be edited (null if a new item should be created).
      */
-    protected abstract void createOrEditItem(@Nullable View oldFullView, @Nullable T oldItem);
+    protected abstract void createOrEditItem(@Nullable T oldItem);
 
     /**
      * Asks the subclass to update the contents of |fullView|, which was previously created by
@@ -298,13 +302,6 @@ public abstract class AssistantPaymentRequestSection<T extends EditableOption> {
      */
     protected abstract void updateSummaryView(View summaryView, T option);
 
-    /**
-     * An item was added to the list or updated in place. Subclasses may react to this event. A
-     * common use case is for subclasses to add the information of the new profile to the
-     * autocomplete fields of their editor.
-     */
-    protected abstract void onItemAddedOrUpdated(T option);
-
     /**
      * For convenience. Hides |view| if it is empty.
      */
@@ -312,27 +309,6 @@ public abstract class AssistantPaymentRequestSection<T extends EditableOption> {
         view.setVisibility(view.length() == 0 ? View.GONE : View.VISIBLE);
     }
 
-    /**
-     * An old item was edited or a new item was created.
-     *
-     * @param oldItem The item that was edited. null to indicate that a new item was created.
-     * @param oldFullView The view associated with |oldItem|. Null if |oldItem| is null.
-     * @param newItem The new or edited item. Cancelling an 'edit' flow will yield the old item.
-     */
-    void onItemCreatedOrEdited(
-            @Nullable T oldItem, @Nullable View oldFullView, @Nullable T newItem) {
-        if (newItem == null) {
-            // User cancelled 'add' flow.
-            return;
-        } else if (!newItem.isComplete()) {
-            // User cancelled 'edit' flow.
-            return;
-            // TODO(crbug.com/806868) add special case for cancelling edit of incomplete item
-        } else {
-            addOrUpdateItem(newItem, /*select = */ true);
-        }
-    }
-
     private boolean isEmpty() {
         return mItems.isEmpty();
     }

chrome/android/features/autofill_assistant/java/src/org/chromium/chrome/browser/autofill_assistant/payment/AssistantPaymentRequestShippingAddressSection.java

@@ -42,13 +42,15 @@ public class AssistantPaymentRequestShippingAddressSection
     }
 
     @Override
-    protected void createOrEditItem(@Nullable View oldFullView, @Nullable AutofillAddress oldItem) {
+    protected void createOrEditItem(@Nullable AutofillAddress oldItem) {
         if (mEditor == null) {
             return;
         }
         mIgnoreProfileChangeNotifications = true;
-        mEditor.edit(
-                oldItem, editedOption -> onItemCreatedOrEdited(oldItem, oldFullView, editedOption));
+        mEditor.edit(oldItem, newItem -> {
+            assert (newItem != null && newItem.isComplete());
+            addOrUpdateItem(newItem, true);
+        }, cancel -> {});
         mIgnoreProfileChangeNotifications = false;
     }
 
@@ -113,8 +115,13 @@ public class AssistantPaymentRequestShippingAddressSection
     }
 
     @Override
-    protected void onItemAddedOrUpdated(AutofillAddress address) {
+    protected void addOrUpdateItem(AutofillAddress address, boolean select) {
+        super.addOrUpdateItem(address, select);
+
         // Update autocomplete information in the editor.
+        if (mEditor == null) {
+            return;
+        }
         mEditor.addPhoneNumberIfValid(address.getProfile().getPhoneNumber());
     }
 }
\ No newline at end of file

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

@@ -138,6 +138,15 @@ public class AddressEditor
         return null;
     }
 
+    /**
+     * Allows calling |edit| with a single callback used for both 'done' and 'cancel'.
+     * @see #edit(AutofillAddress, Callback, Callback)
+     */
+    public void edit(
+            @Nullable final AutofillAddress toEdit, final Callback<AutofillAddress> callback) {
+        edit(toEdit, callback, callback);
+    }
+
     /**
      * Builds and shows an editor model with the following fields.
      *
@@ -150,9 +159,10 @@ public class AddressEditor
      * [ phone number field ] <----- phone is always present and required.
      */
     @Override
-    public void edit(
-            @Nullable final AutofillAddress toEdit, final Callback<AutofillAddress> callback) {
-        super.edit(toEdit, callback);
+    public void edit(@Nullable final AutofillAddress toEdit,
+            final Callback<AutofillAddress> doneCallback,
+            final Callback<AutofillAddress> cancelCallback) {
+        super.edit(toEdit, doneCallback, cancelCallback);
 
         if (mAutofillProfileBridge == null) mAutofillProfileBridge = new AutofillProfileBridge();
 
@@ -275,7 +285,7 @@ public class AddressEditor
             // ever called when Cancel has already occurred.
             mAdminAreasLoaded = true;
             PersonalDataManager.getInstance().cancelPendingGetSubKeys();
-            callback.onResult(toEdit);
+            cancelCallback.onResult(toEdit);
         });
 
         // If the user clicks [Done], save changes on disk, mark the address "complete," and send it
@@ -285,7 +295,7 @@ public class AddressEditor
             PersonalDataManager.getInstance().cancelPendingGetSubKeys();
             commitChanges(mProfile);
             address.completeAddress(mProfile);
-            callback.onResult(address);
+            doneCallback.onResult(address);
         });
 
         loadAdminAreasForCountry(mCountryField.getValue().toString());

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

@@ -336,6 +336,15 @@ public class CardEditor extends EditorBase<AutofillPaymentInstrument>
         }
     }
 
+    /**
+     * Allows calling |edit| with a single callback used for both 'done' and 'cancel'.
+     * @see #edit(AutofillPaymentInstrument, Callback, Callback)
+     */
+    public void edit(@Nullable final AutofillPaymentInstrument toEdit,
+            final Callback<AutofillPaymentInstrument> callback) {
+        edit(toEdit, callback, callback);
+    }
+
     /**
      * Builds and shows an editor model with the following fields for local cards.
      *
@@ -354,8 +363,9 @@ public class CardEditor extends EditorBase<AutofillPaymentInstrument>
      */
     @Override
     public void edit(@Nullable final AutofillPaymentInstrument toEdit,
-            final Callback<AutofillPaymentInstrument> callback) {
-        super.edit(toEdit, callback);
+            final Callback<AutofillPaymentInstrument> doneCallback,
+            final Callback<AutofillPaymentInstrument> cancelCallback) {
+        super.edit(toEdit, doneCallback, cancelCallback);
 
         // If |toEdit| is null, we're creating a new credit card.
         final boolean isNewCard = toEdit == null;
@@ -376,7 +386,7 @@ public class CardEditor extends EditorBase<AutofillPaymentInstrument>
             try {
                 calendar = mCalendar.get();
             } catch (InterruptedException | ExecutionException e) {
-                mHandler.post(() -> callback.onResult(null));
+                mHandler.post(() -> cancelCallback.onResult(toEdit));
                 return;
             }
             assert calendar != null;
@@ -399,7 +409,7 @@ public class CardEditor extends EditorBase<AutofillPaymentInstrument>
 
         // If the user clicks [Cancel], send |toEdit| card back to the caller (will return original
         // state, which could be null, a full card, or a partial card).
-        editor.setCancelCallback(() -> callback.onResult(toEdit));
+        editor.setCancelCallback(() -> cancelCallback.onResult(toEdit));
 
         // If the user clicks [Done], save changes on disk, mark the card "complete," and send it
         // back to the caller.
@@ -417,7 +427,7 @@ public class CardEditor extends EditorBase<AutofillPaymentInstrument>
             assert billingAddress != null;
 
             instrument.completeInstrument(card, methodName, billingAddress);
-            callback.onResult(instrument);
+            doneCallback.onResult(instrument);
         });
 
         mEditorDialog.show(editor);

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

@@ -154,10 +154,20 @@ public class ContactEditor extends EditorBase<AutofillContact> {
         mPayerErrors = errors;
     }
 
-    @Override
+    /**
+     * Allows calling |edit| with a single callback used for both 'done' and 'cancel'.
+     * @see #edit(AutofillContact, Callback, Callback)
+     */
     public void edit(
             @Nullable final AutofillContact toEdit, final Callback<AutofillContact> callback) {
-        super.edit(toEdit, callback);
+        edit(toEdit, callback, callback);
+    }
+
+    @Override
+    public void edit(@Nullable final AutofillContact toEdit,
+            final Callback<AutofillContact> doneCallback,
+            final Callback<AutofillContact> cancelCallback) {
+        super.edit(toEdit, doneCallback, cancelCallback);
 
         final AutofillContact contact = toEdit == null
                 ? new AutofillContact(mContext, new AutofillProfile(), null, null, null,
@@ -215,7 +225,7 @@ public class ContactEditor extends EditorBase<AutofillContact> {
 
         // If the user clicks [Cancel], send |toEdit| contact back to the caller, which was the
         // original state (could be null, a complete contact, a partial contact).
-        editor.setCancelCallback(() -> callback.onResult(toEdit));
+        editor.setCancelCallback(() -> cancelCallback.onResult(toEdit));
 
         editor.setDoneCallback(() -> {
             String name = null;
@@ -251,7 +261,7 @@ public class ContactEditor extends EditorBase<AutofillContact> {
 
             profile.setIsLocal(true);
             contact.completeContact(profile.getGUID(), name, phone, email);
-            callback.onResult(contact);
+            doneCallback.onResult(contact);
         });
 
         mEditorDialog.show(editor);

chrome/android/java/src/org/chromium/chrome/browser/widget/prefeditor/EditorBase.java

@@ -23,7 +23,7 @@ public abstract class EditorBase<T extends EditableOption> {
     /**
      * Sets the user interface to be used for editing contact information.
      *
-     * @param editorView The user interface to be used.
+     * @param editorDialog The user interface to be used.
      */
     public void setEditorDialog(EditorDialog editorDialog) {
         assert editorDialog != null;
@@ -36,11 +36,16 @@ public abstract class EditorBase<T extends EditableOption> {
      *
      * @param toEdit   The information to edit. Can be null if the user is adding new information
      *                 instead of editing an existing one.
-     * @param callback The callback to invoke with the complete and valid information. Can be
-     *                 invoked with null if the user clicked Cancel.
+     * @param doneCallback The callback to invoke when confirming the edit dialog, with the complete
+     *                     and valid information.
+     * @param cancelCallback The callback to invoke when cancelling the edit dialog. Can be called
+     *         with null (|toEdit| was null), incomplete information (|toEdit| was incomplete),
+     *         invalid information (|toEdit| was invalid), or even with complete and valid
+     *         information (|toEdit| was both complete and valid to begin with).
      */
-    protected void edit(@Nullable T toEdit, Callback<T> callback) {
-        assert callback != null;
+    protected void edit(@Nullable T toEdit, Callback<T> doneCallback, Callback<T> cancelCallback) {
+        assert doneCallback != null;
+        assert cancelCallback != null;
         assert mEditorDialog != null;
         assert mContext != null;
     }