Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2492243002:
[Payments] Allow for editing of local cards, addresses and contacts (add pencil icon) (Closed)
DescriptionAllow for editing of local cards, addresses and contacts (add pencil icon)
BUG=664582
Committed: https://crrev.com/e04f46a5ba13bd8cda53b8e35c38c5f12cf81302
Cr-Commit-Position: refs/heads/master@{#432207}
Patch Set 1 #
Total comments: 32
Patch Set 2 : address comments #Messages
Total messages: 52 (39 generated)
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== work version draft one BUG= ========== to ========== Allow for editing of local cards and addresses (add pencil icon) BUG=664582 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
Description was changed from ========== Allow for editing of local cards and addresses (add pencil icon) BUG=664582 ========== to ========== Allow for editing of local cards, addresses and contacts (add pencil icon) BUG=664582 ==========
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Patchset #1 (id:160001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
gogerald@chromium.org changed reviewers: + dfalcantara@chromium.org, rouslan@chromium.org
Hi rouslan@ and dfalcantara@, PTAL.
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi All, PTAL of patch 2 which does not allow edit of server addresses and contacts as discussed.
Patchset #2 (id:200001) has been deleted
Dry run: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
Design changed, please take a look of patch one. Will make changes to save server addresses and contacts in local copies separately.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Great stuff! Except for a few things here and there, you're almost done. Please wait for Dan's review of ui/* code. He's the expert there. Here's his queue: https://codereview.chromium.org/search?closed=3&reviewer=dfalcantara%40chromi... https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/re... File chrome/android/java/res/layout/payment_option_edit_icon.xml (right): https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/re... chrome/android/java/res/layout/payment_option_edit_icon.xml:17: android:background="@color/payments_section_separator"/> Please mark this importantForAccessibility="no", so the screen reader ignores it. https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/re... chrome/android/java/res/layout/payment_option_edit_icon.xml:24: android:contentDescription="@null" /> Please add a description for accessibility. https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:752: && option instanceof AutofillAddress) { You can "assert ption instanceof AutofillAddress" instead of checking it in the if-clause. It's always true, if you look at the init() method. https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:754: mPaymentInformationCallback = callback; Please log the journey metrics for PaymentRequestJourneyLogger.SECTION_SHIPPING_ADDRESS. https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:759: && option instanceof AutofillContact) { Assert instead of check. https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:760: editContact((AutofillContact) option); Log the journey metrics. https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:765: && option instanceof AutofillPaymentInstrument) { Assert instead of check. Non-autofill payment instrument should not have a pencil icon. https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:766: editCard((AutofillPaymentInstrument) option); Log the journey metrics. https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:770: assert false; // should never reach here. Nit: remove the comment. It's clear what's going on.
lgtm % comments https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/re... File chrome/android/java/res/layout/payment_option_edit_icon.xml (right): https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/re... chrome/android/java/res/layout/payment_option_edit_icon.xml:11: <View Indent 8 for attributes and 4 for children XML tags. Been seeing that more lately and it makes the layout easier to parse. https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentOption.java (right): https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentOption.java:146: /** @return True if this option is editable by user. */ "by the user" or "by users" https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (right): https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:715: * editable, clicking of it brings up correspond editor. clicking on corresponding editor https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:739: mOptionIcon = optionIconExists ? createOptionIcon(parent, rowIndex, editIconExists) Indentation rules are different for java and ternary operators; you don't indent to the ?. Do something like this: mOptionIcon = optionIconExists ? createOptionIcon(parent, rowIndex, editIconExists) : null; https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:873: // The label spans two columns if no option or edit icon, or spans three colums if columns. https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:969: boolean wasClicked = row.mButton == v || row.mLabel == v || row.mOptionIcon == v; Rename "wasClicked". It doesn't make sense as a variable name now that there are two things that can be clicked on. Maybe clickedAdd or something.
Patchset #2 (id:220001) has been deleted
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi rouslan@ PTAL of patch set 2 which addressed the comments https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/re... File chrome/android/java/res/layout/payment_option_edit_icon.xml (right): https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/re... chrome/android/java/res/layout/payment_option_edit_icon.xml:11: <View On 2016/11/15 01:19:43, dfalcantara (check my queue) wrote: > Indent 8 for attributes and 4 for children XML tags. Been seeing that more > lately and it makes the layout easier to parse. Done. Curious, how this indentation makes it easier to parse? https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/re... chrome/android/java/res/layout/payment_option_edit_icon.xml:17: android:background="@color/payments_section_separator"/> On 2016/11/14 21:51:41, rouslan wrote: > Please mark this importantForAccessibility="no", so the screen reader ignores > it. Done. https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/re... chrome/android/java/res/layout/payment_option_edit_icon.xml:24: android:contentDescription="@null" /> On 2016/11/14 21:51:41, rouslan wrote: > Please add a description for accessibility. Have set content description for the parent linearlayout. Move it in to this file for clearance since this xml is only been used as edit button. https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:752: && option instanceof AutofillAddress) { On 2016/11/14 21:51:42, rouslan wrote: > You can "assert ption instanceof AutofillAddress" instead of checking it in the > if-clause. It's always true, if you look at the init() method. Done. Yes, was thinking it might be slightly worth to avoid crash even unexpected things happen in the future. https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:754: mPaymentInformationCallback = callback; On 2016/11/14 21:51:41, rouslan wrote: > Please log the journey metrics for > PaymentRequestJourneyLogger.SECTION_SHIPPING_ADDRESS. Do you mean PaymentRequestJourneyLogger.incrementSelectionEdits? it was recorded in editAddress, same as contact and payment method. And we do not intend to change selection here. https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:759: && option instanceof AutofillContact) { On 2016/11/14 21:51:42, rouslan wrote: > Assert instead of check. Done. https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:760: editContact((AutofillContact) option); On 2016/11/14 21:51:41, rouslan wrote: > Log the journey metrics. ditto https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:765: && option instanceof AutofillPaymentInstrument) { On 2016/11/14 21:51:42, rouslan wrote: > Assert instead of check. Non-autofill payment instrument should not have a > pencil icon. Done. https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:766: editCard((AutofillPaymentInstrument) option); On 2016/11/14 21:51:42, rouslan wrote: > Log the journey metrics. ditto https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:770: assert false; // should never reach here. On 2016/11/14 21:51:41, rouslan wrote: > Nit: remove the comment. It's clear what's going on. Done. https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentOption.java (right): https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentOption.java:146: /** @return True if this option is editable by user. */ On 2016/11/15 01:19:43, dfalcantara (check my queue) wrote: > "by the user" or "by users" Done. https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (right): https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:715: * editable, clicking of it brings up correspond editor. On 2016/11/15 01:19:43, dfalcantara (check my queue) wrote: > clicking on > > corresponding editor Done. https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:739: mOptionIcon = optionIconExists ? createOptionIcon(parent, rowIndex, editIconExists) On 2016/11/15 01:19:43, dfalcantara (check my queue) wrote: > Indentation rules are different for java and ternary operators; you don't indent > to the ?. > > Do something like this: > > mOptionIcon = optionIconExists > ? createOptionIcon(parent, rowIndex, editIconExists) : null; Done. https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:873: // The label spans two columns if no option or edit icon, or spans three colums if On 2016/11/15 01:19:43, dfalcantara (check my queue) wrote: > columns. Done. https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:969: boolean wasClicked = row.mButton == v || row.mLabel == v || row.mOptionIcon == v; On 2016/11/15 01:19:43, dfalcantara (check my queue) wrote: > Rename "wasClicked". It doesn't make sense as a variable name now that there > are two things that can be clicked on. Maybe clickedAdd or something. Done.
lgtm https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:752: && option instanceof AutofillAddress) { On 2016/11/15 16:43:39, gogerald1 wrote: > On 2016/11/14 21:51:42, rouslan wrote: > > You can "assert ption instanceof AutofillAddress" instead of checking it in > the > > if-clause. It's always true, if you look at the init() method. > > Done. Yes, was thinking it might be slightly worth to avoid crash even > unexpected things happen in the future. The best way to avoid unexpected is to add tests ;-)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by gogerald@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2492243002/#ps240001 (title: "address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Allow for editing of local cards, addresses and contacts (add pencil icon) BUG=664582 ========== to ========== Allow for editing of local cards, addresses and contacts (add pencil icon) BUG=664582 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Allow for editing of local cards, addresses and contacts (add pencil icon) BUG=664582 ========== to ========== Allow for editing of local cards, addresses and contacts (add pencil icon) BUG=664582 Committed: https://crrev.com/e04f46a5ba13bd8cda53b8e35c38c5f12cf81302 Cr-Commit-Position: refs/heads/master@{#432207} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/e04f46a5ba13bd8cda53b8e35c38c5f12cf81302 Cr-Commit-Position: refs/heads/master@{#432207}
Message was sent while issue was closed.
https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/re... File chrome/android/java/res/layout/payment_option_edit_icon.xml (right): https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/re... chrome/android/java/res/layout/payment_option_edit_icon.xml:11: <View On 2016/11/15 16:43:39, gogerald1 wrote: > On 2016/11/15 01:19:43, dfalcantara (check my queue) wrote: > > Indent 8 for attributes and 4 for children XML tags. Been seeing that more > > lately and it makes the layout easier to parse. > > Done. Curious, how this indentation makes it easier to parse? You can tell where children actually start and what belongs to the previous line. You do the same thing in Java and C++.
Message was sent while issue was closed.
On 2016/11/15 18:07:14, dfalcantara (check my queue) wrote: > https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/re... > File chrome/android/java/res/layout/payment_option_edit_icon.xml (right): > > https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/re... > chrome/android/java/res/layout/payment_option_edit_icon.xml:11: <View > On 2016/11/15 16:43:39, gogerald1 wrote: > > On 2016/11/15 01:19:43, dfalcantara (check my queue) wrote: > > > Indent 8 for attributes and 4 for children XML tags. Been seeing that more > > > lately and it makes the layout easier to parse. > > > > Done. Curious, how this indentation makes it easier to parse? > > You can tell where children actually start and what belongs to the previous > line. You do the same thing in Java and C++. Hmm, OK, I may misunderstand your 'easier to parse' is for machine previously. It might just easier to parse for human since I don't see how it impacts machine.
Message was sent while issue was closed.
On 2016/11/15 19:09:08, gogerald1 wrote: > On 2016/11/15 18:07:14, dfalcantara (check my queue) wrote: > > > https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/re... > > File chrome/android/java/res/layout/payment_option_edit_icon.xml (right): > > > > > https://codereview.chromium.org/2492243002/diff/180001/chrome/android/java/re... > > chrome/android/java/res/layout/payment_option_edit_icon.xml:11: <View > > On 2016/11/15 16:43:39, gogerald1 wrote: > > > On 2016/11/15 01:19:43, dfalcantara (check my queue) wrote: > > > > Indent 8 for attributes and 4 for children XML tags. Been seeing that > more > > > > lately and it makes the layout easier to parse. > > > > > > Done. Curious, how this indentation makes it easier to parse? > > > > You can tell where children actually start and what belongs to the previous > > line. You do the same thing in Java and C++. > > Hmm, OK, I may misunderstand your 'easier to parse' is for machine previously. > It might just easier to parse for human since I don't see how it impacts > machine. Well, yeah. That's why style guides exist. Python's the only language I can think of code style actually affecting how the code is interpreted. |
