Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2285523002:
Add support for method selection in the Payment Request UI on iOS. (Closed)
|
Created:
4 years, 3 months ago by Justin Donnelly Modified:
4 years, 3 months ago CC:
chromium-reviews, rouslan+autofill_chromium.org, estade+watch_chromium.org, vabr+watchlistautofill_chromium.org, browser-components-watch_chromium.org, jdonnelly+autofillwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd support for method selection in the Payment Request UI on iOS.
This includes:
- Moving the mapping between autofill credit card types and types
defined in the spec to components/autofill.
- Adding the basic credit card response type from the spec.
BUG=602666
Committed: https://crrev.com/c3d5660f6d497ea5dffc885f198cefdc7aabad83
Committed: https://crrev.com/03f43b39690971a4a2732d9c48bd962e2c0b145f
Cr-Original-Commit-Position: refs/heads/master@{#415351}
Cr-Commit-Position: refs/heads/master@{#416329}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Clear dictionaries before parsing. #
Total comments: 8
Patch Set 3 : Respond to comments. #
Total comments: 2
Patch Set 4 : Add curly braces. #
Messages
Total messages: 36 (20 generated)
The CQ bit was checked by jdonnelly@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...
jdonnelly@chromium.org changed reviewers: + eugenebut@chromium.org, marq@chromium.org
marq: please provide general review as relates to associated downstream CL eugenebut: for OWNERS review
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2285523002/diff/1/ios/web/payments/payment_re... File ios/web/payments/payment_request.cc (right): https://codereview.chromium.org/2285523002/diff/1/ios/web/payments/payment_re... ios/web/payments/payment_request.cc:68: DCHECK(value); Do you want to clear the dictionary before filling it? https://codereview.chromium.org/2285523002/diff/1/ios/web/payments/payment_re... ios/web/payments/payment_request.cc:69: if (!this->country.empty()) Why do you want to use |this->| everywhere?
https://codereview.chromium.org/2285523002/diff/1/ios/web/payments/payment_re... File ios/web/payments/payment_request.cc (right): https://codereview.chromium.org/2285523002/diff/1/ios/web/payments/payment_re... ios/web/payments/payment_request.cc:68: DCHECK(value); On 2016/08/26 00:52:06, Eugene But wrote: > Do you want to clear the dictionary before filling it? Good idea, thanks. Done. https://codereview.chromium.org/2285523002/diff/1/ios/web/payments/payment_re... ios/web/payments/payment_request.cc:69: if (!this->country.empty()) On 2016/08/26 00:52:07, Eugene But wrote: > Why do you want to use |this->| everywhere? Removing them seems less clear to me. If I were calling methods (e.g. this->city()), it would make more sense to remove the "this->". But given that these are properties it can be hard to follow across this whole method that, for example, "country" is a member variable and not a local variable. Consider: value->SetString(kAddressCity, this->city); vs. value->SetString(kAddressCity, city); The former lets you know exactly what you're looking at without additional context.
jdonnelly@chromium.org changed reviewers: + estade@chromium.org
+estade for c/b/autofill and components/autofill
https://codereview.chromium.org/2285523002/diff/20001/components/autofill/cor... File components/autofill/core/browser/autofill_data_util.cc (right): https://codereview.chromium.org/2285523002/diff/20001/components/autofill/cor... components/autofill/core/browser/autofill_data_util.cc:391: return kPaymentRequestData[0]; seems fairly fragile that you're depending on the generic one to be first. I'd consider separating out the generic case rather than putting it in an array with the rest, and making L391: return kGenericPaymentRequestData; https://codereview.chromium.org/2285523002/diff/20001/components/autofill/cor... File components/autofill/core/browser/autofill_data_util.h (right): https://codereview.chromium.org/2285523002/diff/20001/components/autofill/cor... components/autofill/core/browser/autofill_data_util.h:28: } kPaymentRequestData[]{ seems like this should be in the .cc file rather than .h https://codereview.chromium.org/2285523002/diff/20001/components/autofill/cor... components/autofill/core/browser/autofill_data_util.h:30: is this blank line intentional? seems weird https://codereview.chromium.org/2285523002/diff/20001/ios/web/payments/paymen... File ios/web/payments/payment_request.cc (right): https://codereview.chromium.org/2285523002/diff/20001/ios/web/payments/paymen... ios/web/payments/payment_request.cc:292: void BasicCardResponse::ToDictionaryValue(base::DictionaryValue* value) const { why does this use an outparam instead of return value?
The CQ bit was checked by jdonnelly@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 #3 (id:40001) has been deleted
The CQ bit was checked by jdonnelly@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/2285523002/diff/20001/components/autofill/cor... File components/autofill/core/browser/autofill_data_util.cc (right): https://codereview.chromium.org/2285523002/diff/20001/components/autofill/cor... components/autofill/core/browser/autofill_data_util.cc:391: return kPaymentRequestData[0]; On 2016/08/29 20:34:05, Evan Stade wrote: > seems fairly fragile that you're depending on the generic one to be first. I'd > consider separating out the generic case rather than putting it in an array with > the rest, and making L391: return kGenericPaymentRequestData; Done. https://codereview.chromium.org/2285523002/diff/20001/components/autofill/cor... File components/autofill/core/browser/autofill_data_util.h (right): https://codereview.chromium.org/2285523002/diff/20001/components/autofill/cor... components/autofill/core/browser/autofill_data_util.h:28: } kPaymentRequestData[]{ On 2016/08/29 20:34:06, Evan Stade wrote: > seems like this should be in the .cc file rather than .h Done. https://codereview.chromium.org/2285523002/diff/20001/components/autofill/cor... components/autofill/core/browser/autofill_data_util.h:30: On 2016/08/29 20:34:06, Evan Stade wrote: > is this blank line intentional? seems weird Removed (it made sense to me that it separated the special generic value, but I removed that value). https://codereview.chromium.org/2285523002/diff/20001/ios/web/payments/paymen... File ios/web/payments/payment_request.cc (right): https://codereview.chromium.org/2285523002/diff/20001/ios/web/payments/paymen... ios/web/payments/payment_request.cc:292: void BasicCardResponse::ToDictionaryValue(base::DictionaryValue* value) const { On 2016/08/29 20:34:06, Evan Stade wrote: > why does this use an outparam instead of return value? No good reason, just habit of using out params. Changed. Note that downstream code calls PaymentResponse's out param method so I'm leaving in a version of that until the associated downstream CL lands then I'll remove it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2285523002/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_data_util.cc (right): https://codereview.chromium.org/2285523002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_data_util.cc:413: kPaymentRequestData[i].basic_card_payment_type) nit: curlies (side note: this file isn't consistent but seems to err on the side of curlies)
The CQ bit was checked by jdonnelly@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eugenebut@chromium.org, estade@chromium.org Link to the patchset: https://codereview.chromium.org/2285523002/#ps80001 (title: "Add curly braces.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2285523002/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_data_util.cc (right): https://codereview.chromium.org/2285523002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_data_util.cc:413: kPaymentRequestData[i].basic_card_payment_type) On 2016/08/30 16:06:18, Evan Stade wrote: > nit: curlies > > (side note: this file isn't consistent but seems to err on the side of curlies) Done.
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Add support for method selection in the Payment Request UI on iOS. This includes: - Moving the mapping between autofill credit card types and types defined in the spec to components/autofill. - Adding the basic credit card response type from the spec. BUG=602666 ========== to ========== Add support for method selection in the Payment Request UI on iOS. This includes: - Moving the mapping between autofill credit card types and types defined in the spec to components/autofill. - Adding the basic credit card response type from the spec. BUG=602666 Committed: https://crrev.com/c3d5660f6d497ea5dffc885f198cefdc7aabad83 Cr-Commit-Position: refs/heads/master@{#415351} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/c3d5660f6d497ea5dffc885f198cefdc7aabad83 Cr-Commit-Position: refs/heads/master@{#415351}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:80001) has been created in https://codereview.chromium.org/2289263003/ by mdjones@chromium.org. The reason for reverting is: Breaking testMultilineStreetAddress on KitKat Tablet Tester.
Message was sent while issue was closed.
Description was changed from ========== Add support for method selection in the Payment Request UI on iOS. This includes: - Moving the mapping between autofill credit card types and types defined in the spec to components/autofill. - Adding the basic credit card response type from the spec. BUG=602666 Committed: https://crrev.com/c3d5660f6d497ea5dffc885f198cefdc7aabad83 Cr-Commit-Position: refs/heads/master@{#415351} ========== to ========== Add support for method selection in the Payment Request UI on iOS. This includes: - Moving the mapping between autofill credit card types and types defined in the spec to components/autofill. - Adding the basic credit card response type from the spec. BUG=602666 Committed: https://crrev.com/c3d5660f6d497ea5dffc885f198cefdc7aabad83 Cr-Commit-Position: refs/heads/master@{#415351} ==========
As discussed at http://crrev.com/2287513002, I don't believe this CL was the cause of the failure that led to it's reversion. We suspect that the test in question was flakey. Further, we believe that the flakiness may be resolved by http://crrev.com/2296943002, which landed today. I'm going to try landing this change again.
The CQ bit was checked by jdonnelly@chromium.org
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 ========== Add support for method selection in the Payment Request UI on iOS. This includes: - Moving the mapping between autofill credit card types and types defined in the spec to components/autofill. - Adding the basic credit card response type from the spec. BUG=602666 Committed: https://crrev.com/c3d5660f6d497ea5dffc885f198cefdc7aabad83 Cr-Commit-Position: refs/heads/master@{#415351} ========== to ========== Add support for method selection in the Payment Request UI on iOS. This includes: - Moving the mapping between autofill credit card types and types defined in the spec to components/autofill. - Adding the basic credit card response type from the spec. BUG=602666 Committed: https://crrev.com/c3d5660f6d497ea5dffc885f198cefdc7aabad83 Cr-Commit-Position: refs/heads/master@{#415351} ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Add support for method selection in the Payment Request UI on iOS. This includes: - Moving the mapping between autofill credit card types and types defined in the spec to components/autofill. - Adding the basic credit card response type from the spec. BUG=602666 Committed: https://crrev.com/c3d5660f6d497ea5dffc885f198cefdc7aabad83 Cr-Commit-Position: refs/heads/master@{#415351} ========== to ========== Add support for method selection in the Payment Request UI on iOS. This includes: - Moving the mapping between autofill credit card types and types defined in the spec to components/autofill. - Adding the basic credit card response type from the spec. BUG=602666 Committed: https://crrev.com/c3d5660f6d497ea5dffc885f198cefdc7aabad83 Committed: https://crrev.com/03f43b39690971a4a2732d9c48bd962e2c0b145f Cr-Original-Commit-Position: refs/heads/master@{#415351} Cr-Commit-Position: refs/heads/master@{#416329} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/03f43b39690971a4a2732d9c48bd962e2c0b145f Cr-Commit-Position: refs/heads/master@{#416329} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
