Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2878363004:
If users clear history, the previews recency rule should be cleared (Closed)
DescriptionIf users clear history, the previews recency rule should be cleared
When a user clears a chunk of history, the recency rule (previews aren't
shown for ~5 minutes after an opt out). Should be reset if the chunk of
history is longer than 5 minutes.
BUG=722436
Review-Url: https://codereview.chromium.org/2878363004
Cr-Commit-Position: refs/heads/master@{#474890}
Committed: https://chromium.googlesource.com/chromium/src/+/4cd77228276c278c78ff13639d319fd9e7198752
Patch Set 1 #
Total comments: 7
Patch Set 2 : bengr comments #
Total comments: 1
Patch Set 3 : re-wrote test suite #
Messages
Total messages: 22 (14 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...
ryansturm@chromium.org changed reviewers: + bengr@chromium.org
bengr: PTAL, I noticed this while playing with client-side Lo-Fi this morning. It does not seem critical in any way, but it is a nice to have for testing.
https://codereview.chromium.org/2878363004/diff/1/components/previews/core/pr... File components/previews/core/previews_black_list.cc (right): https://codereview.chromium.org/2878363004/diff/1/components/previews/core/pr... components/previews/core/previews_black_list.cc:156: void PreviewsBlackList::ClearBlackListSync(base::Time begin_time, How robust is your blacklist logic to the system clock being changed? https://codereview.chromium.org/2878363004/diff/1/components/previews/core/pr... File components/previews/core/previews_black_list_unittest.cc (right): https://codereview.chromium.org/2878363004/diff/1/components/previews/core/pr... components/previews/core/previews_black_list_unittest.cc:615: std::map<std::string, std::string> params; #include <map> https://codereview.chromium.org/2878363004/diff/1/components/previews/core/pr... components/previews/core/previews_black_list_unittest.cc:631: } tests[] = {{true}, {false}}; This is an anti-pattern. Write this as two separate tests.
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...
https://codereview.chromium.org/2878363004/diff/1/components/previews/core/pr... File components/previews/core/previews_black_list.cc (right): https://codereview.chromium.org/2878363004/diff/1/components/previews/core/pr... components/previews/core/previews_black_list.cc:156: void PreviewsBlackList::ClearBlackListSync(base::Time begin_time, On 2017/05/15 21:15:14, bengr wrote: > How robust is your blacklist logic to the system clock being changed? About as robust as anything that is in ChromeBrowsingDataRemoverDelegate::RemoveEmbedderData. That being said, changing the system clock can have all sorts of repercussions. Specifically, this will remove things that are time-stamped between begin_time and end_time. If the user clears all browsing history, begin will be Time() and end will be Time::Max(), so it will clear everything in that case, which is likely the most important (for instance NTP category ranking only clears anything when all browsing data is cleared). https://codereview.chromium.org/2878363004/diff/1/components/previews/core/pr... File components/previews/core/previews_black_list_unittest.cc (right): https://codereview.chromium.org/2878363004/diff/1/components/previews/core/pr... components/previews/core/previews_black_list_unittest.cc:615: std::map<std::string, std::string> params; On 2017/05/15 21:15:14, bengr wrote: > #include <map> Done. https://codereview.chromium.org/2878363004/diff/1/components/previews/core/pr... components/previews/core/previews_black_list_unittest.cc:631: } tests[] = {{true}, {false}}; On 2017/05/15 21:15:14, bengr wrote: > This is an anti-pattern. Write this as two separate tests. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2878363004/diff/1/components/previews/core/pr... File components/previews/core/previews_black_list.cc (right): https://codereview.chromium.org/2878363004/diff/1/components/previews/core/pr... components/previews/core/previews_black_list.cc:156: void PreviewsBlackList::ClearBlackListSync(base::Time begin_time, On 2017/05/15 21:58:58, RyanSturm wrote: > On 2017/05/15 21:15:14, bengr wrote: > > How robust is your blacklist logic to the system clock being changed? > > About as robust as anything that is in > ChromeBrowsingDataRemoverDelegate::RemoveEmbedderData. That being said, changing > the system clock can have all sorts of repercussions. Specifically, this will > remove things that are time-stamped between begin_time and end_time. If the user > clears all browsing history, begin will be Time() and end will be Time::Max(), > so it will clear everything in that case, which is likely the most important > (for instance NTP category ranking only clears anything when all browsing data > is cleared). OK. I filed https://bugs.chromium.org/p/chromium/issues/detail?id=722890 for a rainy day. https://codereview.chromium.org/2878363004/diff/20001/components/previews/cor... File components/previews/core/previews_black_list_unittest.cc (right): https://codereview.chromium.org/2878363004/diff/20001/components/previews/cor... components/previews/core/previews_black_list_unittest.cc:653: TEST_F(PreviewsBlackListTest, ClearingBlackListClearsRecentNavigation) { I had imagined a helper function so you wouldn't have to duplicate so much code.
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...
bengr: PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
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": 40001, "attempt_start_ts": 1495757845338680,
"parent_rev": "c1604bc83510da9806cb1c00d3351d8fcb7eb24a", "commit_rev":
"4cd77228276c278c78ff13639d319fd9e7198752"}
Message was sent while issue was closed.
Description was changed from ========== If users clear history, the previews recency rule should be cleared When a user clears a chunk of history, the recency rule (previews aren't shown for ~5 minutes after an opt out). Should be reset if the chunk of history is longer than 5 minutes. BUG=722436 ========== to ========== If users clear history, the previews recency rule should be cleared When a user clears a chunk of history, the recency rule (previews aren't shown for ~5 minutes after an opt out). Should be reset if the chunk of history is longer than 5 minutes. BUG=722436 Review-Url: https://codereview.chromium.org/2878363004 Cr-Commit-Position: refs/heads/master@{#474890} Committed: https://chromium.googlesource.com/chromium/src/+/4cd77228276c278c78ff13639d31... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/4cd77228276c278c78ff13639d31... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
