Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2537563002:
Making offline previews freshness configurable (Closed)
|
Created:
4 years ago by RyanSturm Modified:
4 years ago CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, dimich+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMaking offline previews freshness configurable
This makes the freshness (e.g. the amount of time since download that
the page may be shown as a preview) of offline previews controllable
as a field trial param.
This also sets the default back to 7 days, which was changed during the
offline page refactor that moved the code from the UI thread to the IO
thread.
BUG=669093
Committed: https://crrev.com/e42c5acc8c61a87e6a72352d2428b3f477bd73c1
Cr-Commit-Position: refs/heads/master@{#435251}
Patch Set 1 #
Total comments: 4
Dependent Patchsets: Messages
Total messages: 25 (15 generated)
The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ryansturm@chromium.org changed reviewers: + jianli@chromium.org, tbansal@chromium.org
jainli, tbansal: PTAL I plan to merge this to M56
lgtm % minor nit. https://codereview.chromium.org/2537563002/diff/20001/components/previews/cor... File components/previews/core/previews_experiments.cc (right): https://codereview.chromium.org/2537563002/diff/20001/components/previews/cor... components/previews/core/previews_experiments.cc:167: return base::TimeDelta::FromDays(duration); nit: Add: DCHECK_LE(0, duration);
https://codereview.chromium.org/2537563002/diff/20001/components/previews/cor... File components/previews/core/previews_experiments.cc (right): https://codereview.chromium.org/2537563002/diff/20001/components/previews/cor... components/previews/core/previews_experiments.cc:167: return base::TimeDelta::FromDays(duration); On 2016/11/29 18:05:38, tbansal1 wrote: > nit: Add: > DCHECK_LE(0, duration); Acknowledged. I don't think this is necessary, and we likely could miss this if we send out bad finch params.
lgtm https://codereview.chromium.org/2537563002/diff/20001/components/previews/cor... File components/previews/core/previews_experiments.cc (right): https://codereview.chromium.org/2537563002/diff/20001/components/previews/cor... components/previews/core/previews_experiments.cc:165: return base::TimeDelta::FromDays(7); nit: Would it be simpler to say "duration = 7;", which will also make the binary size a bit less?
Thanks for the review. https://codereview.chromium.org/2537563002/diff/20001/components/previews/cor... File components/previews/core/previews_experiments.cc (right): https://codereview.chromium.org/2537563002/diff/20001/components/previews/cor... components/previews/core/previews_experiments.cc:165: return base::TimeDelta::FromDays(7); On 2016/11/29 22:28:43, jianli wrote: > nit: Would it be simpler to say "duration = 7;", which will also make the binary > size a bit less? I will write a follow-up CL to change all of the methods in this file to follow that pattern. I opened a bug, but I'm not going to add a TODO. crbug.com/669683
The CQ bit was checked by ryansturm@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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by ryansturm@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": 20001, "attempt_start_ts": 1480514693071660,
"parent_rev": "45d4a4b10036d2b884a3257054b366a68ad9f52c", "commit_rev":
"fa8745aa5d465e3fb5ad53f5f37f6cebfe11aaa5"}
Message was sent while issue was closed.
Committed patchset #1 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Making offline previews freshness configurable This makes the freshness (e.g. the amount of time since download that the page may be shown as a preview) of offline previews controllable as a field trial param. This also sets the default back to 7 days, which was changed during the offline page refactor that moved the code from the UI thread to the IO thread. BUG=669093 ========== to ========== Making offline previews freshness configurable This makes the freshness (e.g. the amount of time since download that the page may be shown as a preview) of offline previews controllable as a field trial param. This also sets the default back to 7 days, which was changed during the offline page refactor that moved the code from the UI thread to the IO thread. BUG=669093 Committed: https://crrev.com/e42c5acc8c61a87e6a72352d2428b3f477bd73c1 Cr-Commit-Position: refs/heads/master@{#435251} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/e42c5acc8c61a87e6a72352d2428b3f477bd73c1 Cr-Commit-Position: refs/heads/master@{#435251} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
