Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2038423002:
Adding support for phone and email. (Closed)
|
Created:
4 years, 6 months ago by Byoungkwon Ko Modified:
4 years, 6 months ago CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, darin (slow to review), dglazkov+blink, haraken, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding support for phone and email.
This change adds support for two new PaymentOptions for payer’s email and
payer’s phone number[1].
[1] https://github.com/w3c/browser-payment-api/pull/174
BUG=617195
Committed: https://crrev.com/26dc6730a200dfcb8e199de41a9c594b0744a004
Cr-Commit-Position: refs/heads/master@{#400268}
Patch Set 1 #
Total comments: 2
Patch Set 2 : #Patch Set 3 : #
Total comments: 12
Patch Set 4 : #Patch Set 5 : #
Total comments: 11
Patch Set 6 : #
Total comments: 4
Patch Set 7 : #Patch Set 8 : #
Total comments: 3
Patch Set 9 : Adding support for phone and email. #Patch Set 10 : Adding support for phone and email. #
Total comments: 2
Patch Set 11 : Adding support for phone and email. #
Total comments: 1
Patch Set 12 : Adding support for phone and email. #
Total comments: 10
Patch Set 13 : Adding support for phone and email. #
Total comments: 1
Messages
Total messages: 82 (28 generated)
Description was changed from ========== [WIP] Adding support for phone and email. TODO: Should update description. BUG=617195 ========== to ========== [WIP] Adding support for phone and email. TODO: Should update description. BUG=617195 ==========
gogag2@gmail.com changed reviewers: + jinho.bang@samsung.com
https://codereview.chromium.org/2038423002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/2038423002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:443: m_payerEmail = response->payer_email; This should happen if requestPayerEmail is true. https://codereview.chromium.org/2038423002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:447: m_payerPhone = response->payer_phone; ditto
Description was changed from ========== [WIP] Adding support for phone and email. TODO: Should update description. BUG=617195 ========== to ========== Adding support for phone and email. This change adds support for two new PaymentOptions for payer’s email and payer’s phone number.[1] [1] https://github.com/w3c/browser-payment-api/pull/174 BUG=617195 ==========
Description was changed from ========== Adding support for phone and email. This change adds support for two new PaymentOptions for payer’s email and payer’s phone number.[1] [1] https://github.com/w3c/browser-payment-api/pull/174 BUG=617195 ========== to ========== Adding support for phone and email. This change adds support for two new PaymentOptions for payer’s email and payer’s phone number[1]. [1] https://github.com/w3c/browser-payment-api/pull/174 BUG=617195 ==========
Description was changed from ========== Adding support for phone and email. This change adds support for two new PaymentOptions for payer’s email and payer’s phone number[1]. [1] https://github.com/w3c/browser-payment-api/pull/174 BUG=617195 ========== to ========== Adding support for phone and email. This change adds support for two new PaymentOptions for payer’s email and payer’s phone number[1]. [1] https://github.com/w3c/browser-payment-api/pull/174 BUG=617195 ==========
jinho.bang@samsung.com changed reviewers: + rouslan@chromium.org
Rouslan@ PTAL. https://codereview.chromium.org/2038423002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/2038423002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:454: if (m_options.requestPayerEmail() && !response->payer_email.isNull()) { I think it should be "if (m_options.requestPayerEmail() && !response->payer_email.isEmpty())". https://codereview.chromium.org/2038423002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:454: if (m_options.requestPayerEmail() && !response->payer_email.isNull()) { I'm not sure but the promise may have to reject if requestPayerEmail == true and payer_email is null. Because we can't trust the response data because it comes from browser side via IPC. On the other hands, does user have to select email unconditionally? If not so, then requestPayerEmail is true but response->payer_email can be null. What do you think rouslan@? https://codereview.chromium.org/2038423002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:458: if (m_options.requestPayerPhone() && !response->payer_phone.isNull()) { ditto https://codereview.chromium.org/2038423002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp (right): https://codereview.chromium.org/2038423002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:547: } nit: blank line.
BK@, I think you will have to write some codes in android side as well. (follow-up CL)
The CQ bit was checked by rouslan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2038423002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
Great start! Keep it up! https://codereview.chromium.org/2038423002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentAddress.h (right): https://codereview.chromium.org/2038423002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentAddress.h:36: const String& phone() const { return m_phone; } Let's alter PaymentAddress fields in http://crbug.com/617190 instead. Do not change PaymentAddress in this patch. https://codereview.chromium.org/2038423002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/2038423002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:454: if (m_options.requestPayerEmail() && !response->payer_email.isNull()) { On 2016/06/06 14:19:32, zino wrote: > I'm not sure but the promise may have to reject if requestPayerEmail == true and > payer_email is null. > > Because we can't trust the response data because it comes from browser side via > IPC. > > On the other hands, does user have to select email unconditionally? > > If not so, then requestPayerEmail is true but response->payer_email can be null. > > What do you think rouslan@? The spec is not very clear on this topic. However, I think requestPayerEmail expresses a requirement. Therefore, if response->payer_email is null, then we should reject the show promise. https://codereview.chromium.org/2038423002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:455: m_payerEmail = response->payer_email; There's no need to save email an phone as member variables. If they are null or empty, then reject the promise. Otherwise, PaymentResponse will take them. if (m_options.requestPayerEmail() && (response->payer_email.isNull() || response->payer_email.isEmpty())) { m_showResolver->reject(DOMException::create(SyntaxError)); clearResolversAndCloseMojoConnection(); return; } if (m_options.requestPayerPhone() && (response->payer_phone.isNull() || response->payer_phone.isEmpty())) { m_showResolver->reject(DOMException::create(SyntaxError)); clearResolversAndCloseMojoConnection(); return; } https://codereview.chromium.org/2038423002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentRequest.h (right): https://codereview.chromium.org/2038423002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequest.h:91: String m_payerEmail; No need for m_payerEmail and m_payerPhone here. Remove these. https://codereview.chromium.org/2038423002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentResponse.cpp (right): https://codereview.chromium.org/2038423002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentResponse.cpp:19: , m_payerPhone(response->payer_phone) Check for email and phone in PaymentResponseTest.DataCopiedOver: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/paymen... https://codereview.chromium.org/2038423002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentResponse.idl (right): https://codereview.chromium.org/2038423002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentResponse.idl:12: readonly attribute DOMString? payerPhone; You should preserve the order of elements from the spec for readability. So payerEmail and payerPhone should go below shippingAddress. [ RuntimeEnabled=PaymentRequest, ] interface PaymentResponse { readonly attribute DOMString methodName; [CallWith=ScriptState, RaisesException] readonly attribute object details; readonly attribute PaymentAddress? shippingAddress; readonly attribute DOMString? payerEmail; readonly attribute DOMString? payerPhone; [CallWith=ScriptState] Promise<void> complete(boolean success); } https://codereview.chromium.org/2038423002/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/platform/modules/payments/payment_request.mojom (right): https://codereview.chromium.org/2038423002/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/modules/payments/payment_request.mojom:31: string phone; Remove https://codereview.chromium.org/2038423002/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/modules/payments/payment_request.mojom:47: string? payer_phone; Let's put payer_email and payer_phone at the bottom of this struct.
By the way, don't worry about the Java side. That's quite involved. I can add that code myself.
Dear zino and Rouslan. Thanks for your review and cheer up. I will do my best for chromium. Thank you. (Tomorrow, I will update for PaymentResponseTest code)
https://codereview.chromium.org/2038423002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/2038423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:454: if (m_options.requestPayerEmail() && (response->payer_email.isNull() || response->payer_email.isEmpty())) { nit: Should be "if (m_options.requestPayerEmail() && response->payer_email.isEmpty()) {". We don't need isNull check.. because isEmpty includes isNull check. The isEmpty means "isNull || length == 0". FYI: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/text/WTFSt... https://codereview.chromium.org/2038423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:460: if (m_options.requestPayerPhone() && (response->payer_phone.isNull() || response->payer_phone.isEmpty())) { ditto https://codereview.chromium.org/2038423002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp (right): https://codereview.chromium.org/2038423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:553: response->payer_email = String(); // null string https://codereview.chromium.org/2038423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:576: EXPECT_TRUE(pr->payerEmail().isEmpty()); You should use isNull() instead of isEmpty(). (If we don't allow empty string, then empty string will be converted to null) https://codereview.chromium.org/2038423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:598: TEST_F(PaymentRequestTest, RejectShowPromiseWithRequestPayerEmailFalseAndPayerEmailExistsInResponse) Do we really need this? If we want this, we should reject promise if requestPayerEmail is false and payer email is not empty. https://codereview.chromium.org/2038423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:679: EXPECT_TRUE(pr->payerPhone().isEmpty()); isNull https://codereview.chromium.org/2038423002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentResponse.cpp (right): https://codereview.chromium.org/2038423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentResponse.cpp:18: , m_payerEmail(response->payer_email) The payerEmail can be null. (It has DOMString? type instead of DOMString.) So, this should be m_payerEmail(response->payer_email.isEmpty() ? String() : response->payer_email). https://codereview.chromium.org/2038423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentResponse.cpp:19: , m_payerPhone(response->payer_phone) ditto
https://codereview.chromium.org/2038423002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp (right): https://codereview.chromium.org/2038423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:598: TEST_F(PaymentRequestTest, RejectShowPromiseWithRequestPayerEmailFalseAndPayerEmailExistsInResponse) On 2016/06/07 16:40:08, zino wrote: > Do we really need this? If we want this, we should reject promise if > requestPayerEmail is false and payer email is not empty. OK to remove. https://codereview.chromium.org/2038423002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentResponseTest.cpp (right): https://codereview.chromium.org/2038423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentResponseTest.cpp:64: input->payer_email = "abc@gamil.com"; gmail? https://codereview.chromium.org/2038423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentResponseTest.cpp:71: EXPECT_EQ("abc@gamil.com", output.payerEmail()); gmail?
Dear zino and Rouslan. Thanks for your review in detail !!! I am really happy to learn about chromium project. Thanks.
The CQ bit was checked by rouslan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2038423002/100001
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
dcheng@chromium.org changed reviewers: + dcheng@chromium.org, esprehn@chromium.org, yzshen@chromium.org
https://codereview.chromium.org/2038423002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentResponse.cpp (right): https://codereview.chromium.org/2038423002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentResponse.cpp:18: , m_payerEmail(response->payer_email.isEmpty() ? String() : response->payer_email) I'm kind of surprised there's no better way to do this. +yzshen, +esprehn, do we have best practices for Blink Onion Soup stuff like this?
https://codereview.chromium.org/2038423002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentResponse.cpp (right): https://codereview.chromium.org/2038423002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentResponse.cpp:18: , m_payerEmail(response->payer_email.isEmpty() ? String() : response->payer_email) On 2016/06/08 at 00:31:44, dcheng wrote: > I'm kind of surprised there's no better way to do this. +yzshen, +esprehn, do we have best practices for Blink Onion Soup stuff like this? Actually, I just talked to esprehn@ and he confirmed that in the Blink variant, this is already a WTF::String. Why do we need to do this check at all?
https://codereview.chromium.org/2038423002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentResponse.cpp (right): https://codereview.chromium.org/2038423002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentResponse.cpp:18: , m_payerEmail(response->payer_email.isEmpty() ? String() : response->payer_email) On 2016/06/08 00:37:53, dcheng wrote: > On 2016/06/08 at 00:31:44, dcheng wrote: > > I'm kind of surprised there's no better way to do this. +yzshen, +esprehn, do > we have best practices for Blink Onion Soup stuff like this? > > Actually, I just talked to esprehn@ and he confirmed that in the Blink variant, > this is already a WTF::String. Why do we need to do this check at all? I think we can be simple: , m_payerEmail(response->payer_email) , mPayerPhone(response->payer_phone)
Looks like webexposed test needs to know about the phone and email field in PaymentResponse.
https://codereview.chromium.org/2038423002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentResponse.cpp (right): https://codereview.chromium.org/2038423002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentResponse.cpp:18: , m_payerEmail(response->payer_email.isEmpty() ? String() : response->payer_email) On 2016/06/08 01:27:17, Rouslan (ツ) wrote: > On 2016/06/08 00:37:53, dcheng wrote: > > On 2016/06/08 at 00:31:44, dcheng wrote: > > > I'm kind of surprised there's no better way to do this. +yzshen, +esprehn, > do > > we have best practices for Blink Onion Soup stuff like this? > > > > Actually, I just talked to esprehn@ and he confirmed that in the Blink > variant, > > this is already a WTF::String. Why do we need to do this check at all? > > I think we can be simple: > > , m_payerEmail(response->payer_email) > , mPayerPhone(response->payer_phone) (sorry for late response.) Yes, that is already WTF::String.
Dear Rouslan I addresses your comment. Plz take a look. Thanks.
The CQ bit was checked by rouslan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2038423002/140001
lgtm
You're going to need to fix these tests before committing: PaymentRequestTest.ResolveShowPromiseWithRequestPayerPhoneFalseAndEmptyPayerPhoneInResponse PaymentRequestTest.ResolveShowPromiseWithRequestPayerEmailFalseAndEmptyPayerEmailInResponse You can run them like this: ninja -Cout/lin webkit_unit_tests && out/lin/webkit_unit_tests --gtest_filter=PaymentRequestTest.ResolveShowPromiseWithRequestPayer* I've initialized my out/lin directory with "gn args out/lin" command and the following parameters: is_component_build = true is_debug = true is_clang = true
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2038423002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp (right): https://codereview.chromium.org/2038423002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:628: EXPECT_TRUE(pr->payerEmail().isNull()); Hmm... I'm not sure if it's possible to pass this test. I thought we should always resolve null if the payer_email is empty string or null. Because we already rejected if requestPayerEmail is true and the payer_email is empty as well as null. So, I think we have two options. one thing is that we remove this test and trust the actual response from android platform. Another is adding a logic to convert as null if the payer_email is empty string. What do you think?
https://codereview.chromium.org/2038423002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp (right): https://codereview.chromium.org/2038423002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:628: EXPECT_TRUE(pr->payerEmail().isNull()); On 2016/06/09 17:56:08, zino wrote: > Hmm... > > I'm not sure if it's possible to pass this test. > I thought we should always resolve null if the payer_email is empty string or > null. > Because we already rejected if requestPayerEmail is true and the payer_email is > empty as well as null. > So, I think we have two options. one thing is that we remove this test and trust > the actual response from android platform. > Another is adding a logic to convert as null if the payer_email is empty string. > What do you think? Because options.setRequestPayerEmail(false) is called on lin 615, I think this expectation is OK: EXPECT_TRUE(pr->payerEmail().isNull())
Dear Rouslan As you commented, below 2 tests were failed. PaymentRequestTest.ResolveShowPromiseWithRequestPayerPhoneFalseAndEmptyPayerPhoneInResponse PaymentRequestTest.ResolveShowPromiseWithRequestPayerEmailFalseAndEmptyPayerEmailInResponse If i change "EXPECT_TRUE(pr->payerEmail().isNull())" to "EX"PECT_TRUE(pr->payerEmail().isEmpty())", I think it will be passed. But I don't know whether it is right or not. As zino told, if payer_email is empty string or null, pr->payerEmail should be null. Am I wrong? I don't know exactly, if you don't mind, plz tell me what i can do : ) Thanks.
https://codereview.chromium.org/2038423002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/2038423002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:482: } Let's follow the convention of the shipping address logic above. Add a clause like this here and below: else { if (!response->payer_email.isNull()) { m_showResolver->reject(DOMException::create(SyntaxError)); clearResolversAndCloseMojoConnection(); return; } } Then you test ResolveShowPromiseWithRequestPayerPhoneFalseAndEmptyPayerPhoneInResponse should be renamed into: RejectShowPromiseWithRequestPayerPhoneFalseAndNonNullPayerPhoneInResponse I changed "Resolve"->"Reject" and "Empty"->"NonNull". In your test, you should check that show promise gets rejected. This is because, if email was not requested, then a null email should be in response. Non-null email should reject the show() promise.
On 2016/06/10 17:46:51, Rouslan (ツ) wrote: > https://codereview.chromium.org/2038423002/diff/140001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): > > https://codereview.chromium.org/2038423002/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:482: } > Let's follow the convention of the shipping address logic above. Add a clause > like this here and below: > > else { > if (!response->payer_email.isNull()) { > m_showResolver->reject(DOMException::create(SyntaxError)); > clearResolversAndCloseMojoConnection(); > return; > } > } > > Then you test > ResolveShowPromiseWithRequestPayerPhoneFalseAndEmptyPayerPhoneInResponse should > be renamed into: > > RejectShowPromiseWithRequestPayerPhoneFalseAndNonNullPayerPhoneInResponse > > I changed "Resolve"->"Reject" and "Empty"->"NonNull". > > In your test, you should check that show promise gets rejected. This is because, > if email was not requested, then a null email should be in response. Non-null > email should reject the show() promise. Dear Rouslan. I uploaded new patch! Plz take a look : ) Thanks.
The CQ bit was checked by rouslan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2038423002/180001
Looks much better! Only a couple of minor comments. https://codereview.chromium.org/2038423002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentOptions.idl (right): https://codereview.chromium.org/2038423002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentOptions.idl:5: // https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#paymentop... https://w3c.github.io/browser-payment-api/#paymentoptions-dictionary https://codereview.chromium.org/2038423002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp (right): https://codereview.chromium.org/2038423002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:708: request->show(getScriptState()).then(PaymentResponseFunction::create(getScriptState(), &outValue), MockFunction::expectCall(getScriptState())); When you're rejecting a promise, PaymentResponseFunction will never be called. It's cleaner to use MockFunction::expectNoCall() instead. request->show(getScriptState()).then(MockFunction::expectNoCall(getScriptState()), MockFunction::expectCall(getScriptState())); Same for line 622.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
There seems some delay here due to BlinkOn. As I also need to build the UI for this, I will be basing my patch on this one. I will credit the original patch author in my patch description.
Dear Rouslan. I am so sorry to late. If you don't mine, could you review it again?? I will check it out asap!!! Sorry again.
The CQ bit was checked by gogag2@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2038423002/200001
rouslan@chromium.org changed reviewers: + palmer@chromium.org
lgtm. +palmer@, ptal payment_request.mojom.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by gogag2@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2038423002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
https://codereview.chromium.org/2038423002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp (right): https://codereview.chromium.org/2038423002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:25: class PaymentRequestTest : public PaymentRequestTestBase {}; Looks like you need to rebase, because PaymentRequestTestBase has been removed and all of these tests have been changed from TEST_F to TEST. It might be a messy rebase, sorry.
On 2016/06/16 00:52:22, Rouslan (ツ) wrote: > https://codereview.chromium.org/2038423002/diff/200001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp (right): > > https://codereview.chromium.org/2038423002/diff/200001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:25: class > PaymentRequestTest : public PaymentRequestTestBase {}; > Looks like you need to rebase, because PaymentRequestTestBase has been removed > and all of these tests have been changed from TEST_F to TEST. It might be a > messy rebase, sorry. OK~ I will try it now!!
The CQ bit was checked by gogag2@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2038423002/220001
Looks like a clean rebase. Well done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
Looks like you need to initialize response->total_amount to avoid crashes in unit tests. (Production code always initializes it.) https://codereview.chromium.org/2038423002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp (right): https://codereview.chromium.org/2038423002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:555: mojom::blink::PaymentResponsePtr response = mojom::blink::PaymentResponse::New(); response->total_amount = mojom::blink::CurrencyAmount::New(); https://codereview.chromium.org/2038423002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:577: mojom::blink::PaymentResponsePtr response = mojom::blink::PaymentResponse::New(); response->total_amount = mojom::blink::CurrencyAmount::New(); https://codereview.chromium.org/2038423002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:594: mojom::blink::PaymentResponsePtr response = mojom::blink::PaymentResponse::New(); response->total_amount = mojom::blink::CurrencyAmount::New(); https://codereview.chromium.org/2038423002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:611: mojom::blink::PaymentResponsePtr response = mojom::blink::PaymentResponse::New(); response->total_amount = mojom::blink::CurrencyAmount::New(); https://codereview.chromium.org/2038423002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:628: mojom::blink::PaymentResponsePtr response = mojom::blink::PaymentResponse::New(); response->total_amount = mojom::blink::CurrencyAmount::New(); https://codereview.chromium.org/2038423002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:650: mojom::blink::PaymentResponsePtr response = mojom::blink::PaymentResponse::New(); response->total_amount = mojom::blink::CurrencyAmount::New(); https://codereview.chromium.org/2038423002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:672: mojom::blink::PaymentResponsePtr response = mojom::blink::PaymentResponse::New(); response->total_amount = mojom::blink::CurrencyAmount::New(); https://codereview.chromium.org/2038423002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:689: mojom::blink::PaymentResponsePtr response = mojom::blink::PaymentResponse::New(); response->total_amount = mojom::blink::CurrencyAmount::New(); https://codereview.chromium.org/2038423002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:706: mojom::blink::PaymentResponsePtr response = mojom::blink::PaymentResponse::New(); response->total_amount = mojom::blink::CurrencyAmount::New(); https://codereview.chromium.org/2038423002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:723: mojom::blink::PaymentResponsePtr response = mojom::blink::PaymentResponse::New(); response->total_amount = mojom::blink::CurrencyAmount::New();
The CQ bit was checked by gogag2@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2038423002/240001
Dear Rouslan. I really thank you to explain in detail. I tried to find root cause for crash all day. But I didn't find it;;; sorry; I have a question. when I tried to test on local , it always passed. But when I tried "Dry run(commit bot)", sometimes it failed. Could I test it with same circumstances on "Dry run"??? When I try to test on local, I used to command "./out/Release/webkit_unit_tests --gtest_filter=*Payment*" Am I wrong???
On 2016/06/16 18:38:11, Byoungkwon Ko wrote: > Dear Rouslan. > > I really thank you to explain in detail. > I tried to find root cause for crash all day. > But I didn't find it;;; sorry; > > I have a question. > when I tried to test on local , it always passed. > But when I tried "Dry run(commit bot)", sometimes it failed. > Could I test it with same circumstances on "Dry run"??? > > When I try to test on local, I used to command "./out/Release/webkit_unit_tests > --gtest_filter=*Payment*" > Am I wrong??? Perhaps out/Debug/webkit_unit_tests would be better, as it will catch assertions.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM w/nit. https://codereview.chromium.org/2038423002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/2038423002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:483: if (m_options.requestPayerEmail() && response->payer_email.isEmpty()) { Nit: This code is unnecessarily repetitive. Maybe combine the various conditions into 1 large condition, and only write the body once?
On 2016/06/16 21:23:34, palmer wrote: > LGTM w/nit. > > https://codereview.chromium.org/2038423002/diff/240001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): > > https://codereview.chromium.org/2038423002/diff/240001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:483: if > (m_options.requestPayerEmail() && response->payer_email.isEmpty()) { > Nit: This code is unnecessarily repetitive. Maybe combine the various conditions > into 1 large condition, and only write the body once? If you don't mind, I will address this issue in an immediate follow up. This patch has been gestating for a while and I'd like to land it sooner rather than later to unblock some other people.
The CQ bit was checked by rouslan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org Link to the patchset: https://codereview.chromium.org/2038423002/#ps240001 (title: "Adding support for phone and email.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2038423002/240001
Message was sent while issue was closed.
Description was changed from ========== Adding support for phone and email. This change adds support for two new PaymentOptions for payer’s email and payer’s phone number[1]. [1] https://github.com/w3c/browser-payment-api/pull/174 BUG=617195 ========== to ========== Adding support for phone and email. This change adds support for two new PaymentOptions for payer’s email and payer’s phone number[1]. [1] https://github.com/w3c/browser-payment-api/pull/174 BUG=617195 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Adding support for phone and email. This change adds support for two new PaymentOptions for payer’s email and payer’s phone number[1]. [1] https://github.com/w3c/browser-payment-api/pull/174 BUG=617195 ========== to ========== Adding support for phone and email. This change adds support for two new PaymentOptions for payer’s email and payer’s phone number[1]. [1] https://github.com/w3c/browser-payment-api/pull/174 BUG=617195 Committed: https://crrev.com/26dc6730a200dfcb8e199de41a9c594b0744a004 Cr-Commit-Position: refs/heads/master@{#400268} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/26dc6730a200dfcb8e199de41a9c594b0744a004 Cr-Commit-Position: refs/heads/master@{#400268} |
