Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2866413002:
Update payment phone type for shipping address (Closed)
DescriptionUpdate payment phone type for shipping address
BUG=717711
Review-Url: https://codereview.chromium.org/2866413002
Cr-Commit-Position: refs/heads/master@{#470660}
Committed: https://chromium.googlesource.com/chromium/src/+/b7718e7edfb712e4f6a38b72f0b9a7a8928719fc
Patch Set 1 #
Total comments: 4
Patch Set 2 : Rebase + other CL test fix + CR Comments about comments :-) #
Messages
Total messages: 30 (17 generated)
mad@chromium.org changed reviewers: + sebsg@chromium.org
plz... Thanks! BYE MAD
The CQ bit was checked by mad@chromium.org
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
On 2017/05/10 01:04:46, commit-bot: I haz the power wrote: > No L-G-T-M from a valid reviewer yet. > CQ run can only be started once the patch has received an L-G-T-M from a full > committer. > Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a > full super star committer. > Committers are members of the group "project-chromium-committers". > Note that this has nothing to do with OWNERS files. Oups, I hit the CQ button when what I wanted was a CQ dry run.
The CQ bit was checked by mad@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: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Sweet! Small comments. https://codereview.chromium.org/2866413002/diff/1/chrome/browser/ui/views/pay... File chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc (right): https://codereview.chromium.org/2866413002/diff/1/chrome/browser/ui/views/pay... chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc:137: // current country. nit: Took me a couple of reads to parse this comment; what do you think of this instead? The phone can be empty when restored from a saved state, or it may be formatted based on the currently selected country. Does this convey the same meaning? https://codereview.chromium.org/2866413002/diff/1/chrome/browser/ui/views/pay... chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc:397: /*accept_empty_phone_number=*/true); Can you add a comment explaining why this one needs to accept empty phone numbers?
Thanks! How about this? BYE MAD... https://codereview.chromium.org/2866413002/diff/1/chrome/browser/ui/views/pay... File chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc (right): https://codereview.chromium.org/2866413002/diff/1/chrome/browser/ui/views/pay... chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc:137: // current country. On 2017/05/10 14:32:00, sebsg wrote: > nit: Took me a couple of reads to parse this comment; what do you think of this > instead? > > The phone can be empty when restored from a saved state, or it may be formatted > based on the currently selected country. > > Does this convey the same meaning? Done. https://codereview.chromium.org/2866413002/diff/1/chrome/browser/ui/views/pay... chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc:397: /*accept_empty_phone_number=*/true); On 2017/05/10 14:32:00, sebsg wrote: > Can you add a comment explaining why this one needs to accept empty phone > numbers? Done.
The CQ bit was checked by mad@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...
lgtm!
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 mad@chromium.org
mad@chromium.org changed reviewers: + mathp@chromium.org
OWNER review please... Thanks! BYE MAD
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 mathp@chromium.org
lgtm
The CQ bit was checked by mathp@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Merci! :-) Le mer. 10 mai 2017 à 14:35, commit-bot@chromium.org via codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> a écrit : > CQ is trying da patch. > > Follow status at: > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > https://codereview.chromium.org/2866413002/ > -- BYE MAD... -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1494441254363990,
"parent_rev": "5383621e52891f08341999f4e89a4def85b0b1cd", "commit_rev":
"b7718e7edfb712e4f6a38b72f0b9a7a8928719fc"}
Message was sent while issue was closed.
Description was changed from ========== Update payment phone type for shipping address BUG=717711 ========== to ========== Update payment phone type for shipping address BUG=717711 Review-Url: https://codereview.chromium.org/2866413002 Cr-Commit-Position: refs/heads/master@{#470660} Committed: https://chromium.googlesource.com/chromium/src/+/b7718e7edfb712e4f6a38b72f0b9... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/b7718e7edfb712e4f6a38b72f0b9... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
