Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2442013003:
Add non-host functionality to the previews blacklist (Closed)
DescriptionAdd non-host functionality to the previews blacklist
This adds a rule to prevent previews navigations when j of k of the last
previews navigations in the past x days were opted out of regardless of
the host. j, k, and x are remotely controlled via finch but are
defaulted to 4, 10, and 100*365 by default.
BUG=639091
Committed: https://crrev.com/72c7c9f37eb3337f951c1529f3135fcebad4f250
Cr-Commit-Position: refs/heads/master@{#430371}
Patch Set 1 #Patch Set 2 : typo #
Total comments: 14
Patch Set 3 : tbansal comments #
Total comments: 19
Patch Set 4 : rebase #Patch Set 5 : tbansal comments #Patch Set 6 : rebase #
Total comments: 16
Patch Set 7 : tbansal comments #Patch Set 8 : fix for test and rebased file #
Total comments: 4
Patch Set 9 : rebase and test #Depends on Patchset: Dependent Patchsets: Messages
Total messages: 65 (53 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-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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 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: + tbansal@chromium.org
tbansal: PTAL
https://codereview.chromium.org/2442013003/diff/20001/components/previews/cor... File components/previews/core/previews_black_list.h (right): https://codereview.chromium.org/2442013003/diff/20001/components/previews/cor... components/previews/core/previews_black_list.h:74: // Returns a new PreviewsBlackListItem for the general black list that does "general" is very vague. Can you use a more specific name "host oblivious"? https://codereview.chromium.org/2442013003/diff/20001/components/previews/cor... File components/previews/core/previews_black_list_item.cc (right): https://codereview.chromium.org/2442013003/diff/20001/components/previews/cor... components/previews/core/previews_black_list_item.cc:39: if (opt_out_records_.size() == 0 || s/opt_out_records_.size() == 0/opt_out_records_.empty()/ https://codereview.chromium.org/2442013003/diff/20001/components/previews/cor... components/previews/core/previews_black_list_item.cc:42: // Adding here would be pointless, as it will be removed afterward. This is a bit confusing. May be it is simpler to just add it to the queue, and then take it out later. https://codereview.chromium.org/2442013003/diff/20001/components/previews/cor... components/previews/core/previews_black_list_item.cc:56: }); Add a DCHECK that previous entry is smaller than the new entry. You may also want to override comparator operator (<) in the struct. That might make this more readable. https://codereview.chromium.org/2442013003/diff/20001/components/previews/cor... File components/previews/core/previews_black_list_item.h (right): https://codereview.chromium.org/2442013003/diff/20001/components/previews/cor... components/previews/core/previews_black_list_item.h:55: base::Time entry_time; // The time that the opt out state was determined. What happens if you keep them as const? I am a bit wary of using non const public fields. https://codereview.chromium.org/2442013003/diff/20001/components/previews/cor... components/previews/core/previews_black_list_item.h:68: std::deque<OptOutRecord> opt_out_records_; Since the list needs to be sorted, why not just use std::priority_queue? https://codereview.chromium.org/2442013003/diff/20001/components/previews/cor... File components/previews/core/previews_experiments.cc (right): https://codereview.chromium.org/2442013003/diff/20001/components/previews/cor... components/previews/core/previews_experiments.cc:63: constexpr int kDefaultGeneralBlackListDurationInDays = 365 * 100; that's a bit long.
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 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...)
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...)
The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) 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: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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...
Patchset #3 (id:80001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
tbansal: PTAL https://codereview.chromium.org/2442013003/diff/20001/components/previews/cor... File components/previews/core/previews_black_list.h (right): https://codereview.chromium.org/2442013003/diff/20001/components/previews/cor... components/previews/core/previews_black_list.h:74: // Returns a new PreviewsBlackListItem for the general black list that does On 2016/10/21 22:56:26, tbansal1 wrote: > "general" is very vague. Can you use a more specific name "host oblivious"? Done. https://codereview.chromium.org/2442013003/diff/20001/components/previews/cor... File components/previews/core/previews_black_list_item.cc (right): https://codereview.chromium.org/2442013003/diff/20001/components/previews/cor... components/previews/core/previews_black_list_item.cc:39: if (opt_out_records_.size() == 0 || On 2016/10/21 22:56:26, tbansal1 wrote: > s/opt_out_records_.size() == 0/opt_out_records_.empty()/ Done. https://codereview.chromium.org/2442013003/diff/20001/components/previews/cor... components/previews/core/previews_black_list_item.cc:42: // Adding here would be pointless, as it will be removed afterward. On 2016/10/21 22:56:26, tbansal1 wrote: > This is a bit confusing. May be it is simpler to just add it to the queue, and > then take it out later. Done. https://codereview.chromium.org/2442013003/diff/20001/components/previews/cor... components/previews/core/previews_black_list_item.cc:56: }); On 2016/10/21 22:56:26, tbansal1 wrote: > Add a DCHECK that previous entry is smaller than the new entry. > > You may also want to override comparator operator (<) in the struct. That might > make this more readable. Done. https://codereview.chromium.org/2442013003/diff/20001/components/previews/cor... File components/previews/core/previews_black_list_item.h (right): https://codereview.chromium.org/2442013003/diff/20001/components/previews/cor... components/previews/core/previews_black_list_item.h:55: base::Time entry_time; // The time that the opt out state was determined. On 2016/10/21 22:56:26, tbansal1 wrote: > What happens if you keep them as const? > I am a bit wary of using non const public fields. Since priority_queue is basically a vector of these, to re-order it reassigns values, so this can't be const. Should I transition this to class? https://codereview.chromium.org/2442013003/diff/20001/components/previews/cor... components/previews/core/previews_black_list_item.h:68: std::deque<OptOutRecord> opt_out_records_; On 2016/10/21 22:56:26, tbansal1 wrote: > Since the list needs to be sorted, why not just use std::priority_queue? Done. https://codereview.chromium.org/2442013003/diff/20001/components/previews/cor... File components/previews/core/previews_experiments.cc (right): https://codereview.chromium.org/2442013003/diff/20001/components/previews/cor... components/previews/core/previews_experiments.cc:63: constexpr int kDefaultGeneralBlackListDurationInDays = 365 * 100; On 2016/10/21 22:56:26, tbansal1 wrote: > that's a bit long. Done.
https://codereview.chromium.org/2442013003/diff/100001/components/previews/co... File components/previews/core/previews_black_list.cc (right): https://codereview.chromium.org/2442013003/diff/100001/components/previews/co... components/previews/core/previews_black_list.cc:115: DCHECK(host_indifferent_black_list_item_); This DCHECK is not really necessary. If |host_indifferent_black_list_item_| is null, it would crash on line 117 anyways. https://codereview.chromium.org/2442013003/diff/100001/components/previews/co... File components/previews/core/previews_black_list_item.cc (right): https://codereview.chromium.org/2442013003/diff/100001/components/previews/co... components/previews/core/previews_black_list_item.cc:27: return (entry_time_ == other.entry_time_) ? opt_out_ > other.opt_out_ Why not use std::tie? That's slightly more readable. https://cs.chromium.org/chromium/src/net/nqe/network_id.h?rcl=0&l=82 https://codereview.chromium.org/2442013003/diff/100001/components/previews/co... components/previews/core/previews_black_list_item.cc:56: DCHECK(opt_out_records_.top().entry_time() <= entry_time); DCHECK_LE https://codereview.chromium.org/2442013003/diff/100001/components/previews/co... components/previews/core/previews_black_list_item.cc:56: DCHECK(opt_out_records_.top().entry_time() <= entry_time); Add DCHECK_EQ(opt_out_records_.size(), max_stored_history_length_+1); https://codereview.chromium.org/2442013003/diff/100001/components/previews/co... File components/previews/core/previews_black_list_item.h (right): https://codereview.chromium.org/2442013003/diff/100001/components/previews/co... components/previews/core/previews_black_list_item.h:61: bool opt_out() const { return opt_out_; } nit: reverse the order of functions to match the order in which the variables are declared below. https://codereview.chromium.org/2442013003/diff/100001/components/previews/co... components/previews/core/previews_black_list_item.h:83: // maintained as a list sorted by entry_time (earliest to latest). "sorted" stale comment Also, mention that top element has the highest priority for removal. https://codereview.chromium.org/2442013003/diff/100001/components/previews/co... File components/previews/core/previews_black_list_unittest.cc (right): https://codereview.chromium.org/2442013003/diff/100001/components/previews/co... components/previews/core/previews_black_list_unittest.cc:307: EXPECT_TRUE(black_list->IsLoadedAndAllowed(url_a, PreviewsType::OFFLINE)); Why did these become true? https://codereview.chromium.org/2442013003/diff/100001/components/previews/co... components/previews/core/previews_black_list_unittest.cc:315: TEST_F(PreviewsBlackListTest, HostIndifferentBlackListWithStore) { IIUC, this test is not really much different from the test above since PreviewsBlackList::IsLoadedAndAllowed() just checks the in-memory blacklist, and does not query the opt-out store. It might be useful to have a TestOptOutStore that backs stuff into the memory, then recreate the PreviewsBlackList with a TestOptOutStore that already has some data in it, and verify if TestOptOutStore reads the data correctly. https://codereview.chromium.org/2442013003/diff/100001/components/previews/co... File components/previews/core/previews_opt_out_store_sql.cc (right): https://codereview.chromium.org/2442013003/diff/100001/components/previews/co... components/previews/core/previews_opt_out_store_sql.cc:156: " FROM " PREVIEWS_TABLE_NAME " ORDER BY host_name, time DESC"; May be add a comment explaining why ordering is important so somebody else in future does not remove it (thinking that ordering is just imposing unnecessary computation overhead).
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...
tbansal: PTAL https://codereview.chromium.org/2442013003/diff/100001/components/previews/co... File components/previews/core/previews_black_list.cc (right): https://codereview.chromium.org/2442013003/diff/100001/components/previews/co... components/previews/core/previews_black_list.cc:115: DCHECK(host_indifferent_black_list_item_); On 2016/10/25 16:15:47, tbansal1 wrote: > This DCHECK is not really necessary. If |host_indifferent_black_list_item_| is > null, it would crash on line 117 anyways. Done. https://codereview.chromium.org/2442013003/diff/100001/components/previews/co... File components/previews/core/previews_black_list_item.cc (right): https://codereview.chromium.org/2442013003/diff/100001/components/previews/co... components/previews/core/previews_black_list_item.cc:27: return (entry_time_ == other.entry_time_) ? opt_out_ > other.opt_out_ On 2016/10/25 16:15:47, tbansal1 wrote: > Why not use std::tie? > That's slightly more readable. > https://cs.chromium.org/chromium/src/net/nqe/network_id.h?rcl=0&l=82 couldn't remember what tie was called. Thanks. https://codereview.chromium.org/2442013003/diff/100001/components/previews/co... components/previews/core/previews_black_list_item.cc:56: DCHECK(opt_out_records_.top().entry_time() <= entry_time); On 2016/10/25 16:15:47, tbansal1 wrote: > DCHECK_LE Done. https://codereview.chromium.org/2442013003/diff/100001/components/previews/co... components/previews/core/previews_black_list_item.cc:56: DCHECK(opt_out_records_.top().entry_time() <= entry_time); On 2016/10/25 16:15:47, tbansal1 wrote: > Add DCHECK_EQ(opt_out_records_.size(), max_stored_history_length_+1); Done. https://codereview.chromium.org/2442013003/diff/100001/components/previews/co... File components/previews/core/previews_black_list_item.h (right): https://codereview.chromium.org/2442013003/diff/100001/components/previews/co... components/previews/core/previews_black_list_item.h:61: bool opt_out() const { return opt_out_; } On 2016/10/25 16:15:48, tbansal1 wrote: > nit: reverse the order of functions to match the order in which the variables > are declared below. Done. https://codereview.chromium.org/2442013003/diff/100001/components/previews/co... components/previews/core/previews_black_list_item.h:83: // maintained as a list sorted by entry_time (earliest to latest). On 2016/10/25 16:15:48, tbansal1 wrote: > "sorted" > stale comment > > Also, mention that top element has the highest priority for removal. Done. https://codereview.chromium.org/2442013003/diff/100001/components/previews/co... File components/previews/core/previews_black_list_unittest.cc (right): https://codereview.chromium.org/2442013003/diff/100001/components/previews/co... components/previews/core/previews_black_list_unittest.cc:307: EXPECT_TRUE(black_list->IsLoadedAndAllowed(url_a, PreviewsType::OFFLINE)); On 2016/10/25 16:15:48, tbansal1 wrote: > Why did these become true? Added comment. https://codereview.chromium.org/2442013003/diff/100001/components/previews/co... components/previews/core/previews_black_list_unittest.cc:315: TEST_F(PreviewsBlackListTest, HostIndifferentBlackListWithStore) { On 2016/10/25 16:15:48, tbansal1 wrote: > IIUC, this test is not really much different from the test above since > PreviewsBlackList::IsLoadedAndAllowed() just checks the in-memory blacklist, and > does not query the opt-out store. > > It might be useful to have a TestOptOutStore that backs stuff into the memory, > then recreate the PreviewsBlackList with a TestOptOutStore that already has some > data in it, and verify if TestOptOutStore reads the data correctly. Adding that seems like overkill. When I add the opt out store sql tests, they should cover this. https://codereview.chromium.org/2442013003/diff/100001/components/previews/co... File components/previews/core/previews_opt_out_store_sql.cc (right): https://codereview.chromium.org/2442013003/diff/100001/components/previews/co... components/previews/core/previews_opt_out_store_sql.cc:156: " FROM " PREVIEWS_TABLE_NAME " ORDER BY host_name, time DESC"; On 2016/10/25 16:15:48, tbansal1 wrote: > May be add a comment explaining why ordering is important so somebody else in > future does not remove it (thinking that ordering is just imposing unnecessary > computation overhead). Done.
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 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: This issue passed the CQ dry run.
mostly nits. https://codereview.chromium.org/2442013003/diff/100001/components/previews/co... File components/previews/core/previews_black_list_unittest.cc (right): https://codereview.chromium.org/2442013003/diff/100001/components/previews/co... components/previews/core/previews_black_list_unittest.cc:315: TEST_F(PreviewsBlackListTest, HostIndifferentBlackListWithStore) { On 2016/10/25 18:15:44, Ryan Sturm wrote: > On 2016/10/25 16:15:48, tbansal1 wrote: > > IIUC, this test is not really much different from the test above since > > PreviewsBlackList::IsLoadedAndAllowed() just checks the in-memory blacklist, > and > > does not query the opt-out store. > > > > It might be useful to have a TestOptOutStore that backs stuff into the memory, > > then recreate the PreviewsBlackList with a TestOptOutStore that already has > some > > data in it, and verify if TestOptOutStore reads the data correctly. > > Adding that seems like overkill. When I add the opt out store sql tests, they > should cover this. May be. What I am suggesting is testing the integration of PreviewsBlackList with a class that implements OptOutStore. More like an integration test between the two. https://codereview.chromium.org/2442013003/diff/160001/components/previews/co... File components/previews/core/previews_black_list.cc (right): https://codereview.chromium.org/2442013003/diff/160001/components/previews/co... components/previews/core/previews_black_list.cc:175: DCHECK(host_indifferent_black_list_item); super nit:put DCHECK in the same order as function arguments. https://codereview.chromium.org/2442013003/diff/160001/components/previews/co... File components/previews/core/previews_black_list_unittest.cc (right): https://codereview.chromium.org/2442013003/diff/160001/components/previews/co... components/previews/core/previews_black_list_unittest.cc:88: const int host_indifferent_threshold = 2; s/2/host_indifferent_history + 1/ Also, why are some size_t, others int? here and below. https://codereview.chromium.org/2442013003/diff/160001/components/previews/co... components/previews/core/previews_black_list_unittest.cc:262: const int per_host_threshold = 2; s/2/per_host_history + 1/ https://codereview.chromium.org/2442013003/diff/160001/components/previews/co... components/previews/core/previews_black_list_unittest.cc:309: black_list->AddPreviewNavigation(url_d, true, PreviewsType::OFFLINE); can you put this in a loop: for(size_t i=0; i< host_indifferent_threshold; i++) { black_list->AddPreviewNavigation(url[i], true, PreviewsType::OFFLINE); EXPECT_EQ(i!=3, black_list->IsLoadedAndAllowed(url_a, PreviewsType::OFFLINE)); } This will make it clear that 4 is linked to threshold of 4 set earlier. https://codereview.chromium.org/2442013003/diff/160001/components/previews/co... components/previews/core/previews_black_list_unittest.cc:330: // loading are queued and should run in the order they were queued. s/loading/loading opt-out store/ It is not clear what loading is here (loading webpage?) https://codereview.chromium.org/2442013003/diff/160001/components/previews/co... components/previews/core/previews_black_list_unittest.cc:481: const int host_indifferent_threshold = 2; same comment here and elsewhere: s/2/host_indifferent_history + 1/ https://codereview.chromium.org/2442013003/diff/160001/components/previews/co... File components/previews/core/previews_experiments.cc (right): https://codereview.chromium.org/2442013003/diff/160001/components/previews/co... components/previews/core/previews_experiments.cc:151: return base::TimeDelta::FromDays(kDefaultHostIndifferentDurationInDays); why not just use 365*100 directly like other functions above? https://codereview.chromium.org/2442013003/diff/160001/components/previews/co... File components/previews/core/previews_experiments.h (right): https://codereview.chromium.org/2442013003/diff/160001/components/previews/co... components/previews/core/previews_experiments.h:25: // The number of recent navigations that were opted out of that would trigger s/opted out of that would trigger the host/opted out of for a given host that would trigger that host/
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-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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: This issue passed the CQ dry run.
tbansal: PTAL https://codereview.chromium.org/2442013003/diff/160001/components/previews/co... File components/previews/core/previews_black_list.cc (right): https://codereview.chromium.org/2442013003/diff/160001/components/previews/co... components/previews/core/previews_black_list.cc:175: DCHECK(host_indifferent_black_list_item); On 2016/10/28 21:59:33, tbansal1 wrote: > super nit:put DCHECK in the same order as function arguments. Done. https://codereview.chromium.org/2442013003/diff/160001/components/previews/co... File components/previews/core/previews_black_list_unittest.cc (right): https://codereview.chromium.org/2442013003/diff/160001/components/previews/co... components/previews/core/previews_black_list_unittest.cc:88: const int host_indifferent_threshold = 2; On 2016/10/28 21:59:33, tbansal1 wrote: > s/2/host_indifferent_history + 1/ > Also, why are some size_t, others int? > > here and below. Done. size_t is used here for the actual size of a collection, whereas the ints are used internally for counts. Open a bug on it if you want it fixed throughout the blacklist. I don't think it's a problem. https://codereview.chromium.org/2442013003/diff/160001/components/previews/co... components/previews/core/previews_black_list_unittest.cc:262: const int per_host_threshold = 2; On 2016/10/28 21:59:34, tbansal1 wrote: > s/2/per_host_history + 1/ Done. https://codereview.chromium.org/2442013003/diff/160001/components/previews/co... components/previews/core/previews_black_list_unittest.cc:309: black_list->AddPreviewNavigation(url_d, true, PreviewsType::OFFLINE); On 2016/10/28 21:59:33, tbansal1 wrote: > can you put this in a loop: > for(size_t i=0; i< host_indifferent_threshold; i++) { > black_list->AddPreviewNavigation(url[i], true, PreviewsType::OFFLINE); > EXPECT_EQ(i!=3, black_list->IsLoadedAndAllowed(url_a, PreviewsType::OFFLINE)); > } > > This will make it clear that 4 is linked to threshold of 4 set earlier. Done. https://codereview.chromium.org/2442013003/diff/160001/components/previews/co... components/previews/core/previews_black_list_unittest.cc:330: // loading are queued and should run in the order they were queued. On 2016/10/28 21:59:34, tbansal1 wrote: > s/loading/loading opt-out store/ > > It is not clear what loading is here (loading webpage?) Done. https://codereview.chromium.org/2442013003/diff/160001/components/previews/co... components/previews/core/previews_black_list_unittest.cc:481: const int host_indifferent_threshold = 2; On 2016/10/28 21:59:33, tbansal1 wrote: > same comment here and elsewhere: > s/2/host_indifferent_history + 1/ Done. https://codereview.chromium.org/2442013003/diff/160001/components/previews/co... File components/previews/core/previews_experiments.cc (right): https://codereview.chromium.org/2442013003/diff/160001/components/previews/co... components/previews/core/previews_experiments.cc:151: return base::TimeDelta::FromDays(kDefaultHostIndifferentDurationInDays); On 2016/10/28 21:59:34, tbansal1 wrote: > why not just use 365*100 directly like other functions above? Done. https://codereview.chromium.org/2442013003/diff/160001/components/previews/co... File components/previews/core/previews_experiments.h (right): https://codereview.chromium.org/2442013003/diff/160001/components/previews/co... components/previews/core/previews_experiments.h:25: // The number of recent navigations that were opted out of that would trigger On 2016/10/28 21:59:34, tbansal1 wrote: > s/opted out of that would trigger the host/opted out of for a given host that > would trigger that host/ Done.
lgtm % 2 nits. https://codereview.chromium.org/2442013003/diff/200001/components/previews/co... File components/previews/core/previews_black_list_item.cc (right): https://codereview.chromium.org/2442013003/diff/200001/components/previews/co... components/previews/core/previews_black_list_item.cc:39: total_opt_out_(0) {} DCHECK_LE(opt_out_black_list_threshold, stored_history_length); this would help in catching errors where the caller passes them in reverse order by mistake. https://codereview.chromium.org/2442013003/diff/200001/components/previews/co... File components/previews/core/previews_opt_out_store_sql.cc (right): https://codereview.chromium.org/2442013003/diff/200001/components/previews/co... components/previews/core/previews_opt_out_store_sql.cc:182: host_indifferent_black_list_item->AddPreviewNavigation( May be add a comment here similar to above that the internal logic will keep evicting older entries.
The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run
https://codereview.chromium.org/2442013003/diff/200001/components/previews/co... File components/previews/core/previews_black_list_item.cc (right): https://codereview.chromium.org/2442013003/diff/200001/components/previews/co... components/previews/core/previews_black_list_item.cc:39: total_opt_out_(0) {} On 2016/11/07 19:23:36, tbansal1 wrote: > DCHECK_LE(opt_out_black_list_threshold, stored_history_length); > this would help in catching errors where the caller passes them in reverse order > by mistake This is abused in unit_tests, and for good reason. I'm not going to add it. https://codereview.chromium.org/2442013003/diff/200001/components/previews/co... File components/previews/core/previews_opt_out_store_sql.cc (right): https://codereview.chromium.org/2442013003/diff/200001/components/previews/co... components/previews/core/previews_opt_out_store_sql.cc:182: host_indifferent_black_list_item->AddPreviewNavigation( On 2016/11/07 19:23:36, tbansal1 wrote: > May be add a comment here similar to above that the internal logic will keep > evicting older entries. Done.
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 ryansturm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tbansal@chromium.org Link to the patchset: https://codereview.chromium.org/2442013003/#ps220001 (title: "rebase and test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #9 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Add non-host functionality to the previews blacklist This adds a rule to prevent previews navigations when j of k of the last previews navigations in the past x days were opted out of regardless of the host. j, k, and x are remotely controlled via finch but are defaulted to 4, 10, and 100*365 by default. BUG=639091 ========== to ========== Add non-host functionality to the previews blacklist This adds a rule to prevent previews navigations when j of k of the last previews navigations in the past x days were opted out of regardless of the host. j, k, and x are remotely controlled via finch but are defaulted to 4, 10, and 100*365 by default. BUG=639091 Committed: https://crrev.com/72c7c9f37eb3337f951c1529f3135fcebad4f250 Cr-Commit-Position: refs/heads/master@{#430371} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/72c7c9f37eb3337f951c1529f3135fcebad4f250 Cr-Commit-Position: refs/heads/master@{#430371} |
