Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2020883002:
PaymentRequest: Introduce PaymentMethodData. (Closed)
|
Created:
4 years, 6 months ago by zino Modified:
4 years, 6 months ago CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, darin (slow to review), dglazkov+blink, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPaymentRequest: Introduce PaymentMethodData.
This CL is introducing PaymentMethodData to combine the supported methods and
payment method specific data. By making this change it is possible to share an
instance of data amongst multiple payment methods that wasn't possible before.
Related spec links:
[1] https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#idl-def-paymentmethoddata
[2] https://github.com/w3c/browser-payment-api/pull/162/commits/6d9c92e5d2bc82e4900823e9fa915a5be9584f6b
BUG=none
Committed: https://crrev.com/c6d2d4581607e7642f2f85a386cf3ace7aacf803
Cr-Commit-Position: refs/heads/master@{#398954}
Patch Set 1 : #Patch Set 2 : #
Total comments: 35
Patch Set 3 : #
Total comments: 8
Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #
Total comments: 4
Patch Set 7 : #
Total comments: 6
Patch Set 8 : rebase #
Total comments: 3
Patch Set 9 : #Patch Set 10 : #Messages
Total messages: 73 (35 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== introduce PaymentMethodData BUG= ========== to ========== PaymentRequest: Introduce PaymentMethodData. This CL is introducing PaymentMethodData to combine the supported methods and payment method specific data. By making this change it is possible to share an instance of data amongst multiple payment methods that wasn't possible before. Related spec links: [1] https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#idl-def-p... [2] https://github.com/w3c/browser-payment-api/pull/162/commits/6d9c92e5d2bc82e49... BUG=none ==========
Patchset #2 (id:80001) has been deleted
jinho.bang@samsung.com changed reviewers: + rouslan@chromium.org
Please take a look. Thank you.
rouslan@chromium.org changed reviewers: + mek@chromium.org
Thank you so much for your help in this project! +mek@ for Blink expertise. Please see my questions to you inline. https://codereview.chromium.org/2020883002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2020883002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:261: private HashMap<String, JSONObject> getValidatedMethodData(PaymentMethodData[] methodData) { Check that methodData is not null and not empty. https://codereview.chromium.org/2020883002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:264: String[] methods = methodData[i].supportedMethods; Move |methods| down to line 278-ish, where you're going to use it. https://codereview.chromium.org/2020883002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:268: data = new JSONObject(); Let's use |null| to represent empty data. Object creation is expensive in Android. https://codereview.chromium.org/2020883002/diff/100001/chrome/test/data/andro... File chrome/test/data/android/payments/no_shipping.js (right): https://codereview.chromium.org/2020883002/diff/100001/chrome/test/data/andro... chrome/test/data/android/payments/no_shipping.js:1: /* If this lands after http://crrev.com/2018203002, then you also need to modify dynamic_shipping.js. https://codereview.chromium.org/2020883002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/payments/payment-request-interface.html (right): https://codereview.chromium.org/2020883002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/payments/payment-request-interface.html:142: new PaymentRequest([{'supportedMethods': ['foo'], data : [] }], buildDetails(), {}) No space between [] and } . Here and everywhere: No space between data and : . Here and everywhere: All dictionary keys are quoted in this file, so let's use 'data' instead of data. Here and everywhere: There's no longer a need for the 3rd parameter {}. You can remove it. The 3rd parameter was there because I was testing the 4th parameter. Now that 4th parameter is gone, the 3rd optional parameter can be omitted. https://codereview.chromium.org/2020883002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/payments/payment-request-interface.html:145: new PaymentRequest([{'supportedMethods': ['foo'], data : 'foo' }], buildDetails(), {}) No space between 'foo' and }. https://codereview.chromium.org/2020883002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/payments/payment-request-interface.html:148: new PaymentRequest([{'supportedMethods': ['foo'], data : 42 }], buildDetails(), {}) No space between 42 and }. https://codereview.chromium.org/2020883002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/payments/payment-request-interface.html:153: new PaymentRequest([{'supportedMethods': ['foo'], data : infiniteData }], buildDetails(), {}) No space between infiniteData and }. https://codereview.chromium.org/2020883002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/payments/payment-request-interface.html:153: new PaymentRequest([{'supportedMethods': ['foo'], data : infiniteData }], buildDetails(), {}) Add tests for * 'data': null * 'data': undefined I believe these should throw. mek@: Do you think these should throw? https://codereview.chromium.org/2020883002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/2020883002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:43: using blink::mojom::blink::PaymentMethodDataPtr; Sort alphabetically. https://codereview.chromium.org/2020883002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:108: output->supported_methods = Vector<WTF::String>(input.supportedMethods()); output->supported_methods = mojom::WTFArray<String>::From(input.supportedMethods()); https://codereview.chromium.org/2020883002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:109: if (!input.data().isEmpty()) if (input.hasData() && !input.data().isEmpty()) https://codereview.chromium.org/2020883002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:335: if (methodData.isEmpty()) { This block should be inside of validatePaymentMethodData. https://codereview.chromium.org/2020883002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequest.h (right): https://codereview.chromium.org/2020883002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequest.h:87: HeapVector<PaymentMethodData> m_methodData; mek@: Does storing PaymentMethodData (which includes a "data" object) on stack like this pose any problems for garbage collection? We used to convert the 4th optional parameter |data| from ScriptValue to String for this exact purpose. Perhaps |m_methodData| should be a HeapVector<Method>, where Method is: struct Method { HeapVector<String> names; String stringifiedData; }; (Method may need to be garbage collected.) https://codereview.chromium.org/2020883002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequestDetailsTest.cpp (right): https://codereview.chromium.org/2020883002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestDetailsTest.cpp:80: HeapVector<PaymentMethodData> buildPaymentMethodDataForTest() Rename PaymentDetailsTestHelper into PaymentTestHelper and put this function in there, so you can re-use it in multiple files. https://codereview.chromium.org/2020883002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestDetailsTest.cpp:83: methodData[0].setSupportedMethods(Vector<String>(1, "food")); "foo" ? https://codereview.chromium.org/2020883002/diff/100001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/payments/payment_request.mojom (right): https://codereview.chromium.org/2020883002/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/payments/payment_request.mojom:100: // {"https://android.com/pay": {"gateway": "stripe"}} // {"gateway": "stripe"}
Thank you for review. Could you review this again? https://codereview.chromium.org/2020883002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2020883002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:261: private HashMap<String, JSONObject> getValidatedMethodData(PaymentMethodData[] methodData) { On 2016/05/29 19:57:11, Rouslan (ツ) wrote: > Check that methodData is not null and not empty. Done. https://codereview.chromium.org/2020883002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:264: String[] methods = methodData[i].supportedMethods; On 2016/05/29 19:57:11, Rouslan (ツ) wrote: > Move |methods| down to line 278-ish, where you're going to use it. Done. https://codereview.chromium.org/2020883002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:268: data = new JSONObject(); On 2016/05/29 19:57:11, Rouslan (ツ) wrote: > Let's use |null| to represent empty data. Object creation is expensive in > Android. Done. https://codereview.chromium.org/2020883002/diff/100001/chrome/test/data/andro... File chrome/test/data/android/payments/no_shipping.js (right): https://codereview.chromium.org/2020883002/diff/100001/chrome/test/data/andro... chrome/test/data/android/payments/no_shipping.js:1: /* On 2016/05/29 19:57:11, Rouslan (ツ) wrote: > If this lands after http://crrev.com/2018203002, then you also need to modify > dynamic_shipping.js. Done. https://codereview.chromium.org/2020883002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/payments/payment-request-interface.html (right): https://codereview.chromium.org/2020883002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/payments/payment-request-interface.html:142: new PaymentRequest([{'supportedMethods': ['foo'], data : [] }], buildDetails(), {}) On 2016/05/29 19:57:12, Rouslan (ツ) wrote: > No space between [] and } . > > Here and everywhere: No space between data and : . > > Here and everywhere: All dictionary keys are quoted in this file, so let's use > 'data' instead of data. > > Here and everywhere: There's no longer a need for the 3rd parameter {}. You can > remove it. The 3rd parameter was there because I was testing the 4th parameter. > Now that 4th parameter is gone, the 3rd optional parameter can be omitted. Done. https://codereview.chromium.org/2020883002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/payments/payment-request-interface.html:145: new PaymentRequest([{'supportedMethods': ['foo'], data : 'foo' }], buildDetails(), {}) On 2016/05/29 19:57:12, Rouslan (ツ) wrote: > No space between 'foo' and }. Done. https://codereview.chromium.org/2020883002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/payments/payment-request-interface.html:148: new PaymentRequest([{'supportedMethods': ['foo'], data : 42 }], buildDetails(), {}) On 2016/05/29 19:57:12, Rouslan (ツ) wrote: > No space between 42 and }. Done. https://codereview.chromium.org/2020883002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/payments/payment-request-interface.html:153: new PaymentRequest([{'supportedMethods': ['foo'], data : infiniteData }], buildDetails(), {}) On 2016/05/29 19:57:11, Rouslan (ツ) wrote: > No space between infiniteData and }. Done. I added null case only. The 'data' is not required field and doesn't have default value. So, passing undefined is just undefined. [1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... [2] http://heycam.github.io/webidl/#es-to-dictionary https://codereview.chromium.org/2020883002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/2020883002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:43: using blink::mojom::blink::PaymentMethodDataPtr; On 2016/05/29 19:57:12, Rouslan (ツ) wrote: > Sort alphabetically. Done. https://codereview.chromium.org/2020883002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:108: output->supported_methods = Vector<WTF::String>(input.supportedMethods()); On 2016/05/29 19:57:12, Rouslan (ツ) wrote: > output->supported_methods = > mojom::WTFArray<String>::From(input.supportedMethods()); I'm not sure but it's better to avoid unnecessary copy. We can just assign the WTF::Vector(=RValue) to WTFArray according to this[1]. [1] https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/cpp/bi... Also, if using mojo::WTFArray<WTF::String>::From(input.supportedMethods()), build error will happen now because we don't have TypeConverter<WTFArray, Vector>. https://codereview.chromium.org/2020883002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:109: if (!input.data().isEmpty()) On 2016/05/29 19:57:12, Rouslan (ツ) wrote: > if (input.hasData() && !input.data().isEmpty()) Done. https://codereview.chromium.org/2020883002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:335: if (methodData.isEmpty()) { On 2016/05/29 19:57:12, Rouslan (ツ) wrote: > This block should be inside of validatePaymentMethodData. Done. https://codereview.chromium.org/2020883002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequest.h (right): https://codereview.chromium.org/2020883002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequest.h:87: HeapVector<PaymentMethodData> m_methodData; On 2016/05/29 19:57:12, Rouslan (ツ) wrote: > mek@: Does storing PaymentMethodData (which includes a "data" object) on stack > like this pose any problems for garbage collection? We used to convert the 4th > optional parameter |data| from ScriptValue to String for this exact purpose. > Perhaps |m_methodData| should be a HeapVector<Method>, where Method is: > > struct Method { > HeapVector<String> names; > String stringifiedData; > }; > > (Method may need to be garbage collected.) I found some existing cases. - NFCMessage (NFCRecord has ScriptValue.) [1] - InternalDictionary [2] [1] https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blin... [2] https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blin... But I'm not sure. https://codereview.chromium.org/2020883002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequestDetailsTest.cpp (right): https://codereview.chromium.org/2020883002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestDetailsTest.cpp:80: HeapVector<PaymentMethodData> buildPaymentMethodDataForTest() On 2016/05/29 19:57:12, Rouslan (ツ) wrote: > Rename PaymentDetailsTestHelper into PaymentTestHelper and put this function in > there, so you can re-use it in multiple files. Done. https://codereview.chromium.org/2020883002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequestDetailsTest.cpp:83: methodData[0].setSupportedMethods(Vector<String>(1, "food")); On 2016/05/29 19:57:12, Rouslan (ツ) wrote: > "foo" ? Done. https://codereview.chromium.org/2020883002/diff/100001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/payments/payment_request.mojom (right): https://codereview.chromium.org/2020883002/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/payments/payment_request.mojom:100: // {"https://android.com/pay": {"gateway": "stripe"}} On 2016/05/29 19:57:12, Rouslan (ツ) wrote: > // {"gateway": "stripe"} Done.
https://codereview.chromium.org/2020883002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/payments/payment-request-interface.html (right): https://codereview.chromium.org/2020883002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/payments/payment-request-interface.html:153: new PaymentRequest([{'supportedMethods': ['foo'], data : infiniteData }], buildDetails(), {}) Yeah, throwing for null while treating undefined the same as the attribute not being present seems to be what the specs require. https://codereview.chromium.org/2020883002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequest.h (right): https://codereview.chromium.org/2020883002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequest.h:87: HeapVector<PaymentMethodData> m_methodData; On 2016/06/01 at 17:43:46, zino wrote: > On 2016/05/29 19:57:12, Rouslan (ツ) wrote: > > mek@: Does storing PaymentMethodData (which includes a "data" object) on stack > > like this pose any problems for garbage collection? We used to convert the 4th > > optional parameter |data| from ScriptValue to String for this exact purpose. > > Perhaps |m_methodData| should be a HeapVector<Method>, where Method is: > > > > struct Method { > > HeapVector<String> names; > > String stringifiedData; > > }; > > > > (Method may need to be garbage collected.) > > I found some existing cases. > - NFCMessage (NFCRecord has ScriptValue.) [1] > - InternalDictionary [2] The problem isn't so much that there are dictionary types with object/ScriptValue members, but rather that references to those ScriptValues are stored in other oilpan garbage collected objects. At least that's my understanding of the potential issue. But I don't understand the exact issues very well either. So probably it's best to just leave it like this for now and double check with somebody from the bindings team (maybe haraken@) to see if storing ScriptValue's like this is problematic or not. https://codereview.chromium.org/2020883002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/2020883002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:104: struct TypeConverter<PaymentMethodDataPtr, blink::PaymentMethodData> { I believe these days typemaps and StructTraits are preferred over TypeConverters (https://www.chromium.org/developers/design-documents/mojo/type-mapping). That way you could just pass blink::PaymentMethodData instances directly to/from the mojom generated blink bindings. But other than for urls and origins there aren't really any blink specific typemaps in the code-base yet. Also depending on what happens with PaymentMethodData and the ScriptValue thing the typemaps would end up being a bit different... And maybe it makes more sense to just keep this as a TypeConverter for now, and separately clean up all these TypeConverters to be StructTraits later.
rouslan@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/2020883002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/payments/PaymentRequest.h:87: > HeapVector<PaymentMethodData> m_methodData; > On 2016/06/01 at 17:43:46, zino wrote: > > On 2016/05/29 19:57:12, Rouslan (ツ) wrote: > > > mek@: Does storing PaymentMethodData (which includes a "data" object) on > stack > > > like this pose any problems for garbage collection? We used to convert the > 4th > > > optional parameter |data| from ScriptValue to String for this exact purpose. > > > Perhaps |m_methodData| should be a HeapVector<Method>, where Method is: > > > > > > struct Method { > > > HeapVector<String> names; > > > String stringifiedData; > > > }; > > > > > > (Method may need to be garbage collected.) > > > > I found some existing cases. > > - NFCMessage (NFCRecord has ScriptValue.) [1] > > - InternalDictionary [2] > > The problem isn't so much that there are dictionary types with > object/ScriptValue members, but rather that references to those ScriptValues are > stored in other oilpan garbage collected objects. At least that's my > understanding of the potential issue. But I don't understand the exact issues > very well either. So probably it's best to just leave it like this for now and > double check with somebody from the bindings team (maybe haraken@) to see if > storing ScriptValue's like this is problematic or not. haraken@: Is storing ScriptValue problematic? https://codereview.chromium.org/2020883002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/payments/payment-request-interface.html (right): https://codereview.chromium.org/2020883002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/payments/payment-request-interface.html:155: ['undefined for payment method specific data parameter should throw', null, function() { s/undefined/Undefined https://codereview.chromium.org/2020883002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/payments/payment-request-interface.html:158: ['null for payment method specific data parameter should throw', null, function() { s/null/Null https://codereview.chromium.org/2020883002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/payments/payment-request-interface.html:161: ['empty string for payment method specific data parameter should throw', null, function() { s/empty/Empty https://codereview.chromium.org/2020883002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/payments/payment-request-interface.html:162: new PaymentRequest([{'supportedMethods': ['foo'], 'data': null}], buildDetails()) s/null/'' https://codereview.chromium.org/2020883002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentDetailsTestHelper.h (left): https://codereview.chromium.org/2020883002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentDetailsTestHelper.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. You forgot to add the PaymentTestHelper.h and PaymentTestHelper.cpp to your patch. https://codereview.chromium.org/2020883002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/2020883002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:104: struct TypeConverter<PaymentMethodDataPtr, blink::PaymentMethodData> { On 2016/06/01 18:20:24, Marijn Kruisselbrink wrote: > I believe these days typemaps and StructTraits are preferred over TypeConverters > (https://www.chromium.org/developers/design-documents/mojo/type-mapping). That > way you could just pass blink::PaymentMethodData instances directly to/from the > mojom generated blink bindings. But other than for urls and origins there aren't > really any blink specific typemaps in the code-base yet. Also depending on what > happens with PaymentMethodData and the ScriptValue thing the typemaps would end > up being a bit different... > > And maybe it makes more sense to just keep this as a TypeConverter for now, and > separately clean up all these TypeConverters to be StructTraits later. Let's make TypeConverters into typemaps or StrucTraits all in one go. I have this on my schedule. https://codereview.chromium.org/2020883002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:346: // https://github.com/w3c/browser-payment-api/issues/2 Bad merge? This comment should be gone now.
On 2016/06/01 19:20:21, Rouslan (ツ) wrote: > https://codereview.chromium.org/2020883002/diff/100001/third_party/WebKit/Sou... > > third_party/WebKit/Source/modules/payments/PaymentRequest.h:87: > > HeapVector<PaymentMethodData> m_methodData; > > On 2016/06/01 at 17:43:46, zino wrote: > > > On 2016/05/29 19:57:12, Rouslan (ツ) wrote: > > > > mek@: Does storing PaymentMethodData (which includes a "data" object) on > > stack > > > > like this pose any problems for garbage collection? We used to convert the > > 4th > > > > optional parameter |data| from ScriptValue to String for this exact > purpose. > > > > Perhaps |m_methodData| should be a HeapVector<Method>, where Method is: > > > > > > > > struct Method { > > > > HeapVector<String> names; > > > > String stringifiedData; > > > > }; > > > > > > > > (Method may need to be garbage collected.) > > > > > > I found some existing cases. > > > - NFCMessage (NFCRecord has ScriptValue.) [1] > > > - InternalDictionary [2] > > > > The problem isn't so much that there are dictionary types with > > object/ScriptValue members, but rather that references to those ScriptValues > are > > stored in other oilpan garbage collected objects. At least that's my > > understanding of the potential issue. But I don't understand the exact issues > > very well either. So probably it's best to just leave it like this for now and > > double check with somebody from the bindings team (maybe haraken@) to see if > > storing ScriptValue's like this is problematic or not. > > haraken@: Is storing ScriptValue problematic? If ScriptValue is used only on stack, there's no problem. If you store ScriptValue on some object, there's no problem in terms of Oilpan but you really need to be careful if the ScriptValue doesn't cause a reference cycle that crosses the V8/DOM boundary. Remember that ScriptValue has a strong reference to V8. In common cases, it creates a reference cycle. I suspect NFCRecord is hitting the issue... If you want to keep some V8 object alive from a DOM object, there are multiple (safer) ways to do that. [SetWrapperReferenceTo/From], [Custom=VisitDOMWrapper] etc. I'm happy to explain if you need them.
On 2016/06/02 01:19:46, haraken wrote: > If ScriptValue is used only on stack, there's no problem. > > If you store ScriptValue on some object, there's no problem in terms of Oilpan > but you really need to be careful if the ScriptValue doesn't cause a reference > cycle that crosses the V8/DOM boundary. Remember that ScriptValue has a strong > reference to V8. In common cases, it creates a reference cycle. I suspect > NFCRecord is hitting the issue... > > If you want to keep some V8 object alive from a DOM object, there are multiple > (safer) ways to do that. [SetWrapperReferenceTo/From], [Custom=VisitDOMWrapper] > etc. I'm happy to explain if you need them. haraken@: We want to keep a HeapVector<PaymentMethodData> as a member variable of PaymentRequest [1]. PaymentMethodData.data is an object [2], represented as ScriptValue in C++. Do we need to worry about a reference cycle here? [1] https://codereview.chromium.org/2020883002/diff/140001/third_party/WebKit/Sou... [2] https://codereview.chromium.org/2020883002/diff/140001/third_party/WebKit/Sou...
On 2016/06/02 02:28:20, Rouslan (ツ) wrote: > On 2016/06/02 01:19:46, haraken wrote: > > If ScriptValue is used only on stack, there's no problem. > > > > If you store ScriptValue on some object, there's no problem in terms of Oilpan > > but you really need to be careful if the ScriptValue doesn't cause a reference > > cycle that crosses the V8/DOM boundary. Remember that ScriptValue has a strong > > reference to V8. In common cases, it creates a reference cycle. I suspect > > NFCRecord is hitting the issue... > > > > If you want to keep some V8 object alive from a DOM object, there are multiple > > (safer) ways to do that. [SetWrapperReferenceTo/From], > [Custom=VisitDOMWrapper] > > etc. I'm happy to explain if you need them. > > haraken@: We want to keep a HeapVector<PaymentMethodData> as a member variable > of PaymentRequest [1]. PaymentMethodData.data is an object [2], represented as > ScriptValue in C++. Do we need to worry about a reference cycle here? > > [1] > https://codereview.chromium.org/2020883002/diff/140001/third_party/WebKit/Sou... > [2] > https://codereview.chromium.org/2020883002/diff/140001/third_party/WebKit/Sou... Yes, it will cause leaks. PaymentRequest's wrapper ==(Oilpan's Persistent)=> PaymentRequest ==(Oilpan's Member)=> PaymentMethodData ==(ScriptValue i.e., V8's Persistent)=> any arbitrary V8 object ==(internal strong reference)=> window object ==(window.payment)=> PaymentRequest's wrapper Storing ScriptValue on a DOM object is highly likely to cause such leaks. PromiseRejectionEvent is hitting the same issue. See how PromiseRejectionEvent is addressing it (though it's complicated).
On 2016/06/02 02:34:39, haraken wrote:
> Yes, it will cause leaks.
>
> PaymentRequest's wrapper ==(Oilpan's Persistent)=> PaymentRequest
==(Oilpan's
> Member)=> PaymentMethodData ==(ScriptValue i.e., V8's Persistent)=> any
> arbitrary V8 object ==(internal strong reference)=> window object
> ==(window.payment)=> PaymentRequest's wrapper
>
> Storing ScriptValue on a DOM object is highly likely to cause such leaks.
>
> PromiseRejectionEvent is hitting the same issue. See how PromiseRejectionEvent
> is addressing it (though it's complicated).
We only need the ScriptValue for the short period of time between
PaymentRequest::PaymentRequest() constructor and PaymentRequest::show() call,
which serializes the ScriptValue into a string sends it over IPC to the browser.
1. PaymentRequest is constructed.
2. Store ScriptValue.
3. show() is called.
4. Serialize ScriptValue into a String.
5. Send the String to the browser.
I suggest we use mek@'s old trick of pre-serializing the ScriptValue into String
and storing that as a member variable instead.
1. PaymentRequest is constructed.
2. Serialize ScriptValue into a String.
3. Store the String.
4. show() is called.
5. Send the String to the browser.
haraken@: That should work, right?
jinho.bang@: This would mean creating a struct like this:
struct Method {
Vector<String> methods;
String stringifiedData;
};
Then store Vector<Method> instead of HeapVector<PaymentMethodData>. I don't
think we need to make this struct garbage collected.
On 2016/06/02 03:36:23, Rouslan (ツ) wrote:
> On 2016/06/02 02:34:39, haraken wrote:
> > Yes, it will cause leaks.
> >
> > PaymentRequest's wrapper ==(Oilpan's Persistent)=> PaymentRequest
> ==(Oilpan's
> > Member)=> PaymentMethodData ==(ScriptValue i.e., V8's Persistent)=> any
> > arbitrary V8 object ==(internal strong reference)=> window object
> > ==(window.payment)=> PaymentRequest's wrapper
> >
> > Storing ScriptValue on a DOM object is highly likely to cause such leaks.
> >
> > PromiseRejectionEvent is hitting the same issue. See how
PromiseRejectionEvent
> > is addressing it (though it's complicated).
>
> We only need the ScriptValue for the short period of time between
> PaymentRequest::PaymentRequest() constructor and PaymentRequest::show() call,
> which serializes the ScriptValue into a string sends it over IPC to the
browser.
>
> 1. PaymentRequest is constructed.
> 2. Store ScriptValue.
> 3. show() is called.
> 4. Serialize ScriptValue into a String.
> 5. Send the String to the browser.
>
> I suggest we use mek@'s old trick of pre-serializing the ScriptValue into
String
> and storing that as a member variable instead.
>
> 1. PaymentRequest is constructed.
> 2. Serialize ScriptValue into a String.
> 3. Store the String.
> 4. show() is called.
> 5. Send the String to the browser.
>
> haraken@: That should work, right?
>
> jinho.bang@: This would mean creating a struct like this:
>
> struct Method {
> Vector<String> methods;
> String stringifiedData;
> };
>
> Then store Vector<Method> instead of HeapVector<PaymentMethodData>. I don't
> think we need to make this struct garbage collected.
Sure. Thank you for your explain
On 2016/06/02 03:39:32, zino wrote:
> On 2016/06/02 03:36:23, Rouslan (ツ) wrote:
> > On 2016/06/02 02:34:39, haraken wrote:
> > > Yes, it will cause leaks.
> > >
> > > PaymentRequest's wrapper ==(Oilpan's Persistent)=> PaymentRequest
> > ==(Oilpan's
> > > Member)=> PaymentMethodData ==(ScriptValue i.e., V8's Persistent)=> any
> > > arbitrary V8 object ==(internal strong reference)=> window object
> > > ==(window.payment)=> PaymentRequest's wrapper
> > >
> > > Storing ScriptValue on a DOM object is highly likely to cause such leaks.
> > >
> > > PromiseRejectionEvent is hitting the same issue. See how
> PromiseRejectionEvent
> > > is addressing it (though it's complicated).
> >
> > We only need the ScriptValue for the short period of time between
> > PaymentRequest::PaymentRequest() constructor and PaymentRequest::show()
call,
> > which serializes the ScriptValue into a string sends it over IPC to the
> browser.
> >
> > 1. PaymentRequest is constructed.
> > 2. Store ScriptValue.
> > 3. show() is called.
> > 4. Serialize ScriptValue into a String.
> > 5. Send the String to the browser.
> >
> > I suggest we use mek@'s old trick of pre-serializing the ScriptValue into
> String
> > and storing that as a member variable instead.
> >
> > 1. PaymentRequest is constructed.
> > 2. Serialize ScriptValue into a String.
> > 3. Store the String.
> > 4. show() is called.
> > 5. Send the String to the browser.
> >
> > haraken@: That should work, right?
> >
> > jinho.bang@: This would mean creating a struct like this:
> >
> > struct Method {
> > Vector<String> methods;
> > String stringifiedData;
> > };
> >
> > Then store Vector<Method> instead of HeapVector<PaymentMethodData>. I don't
> > think we need to make this struct garbage collected.
>
> Sure. Thank you for your explain
Yes, I like the pre-serialization approach :)
Patchset #5 (id:160001) has been deleted
I addressed your comments. PTAL.
Thank you! This is looking great! I have only a few minor tweaks. It's not clear to me why your tests are failing. You should be able to run the tests locally like so: ninja -Cout/lin webkit_unit_tests blink_tests && third_party/WebKit/Tools/Scripts/run-webkit-tests --no-retry-failures --target=lin payments && out/lin/webkit_unit_tests --gtest_filter=*Payment* ------- I've initialized my out/lin directory using "gn args out/lin" with the following data: is_component_build = true is_debug = true is_clang = true https://codereview.chromium.org/2020883002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2020883002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:266: JSONObject data; Simpler: JSONObject data = null; if (!TextUtils.isEmpty(methodData[i].data)) { try {... https://codereview.chromium.org/2020883002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/2020883002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:213: if (!value->isNull() && value->getType() != JSONValue::TypeObject) { If value->isNull(), then you should not stringify the JSON object. if (!value->isNull()) { if (!value->getType() != JSONValue::TypeObject)) { exceptionState.throwTypeError("Data should be a JSON-serializable object"); return; } stringifiedData = JSONObject::cast(value)->toJSONString(); }
Thank you for review. I've just fixed bot error as well. PTAL https://codereview.chromium.org/2020883002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2020883002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:266: JSONObject data; On 2016/06/03 18:22:28, Rouslan (ツ) wrote: > Simpler: > > JSONObject data = null; > if (!TextUtils.isEmpty(methodData[i].data)) { > try {... Done. https://codereview.chromium.org/2020883002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/2020883002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:213: if (!value->isNull() && value->getType() != JSONValue::TypeObject) { On 2016/06/03 18:22:28, Rouslan (ツ) wrote: > If value->isNull(), then you should not stringify the JSON object. > > if (!value->isNull()) { > if (!value->getType() != JSONValue::TypeObject)) { > exceptionState.throwTypeError("Data should be a JSON-serializable object"); > return; > } > stringifiedData = JSONObject::cast(value)->toJSONString(); > } Done.
jinho.bang@samsung.com changed reviewers: + dfalcantara@chromium.org, tsepez@chromium.org
lgtm
+tsepez@ for payment_request.mojom +dfalcantara@ for Java. PTAL
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
https://codereview.chromium.org/2020883002/diff/220001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/payments/payment_request.mojom (right): https://codereview.chromium.org/2020883002/diff/220001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/payments/payment_request.mojom:95: // parses the string via base::JSONParser and passes a part of the JSON We cannot use JSONParser on untrusted data in the browser process. Is this already happening today?
https://codereview.chromium.org/2020883002/diff/220001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/payments/payment_request.mojom (right): https://codereview.chromium.org/2020883002/diff/220001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/payments/payment_request.mojom:95: // parses the string via base::JSONParser and passes a part of the JSON On 2016/06/04 02:32:36, dcheng wrote: > We cannot use JSONParser on untrusted data in the browser process. Is this > already happening today? Yes, it's happening here: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... JSONObject result; try { result = new JSONObject(stringifiedData); } catch (JSONException e) { // Payment method specific data should be a JSON object. return null; } What's the correct course of action?
https://codereview.chromium.org/2020883002/diff/220001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/payments/payment_request.mojom (right): https://codereview.chromium.org/2020883002/diff/220001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/payments/payment_request.mojom:95: // parses the string via base::JSONParser and passes a part of the JSON On 2016/06/04 at 02:42:30, Rouslan (ツ) wrote: > On 2016/06/04 02:32:36, dcheng wrote: > > We cannot use JSONParser on untrusted data in the browser process. Is this > > already happening today? > > Yes, it's happening here: > > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > > JSONObject result; > try { > result = new JSONObject(stringifiedData); > } catch (JSONException e) { > // Payment method specific data should be a JSON object. > return null; > } > > What's the correct course of action? Hmm. The comment implies base::JSONParser, which is a C++-based parser (and that's definitely not OK): in C++, the preferred idiom is to use //components/safe_json_parser. However, I'm not sure if the same concerns apply to parsing purely in Java, which should be memory-safe. So maybe this is OK... but I'd start a thread with security@chromium.org to confirm.
https://codereview.chromium.org/2020883002/diff/220001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/payments/payment_request.mojom (right): https://codereview.chromium.org/2020883002/diff/220001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/payments/payment_request.mojom:95: // parses the string via base::JSONParser and passes a part of the JSON On 2016/06/04 03:30:01, dcheng wrote: > On 2016/06/04 at 02:42:30, Rouslan (ツ) wrote: > > On 2016/06/04 02:32:36, dcheng wrote: > > > We cannot use JSONParser on untrusted data in the browser process. Is this > > > already happening today? > > > > Yes, it's happening here: > > > > > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > > > > JSONObject result; > > try { > > result = new JSONObject(stringifiedData); > > } catch (JSONException e) { > > // Payment method specific data should be a JSON object. > > return null; > > } > > > > What's the correct course of action? > > Hmm. The comment implies base::JSONParser, which is a C++-based parser (and > that's definitely not OK): in C++, the preferred idiom is to use > //components/safe_json_parser. However, I'm not sure if the same concerns apply > to parsing purely in Java, which should be memory-safe. So maybe this is OK... > but I'd start a thread with mailto:security@chromium.org to confirm. Yes, the C++ impl has this restriction which the java doesn't.
*.java lgtm https://codereview.chromium.org/2020883002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2020883002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:263: if (methodData == null || methodData.length == 0) return null; nit: newline here. https://codereview.chromium.org/2020883002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:272: return null; I confirmed with rouslan that you're intentionally throwing away everything and returning null here if even only one is bad, but could you add a function comment about why that's the case?
On 2016/06/06 16:15:37, Tom Sepez wrote: > Yes, the C++ impl has this restriction which the java doesn't. Using the Java version of JsonSanitizer in PaymentRequestImpl: http://crrev.com/2039303002
On 2016/06/06 18:36:20, Rouslan (ツ) wrote: > On 2016/06/06 16:15:37, Tom Sepez wrote: > > Yes, the C++ impl has this restriction which the java doesn't. > > Using the Java version of JsonSanitizer in PaymentRequestImpl: > http://crrev.com/2039303002 jinho.bang@: please wait for http://crrev.com/2039303002 to land and rebase your patch on top of that. As a result, you should use "new JSONObject(JsonSanitizer.sanitize(data))" instead of "new JSONObject(data)".
On 2016/06/06 18:37:45, Rouslan (ツ) wrote: > On 2016/06/06 18:36:20, Rouslan (ツ) wrote: > > On 2016/06/06 16:15:37, Tom Sepez wrote: > > > Yes, the C++ impl has this restriction which the java doesn't. > > > > Using the Java version of JsonSanitizer in PaymentRequestImpl: > > http://crrev.com/2039303002 > > jinho.bang@: please wait for http://crrev.com/2039303002 to land and rebase your > patch on top of that. As a result, you should use "new > JSONObject(JsonSanitizer.sanitize(data))" instead of "new JSONObject(data)". Okay. Thank you for information.
Patchset #8 (id:240001) has been deleted
Patchset #11 (id:320001) has been deleted
Patchset #10 (id:300001) has been deleted
Patchset #9 (id:280001) has been deleted
On 2016/06/06 18:52:05, zino wrote: > On 2016/06/06 18:37:45, Rouslan (ツ) wrote: > > jinho.bang@: please wait for http://crrev.com/2039303002 to land and rebase > your > > patch on top of that. As a result, you should use "new > > JSONObject(JsonSanitizer.sanitize(data))" instead of "new JSONObject(data)". http://crrev.com/2039303002 landed. Please rebase.
Patchset #11 (id:380001) has been deleted
Patchset #10 (id:360001) has been deleted
Patchset #9 (id:340001) has been deleted
Patchset #11 (id:440001) has been deleted
Patchset #10 (id:420001) has been deleted
Patchset #9 (id:400001) has been deleted
Rebased. PTAL (Especially *.mojom)
Patchset #9 (id:460001) has been deleted
Patchset #9 (id:480001) has been deleted
Patchset #9 (id:490001) has been deleted
still lgtm https://codereview.chromium.org/2020883002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2020883002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:255: // Payment method specific data should be a JSON object. Looks like your rebase has removed that very useful comment about rejecting all data if any of it is invalid. This patch is getting large and unwieldy. It would be nice to commit it sooner rather than later. https://codereview.chromium.org/2020883002/diff/260001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/payments/payment_request.mojom (right): https://codereview.chromium.org/2020883002/diff/260001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/payments/payment_request.mojom:96: // parses the string via base::JSONParser and passes a part of the JSON Replace "base::JSONParser" with "JSONObject(JsonSantizier.sanitize(stringified_data))". https://codereview.chromium.org/2020883002/diff/260001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/payments/payment_request.mojom:102: string data; Let's call it "stringified_data" to match WebKit.
On 2016/06/09 02:44:13, Rouslan (ツ) wrote: > still lgtm > > https://codereview.chromium.org/2020883002/diff/260001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java > (right): > > https://codereview.chromium.org/2020883002/diff/260001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:255: > // Payment method specific data should be a JSON object. > Looks like your rebase has removed that very useful comment about rejecting all > data if any of it is invalid. This patch is getting large and unwieldy. It would > be nice to commit it sooner rather than later. > > https://codereview.chromium.org/2020883002/diff/260001/third_party/WebKit/pub... > File third_party/WebKit/public/platform/modules/payments/payment_request.mojom > (right): > > https://codereview.chromium.org/2020883002/diff/260001/third_party/WebKit/pub... > third_party/WebKit/public/platform/modules/payments/payment_request.mojom:96: // > parses the string via base::JSONParser and passes a part of the JSON > Replace "base::JSONParser" with > "JSONObject(JsonSantizier.sanitize(stringified_data))". > > https://codereview.chromium.org/2020883002/diff/260001/third_party/WebKit/pub... > third_party/WebKit/public/platform/modules/payments/payment_request.mojom:102: > string data; > Let's call it "stringified_data" to match WebKit. Okay Thank you for review. I'll fix them and bot error as well.
LGTM
Patchset #10 (id:530001) has been deleted
Patchset #9 (id:510001) has been deleted
The CQ bit was checked by jinho.bang@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from dfalcantara@chromium.org, rouslan@chromium.org, haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2020883002/#ps550001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2020883002/550001
The CQ bit was unchecked by jinho.bang@samsung.com
The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2020883002/570001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tsepez@ Could you review payment_reqeust.mojom?
Mojom LGTM, given the extensive discussion of JSON already on this CL. Thanks.
The CQ bit was checked by jinho.bang@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org, haraken@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2020883002/#ps570001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2020883002/570001
Message was sent while issue was closed.
Description was changed from ========== PaymentRequest: Introduce PaymentMethodData. This CL is introducing PaymentMethodData to combine the supported methods and payment method specific data. By making this change it is possible to share an instance of data amongst multiple payment methods that wasn't possible before. Related spec links: [1] https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#idl-def-p... [2] https://github.com/w3c/browser-payment-api/pull/162/commits/6d9c92e5d2bc82e49... BUG=none ========== to ========== PaymentRequest: Introduce PaymentMethodData. This CL is introducing PaymentMethodData to combine the supported methods and payment method specific data. By making this change it is possible to share an instance of data amongst multiple payment methods that wasn't possible before. Related spec links: [1] https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#idl-def-p... [2] https://github.com/w3c/browser-payment-api/pull/162/commits/6d9c92e5d2bc82e49... BUG=none ==========
Message was sent while issue was closed.
Committed patchset #10 (id:570001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== PaymentRequest: Introduce PaymentMethodData. This CL is introducing PaymentMethodData to combine the supported methods and payment method specific data. By making this change it is possible to share an instance of data amongst multiple payment methods that wasn't possible before. Related spec links: [1] https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#idl-def-p... [2] https://github.com/w3c/browser-payment-api/pull/162/commits/6d9c92e5d2bc82e49... BUG=none ========== to ========== PaymentRequest: Introduce PaymentMethodData. This CL is introducing PaymentMethodData to combine the supported methods and payment method specific data. By making this change it is possible to share an instance of data amongst multiple payment methods that wasn't possible before. Related spec links: [1] https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#idl-def-p... [2] https://github.com/w3c/browser-payment-api/pull/162/commits/6d9c92e5d2bc82e49... BUG=none Committed: https://crrev.com/c6d2d4581607e7642f2f85a386cf3ace7aacf803 Cr-Commit-Position: refs/heads/master@{#398954} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/c6d2d4581607e7642f2f85a386cf3ace7aacf803 Cr-Commit-Position: refs/heads/master@{#398954} |
