Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2439203002:
Adding a short blacklist period after every previews opt out (Closed)
DescriptionAdding a short blacklist period after every previews opt out
Within a session, if a user opts out of a preview navigation, the user
won't be shown any previews for a short duration in that session.
This does not extend outside the session because the general rationale
behind this is a user temporarily wanting to consume many non-previews
navigations consecutively.
BUG=656802
Committed: https://crrev.com/960e2c0a2455a4be4e71b9b0923115785bf6cd1e
Cr-Commit-Position: refs/heads/master@{#427480}
Patch Set 1 #
Total comments: 14
Patch Set 2 : tbansal comments #
Total comments: 6
Patch Set 3 : rebase #Patch Set 4 : tbansal commetns #
Dependent Patchsets: Messages
Total messages: 29 (20 generated)
The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ryansturm@chromium.org changed reviewers: + tbansal@chromium.org
tbansal: PTAL
https://codereview.chromium.org/2439203002/diff/1/components/previews/core/pr... File components/previews/core/previews_black_list.h (right): https://codereview.chromium.org/2439203002/diff/1/components/previews/core/pr... components/previews/core/previews_black_list.h:107: base::Optional<base::Time> last_opt_out_time_; #include base/optional.h https://codereview.chromium.org/2439203002/diff/1/components/previews/core/pr... File components/previews/core/previews_black_list_unittest.cc (right): https://codereview.chromium.org/2439203002/diff/1/components/previews/core/pr... components/previews/core/previews_black_list_unittest.cc:87: base::IntToString(single_opt_out_duration); Can you add a comment that it is set to 0 so that single opt-out does not kick in. https://codereview.chromium.org/2439203002/diff/1/components/previews/core/pr... components/previews/core/previews_black_list_unittest.cc:331: // Test that the black list only stores n hosts, and it stores the correct n Comment looks stale. https://codereview.chromium.org/2439203002/diff/1/components/previews/core/pr... components/previews/core/previews_black_list_unittest.cc:371: test_clock->Advance(base::TimeDelta::FromSeconds(1)); s/1/single_opt_out_duration - 1/ https://codereview.chromium.org/2439203002/diff/1/components/previews/core/pr... components/previews/core/previews_black_list_unittest.cc:376: test_clock->Advance(base::TimeDelta::FromSeconds(10)); s/10/single_opt_out_duration + 1/ to make it clearer that 10 is related to the value of |single_opt_out_duration| https://codereview.chromium.org/2439203002/diff/1/components/previews/core/pr... File components/previews/core/previews_experiments.cc (right): https://codereview.chromium.org/2439203002/diff/1/components/previews/core/pr... components/previews/core/previews_experiments.cc:51: // In seconds. Previews are not show for 5 minutes after an opt out. s/show/shown/ https://codereview.chromium.org/2439203002/diff/1/components/previews/core/pr... File components/previews/core/previews_experiments.h (right): https://codereview.chromium.org/2439203002/diff/1/components/previews/core/pr... components/previews/core/previews_experiments.h:29: base::TimeDelta SingleOptOutBlackOutDuration(); Just call it SingleOptOutDuration?
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
Patchset #2 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
tbansal: PTAL https://codereview.chromium.org/2439203002/diff/1/components/previews/core/pr... File components/previews/core/previews_black_list.h (right): https://codereview.chromium.org/2439203002/diff/1/components/previews/core/pr... components/previews/core/previews_black_list.h:107: base::Optional<base::Time> last_opt_out_time_; On 2016/10/21 23:15:33, tbansal1 wrote: > #include base/optional.h Done. https://codereview.chromium.org/2439203002/diff/1/components/previews/core/pr... File components/previews/core/previews_black_list_unittest.cc (right): https://codereview.chromium.org/2439203002/diff/1/components/previews/core/pr... components/previews/core/previews_black_list_unittest.cc:87: base::IntToString(single_opt_out_duration); On 2016/10/21 23:15:33, tbansal1 wrote: > Can you add a comment that it is set to 0 so that single opt-out does not kick > in. Done. https://codereview.chromium.org/2439203002/diff/1/components/previews/core/pr... components/previews/core/previews_black_list_unittest.cc:331: // Test that the black list only stores n hosts, and it stores the correct n On 2016/10/21 23:15:33, tbansal1 wrote: > Comment looks stale. Done. https://codereview.chromium.org/2439203002/diff/1/components/previews/core/pr... components/previews/core/previews_black_list_unittest.cc:371: test_clock->Advance(base::TimeDelta::FromSeconds(1)); On 2016/10/21 23:15:33, tbansal1 wrote: > s/1/single_opt_out_duration - 1/ Done. https://codereview.chromium.org/2439203002/diff/1/components/previews/core/pr... components/previews/core/previews_black_list_unittest.cc:376: test_clock->Advance(base::TimeDelta::FromSeconds(10)); On 2016/10/21 23:15:33, tbansal1 wrote: > s/10/single_opt_out_duration + 1/ > to make it clearer that 10 is related to the value of |single_opt_out_duration| Done. https://codereview.chromium.org/2439203002/diff/1/components/previews/core/pr... File components/previews/core/previews_experiments.cc (right): https://codereview.chromium.org/2439203002/diff/1/components/previews/core/pr... components/previews/core/previews_experiments.cc:51: // In seconds. Previews are not show for 5 minutes after an opt out. On 2016/10/21 23:15:34, tbansal1 wrote: > s/show/shown/ Done. https://codereview.chromium.org/2439203002/diff/1/components/previews/core/pr... File components/previews/core/previews_experiments.h (right): https://codereview.chromium.org/2439203002/diff/1/components/previews/core/pr... components/previews/core/previews_experiments.h:29: base::TimeDelta SingleOptOutBlackOutDuration(); On 2016/10/21 23:15:34, tbansal1 wrote: > Just call it SingleOptOutDuration? Done.
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/2439203002/diff/40001/components/previews/cor... File components/previews/core/previews_black_list.cc (right): https://codereview.chromium.org/2439203002/diff/40001/components/previews/cor... components/previews/core/previews_black_list.cc:11: #include "base/time/time.h" Remove this include. https://codereview.chromium.org/2439203002/diff/40001/components/previews/cor... components/previews/core/previews_black_list.cc:97: bool PreviewsBlackList::IsLoadedAndAllowed(const GURL& url, At some point in future, it might be useful for this function to return an enum describing why the preview can;t be shown. Consumers can than use it to record UMA (which can help them in understanding how to tune different field trial params). https://codereview.chromium.org/2439203002/diff/40001/components/previews/cor... File components/previews/core/previews_black_list.h (right): https://codereview.chromium.org/2439203002/diff/40001/components/previews/cor... components/previews/core/previews_black_list.h:25: class Clock; #include "base/time/time.h" and remove this. (otherwise, you will need to forward declare base::Time).
The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run
https://codereview.chromium.org/2439203002/diff/40001/components/previews/cor... File components/previews/core/previews_black_list.cc (right): https://codereview.chromium.org/2439203002/diff/40001/components/previews/cor... components/previews/core/previews_black_list.cc:11: #include "base/time/time.h" On 2016/10/25 00:47:41, tbansal1 wrote: > Remove this include. Done. https://codereview.chromium.org/2439203002/diff/40001/components/previews/cor... components/previews/core/previews_black_list.cc:97: bool PreviewsBlackList::IsLoadedAndAllowed(const GURL& url, On 2016/10/25 00:47:41, tbansal1 wrote: > At some point in future, it might be useful for this function to return an enum > describing why the preview can;t be shown. Consumers can than use it to record > UMA (which can help them in understanding how to tune different field trial > params). Agreed. Seperate CL when someone wants it. We'd need to "correctly" order them here and in the method that calls this. https://codereview.chromium.org/2439203002/diff/40001/components/previews/cor... File components/previews/core/previews_black_list.h (right): https://codereview.chromium.org/2439203002/diff/40001/components/previews/cor... components/previews/core/previews_black_list.h:25: class Clock; On 2016/10/25 00:47:41, tbansal1 wrote: > #include "base/time/time.h" > and remove this. > > (otherwise, you will need to forward declare base::Time). I definitely need base/time/time.h, but I also need base/time/clock or the forward declare.
The CQ bit was unchecked by ryansturm@chromium.org
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/2439203002/#ps80001 (title: "tbansal commetns")
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
Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by ryansturm@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Adding a short blacklist period after every previews opt out Within a session, if a user opts out of a preview navigation, the user won't be shown any previews for a short duration in that session. This does not extend outside the session because the general rationale behind this is a user temporarily wanting to consume many non-previews navigations consecutively. BUG=656802 ========== to ========== Adding a short blacklist period after every previews opt out Within a session, if a user opts out of a preview navigation, the user won't be shown any previews for a short duration in that session. This does not extend outside the session because the general rationale behind this is a user temporarily wanting to consume many non-previews navigations consecutively. BUG=656802 Committed: https://crrev.com/960e2c0a2455a4be4e71b9b0923115785bf6cd1e Cr-Commit-Position: refs/heads/master@{#427480} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/960e2c0a2455a4be4e71b9b0923115785bf6cd1e Cr-Commit-Position: refs/heads/master@{#427480} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
