[Autofill Assistant] Add payment instrument editor [chromium/src : main]
Sandro Maggi (Gerrit)
Attention is currently required from: Luca Hunkeler.
Patch set 1:Commit-Queue +1
1 comment:
Patchset:
PTAL.
To view, visit change 3480019. To unsubscribe, or for help writing mail filters, visit settings.
Sandro Maggi (Gerrit)
Attention is currently required from: Florian Gauger, Luca Hunkeler.
1 comment:
Patchset:
Adding fga@ as well, looks like he's about to move some of our code.
To view, visit change 3480019. To unsubscribe, or for help writing mail filters, visit settings.
Luca Hunkeler (Gerrit)
Attention is currently required from: Florian Gauger, Sandro Maggi.
Patch set 2:Code-Review +1
2 comments:
Patchset:
LGTM % nit
File components/autofill_assistant/browser/actions/collect_user_data_action.cc:
Patch Set #2, Line 1447:
add_payment_instrument_tokennit: this is more or a problem with the upstream cl, but I don't love proto fields starting with `add_`, it was confusing here for a second.
Wouldn't it make more sense to call this field `payment_instrument_token` anyway?
To view, visit change 3480019. To unsubscribe, or for help writing mail filters, visit settings.
Florian Gauger (Gerrit)
Attention is currently required from: Sandro Maggi.
2 comments:
File chrome/android/features/autofill_assistant/java/src/org/chromium/chrome/browser/autofill_assistant/user_data/AssistantCollectUserDataBinder.java:
if (model.get(AssistantCollectUserDataModel.USE_GMS_CORE_EDIT_DIALOGS)) {
byte[] addInstrumentActionToken =
model.get(AssistantCollectUserDataModel.ADD_PAYMENT_INSTRUMENT_ACTION_TOKEN);
view.mPaymentMethodSection.setEditor(addInstrumentActionToken == null
? null
: view.mEditorFactory.createGmsPaymentInstrumentEditor(view.mActivity,
view.mWindowAndroid,
model.get(AssistantCollectUserDataModel.ACCOUNT_EMAIL),
addInstrumentActionToken));
} else {
view.mPaymentMethodSection.setEditor(
view.mEditorFactory.createPaymentInstrumentEditor(webContents, view.mActivity,
model.get(AssistantCollectUserDataModel.SUPPORTED_BASIC_CARD_NETWORKS),
shouldStoreChanges));
}nit: move a single setEditor call to the end
File chrome/browser/android/autofill_assistant/ui_controller_android.cc:
Patch Set #2, Line 1313:
base::android::ToJavaByteArraynit: using
To view, visit change 3480019. To unsubscribe, or for help writing mail filters, visit settings.
Sandro Maggi (Gerrit)
Attention is currently required from: Florian Gauger, Luca Hunkeler.
2 comments:
File chrome/android/features/autofill_assistant/java/src/org/chromium/chrome/browser/autofill_assistant/user_data/AssistantCollectUserDataBinder.java:
if (model.get(AssistantCollectUserDataModel.USE_GMS_CORE_EDIT_DIALOGS)) {
byte[] addInstrumentActionToken =
model.get(AssistantCollectUserDataModel.ADD_PAYMENT_INSTRUMENT_ACTION_TOKEN);
view.mPaymentMethodSection.setEditor(addInstrumentActionToken == null
? null
: view.mEditorFactory.createGmsPaymentInstrumentEditor(view.mActivity,
view.mWindowAndroid,
model.get(AssistantCollectUserDataModel.ACCOUNT_EMAIL),
addInstrumentActionToken));
} else {
view.mPaymentMethodSection.setEditor(
view.mEditorFactory.createPaymentInstrumentEditor(webContents, view.mActivity,
model.get(AssistantCollectUserDataModel.SUPPORTED_BASIC_CARD_NETWORKS),
shouldStoreChanges));
}nit: move a single setEditor call to the end
I'm assuming you mean
```
AssistantPaymentInstrumentEditor editor;
if (...) { ... }
view.mPaymentMethodSection.setEditor(editor);
```
rather than
```
view.mPaymentMethodSEction.setEditor(useGmsEditDialogs ? ... : ...);
```
File components/autofill_assistant/browser/actions/collect_user_data_action.cc:
Patch Set #2, Line 1447:
add_payment_instrument_token
nit: this is more or a problem with the upstream cl, but I don't love proto fields starting with `ad […]
We had a similar discussion in the upstream as well: There will also be an instrument token for editing (which will be attached to the item, not quite sure yet how to handle this TBH).
So there'll be an `add_payment_instrument_token` and an `edit_payment_instrument_token`.
To view, visit change 3480019. To unsubscribe, or for help writing mail filters, visit settings.
Florian Gauger (Gerrit)
Attention is currently required from: Luca Hunkeler, Sandro Maggi.
1 comment:
File chrome/android/features/autofill_assistant/java/src/org/chromium/chrome/browser/autofill_assistant/user_data/AssistantCollectUserDataBinder.java:
if (model.get(AssistantCollectUserDataModel.USE_GMS_CORE_EDIT_DIALOGS)) {
byte[] addInstrumentActionToken =
model.get(AssistantCollectUserDataModel.ADD_PAYMENT_INSTRUMENT_ACTION_TOKEN);
view.mPaymentMethodSection.setEditor(addInstrumentActionToken == null
? null
: view.mEditorFactory.createGmsPaymentInstrumentEditor(view.mActivity,
view.mWindowAndroid,
model.get(AssistantCollectUserDataModel.ACCOUNT_EMAIL),
addInstrumentActionToken));
} else {
view.mPaymentMethodSection.setEditor(
view.mEditorFactory.createPaymentInstrumentEditor(webContents, view.mActivity,
model.get(AssistantCollectUserDataModel.SUPPORTED_BASIC_CARD_NETWORKS),
shouldStoreChanges));
}
I'm assuming you mean […]
Yeah, your judgement if this makes it clearer
To view, visit change 3480019. To unsubscribe, or for help writing mail filters, visit settings.
Luca Hunkeler (Gerrit)
Attention is currently required from: Sandro Maggi.
1 comment:
File components/autofill_assistant/browser/actions/collect_user_data_action.cc:
Patch Set #2, Line 1447:
add_payment_instrument_token
We had a similar discussion in the upstream as well: There will also be an instrument token for edit […]
I would very slightly prefer other prefixes such as `create_` or `make_`, but the current solution is also fine, I'll leave it to you
To view, visit change 3480019. To unsubscribe, or for help writing mail filters, visit settings.
Sandro Maggi (Gerrit)
Attention is currently required from: Florian Gauger.
3 comments:
File chrome/android/features/autofill_assistant/BUILD.gn:
Patch Set #3, Line 190:
"public/java/src/org/chromium/chrome/browser/autofill_assistant/AssistantPaymentInstrumentEditorGms.java",Waiting on crrev/c/3479937 to move this there.
File chrome/android/features/autofill_assistant/java/src/org/chromium/chrome/browser/autofill_assistant/user_data/AssistantCollectUserDataBinder.java:
if (model.get(AssistantCollectUserDataModel.USE_GMS_CORE_EDIT_DIALOGS)) {
byte[] addInstrumentActionToken =
model.get(AssistantCollectUserDataModel.ADD_PAYMENT_INSTRUMENT_ACTION_TOKEN);
view.mPaymentMethodSection.setEditor(addInstrumentActionToken == null
? null
: view.mEditorFactory.createGmsPaymentInstrumentEditor(view.mActivity,
view.mWindowAndroid,
model.get(AssistantCollectUserDataModel.ACCOUNT_EMAIL),
addInstrumentActionToken));
} else {
view.mPaymentMethodSection.setEditor(
view.mEditorFactory.createPaymentInstrumentEditor(webContents, view.mActivity,
model.get(AssistantCollectUserDataModel.SUPPORTED_BASIC_CARD_NETWORKS),
shouldStoreChanges));
}
Yeah, your judgement if this makes it clearer
No strong preference. Done.
File chrome/browser/android/autofill_assistant/ui_controller_android.cc:
Patch Set #2, Line 1313:
base::android::ToJavaByteArraynit: using
Done
To view, visit change 3480019. To unsubscribe, or for help writing mail filters, visit settings.
Sandro Maggi (Gerrit)
Attention is currently required from: Florian Gauger.
1 comment:
File chrome/android/features/autofill_assistant/BUILD.gn:
Patch Set #3, Line 190:
"public/java/src/org/chromium/chrome/browser/autofill_assistant/AssistantPaymentInstrumentEditorGms.java",Waiting on crrev/c/3479937 to move this there.
Done
To view, visit change 3480019. To unsubscribe, or for help writing mail filters, visit settings.
Florian Gauger (Gerrit)
Patchset:
lgtm, thanks!
To view, visit change 3480019. To unsubscribe, or for help writing mail filters, visit settings.
Sandro Maggi (Gerrit)
1 comment:
Patchset:
Thanks for the reviews :)
To view, visit change 3480019. To unsubscribe, or for help writing mail filters, visit settings.
Sandro Maggi (Gerrit)
Patch set 5:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
2 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/android/features/autofill_assistant/public/java/src/org/chromium/chrome/browser/autofill_assistant/AssistantPaymentInstrumentEditorGms.java
Insertions: 0, Deletions: 55.
The diff is too large to show. Please review the diff.
```
```
The name of the file: chrome/android/features/autofill_assistant/public/java/src/org/chromium/chrome/browser/autofill_assistant/AssistantEditorFactoryChrome.java
Insertions: 6, Deletions: 3.
The diff is too large to show. Please review the diff.
```
```
The name of the file: components/autofill_assistant/android/public/java/src/org/chromium/components/autofill_assistant/AssistantEditorFactory.java
Insertions: 37, Deletions: 0.
The diff is too large to show. Please review the diff.
```
```
The name of the file: chrome/android/features/autofill_assistant/public/java/src/org/chromium/chrome/browser/autofill_assistant/AssistantEditorFactory.java
Insertions: 0, Deletions: 37.
The diff is too large to show. Please review the diff.
```
```
The name of the file: components/autofill_assistant/android/public/java/src/org/chromium/components/autofill_assistant/AssistantPaymentInstrumentEditorGms.java
Insertions: 55, Deletions: 0.
The diff is too large to show. Please review the diff.
```
```
The name of the file: components/autofill_assistant/android/BUILD.gn
Insertions: 83, Deletions: 0.
The diff is too large to show. Please review the diff.
```
```
The name of the file: chrome/android/features/autofill_assistant/public/java/src/org/chromium/chrome/browser/autofill_assistant/AssistantContactEditorAccount.java
Insertions: 0, Deletions: 76.
The diff is too large to show. Please review the diff.
```
```
The name of the file: components/autofill_assistant/android/public/java/src/org/chromium/components/autofill_assistant/AssistantContactEditorAccount.java
Insertions: 76, Deletions: 0.
The diff is too large to show. Please review the diff.
```
```
The name of the file: chrome/browser/android/autofill_assistant/ui_controller_android.cc
Insertions: 2, Deletions: 1.
The diff is too large to show. Please review the diff.
```
```
The name of the file: chrome/android/features/autofill_assistant/java/src/org/chromium/chrome/browser/autofill_assistant/user_data/AssistantCollectUserDataBinder.java
Insertions: 16, Deletions: 10.
The diff is too large to show. Please review the diff.
```
```
The name of the file: chrome/android/features/autofill_assistant/BUILD.gn
Insertions: 0, Deletions: 30.
The diff is too large to show. Please review the diff.
```
[Autofill Assistant] Add payment instrument editor
This loops the GMS editor through for creating a new payment
instrument.
Bug: b:205087796
Change-Id: I215d4f10083ff653889c61e320ed857bbfa67d57
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3480019
Reviewed-by: Luca Hunkeler <hl...@google.com>
Commit-Queue: Sandro Maggi <sandr...@google.com>
Cr-Commit-Position: refs/heads/main@{#975046}
---
M chrome/android/features/autofill_assistant/java/src/org/chromium/chrome/browser/autofill_assistant/user_data/AssistantCollectUserDataBinder.java
M chrome/android/features/autofill_assistant/java/src/org/chromium/chrome/browser/autofill_assistant/user_data/AssistantCollectUserDataModel.java
M chrome/android/features/autofill_assistant/public/java/src/org/chromium/chrome/browser/autofill_assistant/AssistantEditorFactoryChrome.java
M chrome/browser/android/autofill_assistant/ui_controller_android.cc
M components/autofill_assistant/android/BUILD.gn
M components/autofill_assistant/android/public/java/src/org/chromium/components/autofill_assistant/AssistantContactEditorAccount.java
M components/autofill_assistant/android/public/java/src/org/chromium/components/autofill_assistant/AssistantEditorFactory.java
A components/autofill_assistant/android/public/java/src/org/chromium/components/autofill_assistant/AssistantPaymentInstrumentEditorGms.java
M components/autofill_assistant/browser/actions/collect_user_data_action.cc
M components/autofill_assistant/browser/user_data.h
10 files changed, 149 insertions(+), 20 deletions(-)