Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2501593003:
Implement the new basic card specification. (Closed)
|
Created:
4 years, 1 month ago by please use gerrit instead Modified:
3 years, 11 months ago CC:
Aaron Boodman, abarth-chromium, agrieve+watch_chromium.org, blink-reviews, chromium-reviews, darin (slow to review), qsr+mojo_chromium.org, rouslan+payments_chromium.org, sebsg+paymentswatch_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement the new basic card specification.
The basic card specification has been updated to include card types
(credit, debit, prepaid), which are distinct from card networks (amex,
discover, mastercard, visa, etc). This patch adds support for the new
format without removing support for the old format. Parsing of the JSON
data that encodes the parameters happens in the renderer for safety.
Old format (still supported):
new PaymentRequest([{
supportedMethods: ['visa']
}],
shoppingCartContents);
New format (in this patch):
new PaymentRequest([{
supportedMethods: ['basic-card'],
data: {
supportedNetworks: ['visa'],
supportedTypes: ['debit']
}
}],
shoppingCartContents);
Basic card spec:
https://w3c.github.io/webpayments-methods-card/
Intent to implement and ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/IYRjdUKxCoM/8B-jp4g9AgAJ
BUG=665190
Review-Url: https://codereview.chromium.org/2501593003
Cr-Commit-Position: refs/heads/master@{#442281}
Committed: https://chromium.googlesource.com/chromium/src/+/b1be35c4c222d904ae02ea5c5dfdf1972d031f1e
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 4
Patch Set 3 : Don't use TrackExceptionState #
Total comments: 4
Patch Set 4 : Address comments #Patch Set 5 : Hide behind flag #
Total comments: 4
Patch Set 6 : Address review comments for Java #
Total comments: 1
Patch Set 7 : Rebase #Patch Set 8 : Rebase #Patch Set 9 : Rebase #Patch Set 10 : Ignore exception when parsing method-specific data in renderer per issue discussion #Patch Set 11 : Update the tests to ignore exception when parsing method-specific data in renderer per issue discus… #
Total comments: 2
Patch Set 12 : Parse basic-card method data even if Android Pay method data could not be parsed #Patch Set 13 : Fix chrome_test_apk compile #Patch Set 14 : Rebase #Patch Set 15 : Rebase #Patch Set 16 : Rebase #
Total comments: 3
Patch Set 17 : Rebase + comment #Patch Set 18 : Remove last remaining DummyExceptionStateForTesting #Patch Set 19 : Rebase #Dependent Patchsets: Messages
Total messages: 150 (102 generated)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by rouslan@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:40001) has been deleted
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/v2/patch-status/codereview.chromium.or...
Description was changed from
==========
Implement the new basic card specification.
The basic card specification has been updated to include card types
(credit, debit, prepaid), which are distinct from card networks (amex,
discover, mastercard, visa, etc). This patch adds support for the new
format without removing support for the old format. Parsing of the JSON
data that encodes the parameters happens in the renderer for safety.
BUG=665190
==========
to
==========
Implement the new basic card specification.
The basic card specification has been updated to include card types
(credit, debit, prepaid), which are distinct from card networks (amex,
discover, mastercard, visa, etc). This patch adds support for the new
format without removing support for the old format. Parsing of the JSON
data that encodes the parameters happens in the renderer for safety.
Old format (still supported):
new PaymentRequest([{
supportedMethods: ['visa']
}],
shoppingCartContents);
New format (in this patch):
new PaymentRequest([{
supportedMethods: ['basic-card'],
data: {
supportedNetworks: ['visa'],
supportedTypes: ['debit']
}
}],
shoppingCartContents);
BUG=665190
==========
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Description was changed from
==========
Implement the new basic card specification.
The basic card specification has been updated to include card types
(credit, debit, prepaid), which are distinct from card networks (amex,
discover, mastercard, visa, etc). This patch adds support for the new
format without removing support for the old format. Parsing of the JSON
data that encodes the parameters happens in the renderer for safety.
Old format (still supported):
new PaymentRequest([{
supportedMethods: ['visa']
}],
shoppingCartContents);
New format (in this patch):
new PaymentRequest([{
supportedMethods: ['basic-card'],
data: {
supportedNetworks: ['visa'],
supportedTypes: ['debit']
}
}],
shoppingCartContents);
BUG=665190
==========
to
==========
Implement the new basic card specification.
https://w3c.github.io/webpayments-methods-card/
The basic card specification has been updated to include card types
(credit, debit, prepaid), which are distinct from card networks (amex,
discover, mastercard, visa, etc). This patch adds support for the new
format without removing support for the old format. Parsing of the JSON
data that encodes the parameters happens in the renderer for safety.
Old format (still supported):
new PaymentRequest([{
supportedMethods: ['visa']
}],
shoppingCartContents);
New format (in this patch):
new PaymentRequest([{
supportedMethods: ['basic-card'],
data: {
supportedNetworks: ['visa'],
supportedTypes: ['debit']
}
}],
shoppingCartContents);
API surface is not changed.
BUG=665190
==========
Description was changed from ========== Implement the new basic card specification. https://w3c.github.io/webpayments-methods-card/ The basic card specification has been updated to include card types (credit, debit, prepaid), which are distinct from card networks (amex, discover, mastercard, visa, etc). This patch adds support for the new format without removing support for the old format. Parsing of the JSON data that encodes the parameters happens in the renderer for safety. Old format (still supported): new PaymentRequest([{ supportedMethods: ['visa'] }], shoppingCartContents); New format (in this patch): new PaymentRequest([{ supportedMethods: ['basic-card'], data: { supportedNetworks: ['visa'], supportedTypes: ['debit'] } }], shoppingCartContents); API surface is not changed. BUG=665190 ========== to ========== Implement the new basic card specification. The basic card specification has been updated to include card types (credit, debit, prepaid), which are distinct from card networks (amex, discover, mastercard, visa, etc). This patch adds support for the new format without removing support for the old format. Parsing of the JSON data that encodes the parameters happens in the renderer for safety. Old format (still supported): new PaymentRequest([{ supportedMethods: ['visa'] }], shoppingCartContents); New format (in this patch): new PaymentRequest([{ supportedMethods: ['basic-card'], data: { supportedNetworks: ['visa'], supportedTypes: ['debit'] } }], shoppingCartContents); Basic card spec: https://w3c.github.io/webpayments-methods-card/ (The spec does not affect API surface.) The PaymentRequest.canMakeActivePayment() feature, which is currently hidden behind a flag, throttles queries of supported methods to reduce the risk of user fingerprinting. Since 'basic-card' method can be used with different subsets of card networks ('visa', 'mastercard', etc.) and types ('credit', 'debit', 'prepaid'), this patch changes canMakeActivePayment() to return last cached result instead of re-querying every time. This is more inline with the proposed specification language: "Multiple calls to canMakeActivePayment will result in cached response from previous call." PaymentRequest.canMakeActivePayment() spec pull request: https://github.com/w3c/browser-payment-api/pull/316/files BUG=665190 ==========
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/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:120001) has been deleted
rouslan@chromium.org changed reviewers: + dfalcantara@chromium.org, mek@chromium.org, tsepez@chromium.org
Marijn, ptal WebKit. Does this change need an intent to ship on blink-dev. I'm thinking no. Tom, ptal payment_request.mojom. Dan, ptal Android.
rouslan@chromium.org changed reviewers: + esprehn@chromium.org
Elliott, ptal third_party/WebKit/Source/modules/modules_idl_files.gni
Description was changed from
==========
Implement the new basic card specification.
The basic card specification has been updated to include card types
(credit, debit, prepaid), which are distinct from card networks (amex,
discover, mastercard, visa, etc). This patch adds support for the new
format without removing support for the old format. Parsing of the JSON
data that encodes the parameters happens in the renderer for safety.
Old format (still supported):
new PaymentRequest([{
supportedMethods: ['visa']
}],
shoppingCartContents);
New format (in this patch):
new PaymentRequest([{
supportedMethods: ['basic-card'],
data: {
supportedNetworks: ['visa'],
supportedTypes: ['debit']
}
}],
shoppingCartContents);
Basic card spec:
https://w3c.github.io/webpayments-methods-card/
(The spec does not affect API surface.)
The PaymentRequest.canMakeActivePayment() feature, which is currently
hidden behind a flag, throttles queries of supported methods to reduce
the risk of user fingerprinting. Since 'basic-card' method can be used
with different subsets of card networks ('visa', 'mastercard', etc.) and
types ('credit', 'debit', 'prepaid'), this patch changes
canMakeActivePayment() to return last cached result instead of
re-querying every time. This is more inline with the proposed
specification language:
"Multiple calls to canMakeActivePayment will result in cached response
from previous call."
PaymentRequest.canMakeActivePayment() spec pull request:
https://github.com/w3c/browser-payment-api/pull/316/files
BUG=665190
==========
to
==========
Implement the new basic card specification.
The basic card specification has been updated to include card types
(credit, debit, prepaid), which are distinct from card networks (amex,
discover, mastercard, visa, etc). This patch adds support for the new
format without removing support for the old format. Parsing of the JSON
data that encodes the parameters happens in the renderer for safety.
Old format (still supported):
new PaymentRequest([{
supportedMethods: ['visa']
}],
shoppingCartContents);
New format (in this patch):
new PaymentRequest([{
supportedMethods: ['basic-card'],
data: {
supportedNetworks: ['visa'],
supportedTypes: ['debit']
}
}],
shoppingCartContents);
Basic card spec:
https://w3c.github.io/webpayments-methods-card/
(The spec does not affect API surface.)
The PaymentRequest.canMakeActivePayment() feature, which is currently
hidden behind a flag, throttles queries of supported methods to reduce
the risk of user fingerprinting. Since 'basic-card' method can be used
with different subsets of card networks ('visa', 'mastercard', etc.) and
types ('credit', 'debit', 'prepaid'), this patch changes
canMakeActivePayment() to return last cached result instead of
re-querying every time. This also happens to be more inline with the
proposed specification language:
"Multiple calls to canMakeActivePayment will result in cached response
from previous call."
Proposed PaymentRequest.canMakeActivePayment() specification:
https://github.com/w3c/browser-payment-api/pull/316/files
BUG=665190
==========
mojom LGTM
Is there any way to make break this patch up?
On 2016/11/18 20:05:26, dfalcantara (check my queue) wrote: > Is there any way to make break this patch up? I think I broke Dan.
Breaking up in process. Please stand by for a rebase on top of other parts. Tom: no need to re-review mojom, because it will stay in this patch.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from
==========
Implement the new basic card specification.
The basic card specification has been updated to include card types
(credit, debit, prepaid), which are distinct from card networks (amex,
discover, mastercard, visa, etc). This patch adds support for the new
format without removing support for the old format. Parsing of the JSON
data that encodes the parameters happens in the renderer for safety.
Old format (still supported):
new PaymentRequest([{
supportedMethods: ['visa']
}],
shoppingCartContents);
New format (in this patch):
new PaymentRequest([{
supportedMethods: ['basic-card'],
data: {
supportedNetworks: ['visa'],
supportedTypes: ['debit']
}
}],
shoppingCartContents);
Basic card spec:
https://w3c.github.io/webpayments-methods-card/
(The spec does not affect API surface.)
The PaymentRequest.canMakeActivePayment() feature, which is currently
hidden behind a flag, throttles queries of supported methods to reduce
the risk of user fingerprinting. Since 'basic-card' method can be used
with different subsets of card networks ('visa', 'mastercard', etc.) and
types ('credit', 'debit', 'prepaid'), this patch changes
canMakeActivePayment() to return last cached result instead of
re-querying every time. This also happens to be more inline with the
proposed specification language:
"Multiple calls to canMakeActivePayment will result in cached response
from previous call."
Proposed PaymentRequest.canMakeActivePayment() specification:
https://github.com/w3c/browser-payment-api/pull/316/files
BUG=665190
==========
to
==========
Implement the new basic card specification.
The basic card specification has been updated to include card types
(credit, debit, prepaid), which are distinct from card networks (amex,
discover, mastercard, visa, etc). This patch adds support for the new
format without removing support for the old format. Parsing of the JSON
data that encodes the parameters happens in the renderer for safety.
Old format (still supported):
new PaymentRequest([{
supportedMethods: ['visa']
}],
shoppingCartContents);
New format (in this patch):
new PaymentRequest([{
supportedMethods: ['basic-card'],
data: {
supportedNetworks: ['visa'],
supportedTypes: ['debit']
}
}],
shoppingCartContents);
Basic card spec:
https://w3c.github.io/webpayments-methods-card/
(The spec does not affect API surface.)
BUG=665190
==========
Dan, ptal patch 2. I've split out and separately landed several parts of this CL: - http://crrev.com/2507063009 - http://crrev.com/2509323006 - http://crrev.com/2511953004
On 2016/11/22 00:37:17, rouslan wrote: > Dan, ptal patch 2. I've split out and separately landed several parts of this > CL: > > - http://crrev.com/2507063009 > - http://crrev.com/2509323006 > - http://crrev.com/2511953004 No need to review those CLs, btw. They have already been landed and reviewed.
On 2016/11/22 00:37:45, rouslan wrote: > landed and reviewed. Not in that order.
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/2501593003/diff/150001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/2501593003/diff/150001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:453: TrackExceptionState exceptionState; It looks strange to use TrackExceptionState in production code, since it means you're ignoring an exception.
https://codereview.chromium.org/2501593003/diff/150001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/2501593003/diff/150001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:453: TrackExceptionState exceptionState; On 2016/11/22 01:18:59, haraken wrote: > > It looks strange to use TrackExceptionState in production code, since it means > you're ignoring an exception. That's an excellent point. I am indeed ignoring the exception, because the "input" is supposed to be specific to a payment app. The parsing of payment app specific data should not throw exceptions, because it's up to the payment app to decide what to do with this data. Eventually this structured data will reach the app in the browser, (for example, Android Pay). If the data is not suitable for the app, either PaymentRequest.show() promise is eventually rejected or canMakeActivePayment() resolves with "false".
https://codereview.chromium.org/2501593003/diff/150001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/2501593003/diff/150001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:453: TrackExceptionState exceptionState; On 2016/11/22 03:10:55, rouslan wrote: > On 2016/11/22 01:18:59, haraken wrote: > > > > It looks strange to use TrackExceptionState in production code, since it means > > you're ignoring an exception. > > That's an excellent point. I am indeed ignoring the exception, because the > "input" is supposed to be specific to a payment app. The parsing of payment app > specific data should not throw exceptions, because it's up to the payment app to > decide what to do with this data. Eventually this structured data will reach the > app in the browser, (for example, Android Pay). If the data is not suitable for > the app, either PaymentRequest.show() promise is eventually rejected or > canMakeActivePayment() resolves with "false". Yeah, but would it be better to pass in the ExceptionState from validateAndConvertPaymentMethodData? If you want to ignore, you can just explicitly call exceptionState.clear().
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/v2/patch-status/codereview.chromium.or...
ptal patch 3 https://codereview.chromium.org/2501593003/diff/150001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/2501593003/diff/150001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:453: TrackExceptionState exceptionState; Done. I'm deferring to your expertise on this subject, but it would be great to hear an explanation of why you think this approach is best in this case.
LGTM On 2016/11/22 15:20:03, rouslan wrote: > ptal patch 3 > > https://codereview.chromium.org/2501593003/diff/150001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): > > https://codereview.chromium.org/2501593003/diff/150001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:453: > TrackExceptionState exceptionState; > Done. I'm deferring to your expertise on this subject, but it would be great to > hear an explanation of why you think this approach is best in this case. Sorry for not being clear. TrackExceptionState is designed for testing purposes and should not be used in production code. I should have renamed it to IgnoredExceptionStateForTesting :/ If you really want to ignore exceptions for some valid reason, you should explicitly do that by calling .clearException(). I'll do the rename and add a comment to ExceptionState.h.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Why do you want to ignore this exceptions? That's very unusual and sounds like a spec bug. You want to reject the promise with the exception when one occurs.
On 2016/11/22 20:34:58, esprehn wrote: > Why do you want to ignore this exceptions? That's very unusual and sounds like a > spec bug. You want to reject the promise with the exception when one occurs. Short answer: It's for forward compatibility with other payment apps that might use a similar data structure. Long answer: I'm ignoring exceptions in parsing of "data" here. The spec at https://w3c.github.io/browser-payment-api/#dom-paymentmethoddata has this to say about "data": "data is a JSON-serializable object that provides optional information that might be needed by the supported payment methods." Supported payment methods and their behavior are decoupled from the PaymentRequest spec. The job of the PaymentRequest spec is to pass the "data" object to the payment apps that match the supported methods. The two currently inegrated payment apps live in the browser: Autofill and Android Pay. In order to avoid parsing the "data" in the browse, I pre-parse it in the renderer and then pass the structured data over IPC to the browser. Other apps in the future will integrate into Chrome. They might use similar data structures to what's used by Autofill and Android Pay. Although parsing Autofill-specifc and Android-Pay-specific structures out of the "data" might throw exceptions for the similar data formats, we don't want to abort the payment at this point. Instead we want to pass the stringified version of the "data" to the 3rd party payment apps. These apps can attempt to parse out their own data parameters and decide for themselves whether they they are available for payment. For example, this code currently works for Autofill payment app: new PaymentRequest([{supportedMethods: ["basic-card"], data: {"supportedNetworks": ["visa"]}}], shoppingCart); This code will throw in V8BasicCardRequest::toImpl(): new PaymentRequest([{supportedMethods: ["basic-card"], data: {"supportedNetworks": 0}}], shoppingCart); The behavior in my code is to not pass this data to Autofill payment app at all. However, another 3rd party app might be OK with this format of data. So I would not want to reject this payment request. What do you think?
https://codereview.chromium.org/2501593003/diff/170001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentApp.java (right): https://codereview.chromium.org/2501593003/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentApp.java:110: types.add(BasicCardType.CREDIT); Are you basically making sure that supportedTypes has all three of these in there? Is this cleaner than just checking if data.supportedTypes contains all three of those things without constructing the set? Something like if (!data.supportedTypes.contains(BasicCardType.CREDIT)) return false; if (!data.supportedTypes.contains(BasicCardType.DEBIT)) return false; if (!data.supportedTypes.contains(BasicCardType.PREPAID)) return false; https://codereview.chromium.org/2501593003/diff/170001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java (right): https://codereview.chromium.org/2501593003/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java:263: } Would it be clearer to: mAcceptedCardTypeInfos.clear(); mAcceptedCardTypeInfos.addAll(mAcceptedCardTypeInfos, unique); Then you don't need to track write as a separate variable or manually remove things at the end. Alternatively, if you're trying to only keep track of unique things, why not switch to a Set instead of a List?
On 2016/11/22 at 20:52:54, rouslan wrote:
> On 2016/11/22 20:34:58, esprehn wrote:
> > Why do you want to ignore this exceptions? That's very unusual and sounds
like a
> > spec bug. You want to reject the promise with the exception when one occurs.
>
> ...
>
> For example, this code currently works for Autofill payment app:
>
> new PaymentRequest([{supportedMethods: ["basic-card"], data:
{"supportedNetworks": ["visa"]}}], shoppingCart);
>
> This code will throw in V8BasicCardRequest::toImpl():
>
> new PaymentRequest([{supportedMethods: ["basic-card"], data:
{"supportedNetworks": 0}}], shoppingCart);
>
> The behavior in my code is to not pass this data to Autofill payment app at
all. However, another 3rd party app might be OK with this format of data. So I
would not want to reject this payment request. What do you think?
That sounds incredibly hard for authors to use since the spec is too vague to
actually implement and the author will need to reverse engineer the requirements
in chrome for what the structure of the data is required to be for Firefox vs
Chrome vs Opera etc.
I dont think we want this spec to just say "stuff happens in a third party
payment app", it needs to be specific enough that an author can write code that
works in all browsers with confidence. Realistically what will happen is other
browsers will be forced to reverse engineer our data formats to be compatible
with the chrome authored content.
On 2016/11/22 21:42:46, esprehn wrote:
> On 2016/11/22 at 20:52:54, rouslan wrote:
> > On 2016/11/22 20:34:58, esprehn wrote:
> > > Why do you want to ignore this exceptions? That's very unusual and sounds
> like a
> > > spec bug. You want to reject the promise with the exception when one
occurs.
> >
> > ...
> >
> > For example, this code currently works for Autofill payment app:
> >
> > new PaymentRequest([{supportedMethods: ["basic-card"], data:
> {"supportedNetworks": ["visa"]}}], shoppingCart);
> >
> > This code will throw in V8BasicCardRequest::toImpl():
> >
> > new PaymentRequest([{supportedMethods: ["basic-card"], data:
> {"supportedNetworks": 0}}], shoppingCart);
> >
> > The behavior in my code is to not pass this data to Autofill payment app at
> all. However, another 3rd party app might be OK with this format of data. So I
> would not want to reject this payment request. What do you think?
>
> That sounds incredibly hard for authors to use since the spec is too vague to
> actually implement and the author will need to reverse engineer the
requirements
> in chrome for what the structure of the data is required to be for Firefox vs
> Chrome vs Opera etc.
>
> I dont think we want this spec to just say "stuff happens in a third party
> payment app", it needs to be specific enough that an author can write code
that
> works in all browsers with confidence. Realistically what will happen is other
> browsers will be forced to reverse engineer our data formats to be compatible
> with the chrome authored content.
Even though this CL doesn't change the visible API surface area, it is exposing
a new API to the web (otherwise there'd be no need for a spec or developer
documentation about how to use this). Blink has a policy
(https://www.chromium.org/blink#launch-process) of discussing all new APIs on
blink-dev so that:
- Everyone interested is aware of the new APIs being shipped and has a
reasonable opportunity to provide feedback on the design before chromium is
stuck supporting it forever.
- There is an explicit discussion of the interop risk tradeoffs - the risk that
the feature won't ultimately work the same for web developers across all
browsers (such as Elliot is raising here)
- The blink project upholds it's commitment to the community
(https://blog.chromium.org/2013/04/blink-rendering-engine-for-chromium.html) to
be transparent about the changes we're introducing to the web platform, and the
tradeoffs that have gone into a decision that affects all web stakeholders.
So IMHO this is not LGTM until we have an approved "intent to ship".
On 2016/11/22 22:35:17, Rick Byers wrote:
> On 2016/11/22 21:42:46, esprehn wrote:
> > On 2016/11/22 at 20:52:54, rouslan wrote:
> > > On 2016/11/22 20:34:58, esprehn wrote:
> > > > Why do you want to ignore this exceptions? That's very unusual and
sounds
> > like a
> > > > spec bug. You want to reject the promise with the exception when one
> occurs.
> > >
> > > ...
> > >
> > > For example, this code currently works for Autofill payment app:
> > >
> > > new PaymentRequest([{supportedMethods: ["basic-card"], data:
> > {"supportedNetworks": ["visa"]}}], shoppingCart);
> > >
> > > This code will throw in V8BasicCardRequest::toImpl():
> > >
> > > new PaymentRequest([{supportedMethods: ["basic-card"], data:
> > {"supportedNetworks": 0}}], shoppingCart);
> > >
> > > The behavior in my code is to not pass this data to Autofill payment app
at
> > all. However, another 3rd party app might be OK with this format of data. So
I
> > would not want to reject this payment request. What do you think?
> >
> > That sounds incredibly hard for authors to use since the spec is too vague
to
> > actually implement and the author will need to reverse engineer the
> requirements
> > in chrome for what the structure of the data is required to be for Firefox
vs
> > Chrome vs Opera etc.
> >
> > I dont think we want this spec to just say "stuff happens in a third party
> > payment app", it needs to be specific enough that an author can write code
> that
> > works in all browsers with confidence. Realistically what will happen is
other
> > browsers will be forced to reverse engineer our data formats to be
compatible
> > with the chrome authored content.
>
> Even though this CL doesn't change the visible API surface area, it is
exposing
> a new API to the web (otherwise there'd be no need for a spec or developer
> documentation about how to use this). Blink has a policy
> (https://www.chromium.org/blink#launch-process) of discussing all new APIs on
> blink-dev so that:
> - Everyone interested is aware of the new APIs being shipped and has a
> reasonable opportunity to provide feedback on the design before chromium is
> stuck supporting it forever.
> - There is an explicit discussion of the interop risk tradeoffs - the risk
that
> the feature won't ultimately work the same for web developers across all
> browsers (such as Elliot is raising here)
> - The blink project upholds it's commitment to the community
> (https://blog.chromium.org/2013/04/blink-rendering-engine-for-chromium.html)
to
> be transparent about the changes we're introducing to the web platform, and
the
> tradeoffs that have gone into a decision that affects all web stakeholders.
>
> So IMHO this is not LGTM until we have an approved "intent to ship".
Do we need an Intent-to-ship for features under status=experimental?
PaymentRequest is under status=experimental.
(I just want to clarify when we need to send an Intent-to-ship.)
On 2016/11/22 at 23:31:59, haraken wrote: > ... > > So IMHO this is not LGTM until we have an approved "intent to ship". > > Do we need an Intent-to-ship for features under status=experimental? PaymentRequest is under status=experimental. > > (I just want to clarify when we need to send an Intent-to-ship.) Is it really experimental though? I think that's what the runtime flag does but then content forces it on in stable for Android?
It's stable on Android and experimental on all other platforms.
Intent to ship sent out: https://groups.google.com/a/chromium.org/d/msg/blink-dev/IYRjdUKxCoM/8B-jp4g9... We can continue reviewing while waiting for approval. Worst case scenario is placing this behind a runtime flag, right?
Description was changed from
==========
Implement the new basic card specification.
The basic card specification has been updated to include card types
(credit, debit, prepaid), which are distinct from card networks (amex,
discover, mastercard, visa, etc). This patch adds support for the new
format without removing support for the old format. Parsing of the JSON
data that encodes the parameters happens in the renderer for safety.
Old format (still supported):
new PaymentRequest([{
supportedMethods: ['visa']
}],
shoppingCartContents);
New format (in this patch):
new PaymentRequest([{
supportedMethods: ['basic-card'],
data: {
supportedNetworks: ['visa'],
supportedTypes: ['debit']
}
}],
shoppingCartContents);
Basic card spec:
https://w3c.github.io/webpayments-methods-card/
(The spec does not affect API surface.)
BUG=665190
==========
to
==========
Implement the new basic card specification.
The basic card specification has been updated to include card types
(credit, debit, prepaid), which are distinct from card networks (amex,
discover, mastercard, visa, etc). This patch adds support for the new
format without removing support for the old format. Parsing of the JSON
data that encodes the parameters happens in the renderer for safety.
Old format (still supported):
new PaymentRequest([{
supportedMethods: ['visa']
}],
shoppingCartContents);
New format (in this patch):
new PaymentRequest([{
supportedMethods: ['basic-card'],
data: {
supportedNetworks: ['visa'],
supportedTypes: ['debit']
}
}],
shoppingCartContents);
Basic card spec:
https://w3c.github.io/webpayments-methods-card/
Intent to implement and ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/IYRjdUKxCoM/8B-jp4g9...
BUG=665190
==========
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/v2/patch-status/codereview.chromium.or...
Elliot, ptal patch 4. I've found a compromise: throw an exception only if "basic-card" is the method name and its method-specific data does not have correct format. https://codereview.chromium.org/2501593003/diff/170001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentApp.java (right): https://codereview.chromium.org/2501593003/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentApp.java:110: types.add(BasicCardType.CREDIT); On 2016/11/22 21:28:47, dfalcantara (check my queue) wrote: > Are you basically making sure that supportedTypes has all three of these in > there? Is this cleaner than just checking if data.supportedTypes contains all > three of those things without constructing the set? > > Something like > if (!data.supportedTypes.contains(BasicCardType.CREDIT)) return false; > if (!data.supportedTypes.contains(BasicCardType.DEBIT)) return false; > if (!data.supportedTypes.contains(BasicCardType.PREPAID)) return false; data.supportedTypes is int[] array. Still need a set, but I can reduce the number of lines slightly using something similar to your method. https://codereview.chromium.org/2501593003/diff/170001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java (right): https://codereview.chromium.org/2501593003/diff/170001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java:263: } On 2016/11/22 21:28:47, dfalcantara (check my queue) wrote: > Would it be clearer to: > > mAcceptedCardTypeInfos.clear(); > mAcceptedCardTypeInfos.addAll(mAcceptedCardTypeInfos, unique); > > Then you don't need to track write as a separate variable or manually remove > things at the end. > > Alternatively, if you're trying to only keep track of unique things, why not > switch to a Set instead of a List? Need to preserve the order in UI. If the merchant specifies that they support ["visa", "mastercard"], we want to show the Visa icon first, then the MasterCard icon second. Refactored to avoid this monstrosity.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/23 01:12:00, rouslan wrote: > Throw an exception only if > "basic-card" is the method name and its method-specific data does not have > correct format. Opened https://github.com/w3c/webpayments-methods-card/issues/20 to seek opinions from the WG.
On 2016/11/23 01:01:42, rouslan wrote: > Intent to ship sent out: > > https://groups.google.com/a/chromium.org/d/msg/blink-dev/IYRjdUKxCoM/8B-jp4g9... > > We can continue reviewing while waiting for approval. Worst case scenario is > placing this behind a runtime flag, right? Yep. That's how nearly all new substantial features ship in blink - intent to implement, land behind a status=experimental flag, iterate/polish, intent to ship, flip to status=stable. For simple / non-controversial features it's usually easier to collapse that all down to a single CL.
On 2016/11/22 23:31:59, haraken wrote:
> On 2016/11/22 22:35:17, Rick Byers wrote:
> > On 2016/11/22 21:42:46, esprehn wrote:
> > > On 2016/11/22 at 20:52:54, rouslan wrote:
> > > > On 2016/11/22 20:34:58, esprehn wrote:
> > > > > Why do you want to ignore this exceptions? That's very unusual and
> sounds
> > > like a
> > > > > spec bug. You want to reject the promise with the exception when one
> > occurs.
> > > >
> > > > ...
> > > >
> > > > For example, this code currently works for Autofill payment app:
> > > >
> > > > new PaymentRequest([{supportedMethods: ["basic-card"], data:
> > > {"supportedNetworks": ["visa"]}}], shoppingCart);
> > > >
> > > > This code will throw in V8BasicCardRequest::toImpl():
> > > >
> > > > new PaymentRequest([{supportedMethods: ["basic-card"], data:
> > > {"supportedNetworks": 0}}], shoppingCart);
> > > >
> > > > The behavior in my code is to not pass this data to Autofill payment app
> at
> > > all. However, another 3rd party app might be OK with this format of data.
So
> I
> > > would not want to reject this payment request. What do you think?
> > >
> > > That sounds incredibly hard for authors to use since the spec is too vague
> to
> > > actually implement and the author will need to reverse engineer the
> > requirements
> > > in chrome for what the structure of the data is required to be for Firefox
> vs
> > > Chrome vs Opera etc.
> > >
> > > I dont think we want this spec to just say "stuff happens in a third party
> > > payment app", it needs to be specific enough that an author can write code
> > that
> > > works in all browsers with confidence. Realistically what will happen is
> other
> > > browsers will be forced to reverse engineer our data formats to be
> compatible
> > > with the chrome authored content.
> >
> > Even though this CL doesn't change the visible API surface area, it is
> exposing
> > a new API to the web (otherwise there'd be no need for a spec or developer
> > documentation about how to use this). Blink has a policy
> > (https://www.chromium.org/blink#launch-process) of discussing all new APIs
on
> > blink-dev so that:
> > - Everyone interested is aware of the new APIs being shipped and has a
> > reasonable opportunity to provide feedback on the design before chromium is
> > stuck supporting it forever.
> > - There is an explicit discussion of the interop risk tradeoffs - the risk
> that
> > the feature won't ultimately work the same for web developers across all
> > browsers (such as Elliot is raising here)
> > - The blink project upholds it's commitment to the community
> > (https://blog.chromium.org/2013/04/blink-rendering-engine-for-chromium.html)
> to
> > be transparent about the changes we're introducing to the web platform, and
> the
> > tradeoffs that have gone into a decision that affects all web stakeholders.
> >
> > So IMHO this is not LGTM until we have an approved "intent to ship".
>
> Do we need an Intent-to-ship for features under status=experimental?
> PaymentRequest is under status=experimental.
I was confused by this too, added a comment to help clarify:
https://codereview.chromium.org/2512823002.
If that's still confusing then we can explore other ideas
("status=experimental,Android:stable" maybe).
> (I just want to clarify when we need to send an Intent-to-ship.)
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/v2/patch-status/codereview.chromium.or...
Patchset #5 (id:210001) has been deleted
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
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 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/v2/patch-status/codereview.chromium.or...
PTAL patch 5. Rick: I've placed the feature behind a runtime flag, so it should be OK to land before approval on blink-dev. Elliot: I'm not ignoring the parsing errors. Dan: I've simplified the code that removes duplicate icons from UI.
Java bits lgtm https://codereview.chromium.org/2501593003/diff/230001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentApp.java (right): https://codereview.chromium.org/2501593003/diff/230001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentApp.java:48: public void getInstruments(Map<String, PaymentMethodData> methodData, nit: I actually prefer keeping the arguments are together on the same line, if they fit. https://codereview.chromium.org/2501593003/diff/230001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java (right): https://codereview.chromium.org/2501593003/diff/230001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java:250: * times this method is called. indent 'times' under 'An'
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 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/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2501593003/diff/230001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentApp.java (right): https://codereview.chromium.org/2501593003/diff/230001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentApp.java:48: public void getInstruments(Map<String, PaymentMethodData> methodData, On 2016/11/28 21:58:29, dfalcantara (check my queue) wrote: > nit: I actually prefer keeping the arguments are together on the same line, if > they fit. Done. https://codereview.chromium.org/2501593003/diff/230001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java (right): https://codereview.chromium.org/2501593003/diff/230001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java:250: * times this method is called. On 2016/11/28 21:58:29, dfalcantara (check my queue) wrote: > indent 'times' under 'An' Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mek@chromium.org changed reviewers: + rbyers@chromium.org
https://codereview.chromium.org/2501593003/diff/250001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/2501593003/diff/250001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:545: if (exceptionState.hadException()) So after this change the PaymentRequest constructor will throw exceptions if invalid data is passed for the given supportedMethods? That doesn't seem to be something that's currently in the spec. If that's actually the desired behavior it's probably a good idea to actually get that written in the PaymentRequest constructor definition somewhere Insert a step that calls out to some "payment method specific validation steps" or something, where the basic card spec can then specify those steps? Not sure how to best phrase that. Also not clear to me what the desired behavior is with multiple payment methods in one PaymentMethodData. Should the data be valid for all specified methods? Or for at least one? There seem to be many possibly valid choices here, and without actually having this written down somewhere it seems likely that browsers might make different choices. At least the previous behavior of ignoring exceptions when the data doesn't match the methods matched what is currently specified.
On 2016/11/29 21:09:53, Marijn Kruisselbrink wrote: > https://codereview.chromium.org/2501593003/diff/250001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): > > https://codereview.chromium.org/2501593003/diff/250001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:545: if > (exceptionState.hadException()) > So after this change the PaymentRequest constructor will throw exceptions if > invalid data is passed for the given supportedMethods? That doesn't seem to be > something that's currently in the spec. If that's actually the desired behavior > it's probably a good idea to actually get that written in the PaymentRequest > constructor definition somewhere > Insert a step that calls out to some "payment method specific validation steps" > or something, where the basic card spec can then specify those steps? Not sure > how to best phrase that. Also not clear to me what the desired behavior is with > multiple payment methods in one PaymentMethodData. Should the data be valid for > all specified methods? Or for at least one? There seem to be many possibly valid > choices here, and without actually having this written down somewhere it seems > likely that browsers might make different choices. > > At least the previous behavior of ignoring exceptions when the data doesn't > match the methods matched what is currently specified. I've opened https://github.com/w3c/webpayments-methods-card/issues/20 to seek opinions from the WG and placed this on agenda for our Thursday meeting.
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
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/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.
On 2016/11/29 21:43:56, rouslan-intermittent-holidays wrote: > I've opened https://github.com/w3c/webpayments-methods-card/issues/20 to seek > opinions from the WG and placed this on agenda for our Thursday meeting. The conclusion was to not rethrow exceptions when parsing method-specific data in the renderer. PTAL.
On 2016/11/28 18:58:11, rouslan-intermittent-holidays wrote: > PTAL patch 5. > > Rick: I've placed the feature behind a runtime flag, so it should be OK to land > before approval on blink-dev. Sorry I missed this earlier. LGTM to reverse my previous objection. Thank you! I also looked over third_party/WebKit: OWNERS LGTM stamp for that. > Elliot: I'm not ignoring the parsing errors. > > Dan: I've simplified the code that removes duplicate icons from UI.
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/v2/patch-status/codereview.chromium.or...
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/v2/patch-status/codereview.chromium.or...
Elliot and Marijn: PTAL patch 10. I'm not rethrowing exception state when parsing method-specific data in the renderer, per resolution of https://github.com/w3c/webpayments-methods-card/issues/20.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
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/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2501593003/diff/350001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/2501593003/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:393: return; is return here really the right thing? I'm a bit confused about how the API allows multiple payment methods with one set of data, but as it stands now supportedMethods: ["https://android.com/pay", "basic-card"] with data that is valid basic card data but not android pay data will fail, yet the same supportedMethods with data that is valid android pay data (but not basic card data) will possibly work?
The CQ bit was unchecked by commit-bot@chromium.org
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...)
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/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2501593003/diff/350001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/2501593003/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:393: return; On 2016/12/22 21:58:20, Marijn Kruisselbrink wrote: > is return here really the right thing? I'm a bit confused about how the API > allows multiple payment methods with one set of data, but as it stands now > supportedMethods: ["https://android.com/pay", "basic-card"] with data that is > valid basic card data but not android pay data will fail, yet the same > supportedMethods with data that is valid android pay data (but not basic card > data) will possibly work? Doh! I'm dumb. Fixed in patch 12.
The CQ bit was unchecked by commit-bot@chromium.org
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...)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
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/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 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/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.
Marijn & Elliot, ptal patch 16.
lgtm https://codereview.chromium.org/2501593003/diff/450001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/2501593003/diff/450001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:391: if (exceptionState.hadException()) I recently came across Source/bindings/core/v8/ExceptionStatePlaceholder.h which has an IGNORE_EXCEPTION thing, which you could use rather than passing the real ExceptionState and clearing all exceptions that are thrown. But not sure if that is any better (or worse) than what people used to use DummyExceptionStateForTesting for...
https://codereview.chromium.org/2501593003/diff/450001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/2501593003/diff/450001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:391: if (exceptionState.hadException()) On 2017/01/05 22:38:03, Marijn Kruisselbrink wrote: > I recently came across Source/bindings/core/v8/ExceptionStatePlaceholder.h which > has an IGNORE_EXCEPTION thing, which you could use rather than passing the real > ExceptionState and clearing all exceptions that are thrown. But not sure if that > is any better (or worse) than what people used to use > DummyExceptionStateForTesting for... I intentionally removed DummyExceptionStateForTesting because people have abused it. ExceptionState must not be ignored unless you have a strong reason to do so. Would you elaborate on why you want to ignore an exception here?
https://codereview.chromium.org/2501593003/diff/450001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/2501593003/diff/450001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:391: if (exceptionState.hadException()) On 2017/01/06 at 00:04:17, haraken wrote: > On 2017/01/05 22:38:03, Marijn Kruisselbrink wrote: > > I recently came across Source/bindings/core/v8/ExceptionStatePlaceholder.h which > > has an IGNORE_EXCEPTION thing, which you could use rather than passing the real > > ExceptionState and clearing all exceptions that are thrown. But not sure if that > > is any better (or worse) than what people used to use > > DummyExceptionStateForTesting for... > > I intentionally removed DummyExceptionStateForTesting because people have abused it. ExceptionState must not be ignored unless you have a strong reason to do so. > > Would you elaborate on why you want to ignore an exception here? I think the reason why exceptions are being ignored here has already been sufficiently discussed earlier in this CL/thread? My question was more that if using TrackExceptionState/DummyExceptionStateForTesting (which seems to be mostly used by production code to transform an exception into some other way of reporting the error) is bad because it can be used to ignore exceptions, why isn't using IGNORE_EXCEPTION just as bad? Should IGNORE_EXCEPTION also be renamed/changed/discouraged? It's used almost a hundred times, almost none of those in test code.
On 2017/01/06 00:16:07, Marijn Kruisselbrink wrote: > https://codereview.chromium.org/2501593003/diff/450001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): > > https://codereview.chromium.org/2501593003/diff/450001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:391: if > (exceptionState.hadException()) > On 2017/01/06 at 00:04:17, haraken wrote: > > On 2017/01/05 22:38:03, Marijn Kruisselbrink wrote: > > > I recently came across Source/bindings/core/v8/ExceptionStatePlaceholder.h > which > > > has an IGNORE_EXCEPTION thing, which you could use rather than passing the > real > > > ExceptionState and clearing all exceptions that are thrown. But not sure if > that > > > is any better (or worse) than what people used to use > > > DummyExceptionStateForTesting for... > > > > I intentionally removed DummyExceptionStateForTesting because people have > abused it. ExceptionState must not be ignored unless you have a strong reason to > do so. > > > > Would you elaborate on why you want to ignore an exception here? > > > I think the reason why exceptions are being ignored here has already been > sufficiently discussed earlier in this CL/thread? Sorry, I forgot we've discussed that :/ Yeah, looking at the discussion, it makes sense to not rethrow an exception in this case. Can we add a comment about it? > My question was more that if using > TrackExceptionState/DummyExceptionStateForTesting (which seems to be mostly used > by production code to transform an exception into some other way of reporting > the error) is bad because it can be used to ignore exceptions, why isn't using > IGNORE_EXCEPTION just as bad? Should IGNORE_EXCEPTION also be > renamed/changed/discouraged? It's used almost a hundred times, almost none of > those in test code. Yeah, I want to limit IGNORE_EXCEPTION to testing code as well. At a first glance, I can easily find a bunch of IGNORE_EXCEPTIONs that should be replaced with non-ignorable ExceptionState...
LGTM % adding a comment why you want to clear the exception.
The CQ bit was checked by rouslan@chromium.org to run a CQ dry run
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/v2/patch-status/codereview.chromium.or...
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/v2/patch-status/codereview.chromium.or...
Added a comment about the reason for clearing an exception and removed the last remaining usage of DummyExceptionStateForTesting in PaymentRequest.cpp.
The CQ bit was unchecked by rouslan@chromium.org
The CQ bit was checked by rouslan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, dfalcantara@chromium.org, rbyers@chromium.org, haraken@chromium.org, mek@chromium.org Link to the patchset: https://codereview.chromium.org/2501593003/#ps490001 (title: "Remove last remaining DummyExceptionStateForTesting")
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
Failed to apply patch for
chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java:
While running git apply --index -p1;
error: patch failed:
chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java:23
error:
chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java:
patch does not apply
Patch:
chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java
Index:
chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java
diff --git
a/chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java
b/chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java
index
5f6d3b4e59ed26508d95f34e85aa963de0d473e5..2ecf677116dcc690fbe1263e5b7037992cd4b621
100644
---
a/chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java
+++
b/chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java
@@ -23,6 +23,7 @@ import org.chromium.chrome.browser.payments.ui.EditorModel;
import
org.chromium.chrome.browser.preferences.autofill.AutofillProfileBridge.DropdownKeyValue;
import org.chromium.content.browser.ContentViewCore;
import org.chromium.content_public.browser.WebContents;
+import org.chromium.payments.mojom.PaymentMethodData;
import java.text.SimpleDateFormat;
import java.util.ArrayList;
@@ -113,6 +114,13 @@ public class CardEditor extends
EditorBase<AutofillPaymentInstrument>
private final Set<String> mAcceptedCardTypes;
/**
+ * The card types accepted by the merchant website that should have
"basic-card" as the payment
+ * method. This is a subset of the accepted card types. Used when creating
the complete payment
+ * instrument.
+ */
+ private final Set<String> mAcceptedBasicCardTypes;
+
+ /**
* The information about the accepted card types. Used in the editor as a
hint to the user about
* the valid card types. This is important to keep in a list, because the
display order matters.
*/
@@ -185,6 +193,7 @@ public class CardEditor extends
EditorBase<AutofillPaymentInstrument>
mCardTypes.put(VISA, new CardTypeInfo(R.drawable.pr_visa,
R.string.autofill_cc_visa));
mAcceptedCardTypes = new HashSet<>();
+ mAcceptedBasicCardTypes = new HashSet<>();
mAcceptedCardTypeInfos = new ArrayList<>();
mHandler = new Handler();
@@ -253,21 +262,40 @@ public class CardEditor extends
EditorBase<AutofillPaymentInstrument>
/**
* Adds accepted payment methods to the editor, if they are recognized
credit card types.
*
- * @param acceptedMethods The accepted method payments.
+ * @param data Supported methods and method specific data. Should not be
null.
*/
- public void addAcceptedPaymentMethodsIfRecognized(String[] acceptedMethods)
{
- assert acceptedMethods != null;
- for (int i = 0; i < acceptedMethods.length; i++) {
- String method = acceptedMethods[i];
+ public void addAcceptedPaymentMethodsIfRecognized(PaymentMethodData data) {
+ assert data != null;
+ for (int i = 0; i < data.supportedMethods.length; i++) {
+ String method = data.supportedMethods[i];
if (mCardTypes.containsKey(method)) {
- assert !mAcceptedCardTypes.contains(method);
- mAcceptedCardTypes.add(method);
- mAcceptedCardTypeInfos.add(mCardTypes.get(method));
+ addAcceptedNetwork(method);
+ } else if
(AutofillPaymentApp.BASIC_CARD_METHOD_NAME.equals(method)) {
+ Set<String> basicCardNetworks =
AutofillPaymentApp.convertBasicCardToNetworks(data);
+ if (basicCardNetworks != null) {
+ mAcceptedBasicCardTypes.addAll(basicCardNetworks);
+ for (String network : basicCardNetworks) {
+ addAcceptedNetwork(network);
+ }
+ }
}
}
}
/**
+ * Adds a card network to the list of accepted networks.
+ *
+ * @param network An accepted network. Will be shown in UI only once,
regardless of how many
+ * times this method is called.
+ */
+ private void addAcceptedNetwork(String network) {
+ if (!mAcceptedCardTypes.contains(network)) {
+ mAcceptedCardTypes.add(network);
+ mAcceptedCardTypeInfos.add(mCardTypes.get(network));
+ }
+ }
+
+ /**
* Builds and shows an editor model with the following fields for local
cards.
*
* [ accepted card types hint images ]
@@ -292,7 +320,8 @@ public class CardEditor extends
EditorBase<AutofillPaymentInstrument>
// Ensure that |instrument| and |card| are never null.
final AutofillPaymentInstrument instrument = isNewCard
- ? new AutofillPaymentInstrument(mContext, mWebContents, new
CreditCard(), null)
+ ? new AutofillPaymentInstrument(mContext, mWebContents, new
CreditCard(),
+ null /* billingAddress */, null /* methodName */)
: toEdit;
final CreditCard card = instrument.getCard();
@@ -345,13 +374,24 @@ public class CardEditor extends
EditorBase<AutofillPaymentInstrument>
@Override
public void run() {
commitChanges(card, isNewCard);
+
+ String methodName = card.getBasicCardPaymentType();
+ if (mAcceptedBasicCardTypes.contains(methodName)) {
+ methodName = AutofillPaymentApp.BASIC_CARD_METHOD_NAME;
+ }
+ assert methodName != null;
+
+ AutofillProfile billingAddress = null;
for (int i = 0; i < mProfilesForBillingAddress.size(); ++i) {
if
(TextUtils.equals(mProfilesForBillingAddress.get(i).getGUID(),
card.getBillingAddressId())) {
- instrument.completeInstrument(card,
mProfilesForBillingAddress.get(i));
+ billingAddress = mProfilesForBillingAddress.get(i);
break;
}
}
+ assert billingAddress != null;
+
+ instrument.completeInstrument(card, methodName,
billingAddress);
callback.onResult(instrument);
}
});
The CQ bit was checked by rouslan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rbyers@chromium.org, haraken@chromium.org, tsepez@chromium.org, mek@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2501593003/#ps510001 (title: "Rebase")
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": 510001, "attempt_start_ts": 1483975546202330,
"parent_rev": "d865ea9845bba99cbab0838fa309f4d4fbd4ca39", "commit_rev":
"b1be35c4c222d904ae02ea5c5dfdf1972d031f1e"}
Message was sent while issue was closed.
Description was changed from
==========
Implement the new basic card specification.
The basic card specification has been updated to include card types
(credit, debit, prepaid), which are distinct from card networks (amex,
discover, mastercard, visa, etc). This patch adds support for the new
format without removing support for the old format. Parsing of the JSON
data that encodes the parameters happens in the renderer for safety.
Old format (still supported):
new PaymentRequest([{
supportedMethods: ['visa']
}],
shoppingCartContents);
New format (in this patch):
new PaymentRequest([{
supportedMethods: ['basic-card'],
data: {
supportedNetworks: ['visa'],
supportedTypes: ['debit']
}
}],
shoppingCartContents);
Basic card spec:
https://w3c.github.io/webpayments-methods-card/
Intent to implement and ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/IYRjdUKxCoM/8B-jp4g9...
BUG=665190
==========
to
==========
Implement the new basic card specification.
The basic card specification has been updated to include card types
(credit, debit, prepaid), which are distinct from card networks (amex,
discover, mastercard, visa, etc). This patch adds support for the new
format without removing support for the old format. Parsing of the JSON
data that encodes the parameters happens in the renderer for safety.
Old format (still supported):
new PaymentRequest([{
supportedMethods: ['visa']
}],
shoppingCartContents);
New format (in this patch):
new PaymentRequest([{
supportedMethods: ['basic-card'],
data: {
supportedNetworks: ['visa'],
supportedTypes: ['debit']
}
}],
shoppingCartContents);
Basic card spec:
https://w3c.github.io/webpayments-methods-card/
Intent to implement and ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/IYRjdUKxCoM/8B-jp4g9...
BUG=665190
Review-Url: https://codereview.chromium.org/2501593003
Cr-Commit-Position: refs/heads/master@{#442281}
Committed:
https://chromium.googlesource.com/chromium/src/+/b1be35c4c222d904ae02ea5c5dfd...
==========
Message was sent while issue was closed.
Committed patchset #19 (id:510001) as https://chromium.googlesource.com/chromium/src/+/b1be35c4c222d904ae02ea5c5dfd... |
