Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2845753003:
[Payments] Set cached manifest expiration date (Closed)
|
Created:
3 years, 8 months ago by gogerald1 Modified:
3 years, 7 months ago Reviewers:
please use gerrit instead CC:
chromium-reviews, rouslan+autofill_chromium.org, rogerm+autofillwatch_chromium.org, sebsg+autofillwatch_chromium.org, gogerald+paymentswatch_chromium.org, rouslan+payments_chromium.org, mathp+autofillwatch_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. |
DescriptionSet cached manifest expiration date
The expiration days is set to 90 days.
Note that the expired cached manifest will be refreshed by downloading it online.
BUG=708508
Review-Url: https://codereview.chromium.org/2845753003
Cr-Commit-Position: refs/heads/master@{#467790}
Committed: https://chromium.googlesource.com/chromium/src/+/69a1f75e22720b35e521bb3776cb3e0add382f74
Patch Set 1 #
Total comments: 8
Patch Set 2 : address comments #
Total comments: 2
Patch Set 3 : improve comments #
Messages
Total messages: 32 (21 generated)
Description was changed from ========== set data expiration date BUG= ========== to ========== Set cached manifest expiration date The expiration days is set to 90 days. Note that the expired manifest will be refreshed after downloading and parsing online. BUG=708508 ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== Set cached manifest expiration date The expiration days is set to 90 days. Note that the expired manifest will be refreshed after downloading and parsing online. BUG=708508 ========== to ========== Set cached manifest expiration date The expiration days is set to 90 days. Note that the expired cached manifest will be refreshed after downloading and parsing online. BUG=708508 ==========
Description was changed from ========== Set cached manifest expiration date The expiration days is set to 90 days. Note that the expired cached manifest will be refreshed after downloading and parsing online. BUG=708508 ========== to ========== Set cached manifest expiration date The expiration days is set to 90 days. Note that the expired cached manifest will be refreshed by downloading it from online. BUG=708508 ==========
Description was changed from ========== Set cached manifest expiration date The expiration days is set to 90 days. Note that the expired cached manifest will be refreshed by downloading it from online. BUG=708508 ========== to ========== Set cached manifest expiration date The expiration days is set to 90 days. Note that the expired cached manifest will be refreshed by downloading it online. BUG=708508 ==========
gogerald@chromium.org changed reviewers: + rouslan@chromium.org
Hi rouslan@, ptal,
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: 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_...)
https://codereview.chromium.org/2845753003/diff/20001/components/payments/and... File components/payments/android/payment_method_manifest_table.cc (right): https://codereview.chromium.org/2845753003/diff/20001/components/payments/and... components/payments/android/payment_method_manifest_table.cc:14: const time_t DATA_VALID_TIME_IN_SECONDS = 90 * 24 * 60; #include <time.h> https://codereview.chromium.org/2845753003/diff/20001/components/payments/and... components/payments/android/payment_method_manifest_table.cc:14: const time_t DATA_VALID_TIME_IN_SECONDS = 90 * 24 * 60; That's 1.5 days. You're missing an extra "* 60". https://codereview.chromium.org/2845753003/diff/20001/components/payments/and... components/payments/android/payment_method_manifest_table.cc:107: web_app_ids.clear(); You're leaving the stale data in the table, which will grow to large size as time passes. Please clear the stale data from the table before querying it. You should be able to clear the stale data using a single SQL statement. https://codereview.chromium.org/2845753003/diff/20001/components/payments/and... File components/payments/android/payment_method_manifest_table.h (right): https://codereview.chromium.org/2845753003/diff/20001/components/payments/and... components/payments/android/payment_method_manifest_table.h:24: // expire_date The expire date in seconds. Which epoch? Windows epoch starts in 1600. Unix epoch starts in 1971.
Patchset #2 (id:40001) 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/2845753003/diff/20001/components/payments/and... File components/payments/android/payment_method_manifest_table.cc (right): https://codereview.chromium.org/2845753003/diff/20001/components/payments/and... components/payments/android/payment_method_manifest_table.cc:14: const time_t DATA_VALID_TIME_IN_SECONDS = 90 * 24 * 60; On 2017/04/27 15:45:18, ಠ_ಠ wrote: > #include <time.h> Done. https://codereview.chromium.org/2845753003/diff/20001/components/payments/and... components/payments/android/payment_method_manifest_table.cc:14: const time_t DATA_VALID_TIME_IN_SECONDS = 90 * 24 * 60; On 2017/04/27 15:45:17, ಠ_ಠ wrote: > That's 1.5 days. You're missing an extra "* 60". Done. Good catch https://codereview.chromium.org/2845753003/diff/20001/components/payments/and... components/payments/android/payment_method_manifest_table.cc:107: web_app_ids.clear(); On 2017/04/27 15:45:18, ಠ_ಠ wrote: > You're leaving the stale data in the table, which will grow to large size as > time passes. Please clear the stale data from the table before querying it. You > should be able to clear the stale data using a single SQL statement. Thought about this, but we have two tables, we might only be able to clear expired data in one table if we clear it here. Anyway, moved to remove expired data in web data service before query https://codereview.chromium.org/2845753003/diff/20001/components/payments/and... File components/payments/android/payment_method_manifest_table.h (right): https://codereview.chromium.org/2845753003/diff/20001/components/payments/and... components/payments/android/payment_method_manifest_table.h:24: // expire_date The expire date in seconds. On 2017/04/27 15:45:18, ಠ_ಠ wrote: > Which epoch? Windows epoch starts in 1600. Unix epoch starts in 1971. Done.
LGTM % better phrasing for the comment. https://codereview.chromium.org/2845753003/diff/60001/components/payments/and... File components/payments/android/payment_method_manifest_table.cc (right): https://codereview.chromium.org/2845753003/diff/60001/components/payments/and... components/payments/android/payment_method_manifest_table.cc:15: // Data valid time in seconds. "The length of time (in seconds) during which data is considered valid." Perhaps?
Thanks, sending to CQ, https://codereview.chromium.org/2845753003/diff/60001/components/payments/and... File components/payments/android/payment_method_manifest_table.cc (right): https://codereview.chromium.org/2845753003/diff/60001/components/payments/and... components/payments/android/payment_method_manifest_table.cc:15: // Data valid time in seconds. On 2017/04/27 18:29:10, ಠ_ಠ wrote: > "The length of time (in seconds) during which data is considered valid." > > Perhaps? Used a shorter version "Data valid duration in seconds."
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 Link to the patchset: https://codereview.chromium.org/2845753003/#ps80001 (title: "improve comments")
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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by 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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
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": 80001, "attempt_start_ts": 1493326831859030,
"parent_rev": "9a822f4b8c9d7d1ec7df123e48d24118ad102f00", "commit_rev":
"69a1f75e22720b35e521bb3776cb3e0add382f74"}
Message was sent while issue was closed.
Description was changed from ========== Set cached manifest expiration date The expiration days is set to 90 days. Note that the expired cached manifest will be refreshed by downloading it online. BUG=708508 ========== to ========== Set cached manifest expiration date The expiration days is set to 90 days. Note that the expired cached manifest will be refreshed by downloading it online. BUG=708508 Review-Url: https://codereview.chromium.org/2845753003 Cr-Commit-Position: refs/heads/master@{#467790} Committed: https://chromium.googlesource.com/chromium/src/+/69a1f75e22720b35e521bb3776cb... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001) as https://chromium.googlesource.com/chromium/src/+/69a1f75e22720b35e521bb3776cb... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
