Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2838433002:
[Payments] Cache payment manifests. (Closed)
|
Created:
3 years, 8 months ago by gogerald1 Modified:
3 years, 7 months ago CC:
chromium-reviews, rouslan+autofill_chromium.org, rogerm+autofillwatch_chromium.org, sebsg+autofillwatch_chromium.org, jam, gogerald+paymentswatch_chromium.org, rouslan+payments_chromium.org, darin-cc_chromium.org, mathp+autofillwatch_chromium.org, asvitkine+watch_chromium.org, agrieve+watch_chromium.org, mahmadi+paymentswatch_chromium.org, estade+watch_chromium.org, vabr+watchlistautofill_chromium.org, sebsg+paymentswatch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionCache payment manifests.
In this patch:
Try to fetch all supported web app ids for the payment method from
the cache before downloading its manifest online. If cached web app
ids contain all matching apps ids, proceed fetching corresponding web
apps manifests from the cache. Otherwise fall back to download the
payment method's manifest online. If failed to fetch at least one web
app's manifest from the cache, then fall back to download payment
method's manifest online.
In addition:
Cache downloaded and parsed supported web apps ids of the payment
method for future reference. Cache downloaded and parsed web app
manifest sections for future reference.
Refer to below doc for details.
https://docs.google.com/a/google.com/document/d/1Ncsp96Ae5836NLOdPwHRzcX3zkgj-4-5fq_nI2azC7M
BUG=708508
Review-Url: https://codereview.chromium.org/2838433002
Cr-Commit-Position: refs/heads/master@{#467688}
Committed: https://chromium.googlesource.com/chromium/src/+/7947209945cff83f3301963f5a01821d02e100a2
Patch Set 1 #Patch Set 2 : fix tests #
Total comments: 94
Patch Set 3 : address comments #
Total comments: 34
Patch Set 4 : fix variable names and comments #Patch Set 5 : rebase #Messages
Total messages: 78 (57 generated)
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...
Description was changed from ========== Cache payment manifests to accelerate PR response BUG= ========== to ========== Cache payment method and web app's manifest to accelerate PR response BUG= ==========
Description was changed from ========== Cache payment method and web app's manifest to accelerate PR response BUG= ========== to ========== Use cached payment method and web app's manifest to accelerate PR response BUG= ==========
Description was changed from ========== Use cached payment method and web app's manifest to accelerate PR response BUG= ========== to ========== Use cached payment method and web app's manifest to accelerate PR response BUG=708508 ==========
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: + rouslan@chromium.org
Hi rouslan@, ptal,
Please put some more information in the CL description. Can you make sure the design doc is up-to-date and link to it as well?
Description was changed from ========== Use cached payment method and web app's manifest to accelerate PR response BUG=708508 ========== to ========== Use cached payment method and web app's manifest to accelerate PR response In this patch: Try to fetch supported web apps Ids from the cache for the payment method before downloading its manifest online. If cached web apps Ids contain all matching apps Ids, proceed fetching correspond web apps manifests from the cache. Otherwise start downloading the payment method's manifest online. If failed to fetch at least one web app's manifest from the cache, then fall back to downloading payment method's manifest online BUG=708508 ==========
Description was changed from ========== Use cached payment method and web app's manifest to accelerate PR response In this patch: Try to fetch supported web apps Ids from the cache for the payment method before downloading its manifest online. If cached web apps Ids contain all matching apps Ids, proceed fetching correspond web apps manifests from the cache. Otherwise start downloading the payment method's manifest online. If failed to fetch at least one web app's manifest from the cache, then fall back to downloading payment method's manifest online BUG=708508 ========== to ========== Use cached payment method and web app's manifest to accelerate PR response In this patch: Try to fetch all supported web apps Ids for the payment method from the cache before downloading its manifest online. If cached web apps Ids contain all matching apps Ids, proceed fetching correspond web apps manifests from the cache. Otherwise fall back to download the payment method's manifest online. If failed to fetch at least one web app's manifest from the cache, then fall back to download payment method's manifest online BUG=708508 ==========
Description was changed from ========== Use cached payment method and web app's manifest to accelerate PR response In this patch: Try to fetch all supported web apps Ids for the payment method from the cache before downloading its manifest online. If cached web apps Ids contain all matching apps Ids, proceed fetching correspond web apps manifests from the cache. Otherwise fall back to download the payment method's manifest online. If failed to fetch at least one web app's manifest from the cache, then fall back to download payment method's manifest online BUG=708508 ========== to ========== Use cached payment method and web app's manifest to accelerate PR response In this patch: Try to fetch all supported web apps Ids for the payment method from the cache before downloading its manifest online. If cached web apps Ids contain all matching apps Ids, proceed fetching correspond web apps manifests from the cache. Otherwise fall back to download the payment method's manifest online. If failed to fetch at least one web app's manifest from the cache, then fall back to download payment method's manifest online In addition: Cache downloaded and parsed supported web apps Ids of the payment method for future reference. Cache downloaded and parsed web app manifest sections for future reference BUG=708508 ==========
Description was changed from ========== Use cached payment method and web app's manifest to accelerate PR response In this patch: Try to fetch all supported web apps Ids for the payment method from the cache before downloading its manifest online. If cached web apps Ids contain all matching apps Ids, proceed fetching correspond web apps manifests from the cache. Otherwise fall back to download the payment method's manifest online. If failed to fetch at least one web app's manifest from the cache, then fall back to download payment method's manifest online In addition: Cache downloaded and parsed supported web apps Ids of the payment method for future reference. Cache downloaded and parsed web app manifest sections for future reference BUG=708508 ========== to ========== Use cached payment method and web app's manifest to accelerate PR response In this patch: Try to fetch all supported web apps Ids for the payment method from the cache before downloading its manifest online. If cached web apps Ids contain all matching apps Ids, proceed fetching correspond web apps manifests from the cache. Otherwise fall back to download the payment method's manifest online. If failed to fetch at least one web app's manifest from the cache, then fall back to download payment method's manifest online In addition: Cache downloaded and parsed supported web apps Ids of the payment method for future reference. Cache downloaded and parsed web app manifest sections for future reference. BUG=708508 ==========
Description was changed from ========== Use cached payment method and web app's manifest to accelerate PR response In this patch: Try to fetch all supported web apps Ids for the payment method from the cache before downloading its manifest online. If cached web apps Ids contain all matching apps Ids, proceed fetching correspond web apps manifests from the cache. Otherwise fall back to download the payment method's manifest online. If failed to fetch at least one web app's manifest from the cache, then fall back to download payment method's manifest online In addition: Cache downloaded and parsed supported web apps Ids of the payment method for future reference. Cache downloaded and parsed web app manifest sections for future reference. BUG=708508 ========== to ========== Use cached payment method and web app's manifest to accelerate PR response In this patch: Try to fetch all supported web apps Ids for the payment method from the cache before downloading its manifest online. If cached web apps Ids contain all matching apps Ids, proceed fetching correspond web apps manifests from the cache. Otherwise fall back to download the payment method's manifest online. If failed to fetch at least one web app's manifest from the cache, then fall back to download payment method's manifest online In addition: Cache downloaded and parsed supported web apps Ids of the payment method for future reference. Cache downloaded and parsed web app manifest sections for future reference. refer below doc for details. https://docs.google.com/a/google.com/document/d/1Ncsp96Ae5836NLOdPwHRzcX3zkgj... BUG=708508 ==========
Description was changed from ========== Use cached payment method and web app's manifest to accelerate PR response In this patch: Try to fetch all supported web apps Ids for the payment method from the cache before downloading its manifest online. If cached web apps Ids contain all matching apps Ids, proceed fetching correspond web apps manifests from the cache. Otherwise fall back to download the payment method's manifest online. If failed to fetch at least one web app's manifest from the cache, then fall back to download payment method's manifest online In addition: Cache downloaded and parsed supported web apps Ids of the payment method for future reference. Cache downloaded and parsed web app manifest sections for future reference. refer below doc for details. https://docs.google.com/a/google.com/document/d/1Ncsp96Ae5836NLOdPwHRzcX3zkgj... BUG=708508 ========== to ========== Use cached payment method and web app's manifest to accelerate PR response In this patch: Try to fetch all supported web apps Ids for the payment method from the cache before downloading its manifest online. If cached web apps Ids contain all matching apps Ids, proceed fetching correspond web apps manifests from the cache. Otherwise fall back to download the payment method's manifest online. If failed to fetch at least one web app's manifest from the cache, then fall back to download payment method's manifest online In addition: Cache downloaded and parsed supported web apps Ids of the payment method for future reference. Cache downloaded and parsed web app manifest sections for future reference. Refer below doc for details. https://docs.google.com/a/google.com/document/d/1Ncsp96Ae5836NLOdPwHRzcX3zkgj... BUG=708508 ==========
On 2017/04/21 19:28:23, ಠ_ಠ wrote: > Please put some more information in the CL description. Can you make sure the > design doc is up-to-date and link to it as well? Updated the CL description, ptal. In the mean time, I will update the design doc.
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 ========== Use cached payment method and web app's manifest to accelerate PR response In this patch: Try to fetch all supported web apps Ids for the payment method from the cache before downloading its manifest online. If cached web apps Ids contain all matching apps Ids, proceed fetching correspond web apps manifests from the cache. Otherwise fall back to download the payment method's manifest online. If failed to fetch at least one web app's manifest from the cache, then fall back to download payment method's manifest online In addition: Cache downloaded and parsed supported web apps Ids of the payment method for future reference. Cache downloaded and parsed web app manifest sections for future reference. Refer below doc for details. https://docs.google.com/a/google.com/document/d/1Ncsp96Ae5836NLOdPwHRzcX3zkgj... BUG=708508 ========== to ========== Cache payment manifests. In this patch: Try to fetch all supported web apps Ids for the payment method from the cache before downloading its manifest online. If cached web apps Ids contain all matching apps Ids, proceed fetching correspond web apps manifests from the cache. Otherwise fall back to download the payment method's manifest online. If failed to fetch at least one web app's manifest from the cache, then fall back to download payment method's manifest online In addition: Cache downloaded and parsed supported web apps Ids of the payment method for future reference. Cache downloaded and parsed web app manifest sections for future reference. Refer below doc for details. https://docs.google.com/a/google.com/document/d/1Ncsp96Ae5836NLOdPwHRzcX3zkgj... BUG=708508 ==========
Description was changed from ========== Cache payment manifests. In this patch: Try to fetch all supported web apps Ids for the payment method from the cache before downloading its manifest online. If cached web apps Ids contain all matching apps Ids, proceed fetching correspond web apps manifests from the cache. Otherwise fall back to download the payment method's manifest online. If failed to fetch at least one web app's manifest from the cache, then fall back to download payment method's manifest online In addition: Cache downloaded and parsed supported web apps Ids of the payment method for future reference. Cache downloaded and parsed web app manifest sections for future reference. Refer below doc for details. https://docs.google.com/a/google.com/document/d/1Ncsp96Ae5836NLOdPwHRzcX3zkgj... BUG=708508 ========== to ========== Cache payment manifests. In this patch: Try to fetch all supported web apps Ids for the payment method from the cache before downloading its manifest online. If cached web apps Ids contain all matching apps Ids, proceed fetching correspond web apps manifests from the cache. Otherwise fall back to download the payment method's manifest online. If failed to fetch at least one web app's manifest from the cache, then fall back to download payment method's manifest online. In addition: Cache downloaded and parsed supported web apps Ids of the payment method for future reference. Cache downloaded and parsed web app manifest sections for future reference. Refer to below doc for details. https://docs.google.com/a/google.com/document/d/1Ncsp96Ae5836NLOdPwHRzcX3zkgj... BUG=708508 ==========
Description was changed from ========== Cache payment manifests. In this patch: Try to fetch all supported web apps Ids for the payment method from the cache before downloading its manifest online. If cached web apps Ids contain all matching apps Ids, proceed fetching correspond web apps manifests from the cache. Otherwise fall back to download the payment method's manifest online. If failed to fetch at least one web app's manifest from the cache, then fall back to download payment method's manifest online. In addition: Cache downloaded and parsed supported web apps Ids of the payment method for future reference. Cache downloaded and parsed web app manifest sections for future reference. Refer to below doc for details. https://docs.google.com/a/google.com/document/d/1Ncsp96Ae5836NLOdPwHRzcX3zkgj... BUG=708508 ========== to ========== Cache payment manifests. In this patch: Try to fetch all supported web apps Ids for the payment method from the cache before downloading its manifest online. If cached web apps Ids contain all matching apps Ids, proceed fetching corresponding web apps manifests from the cache. Otherwise fall back to download the payment method's manifest online. If failed to fetch at least one web app's manifest from the cache, then fall back to download payment method's manifest online. In addition: Cache downloaded and parsed supported web apps Ids of the payment method for future reference. Cache downloaded and parsed web app manifest sections for future reference. Refer to below doc for details. https://docs.google.com/a/google.com/document/d/1Ncsp96Ae5836NLOdPwHRzcX3zkgj... BUG=708508 ==========
Description was changed from ========== Cache payment manifests. In this patch: Try to fetch all supported web apps Ids for the payment method from the cache before downloading its manifest online. If cached web apps Ids contain all matching apps Ids, proceed fetching corresponding web apps manifests from the cache. Otherwise fall back to download the payment method's manifest online. If failed to fetch at least one web app's manifest from the cache, then fall back to download payment method's manifest online. In addition: Cache downloaded and parsed supported web apps Ids of the payment method for future reference. Cache downloaded and parsed web app manifest sections for future reference. Refer to below doc for details. https://docs.google.com/a/google.com/document/d/1Ncsp96Ae5836NLOdPwHRzcX3zkgj... BUG=708508 ========== to ========== Cache payment manifests. In this patch: Try to fetch all supported web apps Ids for the payment method from the cache before downloading its manifest online. If cached web apps Ids contain all matching apps ids, proceed fetching corresponding web apps manifests from the cache. Otherwise fall back to download the payment method's manifest online. If failed to fetch at least one web app's manifest from the cache, then fall back to download payment method's manifest online. In addition: Cache downloaded and parsed supported web apps Ids of the payment method for future reference. Cache downloaded and parsed web app manifest sections for future reference. Refer to below doc for details. https://docs.google.com/a/google.com/document/d/1Ncsp96Ae5836NLOdPwHRzcX3zkgj... BUG=708508 ==========
Description was changed from ========== Cache payment manifests. In this patch: Try to fetch all supported web apps Ids for the payment method from the cache before downloading its manifest online. If cached web apps Ids contain all matching apps ids, proceed fetching corresponding web apps manifests from the cache. Otherwise fall back to download the payment method's manifest online. If failed to fetch at least one web app's manifest from the cache, then fall back to download payment method's manifest online. In addition: Cache downloaded and parsed supported web apps Ids of the payment method for future reference. Cache downloaded and parsed web app manifest sections for future reference. Refer to below doc for details. https://docs.google.com/a/google.com/document/d/1Ncsp96Ae5836NLOdPwHRzcX3zkgj... BUG=708508 ========== to ========== Cache payment manifests. In this patch: Try to fetch all supported web apps ids for the payment method from the cache before downloading its manifest online. If cached web apps Ids contain all matching apps ids, proceed fetching corresponding web apps manifests from the cache. Otherwise fall back to download the payment method's manifest online. If failed to fetch at least one web app's manifest from the cache, then fall back to download payment method's manifest online. In addition: Cache downloaded and parsed supported web apps Ids of the payment method for future reference. Cache downloaded and parsed web app manifest sections for future reference. Refer to below doc for details. https://docs.google.com/a/google.com/document/d/1Ncsp96Ae5836NLOdPwHRzcX3zkgj... BUG=708508 ==========
Description was changed from ========== Cache payment manifests. In this patch: Try to fetch all supported web apps ids for the payment method from the cache before downloading its manifest online. If cached web apps Ids contain all matching apps ids, proceed fetching corresponding web apps manifests from the cache. Otherwise fall back to download the payment method's manifest online. If failed to fetch at least one web app's manifest from the cache, then fall back to download payment method's manifest online. In addition: Cache downloaded and parsed supported web apps Ids of the payment method for future reference. Cache downloaded and parsed web app manifest sections for future reference. Refer to below doc for details. https://docs.google.com/a/google.com/document/d/1Ncsp96Ae5836NLOdPwHRzcX3zkgj... BUG=708508 ========== to ========== Cache payment manifests. In this patch: Try to fetch all supported web app ids for the payment method from the cache before downloading its manifest online. If cached web apps Ids contain all matching apps ids, proceed fetching corresponding web apps manifests from the cache. Otherwise fall back to download the payment method's manifest online. If failed to fetch at least one web app's manifest from the cache, then fall back to download payment method's manifest online. In addition: Cache downloaded and parsed supported web apps Ids of the payment method for future reference. Cache downloaded and parsed web app manifest sections for future reference. Refer to below doc for details. https://docs.google.com/a/google.com/document/d/1Ncsp96Ae5836NLOdPwHRzcX3zkgj... BUG=708508 ==========
Description was changed from ========== Cache payment manifests. In this patch: Try to fetch all supported web app ids for the payment method from the cache before downloading its manifest online. If cached web apps Ids contain all matching apps ids, proceed fetching corresponding web apps manifests from the cache. Otherwise fall back to download the payment method's manifest online. If failed to fetch at least one web app's manifest from the cache, then fall back to download payment method's manifest online. In addition: Cache downloaded and parsed supported web apps Ids of the payment method for future reference. Cache downloaded and parsed web app manifest sections for future reference. Refer to below doc for details. https://docs.google.com/a/google.com/document/d/1Ncsp96Ae5836NLOdPwHRzcX3zkgj... BUG=708508 ========== to ========== Cache payment manifests. In this patch: Try to fetch all supported web app ids for the payment method from the cache before downloading its manifest online. If cached web app ids contain all matching apps ids, proceed fetching corresponding web apps manifests from the cache. Otherwise fall back to download the payment method's manifest online. If failed to fetch at least one web app's manifest from the cache, then fall back to download payment method's manifest online. In addition: Cache downloaded and parsed supported web apps Ids of the payment method for future reference. Cache downloaded and parsed web app manifest sections for future reference. Refer to below doc for details. https://docs.google.com/a/google.com/document/d/1Ncsp96Ae5836NLOdPwHRzcX3zkgj... BUG=708508 ==========
> Try to fetch all supported web app ids for the payment > method from the cache before downloading its manifest > online. If cached web app ids contain all matching apps > ids, proceed fetching corresponding web apps manifests from > the cache. Otherwise fall back to download the payment > method's manifest online. If failed to fetch at least one > web app's manifest from the cache, then fall back to > download payment method's manifest online. Can you provide a diagram in your design doc? If the diagram is simple, an ASCII diagram in code and CL description would also be good. This algorithm is important to get right and is hard to understand. By the way, an important addition to your algorithm is to always fetch the web app manifests, even if the cached manifests contain all the necessary data. The cached manifest data should be used to show the UI quickly. The downloaded manifest data should be used to update the cache for future invocations.
A lot of work here! Good job! https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java (right): https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:94: /** A set of apps verified by using cached manifest. */ Please clarify in the comment that these are app package names. https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:95: private final Set<String> mVerifiedAppsByCachedManifest; s/Apps/PackageNames/ https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:97: /** A set of web apps Ids of the verified method supports. */ Clarify the comment that these are app package names. https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:98: private final Set<String> mSupportedWebAppsIdsOfTheMethod; s/WebAppIds/PackageNames/ https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:108: * A flag indicates whether there is a web app Id exists in the cache but failed to fetch "Fetch" is an overloaded term. Downloading can also be called fetching. But here you're talking about fetching from cache. Use the phrase "fetch from cache" instead. https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:111: private boolean mAtLeastOneWebAppManifestFailedToFetch; mIsCacheStale might be a better name. https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:203: mMethodName.toString(), this)) { Can you also make sure that the cache is used only up to a certain number of days? Let's 90 for now. If the cache is older than 90 days, clear it and redownload the web app manifest. https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:205: } Let's attempt a download regardless of cache state. The downloaded manifests should be used to refresh the cache. Like this: Get apps->Get cache->Success-------------------> Show UI. \ \->Fail---- / \ \ / ->Download manifests-> Update cache- Does my "diagram" make sense? https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:227: public void onPaymentMethodManifestFetched(String[] webAppsIds) { What string identifies a web app? https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:238: for (String app : fetchedApps) { Let's iterate over webAppIds using "for (int i = 0..." type loop, because it's more efficient than "for (String..." type loop. https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:259: String verifiedApp = verifyAppWithWebAppManifest(manifest); What is this string? A package name? https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:269: for (String app : mVerifiedAppsByCachedManifest) { You only ever add thing to mVerifiedAppsByCachedManifest and then iterate over it. An ArrayList would work better here, because then you would be able to use the more efficient "for (int i =0..." type loop here instead. https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestWebDataService.java (right): https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestWebDataService.java:31: void onPaymentWebAppManifestFetched(WebAppManifestSection[] manifest); Can you define a simpler interface that hides the fact that you have two tables? It would be preferable to query for payment method name and get back a list of (package_mame, min_version, fingerprints) tuples. Similarly, it would be preferable to define a single call to store this information on disk: saveManifests(manifests), where each item in the |manifests| is a (method_mame, package_mame, min_version, fingerprints) tuple. https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestWebDataService.java:49: private final long mManifestWebDataServiceAndroid; Please put all member variables together. https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestWebDataService.java:68: * Adds the supported web apps Ids of the method. s/web app Ids/Android app package names/g Here and everywhere. https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestWebDataService.java:80: * @param webAppId The id of the payment web app. s/webAppId/androidAppPackageName/ (or packageName) It's verify confusing to talk about web app identifiers when dealing with Android payment apps. https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestWebDataService.java:109: private native long nativeInit(); If there's initialization, then you also need destruction to avoid leaking stuff. https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestWebDataService.java:116: private native boolean nativeGetPaymentWebAppManifest( Can you group the "Get" methods together? https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/payments... File chrome/browser/payments/android/payment_manifest_downloader_android.cc (right): https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/payments... chrome/browser/payments/android/payment_manifest_downloader_android.cc:5: #include "chrome/browser/payments/android/payment_manifest_downloader_android.h" There appears to be no need to move the downloader from components/ to chrome/browser/. https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/payments... File chrome/browser/payments/android/payment_manifest_parser_android.cc (right): https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/payments... chrome/browser/payments/android/payment_manifest_parser_android.cc:96: PaymentManifestWebDataServiceAndroid::AddPaymentWebAppManifest(manifest); Please cache the manifests from PaymentMethodVerifier.java instead of from here. (Chrome should cache the manifests for https://bobpay.com only if all of them downloaded successfully.) Then please move the parser back to components. Thank you! https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/payments... File chrome/browser/payments/android/payment_manifest_web_data_service_android.cc (right): https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/payments... chrome/browser/payments/android/payment_manifest_web_data_service_android.cc:17: namespace payments { Add an empty line after the namespace start. https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/payments... chrome/browser/payments/android/payment_manifest_web_data_service_android.cc:39: case PAYMENT_WEB_APP_MANIFEST: Question: Where is this defined? https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/payments... chrome/browser/payments/android/payment_manifest_web_data_service_android.cc:57: ->GetValue()); Ouch, this is confusing. Please separate this into several statements. https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/payments... chrome/browser/payments/android/payment_manifest_web_data_service_android.cc:63: const mojom::WebAppManifestSectionPtr& section = (*manifest)[i]; manifest->get(i) is less ugly, IMHO. https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/payments... chrome/browser/payments/android/payment_manifest_web_data_service_android.cc:67: env, jmanifest.obj(), base::checked_cast<int>(i), #include "base/numerics/safe_conversions.h" https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/payments... chrome/browser/payments/android/payment_manifest_web_data_service_android.cc:90: const std::vector<std::string>* web_apps_ids = #include <string> https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/payments... chrome/browser/payments/android/payment_manifest_web_data_service_android.cc:92: ->GetValue()); Confusing. Please separate into several statements. https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/payments... File chrome/browser/payments/android/payment_manifest_web_data_service_android.h (right): https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/payments... chrome/browser/payments/android/payment_manifest_web_data_service_android.h:21: // therefore a single instance of this wrapper. This comment should be on the class, not on the namespace. https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/payments... chrome/browser/payments/android/payment_manifest_web_data_service_android.h:29: PaymentManifestWebDataServiceAndroid(JNIEnv* env, jobject obj); Need to override the parent destructor. https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/payments... chrome/browser/payments/android/payment_manifest_web_data_service_android.h:53: bool GetPaymentWebAppManifest( Please group the Get and Add methods together. https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/payments... chrome/browser/payments/android/payment_manifest_web_data_service_android.h:62: const std::vector<mojom::WebAppManifestSectionPtr>& manifest); #include <vector> #include "components/payments/mojom/payment_manifest_parser.mojom.h" https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/web_data... File chrome/browser/web_data_service_factory.h (right): https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/web_data... chrome/browser/web_data_service_factory.h:30: } // namespace payments Remove this comment to match the rest of the file. https://codereview.chromium.org/2838433002/diff/20001/components/payments/and... File components/payments/android/payment_manifest_web_data_service.cc (right): https://codereview.chromium.org/2838433002/diff/20001/components/payments/and... components/payments/android/payment_manifest_web_data_service.cc:21: const std::vector<mojom::WebAppManifestSectionPtr>& manifest) { I recommend that you change the type of this parameter to be "std::vector<mojom::WebAppManifestSectionPtr> manifest". Then you don't need to make a |manifest_copy| below. (This whole file would benefit from using the move-only type like this.) https://codereview.chromium.org/2838433002/diff/20001/components/payments/and... components/payments/android/payment_manifest_web_data_service.cc:23: // Note that mojom::WebAppManifestSectionPtr is DISALLOW_COPY_AND_ASSIGN. Actually, mojom::WebAppManifestSectionPtr is a move-only type. It's more restrictive than DISALLOW_COPY_AND_ASSIGN. https://codereview.chromium.org/2838433002/diff/20001/components/payments/and... components/payments/android/payment_manifest_web_data_service.cc:63: result_ptr.reset(new WDResult<std::vector<mojom::WebAppManifestSectionPtr>>( Use base::MakeUnique. return base::MakeUnique<WDResult<std::vector<mojom::WebAppManifestSectionPtr>>>(PAYMENT_WEB_APP_MANIFEST, WebAppManifestSectionTable::FromWebDatabase(db)->GetWebAppManifest(web_app)))); https://codereview.chromium.org/2838433002/diff/20001/components/payments/and... components/payments/android/payment_manifest_web_data_service.cc:109: result_ptr.reset(new WDResult<std::vector<std::string>>( Ditto. https://codereview.chromium.org/2838433002/diff/20001/components/payments/and... File components/payments/android/payment_manifest_web_data_service.h (right): https://codereview.chromium.org/2838433002/diff/20001/components/payments/and... components/payments/android/payment_manifest_web_data_service.h:6: #define COMPONENTS_PAYMENTS_ANDROID_PAYMENT_MANIFEST_WEB_DATA_SERVICE_H_ https://codereview.chromium.org/2838433002/diff/20001/components/payments/and... components/payments/android/payment_manifest_web_data_service.h:37: const std::vector<std::string>& web_app_ids); s/web_app_id/android_app_package_name/g (or simply "package_name") Here and everywhere. https://codereview.chromium.org/2838433002/diff/20001/components/payments/and... components/payments/android/payment_manifest_web_data_service.h:45: ~PaymentManifestWebDataService() override; Why is this protected? https://codereview.chromium.org/2838433002/diff/20001/components/payments/and... File components/payments/android/web_app_manifest_section_table.cc (right): https://codereview.chromium.org/2838433002/diff/20001/components/payments/and... components/payments/android/web_app_manifest_section_table.cc:75: "id VARCHAR, " What happened to the primary key? https://codereview.chromium.org/2838433002/diff/20001/components/payments/and... components/payments/android/web_app_manifest_section_table.cc:97: DCHECK(manifest.size() > 0); DCHECK_LT(0U, manifest.size()); https://codereview.chromium.org/2838433002/diff/20001/components/payments/and... components/payments/android/web_app_manifest_section_table.cc:113: for (const auto& section : manifest) { DCHECK_EQ(manifest[0]->id, section->id); https://codereview.chromium.org/2838433002/diff/20001/components/payments/and... components/payments/android/web_app_manifest_section_table.cc:150: if (!s.ColumnBlobAsVector(index, &fingerprints)) This is an error condition, so you should signal an error the caller. Your error signal is an empty manifest. This can be 2nd, 3rd, 4th iteration of this loop, so the |manifest| vector can already be non-empty. Therefore, you should clear the |manifest| vector before breaking here.
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #3 (id:100001) 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...
Hi rouslan@, please take another look? https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java (right): https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:94: /** A set of apps verified by using cached manifest. */ On 2017/04/24 18:22:30, ಠ_ಠ wrote: > Please clarify in the comment that these are app package names. Done. https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:95: private final Set<String> mVerifiedAppsByCachedManifest; On 2017/04/24 18:22:30, ಠ_ಠ wrote: > s/Apps/PackageNames/ Done. https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:97: /** A set of web apps Ids of the verified method supports. */ On 2017/04/24 18:22:30, ಠ_ಠ wrote: > Clarify the comment that these are app package names. Done. https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:98: private final Set<String> mSupportedWebAppsIdsOfTheMethod; On 2017/04/24 18:22:31, ಠ_ಠ wrote: > s/WebAppIds/PackageNames/ Done. https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:108: * A flag indicates whether there is a web app Id exists in the cache but failed to fetch On 2017/04/24 18:22:31, ಠ_ಠ wrote: > "Fetch" is an overloaded term. Downloading can also be called fetching. But here > you're talking about fetching from cache. Use the phrase "fetch from cache" > instead. Done. https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:111: private boolean mAtLeastOneWebAppManifestFailedToFetch; On 2017/04/24 18:22:30, ಠ_ಠ wrote: > mIsCacheStale might be a better name. Done. https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:203: mMethodName.toString(), this)) { On 2017/04/24 18:22:30, ಠ_ಠ wrote: > Can you also make sure that the cache is used only up to a certain number of > days? Let's 90 for now. If the cache is older than 90 days, clear it and > redownload the web app manifest. No necessary now since we refresh cache after each PR https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:205: } On 2017/04/24 18:22:31, ಠ_ಠ wrote: > Let's attempt a download regardless of cache state. The downloaded manifests > should be used to refresh the cache. Like this: > > Get apps->Get cache->Success-------------------> Show UI. > \ \->Fail---- / > \ \ / > ->Download manifests-> Update cache- > > Does my "diagram" make sense? I do not do cache check and manifest download in parallel since it's complicated to sync them and cache check should be quick. 1) Check cache first. 2) If failed, then start download and parse manifest as before, update cache at the end. 3) If success, notify caller to enable UI, then start download and parse manifest to update cache without notifying caller again. https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:227: public void onPaymentMethodManifestFetched(String[] webAppsIds) { On 2017/04/24 18:22:30, ಠ_ಠ wrote: > What string identifies a web app? Done. https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:238: for (String app : fetchedApps) { On 2017/04/24 18:22:31, ಠ_ಠ wrote: > Let's iterate over webAppIds using "for (int i = 0..." type loop, because it's > more efficient than "for (String..." type loop. Done. https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:259: String verifiedApp = verifyAppWithWebAppManifest(manifest); On 2017/04/24 18:22:30, ಠ_ಠ wrote: > What is this string? A package name? Done. Yes, renamed the temp variable https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:269: for (String app : mVerifiedAppsByCachedManifest) { On 2017/04/24 18:22:30, ಠ_ಠ wrote: > You only ever add thing to mVerifiedAppsByCachedManifest and then iterate over > it. An ArrayList would work better here, because then you would be able to use > the more efficient "for (int i =0..." type loop here instead. Done. https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestWebDataService.java (right): https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestWebDataService.java:31: void onPaymentWebAppManifestFetched(WebAppManifestSection[] manifest); On 2017/04/24 18:22:31, ಠ_ಠ wrote: > > Can you define a simpler interface that hides the fact that you have two tables? > > It would be preferable to query for payment method name and get back a list of > (package_mame, min_version, fingerprints) tuples. > > Similarly, it would be preferable to define a single call to store this > information on disk: saveManifests(manifests), where each item in the > |manifests| is a (method_mame, package_mame, min_version, fingerprints) tuple. Distinguish them looks a little more consistent here since we distinguished them in PaymentManifestDownloader and PaymentManifestParser. Moreover, query payment method manifest only is faster so as to switch to download manifest online quicker for the first time. Make sense? We could also modify it later given this CL is already big. https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestWebDataService.java:49: private final long mManifestWebDataServiceAndroid; On 2017/04/24 18:22:31, ಠ_ಠ wrote: > Please put all member variables together. Done. https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestWebDataService.java:68: * Adds the supported web apps Ids of the method. On 2017/04/24 18:22:31, ಠ_ಠ wrote: > s/web app Ids/Android app package names/g > > Here and everywhere. Done. https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestWebDataService.java:80: * @param webAppId The id of the payment web app. On 2017/04/24 18:22:31, ಠ_ಠ wrote: > s/webAppId/androidAppPackageName/ (or packageName) > > It's verify confusing to talk about web app identifiers when dealing with > Android payment apps. Done. https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestWebDataService.java:109: private native long nativeInit(); On 2017/04/24 18:22:31, ಠ_ಠ wrote: > If there's initialization, then you also need destruction to avoid leaking > stuff. Done. https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestWebDataService.java:116: private native boolean nativeGetPaymentWebAppManifest( On 2017/04/24 18:22:31, ಠ_ಠ wrote: > Can you group the "Get" methods together? Done. https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/payments... File chrome/browser/payments/android/payment_manifest_downloader_android.cc (right): https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/payments... chrome/browser/payments/android/payment_manifest_downloader_android.cc:5: #include "chrome/browser/payments/android/payment_manifest_downloader_android.h" On 2017/04/24 18:22:31, ಠ_ಠ wrote: > There appears to be no need to move the downloader from components/ to > chrome/browser/. Not applicable, thought it might be better to put it together with parser https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/payments... File chrome/browser/payments/android/payment_manifest_parser_android.cc (right): https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/payments... chrome/browser/payments/android/payment_manifest_parser_android.cc:96: PaymentManifestWebDataServiceAndroid::AddPaymentWebAppManifest(manifest); On 2017/04/24 18:22:31, ಠ_ಠ wrote: > Please cache the manifests from PaymentMethodVerifier.java instead of from here. > (Chrome should cache the manifests for https://bobpay.com only if all of them > downloaded successfully.) Then please move the parser back to components. Thank > you! Done. https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/payments... File chrome/browser/payments/android/payment_manifest_web_data_service_android.cc (right): https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/payments... chrome/browser/payments/android/payment_manifest_web_data_service_android.cc:17: namespace payments { On 2017/04/24 18:22:31, ಠ_ಠ wrote: > Add an empty line after the namespace start. Done. https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/payments... chrome/browser/payments/android/payment_manifest_web_data_service_android.cc:39: case PAYMENT_WEB_APP_MANIFEST: On 2017/04/24 18:22:32, ಠ_ಠ wrote: > Question: Where is this defined? web_data_results.h https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/payments... chrome/browser/payments/android/payment_manifest_web_data_service_android.cc:57: ->GetValue()); On 2017/04/24 18:22:31, ಠ_ಠ wrote: > Ouch, this is confusing. Please separate this into several statements. Done. https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/payments... chrome/browser/payments/android/payment_manifest_web_data_service_android.cc:63: const mojom::WebAppManifestSectionPtr& section = (*manifest)[i]; On 2017/04/24 18:22:32, ಠ_ಠ wrote: > manifest->get(i) is less ugly, IMHO. Done. https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/payments... chrome/browser/payments/android/payment_manifest_web_data_service_android.cc:67: env, jmanifest.obj(), base::checked_cast<int>(i), On 2017/04/24 18:22:31, ಠ_ಠ wrote: > #include "base/numerics/safe_conversions.h" Done. https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/payments... chrome/browser/payments/android/payment_manifest_web_data_service_android.cc:90: const std::vector<std::string>* web_apps_ids = On 2017/04/24 18:22:32, ಠ_ಠ wrote: > #include <string> Done. https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/payments... chrome/browser/payments/android/payment_manifest_web_data_service_android.cc:92: ->GetValue()); On 2017/04/24 18:22:32, ಠ_ಠ wrote: > Confusing. Please separate into several statements. Done. https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/payments... File chrome/browser/payments/android/payment_manifest_web_data_service_android.h (right): https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/payments... chrome/browser/payments/android/payment_manifest_web_data_service_android.h:21: // therefore a single instance of this wrapper. On 2017/04/24 18:22:32, ಠ_ಠ wrote: > This comment should be on the class, not on the namespace. Done. https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/payments... chrome/browser/payments/android/payment_manifest_web_data_service_android.h:29: PaymentManifestWebDataServiceAndroid(JNIEnv* env, jobject obj); On 2017/04/24 18:22:32, ಠ_ಠ wrote: > Need to override the parent destructor. Done. https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/payments... chrome/browser/payments/android/payment_manifest_web_data_service_android.h:53: bool GetPaymentWebAppManifest( On 2017/04/24 18:22:32, ಠ_ಠ wrote: > Please group the Get and Add methods together. Done. https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/payments... chrome/browser/payments/android/payment_manifest_web_data_service_android.h:62: const std::vector<mojom::WebAppManifestSectionPtr>& manifest); On 2017/04/24 18:22:32, ಠ_ಠ wrote: > #include <vector> > #include "components/payments/mojom/payment_manifest_parser.mojom.h" Done. https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/web_data... File chrome/browser/web_data_service_factory.h (right): https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/web_data... chrome/browser/web_data_service_factory.h:30: } // namespace payments On 2017/04/24 18:22:32, ಠ_ಠ wrote: > Remove this comment to match the rest of the file. Done. https://codereview.chromium.org/2838433002/diff/20001/components/payments/and... File components/payments/android/payment_manifest_web_data_service.cc (right): https://codereview.chromium.org/2838433002/diff/20001/components/payments/and... components/payments/android/payment_manifest_web_data_service.cc:21: const std::vector<mojom::WebAppManifestSectionPtr>& manifest) { On 2017/04/24 18:22:32, ಠ_ಠ wrote: > I recommend that you change the type of this parameter to be > "std::vector<mojom::WebAppManifestSectionPtr> manifest". Then you don't need to > make a |manifest_copy| below. (This whole file would benefit from using the > move-only type like this.) I can not move it since the input manifest is also be used by other place. Not applicable right now. https://codereview.chromium.org/2838433002/diff/20001/components/payments/and... components/payments/android/payment_manifest_web_data_service.cc:23: // Note that mojom::WebAppManifestSectionPtr is DISALLOW_COPY_AND_ASSIGN. On 2017/04/24 18:22:32, ಠ_ಠ wrote: > Actually, mojom::WebAppManifestSectionPtr is a move-only type. It's more > restrictive than DISALLOW_COPY_AND_ASSIGN. Acknowledged. https://codereview.chromium.org/2838433002/diff/20001/components/payments/and... components/payments/android/payment_manifest_web_data_service.cc:63: result_ptr.reset(new WDResult<std::vector<mojom::WebAppManifestSectionPtr>>( On 2017/04/24 18:22:32, ಠ_ಠ wrote: > Use base::MakeUnique. > > return > base::MakeUnique<WDResult<std::vector<mojom::WebAppManifestSectionPtr>>>(PAYMENT_WEB_APP_MANIFEST, > WebAppManifestSectionTable::FromWebDatabase(db)->GetWebAppManifest(web_app)))); Done. https://codereview.chromium.org/2838433002/diff/20001/components/payments/and... components/payments/android/payment_manifest_web_data_service.cc:109: result_ptr.reset(new WDResult<std::vector<std::string>>( On 2017/04/24 18:22:32, ಠ_ಠ wrote: > Ditto. Done. https://codereview.chromium.org/2838433002/diff/20001/components/payments/and... File components/payments/android/payment_manifest_web_data_service.h (right): https://codereview.chromium.org/2838433002/diff/20001/components/payments/and... components/payments/android/payment_manifest_web_data_service.h:6: On 2017/04/24 18:22:32, ಠ_ಠ wrote: > #define COMPONENTS_PAYMENTS_ANDROID_PAYMENT_MANIFEST_WEB_DATA_SERVICE_H_ Done. https://codereview.chromium.org/2838433002/diff/20001/components/payments/and... components/payments/android/payment_manifest_web_data_service.h:37: const std::vector<std::string>& web_app_ids); On 2017/04/24 18:22:32, ಠ_ಠ wrote: > s/web_app_id/android_app_package_name/g (or simply "package_name") > > Here and everywhere. Done. https://codereview.chromium.org/2838433002/diff/20001/components/payments/and... components/payments/android/payment_manifest_web_data_service.h:45: ~PaymentManifestWebDataService() override; On 2017/04/24 18:22:32, ಠ_ಠ wrote: > Why is this protected? Not allowed to be destructed from outside of this class. Moved it to private. This class object is destructed by calling Destroy(). https://codereview.chromium.org/2838433002/diff/20001/components/payments/and... File components/payments/android/web_app_manifest_section_table.cc (right): https://codereview.chromium.org/2838433002/diff/20001/components/payments/and... components/payments/android/web_app_manifest_section_table.cc:75: "id VARCHAR, " On 2017/04/24 18:22:32, ಠ_ಠ wrote: > What happened to the primary key? id is not unique since an app may have multiple WebAppManifestSection. https://codereview.chromium.org/2838433002/diff/20001/components/payments/and... components/payments/android/web_app_manifest_section_table.cc:97: DCHECK(manifest.size() > 0); On 2017/04/24 18:22:33, ಠ_ಠ wrote: > DCHECK_LT(0U, manifest.size()); Done. https://codereview.chromium.org/2838433002/diff/20001/components/payments/and... components/payments/android/web_app_manifest_section_table.cc:113: for (const auto& section : manifest) { On 2017/04/24 18:22:32, ಠ_ಠ wrote: > DCHECK_EQ(manifest[0]->id, section->id); Done. https://codereview.chromium.org/2838433002/diff/20001/components/payments/and... components/payments/android/web_app_manifest_section_table.cc:150: if (!s.ColumnBlobAsVector(index, &fingerprints)) On 2017/04/24 18:22:32, ಠ_ಠ wrote: > This is an error condition, so you should signal an error the caller. Your error > signal is an empty manifest. This can be 2nd, 3rd, 4th iteration of this loop, > so the |manifest| vector can already be non-empty. Therefore, you should clear > the |manifest| vector before breaking here. Done.
Description was changed from ========== Cache payment manifests. In this patch: Try to fetch all supported web app ids for the payment method from the cache before downloading its manifest online. If cached web app ids contain all matching apps ids, proceed fetching corresponding web apps manifests from the cache. Otherwise fall back to download the payment method's manifest online. If failed to fetch at least one web app's manifest from the cache, then fall back to download payment method's manifest online. In addition: Cache downloaded and parsed supported web apps Ids of the payment method for future reference. Cache downloaded and parsed web app manifest sections for future reference. Refer to below doc for details. https://docs.google.com/a/google.com/document/d/1Ncsp96Ae5836NLOdPwHRzcX3zkgj... BUG=708508 ========== to ========== Cache payment manifests. In this patch: Try to fetch all supported web app ids for the payment method from the cache before downloading its manifest online. If cached web app ids contain all matching apps ids, proceed fetching corresponding web apps manifests from the cache. Otherwise fall back to download the payment method's manifest online. If failed to fetch at least one web app's manifest from the cache, then fall back to download payment method's manifest online. In addition: Cache downloaded and parsed supported web apps ids of the payment method for future reference. Cache downloaded and parsed web app manifest sections for future reference. Refer to below doc for details. https://docs.google.com/a/google.com/document/d/1Ncsp96Ae5836NLOdPwHRzcX3zkgj... BUG=708508 ==========
Good hustle! Can you work with someone in your office to improve your comments and variable names? See my comments inline about individual cases. https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java (right): https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:203: mMethodName.toString(), this)) { On 2017/04/26 13:46:31, gogerald1 wrote: > On 2017/04/24 18:22:30, ಠ_ಠ wrote: > > Can you also make sure that the cache is used only up to a certain number of > > days? Let's 90 for now. If the cache is older than 90 days, clear it and > > redownload the web app manifest. > > No necessary now since we refresh cache after each PR If download fails after every PR for 90 days, we should stop using the cache. https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestWebDataService.java (right): https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestWebDataService.java:31: void onPaymentWebAppManifestFetched(WebAppManifestSection[] manifest); On 2017/04/26 13:46:31, gogerald1 wrote: > On 2017/04/24 18:22:31, ಠ_ಠ wrote: > > > > Can you define a simpler interface that hides the fact that you have two > tables? > > > > It would be preferable to query for payment method name and get back a list of > > (package_mame, min_version, fingerprints) tuples. > > > > Similarly, it would be preferable to define a single call to store this > > information on disk: saveManifests(manifests), where each item in the > > |manifests| is a (method_mame, package_mame, min_version, fingerprints) tuple. > > Distinguish them looks a little more consistent here since we distinguished them > in PaymentManifestDownloader and PaymentManifestParser. You're right that PaymentManifestDownloader has two methods used in sequence that could be abstracted into a single method instead (downloadPaymentMethodManifest() and downloadWebAppManifest()). However, PaymentManifestDownloader performs two downloads when looking for payment method manifest: One HTTP HEAD download payment method name and one GET download for the actual JSON file with payment method information. This is abstracted away behind a single call downloadPaymentMethodManifest(). There's a fine line between exposing and abstracting parts of the implementation. I will let you use your own judgement here. > Moreover, query payment method manifest only is faster so as to switch to > download manifest online quicker for the first time. > > Make sense? We could also modify it later given this CL is already big. This argument makes sense. https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/payments... File chrome/browser/payments/android/payment_manifest_web_data_service_android.cc (right): https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/payments... chrome/browser/payments/android/payment_manifest_web_data_service_android.cc:39: case PAYMENT_WEB_APP_MANIFEST: On 2017/04/26 13:46:32, gogerald1 wrote: > On 2017/04/24 18:22:32, ಠ_ಠ wrote: > > Question: Where is this defined? > > web_data_results.h Include it? https://codereview.chromium.org/2838433002/diff/20001/components/payments/and... File components/payments/android/payment_manifest_web_data_service.h (right): https://codereview.chromium.org/2838433002/diff/20001/components/payments/and... components/payments/android/payment_manifest_web_data_service.h:45: ~PaymentManifestWebDataService() override; On 2017/04/26 13:46:33, gogerald1 wrote: > On 2017/04/24 18:22:32, ಠ_ಠ wrote: > > Why is this protected? > > Not allowed to be destructed from outside of this class. Moved it to private. > This class object is destructed by calling Destroy(). I don't see Destroy() anywhere in payment_manifest_web_data_service.h. Can you point me to it? https://codereview.chromium.org/2838433002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java (right): https://codereview.chromium.org/2838433002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:77: * resources used by this class. @param https://codereview.chromium.org/2838433002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:104: private final ArrayList<String> mVerifiedAppsPackageNamesByCachedManifest; 1) List<String> instead of ArrayList<String>. 2) AppPackageNames instead of AppsPackageNames. (No "s" in "App".) https://codereview.chromium.org/2838433002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:106: /** A set of package names of the apps support the verified method. */ Please correct the grammar here. I don't understand this sentence. https://codereview.chromium.org/2838433002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:107: private final Set<String> mSupportedAppsPackageNames; AppPackageNames instead of AppsPackageNames. (No "s" in "App".) https://codereview.chromium.org/2838433002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:109: /** A list of manifests of the apps support the verified method. */ Please correct grammar here as well. https://codereview.chromium.org/2838433002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:110: private final ArrayList<WebAppManifestSection[]> mSupportedAppsParsedManifest; List<String> instead of ArrayList<String>. https://codereview.chromium.org/2838433002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:120: /** A flag indicates whether the manifest cache is stale or unusable. */ /** Whether the manifest cache is stale (unusable). */ https://codereview.chromium.org/2838433002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:167: mVerifiedAppsPackageNamesByCachedManifest = new ArrayList<String>(); <> instead of <String> https://codereview.chromium.org/2838433002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:168: mSupportedAppsPackageNames = new HashSet<String>(); Ditto https://codereview.chromium.org/2838433002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:169: mSupportedAppsParsedManifest = new ArrayList<WebAppManifestSection[]>(); <> instead of <WebAppManifestSection[]> https://codereview.chromium.org/2838433002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:243: public void onPaymentMethodManifestFetched(String[] appsPackageNames) { appPackageNames instead of appsPackageNames (no "s" in "app"). https://codereview.chromium.org/2838433002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:359: // Cache supported apps package names. Make sure to cache only if all manifests downloaded successfully. We don't want to store partially correct data on disk. (Partially correct is slightly corrupted.) https://codereview.chromium.org/2838433002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:416: mCallback.onVerifyFinished(this); Including the comments, you use 12 lines for finishVerify() method and calls to it. If you inline the calls instead, you will use 9 lines of code without the need for clarifying comments. In addition, a boolean parameter is hard to understand for engineers that maintain your code and, therefore, likely to introduce bugs. Let's not use this method, but inline the function instead. https://codereview.chromium.org/2838433002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestWebDataService.java (right): https://codereview.chromium.org/2838433002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestWebDataService.java:61: * Gets the correspond payment web app's manifest. corresponding https://codereview.chromium.org/2838433002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestWebDataService.java:74: * Adds the supported Android apps package names of the method. s/apps/app/ https://codereview.chromium.org/2838433002/diff/120001/components/payments/an... File components/payments/android/payment_manifest_web_data_service.h (right): https://codereview.chromium.org/2838433002/diff/120001/components/payments/an... components/payments/android/payment_manifest_web_data_service.h:29: std::vector<mojom::WebAppManifestSectionPtr> manifest); Good that you've removed const-ref here, because mojom::WebAppManifestSectionPtr is a move-only type. https://codereview.chromium.org/2838433002/diff/120001/components/payments/an... components/payments/android/payment_manifest_web_data_service.h:33: std::vector<std::string> apps_package_names); Please use const-ref for apps_package_names, otherwise std::vector<std::string> will be inefficiently copied.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #4 (id:140001) has been deleted
Patchset #4 (id:160001) 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/2838433002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java (right): https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:203: mMethodName.toString(), this)) { On 2017/04/26 15:00:43, ಠ_ಠ wrote: > On 2017/04/26 13:46:31, gogerald1 wrote: > > On 2017/04/24 18:22:30, ಠ_ಠ wrote: > > > Can you also make sure that the cache is used only up to a certain number of > > > days? Let's 90 for now. If the cache is older than 90 days, clear it and > > > redownload the web app manifest. > > > > No necessary now since we refresh cache after each PR > > If download fails after every PR for 90 days, we should stop using the cache. OKay, I will fix this in the next CL by introducing a preference to record refreshing date. https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestWebDataService.java (right): https://codereview.chromium.org/2838433002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestWebDataService.java:31: void onPaymentWebAppManifestFetched(WebAppManifestSection[] manifest); On 2017/04/26 15:00:43, ಠ_ಠ wrote: > On 2017/04/26 13:46:31, gogerald1 wrote: > > On 2017/04/24 18:22:31, ಠ_ಠ wrote: > > > > > > Can you define a simpler interface that hides the fact that you have two > > tables? > > > > > > It would be preferable to query for payment method name and get back a list > of > > > (package_mame, min_version, fingerprints) tuples. > > > > > > Similarly, it would be preferable to define a single call to store this > > > information on disk: saveManifests(manifests), where each item in the > > > |manifests| is a (method_mame, package_mame, min_version, fingerprints) > tuple. > > > > Distinguish them looks a little more consistent here since we distinguished > them > > in PaymentManifestDownloader and PaymentManifestParser. > > You're right that PaymentManifestDownloader has two methods used in sequence > that could be abstracted into a single method instead > (downloadPaymentMethodManifest() and downloadWebAppManifest()). > > However, PaymentManifestDownloader performs two downloads when looking for > payment method manifest: One HTTP HEAD download payment method name and one GET > download for the actual JSON file with payment method information. This is > abstracted away behind a single call downloadPaymentMethodManifest(). > > There's a fine line between exposing and abstracting parts of the > implementation. I will let you use your own judgement here. > > > Moreover, query payment method manifest only is faster so as to switch to > > download manifest online quicker for the first time. > > > > Make sense? We could also modify it later given this CL is already big. > > This argument makes sense. Acknowledged. https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/payments... File chrome/browser/payments/android/payment_manifest_web_data_service_android.cc (right): https://codereview.chromium.org/2838433002/diff/20001/chrome/browser/payments... chrome/browser/payments/android/payment_manifest_web_data_service_android.cc:39: case PAYMENT_WEB_APP_MANIFEST: On 2017/04/26 15:00:43, ಠ_ಠ wrote: > On 2017/04/26 13:46:32, gogerald1 wrote: > > On 2017/04/24 18:22:32, ಠ_ಠ wrote: > > > Question: Where is this defined? > > > > web_data_results.h > > Include it? Done. https://codereview.chromium.org/2838433002/diff/20001/components/payments/and... File components/payments/android/payment_manifest_web_data_service.h (right): https://codereview.chromium.org/2838433002/diff/20001/components/payments/and... components/payments/android/payment_manifest_web_data_service.h:45: ~PaymentManifestWebDataService() override; On 2017/04/26 15:00:43, ಠ_ಠ wrote: > On 2017/04/26 13:46:33, gogerald1 wrote: > > On 2017/04/24 18:22:32, ಠ_ಠ wrote: > > > Why is this protected? > > > > Not allowed to be destructed from outside of this class. Moved it to private. > > This class object is destructed by calling Destroy(). > > I don't see Destroy() anywhere in payment_manifest_web_data_service.h. Can you > point me to it? Ah, sorry, I thought this is payment_manifest_web_data_service_android. Actually, this class is instantiated when constructing WebDataServiceWrapper which is a keyed service. And it is not expected to be destructed. Note that the destructor of the base class WebDataServiceBase is declared protected. https://codereview.chromium.org/2838433002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java (right): https://codereview.chromium.org/2838433002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:77: * resources used by this class. On 2017/04/26 15:00:44, ಠ_ಠ wrote: > @param Done. https://codereview.chromium.org/2838433002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:104: private final ArrayList<String> mVerifiedAppsPackageNamesByCachedManifest; On 2017/04/26 15:00:44, ಠ_ಠ wrote: > 1) List<String> instead of ArrayList<String>. > 2) AppPackageNames instead of AppsPackageNames. (No "s" in "App".) Done. https://codereview.chromium.org/2838433002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:106: /** A set of package names of the apps support the verified method. */ On 2017/04/26 15:00:44, ಠ_ಠ wrote: > Please correct the grammar here. I don't understand this sentence. Done. https://codereview.chromium.org/2838433002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:107: private final Set<String> mSupportedAppsPackageNames; On 2017/04/26 15:00:44, ಠ_ಠ wrote: > AppPackageNames instead of AppsPackageNames. (No "s" in "App".) Done. https://codereview.chromium.org/2838433002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:109: /** A list of manifests of the apps support the verified method. */ On 2017/04/26 15:00:44, ಠ_ಠ wrote: > Please correct grammar here as well. Done. https://codereview.chromium.org/2838433002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:110: private final ArrayList<WebAppManifestSection[]> mSupportedAppsParsedManifest; On 2017/04/26 15:00:44, ಠ_ಠ wrote: > List<String> instead of ArrayList<String>. Done. https://codereview.chromium.org/2838433002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:120: /** A flag indicates whether the manifest cache is stale or unusable. */ On 2017/04/26 15:00:44, ಠ_ಠ wrote: > /** Whether the manifest cache is stale (unusable). */ Done. https://codereview.chromium.org/2838433002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:167: mVerifiedAppsPackageNamesByCachedManifest = new ArrayList<String>(); On 2017/04/26 15:00:44, ಠ_ಠ wrote: > <> instead of <String> Done. https://codereview.chromium.org/2838433002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:168: mSupportedAppsPackageNames = new HashSet<String>(); On 2017/04/26 15:00:44, ಠ_ಠ wrote: > Ditto Done. https://codereview.chromium.org/2838433002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:169: mSupportedAppsParsedManifest = new ArrayList<WebAppManifestSection[]>(); On 2017/04/26 15:00:44, ಠ_ಠ wrote: > <> instead of <WebAppManifestSection[]> Done. https://codereview.chromium.org/2838433002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:243: public void onPaymentMethodManifestFetched(String[] appsPackageNames) { On 2017/04/26 15:00:44, ಠ_ಠ wrote: > appPackageNames instead of appsPackageNames (no "s" in "app"). Done. https://codereview.chromium.org/2838433002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:359: // Cache supported apps package names. On 2017/04/26 15:00:43, ಠ_ಠ wrote: > Make sure to cache only if all manifests downloaded successfully. We don't want > to store partially correct data on disk. (Partially correct is slightly > corrupted.) This is guaranteed by mPendingWebAppManifestsCount and mAtLeastOneManifestFailedToDownloadOrParse https://codereview.chromium.org/2838433002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestVerifier.java:416: mCallback.onVerifyFinished(this); On 2017/04/26 15:00:43, ಠ_ಠ wrote: > Including the comments, you use 12 lines for finishVerify() method and calls to > it. If you inline the calls instead, you will use 9 lines of code without the > need for clarifying comments. In addition, a boolean parameter is hard to > understand for engineers that maintain your code and, therefore, likely to > introduce bugs. Let's not use this method, but inline the function instead. Done. https://codereview.chromium.org/2838433002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestWebDataService.java (right): https://codereview.chromium.org/2838433002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestWebDataService.java:61: * Gets the correspond payment web app's manifest. On 2017/04/26 15:00:44, ಠ_ಠ wrote: > corresponding Done. https://codereview.chromium.org/2838433002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentManifestWebDataService.java:74: * Adds the supported Android apps package names of the method. On 2017/04/26 15:00:44, ಠ_ಠ wrote: > s/apps/app/ Used possession symbol to be precise. Renamed below variable to appPackageNames https://codereview.chromium.org/2838433002/diff/120001/components/payments/an... File components/payments/android/payment_manifest_web_data_service.h (right): https://codereview.chromium.org/2838433002/diff/120001/components/payments/an... components/payments/android/payment_manifest_web_data_service.h:29: std::vector<mojom::WebAppManifestSectionPtr> manifest); On 2017/04/26 15:00:44, ಠ_ಠ wrote: > Good that you've removed const-ref here, because mojom::WebAppManifestSectionPtr > is a move-only type. Acknowledged. https://codereview.chromium.org/2838433002/diff/120001/components/payments/an... components/payments/android/payment_manifest_web_data_service.h:33: std::vector<std::string> apps_package_names); On 2017/04/26 15:00:44, ಠ_ಠ wrote: > Please use const-ref for apps_package_names, otherwise std::vector<std::string> > will be inefficiently copied. There is no copy here since I used std::move when calling AddPaymentMethodManifest and inside AddPaymentMethodManifest function. I did this to make it efficient and consistent with AddPaymentWebAppManifest.
lgtm Do you plan to add the 90 day limit in a follow up or in here?
gogerald@chromium.org changed reviewers: + pkasting@chromium.org
I will add it in the next CL, Hi pkasting@, ptal of the changes in chrome/browser/ui/profile_error_dialog.h chrome/browser/web_data_service_factory.* components/webdata/* components/webdata_services/*
gogerald@chromium.org changed reviewers: + jwd@chromium.org
Hi jwd@, ptal of the changes in tools/metrics/histograms/histograms.xml
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
histograms LGTM
LGTM
The CQ bit was checked by gogerald@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
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: This issue passed the CQ dry run.
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, pkasting@chromium.org, jwd@chromium.org Link to the patchset: https://codereview.chromium.org/2838433002/#ps200001 (title: "rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
gogerald@chromium.org changed reviewers: + sky@chromium.org
Hi sky@, ptal of the changes in chrome/browser/web_data_service_factory.cc and chrome/browser/web_data_service_factory.h
LGTM
The CQ bit was checked by gogerald@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": 200001, "attempt_start_ts": 1493306949828060,
"parent_rev": "9567f9fa3fcfcaa50fc7b6a6adf66106a2eb6be5", "commit_rev":
"7947209945cff83f3301963f5a01821d02e100a2"}
Message was sent while issue was closed.
Description was changed from ========== Cache payment manifests. In this patch: Try to fetch all supported web app ids for the payment method from the cache before downloading its manifest online. If cached web app ids contain all matching apps ids, proceed fetching corresponding web apps manifests from the cache. Otherwise fall back to download the payment method's manifest online. If failed to fetch at least one web app's manifest from the cache, then fall back to download payment method's manifest online. In addition: Cache downloaded and parsed supported web apps ids of the payment method for future reference. Cache downloaded and parsed web app manifest sections for future reference. Refer to below doc for details. https://docs.google.com/a/google.com/document/d/1Ncsp96Ae5836NLOdPwHRzcX3zkgj... BUG=708508 ========== to ========== Cache payment manifests. In this patch: Try to fetch all supported web app ids for the payment method from the cache before downloading its manifest online. If cached web app ids contain all matching apps ids, proceed fetching corresponding web apps manifests from the cache. Otherwise fall back to download the payment method's manifest online. If failed to fetch at least one web app's manifest from the cache, then fall back to download payment method's manifest online. In addition: Cache downloaded and parsed supported web apps ids of the payment method for future reference. Cache downloaded and parsed web app manifest sections for future reference. Refer to below doc for details. https://docs.google.com/a/google.com/document/d/1Ncsp96Ae5836NLOdPwHRzcX3zkgj... BUG=708508 Review-Url: https://codereview.chromium.org/2838433002 Cr-Commit-Position: refs/heads/master@{#467688} Committed: https://chromium.googlesource.com/chromium/src/+/7947209945cff83f3301963f5a01... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:200001) as https://chromium.googlesource.com/chromium/src/+/7947209945cff83f3301963f5a01... |
