Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2760063002:
Add support to previews/ for Server LoFi and LitePages (Closed)
DescriptionAdd support to previews/ for Server LoFi and LitePages
This adds support for using the previews blacklist for LoFi and
LitePages as well as adding histograms for LoFi and LitePages within the
previews framework.
BUG=703293
Review-Url: https://codereview.chromium.org/2760063002
Cr-Commit-Position: refs/heads/master@{#469800}
Committed: https://chromium.googlesource.com/chromium/src/+/b24d2336dc516900281fc2034adaddc29769e3a5
Patch Set 1 #Patch Set 2 : build fix #
Total comments: 8
Patch Set 3 : tbansal comments #Patch Set 4 : rebase and previews_service_unittest.cc #
Total comments: 31
Patch Set 5 : tbansal comments #Patch Set 6 : passing string into variadic StringPrintF #
Total comments: 2
Patch Set 7 : tbansal comment #
Total comments: 2
Patch Set 8 : Coverging LoFi types #
Total comments: 8
Patch Set 9 : rebase #
Total comments: 4
Patch Set 10 : tbansal comments #
Total comments: 4
Patch Set 11 : comment fix #Messages
Total messages: 100 (82 generated)
The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_asan_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...
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/2760063002/diff/20001/components/previews/cor... File components/previews/core/previews_io_data.cc (right): https://codereview.chromium.org/2760063002/diff/20001/components/previews/cor... components/previews/core/previews_io_data.cc:33: "Previews.EligibilityReason.LitePage", static_cast<int>(status), Maybe use histogram factory here to avoid code duplication? https://codereview.chromium.org/2760063002/diff/20001/components/previews/cor... components/previews/core/previews_io_data.cc:52: bool ShouldCheckNetworkQuality(PreviewsType type) { May be change it to return the ECT threshold? and if ECT_LAST is returned, then do not check the network conditions? https://codereview.chromium.org/2760063002/diff/20001/components/previews/cor... components/previews/core/previews_io_data.cc:154: if (ShouldCheckNetworkQuality(type)) { nit: it might be more readable to move this (including all the ECT related stuff) to a separate method. https://codereview.chromium.org/2760063002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2760063002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:53392: +<histogram name="Previews.EligibilityReason.LitePage" Use a common histogram "Previews.EligibilityReason" and then supply suffixes. Similar, for other histograms below.
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: 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/2760063002/diff/20001/components/previews/cor... File components/previews/core/previews_io_data.cc (right): https://codereview.chromium.org/2760063002/diff/20001/components/previews/cor... components/previews/core/previews_io_data.cc:33: "Previews.EligibilityReason.LitePage", static_cast<int>(status), On 2017/03/20 22:55:13, tbansal1 wrote: > Maybe use histogram factory here to avoid code duplication? Done. https://codereview.chromium.org/2760063002/diff/20001/components/previews/cor... components/previews/core/previews_io_data.cc:52: bool ShouldCheckNetworkQuality(PreviewsType type) { On 2017/03/20 22:55:13, tbansal1 wrote: > May be change it to return the ECT threshold? > and if ECT_LAST is returned, then do not check the network conditions? Acknowledged. https://codereview.chromium.org/2760063002/diff/20001/components/previews/cor... components/previews/core/previews_io_data.cc:154: if (ShouldCheckNetworkQuality(type)) { On 2017/03/20 22:55:13, tbansal1 wrote: > nit: it might be more readable to move this (including all the ECT related > stuff) to a separate method. Done. https://codereview.chromium.org/2760063002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2760063002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:53392: +<histogram name="Previews.EligibilityReason.LitePage" On 2017/03/20 22:55:13, tbansal1 wrote: > Use a common histogram > "Previews.EligibilityReason" > and then supply suffixes. > > Similar, for other histograms below. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2760063002/diff/60001/chrome/browser/previews... File chrome/browser/previews/previews_service.cc (right): https://codereview.chromium.org/2760063002/diff/60001/chrome/browser/previews... chrome/browser/previews/previews_service.cc:37: // List remaining enum cases vs. default to catch when new one is added. nit: I do not think this comment is needed since it is pretty common pattern in switch case (probably it's also a part of the style guide). https://codereview.chromium.org/2760063002/diff/60001/chrome/browser/previews... File chrome/browser/previews/previews_service_unittest.cc (right): https://codereview.chromium.org/2760063002/diff/60001/chrome/browser/previews... chrome/browser/previews/previews_service_unittest.cc:37: // This test class intercepts the is_enabled_callback and is used to test its s/is_enabled_callback/|is_enabled_callback|/ https://codereview.chromium.org/2760063002/diff/60001/chrome/browser/previews... chrome/browser/previews/previews_service_unittest.cc:84: TestPreviewsIOData* io_data() { return io_data_.get(); } const method? https://codereview.chromium.org/2760063002/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.h (right): https://codereview.chromium.org/2760063002/diff/60001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:149: int LoFiVersion(); s/LoFiVersion/ServerLoFiVersion/? https://codereview.chromium.org/2760063002/diff/60001/components/previews/cor... File components/previews/core/previews_black_list.cc (right): https://codereview.chromium.org/2760063002/diff/60001/components/previews/cor... components/previews/core/previews_black_list.cc:77: std::string histogram_name("Previews.OptOut.UserOptedOut."); Use StringPiece or base::StringPrintf to reduce overhead of string manipulation. https://codereview.chromium.org/2760063002/diff/60001/components/previews/cor... File components/previews/core/previews_decider.h (right): https://codereview.chromium.org/2760063002/diff/60001/components/previews/cor... components/previews/core/previews_decider.h:19: // not use the default network quality threhold should check the network typo in threhold. May be somehow refer to GetEffectiveConnectionTypeThresholdForPreviewsType() to make it clear how the place of the code where previews "use the default network quality threshold". https://codereview.chromium.org/2760063002/diff/60001/components/previews/cor... File components/previews/core/previews_experiments.cc (right): https://codereview.chromium.org/2760063002/diff/60001/components/previews/cor... components/previews/core/previews_experiments.cc:7: #include <string> rm this include since this is now in header file. https://codereview.chromium.org/2760063002/diff/60001/components/previews/cor... File components/previews/core/previews_io_data.cc (right): https://codereview.chromium.org/2760063002/diff/60001/components/previews/cor... components/previews/core/previews_io_data.cc:7: #include <string> This is already included in the header file, so it might not be needed here. https://codereview.chromium.org/2760063002/diff/60001/components/previews/cor... components/previews/core/previews_io_data.cc:34: histogram_name.append(GetStringNameForType(type)); same here. Use StringPiece of Printf as mentioned elsewhere. https://codereview.chromium.org/2760063002/diff/60001/components/previews/cor... components/previews/core/previews_io_data.cc:38: histogram_name, 0, max_limit, max_limit + 1, min_bucket should be >= 1. https://cs.chromium.org/chromium/src/base/metrics/histogram.h?rcl=9d3c221bdef... https://codereview.chromium.org/2760063002/diff/60001/components/previews/cor... components/previews/core/previews_io_data.cc:46: // These types check network qualtiy on their own. typo in quality. https://codereview.chromium.org/2760063002/diff/60001/components/previews/cor... File components/previews/core/previews_io_data_unittest.cc (right): https://codereview.chromium.org/2760063002/diff/60001/components/previews/cor... components/previews/core/previews_io_data_unittest.cc:49: // Use the offline field trial to simulate LoFi and LitePages being enabled. Is it possible to expand on this comment a bit? This is a bit confusing (why is LoFi/Lite using offline field trial). https://codereview.chromium.org/2760063002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2760063002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:126024: + <affected-histogram name="Previews.EligibilityReason"/> rm this line since it is already present below. https://codereview.chromium.org/2760063002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:126030: + <affected-histogram name="Previews.InfoBarAction"/> previews_infobar_delegate.cc already defines Previews.InfoBarAction.LoFi. That sounds a bit same as Previews.InfoBarAction.ServerLoFi defined here. https://codereview.chromium.org/2760063002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:126030: + <affected-histogram name="Previews.InfoBarAction"/> Also, where is Previews.InfoBarAction.ClientLoFi populated?
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/2760063002/diff/60001/chrome/browser/previews... File chrome/browser/previews/previews_service.cc (right): https://codereview.chromium.org/2760063002/diff/60001/chrome/browser/previews... chrome/browser/previews/previews_service.cc:37: // List remaining enum cases vs. default to catch when new one is added. On 2017/05/02 21:49:30, tbansal1 wrote: > nit: I do not think this comment is needed since it is pretty common pattern in > switch case (probably it's also a part of the style guide). Done. https://codereview.chromium.org/2760063002/diff/60001/chrome/browser/previews... File chrome/browser/previews/previews_service_unittest.cc (right): https://codereview.chromium.org/2760063002/diff/60001/chrome/browser/previews... chrome/browser/previews/previews_service_unittest.cc:37: // This test class intercepts the is_enabled_callback and is used to test its On 2017/05/02 21:49:31, tbansal1 wrote: > s/is_enabled_callback/|is_enabled_callback|/ Done. https://codereview.chromium.org/2760063002/diff/60001/chrome/browser/previews... chrome/browser/previews/previews_service_unittest.cc:84: TestPreviewsIOData* io_data() { return io_data_.get(); } On 2017/05/02 21:49:31, tbansal1 wrote: > const method? Done. https://codereview.chromium.org/2760063002/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.h (right): https://codereview.chromium.org/2760063002/diff/60001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:149: int LoFiVersion(); On 2017/05/02 21:49:31, tbansal1 wrote: > s/LoFiVersion/ServerLoFiVersion/? Done. https://codereview.chromium.org/2760063002/diff/60001/components/previews/cor... File components/previews/core/previews_black_list.cc (right): https://codereview.chromium.org/2760063002/diff/60001/components/previews/cor... components/previews/core/previews_black_list.cc:77: std::string histogram_name("Previews.OptOut.UserOptedOut."); On 2017/05/02 21:49:31, tbansal1 wrote: > Use StringPiece or base::StringPrintf to reduce overhead of string > manipulation. Done. https://codereview.chromium.org/2760063002/diff/60001/components/previews/cor... File components/previews/core/previews_decider.h (right): https://codereview.chromium.org/2760063002/diff/60001/components/previews/cor... components/previews/core/previews_decider.h:19: // not use the default network quality threhold should check the network On 2017/05/02 21:49:31, tbansal1 wrote: > typo in threhold. > > May be somehow refer to GetEffectiveConnectionTypeThresholdForPreviewsType() to > make it clear how the place of the code where previews "use the default network > quality threshold". Done. https://codereview.chromium.org/2760063002/diff/60001/components/previews/cor... File components/previews/core/previews_experiments.cc (right): https://codereview.chromium.org/2760063002/diff/60001/components/previews/cor... components/previews/core/previews_experiments.cc:7: #include <string> On 2017/05/02 21:49:31, tbansal1 wrote: > rm this include since this is now in header file. Done. https://codereview.chromium.org/2760063002/diff/60001/components/previews/cor... components/previews/core/previews_experiments.cc:7: #include <string> On 2017/05/02 21:49:31, tbansal1 wrote: > rm this include since this is now in header file. Done. https://codereview.chromium.org/2760063002/diff/60001/components/previews/cor... File components/previews/core/previews_io_data.cc (right): https://codereview.chromium.org/2760063002/diff/60001/components/previews/cor... components/previews/core/previews_io_data.cc:7: #include <string> On 2017/05/02 21:49:31, tbansal1 wrote: > This is already included in the header file, so it might not be needed here. Done. https://codereview.chromium.org/2760063002/diff/60001/components/previews/cor... components/previews/core/previews_io_data.cc:34: histogram_name.append(GetStringNameForType(type)); On 2017/05/02 21:49:31, tbansal1 wrote: > same here. > Use StringPiece of Printf as mentioned elsewhere. Done. https://codereview.chromium.org/2760063002/diff/60001/components/previews/cor... components/previews/core/previews_io_data.cc:38: histogram_name, 0, max_limit, max_limit + 1, On 2017/05/02 21:49:31, tbansal1 wrote: > min_bucket should be >= 1. > https://cs.chromium.org/chromium/src/base/metrics/histogram.h?rcl=9d3c221bdef... Done. https://codereview.chromium.org/2760063002/diff/60001/components/previews/cor... components/previews/core/previews_io_data.cc:46: // These types check network qualtiy on their own. On 2017/05/02 21:49:31, tbansal1 wrote: > typo in quality. Done. https://codereview.chromium.org/2760063002/diff/60001/components/previews/cor... File components/previews/core/previews_io_data_unittest.cc (right): https://codereview.chromium.org/2760063002/diff/60001/components/previews/cor... components/previews/core/previews_io_data_unittest.cc:49: // Use the offline field trial to simulate LoFi and LitePages being enabled. On 2017/05/02 21:49:31, tbansal1 wrote: > Is it possible to expand on this comment a bit? This is a bit confusing (why is > LoFi/Lite using offline field trial). Done. I removed sclittle's comment as it makes little sense to try to remove a layering abstraction in test. Using the field trials is syntactically easy, but this check could be replaced with any other bool that controls the flow of this method for the purpose of the test. https://codereview.chromium.org/2760063002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2760063002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:126024: + <affected-histogram name="Previews.EligibilityReason"/> On 2017/05/02 21:49:31, tbansal1 wrote: > rm this line since it is already present below. Done. https://codereview.chromium.org/2760063002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:126030: + <affected-histogram name="Previews.InfoBarAction"/> On 2017/05/02 21:49:31, tbansal1 wrote: > previews_infobar_delegate.cc already defines Previews.InfoBarAction.LoFi. > That sounds a bit same as Previews.InfoBarAction.ServerLoFi defined here. Absolutely. This was a typo that is addressed throughout the CL now. https://codereview.chromium.org/2760063002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:126030: + <affected-histogram name="Previews.InfoBarAction"/> On 2017/05/02 21:49:31, tbansal1 wrote: > Also, where is Previews.InfoBarAction.ClientLoFi populated? TBD. Since the work to actually expose Client LoFi just isn't done... I haven't landed infobar yet. Complete CL here: https://codereview.chromium.org/2848293002/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) 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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % nit. https://codereview.chromium.org/2760063002/diff/100001/chrome/browser/preview... File chrome/browser/previews/previews_service_unittest.cc (right): https://codereview.chromium.org/2760063002/diff/100001/chrome/browser/preview... chrome/browser/previews/previews_service_unittest.cc:99: variations::testing::ClearAllVariationParams(); nit: This repeated line can be moved to the TearDown() function of class PreviewsServiceTest.
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: + holte@chromium.org
holte: PTAL @ histograms.xml https://codereview.chromium.org/2760063002/diff/100001/chrome/browser/preview... File chrome/browser/previews/previews_service_unittest.cc (right): https://codereview.chromium.org/2760063002/diff/100001/chrome/browser/preview... chrome/browser/previews/previews_service_unittest.cc:99: variations::testing::ClearAllVariationParams(); On 2017/05/03 17:29:49, tbansal1 wrote: > nit: This repeated line can be moved to the TearDown() function of > class PreviewsServiceTest. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2760063002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2760063002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:55402: - User interactions with the previews LitePage "Saved data" infobar. Mark histograms obsolete rather than deleting them.
lgtm after another look https://codereview.chromium.org/2760063002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2760063002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:55402: - User interactions with the previews LitePage "Saved data" infobar. On 2017/05/03 23:57:44, Steven Holte wrote: > Mark histograms obsolete rather than deleting them. Oh, actually I see these are just now described through histogram suffixes? That is fine then.
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_chromium_chromeos_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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run
Patchset #8 (id:140001) has been deleted
Patchset #8 (id:160001) 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...
Patchset #8 (id:180001) has been deleted
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 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_chromium_chromeos_ozone_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
Patchset #8 (id:200001) 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 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 #9 (id:240001) has been deleted
https://codereview.chromium.org/2760063002/diff/220001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc (right): https://codereview.chromium.org/2760063002/diff/220001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc:196: bool ShouldAllowPreview( I can dig up but IIRC, the style guide prohibits (discourages?) multiple methods with same name even if they have different arguments. https://codereview.chromium.org/2760063002/diff/220001/chrome/browser/preview... File chrome/browser/previews/previews_service.cc (right): https://codereview.chromium.org/2760063002/diff/220001/chrome/browser/preview... chrome/browser/previews/previews_service.cc:40: LOG(WARNING) << static_cast<int>(type); rm this https://codereview.chromium.org/2760063002/diff/220001/components/previews/co... File components/previews/core/previews_decider.h (right): https://codereview.chromium.org/2760063002/diff/220001/components/previews/co... components/previews/core/previews_decider.h:24: virtual bool ShouldAllowPreview( add comments for the args. https://codereview.chromium.org/2760063002/diff/220001/components/previews/co... File components/previews/core/previews_experiments.h (right): https://codereview.chromium.org/2760063002/diff/220001/components/previews/co... components/previews/core/previews_experiments.h:64: // The blacklist version for Client LoFi previews. Is this also the blacklist version for the server lofi? https://codereview.chromium.org/2760063002/diff/260001/components/previews/co... File components/previews/core/previews_experiments.h (right): https://codereview.chromium.org/2760063002/diff/260001/components/previews/co... components/previews/core/previews_experiments.h:53: net::EffectiveConnectionType EffectiveConnectionTypeThreshold(); why rename this? As per the comment above, seems like this is specific for offline previews. https://codereview.chromium.org/2760063002/diff/260001/components/previews/co... File components/previews/core/previews_io_data.cc (right): https://codereview.chromium.org/2760063002/diff/260001/components/previews/co... components/previews/core/previews_io_data.cc:145: effective_connection_type_threshold) { Why was the API changed?
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 #10 (id:210018) 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...
https://codereview.chromium.org/2760063002/diff/220001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc (right): https://codereview.chromium.org/2760063002/diff/220001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc:196: bool ShouldAllowPreview( On 2017/05/05 20:34:38, tbansal1 wrote: > I can dig up but IIRC, the style guide prohibits (discourages?) multiple methods > with same name even if they have different arguments. Done. https://codereview.chromium.org/2760063002/diff/220001/chrome/browser/preview... File chrome/browser/previews/previews_service.cc (right): https://codereview.chromium.org/2760063002/diff/220001/chrome/browser/preview... chrome/browser/previews/previews_service.cc:40: LOG(WARNING) << static_cast<int>(type); On 2017/05/05 20:34:39, tbansal1 wrote: > rm this Done. https://codereview.chromium.org/2760063002/diff/220001/components/previews/co... File components/previews/core/previews_decider.h (right): https://codereview.chromium.org/2760063002/diff/220001/components/previews/co... components/previews/core/previews_decider.h:24: virtual bool ShouldAllowPreview( On 2017/05/05 20:34:39, tbansal1 wrote: > add comments for the args. Done. https://codereview.chromium.org/2760063002/diff/220001/components/previews/co... File components/previews/core/previews_experiments.h (right): https://codereview.chromium.org/2760063002/diff/220001/components/previews/co... components/previews/core/previews_experiments.h:64: // The blacklist version for Client LoFi previews. On 2017/05/05 20:34:39, tbansal1 wrote: > Is this also the blacklist version for the server lofi? Yes. At that layer, the two optimizations are indistinguishable and clearing them will use just this param. https://codereview.chromium.org/2760063002/diff/260001/components/previews/co... File components/previews/core/previews_experiments.h (right): https://codereview.chromium.org/2760063002/diff/260001/components/previews/co... components/previews/core/previews_experiments.h:53: net::EffectiveConnectionType EffectiveConnectionTypeThreshold(); On 2017/05/05 20:34:39, tbansal1 wrote: > why rename this? As per the comment above, seems like this is specific for > offline previews. Done. https://codereview.chromium.org/2760063002/diff/260001/components/previews/co... File components/previews/core/previews_io_data.cc (right): https://codereview.chromium.org/2760063002/diff/260001/components/previews/co... components/previews/core/previews_io_data.cc:145: effective_connection_type_threshold) { On 2017/05/05 20:34:39, tbansal1 wrote: > Why was the API changed? Because the LoFis have different params despite sharing an enum at this layer.
https://codereview.chromium.org/2760063002/diff/290001/components/previews/co... File components/previews/core/previews_decider.h (right): https://codereview.chromium.org/2760063002/diff/290001/components/previews/co... components/previews/core/previews_decider.h:23: // network quality before calling ShouldAllowPreview. |request| is the main s/ShouldAllowPreview/ShouldAllowPreviewAtECT/ https://codereview.chromium.org/2760063002/diff/290001/components/previews/co... components/previews/core/previews_decider.h:25: // If the current ECT better than |effective_connection_type_threshold|, the s/better/strictly faster/ OR s/better/at least as fast as/
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
The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run
Patchset #11 (id:310001) 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...
ryansturm@chromium.org changed reviewers: + jianli@chromium.org, mmenke@chromium.org
mmenke: PTAL at c_r_d_h_d, thanks again! jianli: PTAL @ PreviewsDecider interface change in test. https://codereview.chromium.org/2760063002/diff/290001/components/previews/co... File components/previews/core/previews_decider.h (right): https://codereview.chromium.org/2760063002/diff/290001/components/previews/co... components/previews/core/previews_decider.h:23: // network quality before calling ShouldAllowPreview. |request| is the main On 2017/05/05 21:31:31, tbansal1 wrote: > s/ShouldAllowPreview/ShouldAllowPreviewAtECT/ Done. https://codereview.chromium.org/2760063002/diff/290001/components/previews/co... components/previews/core/previews_decider.h:25: // If the current ECT better than |effective_connection_type_threshold|, the On 2017/05/05 21:31:31, tbansal1 wrote: > s/better/strictly faster/ > OR s/better/at least as fast as/ Done.
still lgtm. Thanks for the changes.
c/b/l LGTM
Description was changed from ========== Add support to previews/ for Server LoFi and LitePages This adds support for using the previews blacklist for LoFi and LitePages as well as adding histograms for LoFi and LitePages within the previews framework. BUG=703293 ========== to ========== Add support to previews/ for Server LoFi and LitePages This adds support for using the previews blacklist for LoFi and LitePages as well as adding histograms for LoFi and LitePages within the previews framework. BUG=703293 TBR=jianli ==========
offline_pages lgtm
Description was changed from ========== Add support to previews/ for Server LoFi and LitePages This adds support for using the previews blacklist for LoFi and LitePages as well as adding histograms for LoFi and LitePages within the previews framework. BUG=703293 TBR=jianli ========== to ========== Add support to previews/ for Server LoFi and LitePages This adds support for using the previews blacklist for LoFi and LitePages as well as adding histograms for LoFi and LitePages within the previews framework. BUG=703293 ==========
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 holte@chromium.org Link to the patchset: https://codereview.chromium.org/2760063002/#ps330001 (title: "comment fix")
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": 330001, "attempt_start_ts": 1494026832702540,
"parent_rev": "a4449725c3c1d07ecd8008d5d03227c4588b85dc", "commit_rev":
"b24d2336dc516900281fc2034adaddc29769e3a5"}
CQ is committing da patch.
Bot data: {"patchset_id": 330001, "attempt_start_ts": 1494026832702540,
"parent_rev": "a4449725c3c1d07ecd8008d5d03227c4588b85dc", "commit_rev":
"b24d2336dc516900281fc2034adaddc29769e3a5"}
CQ is committing da patch.
Bot data: {"patchset_id": 330001, "attempt_start_ts": 1494026832702540,
"parent_rev": "a4449725c3c1d07ecd8008d5d03227c4588b85dc", "commit_rev":
"b24d2336dc516900281fc2034adaddc29769e3a5"}
Message was sent while issue was closed.
Description was changed from ========== Add support to previews/ for Server LoFi and LitePages This adds support for using the previews blacklist for LoFi and LitePages as well as adding histograms for LoFi and LitePages within the previews framework. BUG=703293 ========== to ========== Add support to previews/ for Server LoFi and LitePages This adds support for using the previews blacklist for LoFi and LitePages as well as adding histograms for LoFi and LitePages within the previews framework. BUG=703293 Review-Url: https://codereview.chromium.org/2760063002 Cr-Commit-Position: refs/heads/master@{#469800} Committed: https://chromium.googlesource.com/chromium/src/+/b24d2336dc516900281fc2034ada... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:330001) as https://chromium.googlesource.com/chromium/src/+/b24d2336dc516900281fc2034ada... |
