Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2578323006:
Parsing new Android Pay Payment Request API member: minGooglePlayServicesVersion (Closed)
|
Created:
4 years ago by Walter Cacau Modified:
4 years ago CC:
chromium-reviews, qsr+mojo_chromium.org, gogerald+paymentswatch_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, haraken, Aaron Boodman, rouslan+payments_chromium.org, blink-reviews, darin (slow to review), sebsg+paymentswatch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionParsing new Android Pay Payment Request API member: minGooglePlayServicesVersion
BUG=675266
Committed: https://crrev.com/94bbd0c80544b7123f0cc62e3c2e1611b6e5f1eb
Cr-Commit-Position: refs/heads/master@{#439846}
Patch Set 1 #
Total comments: 2
Patch Set 2 : making minGooglePlayServicesVersion an int32 #
Total comments: 6
Patch Set 3 : review comments #
Messages
Total messages: 26 (13 generated)
Description was changed from ========== Parsing new Android Pay Payment Request API member: minGooglePlayServicesVersion BUG=675266 ========== to ========== Parsing new Android Pay Payment Request API member: minGooglePlayServicesVersion BUG=675266 ==========
waltercacau@chromium.org changed reviewers: + rouslan@chromium.org
The CQ bit was checked by waltercacau@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.
WebKit/Source/modules/payments/* LGTM You need someone from ipc/SECURITY_OWNERS to approve the change to mojom.
waltercacau@chromium.org changed reviewers: + palmer@chromium.org
Added palmer@ for ipc/
https://codereview.chromium.org/2578323006/diff/1/components/payments/payment... File components/payments/payment_request.mojom (right): https://codereview.chromium.org/2578323006/diff/1/components/payments/payment... components/payments/payment_request.mojom:131: string? min_google_play_services_version; Can this be an Integer? I've asked for a boolean (or enum) elsewhere, but now realize that solution is impossible, because the renderer does not have information about Google Play Services.
https://codereview.chromium.org/2578323006/diff/1/components/payments/payment... File components/payments/payment_request.mojom (right): https://codereview.chromium.org/2578323006/diff/1/components/payments/payment... components/payments/payment_request.mojom:131: string? min_google_play_services_version; On 2016/12/19 20:25:04, rouslan-intermittent-holidays wrote: > Can this be an Integer? > > I've asked for a boolean (or enum) elsewhere, but now realize that solution is > impossible, because the renderer does not have information about Google Play > Services. Done.
https://codereview.chromium.org/2578323006/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/AndroidPayMethodData.idl (right): https://codereview.chromium.org/2578323006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/AndroidPayMethodData.idl:14: DOMString minGooglePlayServicesVersion; You will get free parsing and error handling if you change DOMString to long in your API. So users will need to specify data.minGooglePlayServicesVersion=123 instead of data.minGooglePlayServicesVersion="123". Would this be better for you? https://www.w3.org/TR/WebIDL/#idl-long https://codereview.chromium.org/2578323006/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/2578323006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:38: #include "wtf/text/WTFString.h" Already included in PaymnentRequest.h, so no need for it in PaymentRequest.cpp. https://codereview.chromium.org/2578323006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:256: You should initialize output->min_google_play_services_version to an explicit value that means "merchant did not specify" or "does not matter to this merchant", such as 0. Then add a comment in payment_request.mojom that 0 means unspecified. If you follow my advice about making this a long in the WebIDL file, then you can use a single statement here: output->min_google_play_services_version = androidPay.hasMinGooglePlayServicesVersion() ? androidPay.minGooglePlayServicesVersion() : 0;
https://codereview.chromium.org/2578323006/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/AndroidPayMethodData.idl (right): https://codereview.chromium.org/2578323006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/AndroidPayMethodData.idl:14: DOMString minGooglePlayServicesVersion; On 2016/12/19 21:47:38, rouslan-intermittent-holidays wrote: > You will get free parsing and error handling if you change DOMString to long in > your API. So users will need to specify data.minGooglePlayServicesVersion=123 > instead of data.minGooglePlayServicesVersion="123". Would this be better for > you? > > https://www.w3.org/TR/WebIDL/#idl-long Generally with numbers where we care about precision we have chosen to encode them in strings rather than numbers in the JSON API and the fact that we don't have very flexible JSON parsing libraries within Google Play Services. Rather keep it as a string because the plan is to actually move the parsing it to Google Play Services. https://codereview.chromium.org/2578323006/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/2578323006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:38: #include "wtf/text/WTFString.h" On 2016/12/19 21:47:38, rouslan-intermittent-holidays wrote: > Already included in PaymnentRequest.h, so no need for it in PaymentRequest.cpp. Done. https://codereview.chromium.org/2578323006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:256: On 2016/12/19 21:47:38, rouslan-intermittent-holidays wrote: > You should initialize output->min_google_play_services_version to an explicit > value that means "merchant did not specify" or "does not matter to this > merchant", such as 0. Then add a comment in payment_request.mojom that 0 means > unspecified. > > If you follow my advice about making this a long in the WebIDL file, then you > can use a single statement here: > > output->min_google_play_services_version = > androidPay.hasMinGooglePlayServicesVersion() ? > androidPay.minGooglePlayServicesVersion() : 0; Done.
waltercacau@chromium.org changed reviewers: + kenrb@chromium.org - palmer@chromium.org
added kenrb for the mojom changes
waltercacau@chromium.org changed reviewers: + loums@google.com
lgtm
mojo lgtm
The CQ bit was checked by waltercacau@chromium.org
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": 40001, "attempt_start_ts": 1482251402592890,
"parent_rev": "19def6560960093319a9925be48fc5be62bdbda7", "commit_rev":
"a65c4408ed237b0a0518d87b826a6afb83deca8e"}
Message was sent while issue was closed.
Description was changed from ========== Parsing new Android Pay Payment Request API member: minGooglePlayServicesVersion BUG=675266 ========== to ========== Parsing new Android Pay Payment Request API member: minGooglePlayServicesVersion BUG=675266 Review-Url: https://codereview.chromium.org/2578323006 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Parsing new Android Pay Payment Request API member: minGooglePlayServicesVersion BUG=675266 Review-Url: https://codereview.chromium.org/2578323006 ========== to ========== Parsing new Android Pay Payment Request API member: minGooglePlayServicesVersion BUG=675266 Committed: https://crrev.com/94bbd0c80544b7123f0cc62e3c2e1611b6e5f1eb Cr-Commit-Position: refs/heads/master@{#439846} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/94bbd0c80544b7123f0cc62e3c2e1611b6e5f1eb Cr-Commit-Position: refs/heads/master@{#439846} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
