Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2145553002:
Parameterize OnError method. (Closed)
|
Created:
4 years, 5 months ago by pals Modified:
4 years, 5 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, Aaron Boodman, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, haraken, dglazkov+blink, blink-reviews, darin (slow to review), ben+mojo_chromium.org, blink-reviews-api_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionParameterize OnError method.
This change is required to support following part of the
spec. "Let acceptedMethods be supportedMethods with all
identifiers removed that the user agent does not accept.
If the length of acceptedMethods is zero, then reject
acceptPromise with a NotSupportedError."
BUG=627705
Committed: https://crrev.com/e542d0af7a4908ec043b86d858bb9206a1c9c7f9
Cr-Commit-Position: refs/heads/master@{#406227}
Patch Set 1 #Patch Set 2 : Parameterize OnError method. #
Total comments: 26
Patch Set 3 : Parameterize OnError method. #
Total comments: 20
Patch Set 4 : Fixed review comments #
Total comments: 4
Patch Set 5 : Fixed comments #Patch Set 6 : Test1 #
Total comments: 12
Patch Set 7 : fixing integration test failures #Patch Set 8 : fixing integration test failures #Messages
Total messages: 53 (31 generated)
Description was changed from ========== Parameterize OnError method. BUG= ========== to ========== Parameterize OnError method. BUG= ==========
sanjoy.pal@samsung.com changed reviewers: + rouslan@chromium.org
Facing a issue while testing the change. Could you please suggest if there is simple way to test it? https://codereview.chromium.org/2145553002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentTestHelper.cpp (right): https://codereview.chromium.org/2145553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentTestHelper.cpp:165: // EXPECT_CALL(*m_mockFunctions.last(), call(testing::_)); As we have overridden the "call" method, EXPECT_CALL does not work. We can set a string (called/not called) here, but in that case we need to in many places in test files. Is there any better way to do it? Shall I create a separate class to test this change similar to [1] instead of modifying PaymentRequestMockFunctionScope class?
https://codereview.chromium.org/2145553002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentTestHelper.cpp (right): https://codereview.chromium.org/2145553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentTestHelper.cpp:165: // EXPECT_CALL(*m_mockFunctions.last(), call(testing::_)); [1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/streams/R...
Facing a issue while testing the change. Could you please suggest if there is simple way to test it?
See the solution to the testing issue inline. https://codereview.chromium.org/2145553002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/2145553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:593: String message = "The payment request is cancelled due to unknown reasons."; Other error messages in this file do not have a period (.) at the end. Let's not add it here either. https://codereview.chromium.org/2145553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:596: message = "User cancelled the payment request."; Remove the period. https://codereview.chromium.org/2145553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:600: message = "The payment method is not supported."; Remove the period. https://codereview.chromium.org/2145553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:602: default: Instead of "default", put the "UNKNOWN" enum value here. This way the compiler will ensure that you've handled all enum values. https://codereview.chromium.org/2145553002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp (right): https://codereview.chromium.org/2145553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:252: EXPECT_TRUE(onRejected.isNull()); Remove this EXPECT_TRUE(onRejected.isNull()) check. It's not important for the test. https://codereview.chromium.org/2145553002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentTestHelper.cpp (right): https://codereview.chromium.org/2145553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentTestHelper.cpp:165: // EXPECT_CALL(*m_mockFunctions.last(), call(testing::_)); Uncomment https://codereview.chromium.org/2145553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentTestHelper.cpp:172: // EXPECT_CALL(*m_mockFunctions.last(), call(testing::_)).Times(0); Uncomment https://codereview.chromium.org/2145553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentTestHelper.cpp:175: Put the following code before MockFunction() constructor here: ACTION_P(SaveVaueIn, captor) { *captor = arg0; } https://github.com/google/googlemock/blob/master/googlemock/docs/CheatSheet.m... https://codereview.chromium.org/2145553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentTestHelper.cpp:179: // ON_CALL(*this, call(testing::_)).WillByDefault(testing::ReturnArg<0>()); Change this line to the following: ON_CALL(*this, call(testing::_)).WillByDefault( testing::DoAll(SaveValueIn(m_value), testing::ReturnArg<0>())); https://codereview.chromium.org/2145553002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentTestHelper.h (right): https://codereview.chromium.org/2145553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentTestHelper.h:79: explicit MockFunction(ScriptState*, String *captor); Remove "explicit" on this line. The keyword "explicit" is necessary only for single-parameter constructors. https://codereview.chromium.org/2145553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentTestHelper.h:81: // MOCK_METHOD1(call, ScriptValue(ScriptValue)); Leave MOCK_METHOD1() unmodified. https://codereview.chromium.org/2145553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentTestHelper.h:82: ScriptValue call(ScriptValue value) override Remove this method.
Patchset #3 (id:40001) has been deleted
Description was changed from ========== Parameterize OnError method. BUG= ========== to ========== Parameterize OnError method. This change is required to support following part of the spec. "Let acceptedMethods be supportedMethods with all identifiers removed that the user agent does not accept. If the length of acceptedMethods is zero, then reject acceptPromise with a NotSupportedError." BUG= ==========
Description was changed from ========== Parameterize OnError method. This change is required to support following part of the spec. "Let acceptedMethods be supportedMethods with all identifiers removed that the user agent does not accept. If the length of acceptedMethods is zero, then reject acceptPromise with a NotSupportedError." BUG= ========== to ========== Parameterize OnError method. This change is required to support following part of the spec. "Let acceptedMethods be supportedMethods with all identifiers removed that the user agent does not accept. If the length of acceptedMethods is zero, then reject acceptPromise with a NotSupportedError." BUG=627705 ==========
Thank you for the help. Please take another look. https://codereview.chromium.org/2145553002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/2145553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:593: String message = "The payment request is cancelled due to unknown reasons."; On 2016/07/12 15:57:35, Rouslan (ツ) wrote: > Other error messages in this file do not have a period (.) at the end. Let's not > add it here either. Done. https://codereview.chromium.org/2145553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:596: message = "User cancelled the payment request."; On 2016/07/12 15:57:35, Rouslan (ツ) wrote: > Remove the period. Done. https://codereview.chromium.org/2145553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:600: message = "The payment method is not supported."; On 2016/07/12 15:57:35, Rouslan (ツ) wrote: > Remove the period. Done. https://codereview.chromium.org/2145553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:602: default: On 2016/07/12 15:57:35, Rouslan (ツ) wrote: > Instead of "default", put the "UNKNOWN" enum value here. This way the compiler > will ensure that you've handled all enum values. Done. https://codereview.chromium.org/2145553002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp (right): https://codereview.chromium.org/2145553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:252: EXPECT_TRUE(onRejected.isNull()); On 2016/07/12 15:57:35, Rouslan (ツ) wrote: > Remove this EXPECT_TRUE(onRejected.isNull()) check. It's not important for the > test. Done. https://codereview.chromium.org/2145553002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentTestHelper.cpp (right): https://codereview.chromium.org/2145553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentTestHelper.cpp:165: // EXPECT_CALL(*m_mockFunctions.last(), call(testing::_)); On 2016/07/12 15:57:35, Rouslan (ツ) wrote: > Uncomment Done. https://codereview.chromium.org/2145553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentTestHelper.cpp:172: // EXPECT_CALL(*m_mockFunctions.last(), call(testing::_)).Times(0); On 2016/07/12 15:57:35, Rouslan (ツ) wrote: > Uncomment Done. https://codereview.chromium.org/2145553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentTestHelper.cpp:175: On 2016/07/12 15:57:35, Rouslan (ツ) wrote: > Put the following code before MockFunction() constructor here: > > ACTION_P(SaveVaueIn, captor) { > *captor = arg0; > } > > https://github.com/google/googlemock/blob/master/googlemock/docs/CheatSheet.m... Nice. Thank you. Done. Thanks https://codereview.chromium.org/2145553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentTestHelper.cpp:179: // ON_CALL(*this, call(testing::_)).WillByDefault(testing::ReturnArg<0>()); On 2016/07/12 15:57:35, Rouslan (ツ) wrote: > Change this line to the following: > > ON_CALL(*this, call(testing::_)).WillByDefault( > testing::DoAll(SaveValueIn(m_value), testing::ReturnArg<0>())); Done. https://codereview.chromium.org/2145553002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentTestHelper.h (right): https://codereview.chromium.org/2145553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentTestHelper.h:79: explicit MockFunction(ScriptState*, String *captor); On 2016/07/12 15:57:36, Rouslan (ツ) wrote: > Remove "explicit" on this line. The keyword "explicit" is necessary only for > single-parameter constructors. Done. https://codereview.chromium.org/2145553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentTestHelper.h:81: // MOCK_METHOD1(call, ScriptValue(ScriptValue)); On 2016/07/12 15:57:36, Rouslan (ツ) wrote: > Leave MOCK_METHOD1() unmodified. Done. https://codereview.chromium.org/2145553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentTestHelper.h:82: ScriptValue call(ScriptValue value) override On 2016/07/12 15:57:36, Rouslan (ツ) wrote: > Remove this method. Done.
The CQ bit was checked by sanjoy.pal@samsung.com 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 to run a CQ dry run
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
Patchset #4 (id:80001) has been deleted
Patchset #3 (id:60001) has been deleted
Great stuff! A few more comments + advice on fixing the integration tests. https://codereview.chromium.org/2145553002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2145553002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:834: mClient.onError(PaymentErrorReason.UNKNOWN); Let's use PaymentErrorReason.CANCEL for now. I will fix the Java UI code later. Samsung browser does not use Chrome's Java UI, so this code does not matter much to you. https://codereview.chromium.org/2145553002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/2145553002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:593: String message = "The payment request is cancelled due to unknown reasons"; No need to initialize this string. It will be overridden inside of the switch statement. https://codereview.chromium.org/2145553002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:596: message = "User cancelled the payment request"; Change this to "Request cancelled" to fix the integration tests. https://codereview.chromium.org/2145553002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:603: message = "The payment request is cancelled due to unknown reasons"; Let's use a shorter message, like "Request failed". https://codereview.chromium.org/2145553002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:605: } DCHECK(!message.IsEmpty()); https://codereview.chromium.org/2145553002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp (right): https://codereview.chromium.org/2145553002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:248: String onRejected; Let's use the variable name "errorMessage". https://codereview.chromium.org/2145553002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:265: String onRejected; errorMessage https://codereview.chromium.org/2145553002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:284: String onRejected; errorMessage https://codereview.chromium.org/2145553002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentTestHelper.h (right): https://codereview.chromium.org/2145553002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentTestHelper.h:72: v8::Local<v8::Function> expectCall(String*); String* captor https://codereview.chromium.org/2145553002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentTestHelper.h:79: MockFunction(ScriptState*, String *captor); String* captor
The CQ bit was checked by sanjoy.pal@samsung.com 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.
PTAL. https://codereview.chromium.org/2145553002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2145553002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:834: mClient.onError(PaymentErrorReason.UNKNOWN); On 2016/07/13 15:55:25, Rouslan (ツ) wrote: > Let's use PaymentErrorReason.CANCEL for now. I will fix the Java UI code later. > Samsung browser does not use Chrome's Java UI, so this code does not matter much > to you. Ok. I was going through this file, looks like parseAndValidateDetailsOrDisconnectFromClient and getValidatedMethodData are doing similar validation which is done in blink side. https://codereview.chromium.org/2145553002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/2145553002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:593: String message = "The payment request is cancelled due to unknown reasons"; On 2016/07/13 15:55:26, Rouslan (ツ) wrote: > No need to initialize this string. It will be overridden inside of the switch > statement. Done. https://codereview.chromium.org/2145553002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:596: message = "User cancelled the payment request"; On 2016/07/13 15:55:26, Rouslan (ツ) wrote: > Change this to "Request cancelled" to fix the integration tests. Done. https://codereview.chromium.org/2145553002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:603: message = "The payment request is cancelled due to unknown reasons"; On 2016/07/13 15:55:26, Rouslan (ツ) wrote: > Let's use a shorter message, like "Request failed". Done. https://codereview.chromium.org/2145553002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:605: } On 2016/07/13 15:55:26, Rouslan (ツ) wrote: > DCHECK(!message.IsEmpty()); Done. https://codereview.chromium.org/2145553002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp (right): https://codereview.chromium.org/2145553002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:248: String onRejected; On 2016/07/13 15:55:26, Rouslan (ツ) wrote: > Let's use the variable name "errorMessage". Done. https://codereview.chromium.org/2145553002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:265: String onRejected; On 2016/07/13 15:55:26, Rouslan (ツ) wrote: > errorMessage Done. https://codereview.chromium.org/2145553002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:284: String onRejected; On 2016/07/13 15:55:26, Rouslan (ツ) wrote: > errorMessage Done. https://codereview.chromium.org/2145553002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentTestHelper.h (right): https://codereview.chromium.org/2145553002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentTestHelper.h:72: v8::Local<v8::Function> expectCall(String*); On 2016/07/13 15:55:26, Rouslan (ツ) wrote: > String* captor Done. https://codereview.chromium.org/2145553002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentTestHelper.h:79: MockFunction(ScriptState*, String *captor); On 2016/07/13 15:55:26, Rouslan (ツ) wrote: > String* captor Done.
lgtm https://codereview.chromium.org/2145553002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2145553002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:834: mClient.onError(PaymentErrorReason.CANCEL); Sorry, didn't mean to make you switch from USER_CANCEL to CANCEL. Either one is OK. No need to change now.
rouslan@chromium.org changed reviewers: + dfalcantara@chromium.org, palmer@chromium.org
dfalcantara@, owners ptal PaymentRequestImpl.java (I should really add myself to owners there.) palmer@, ptal payment_request.mojom.
Java OWNERS lgtm
The CQ bit was checked by sanjoy.pal@samsung.com
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2016/07/15 03:51:04, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) Oops. palmer@ ptal.
IPC security LGTM but you might want to set more meaningful |ExceptionCode|s, if possible. https://codereview.chromium.org/2145553002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/2145553002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:595: case mojom::blink::PaymentErrorReason::CANCEL: |SyntaxError| doesn't seem to be the right |ExceptionCode| for CANCEL or UNKNOWN. Are there other codes you can use for those?
https://codereview.chromium.org/2145553002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/2145553002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:595: case mojom::blink::PaymentErrorReason::CANCEL: On 2016/07/16 00:37:33, palmer wrote: > |SyntaxError| doesn't seem to be the right |ExceptionCode| for CANCEL or > UNKNOWN. Are there other codes you can use for those? The spec is vague here, but I think the correct approach is to reject with a string for CANCEL and UnknownError for UNKNOWN. bool isError = false; ExceptionCode ec; String message; switch (error) { case mojom::blink::PaymentErrorReason::CANCEL: message = "Request cancelled"; break; case mojom::blink::PaymentErrorReason::NOT_SUPPORTED: isError = true; ec = NotSupportedError; message = "The payment method is not supported"; break; case mojom::blink::PaymentErrorReason::UNKNOWN: isError = true; ec = UnknownError; message = "Request failed"; break; } DCHECK(!message.isEmpty()); if (m_completeResolver) m_completeResolver->reject(isError ? DOMException::create(ec, message) : message); if (m_showResolver) m_showResolver->reject(isError ? DOMException::create(ec, message) : message); if (m_abortResolver) m_abortResolver->reject(isError ? DOMException::create(ec, message) : message); clearResolversAndCloseMojoConnection();
sanjoy.pal@: Please change the code as I've specified and then you're OK to commit.
Please take a look. https://codereview.chromium.org/2145553002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/2145553002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:595: case mojom::blink::PaymentErrorReason::CANCEL: Compiler gives following error for the above code. "../../third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:615:44: error: incompatible operand types ('blink::DOMException *' and 'WTF::String') m_completeResolver->reject(isError ? DOMException::create(ec, message) : message); " So, I changed the code accordingly. Please review once more.
Patchset #6 (id:160001) has been deleted
https://codereview.chromium.org/2145553002/diff/180001/chrome/test/data/andro... File chrome/test/data/android/payments/contact_details.js (right): https://codereview.chromium.org/2145553002/diff/180001/chrome/test/data/andro... chrome/test/data/android/payments/contact_details.js:26: }, function(error) { This change should not be necessary. The following two ways of using promises are equivalent: #1: complete().then(functionA, functionB); #2: complete().then(functionA).catch(functionB); https://codereview.chromium.org/2145553002/diff/180001/chrome/test/data/andro... chrome/test/data/android/payments/contact_details.js:32: }, function(error) { Ditto https://codereview.chromium.org/2145553002/diff/180001/chrome/test/data/andro... File chrome/test/data/android/payments/dynamic_shipping.js (right): https://codereview.chromium.org/2145553002/diff/180001/chrome/test/data/andro... chrome/test/data/android/payments/dynamic_shipping.js:41: }, function(error) { Ditto https://codereview.chromium.org/2145553002/diff/180001/chrome/test/data/andro... chrome/test/data/android/payments/dynamic_shipping.js:47: }, function(error) { Ditto https://codereview.chromium.org/2145553002/diff/180001/chrome/test/data/andro... File chrome/test/data/android/payments/email.js (right): https://codereview.chromium.org/2145553002/diff/180001/chrome/test/data/andro... chrome/test/data/android/payments/email.js:25: }, function(error) { Ditto https://codereview.chromium.org/2145553002/diff/180001/chrome/test/data/andro... chrome/test/data/android/payments/email.js:31: }, function(error) { Ditto https://codereview.chromium.org/2145553002/diff/180001/chrome/test/data/andro... File chrome/test/data/android/payments/free_shipping.js (right): https://codereview.chromium.org/2145553002/diff/180001/chrome/test/data/andro... chrome/test/data/android/payments/free_shipping.js:36: }, function(error) { Ditto https://codereview.chromium.org/2145553002/diff/180001/chrome/test/data/andro... chrome/test/data/android/payments/free_shipping.js:42: }, function(error) { Ditto https://codereview.chromium.org/2145553002/diff/180001/chrome/test/data/andro... File chrome/test/data/android/payments/no_shipping.js (right): https://codereview.chromium.org/2145553002/diff/180001/chrome/test/data/andro... chrome/test/data/android/payments/no_shipping.js:24: }, function(error) { Ditto https://codereview.chromium.org/2145553002/diff/180001/chrome/test/data/andro... chrome/test/data/android/payments/no_shipping.js:30: }, function(error) { Ditto https://codereview.chromium.org/2145553002/diff/180001/chrome/test/data/andro... File chrome/test/data/android/payments/phone.js (right): https://codereview.chromium.org/2145553002/diff/180001/chrome/test/data/andro... chrome/test/data/android/payments/phone.js:25: }, function(error) { Ditto https://codereview.chromium.org/2145553002/diff/180001/chrome/test/data/andro... chrome/test/data/android/payments/phone.js:31: }, function(error) { Ditto
The CQ bit was checked by sanjoy.pal@samsung.com 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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by sanjoy.pal@samsung.com 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 sanjoy.pal@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org, rouslan@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2145553002/#ps220001 (title: "fixing integration test failures")
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 ========== Parameterize OnError method. This change is required to support following part of the spec. "Let acceptedMethods be supportedMethods with all identifiers removed that the user agent does not accept. If the length of acceptedMethods is zero, then reject acceptPromise with a NotSupportedError." BUG=627705 ========== to ========== Parameterize OnError method. This change is required to support following part of the spec. "Let acceptedMethods be supportedMethods with all identifiers removed that the user agent does not accept. If the length of acceptedMethods is zero, then reject acceptPromise with a NotSupportedError." BUG=627705 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Parameterize OnError method. This change is required to support following part of the spec. "Let acceptedMethods be supportedMethods with all identifiers removed that the user agent does not accept. If the length of acceptedMethods is zero, then reject acceptPromise with a NotSupportedError." BUG=627705 ========== to ========== Parameterize OnError method. This change is required to support following part of the spec. "Let acceptedMethods be supportedMethods with all identifiers removed that the user agent does not accept. If the length of acceptedMethods is zero, then reject acceptPromise with a NotSupportedError." BUG=627705 Committed: https://crrev.com/e542d0af7a4908ec043b86d858bb9206a1c9c7f9 Cr-Commit-Position: refs/heads/master@{#406227} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/e542d0af7a4908ec043b86d858bb9206a1c9c7f9 Cr-Commit-Position: refs/heads/master@{#406227} |
