Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2335023002:
Adding a previews IO-thread blacklist (Closed)
DescriptionAdding a previews IO-thread blacklist
This introduces an in memory blacklist for previews. This also
introduces an interface for retrieiving prior sessions information from
a store. The blacklist is keyed on fully qualified domain name and will
blacklist domains based on user history of opt-outs on those domains.
This does not yet implement adding preview navigations to the blacklist
or using the blacklist to determine site navigation eligibility. Nor
does this CL implement the backing store.
BUG=639087
Committed: https://crrev.com/d0b4726daad2e6f410df05299bc5d37c23aa339c
Cr-Commit-Position: refs/heads/master@{#420726}
Patch Set 1 #Patch Set 2 : missing file #Patch Set 3 : unittests #Patch Set 4 : RunLoop #Patch Set 5 : updated comments #
Total comments: 12
Patch Set 6 : tbansal comments #
Total comments: 42
Patch Set 7 : tbansal comments #Patch Set 8 : build.gn dependency on gurl #
Total comments: 12
Patch Set 9 : tbansal comments #
Total comments: 16
Patch Set 10 : tbansal comments #Patch Set 11 : tbansal comments #
Total comments: 34
Patch Set 12 : Adding host limit and addressing comments #Patch Set 13 : big rebase #Patch Set 14 : typo in include #Patch Set 15 : another typo #
Total comments: 34
Patch Set 16 : tbansal comments #
Total comments: 50
Patch Set 17 : tbansal comments #
Total comments: 20
Patch Set 18 : rebase #Patch Set 19 : tbansal comments #Messages
Total messages: 107 (84 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 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: 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 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: 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
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 @ *, It's pretty large, but the only way I see to split it up would save only a hundred lines or so. I'm not expecting super quick feedback since it is large.
https://codereview.chromium.org/2335023002/diff/80001/components/previews/pre... File components/previews/previews_black_list.cc (right): https://codereview.chromium.org/2335023002/diff/80001/components/previews/pre... components/previews/previews_black_list.cc:69: void PreviewsBlackList::ClearBlackList(const base::Time& begin_time, Can this be done in a separate CL? https://codereview.chromium.org/2335023002/diff/80001/components/previews/pre... File components/previews/previews_black_list.h (right): https://codereview.chromium.org/2335023002/diff/80001/components/previews/pre... components/previews/previews_black_list.h:37: PreviewsBlackList(std::unique_ptr<PreviewsOptOutStore> opt_out_store); explicit. https://codereview.chromium.org/2335023002/diff/80001/components/previews/pre... components/previews/previews_black_list.h:51: // If LoadBlackListDone has not been called yet, this will always return what is LoadBlackListDone? https://codereview.chromium.org/2335023002/diff/80001/components/previews/pre... components/previews/previews_black_list.h:56: // |opt_out_store_| to delete entries between |begin_time| and |end_time|. Generally, documentation of public functions should not refer to private variables. https://codereview.chromium.org/2335023002/diff/80001/components/previews/pre... components/previews/previews_black_list.h:71: PreviewsBlackListItem* GetBlackListItem(std::string host_name, pass std::string as const ref? https://codereview.chromium.org/2335023002/diff/80001/components/previews/pre... File components/previews/previews_black_list_item.h (right): https://codereview.chromium.org/2335023002/diff/80001/components/previews/pre... components/previews/previews_black_list_item.h:37: struct TimeOptOut { comments please.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Adding a previews IO-thread blacklist This introduces an in memory blacklist for previews. This also introduces an interface for retrieiving prior sessions information from a store. The blacklist is keyed on fully qualified domain name and will blacklist domains based on user history of opt-outs on those domains. This does not yet implement adding preview navigations to the blacklist or using the blacklist to determine site navigation eligibility. Nor does this CL implement the backign store. BUG=639087 ========== to ========== Adding a previews IO-thread blacklist This introduces an in memory blacklist for previews. This also introduces an interface for retrieiving prior sessions information from a store. The blacklist is keyed on fully qualified domain name and will blacklist domains based on user history of opt-outs on those domains. This does not yet implement adding preview navigations to the blacklist or using the blacklist to determine site navigation eligibility. Nor does this CL implement the backing store. BUG=639087 ==========
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 #6 (id:100001) has been deleted
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 @ *, I removed clear black list and ui-thread functionality to save some lines. https://codereview.chromium.org/2335023002/diff/80001/components/previews/pre... File components/previews/previews_black_list.cc (right): https://codereview.chromium.org/2335023002/diff/80001/components/previews/pre... components/previews/previews_black_list.cc:69: void PreviewsBlackList::ClearBlackList(const base::Time& begin_time, On 2016/09/13 23:52:45, tbansal1 wrote: > Can this be done in a separate CL? Done. https://codereview.chromium.org/2335023002/diff/80001/components/previews/pre... File components/previews/previews_black_list.h (right): https://codereview.chromium.org/2335023002/diff/80001/components/previews/pre... components/previews/previews_black_list.h:37: PreviewsBlackList(std::unique_ptr<PreviewsOptOutStore> opt_out_store); On 2016/09/13 23:52:45, tbansal1 wrote: > explicit. Done. https://codereview.chromium.org/2335023002/diff/80001/components/previews/pre... components/previews/previews_black_list.h:51: // If LoadBlackListDone has not been called yet, this will always return On 2016/09/13 23:52:45, tbansal1 wrote: > what is LoadBlackListDone? Done. https://codereview.chromium.org/2335023002/diff/80001/components/previews/pre... components/previews/previews_black_list.h:56: // |opt_out_store_| to delete entries between |begin_time| and |end_time|. On 2016/09/13 23:52:45, tbansal1 wrote: > Generally, documentation of public functions should not refer to private > variables. Done. https://codereview.chromium.org/2335023002/diff/80001/components/previews/pre... components/previews/previews_black_list.h:71: PreviewsBlackListItem* GetBlackListItem(std::string host_name, On 2016/09/13 23:52:45, tbansal1 wrote: > pass std::string as const ref? Done. https://codereview.chromium.org/2335023002/diff/80001/components/previews/pre... File components/previews/previews_black_list_item.h (right): https://codereview.chromium.org/2335023002/diff/80001/components/previews/pre... components/previews/previews_black_list_item.h:37: struct TimeOptOut { On 2016/09/13 23:52:45, tbansal1 wrote: > comments please. Done.
https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... File components/previews/previews_black_list.cc (right): https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_black_list.cc:34: DCHECK(thread_checker_.CalledOnValidThread()); Thread checker is typically the first check. https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_black_list.cc:42: weak_factory_.GetWeakPtr(), host_name, opt_out, Since the pending task run on the same thread, you can pass in the raw ptr as well. Since |this| owns the pending task queue, if |this| is deleted, no tasks would run anyways. https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... File components/previews/previews_black_list.h (right): https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_black_list.h:38: // information, and can be null. May be explain how the |opt_out_store| is used? It is not clear what happens when it is null vs. when it is not. https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_black_list.h:49: void AddPreviewNavigation(const std::string& host_name, Is it possible to pass GURL? So that, the conversion from GURL -> Host ad vice-versa is internal to the black list code. https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_black_list.h:55: bool IsLoadedAndAllowed(const std::string& host_name); Why does this function not take PreviewsType as input? is the blacklist indexed by type? https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_black_list.h:92: std::queue<QueueClosure> pending_callbacks_; Is the queue really needed? Is it possible to just drop the events and record UMA for that? If it happens too frequent, we can possibly do something.
more comments. https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... File components/previews/previews_black_list_item.cc (right): https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_black_list_item.cc:9: #include "base/time/time.h" No need to include here since it is included in .h file. https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_black_list_item.cc:15: const int stored_history_length, No need to pass int arguments as cont since they are passed by value. https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_black_list_item.cc:17: const base::TimeDelta& black_list_duration) base::Time* should be passed by value. https://cs.chromium.org/chromium/src/base/time/time.h?rcl=0&l=24 https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... File components/previews/previews_black_list_item.h (right): https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_black_list_item.h:54: const int stored_history_length_; can this be size_t, and same for other variables that record length? https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_black_list_item.h:60: // The |stored_history_length_| most recent preivews navigation. Is maintained typo in previews. https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_black_list_item.h:72: BlackListItemMap; Why is this defined here since this is not used in .cc. https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... File components/previews/previews_experiments.cc (right): https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_experiments.cc:42: // |param|, reutnrs an empty string. typo in returns https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_experiments.cc:43: std::string ParamValue(std::string param) { pass |param| by const ref https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_experiments.cc:119: const int opt_out_threshold, Why is this function needed? Can you not set the variation param using the test framework provided already. https://cs.chromium.org/chromium/src/testing/gtest/include/gtest/gtest.h?l=44... https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... File components/previews/previews_ui_service.cc (right): https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_ui_service.cc:8: #include "base/files/file_path.h" Is this include needed? https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_ui_service.cc:9: #include "base/sequenced_task_runner.h" forward declare SingleThreadTaskRunner
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.
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.
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/2335023002/diff/120001/components/previews/pr... File components/previews/previews_black_list.cc (right): https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_black_list.cc:34: DCHECK(thread_checker_.CalledOnValidThread()); On 2016/09/14 17:00:03, tbansal1 wrote: > Thread checker is typically the first check. Done. https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_black_list.cc:42: weak_factory_.GetWeakPtr(), host_name, opt_out, On 2016/09/14 17:00:03, tbansal1 wrote: > Since the pending task run on the same thread, you can pass in the raw ptr as > well. Since |this| owns the pending task queue, if |this| is deleted, no tasks > would run anyways. Done. https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... File components/previews/previews_black_list.h (right): https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_black_list.h:38: // information, and can be null. On 2016/09/14 17:00:03, tbansal1 wrote: > May be explain how the |opt_out_store| is used? It is not clear what happens > when it is null vs. when it is not. Done. https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_black_list.h:49: void AddPreviewNavigation(const std::string& host_name, On 2016/09/14 17:00:03, tbansal1 wrote: > Is it possible to pass GURL? So that, the conversion from GURL -> Host ad > vice-versa is internal to the black list code. Done. https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_black_list.h:55: bool IsLoadedAndAllowed(const std::string& host_name); On 2016/09/14 17:00:03, tbansal1 wrote: > Why does this function not take PreviewsType as input? is the blacklist indexed > by type? The store will track PreviewsType, but PreviewsType is not part of the decision right now. The goal of this is to allow the database to be created and have all the information we need in all versions. I'll add a PreviewsType to this, but not use it in decision making to make the API seem clearer. https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_black_list.h:92: std::queue<QueueClosure> pending_callbacks_; On 2016/09/14 17:00:03, tbansal1 wrote: > Is the queue really needed? Is it possible to just drop the events and record > UMA for that? If it happens too frequent, we can possibly do something. I'd rather have it. It's possible, but unlikely that it is really needed. It keeps the in-memory map and backing store in sync, so I'd prefer to keep it. https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... File components/previews/previews_black_list_item.cc (right): https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_black_list_item.cc:9: #include "base/time/time.h" On 2016/09/14 17:17:28, tbansal1 wrote: > No need to include here since it is included in .h file. Done. https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_black_list_item.cc:15: const int stored_history_length, On 2016/09/14 17:17:28, tbansal1 wrote: > No need to pass int arguments as cont since they are passed by value. Done. https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_black_list_item.cc:17: const base::TimeDelta& black_list_duration) On 2016/09/14 17:17:27, tbansal1 wrote: > base::Time* should be passed by value. > https://cs.chromium.org/chromium/src/base/time/time.h?rcl=0&l=24 Done. https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... File components/previews/previews_black_list_item.h (right): https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_black_list_item.h:54: const int stored_history_length_; On 2016/09/14 17:17:29, tbansal1 wrote: > can this be size_t, and same for other variables that record length? Done. https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_black_list_item.h:60: // The |stored_history_length_| most recent preivews navigation. Is maintained On 2016/09/14 17:17:28, tbansal1 wrote: > typo in previews. Done. https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_black_list_item.h:72: BlackListItemMap; On 2016/09/14 17:17:28, tbansal1 wrote: > Why is this defined here since this is not used in .cc. Moved to opt_out_store.h https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... File components/previews/previews_experiments.cc (right): https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_experiments.cc:42: // |param|, reutnrs an empty string. On 2016/09/14 17:17:29, tbansal1 wrote: > typo in returns Done. https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_experiments.cc:43: std::string ParamValue(std::string param) { On 2016/09/14 17:17:30, tbansal1 wrote: > pass |param| by const ref Done. https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_experiments.cc:119: const int opt_out_threshold, On 2016/09/14 17:17:30, tbansal1 wrote: > Why is this function needed? Can you not set the variation param using the test > framework provided already. > https://cs.chromium.org/chromium/src/testing/gtest/include/gtest/gtest.h?l=44... This uses internal strings that I don't need to expose outside of this class. https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... File components/previews/previews_ui_service.cc (right): https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_ui_service.cc:8: #include "base/files/file_path.h" On 2016/09/14 17:17:30, tbansal1 wrote: > Is this include needed? Done. https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_ui_service.cc:9: #include "base/sequenced_task_runner.h" On 2016/09/14 17:17:31, tbansal1 wrote: > forward declare SingleThreadTaskRunner Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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/2335023002/diff/120001/components/previews/pr... File components/previews/previews_black_list.h (right): https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_black_list.h:55: bool IsLoadedAndAllowed(const std::string& host_name); On 2016/09/14 18:36:42, Ryan Sturm wrote: > On 2016/09/14 17:00:03, tbansal1 wrote: > > Why does this function not take PreviewsType as input? is the blacklist > indexed > > by type? > > The store will track PreviewsType, but PreviewsType is not part of the decision > right now. The goal of this is to allow the database to be created and have all > the information we need in all versions. > > I'll add a PreviewsType to this, but not use it in decision making to make the > API seem clearer. okay, please add PreviewType so that callers do not have to make any modifications once you really start using this. https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_black_list.h:92: std::queue<QueueClosure> pending_callbacks_; On 2016/09/14 18:36:42, Ryan Sturm wrote: > On 2016/09/14 17:00:03, tbansal1 wrote: > > Is the queue really needed? Is it possible to just drop the events and record > > UMA for that? If it happens too frequent, we can possibly do something. > > I'd rather have it. It's possible, but unlikely that it is really needed. It > keeps the in-memory map and backing store in sync, so I'd prefer to keep it. You can keep them in sync by simply dropping events until the map is loaded. This basically means loss of recording of an opt-out in the worst case (may be also add UMA to track that). https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... File components/previews/previews_experiments.cc (right): https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_experiments.cc:92: } nit: space between params. also, add the comment // namespace params. https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_experiments.cc:119: const int opt_out_threshold, On 2016/09/14 18:36:43, Ryan Sturm wrote: > On 2016/09/14 17:17:30, tbansal1 wrote: > > Why is this function needed? Can you not set the variation param using the > test > > framework provided already. > > > https://cs.chromium.org/chromium/src/testing/gtest/include/gtest/gtest.h?l=44... > > This uses internal strings that I don't need to expose outside of this class. These are not really internal strings since they will be used outside of this class (in the experiment configs on the server side). The goal typically is to reduce the release binary size. So, I would still prefer if this is removed. https://cs.chromium.org/chromium/src/testing/gtest/include/gtest/gtest.h?l=44... https://codereview.chromium.org/2335023002/diff/160001/components/previews/pr... File components/previews/previews_black_list.cc (right): https://codereview.chromium.org/2335023002/diff/160001/components/previews/pr... components/previews/previews_black_list.cc:65: if (!loaded_ || !previews::params::BlackListParamsAreValid()) { nit: no parenthesis required. https://codereview.chromium.org/2335023002/diff/160001/components/previews/pr... File components/previews/previews_black_list.h (right): https://codereview.chromium.org/2335023002/diff/160001/components/previews/pr... components/previews/previews_black_list.h:63: // navigation to backing store. |opt_out| is whether the uesr opted out of typo in uesr. https://codereview.chromium.org/2335023002/diff/160001/components/previews/pr... components/previews/previews_black_list.h:64: // the preview or navigated away from the page by another way. |type| is only What is "another way"? https://codereview.chromium.org/2335023002/diff/160001/components/previews/pr... components/previews/previews_black_list.h:66: void AddPreviewNavigationSync(const GURL& host_name, For this function, you can simply say: "Synchronous version of AddPreviewNavigation method". I do not think there is a need to duplicate comments. https://codereview.chromium.org/2335023002/diff/160001/components/previews/pr... File components/previews/previews_black_list_item.h (right): https://codereview.chromium.org/2335023002/diff/160001/components/previews/pr... components/previews/previews_black_list_item.h:39: struct TimeOptOut { Why is this public? https://codereview.chromium.org/2335023002/diff/160001/components/previews/pr... components/previews/previews_black_list_item.h:61: // as a sorted vector. sorted by what?
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...
tbansal: ptal @ * https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... File components/previews/previews_black_list.h (right): https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_black_list.h:55: bool IsLoadedAndAllowed(const std::string& host_name); On 2016/09/14 20:17:50, tbansal1 wrote: > On 2016/09/14 18:36:42, Ryan Sturm wrote: > > On 2016/09/14 17:00:03, tbansal1 wrote: > > > Why does this function not take PreviewsType as input? is the blacklist > > indexed > > > by type? > > > > The store will track PreviewsType, but PreviewsType is not part of the > decision > > right now. The goal of this is to allow the database to be created and have > all > > the information we need in all versions. > > > > I'll add a PreviewsType to this, but not use it in decision making to make > the > > API seem clearer. > > okay, please add PreviewType so that callers do not have to make any > modifications once you really start using this. Done. https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_black_list.h:92: std::queue<QueueClosure> pending_callbacks_; On 2016/09/14 20:17:50, tbansal1 wrote: > On 2016/09/14 18:36:42, Ryan Sturm wrote: > > On 2016/09/14 17:00:03, tbansal1 wrote: > > > Is the queue really needed? Is it possible to just drop the events and > record > > > UMA for that? If it happens too frequent, we can possibly do something. > > > > I'd rather have it. It's possible, but unlikely that it is really needed. It > > keeps the in-memory map and backing store in sync, so I'd prefer to keep it. > > You can keep them in sync by simply dropping events until the map is loaded. > This basically means loss of recording of an opt-out in the worst case (may be > also add UMA to track that). Acknowledged. Seems like we might as well not drop them since the implementation is straight forward. If you push again, I'll remove it. https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... File components/previews/previews_experiments.cc (right): https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_experiments.cc:92: } On 2016/09/14 20:17:50, tbansal1 wrote: > nit: space between params. > also, add the comment // namespace params. Done. https://codereview.chromium.org/2335023002/diff/120001/components/previews/pr... components/previews/previews_experiments.cc:119: const int opt_out_threshold, On 2016/09/14 20:17:50, tbansal1 wrote: > On 2016/09/14 18:36:43, Ryan Sturm wrote: > > On 2016/09/14 17:17:30, tbansal1 wrote: > > > Why is this function needed? Can you not set the variation param using the > > test > > > framework provided already. > > > > > > https://cs.chromium.org/chromium/src/testing/gtest/include/gtest/gtest.h?l=44... > > > > This uses internal strings that I don't need to expose outside of this class. > > These are not really internal strings since they will be used outside of this > class (in the experiment configs on the server side). The goal typically is to > reduce the release binary size. So, I would still prefer if this is removed. > > https://cs.chromium.org/chromium/src/testing/gtest/include/gtest/gtest.h?l=44... Done. https://codereview.chromium.org/2335023002/diff/160001/components/previews/pr... File components/previews/previews_black_list.cc (right): https://codereview.chromium.org/2335023002/diff/160001/components/previews/pr... components/previews/previews_black_list.cc:65: if (!loaded_ || !previews::params::BlackListParamsAreValid()) { On 2016/09/14 20:17:50, tbansal1 wrote: > nit: no parenthesis required. Done. https://codereview.chromium.org/2335023002/diff/160001/components/previews/pr... File components/previews/previews_black_list.h (right): https://codereview.chromium.org/2335023002/diff/160001/components/previews/pr... components/previews/previews_black_list.h:63: // navigation to backing store. |opt_out| is whether the uesr opted out of On 2016/09/14 20:17:50, tbansal1 wrote: > typo in uesr. Done. https://codereview.chromium.org/2335023002/diff/160001/components/previews/pr... components/previews/previews_black_list.h:64: // the preview or navigated away from the page by another way. |type| is only On 2016/09/14 20:17:50, tbansal1 wrote: > What is "another way"? Done. https://codereview.chromium.org/2335023002/diff/160001/components/previews/pr... components/previews/previews_black_list.h:66: void AddPreviewNavigationSync(const GURL& host_name, On 2016/09/14 20:17:50, tbansal1 wrote: > For this function, you can simply say: "Synchronous version of > AddPreviewNavigation method". I do not think there is a need to duplicate > comments. Done. https://codereview.chromium.org/2335023002/diff/160001/components/previews/pr... File components/previews/previews_black_list_item.h (right): https://codereview.chromium.org/2335023002/diff/160001/components/previews/pr... components/previews/previews_black_list_item.h:39: struct TimeOptOut { On 2016/09/14 20:17:50, tbansal1 wrote: > Why is this public? Done. https://codereview.chromium.org/2335023002/diff/160001/components/previews/pr... components/previews/previews_black_list_item.h:61: // as a sorted vector. On 2016/09/14 20:17:50, tbansal1 wrote: > sorted by what? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
https://codereview.chromium.org/2335023002/diff/180001/components/previews/pr... File components/previews/previews_black_list.h (right): https://codereview.chromium.org/2335023002/diff/180001/components/previews/pr... components/previews/previews_black_list.h:47: typedef base::Closure QueueClosure; Why are these typedefs public? https://codereview.chromium.org/2335023002/diff/180001/components/previews/pr... File components/previews/previews_experiments.cc (right): https://codereview.chromium.org/2335023002/diff/180001/components/previews/pr... components/previews/previews_experiments.cc:84: return base::TimeDelta::FromSeconds(0); Can you rather use some meaningful default values? https://codereview.chromium.org/2335023002/diff/180001/components/previews/pr... components/previews/previews_experiments.cc:89: bool BlackListParamsAreValid() { This function should not be needed. I would rather use some meaningful default values, and then override them with field trial config (if needed). https://codereview.chromium.org/2335023002/diff/180001/components/previews/pr... File components/previews/previews_experiments_unittest.cc (right): https://codereview.chromium.org/2335023002/diff/180001/components/previews/pr... components/previews/previews_experiments_unittest.cc:27: std::map<std::string, std::string> params; #include map https://codereview.chromium.org/2335023002/diff/180001/components/previews/pr... File components/previews/previews_io_data.h (right): https://codereview.chromium.org/2335023002/diff/180001/components/previews/pr... components/previews/previews_io_data.h:38: PreviewsBlackList* black_list() { return black_list_.get(); } const method. https://codereview.chromium.org/2335023002/diff/180001/components/previews/pr... File components/previews/previews_opt_out_store.h (right): https://codereview.chromium.org/2335023002/diff/180001/components/previews/pr... components/previews/previews_opt_out_store.h:29: typedef std::map<std::string, std::unique_ptr<PreviewsBlackListItem>> #include map memory string https://codereview.chromium.org/2335023002/diff/180001/components/previews/pr... components/previews/previews_opt_out_store.h:38: // operation finishes will depend on implementation. It should be possible to "should be" sounds vague. s/should be/is/? https://codereview.chromium.org/2335023002/diff/180001/components/previews/pr... components/previews/previews_opt_out_store.h:50: const base::Time& now) = 0; Pass Time by value and just include the base/time/time.h instead of fwd declaration.
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
tbansal: PTAL https://codereview.chromium.org/2335023002/diff/180001/components/previews/pr... File components/previews/previews_black_list.h (right): https://codereview.chromium.org/2335023002/diff/180001/components/previews/pr... components/previews/previews_black_list.h:47: typedef base::Closure QueueClosure; On 2016/09/14 21:36:26, tbansal1 wrote: > Why are these typedefs public? Done. https://codereview.chromium.org/2335023002/diff/180001/components/previews/pr... File components/previews/previews_experiments.cc (right): https://codereview.chromium.org/2335023002/diff/180001/components/previews/pr... components/previews/previews_experiments.cc:84: return base::TimeDelta::FromSeconds(0); On 2016/09/14 21:36:26, tbansal1 wrote: > Can you rather use some meaningful default values? Done. https://codereview.chromium.org/2335023002/diff/180001/components/previews/pr... components/previews/previews_experiments.cc:89: bool BlackListParamsAreValid() { On 2016/09/14 21:36:26, tbansal1 wrote: > This function should not be needed. I would rather use some meaningful default > values, and then override them with field trial config (if needed). Done. https://codereview.chromium.org/2335023002/diff/180001/components/previews/pr... File components/previews/previews_experiments_unittest.cc (right): https://codereview.chromium.org/2335023002/diff/180001/components/previews/pr... components/previews/previews_experiments_unittest.cc:27: std::map<std::string, std::string> params; On 2016/09/14 21:36:26, tbansal1 wrote: > #include map I decided to remove EnableOfflinePreviewsForTesting() in a different CL https://codereview.chromium.org/2335023002/diff/180001/components/previews/pr... File components/previews/previews_io_data.h (right): https://codereview.chromium.org/2335023002/diff/180001/components/previews/pr... components/previews/previews_io_data.h:38: PreviewsBlackList* black_list() { return black_list_.get(); } On 2016/09/14 21:36:26, tbansal1 wrote: > const method. Done. https://codereview.chromium.org/2335023002/diff/180001/components/previews/pr... File components/previews/previews_opt_out_store.h (right): https://codereview.chromium.org/2335023002/diff/180001/components/previews/pr... components/previews/previews_opt_out_store.h:29: typedef std::map<std::string, std::unique_ptr<PreviewsBlackListItem>> On 2016/09/14 21:36:26, tbansal1 wrote: > #include map > memory > string Done. https://codereview.chromium.org/2335023002/diff/180001/components/previews/pr... components/previews/previews_opt_out_store.h:38: // operation finishes will depend on implementation. It should be possible to On 2016/09/14 21:36:26, tbansal1 wrote: > "should be" sounds vague. > s/should be/is/? Done. https://codereview.chromium.org/2335023002/diff/180001/components/previews/pr... components/previews/previews_opt_out_store.h:50: const base::Time& now) = 0; On 2016/09/14 21:36:26, tbansal1 wrote: > Pass Time by value and just include the base/time/time.h instead of fwd > declaration. 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.
https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... File components/previews/previews_black_list.cc (right): https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... components/previews/previews_black_list.cc:51: DCHECK(url.has_host()); DCHECK(loaded_); https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... components/previews/previews_black_list.cc:83: while (pending_callbacks_.size() > 0 && loaded_) { can loaded_ become false after this point? May be instead add DCHECK(loaded_) inside the while loop? https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... File components/previews/previews_black_list.h (right): https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... components/previews/previews_black_list.h:83: std::unique_ptr<BlackListItemMap> black_list_item_map_; Is there a limit on the size of the map? https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... File components/previews/previews_black_list_item.cc (right): https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... components/previews/previews_black_list_item.cc:29: most_recent_opt_out_time_ = entry_time; I think you need to check if opt_out is true before updating most_recent_opt_out_time_? https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... components/previews/previews_black_list_item.cc:37: while (iter != opt_out_history_.rend() && iter->entry_time > entry_time) { why not start from rend and go back? That should be faster, right? https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... components/previews/previews_black_list_item.cc:52: bool PreviewsBlackListItem::IsBlackListed(base::Time now) const { If the argument is always the current time, then why does it need to be passed as a variable? Can |now| be anything significantly different than the current time? I am asking because if |now| is significantly different then current time, then I think the checks below are not correct (and also meaningless). https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... File components/previews/previews_black_list_item.h (right): https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... components/previews/previews_black_list_item.h:48: struct TimeOptOut { May be call it OptOutRecord? https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... components/previews/previews_black_list_item.h:49: base::Time entry_time; // The time that the opt out state was determined. can these be const members? https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... components/previews/previews_black_list_item.h:49: base::Time entry_time; // The time that the opt out state was determined. Why is entry_time needed? strictly speaking, you do need to store Time with every opt-out entry. All that is needed is the most recent opt-out time, and a sorted list (sorted naturally by order of arrival). That will also remove the need for iterating through all entries when adding a new entry. https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... components/previews/previews_black_list_item.h:53: // The amount of entries to store to determine preview eligibility. s/amount/number/ https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... components/previews/previews_black_list_item.h:54: const size_t stored_history_length_; s/stored_history_length_/max_stored_history_length_/ to make it clear that it is not the current history length but the max possible? https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... components/previews/previews_black_list_item.h:58: const base::TimeDelta black_list_duration_; s/black_list_duration_/max_black_list_duration_/ https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... File components/previews/previews_experiments.cc (right): https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... components/previews/previews_experiments.cc:27: // The number of most recent previews navigations the black list looks at to s/The number of most recent/The maximum number of recent/ https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... components/previews/previews_experiments.cc:36: const char kBlackListDuration[] = "black_list_duration"; Specify the units of duration both in the var name and the field trial param name (i.e., change to "black_list_duration_seconds"). This makes it easier to read the experiment config without refering to the chromium code. Also, I would prefer if it is in days (instead of seconds). That would make it easier to write/read the config. I do not think we would ever make the black list duration shorter than a day. https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... components/previews/previews_experiments.cc:42: constexpr int kDefaultBlackListDuration = 30 * 24 * 60 * 60; may be change to kDefaultBlackListDurationDays = 30; and then use FromDays() below. https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... components/previews/previews_experiments.cc:56: if (it == experiment_params.end()) replace the last three lines by a ternary operator. https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... components/previews/previews_experiments.cc:69: return 4; Note that this returns "4" even if the Chrome is not part of field trial. Is that okay?
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...)
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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run
tbansal: PTAL https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... File components/previews/previews_black_list.cc (right): https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... components/previews/previews_black_list.cc:51: DCHECK(url.has_host()); On 2016/09/15 16:34:24, tbansal1 wrote: > DCHECK(loaded_); Acknowledged. https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... components/previews/previews_black_list.cc:83: while (pending_callbacks_.size() > 0 && loaded_) { On 2016/09/15 16:34:24, tbansal1 wrote: > can loaded_ become false after this point? > May be instead add DCHECK(loaded_) inside the while loop? Acknowledged. https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... File components/previews/previews_black_list.h (right): https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... components/previews/previews_black_list.h:83: std::unique_ptr<BlackListItemMap> black_list_item_map_; On 2016/09/15 16:34:25, tbansal1 wrote: > Is there a limit on the size of the map? Done. https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... File components/previews/previews_black_list_item.cc (right): https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... components/previews/previews_black_list_item.cc:29: most_recent_opt_out_time_ = entry_time; On 2016/09/15 16:34:25, tbansal1 wrote: > I think you need to check if opt_out is true before updating > most_recent_opt_out_time_? Done. https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... components/previews/previews_black_list_item.cc:37: while (iter != opt_out_history_.rend() && iter->entry_time > entry_time) { On 2016/09/15 16:34:25, tbansal1 wrote: > why not start from rend and go back? That should be faster, right? rend is the equivalent of begin, and rbeing is like end, reverse iterators are weird. https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... components/previews/previews_black_list_item.cc:52: bool PreviewsBlackListItem::IsBlackListed(base::Time now) const { On 2016/09/15 16:34:25, tbansal1 wrote: > If the argument is always the current time, then why does it need to be passed > as a variable? Can |now| be anything significantly different than the current > time? > > I am asking because if |now| is significantly different then current time, then > I think the checks below are not correct (and also meaningless). I want the same time to be passed to the store and the black list item, so passing it in from there seems better. The time is still determined in the black list as a whole, just not the black list item. https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... File components/previews/previews_black_list_item.h (right): https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... components/previews/previews_black_list_item.h:48: struct TimeOptOut { On 2016/09/15 16:34:25, tbansal1 wrote: > May be call it OptOutRecord? Done. https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... components/previews/previews_black_list_item.h:49: base::Time entry_time; // The time that the opt out state was determined. On 2016/09/15 16:34:25, tbansal1 wrote: > Why is entry_time needed? strictly speaking, you do need to store Time with > every opt-out entry. All that is needed is the most recent opt-out time, and a > sorted list (sorted naturally by order of arrival). That will also remove the > need for iterating through all entries when adding a new entry. Talked offline about this, it is needed for the database sorting entries. If a user changes their clock, we should remain consistent with that change. https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... components/previews/previews_black_list_item.h:49: base::Time entry_time; // The time that the opt out state was determined. On 2016/09/15 16:34:25, tbansal1 wrote: > can these be const members? Done. https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... components/previews/previews_black_list_item.h:53: // The amount of entries to store to determine preview eligibility. On 2016/09/15 16:34:25, tbansal1 wrote: > s/amount/number/ Done. https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... components/previews/previews_black_list_item.h:54: const size_t stored_history_length_; On 2016/09/15 16:34:25, tbansal1 wrote: > s/stored_history_length_/max_stored_history_length_/ to make it clear that it is > not the current history length but the max possible? Done. https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... components/previews/previews_black_list_item.h:58: const base::TimeDelta black_list_duration_; On 2016/09/15 16:34:25, tbansal1 wrote: > s/black_list_duration_/max_black_list_duration_/ Done. https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... File components/previews/previews_experiments.cc (right): https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... components/previews/previews_experiments.cc:27: // The number of most recent previews navigations the black list looks at to On 2016/09/15 16:34:25, tbansal1 wrote: > s/The number of most recent/The maximum number of recent/ Done. https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... components/previews/previews_experiments.cc:36: const char kBlackListDuration[] = "black_list_duration"; On 2016/09/15 16:34:25, tbansal1 wrote: > Specify the units of duration both in the var name and the field trial param > name (i.e., change to "black_list_duration_seconds"). This makes it easier to > read the experiment config without refering to the chromium code. > > Also, I would prefer if it is in days (instead of seconds). That would make it > easier to write/read the config. I do not think we would ever make the black > list duration shorter than a day. Done. https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... components/previews/previews_experiments.cc:42: constexpr int kDefaultBlackListDuration = 30 * 24 * 60 * 60; On 2016/09/15 16:34:25, tbansal1 wrote: > may be change to kDefaultBlackListDurationDays = 30; > and then use FromDays() below. Done. https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... components/previews/previews_experiments.cc:56: if (it == experiment_params.end()) On 2016/09/15 16:34:25, tbansal1 wrote: > replace the last three lines by a ternary operator. Done. https://codereview.chromium.org/2335023002/diff/220001/components/previews/pr... components/previews/previews_experiments.cc:69: return 4; On 2016/09/15 16:34:25, tbansal1 wrote: > Note that this returns "4" even if the Chrome is not part of field trial. Is > that okay? I believe so, the blacklist won't be used until previews are turned on, and when they are turned on, either they use the default or they set their own. Either way, the blacklist won't start storing until a preview is enabled.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... File components/previews/core/previews_black_list.cc (right): https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... components/previews/core/previews_black_list.cc:26: &PreviewsBlackList::LoadBlackListDone, weak_factory_.GetWeakPtr())); Since opt_out_store_ is owned by |this|, is it possible to pass a raw ptr here (and remove all weak ptr code). https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... components/previews/core/previews_black_list.cc:61: opt_out_store_->AddPreviewNavigation(opt_out, host_name, type, now); DCHECK_LE(black_list_item_map_->size(), params::MaxInMemoryHostsInBlackList()); https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... components/previews/core/previews_black_list.cc:76: DCHECK(!callback.is_null()); DCHECK(!loaded_); https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... components/previews/core/previews_black_list.cc:86: // Run all pending tasks. |loaded_| may change if ClearBlackListIs queued. s/ClearBlackListIs/ClearBlackList is/ https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... components/previews/core/previews_black_list.cc:106: } DCHECK_LE(black_list_item_map_->size(), params::MaxInMemoryHostsInBlackList()); https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... components/previews/core/previews_black_list.cc:116: void PreviewsBlackList::EvictOldestOptOut() { I think the eviction should happen on the disk also (much more preferable), or at the time of loading it must be ensured that at most |params::MaxInMemoryHostsInBlackList()| hosts are read (less preferable). Currently, the limit |params::MaxInMemoryHostsInBlackList()| is a soft limit, and it is possible for program to read more than the threshold number of entries in the memory. https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... File components/previews/core/previews_black_list.h (right): https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... components/previews/core/previews_black_list.h:25: class Time; nit: rm forward declare Time. https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... components/previews/core/previews_black_list.h:46: explicit PreviewsBlackList(std::unique_ptr<PreviewsOptOutStore> opt_out_store, nit: explicit not needed. https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... File components/previews/core/previews_black_list_item.cc (right): https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... components/previews/core/previews_black_list_item.cc:50: } Add DCHECK_LE(opt_out_history_.size(), max_stored_history_length_); to ensure that the condition holds even after the entry is added to the list. https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... File components/previews/core/previews_black_list_item.h (right): https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... components/previews/core/previews_black_list_item.h:45: base::Optional<base::Time> most_recent_opt_out_time() { nit: const method. https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... components/previews/core/previews_black_list_item.h:53: public: structs can't have private members. One typical option is to keep everything public, and override copy and assignment constructors. If some fields need to be private, then it is better to just use class. https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... File components/previews/core/previews_experiments.h (right): https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... components/previews/core/previews_experiments.h:5: #ifndef COMPONENTS_PREVIEWS_PREVIEWS_EXPERIMENTS_H_ s/COMPONENTS_PREVIEWS_PREVIEWS_EXPERIMENTS_H_/??/ _CORE_ missing. https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... File components/previews/core/previews_io_data.h (right): https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... components/previews/core/previews_io_data.h:6: #define COMPONENTS_PREVIEWS_PREVIEWS_IO_DATA_H_ _CORE_ missing. check other header files too. https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... components/previews/core/previews_io_data.h:21: enum class PreviewsType; Why is this defined here? https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... components/previews/core/previews_io_data.h:50: std::unique_ptr<PreviewsBlackList> black_list_; either s/black_list_/previews_black_list_/ or s/PreviewsBlackList/BlackList/ https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... File components/previews/core/previews_opt_out_store.h (right): https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... components/previews/core/previews_opt_out_store.h:29: typedef std::map<std::string, std::unique_ptr<PreviewsBlackListItem>> Why not use an unordered_map? If it needs to be ordered, can you add a comment on what it is ordered by, and why. https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... components/previews/core/previews_opt_out_store.h:52: // Asynchronously loads a map of host names to PreviewsBlackListItem for that add that map should be sorted, and what is the key for sorting. So that implementors of this interface know what is expected.
The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run
tbansal: PTAL https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... File components/previews/core/previews_black_list.cc (right): https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... components/previews/core/previews_black_list.cc:26: &PreviewsBlackList::LoadBlackListDone, weak_factory_.GetWeakPtr())); On 2016/09/19 21:26:07, tbansal1 wrote: > Since opt_out_store_ is owned by |this|, is it possible to pass a raw ptr here > (and remove all weak ptr code). I don't think we should limit the OptOutStore implementation (currently coded the OptOutStore could be deleted before this callback is called). https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... components/previews/core/previews_black_list.cc:61: opt_out_store_->AddPreviewNavigation(opt_out, host_name, type, now); On 2016/09/19 21:26:08, tbansal1 wrote: > DCHECK_LE(black_list_item_map_->size(), params::MaxInMemoryHostsInBlackList()); Done. https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... components/previews/core/previews_black_list.cc:76: DCHECK(!callback.is_null()); On 2016/09/19 21:26:07, tbansal1 wrote: > DCHECK(!loaded_); Done. https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... components/previews/core/previews_black_list.cc:86: // Run all pending tasks. |loaded_| may change if ClearBlackListIs queued. On 2016/09/19 21:26:07, tbansal1 wrote: > s/ClearBlackListIs/ClearBlackList is/ Done. https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... components/previews/core/previews_black_list.cc:106: } On 2016/09/19 21:26:08, tbansal1 wrote: > DCHECK_LE(black_list_item_map_->size(), params::MaxInMemoryHostsInBlackList()); Done. https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... components/previews/core/previews_black_list.cc:116: void PreviewsBlackList::EvictOldestOptOut() { On 2016/09/19 21:26:07, tbansal1 wrote: > I think the eviction should happen on the disk also (much more preferable), or > at the time of loading it must be ensured that at most > |params::MaxInMemoryHostsInBlackList()| hosts are read (less preferable). > > Currently, the limit |params::MaxInMemoryHostsInBlackList()| is a soft limit, > and it is possible for program to read more than the threshold number of entries > in the memory. I'm planning to limit the SQL implementation to the same number of hosts. https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... File components/previews/core/previews_black_list.h (right): https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... components/previews/core/previews_black_list.h:25: class Time; On 2016/09/19 21:26:08, tbansal1 wrote: > nit: rm forward declare Time. Done. https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... components/previews/core/previews_black_list.h:46: explicit PreviewsBlackList(std::unique_ptr<PreviewsOptOutStore> opt_out_store, On 2016/09/19 21:26:08, tbansal1 wrote: > nit: explicit not needed. Done. https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... File components/previews/core/previews_black_list_item.cc (right): https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... components/previews/core/previews_black_list_item.cc:50: } On 2016/09/19 21:26:08, tbansal1 wrote: > Add DCHECK_LE(opt_out_history_.size(), max_stored_history_length_); > to ensure that the condition holds even after the entry is added to the list. Done. https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... File components/previews/core/previews_black_list_item.h (right): https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... components/previews/core/previews_black_list_item.h:45: base::Optional<base::Time> most_recent_opt_out_time() { On 2016/09/19 21:26:08, tbansal1 wrote: > nit: const method. Done. https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... components/previews/core/previews_black_list_item.h:53: public: On 2016/09/19 21:26:08, tbansal1 wrote: > structs can't have private members. > One typical option is to keep everything public, and override copy and > assignment constructors. > If some fields need to be private, then it is better to just use class. Done. https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... File components/previews/core/previews_experiments.h (right): https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... components/previews/core/previews_experiments.h:5: #ifndef COMPONENTS_PREVIEWS_PREVIEWS_EXPERIMENTS_H_ On 2016/09/19 21:26:08, tbansal1 wrote: > s/COMPONENTS_PREVIEWS_PREVIEWS_EXPERIMENTS_H_/??/ > _CORE_ missing. Good catch. I've fixed others. https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... File components/previews/core/previews_io_data.h (right): https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... components/previews/core/previews_io_data.h:6: #define COMPONENTS_PREVIEWS_PREVIEWS_IO_DATA_H_ On 2016/09/19 21:26:08, tbansal1 wrote: > _CORE_ missing. check other header files too. Done. https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... components/previews/core/previews_io_data.h:21: enum class PreviewsType; On 2016/09/19 21:26:08, tbansal1 wrote: > Why is this defined here? Done. Left over from UI to IO messages. https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... components/previews/core/previews_io_data.h:50: std::unique_ptr<PreviewsBlackList> black_list_; On 2016/09/19 21:26:08, tbansal1 wrote: > either s/black_list_/previews_black_list_/ > or s/PreviewsBlackList/BlackList/ Done. https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... File components/previews/core/previews_opt_out_store.h (right): https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... components/previews/core/previews_opt_out_store.h:29: typedef std::map<std::string, std::unique_ptr<PreviewsBlackListItem>> On 2016/09/19 21:26:08, tbansal1 wrote: > Why not use an unordered_map? > If it needs to be ordered, can you add a comment on what it is ordered by, and > why. Done. https://codereview.chromium.org/2335023002/diff/300001/components/previews/co... components/previews/core/previews_opt_out_store.h:52: // Asynchronously loads a map of host names to PreviewsBlackListItem for that On 2016/09/19 21:26:08, tbansal1 wrote: > add that map should be sorted, and what is the key for sorting. > So that implementors of this interface know what is expected. Acknowledged.
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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
almost there. https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... File components/previews/core/previews_black_list.cc (right): https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_black_list.cc:66: bool PreviewsBlackList::IsLoadedAndAllowed(const GURL& url, PreviewsType type) { I wish this was a const method so callers can call it from their const methods. https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_black_list.cc:103: if (iter != black_list_item_map_->end()) { nit: braces not needed. https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_black_list.cc:108: if (black_list_item_map_->size() >= params::MaxInMemoryHostsInBlackList()) { nit: braces not needed. https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_black_list.cc:111: DCHECK_LE(black_list_item_map_->size(), s/DCHECK_LE/DCHECK_LT/ https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_black_list.cc:113: // Create the item if it doesn't exist yet. s/Create the item if it doesn't exist yet./Create the item since it doesn't exist./ https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_black_list.cc:130: if (!iter->second->most_recent_opt_out_time()) { #include "base/optional.h" https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... File components/previews/core/previews_black_list.h (right): https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_black_list.h:43: // map will be immeadiately loaded to empty. If |opt_out_store| is non-null, typo in immeadiately. https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... File components/previews/core/previews_black_list_item.cc (right): https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_black_list_item.cc:41: while (iter != opt_out_history_.rend() && iter->entry_time > entry_time) { nits: braces not needed. https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... File components/previews/core/previews_black_list_item.h (right): https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_black_list_item.h:37: // Adds a new navigation and returns the base::Time that the event was nit: method comment is probably incorrect. https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_black_list_item.h:50: // A previews navigation to this host can be represented by time and whether s/can be/is/ https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_black_list_item.h:70: std::list<OptOutRecord> opt_out_history_; s/opt_out_history_/opt_out_records_/ to match the struct name. https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... File components/previews/core/previews_black_list_item_unittest.cc (right): https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_black_list_item_unittest.cc:20: TEST_F(PreviewsBlackListItemTest, BlackListState) { Can you also check if most_recent_opt_out_time() returns correct value since that is exposed as a public method. https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_black_list_item_unittest.cc:23: const base::TimeDelta duration = base::TimeDelta::FromSeconds(30); s/duration/max_blacklist_duration/ since duration is very vague. https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_black_list_item_unittest.cc:28: std::unique_ptr<PreviewsBlackListItem> black_list_item( Can this be declared without unique_ptr? PreviewsBlackListItem black_list_item(history, threshold, duration); https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... File components/previews/core/previews_black_list_unittest.cc (right): https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_black_list_unittest.cc:51: base::Time now) override {} nit: missing line break https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_black_list_unittest.cc:68: // Hopefully, this test runs within a calendar year. The test provides the clock to |black_list|. So, even if test takes more than 365 days to run, it should probably not be a problem since the test clock is never advanced. https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_black_list_unittest.cc:171: TEST_F(PreviewsBlackListTest, QueueBehavior) { can you add a comment describing what the test is verifying? https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_black_list_unittest.cc:185: for (int i = 0; i < 2; i++) { s/i++/++i/ https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_black_list_unittest.cc:193: black_list->AddPreviewNavigation(url, i < 1, PreviewsType::OFFLINE); this is slightly confusing. It might be easier if you create a vector of bools (2 entries), and iterate over it. The, it will be clearer what i=0 maps to, and what i=1 maps to. https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_black_list_unittest.cc:214: const size_t hosts = 2; s/hosts/max_hosts_in_blacklist/ It would be more readable to match the names in the field trial for all variables here. https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... File components/previews/core/previews_experiments.cc (right): https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_experiments.cc:114: std::string param_value = ParamValue(kOfflinePagesSlowNetwork); nit: combine the two into a single line. https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... File components/previews/core/previews_experiments.h (right): https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_experiments.h:15: // determine if a domain is blacklisted. s/domain/host/ to be consistent with other code. here and on line 25 below https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_experiments.h:27: } nit: line break before } and add // namespace params https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... File components/previews/core/previews_io_data.h (right): https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_io_data.h:15: #include "base/time/time.h" #include "base/time/time.h" not needed https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... File components/previews/core/previews_opt_out_store.h (right): https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_opt_out_store.h:43: virtual ~PreviewsOptOutStore(); You can remove the constructor. Inline the destructor. and get rid of .cc file.
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/2335023002/diff/320001/components/previews/co... File components/previews/core/previews_black_list.cc (right): https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_black_list.cc:66: bool PreviewsBlackList::IsLoadedAndAllowed(const GURL& url, PreviewsType type) { On 2016/09/20 17:48:50, tbansal1 wrote: > I wish this was a const method so callers can call it from their const methods. I can make it const, i'll add another method for CreateBlackListItem in addition to GetBlackListItem https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_black_list.cc:103: if (iter != black_list_item_map_->end()) { On 2016/09/20 17:48:50, tbansal1 wrote: > nit: braces not needed. Done. https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_black_list.cc:108: if (black_list_item_map_->size() >= params::MaxInMemoryHostsInBlackList()) { On 2016/09/20 17:48:50, tbansal1 wrote: > nit: braces not needed. Done. https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_black_list.cc:111: DCHECK_LE(black_list_item_map_->size(), On 2016/09/20 17:48:50, tbansal1 wrote: > s/DCHECK_LE/DCHECK_LT/ Done. https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_black_list.cc:113: // Create the item if it doesn't exist yet. On 2016/09/20 17:48:50, tbansal1 wrote: > s/Create the item if it doesn't exist yet./Create the item since it doesn't > exist./ I removed the comment based on refactor. https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_black_list.cc:130: if (!iter->second->most_recent_opt_out_time()) { On 2016/09/20 17:48:50, tbansal1 wrote: > #include "base/optional.h" Done. https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... File components/previews/core/previews_black_list.h (right): https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_black_list.h:43: // map will be immeadiately loaded to empty. If |opt_out_store| is non-null, On 2016/09/20 17:48:50, tbansal1 wrote: > typo in immeadiately. Done. https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... File components/previews/core/previews_black_list_item.cc (right): https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_black_list_item.cc:41: while (iter != opt_out_history_.rend() && iter->entry_time > entry_time) { On 2016/09/20 17:48:50, tbansal1 wrote: > nits: braces not needed. Done. https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... File components/previews/core/previews_black_list_item.h (right): https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_black_list_item.h:37: // Adds a new navigation and returns the base::Time that the event was On 2016/09/20 17:48:51, tbansal1 wrote: > nit: method comment is probably incorrect. Done. https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_black_list_item.h:50: // A previews navigation to this host can be represented by time and whether On 2016/09/20 17:48:51, tbansal1 wrote: > s/can be/is/ Done. https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_black_list_item.h:70: std::list<OptOutRecord> opt_out_history_; On 2016/09/20 17:48:51, tbansal1 wrote: > s/opt_out_history_/opt_out_records_/ > to match the struct name. Done. https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... File components/previews/core/previews_black_list_item_unittest.cc (right): https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_black_list_item_unittest.cc:20: TEST_F(PreviewsBlackListItemTest, BlackListState) { On 2016/09/20 17:48:51, tbansal1 wrote: > Can you also check if most_recent_opt_out_time() returns correct value since > that is exposed as a public method. Done. https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_black_list_item_unittest.cc:23: const base::TimeDelta duration = base::TimeDelta::FromSeconds(30); On 2016/09/20 17:48:51, tbansal1 wrote: > s/duration/max_blacklist_duration/ > since duration is very vague. Done. https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_black_list_item_unittest.cc:28: std::unique_ptr<PreviewsBlackListItem> black_list_item( On 2016/09/20 17:48:51, tbansal1 wrote: > Can this be declared without unique_ptr? > PreviewsBlackListItem black_list_item(history, threshold, duration); Done. https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... File components/previews/core/previews_black_list_unittest.cc (right): https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_black_list_unittest.cc:51: base::Time now) override {} On 2016/09/20 17:48:51, tbansal1 wrote: > nit: missing line break Done. https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_black_list_unittest.cc:68: // Hopefully, this test runs within a calendar year. On 2016/09/20 17:48:51, tbansal1 wrote: > The test provides the clock to |black_list|. So, even if test takes more than > 365 days to run, it should probably not be a problem since the test clock is > never advanced. Done. https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_black_list_unittest.cc:171: TEST_F(PreviewsBlackListTest, QueueBehavior) { On 2016/09/20 17:48:51, tbansal1 wrote: > can you add a comment describing what the test is verifying? Done. https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_black_list_unittest.cc:185: for (int i = 0; i < 2; i++) { On 2016/09/20 17:48:51, tbansal1 wrote: > s/i++/++i/ Done. https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_black_list_unittest.cc:193: black_list->AddPreviewNavigation(url, i < 1, PreviewsType::OFFLINE); On 2016/09/20 17:48:51, tbansal1 wrote: > this is slightly confusing. It might be easier if you create a vector of bools > (2 entries), and iterate over it. The, it will be clearer what i=0 maps to, and > what i=1 maps to. Done. https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_black_list_unittest.cc:214: const size_t hosts = 2; On 2016/09/20 17:48:51, tbansal1 wrote: > s/hosts/max_hosts_in_blacklist/ > It would be more readable to match the names in the field trial for all > variables here. Done. https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... File components/previews/core/previews_experiments.cc (right): https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_experiments.cc:114: std::string param_value = ParamValue(kOfflinePagesSlowNetwork); On 2016/09/20 17:48:51, tbansal1 wrote: > nit: combine the two into a single line. Done. https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... File components/previews/core/previews_experiments.h (right): https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_experiments.h:15: // determine if a domain is blacklisted. On 2016/09/20 17:48:51, tbansal1 wrote: > s/domain/host/ to be consistent with other code. > here and on line 25 below Done. https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_experiments.h:27: } On 2016/09/20 17:48:51, tbansal1 wrote: > nit: line break before } > and add // namespace params Done. https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... File components/previews/core/previews_io_data.h (right): https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_io_data.h:15: #include "base/time/time.h" On 2016/09/20 17:48:52, tbansal1 wrote: > #include "base/time/time.h" not needed Done. https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... File components/previews/core/previews_opt_out_store.h (right): https://codereview.chromium.org/2335023002/diff/320001/components/previews/co... components/previews/core/previews_opt_out_store.h:43: virtual ~PreviewsOptOutStore(); On 2016/09/20 17:48:52, tbansal1 wrote: > You can remove the constructor. Inline the destructor. and get rid of .cc file. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
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.
lgtm % nits. https://codereview.chromium.org/2335023002/diff/330001/components/previews/co... File components/previews/core/previews_black_list.cc (right): https://codereview.chromium.org/2335023002/diff/330001/components/previews/co... components/previews/core/previews_black_list.cc:75: std::string host_name = url.host(); nit: you can move this statement after the if-conditional. Also, you can just combine it with the statement PreviewsBlackListItem* black_list_item = GetBlackListItem(host_name); https://codereview.chromium.org/2335023002/diff/330001/components/previews/co... components/previews/core/previews_black_list.cc:117: DCHECK(black_list_item_map_->find(host_name) == black_list_item_map_->end()); s/DCHECK/DHECK_EQ/ or DCHECK(!GetBlackListItem(host_name)); https://codereview.chromium.org/2335023002/diff/330001/components/previews/co... File components/previews/core/previews_black_list.h (right): https://codereview.chromium.org/2335023002/diff/330001/components/previews/co... components/previews/core/previews_black_list.h:63: typedef base::Closure QueueClosure; this typedef is confusing since it really does not have anything to do with Queue by itself (except that it is stored in a queue at some point). May be just remove the typedef? https://codereview.chromium.org/2335023002/diff/330001/components/previews/co... components/previews/core/previews_black_list.h:89: // Evicts entries from the in-memory black list based on recency of a hosts s/entries/one entry/ s/hosts/host's/ https://codereview.chromium.org/2335023002/diff/330001/components/previews/co... File components/previews/core/previews_black_list_item.h (right): https://codereview.chromium.org/2335023002/diff/330001/components/previews/co... components/previews/core/previews_black_list_item.h:21: enum class PreviewsType; Why is this declared here? https://codereview.chromium.org/2335023002/diff/330001/components/previews/co... components/previews/core/previews_black_list_item.h:68: // as a list sorted by entry_time (earliest to latest). merge with line above. https://codereview.chromium.org/2335023002/diff/330001/components/previews/co... File components/previews/core/previews_experiments.cc (right): https://codereview.chromium.org/2335023002/diff/330001/components/previews/co... components/previews/core/previews_experiments.cc:28: // determine if a domain is blacklisted. s/domain/host/ and everywhere else. https://codereview.chromium.org/2335023002/diff/330001/components/previews/co... File components/previews/core/previews_opt_out_store.h (right): https://codereview.chromium.org/2335023002/diff/330001/components/previews/co... components/previews/core/previews_opt_out_store.h:44: // Adds a new navigation to the store. |opt_out| is whether the uesr opted out typo in uesr. https://codereview.chromium.org/2335023002/diff/330001/components/previews/co... components/previews/core/previews_opt_out_store.h:45: // of the preview or navigated away from the page by another way. "navigated away from the page by another way" is very vague. Does it also include user clicking on a link on the page? https://codereview.chromium.org/2335023002/diff/330001/components/previews/co... components/previews/core/previews_opt_out_store.h:52: // host from the store. Say something about the arguments of the method. e.g,: "and runs |callback| once loading is finished."
The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run
https://codereview.chromium.org/2335023002/diff/330001/components/previews/co... File components/previews/core/previews_black_list.cc (right): https://codereview.chromium.org/2335023002/diff/330001/components/previews/co... components/previews/core/previews_black_list.cc:75: std::string host_name = url.host(); On 2016/09/22 21:12:27, tbansal1 wrote: > nit: you can move this statement after the if-conditional. > Also, you can just combine it with the statement > PreviewsBlackListItem* black_list_item = GetBlackListItem(host_name); Done. https://codereview.chromium.org/2335023002/diff/330001/components/previews/co... components/previews/core/previews_black_list.cc:117: DCHECK(black_list_item_map_->find(host_name) == black_list_item_map_->end()); On 2016/09/22 21:12:27, tbansal1 wrote: > s/DCHECK/DHECK_EQ/ > or > DCHECK(!GetBlackListItem(host_name)); Done. https://codereview.chromium.org/2335023002/diff/330001/components/previews/co... File components/previews/core/previews_black_list.h (right): https://codereview.chromium.org/2335023002/diff/330001/components/previews/co... components/previews/core/previews_black_list.h:63: typedef base::Closure QueueClosure; On 2016/09/22 21:12:27, tbansal1 wrote: > this typedef is confusing since it really does not have anything to do with > Queue by itself (except that it is stored in a queue at some point). > May be just remove the typedef? Done. https://codereview.chromium.org/2335023002/diff/330001/components/previews/co... components/previews/core/previews_black_list.h:89: // Evicts entries from the in-memory black list based on recency of a hosts On 2016/09/22 21:12:27, tbansal1 wrote: > s/entries/one entry/ > s/hosts/host's/ Done. https://codereview.chromium.org/2335023002/diff/330001/components/previews/co... File components/previews/core/previews_black_list_item.h (right): https://codereview.chromium.org/2335023002/diff/330001/components/previews/co... components/previews/core/previews_black_list_item.h:21: enum class PreviewsType; On 2016/09/22 21:12:28, tbansal1 wrote: > Why is this declared here? Done. https://codereview.chromium.org/2335023002/diff/330001/components/previews/co... components/previews/core/previews_black_list_item.h:68: // as a list sorted by entry_time (earliest to latest). On 2016/09/22 21:12:28, tbansal1 wrote: > merge with line above. Done. https://codereview.chromium.org/2335023002/diff/330001/components/previews/co... File components/previews/core/previews_experiments.cc (right): https://codereview.chromium.org/2335023002/diff/330001/components/previews/co... components/previews/core/previews_experiments.cc:28: // determine if a domain is blacklisted. On 2016/09/22 21:12:28, tbansal1 wrote: > s/domain/host/ > and everywhere else. Done. https://codereview.chromium.org/2335023002/diff/330001/components/previews/co... File components/previews/core/previews_opt_out_store.h (right): https://codereview.chromium.org/2335023002/diff/330001/components/previews/co... components/previews/core/previews_opt_out_store.h:44: // Adds a new navigation to the store. |opt_out| is whether the uesr opted out On 2016/09/22 21:12:28, tbansal1 wrote: > typo in uesr. Done. https://codereview.chromium.org/2335023002/diff/330001/components/previews/co... components/previews/core/previews_opt_out_store.h:45: // of the preview or navigated away from the page by another way. On 2016/09/22 21:12:28, tbansal1 wrote: > "navigated away from the page by another way" is very vague. Does it also > include user clicking on a link on the page? Done. https://codereview.chromium.org/2335023002/diff/330001/components/previews/co... components/previews/core/previews_opt_out_store.h:52: // host from the store. On 2016/09/22 21:12:28, tbansal1 wrote: > Say something about the arguments of the method. e.g,: "and runs |callback| once > loading is finished." Done.
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: + mmenke@chromium.org
mmenke: PTAL @ chrome/browser/profiles/profile_impl_io_data.cc As a note, In a future CL I will change PreviewsService::set_previews_ui_service to PreviewsService::Initialize, which will create the PreviewsOptOutStoreImpl instance and pass in profile_path to keep as much out of profile_impl_io_data as possible, but for now I am just passing in nullptr. Thanks!
On 2016/09/23 17:27:42, Ryan Sturm wrote: > mmenke: PTAL @ chrome/browser/profiles/profile_impl_io_data.cc > > As a note, In a future CL I will change PreviewsService::set_previews_ui_service > to PreviewsService::Initialize, which will create the PreviewsOptOutStoreImpl > instance and pass in profile_path to keep as much out of profile_impl_io_data as > possible, but for now I am just passing in nullptr. > > Thanks! LGTM, rubberstamp
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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 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/2335023002/#ps370001 (title: "tbansal comments")
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.
Description was changed from ========== Adding a previews IO-thread blacklist This introduces an in memory blacklist for previews. This also introduces an interface for retrieiving prior sessions information from a store. The blacklist is keyed on fully qualified domain name and will blacklist domains based on user history of opt-outs on those domains. This does not yet implement adding preview navigations to the blacklist or using the blacklist to determine site navigation eligibility. Nor does this CL implement the backing store. BUG=639087 ========== to ========== Adding a previews IO-thread blacklist This introduces an in memory blacklist for previews. This also introduces an interface for retrieiving prior sessions information from a store. The blacklist is keyed on fully qualified domain name and will blacklist domains based on user history of opt-outs on those domains. This does not yet implement adding preview navigations to the blacklist or using the blacklist to determine site navigation eligibility. Nor does this CL implement the backing store. BUG=639087 ==========
Message was sent while issue was closed.
Committed patchset #19 (id:370001)
Message was sent while issue was closed.
Description was changed from ========== Adding a previews IO-thread blacklist This introduces an in memory blacklist for previews. This also introduces an interface for retrieiving prior sessions information from a store. The blacklist is keyed on fully qualified domain name and will blacklist domains based on user history of opt-outs on those domains. This does not yet implement adding preview navigations to the blacklist or using the blacklist to determine site navigation eligibility. Nor does this CL implement the backing store. BUG=639087 ========== to ========== Adding a previews IO-thread blacklist This introduces an in memory blacklist for previews. This also introduces an interface for retrieiving prior sessions information from a store. The blacklist is keyed on fully qualified domain name and will blacklist domains based on user history of opt-outs on those domains. This does not yet implement adding preview navigations to the blacklist or using the blacklist to determine site navigation eligibility. Nor does this CL implement the backing store. BUG=639087 Committed: https://crrev.com/d0b4726daad2e6f410df05299bc5d37c23aa339c Cr-Commit-Position: refs/heads/master@{#420726} ==========
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/d0b4726daad2e6f410df05299bc5d37c23aa339c Cr-Commit-Position: refs/heads/master@{#420726} |
