Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2875103003:
DCHECK -> LOG_IF in payment request shipping address (Closed)
DescriptionDCHECK -> LOG_IF in payment request shipping address
BUG=721502
Review-Url: https://codereview.chromium.org/2875103003
Cr-Commit-Position: refs/heads/master@{#471091}
Committed: https://chromium.googlesource.com/chromium/src/+/b69e051f0d66a92428423df72843d0d53ccbd6cd
Patch Set 1 #
Total comments: 3
Messages
Total messages: 14 (7 generated)
mad@chromium.org changed reviewers: + anthonyvd@chromium.org
plz... Thanks! BYE MAD
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...
https://codereview.chromium.org/2875103003/diff/1/chrome/browser/ui/views/pay... File chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc (right): https://codereview.chromium.org/2875103003/diff/1/chrome/browser/ui/views/pay... chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:337: LOG_IF(ERROR, success || ignore_errors) Are those going to be useful (especially after we disable the "Done" button until all fields are valid)?
Thanks... Answer below. BYE MAD! https://codereview.chromium.org/2875103003/diff/1/chrome/browser/ui/views/pay... File chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc (right): https://codereview.chromium.org/2875103003/diff/1/chrome/browser/ui/views/pay... chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:337: LOG_IF(ERROR, success || ignore_errors) On 2017/05/11 20:06:29, anthonyvd wrote: > Are those going to be useful (especially after we disable the "Done" button > until all fields are valid)? Should still be useful in browser tests. Doesn't the Done button enable state depends on the return value of this function? So tests where the done button is unexpectadly disabled could be easier to debug with this. No?
lgtm https://codereview.chromium.org/2875103003/diff/1/chrome/browser/ui/views/pay... File chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc (right): https://codereview.chromium.org/2875103003/diff/1/chrome/browser/ui/views/pay... chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:337: LOG_IF(ERROR, success || ignore_errors) On 2017/05/11 at 20:19:53, MAD wrote: > On 2017/05/11 20:06:29, anthonyvd wrote: > > Are those going to be useful (especially after we disable the "Done" button > > until all fields are valid)? > > Should still be useful in browser tests. Doesn't the Done button enable state depends on the return value of this function? So tests where the done button is unexpectadly disabled could be easier to debug with this. No? I don't think so. This is called when the Done button is clicked (I think it's currently always enabled). In any case, that's a good point.
On 2017/05/11 20:22:00, anthonyvd wrote: > lgtm > > https://codereview.chromium.org/2875103003/diff/1/chrome/browser/ui/views/pay... > File chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc > (right): > > https://codereview.chromium.org/2875103003/diff/1/chrome/browser/ui/views/pay... > chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:337: > LOG_IF(ERROR, success || ignore_errors) > On 2017/05/11 at 20:19:53, MAD wrote: > > On 2017/05/11 20:06:29, anthonyvd wrote: > > > Are those going to be useful (especially after we disable the "Done" button > > > until all fields are valid)? > > > > Should still be useful in browser tests. Doesn't the Done button enable state > depends on the return value of this function? So tests where the done button is > unexpectadly disabled could be easier to debug with this. No? > > I don't think so. This is called when the Done button is clicked (I think it's > currently always enabled). In any case, that's a good point. Ho, yes of course, we don't save until clicked... :-) Thanks...
The CQ bit was unchecked by mad@chromium.org
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...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1494535672747780, "parent_rev":
"c6aa511064b40136b0bf83d926690898b285f114", "commit_rev":
"b69e051f0d66a92428423df72843d0d53ccbd6cd"}
Message was sent while issue was closed.
Description was changed from ========== DCHECK -> LOG_IF in payment request shipping address BUG=721502 ========== to ========== DCHECK -> LOG_IF in payment request shipping address BUG=721502 Review-Url: https://codereview.chromium.org/2875103003 Cr-Commit-Position: refs/heads/master@{#471091} Committed: https://chromium.googlesource.com/chromium/src/+/b69e051f0d66a92428423df72843... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/b69e051f0d66a92428423df72843... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
