Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2864333003:
Use the Previews Black List for server previews (Closed)
|
Created:
3 years, 7 months ago by RyanSturm Modified:
3 years, 7 months ago CC:
chromium-reviews, droger+watchlist_chromium.org, blundell+watchlist_chromium.org, sdefresne+watchlist_chromium.org, Randy Smith (Not in Mondays), loading-reviews_chromium.org, tbansal+watch-data-reduction-proxy_chromium.org, mmenke Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse the Previews Black List for server previews
This introduces a field trial to allow Flywheel server previews to use
the PreviewsBlackList to make decisions about whether to allow a
preview. The 3x3 rule that LoFi/WebLite currently use will still be
respected for legacy opted out users, but it will no longer be written
to while in the field trial.
BUG=671334
Review-Url: https://codereview.chromium.org/2864333003
Cr-Commit-Position: refs/heads/master@{#470845}
Committed: https://chromium.googlesource.com/chromium/src/+/ef62bd2a4181d1bd3e0e2e17d66d8f8e717377ee
Patch Set 1 #Patch Set 2 : deps change #
Total comments: 22
Patch Set 3 : megjablon comments #Patch Set 4 : missed a comment #Patch Set 5 : . #
Total comments: 6
Patch Set 6 : megjablon comments #
Total comments: 6
Patch Set 7 : megjablon nits #
Total comments: 4
Patch Set 8 : mmenke nits #Patch Set 9 : . #Messages
Total messages: 69 (52 generated)
The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run
Description was changed from ========== Use the Previews Black List for server previews This introduces a field trial to allow Flywheel server previews to use the PreviewsBlackList to make decisions about whether to allow a preview. The 3x3 rule that LoFi/WebLite currently use will stil lbe respected for legacy opted out users, but it will no longer be written to while in the field trial. BUG=71334 ========== to ========== Use the Previews Black List for server previews This introduces a field trial to allow Flywheel server previews to use the PreviewsBlackList to make decisions about whether to allow a preview. The 3x3 rule that LoFi/WebLite currently use will stil lbe respected for legacy opted out users, but it will no longer be written to while in the field trial. BUG=671334 ==========
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: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
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: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_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 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...)
ryansturm@chromium.org changed reviewers: + megjablon@chromium.org
megjablon: PTAL
Fix "use will stil lbe" typo in description please. https://codereview.chromium.org/2864333003/diff/40001/chrome/browser/previews... File chrome/browser/previews/previews_infobar_delegate.cc (right): https://codereview.chromium.org/2864333003/diff/40001/chrome/browser/previews... chrome/browser/previews/previews_infobar_delegate.cc:147: if (!on_dismiss_callback_.is_null()) Do Lo-Fi and Lite pages already have on_dismiss_callback set? https://codereview.chromium.org/2864333003/diff/40001/chrome/browser/previews... File chrome/browser/previews/previews_infobar_delegate_unittest.cc (right): https://codereview.chromium.org/2864333003/diff/40001/chrome/browser/previews... chrome/browser/previews/previews_infobar_delegate_unittest.cc:300: InfobarTestClickLinkLoFiBlackListExperiment) { Instead of duplicating all this code can we just add a test_cases struct to the above test with a blacklist_experiment bool? https://codereview.chromium.org/2864333003/diff/40001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/2864333003/diff/40001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1005: // speed is checked in IsNetworkQualityProhibitivelySlow(). Can we have ShouldAllowPreviewAtECT check the NQE instead? If yes but not in this cl, can we file a bug and add a TODO? https://codereview.chromium.org/2864333003/diff/40001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1015: // blacklist. Fix comment formatting https://codereview.chromium.org/2864333003/diff/40001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1045: if (params::IsBlackListEnabledForServerPreviews()) { Same two comments as above https://codereview.chromium.org/2864333003/diff/40001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://codereview.chromium.org/2864333003/diff/40001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:202: previews::PreviewsDecider* previews_decider); Add comment for what |previews_decider| is for. Same for three methods below. https://codereview.chromium.org/2864333003/diff/40001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc (right): https://codereview.chromium.org/2864333003/diff/40001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:934: false, true, std::string(), false, false, 0, Are all of these necessary? This is getting to be a massive list. Could we do a separate test? https://codereview.chromium.org/2864333003/diff/40001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc (right): https://codereview.chromium.org/2864333003/diff/40001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:1243: TestPreviewsDecider test_previews_decider; Add same comment as above: // Needed as a parameter, but functionality is not tested. https://codereview.chromium.org/2864333003/diff/40001/components/data_reducti... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc (right): https://codereview.chromium.org/2864333003/diff/40001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:49: "DataReductionProxyPreviewsBlacklistTransition"; s/Blacklist/BlackList Please check that this consistent everywhere. https://codereview.chromium.org/2864333003/diff/40001/components/data_reducti... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.h (right): https://codereview.chromium.org/2864333003/diff/40001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:148: // Whether the blacklist should be used for server LoFi and server Weblite Use Lite Page here. We use Lite Page to refer to WebLite everywhere in 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...
The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run
https://codereview.chromium.org/2864333003/diff/40001/chrome/browser/previews... File chrome/browser/previews/previews_infobar_delegate.cc (right): https://codereview.chromium.org/2864333003/diff/40001/chrome/browser/previews... chrome/browser/previews/previews_infobar_delegate.cc:147: if (!on_dismiss_callback_.is_null()) On 2017/05/09 19:58:33, megjablon wrote: > Do Lo-Fi and Lite pages already have on_dismiss_callback set? Yes. That landed yesterday. https://codereview.chromium.org/2864333003/diff/40001/chrome/browser/previews... File chrome/browser/previews/previews_infobar_delegate_unittest.cc (right): https://codereview.chromium.org/2864333003/diff/40001/chrome/browser/previews... chrome/browser/previews/previews_infobar_delegate_unittest.cc:300: InfobarTestClickLinkLoFiBlackListExperiment) { On 2017/05/09 19:58:33, megjablon wrote: > Instead of duplicating all this code can we just add a test_cases struct to the > above test with a blacklist_experiment bool? Done. https://codereview.chromium.org/2864333003/diff/40001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/2864333003/diff/40001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1005: // speed is checked in IsNetworkQualityProhibitivelySlow(). On 2017/05/09 19:58:33, megjablon wrote: > Can we have ShouldAllowPreviewAtECT check the NQE instead? If yes but not in > this cl, can we file a bug and add a TODO? Done. This is slightly complicated based on other code that runs in IsNetworkQualityProhibitivelySlow, but this could be moved to CRDHD probably at some point. https://codereview.chromium.org/2864333003/diff/40001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1015: // blacklist. On 2017/05/09 19:58:33, megjablon wrote: > Fix comment formatting Done. https://codereview.chromium.org/2864333003/diff/40001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1045: if (params::IsBlackListEnabledForServerPreviews()) { On 2017/05/09 19:58:33, megjablon wrote: > Same two comments as above Done. https://codereview.chromium.org/2864333003/diff/40001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://codereview.chromium.org/2864333003/diff/40001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:202: previews::PreviewsDecider* previews_decider); On 2017/05/09 19:58:33, megjablon wrote: > Add comment for what |previews_decider| is for. Same for three methods below. Done. https://codereview.chromium.org/2864333003/diff/40001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc (right): https://codereview.chromium.org/2864333003/diff/40001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:934: false, true, std::string(), false, false, 0, On 2017/05/09 19:58:33, megjablon wrote: > Are all of these necessary? This is getting to be a massive list. Could we do a > separate test? I think we should leave it for now, but when we deprecate 3x3 a lot of these test cases will disappear. I can move it if you like however (or I can just add a second for loop that just runs over true/false). https://codereview.chromium.org/2864333003/diff/40001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc (right): https://codereview.chromium.org/2864333003/diff/40001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:1243: TestPreviewsDecider test_previews_decider; On 2017/05/09 19:58:33, megjablon wrote: > Add same comment as above: > // Needed as a parameter, but functionality is not tested. Done. https://codereview.chromium.org/2864333003/diff/40001/components/data_reducti... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc (right): https://codereview.chromium.org/2864333003/diff/40001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:49: "DataReductionProxyPreviewsBlacklistTransition"; On 2017/05/09 19:58:33, megjablon wrote: > s/Blacklist/BlackList > Please check that this consistent everywhere. Done. https://codereview.chromium.org/2864333003/diff/40001/components/data_reducti... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.h (right): https://codereview.chromium.org/2864333003/diff/40001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:148: // Whether the blacklist should be used for server LoFi and server Weblite On 2017/05/09 19:58:33, megjablon wrote: > Use Lite Page here. We use Lite Page to refer to WebLite everywhere in chromium. 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: 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...)
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...
weird harness code doesn't always do the same thing as production code when simulating navigations, so I left the tests seperate. https://codereview.chromium.org/2864333003/diff/40001/chrome/browser/previews... File chrome/browser/previews/previews_infobar_delegate_unittest.cc (right): https://codereview.chromium.org/2864333003/diff/40001/chrome/browser/previews... chrome/browser/previews/previews_infobar_delegate_unittest.cc:300: InfobarTestClickLinkLoFiBlackListExperiment) { On 2017/05/09 22:35:18, RyanSturm wrote: > On 2017/05/09 19:58:33, megjablon wrote: > > Instead of duplicating all this code can we just add a test_cases struct to > the > > above test with a blacklist_experiment bool? > > Done. NavigateAndCommit doesn't really do the right thing wrt calling DidFinishNavigation and the hack I tried to use didn't help, so I am leaving these seperate.
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
Patchset #5 (id:100001) 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...
On 2017/05/10 16:01:46, RyanSturm wrote: > weird harness code doesn't always do the same thing as production code when > simulating navigations, so I left the tests seperate. > > https://codereview.chromium.org/2864333003/diff/40001/chrome/browser/previews... > File chrome/browser/previews/previews_infobar_delegate_unittest.cc (right): > > https://codereview.chromium.org/2864333003/diff/40001/chrome/browser/previews... > chrome/browser/previews/previews_infobar_delegate_unittest.cc:300: > InfobarTestClickLinkLoFiBlackListExperiment) { > On 2017/05/09 22:35:18, RyanSturm wrote: > > On 2017/05/09 19:58:33, megjablon wrote: > > > Instead of duplicating all this code can we just add a test_cases struct to > > the > > > above test with a blacklist_experiment bool? > > > > Done. > > NavigateAndCommit doesn't really do the right thing wrt calling > DidFinishNavigation and the hack I tried to use didn't help, so I am leaving > these seperate. Trying something else. If the try bots pass, I'll keep it as is. I'll let you know when it should be reviewed.
megjablon: PTAL, sorry about all the extra comments.
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_...)
Fix "use will stil lbe" typo in description please. https://codereview.chromium.org/2864333003/diff/40001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc (right): https://codereview.chromium.org/2864333003/diff/40001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:934: false, true, std::string(), false, false, 0, On 2017/05/09 22:35:19, RyanSturm wrote: > On 2017/05/09 19:58:33, megjablon wrote: > > Are all of these necessary? This is getting to be a massive list. Could we do > a > > separate test? > > I think we should leave it for now, but when we deprecate 3x3 a lot of these > test cases will disappear. I can move it if you like however (or I can just add > a second for loop that just runs over true/false). Waiting for 3x3 deprecation and CPAT SGTM. https://codereview.chromium.org/2864333003/diff/120001/chrome/browser/loader/... File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2864333003/diff/120001/chrome/browser/loader/... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:895: previews::PreviewsIOData* previews_io_data = io_data->previews_io_data(); Should there be a null check here? Can we move this line above and do: if (data_reduction_proxy_io_data && previews_io_data) and remove the previews_io_data check in the if statement on 907. https://codereview.chromium.org/2864333003/diff/120001/chrome/browser/preview... File chrome/browser/previews/previews_infobar_delegate_unittest.cc (right): https://codereview.chromium.org/2864333003/diff/120001/chrome/browser/preview... chrome/browser/previews/previews_infobar_delegate_unittest.cc:275: bool is_transitioned; using_previews_black_list? or something that mentions the blacklist. thanks for combining these. https://codereview.chromium.org/2864333003/diff/120001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h (right): https://codereview.chromium.org/2864333003/diff/120001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:104: // resources, except when the user is in the Lo-Fi control group. Add comment for previews_decider here like in DRP config.
Description was changed from ========== Use the Previews Black List for server previews This introduces a field trial to allow Flywheel server previews to use the PreviewsBlackList to make decisions about whether to allow a preview. The 3x3 rule that LoFi/WebLite currently use will stil lbe respected for legacy opted out users, but it will no longer be written to while in the field trial. BUG=671334 ========== to ========== Use the Previews Black List for server previews This introduces a field trial to allow Flywheel server previews to use the PreviewsBlackList to make decisions about whether to allow a preview. The 3x3 rule that LoFi/WebLite currently use will still be respected for legacy opted out users, but it will no longer be written to while in the field trial. BUG=671334 ==========
The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run
fixed description typo as well megjablon: PTAL mmenke: PTAL @ crdhd https://codereview.chromium.org/2864333003/diff/120001/chrome/browser/loader/... File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2864333003/diff/120001/chrome/browser/loader/... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:895: previews::PreviewsIOData* previews_io_data = io_data->previews_io_data(); On 2017/05/10 19:11:23, megjablon wrote: > Should there be a null check here? Can we move this line above and do: > > if (data_reduction_proxy_io_data && previews_io_data) > > and remove the previews_io_data check in the if statement on 907. Done. https://codereview.chromium.org/2864333003/diff/120001/chrome/browser/preview... File chrome/browser/previews/previews_infobar_delegate_unittest.cc (right): https://codereview.chromium.org/2864333003/diff/120001/chrome/browser/preview... chrome/browser/previews/previews_infobar_delegate_unittest.cc:275: bool is_transitioned; On 2017/05/10 19:11:23, megjablon wrote: > using_previews_black_list? or something that mentions the blacklist. thanks for > combining these. Done. https://codereview.chromium.org/2864333003/diff/120001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h (right): https://codereview.chromium.org/2864333003/diff/120001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:104: // resources, except when the user is in the Lo-Fi control group. On 2017/05/10 19:11:23, megjablon wrote: > Add comment for previews_decider here like in DRP config. 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 @ crdhd
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm % comments https://codereview.chromium.org/2864333003/diff/140001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/2864333003/diff/140001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1008: request, previews::PreviewsType::LOFI, #include "components/previews/core/previews_experiments.h" https://codereview.chromium.org/2864333003/diff/140001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc (right): https://codereview.chromium.org/2864333003/diff/140001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:91: previews::PreviewsType type, #include "components/previews/core/previews_experiments.h" https://codereview.chromium.org/2864333003/diff/140001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc (right): https://codereview.chromium.org/2864333003/diff/140001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:236: previews::PreviewsType type, #include "components/previews/core/previews_experiments.h"
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/2864333003/diff/140001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/2864333003/diff/140001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:1008: request, previews::PreviewsType::LOFI, On 2017/05/10 21:48:44, megjablon wrote: > #include "components/previews/core/previews_experiments.h" Done. https://codereview.chromium.org/2864333003/diff/140001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc (right): https://codereview.chromium.org/2864333003/diff/140001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:91: previews::PreviewsType type, On 2017/05/10 21:48:44, megjablon wrote: > #include "components/previews/core/previews_experiments.h" Done. https://codereview.chromium.org/2864333003/diff/140001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc (right): https://codereview.chromium.org/2864333003/diff/140001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:236: previews::PreviewsType type, On 2017/05/10 21:48:44, megjablon wrote: > #include "components/previews/core/previews_experiments.h" Done.
chrome/browser/loader/ LGTM https://codereview.chromium.org/2864333003/diff/160001/chrome/browser/loader/... File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2864333003/diff/160001/chrome/browser/loader/... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:898: previews_state |= content::SERVER_LOFI_ON; nit: Add braces https://codereview.chromium.org/2864333003/diff/160001/chrome/browser/loader/... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:901: previews_state |= content::SERVER_LITE_PAGE_ON; +braces
The CQ bit was checked by ryansturm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from megjablon@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2864333003/#ps180001 (title: "mmenke nits")
https://codereview.chromium.org/2864333003/diff/160001/chrome/browser/loader/... File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2864333003/diff/160001/chrome/browser/loader/... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:898: previews_state |= content::SERVER_LOFI_ON; On 2017/05/10 22:32:47, mmenke wrote: > nit: Add braces Done. https://codereview.chromium.org/2864333003/diff/160001/chrome/browser/loader/... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:901: previews_state |= content::SERVER_LITE_PAGE_ON; On 2017/05/10 22:32:47, mmenke wrote: > +braces Done.
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 ryansturm@chromium.org
The CQ bit was checked by ryansturm@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by ryansturm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org, megjablon@chromium.org Link to the patchset: https://codereview.chromium.org/2864333003/#ps190001 (title: ".")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 190001, "attempt_start_ts": 1494477454580930,
"parent_rev": "55ad6749e01eb6bb63b6fb2f64655a82cf4e7d2c", "commit_rev":
"ef62bd2a4181d1bd3e0e2e17d66d8f8e717377ee"}
Message was sent while issue was closed.
Description was changed from ========== Use the Previews Black List for server previews This introduces a field trial to allow Flywheel server previews to use the PreviewsBlackList to make decisions about whether to allow a preview. The 3x3 rule that LoFi/WebLite currently use will still be respected for legacy opted out users, but it will no longer be written to while in the field trial. BUG=671334 ========== to ========== Use the Previews Black List for server previews This introduces a field trial to allow Flywheel server previews to use the PreviewsBlackList to make decisions about whether to allow a preview. The 3x3 rule that LoFi/WebLite currently use will still be respected for legacy opted out users, but it will no longer be written to while in the field trial. BUG=671334 Review-Url: https://codereview.chromium.org/2864333003 Cr-Commit-Position: refs/heads/master@{#470845} Committed: https://chromium.googlesource.com/chromium/src/+/ef62bd2a4181d1bd3e0e2e17d66d... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:190001) as https://chromium.googlesource.com/chromium/src/+/ef62bd2a4181d1bd3e0e2e17d66d... |
