Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2925063003:
[Payments] Implement payment instrument icons (Closed)
|
Created:
3 years, 6 months ago by gogerald1 Modified:
3 years, 6 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, Aaron Boodman, viettrungluu+watch_chromium.org, jam, yzshen+watch_chromium.org, gogerald+paymentswatch_chromium.org, rouslan+payments_chromium.org, blink-reviews, darin-cc_chromium.org, agrieve+watch_chromium.org, mahmadi+paymentswatch_chromium.org, darin (slow to review), abarth-chromium, sebsg+paymentswatch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement payment instrument icons
Tested bobpay:
https://github.com/gogerald/gogerald.github.io/blob/master/pr/bobpaywebapp/register.js
The spec:
https://github.com/w3c/payment-handler/pull/174
BUG=720029
Review-Url: https://codereview.chromium.org/2925063003
Cr-Commit-Position: refs/heads/master@{#479258}
Committed: https://chromium.googlesource.com/chromium/src/+/d8c5af3464ac1f827f8e673095c8e24496eea1da
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 27
Patch Set 3 : address comments and add DEPS #
Total comments: 2
Patch Set 4 : remove unused attributes #
Total comments: 10
Patch Set 5 : resolve url based on execution context #
Total comments: 34
Patch Set 6 : reference count #
Total comments: 17
Patch Set 7 : fix comment lines #Messages
Total messages: 73 (52 generated)
Description was changed from ========== Implement payment instrument icons BUG= ========== to ========== Implement payment instrument icons BUG= ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== Implement payment instrument icons BUG= ========== to ========== Implement payment instrument icons BUG=720029 ==========
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Implement payment instrument icons BUG=720029 ========== to ========== Implement payment instrument icons Tested bobpay: https://github.com/gogerald/gogerald.github.io/blob/master/pr/bobpaywebapp/re... The spec: https://w3c.github.io/payment-handler/#dom-paymentinstrument BUG=720029 ==========
The CQ bit was checked by gogerald@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: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by gogerald@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...
gogerald@chromium.org changed reviewers: + jinho.bang@samsung.com, rouslan@chromium.org
Hi, rouslan@ and jinho.bang@, ptal,
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...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CL description should link to the pull request that describes the ImageObject IDL. https://codereview.chromium.org/2925063003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentInstrument.java (right): https://codereview.chromium.org/2925063003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentInstrument.java:49: icon /* icon */); Please remove the clarifying comment /* icon */, because it matches the name of the parameter exactly now. https://codereview.chromium.org/2925063003/diff/80001/components/payments/moj... File components/payments/mojom/payment_app.mojom (right): https://codereview.chromium.org/2925063003/diff/80001/components/payments/moj... components/payments/mojom/payment_app.mojom:21: string src; use mojom.url.GURL instead. https://codereview.chromium.org/2925063003/diff/80001/components/payments/moj... components/payments/mojom/payment_app.mojom:22: string sizes; This is never used. Discard, please. If you need it, then use an array of integer pairs. struct ImageSize { int32_t height; int32_t width; } array<ImageSize> sizes; https://codereview.chromium.org/2925063003/diff/80001/components/payments/moj... components/payments/mojom/payment_app.mojom:23: string type; This is never used. Discard, please. If you need it, use an enum instead. https://codereview.chromium.org/2925063003/diff/80001/content/browser/payment... File content/browser/payments/payment_app_database.cc (right): https://codereview.chromium.org/2925063003/diff/80001/content/browser/payment... content/browser/payments/payment_app_database.cc:97: gfx::Image icon_image = gfx::Image::CreateFrom1xPNGBytes( Add a note that you've converted the image to PNG at this point already, regardless of the icon format that was downloaded. https://codereview.chromium.org/2925063003/diff/80001/content/browser/payment... File content/browser/payments/payment_instrument_icon_fetcher.cc (right): https://codereview.chromium.org/2925063003/diff/80001/content/browser/payment... content/browser/payments/payment_instrument_icon_fetcher.cc:24: net::NetworkTrafficAnnotationTag g_traffic_annotation = Does this cause an exit time destructor? No reason to keep it in the anonymous namespace, since it's used only once. Please put it inside the function where it's being used. https://codereview.chromium.org/2925063003/diff/80001/content/browser/payment... File content/browser/payments/payment_instrument_icon_fetcher.h (right): https://codereview.chromium.org/2925063003/diff/80001/content/browser/payment... content/browser/payments/payment_instrument_icon_fetcher.h:27: ~PaymentInstrumentIconFetcher() override {} Don't inline destructors for classes with complex member variables. https://codereview.chromium.org/2925063003/diff/80001/content/browser/payment... content/browser/payments/payment_instrument_icon_fetcher.h:59: size_t checking_image_object_index_; Need to initialize in the constructor, otherwise it's a random value, which leads to all kinds of bugs. https://codereview.chromium.org/2925063003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/ImageObject.idl (right): https://codereview.chromium.org/2925063003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/ImageObject.idl:5: // https://www.w3.org/TR/appmanifest/#icons-member Please link to the IDL, even if it's in a pull request. https://codereview.chromium.org/2925063003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/ImageObject.idl:8: required DOMString src; USVString is usually used for URLs. https://codereview.chromium.org/2925063003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentInstrument.idl (right): https://codereview.chromium.org/2925063003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentInstrument.idl:5: // https://w3c.github.io/webpayments-payment-apps-api/#payment-instrument Please update the URL.
The CQ bit was checked by gogerald@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: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by gogerald@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 #3 (id:100001) has been deleted
Hi rouslan@, ptal, https://codereview.chromium.org/2925063003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentInstrument.java (right): https://codereview.chromium.org/2925063003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentInstrument.java:49: icon /* icon */); On 2017/06/08 21:34:10, rouslan wrote: > Please remove the clarifying comment /* icon */, because it matches the name of > the parameter exactly now. Done. https://codereview.chromium.org/2925063003/diff/80001/components/payments/moj... File components/payments/mojom/payment_app.mojom (right): https://codereview.chromium.org/2925063003/diff/80001/components/payments/moj... components/payments/mojom/payment_app.mojom:21: string src; On 2017/06/08 21:34:10, rouslan wrote: > use mojom.url.GURL instead. Why using mojom.url.GURL? PaymentInstruments has get method in the spec which should return exact content used in set method. https://w3c.github.io/payment-handler/#paymentinstruments-interface So it looks better to store original representation of them directly, instead convert them back and forth. https://codereview.chromium.org/2925063003/diff/80001/components/payments/moj... components/payments/mojom/payment_app.mojom:22: string sizes; On 2017/06/08 21:34:10, rouslan wrote: > This is never used. Discard, please. If you need it, then use an array of > integer pairs. > > struct ImageSize { > int32_t height; > int32_t width; > } > > array<ImageSize> sizes; This is used by get interface. We could decode it when using it later. And the usage is expected one time usage when fetching and decoding the instrument icon. https://codereview.chromium.org/2925063003/diff/80001/components/payments/moj... components/payments/mojom/payment_app.mojom:23: string type; On 2017/06/08 21:34:10, rouslan wrote: > This is never used. Discard, please. If you need it, use an enum instead. ditto https://codereview.chromium.org/2925063003/diff/80001/content/browser/payment... File content/browser/payments/payment_app_database.cc (right): https://codereview.chromium.org/2925063003/diff/80001/content/browser/payment... content/browser/payments/payment_app_database.cc:97: gfx::Image icon_image = gfx::Image::CreateFrom1xPNGBytes( On 2017/06/08 21:34:10, rouslan wrote: > Add a note that you've converted the image to PNG at this point already, > regardless of the icon format that was downloaded. Done. https://codereview.chromium.org/2925063003/diff/80001/content/browser/payment... File content/browser/payments/payment_instrument_icon_fetcher.cc (right): https://codereview.chromium.org/2925063003/diff/80001/content/browser/payment... content/browser/payments/payment_instrument_icon_fetcher.cc:24: net::NetworkTrafficAnnotationTag g_traffic_annotation = On 2017/06/08 21:34:11, rouslan wrote: > Does this cause an exit time destructor? No reason to keep it in the anonymous > namespace, since it's used only once. Please put it inside the function where > it's being used. It may be used multiple times when there are multiple image objects for a instrument. That's the main reason I want to keep it. net::NetworkTrafficAnnotationTag in essence is a struct has an unsigned integer as its member. https://codereview.chromium.org/2925063003/diff/80001/content/browser/payment... File content/browser/payments/payment_instrument_icon_fetcher.h (right): https://codereview.chromium.org/2925063003/diff/80001/content/browser/payment... content/browser/payments/payment_instrument_icon_fetcher.h:27: ~PaymentInstrumentIconFetcher() override {} On 2017/06/08 21:34:11, rouslan wrote: > Don't inline destructors for classes with complex member variables. Done. https://codereview.chromium.org/2925063003/diff/80001/content/browser/payment... content/browser/payments/payment_instrument_icon_fetcher.h:59: size_t checking_image_object_index_; On 2017/06/08 21:34:11, rouslan wrote: > Need to initialize in the constructor, otherwise it's a random value, which > leads to all kinds of bugs. It was initialized in start() public interface, https://codereview.chromium.org/2925063003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/ImageObject.idl (right): https://codereview.chromium.org/2925063003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/ImageObject.idl:5: // https://www.w3.org/TR/appmanifest/#icons-member On 2017/06/08 21:34:11, rouslan wrote: > Please link to the IDL, even if it's in a pull request. Done. https://codereview.chromium.org/2925063003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/ImageObject.idl:8: required DOMString src; On 2017/06/08 21:34:11, rouslan wrote: > USVString is usually used for URLs. Done. https://codereview.chromium.org/2925063003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentInstrument.idl (right): https://codereview.chromium.org/2925063003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentInstrument.idl:5: // https://w3c.github.io/webpayments-payment-apps-api/#payment-instrument On 2017/06/08 21:34:11, rouslan wrote: > Please update the URL. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
Description was changed from ========== Implement payment instrument icons Tested bobpay: https://github.com/gogerald/gogerald.github.io/blob/master/pr/bobpaywebapp/re... The spec: https://w3c.github.io/payment-handler/#dom-paymentinstrument BUG=720029 ========== to ========== Implement payment instrument icons Tested bobpay: https://github.com/gogerald/gogerald.github.io/blob/master/pr/bobpaywebapp/re... The spec: https://github.com/w3c/payment-handler/pull/174 BUG=720029 ==========
Sorry if I seem like I'm making unreasonable requests for the IPC struct formats. This will save you some time when a mojom file owner (from the security team) will be reviewing the patch. https://codereview.chromium.org/2925063003/diff/80001/components/payments/moj... File components/payments/mojom/payment_app.mojom (right): https://codereview.chromium.org/2925063003/diff/80001/components/payments/moj... components/payments/mojom/payment_app.mojom:21: string src; On 2017/06/09 14:23:24, gogerald1 wrote: > On 2017/06/08 21:34:10, rouslan wrote: > > use mojom.url.GURL instead. > > Why using mojom.url.GURL? PaymentInstruments has get method in the spec which > should return exact content used in set method. > https://w3c.github.io/payment-handler/#paymentinstruments-interface > > So it looks better to store original representation of them directly, instead > convert them back and forth. A strict IPC interface is better for security. (This data will always be a URL.) In fact, using the GURL object up-and-down the full stack that passes around this variable is preferable. See "url.mojom.Url top_level_origin" below for example. https://codereview.chromium.org/2925063003/diff/80001/components/payments/moj... components/payments/mojom/payment_app.mojom:22: string sizes; On 2017/06/09 14:23:24, gogerald1 wrote: > On 2017/06/08 21:34:10, rouslan wrote: > > This is never used. Discard, please. If you need it, then use an array of > > integer pairs. > > > > struct ImageSize { > > int32_t height; > > int32_t width; > > } > > > > array<ImageSize> sizes; > > This is used by get interface. We could decode it when using it later. And the > usage is expected one time usage when fetching and decoding the instrument icon. > All additional parameters increase the attack surface for IPC messages. Adding a parameter that's not used increases the attack surface without any benefits. Therefore, parameters should be added at the time when they are being used. https://codereview.chromium.org/2925063003/diff/120001/content/browser/paymen... File content/browser/payments/payment_instrument_icon_fetcher.cc (right): https://codereview.chromium.org/2925063003/diff/120001/content/browser/paymen... content/browser/payments/payment_instrument_icon_fetcher.cc:47: PaymentInstrumentIconFetcher::PaymentInstrumentIconFetcher() {} Please initialize the checking_image_object_index_ to be more paranoid.
The CQ bit was checked by gogerald@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...
gogerald@chromium.org changed reviewers: + dcheng@chromium.org
Hi dcheng@, ptal of changes in components/payments/mojom/payment_app.mojom which corresponds https://codereview.chromium.org/2925063003/diff/80001/components/payments/moj... File components/payments/mojom/payment_app.mojom (right): https://codereview.chromium.org/2925063003/diff/80001/components/payments/moj... components/payments/mojom/payment_app.mojom:21: string src; On 2017/06/09 14:52:53, rouslan wrote: > On 2017/06/09 14:23:24, gogerald1 wrote: > > On 2017/06/08 21:34:10, rouslan wrote: > > > use mojom.url.GURL instead. > > > > Why using mojom.url.GURL? PaymentInstruments has get method in the spec which > > should return exact content used in set method. > > https://w3c.github.io/payment-handler/#paymentinstruments-interface > > > > So it looks better to store original representation of them directly, instead > > convert them back and forth. > > A strict IPC interface is better for security. (This data will always be a URL.) > In fact, using the GURL object up-and-down the full stack that passes around > this variable is preferable. > > See "url.mojom.Url top_level_origin" below for example. as talked offline https://codereview.chromium.org/2925063003/diff/80001/components/payments/moj... components/payments/mojom/payment_app.mojom:22: string sizes; On 2017/06/09 14:52:53, rouslan wrote: > On 2017/06/09 14:23:24, gogerald1 wrote: > > On 2017/06/08 21:34:10, rouslan wrote: > > > This is never used. Discard, please. If you need it, then use an array of > > > integer pairs. > > > > > > struct ImageSize { > > > int32_t height; > > > int32_t width; > > > } > > > > > > array<ImageSize> sizes; > > > > This is used by get interface. We could decode it when using it later. And the > > usage is expected one time usage when fetching and decoding the instrument > icon. > > > > All additional parameters increase the attack surface for IPC messages. Adding a > parameter that's not used increases the attack surface without any benefits. > Therefore, parameters should be added at the time when they are being used. Done. https://codereview.chromium.org/2925063003/diff/80001/components/payments/moj... components/payments/mojom/payment_app.mojom:23: string type; On 2017/06/09 14:23:23, gogerald1 wrote: > On 2017/06/08 21:34:10, rouslan wrote: > > This is never used. Discard, please. If you need it, use an enum instead. > > ditto Done. https://codereview.chromium.org/2925063003/diff/120001/content/browser/paymen... File content/browser/payments/payment_instrument_icon_fetcher.cc (right): https://codereview.chromium.org/2925063003/diff/120001/content/browser/paymen... content/browser/payments/payment_instrument_icon_fetcher.cc:47: PaymentInstrumentIconFetcher::PaymentInstrumentIconFetcher() {} On 2017/06/09 14:52:53, rouslan wrote: > Please initialize the checking_image_object_index_ to be more paranoid. Done.
lgtm
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...)
lgtm with some comments. https://codereview.chromium.org/2925063003/diff/140001/content/browser/paymen... File content/browser/payments/payment_app_database.cc (right): https://codereview.chromium.org/2925063003/diff/140001/content/browser/paymen... content/browser/payments/payment_app_database.cc:100: instrument->icon = base::MakeUnique<SkBitmap>(icon_image.AsBitmap()); Although there is no desktop implementation yet, we can not use each different icon in desktop and mobile. We will have to store multiple icons unless Chrome decided to use only one size/type icon in all platforms and all screens. (But leaving a comment and filing bug is okay for now.) It might be better to fetch and cache before actual displaying the icon. https://codereview.chromium.org/2925063003/diff/140001/content/browser/paymen... File content/browser/payments/payment_instrument_icon_fetcher.cc (right): https://codereview.chromium.org/2925063003/diff/140001/content/browser/paymen... content/browser/payments/payment_instrument_icon_fetcher.cc:51: void PaymentInstrumentIconFetcher::start( nit: s/start/Start https://codereview.chromium.org/2925063003/diff/140001/content/browser/paymen... content/browser/payments/payment_instrument_icon_fetcher.cc:61: for (const auto& obj : image_objects) { If my understanding is correct, the current implementation attempts to fetch sequentially and then stores the first successful fetched image. But we will have to consider |size| and |mime| type later. So, isn't it better to leave a TODO comment with filing a bug for tracking? https://codereview.chromium.org/2925063003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp (right): https://codereview.chromium.org/2925063003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp:150: if (image_object.hasSrc()) Do we need this check? Because it is a required field, I thought that the check is unnecessary. (but I'm not sure) https://codereview.chromium.org/2925063003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp:150: if (image_object.hasSrc()) The url can be relative, so, we should parse the url as follows: ExecutionContext* context = ExecutionContext::From(script_state); KURL parsed_url = KURL(ToWorkerGlobalScope(context)->location()->Url(), image_object.src()); if (!parsed_url.IsValid() || !parsed_url.ProtocolIsInHTTPFamily()) { resolver->Reject(V8ThrowException::CreateTypeError( script_state->GetIsolate(), "'" + image_object.src() + "' is not a valid URL.")); return promise; }
Patchset #5 (id:160001) has been deleted
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Patchset #5 (id:180001) 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...
Thanks https://codereview.chromium.org/2925063003/diff/140001/content/browser/paymen... File content/browser/payments/payment_app_database.cc (right): https://codereview.chromium.org/2925063003/diff/140001/content/browser/paymen... content/browser/payments/payment_app_database.cc:100: instrument->icon = base::MakeUnique<SkBitmap>(icon_image.AsBitmap()); On 2017/06/09 20:29:48, zino wrote: > Although there is no desktop implementation yet, we can not use each different > icon in desktop and mobile. > We will have to store multiple icons unless Chrome decided to use only one > size/type icon in all platforms and all screens. > (But leaving a comment and filing bug is okay for now.) > > It might be better to fetch and cache before actual displaying the icon. yep, I added TODO on the start interface, https://codereview.chromium.org/2925063003/diff/140001/content/browser/paymen... File content/browser/payments/payment_instrument_icon_fetcher.cc (right): https://codereview.chromium.org/2925063003/diff/140001/content/browser/paymen... content/browser/payments/payment_instrument_icon_fetcher.cc:51: void PaymentInstrumentIconFetcher::start( On 2017/06/09 20:29:48, zino wrote: > nit: s/start/Start Done. https://codereview.chromium.org/2925063003/diff/140001/content/browser/paymen... content/browser/payments/payment_instrument_icon_fetcher.cc:61: for (const auto& obj : image_objects) { On 2017/06/09 20:29:48, zino wrote: > If my understanding is correct, the current implementation attempts to fetch > sequentially and then stores the first successful fetched image. > But we will have to consider |size| and |mime| type later. So, isn't it better > to leave a TODO comment with filing a bug for tracking? Yes, I added a TODO for start interface, plan to use the same bug to track these works https://codereview.chromium.org/2925063003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp (right): https://codereview.chromium.org/2925063003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp:150: if (image_object.hasSrc()) On 2017/06/09 20:29:48, zino wrote: > Do we need this check? > Because it is a required field, I thought that the check is unnecessary. (but > I'm not sure) Done. https://codereview.chromium.org/2925063003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp:150: if (image_object.hasSrc()) On 2017/06/09 20:29:48, zino wrote: > The url can be relative, so, we should parse the url as follows: > > ExecutionContext* context = ExecutionContext::From(script_state); > KURL parsed_url = KURL(ToWorkerGlobalScope(context)->location()->Url(), > image_object.src()); > if (!parsed_url.IsValid() || !parsed_url.ProtocolIsInHTTPFamily()) { > resolver->Reject(V8ThrowException::CreateTypeError( > script_state->GetIsolate(), "'" + image_object.src() + "' is not a valid > URL.")); > return promise; > } Done.
gogerald@chromium.org changed reviewers: + alexmos@chromium.org, haraken@chromium.org
Hi haraken@, Please review changes in third_party/WebKit/Source/modules/modules_idl_files.gni alexmos@, Please review changes in content/browser/BUILD.gn content/public/browser/stored_payment_instrument.h
https://codereview.chromium.org/2925063003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/ImageObject.idl (right): https://codereview.chromium.org/2925063003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/ImageObject.idl:5: // https://github.com/w3c/payment-handler/pull/174/files/6e6768c423e24719cb0e592... Can we add a more stable URL instead of a specific commit? https://codereview.chromium.org/2925063003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp (right): https://codereview.chromium.org/2925063003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp:231: instrument.setIcons(icons); dcheng@: If we add a type mapping between mojom::blink::PaymentInstrumentPtr and PaymentInstrument, can we probably remove this code?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2925063003/diff/200001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestServiceWorkerPaymentAppTest.java (right): https://codereview.chromium.org/2925063003/diff/200001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestServiceWorkerPaymentAppTest.java:69: "https://bobpay.com", "basic-card")) /* methodNames */, Nit: this should probably using something like example.com. Using random domains is a bit dangerous. https://codereview.chromium.org/2925063003/diff/200001/components/payments/mo... File components/payments/mojom/payment_app.mojom (right): https://codereview.chromium.org/2925063003/diff/200001/components/payments/mo... components/payments/mojom/payment_app.mojom:24: struct PaymentInstrument { In general, this mojom file needs more comments. It's not clear what interfaces live in the browser vs the renderer or some other process, nor what the different structs mean / what they correspond to. https://codereview.chromium.org/2925063003/diff/200001/content/browser/paymen... File content/browser/payments/payment_app_database.cc (right): https://codereview.chromium.org/2925063003/diff/200001/content/browser/paymen... content/browser/payments/payment_app_database.cc:67: image_object->src = GURL(icon.src()); I'm a bit confused about how the payment flow works. Do the icon images originally come from the renderer? From some external source? Why does it start in proto form? etc. https://codereview.chromium.org/2925063003/diff/200001/content/browser/paymen... content/browser/payments/payment_app_database.cc:68: instrument->icons.emplace_back(std::move(image_object)); Note: New() has an overload that takes arguments, so it's possible to write: instrument->icons.emplace_back(payments::mojom::ImageObject::New( GURL(icon.src())); https://codereview.chromium.org/2925063003/diff/200001/content/browser/paymen... content/browser/payments/payment_app_database.cc:204: base::Passed(std::move(instrument)), "", Nit: prefer std::string() instead of "" (there's a few instances of this in other files as well) std::string() is a bit more efficient than "", since it doesn't require a memcpy and/or allocation https://codereview.chromium.org/2925063003/diff/200001/content/browser/paymen... File content/browser/payments/payment_instrument_icon_fetcher.cc (right): https://codereview.chromium.org/2925063003/diff/200001/content/browser/paymen... content/browser/payments/payment_instrument_icon_fetcher.cc:61: image_objects_.emplace_back(std::move(image_object)); Can this take |image_objects| by value, so we can just std::move() it? https://codereview.chromium.org/2925063003/diff/200001/content/browser/paymen... content/browser/payments/payment_instrument_icon_fetcher.cc:69: base::Bind(&PaymentInstrumentIconFetcher::StartFromUIThread, Nit: might be nice to use base::BindOnce here, since this will only be called once. https://codereview.chromium.org/2925063003/diff/200001/content/browser/paymen... content/browser/payments/payment_instrument_icon_fetcher.cc:70: base::Unretained(this), service_worker_context)); Please document why Unretained is safe here. https://codereview.chromium.org/2925063003/diff/200001/content/browser/paymen... content/browser/payments/payment_instrument_icon_fetcher.cc:70: base::Unretained(this), service_worker_context)); Also, please use std::move(service_worker_context) since we're passing |service_worker_context| by value here. https://codereview.chromium.org/2925063003/diff/200001/content/browser/paymen... content/browser/payments/payment_instrument_icon_fetcher.cc:77: url_request_context_getter_ = scoped_refptr<net::URLRequestContextGetter>( It's pretty unusual to see this written out explicitly. I would suggest using: url_request_context_getter_.reset( service_worker_context->storage_partition()->GetURLRequestContext()); https://codereview.chromium.org/2925063003/diff/200001/content/browser/paymen... content/browser/payments/payment_instrument_icon_fetcher.cc:79: if (url_request_context_getter_.get() == nullptr) { Nit: just if (url_request_context_getter_) here https://codereview.chromium.org/2925063003/diff/200001/content/browser/paymen... content/browser/payments/payment_instrument_icon_fetcher.cc:145: if (bitmap.isNull() || bitmap.empty()) { Use bitmap.drawsNothing(), which does both of these checks. https://codereview.chromium.org/2925063003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp (right): https://codereview.chromium.org/2925063003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp:231: instrument.setIcons(icons); On 2017/06/09 23:13:22, haraken wrote: > > dcheng@: If we add a type mapping between mojom::blink::PaymentInstrumentPtr and > PaymentInstrument, can we probably remove this code? Unfortunately it's not easy to typemap from a mojo struct to a type generated for IDL =/ There are circular dependencies that need to be resolved. I don't think this can be done without breaking apart the modules target =/
The CQ bit was checked by gogerald@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 #6 (id:220001) has been deleted
The CQ bit was checked by gogerald@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...
Thanks, another look? https://codereview.chromium.org/2925063003/diff/200001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestServiceWorkerPaymentAppTest.java (right): https://codereview.chromium.org/2925063003/diff/200001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestServiceWorkerPaymentAppTest.java:69: "https://bobpay.com", "basic-card")) /* methodNames */, On 2017/06/10 00:37:10, dcheng wrote: > Nit: this should probably using something like http://example.com. Using random domains > is a bit dangerous. This is not a complete random domain, it is a fake payment method used everywhere to explain and test payment request (even in spec). https://cs.chromium.org/search/?q=bobpay&sq=package:chromium&type=cs We've created a public test website for it as well, https://bobpay.xyz/ So I would like not change it at least in this CL https://codereview.chromium.org/2925063003/diff/200001/components/payments/mo... File components/payments/mojom/payment_app.mojom (right): https://codereview.chromium.org/2925063003/diff/200001/components/payments/mo... components/payments/mojom/payment_app.mojom:24: struct PaymentInstrument { On 2017/06/10 00:37:10, dcheng wrote: > In general, this mojom file needs more comments. It's not clear what interfaces > live in the browser vs the renderer or some other process, nor what the > different structs mean / what they correspond to. Done. Add few simple comments. https://codereview.chromium.org/2925063003/diff/200001/content/browser/paymen... File content/browser/payments/payment_app_database.cc (right): https://codereview.chromium.org/2925063003/diff/200001/content/browser/paymen... content/browser/payments/payment_app_database.cc:67: image_object->src = GURL(icon.src()); On 2017/06/10 00:37:10, dcheng wrote: > I'm a bit confused about how the payment flow works. Do the icon images > originally come from the renderer? From some external source? Why does it start > in proto form? etc. PaymentInstrument was registered from renderer, but we also have to provide a method to get registered PaymentInstrument for renderer. This is part of the spec. https://codereview.chromium.org/2925063003/diff/200001/content/browser/paymen... content/browser/payments/payment_app_database.cc:68: instrument->icons.emplace_back(std::move(image_object)); On 2017/06/10 00:37:10, dcheng wrote: > Note: New() has an overload that takes arguments, so it's possible to write: > > instrument->icons.emplace_back(payments::mojom::ImageObject::New( > GURL(icon.src())); Done. https://codereview.chromium.org/2925063003/diff/200001/content/browser/paymen... content/browser/payments/payment_app_database.cc:204: base::Passed(std::move(instrument)), "", On 2017/06/10 00:37:10, dcheng wrote: > Nit: prefer std::string() instead of "" (there's a few instances of this in > other files as well) > > std::string() is a bit more efficient than "", since it doesn't require a memcpy > and/or allocation Done. https://codereview.chromium.org/2925063003/diff/200001/content/browser/paymen... File content/browser/payments/payment_instrument_icon_fetcher.cc (right): https://codereview.chromium.org/2925063003/diff/200001/content/browser/paymen... content/browser/payments/payment_instrument_icon_fetcher.cc:61: image_objects_.emplace_back(std::move(image_object)); On 2017/06/10 00:37:10, dcheng wrote: > Can this take |image_objects| by value, so we can just std::move() it? Done. https://codereview.chromium.org/2925063003/diff/200001/content/browser/paymen... content/browser/payments/payment_instrument_icon_fetcher.cc:69: base::Bind(&PaymentInstrumentIconFetcher::StartFromUIThread, On 2017/06/10 00:37:10, dcheng wrote: > Nit: might be nice to use base::BindOnce here, since this will only be called > once. Done. https://codereview.chromium.org/2925063003/diff/200001/content/browser/paymen... content/browser/payments/payment_instrument_icon_fetcher.cc:70: base::Unretained(this), service_worker_context)); On 2017/06/10 00:37:10, dcheng wrote: > Also, please use std::move(service_worker_context) since we're passing > |service_worker_context| by value here. Done. https://codereview.chromium.org/2925063003/diff/200001/content/browser/paymen... content/browser/payments/payment_instrument_icon_fetcher.cc:70: base::Unretained(this), service_worker_context)); On 2017/06/10 00:37:10, dcheng wrote: > Please document why Unretained is safe here. Done. https://codereview.chromium.org/2925063003/diff/200001/content/browser/paymen... content/browser/payments/payment_instrument_icon_fetcher.cc:77: url_request_context_getter_ = scoped_refptr<net::URLRequestContextGetter>( On 2017/06/10 00:37:10, dcheng wrote: > It's pretty unusual to see this written out explicitly. I would suggest using: > url_request_context_getter_.reset( > service_worker_context->storage_partition()->GetURLRequestContext()); Done. scoped_refptr has no reset method, but I've simplified the expression. https://codereview.chromium.org/2925063003/diff/200001/content/browser/paymen... content/browser/payments/payment_instrument_icon_fetcher.cc:79: if (url_request_context_getter_.get() == nullptr) { On 2017/06/10 00:37:10, dcheng wrote: > Nit: just if (url_request_context_getter_) here Done. https://codereview.chromium.org/2925063003/diff/200001/content/browser/paymen... content/browser/payments/payment_instrument_icon_fetcher.cc:145: if (bitmap.isNull() || bitmap.empty()) { On 2017/06/10 00:37:10, dcheng wrote: > Use bitmap.drawsNothing(), which does both of these checks. Done. https://codereview.chromium.org/2925063003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/ImageObject.idl (right): https://codereview.chromium.org/2925063003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/ImageObject.idl:5: // https://github.com/w3c/payment-handler/pull/174/files/6e6768c423e24719cb0e592... On 2017/06/09 23:13:22, haraken wrote: > > Can we add a more stable URL instead of a specific commit? > This URL is used deliberately according to rouslan's suggestion since the spec is not updated officially. I will update this CL when the spec is updated. https://codereview.chromium.org/2925063003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp (right): https://codereview.chromium.org/2925063003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp:231: instrument.setIcons(icons); On 2017/06/10 00:37:10, dcheng wrote: > On 2017/06/09 23:13:22, haraken wrote: > > > > dcheng@: If we add a type mapping between mojom::blink::PaymentInstrumentPtr > and > > PaymentInstrument, can we probably remove this code? > > Unfortunately it's not easy to typemap from a mojo struct to a type generated > for IDL =/ > > There are circular dependencies that need to be resolved. I don't think this can > be done without breaking apart the modules target =/ Acknowledged. https://codereview.chromium.org/2925063003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp:231: instrument.setIcons(icons); On 2017/06/09 23:13:22, haraken wrote: > > dcheng@: If we add a type mapping between mojom::blink::PaymentInstrumentPtr and > PaymentInstrument, can we probably remove this code? Acknowledged.
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_...)
https://codereview.chromium.org/2925063003/diff/200001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestServiceWorkerPaymentAppTest.java (right): https://codereview.chromium.org/2925063003/diff/200001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestServiceWorkerPaymentAppTest.java:69: "https://bobpay.com", "basic-card")) /* methodNames */, On 2017/06/12 16:19:43, gogerald1 wrote: > On 2017/06/10 00:37:10, dcheng wrote: > > Nit: this should probably using something like http://example.com. Using > random domains > > is a bit dangerous. > > This is not a complete random domain, it is a fake payment method used > everywhere to explain and test payment request (even in spec). > https://cs.chromium.org/search/?q=bobpay&sq=package:chromium&type=cs > > We've created a public test website for it as well, https://bobpay.xyz/ > > So I would like not change it at least in this CL Can we change it to point to the test domain we control rather than a random third party domain? https://codereview.chromium.org/2925063003/diff/240001/components/payments/mo... File components/payments/mojom/payment_app.mojom (right): https://codereview.chromium.org/2925063003/diff/240001/components/payments/mo... components/payments/mojom/payment_app.mojom:20: // This struct is provided to hold an image object from render side (ImageObject.idl). Nit: 80 chars https://codereview.chromium.org/2925063003/diff/240001/components/payments/mo... components/payments/mojom/payment_app.mojom:25: // This struct is provided to hold a payment instrument from render side (PaymentInstrument.idl). To elaborate: my questions are more of "what is a payment instrument? What does it represent? What are the methods and capabilities that are the fields"? As someone who's not familiar with the payments spec, these fields are pretty mysterious. https://codereview.chromium.org/2925063003/diff/240001/components/payments/mo... components/payments/mojom/payment_app.mojom:52: url.mojom.Url top_level_origin; Similarly, what are the two origins here, why do we need both, what are they used for https://codereview.chromium.org/2925063003/diff/240001/components/payments/mo... components/payments/mojom/payment_app.mojom:54: string payment_request_id; Is there any structure to the payment request IDs? What can be in method data below? What are modifiers? What's the instrument key? https://codereview.chromium.org/2925063003/diff/240001/content/browser/paymen... File content/browser/payments/payment_app_database.cc (right): https://codereview.chromium.org/2925063003/diff/240001/content/browser/paymen... content/browser/payments/payment_app_database.cc:189: instrument_icon_fetcher_ = new PaymentInstrumentIconFetcher(); Nit: base::MakeRefCounted<PaymentInstrumentIconFetcher>(); https://codereview.chromium.org/2925063003/diff/240001/content/browser/paymen... content/browser/payments/payment_app_database.cc:448: image_object_proto->set_src(image_object->src.spec()); Why do we persist both the urls and the decoded image? I think this is one of the things that confused me when I initially read the code https://codereview.chromium.org/2925063003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp (right): https://codereview.chromium.org/2925063003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp:158: instrument->icons.push_back(payments::mojom::blink::ImageObject::New()); Note: it's possible to combine this into one expression: instruments->icons.push_back( payments::mojom::blink::ImageObject::New(parser_url)); Up to you if you think it's worth it.
The CQ bit was checked by gogerald@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...
another look? https://codereview.chromium.org/2925063003/diff/200001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestServiceWorkerPaymentAppTest.java (right): https://codereview.chromium.org/2925063003/diff/200001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestServiceWorkerPaymentAppTest.java:69: "https://bobpay.com", "basic-card")) /* methodNames */, On 2017/06/13 09:02:55, dcheng wrote: > On 2017/06/12 16:19:43, gogerald1 wrote: > > On 2017/06/10 00:37:10, dcheng wrote: > > > Nit: this should probably using something like http://example.com. Using > > random domains > > > is a bit dangerous. > > > > This is not a complete random domain, it is a fake payment method used > > everywhere to explain and test payment request (even in spec). > > https://cs.chromium.org/search/?q=bobpay&sq=package:chromium&type=cs > > > > We've created a public test website for it as well, https://bobpay.xyz/ > > > > So I would like not change it at least in this CL > > Can we change it to point to the test domain we control rather than a random > third party domain? Not completely understand what's the difference of using "https://bobpay.com" and "https://example.com". They are used as method names instead of domains here. Both of them are fake names. But "https://bobpay.com" is the name we used everywhere in our code and document as example. Anyway, this is not directly related to this CL and we already used it everywhere, so I would like to not change it at least in this CL if you don't mind. https://codereview.chromium.org/2925063003/diff/240001/components/payments/mo... File components/payments/mojom/payment_app.mojom (right): https://codereview.chromium.org/2925063003/diff/240001/components/payments/mo... components/payments/mojom/payment_app.mojom:20: // This struct is provided to hold an image object from render side (ImageObject.idl). On 2017/06/13 09:02:55, dcheng wrote: > Nit: 80 chars Done. https://codereview.chromium.org/2925063003/diff/240001/components/payments/mo... components/payments/mojom/payment_app.mojom:25: // This struct is provided to hold a payment instrument from render side (PaymentInstrument.idl). On 2017/06/13 09:02:55, dcheng wrote: > To elaborate: my questions are more of "what is a payment instrument? What does > it represent? What are the methods and capabilities that are the fields"? > > As someone who's not familiar with the payments spec, these fields are pretty > mysterious. These structures and interfaces are almost exactly match the structures and interfaces in the spec, so I think the best place to understand them is the spec. We already put the links in the corresponding *.idl, like here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/paymen... Someone who want read or change these codes have to know the spec first. Make sense? https://codereview.chromium.org/2925063003/diff/240001/components/payments/mo... components/payments/mojom/payment_app.mojom:52: url.mojom.Url top_level_origin; On 2017/06/13 09:02:55, dcheng wrote: > Similarly, what are the two origins here, why do we need both, what are they > used for Please refer the spec here https://w3c.github.io/payment-handler/#the-paymentrequestevent https://codereview.chromium.org/2925063003/diff/240001/components/payments/mo... components/payments/mojom/payment_app.mojom:54: string payment_request_id; On 2017/06/13 09:02:55, dcheng wrote: > Is there any structure to the payment request IDs? What can be in method data > below? What are modifiers? What's the instrument key? Please refer the spec here https://w3c.github.io/payment-handler/#the-paymentrequestevent https://codereview.chromium.org/2925063003/diff/240001/content/browser/paymen... File content/browser/payments/payment_app_database.cc (right): https://codereview.chromium.org/2925063003/diff/240001/content/browser/paymen... content/browser/payments/payment_app_database.cc:189: instrument_icon_fetcher_ = new PaymentInstrumentIconFetcher(); On 2017/06/13 09:02:55, dcheng wrote: > Nit: base::MakeRefCounted<PaymentInstrumentIconFetcher>(); Done. https://codereview.chromium.org/2925063003/diff/240001/content/browser/paymen... content/browser/payments/payment_app_database.cc:448: image_object_proto->set_src(image_object->src.spec()); On 2017/06/13 09:02:55, dcheng wrote: > Why do we persist both the urls and the decoded image? I think this is one of > the things that confused me when I initially read the code The caller could get payment instrument data back according to the spec (https://w3c.github.io/payment-handler/#paymentinstruments-interface). The icon is used to show the payment instrument to user. We persist it to avoid fetch and decode it every time when the payment instrument is showing. That will delay payment response. https://codereview.chromium.org/2925063003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp (right): https://codereview.chromium.org/2925063003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/payments/PaymentInstruments.cpp:158: instrument->icons.push_back(payments::mojom::blink::ImageObject::New()); On 2017/06/13 09:02:55, dcheng wrote: > Note: it's possible to combine this into one expression: > > instruments->icons.push_back( > payments::mojom::blink::ImageObject::New(parser_url)); > > Up to you if you think it's worth it. Acknowledged. Looks a little more readable,
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_...)
ipc lgtm https://codereview.chromium.org/2925063003/diff/200001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestServiceWorkerPaymentAppTest.java (right): https://codereview.chromium.org/2925063003/diff/200001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestServiceWorkerPaymentAppTest.java:69: "https://bobpay.com", "basic-card")) /* methodNames */, On 2017/06/13 15:37:19, gogerald1 wrote: > On 2017/06/13 09:02:55, dcheng wrote: > > On 2017/06/12 16:19:43, gogerald1 wrote: > > > On 2017/06/10 00:37:10, dcheng wrote: > > > > Nit: this should probably using something like http://example.com. Using > > > random domains > > > > is a bit dangerous. > > > > > > This is not a complete random domain, it is a fake payment method used > > > everywhere to explain and test payment request (even in spec). > > > https://cs.chromium.org/search/?q=bobpay&sq=package:chromium&type=cs > > > > > > We've created a public test website for it as well, https://bobpay.xyz/ > > > > > > So I would like not change it at least in this CL > > > > Can we change it to point to the test domain we control rather than a random > > third party domain? > > Not completely understand what's the difference of using "https://bobpay.com" > and "https://example.com". They are used as method names instead of domains > here. Both of them are fake names. But "https://bobpay.com" is the name we used > everywhere in our code and document as example. > > Anyway, this is not directly related to this CL and we already used it > everywhere, so I would like to not change it at least in this CL if you don't > mind. bobpay.com => points to a parked domain bobpay.xyz => points to demo app seems like we should point it to the demo app at least? We can certainly fix the others in a followup, but fixing the ones we're changing anyway seems reasonable. https://codereview.chromium.org/2925063003/diff/240001/components/payments/mo... File components/payments/mojom/payment_app.mojom (right): https://codereview.chromium.org/2925063003/diff/240001/components/payments/mo... components/payments/mojom/payment_app.mojom:25: // This struct is provided to hold a payment instrument from render side (PaymentInstrument.idl). On 2017/06/13 15:37:19, gogerald1 wrote: > On 2017/06/13 09:02:55, dcheng wrote: > > To elaborate: my questions are more of "what is a payment instrument? What > does > > it represent? What are the methods and capabilities that are the fields"? > > > > As someone who's not familiar with the payments spec, these fields are pretty > > mysterious. > > These structures and interfaces are almost exactly match the structures and > interfaces in the spec, so I think the best place to understand them is the > spec. We already put the links in the corresponding *.idl, like here > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/paymen... > Someone who want read or change these codes have to know the spec first. > Make sense? - Web specs are not the easiest thing to understand - It's not helpful to jump between pages (that might disappear) when trying to understand code - Seems contrary to the guidelines to have more comments for non-obvious things (https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/blink-...) But we can address this in a followup.
Hi dcheng@, I'm not sure if my comment is helpful, but I hope it helps you. Something in details can be wrong but big-picture will be right. https://codereview.chromium.org/2925063003/diff/240001/components/payments/mo... File components/payments/mojom/payment_app.mojom (right): https://codereview.chromium.org/2925063003/diff/240001/components/payments/mo... components/payments/mojom/payment_app.mojom:25: // This struct is provided to hold a payment instrument from render side (PaymentInstrument.idl). On 2017/06/13 09:02:55, dcheng wrote: > To elaborate: my questions are more of "what is a payment instrument? What does > it represent? What are the methods and capabilities that are the fields"? > > As someone who's not familiar with the payments spec, these fields are pretty > mysterious. There are two specs PaymentRequest API and PaymentHandler API. Term: UA (You already know well :-) ) Merchant (e.g. Amazon) Third-party Payment App Provider (e.g. PayPal) The PaymentRequest API is a mediator role between them. Traditionally, mechants had to connect to third-party providers for payment directly. It made so many each different ways for connection for a long time. But now, mechants can use PaymentRequest API for payment. The API doesn't provide any payment methods but can connect to third-party providers indirectly. Actual payment process will run on server backend in merchant side but this way has some advances. (e.g. improving UX, reducing integration efforts) On the other hands, PaymentHandler API provides a way to make third-party payment apps based on the web-standard way. This patch is related to the PaymentHandler API and the PaymentInstrument is an actual payment instrument such as credit card, bit coin, mileage or other things. The PaymentInstrument data will be used to display instrument UI[1] on PaymentSheet UI created by PaymentRequest API. FYI, PaymentHandler API is implemented by extending service worker like push API. So, third-party providers can write codes to provider payment app as follows: navgiator.serviceWorker.register('payment.js') .then(...) .then(registration => { registration.paymentManager.paymentInstrument.set({ name: "Test Pay", icons: [{ ... }] }); }); Once above code runs via user interaction(accessing the payment app provider site? or something?), then the payment instrument data is stored in browser. and then browser can display the stored payment instruments when some payment request is initiated from merchant site. Although it's an old thing, this PPT might help your understanding. - https://docs.google.com/presentation/d/1rQ4SW3huSTKOSHc97ebnhYWa7UfJt4swHeFlS... [1] The screenshot was provided in this bug tracking: https://bugs.chromium.org/p/chromium/issues/detail?id=720029
> alexmos@, Please review changes in > content/browser/BUILD.gn > content/public/browser/stored_payment_instrument.h Those two files LGTM
LGTM
Thanks all for reviewing, especially zino@ for further explanation, sending to CQ for now. https://codereview.chromium.org/2925063003/diff/200001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestServiceWorkerPaymentAppTest.java (right): https://codereview.chromium.org/2925063003/diff/200001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestServiceWorkerPaymentAppTest.java:69: "https://bobpay.com", "basic-card")) /* methodNames */, On 2017/06/13 17:33:15, dcheng wrote: > On 2017/06/13 15:37:19, gogerald1 wrote: > > On 2017/06/13 09:02:55, dcheng wrote: > > > On 2017/06/12 16:19:43, gogerald1 wrote: > > > > On 2017/06/10 00:37:10, dcheng wrote: > > > > > Nit: this should probably using something like http://example.com. Using > > > > random domains > > > > > is a bit dangerous. > > > > > > > > This is not a complete random domain, it is a fake payment method used > > > > everywhere to explain and test payment request (even in spec). > > > > https://cs.chromium.org/search/?q=bobpay&sq=package:chromium&type=cs > > > > > > > > We've created a public test website for it as well, https://bobpay.xyz/ > > > > > > > > So I would like not change it at least in this CL > > > > > > Can we change it to point to the test domain we control rather than a random > > > third party domain? > > > > Not completely understand what's the difference of using "https://bobpay.com" > > and "https://example.com". They are used as method names instead of domains > > here. Both of them are fake names. But "https://bobpay.com" is the name we > used > > everywhere in our code and document as example. > > > > Anyway, this is not directly related to this CL and we already used it > > everywhere, so I would like to not change it at least in this CL if you don't > > mind. > > http://bobpay.com => points to a parked domain > bobpay.xyz => points to demo app > > seems like we should point it to the demo app at least? We can certainly fix the > others in a followup, but fixing the ones we're changing anyway seems > reasonable. Acknowledged. will bring it up in our team to discuss whether we want to make these changes, https://codereview.chromium.org/2925063003/diff/240001/components/payments/mo... File components/payments/mojom/payment_app.mojom (right): https://codereview.chromium.org/2925063003/diff/240001/components/payments/mo... components/payments/mojom/payment_app.mojom:25: // This struct is provided to hold a payment instrument from render side (PaymentInstrument.idl). On 2017/06/13 17:33:15, dcheng wrote: > On 2017/06/13 15:37:19, gogerald1 wrote: > > On 2017/06/13 09:02:55, dcheng wrote: > > > To elaborate: my questions are more of "what is a payment instrument? What > > does > > > it represent? What are the methods and capabilities that are the fields"? > > > > > > As someone who's not familiar with the payments spec, these fields are > pretty > > > mysterious. > > > > These structures and interfaces are almost exactly match the structures and > > interfaces in the spec, so I think the best place to understand them is the > > spec. We already put the links in the corresponding *.idl, like here > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/paymen... > > Someone who want read or change these codes have to know the spec first. > > Make sense? > > - Web specs are not the easiest thing to understand > - It's not helpful to jump between pages (that might disappear) when trying to > understand code > - Seems contrary to the guidelines to have more comments for non-obvious things > (https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/blink-...) > > But we can address this in a followup. Acknowledged.
The CQ bit was checked by gogerald@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org, jinho.bang@samsung.com Link to the patchset: https://codereview.chromium.org/2925063003/#ps260001 (title: "fix comment lines")
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": 260001, "attempt_start_ts": 1497403705838940,
"parent_rev": "c3c82afc38710daaba052350e225cfb7cbdf81d3", "commit_rev":
"d8c5af3464ac1f827f8e673095c8e24496eea1da"}
Message was sent while issue was closed.
Description was changed from ========== Implement payment instrument icons Tested bobpay: https://github.com/gogerald/gogerald.github.io/blob/master/pr/bobpaywebapp/re... The spec: https://github.com/w3c/payment-handler/pull/174 BUG=720029 ========== to ========== Implement payment instrument icons Tested bobpay: https://github.com/gogerald/gogerald.github.io/blob/master/pr/bobpaywebapp/re... The spec: https://github.com/w3c/payment-handler/pull/174 BUG=720029 Review-Url: https://codereview.chromium.org/2925063003 Cr-Commit-Position: refs/heads/master@{#479258} Committed: https://chromium.googlesource.com/chromium/src/+/d8c5af3464ac1f827f8e673095c8... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:260001) as https://chromium.googlesource.com/chromium/src/+/d8c5af3464ac1f827f8e673095c8... |
