Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2369673004:
Wire NQE Prefs to Profile (Closed)
DescriptionWire Network Quality Estimator (NQE) Prefs to Profile
With this CL, prefs containing the network quality would be
written to the disk if |enable_prefs| is set to true in the
field trial config params.
The prefs are written by PrefDelegateImpl which is
profile-specific.
Additionally, prefs would be read at the time of startup,
and read prefs would be parsed and notified to
NetworkQualityEstimator.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet
BUG=490870
Committed: https://crrev.com/0d42d6c0fa774d6682b6ca90edc90b14cb94de3d
Cr-Commit-Position: refs/heads/master@{#426802}
Patch Set 1 : ps #
Total comments: 12
Patch Set 2 : Added functionality for reading and writing multiple network IDs, Rebased, Addressed Ryan's comments #
Total comments: 10
Patch Set 3 : Addressed ryansturm comments #
Total comments: 12
Patch Set 4 : rebased #Patch Set 5 : Addressed ryansturm comments #
Total comments: 26
Patch Set 6 : Addressed bengr comments #
Total comments: 10
Patch Set 7 : Addressed bengr, mmenke comments #Patch Set 8 : Rebased #Patch Set 9 : Fixed the errors #Patch Set 10 : More fixups #Messages
Total messages: 191 (168 generated)
The CQ bit was checked by tbansal@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_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 tbansal@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 tbansal@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 #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_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 tbansal@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 #1 (id:40001) has been deleted
Description was changed from ========== wire nqe prefs to profile BUG= ========== to ========== Wire nqe prefs to profile With this CL, prefs containing the network quality would be written to the disk if |enable_prefs| is set to true in the field trial config params. BUG= ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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_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 tbansal@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...) 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 tbansal@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 #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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 tbansal@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 #1 (id:100001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Patchset #2 (id:140001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Patchset #1 (id:120001) 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: 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 tbansal@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 #1 (id:160001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Patchset #1 (id:180001) 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 tbansal@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 tbansal@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 tbansal@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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by tbansal@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 tbansal@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: 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 tbansal@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_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Patchset #1 (id:200001) has been deleted
Patchset #1 (id:220001) has been deleted
Patchset #1 (id:240001) has been deleted
Patchset #1 (id:260001) has been deleted
Patchset #1 (id:280001) has been deleted
Patchset #1 (id:300001) has been deleted
Patchset #1 (id:320001) has been deleted
The CQ bit was checked by tbansal@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 tbansal@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 #1 (id:340001) has been deleted
Patchset #1 (id:360001) has been deleted
Patchset #1 (id:380001) has been deleted
Patchset #1 (id:400001) has been deleted
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 tbansal@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 #2 (id:440001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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 tbansal@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 #2 (id:460001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:420001) has been deleted
Description was changed from ========== Wire nqe prefs to profile With this CL, prefs containing the network quality would be written to the disk if |enable_prefs| is set to true in the field trial config params. BUG= ========== to ========== Wire nqe prefs to profile With this CL, prefs containing the network quality would be written to the disk if |enable_prefs| is set to true in the field trial config params. BUG=490870 ==========
The CQ bit was checked by tbansal@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.
Patchset #1 (id:480001) has been deleted
Patchset #1 (id:500001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Patchset #1 (id:520001) 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...
Description was changed from ========== Wire nqe prefs to profile With this CL, prefs containing the network quality would be written to the disk if |enable_prefs| is set to true in the field trial config params. BUG=490870 ========== to ========== Wire Network Quality Estimator (NQE) Prefs to Profile With this CL, prefs containing the network quality would be written to the disk if |enable_prefs| is set to true in the field trial config params. The prefs are written by PrefDelegateImpl which is profile-specific. BUG=490870 ==========
tbansal@chromium.org changed reviewers: + ryansturm@chromium.org
ryansturm: ptal. thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I need more time to look at this to make sure its doing what I think it's doing, but most of it looks good. https://codereview.chromium.org/2369673004/diff/540001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service_factory.cc (right): https://codereview.chromium.org/2369673004/diff/540001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service_factory.cc:37: return new UINetworkQualityEstimatorService(static_cast<Profile*>(context)); Profile::FromBrowserContext instead of static_cast https://codereview.chromium.org/2369673004/diff/540001/chrome/browser/profile... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/2369673004/diff/540001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.cc:218: UINetworkQualityEstimatorServiceFactory::GetForProfile(profile_); Comment about this initializes the Service
https://codereview.chromium.org/2369673004/diff/540001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service.cc (right): https://codereview.chromium.org/2369673004/diff/540001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:65: pref_service_->Set(path_, value); Won't this overwrite the previous value? Is this only supposed to store one cachedNetworkId at a time? pref_service_->GetDictionary(path_)->MergeDictionary(&value); https://codereview.chromium.org/2369673004/diff/540001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:184: DCHECK(content::BrowserThread::DeleteSoon(content::BrowserThread::IO, When this got moved to Shutdown, we should have invalidated the pointers. Please set io_observer_ and prefs_manager_ to nullptr. https://codereview.chromium.org/2369673004/diff/540001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service_browsertest.cc (right): https://codereview.chromium.org/2369673004/diff/540001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service_browsertest.cc:106: histogram_tester.GetAllSamples("NQE.Prefs.WriteCount").empty()); Please test that multiple network ids can be written to prefs. Please check that the values are all still in the prefs as well. This only checks that they were written. https://codereview.chromium.org/2369673004/diff/540001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2369673004/diff/540001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:38059: +<histogram name="NQE.Prefs.WriteCount" units="coount"> s/coount/count
Patchset #2 (id:560001) has been deleted
The CQ bit was checked by tbansal@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: ptal. Thanks. https://codereview.chromium.org/2369673004/diff/540001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service.cc (right): https://codereview.chromium.org/2369673004/diff/540001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:65: pref_service_->Set(path_, value); On 2016/10/05 20:59:42, Ryan Sturm wrote: > Won't this overwrite the previous value? Is this only supposed to store one > cachedNetworkId at a time? > > pref_service_->GetDictionary(path_)->MergeDictionary(&value); Done. I was planning to do multiple values in the next CL, but now this CL does it. https://codereview.chromium.org/2369673004/diff/540001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:184: DCHECK(content::BrowserThread::DeleteSoon(content::BrowserThread::IO, On 2016/10/05 20:59:42, Ryan Sturm wrote: > When this got moved to Shutdown, we should have invalidated the pointers. Please > set io_observer_ and prefs_manager_ to nullptr. Done. https://codereview.chromium.org/2369673004/diff/540001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service_browsertest.cc (right): https://codereview.chromium.org/2369673004/diff/540001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service_browsertest.cc:106: histogram_tester.GetAllSamples("NQE.Prefs.WriteCount").empty()); On 2016/10/05 20:59:42, Ryan Sturm wrote: > Please test that multiple network ids can be written to prefs. Please check that > the values are all still in the prefs as well. This only checks that they were > written. Done. I was planning to add Read() in a separate CL, but now I have moved those changes in this CL to avoid confusion. https://codereview.chromium.org/2369673004/diff/540001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service_factory.cc (right): https://codereview.chromium.org/2369673004/diff/540001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service_factory.cc:37: return new UINetworkQualityEstimatorService(static_cast<Profile*>(context)); On 2016/10/04 22:08:00, Ryan Sturm wrote: > Profile::FromBrowserContext instead of static_cast Done. https://codereview.chromium.org/2369673004/diff/540001/chrome/browser/profile... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/2369673004/diff/540001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.cc:218: UINetworkQualityEstimatorServiceFactory::GetForProfile(profile_); On 2016/10/04 22:08:00, Ryan Sturm wrote: > Comment about this initializes the Service Done. https://codereview.chromium.org/2369673004/diff/540001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2369673004/diff/540001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:38059: +<histogram name="NQE.Prefs.WriteCount" units="coount"> On 2016/10/05 20:59:42, Ryan Sturm wrote: > s/coount/count Done.
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...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Description was changed from ========== Wire Network Quality Estimator (NQE) Prefs to Profile With this CL, prefs containing the network quality would be written to the disk if |enable_prefs| is set to true in the field trial config params. The prefs are written by PrefDelegateImpl which is profile-specific. BUG=490870 ========== to ========== Wire Network Quality Estimator (NQE) Prefs to Profile With this CL, prefs containing the network quality would be written to the disk if |enable_prefs| is set to true in the field trial config params. The prefs are written by PrefDelegateImpl which is profile-specific. Additionally, prefs would be read at the time of startup, and read prefs would be parsed and notified to NetworkQualityEstimator. BUG=490870 ==========
The CQ bit was checked by tbansal@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.
I need to look again later, but here are some comments from a first pass. https://codereview.chromium.org/2369673004/diff/580001/net/base/network_chang... File net/base/network_change_notifier.cc (right): https://codereview.chromium.org/2369673004/diff/580001/net/base/network_chang... net/base/network_change_notifier.cc:697: kConnectionTypeNames, Can this be more C++11-ish and have std::find(std::begin(kConnectionTypeNames), std::end(kConnectionTypeNames), connection_type) and it != std::end(kConnectionTypeNames) below. https://codereview.chromium.org/2369673004/diff/580001/net/base/network_chang... net/base/network_change_notifier.cc:700: ? static_cast<NetworkChangeNotifier::ConnectionType>( I'm not convinced this static_cast is a good way to do this, but the other option is doing a switch/case and I am not totally sold that that is better. I'm worried about someone re-ordering the enum or the array without re-ordering the other. Use your judgement on which approach you want to go with, but I'd prefer std::distance(std::begin(kConnectionTypeNames), it) instead of plain subtraction if you stick with the current approach. It will do the same thing, but might be more clear to read. https://codereview.chromium.org/2369673004/diff/580001/net/base/network_chang... File net/base/network_change_notifier.h (right): https://codereview.chromium.org/2369673004/diff/580001/net/base/network_chang... net/base/network_change_notifier.h:410: // Parses |type| and return the equivalent ConnectionType. s/type/connection_type/ https://codereview.chromium.org/2369673004/diff/580001/net/nqe/network_id.h File net/nqe/network_id.h (right): https://codereview.chromium.org/2369673004/diff/580001/net/nqe/network_id.h#n... net/nqe/network_id.h:50: return type != other.type || id != other.id; maybe something like this: return !operator==(other); https://codereview.chromium.org/2369673004/diff/580001/net/nqe/network_qualit... File net/nqe/network_qualities_prefs_manager.h (right): https://codereview.chromium.org/2369673004/diff/580001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.h:50: // Sets the persistent pref on the disk to the given value. nit: "on the disk" probably isn't necessary, I think prefs are supposed to abstract away the concept of disk, so saying "persistent pref" should be enough. Same below if you decide to change it.
The CQ bit was checked by tbansal@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: ptal. Thanks. https://codereview.chromium.org/2369673004/diff/580001/net/base/network_chang... File net/base/network_change_notifier.cc (right): https://codereview.chromium.org/2369673004/diff/580001/net/base/network_chang... net/base/network_change_notifier.cc:697: kConnectionTypeNames, On 2016/10/12 21:33:34, Ryan Sturm wrote: > Can this be more C++11-ish and have std::find(std::begin(kConnectionTypeNames), > std::end(kConnectionTypeNames), connection_type) > > and > > it != std::end(kConnectionTypeNames) below. Obsolete. https://codereview.chromium.org/2369673004/diff/580001/net/base/network_chang... net/base/network_change_notifier.cc:700: ? static_cast<NetworkChangeNotifier::ConnectionType>( On 2016/10/12 21:33:34, Ryan Sturm wrote: > I'm not convinced this static_cast is a good way to do this, but the other > option is doing a switch/case and I am not totally sold that that is better. I'm > worried about someone re-ordering the enum or the array without re-ordering the > other. From https://cs.chromium.org/chromium/src/net/base/network_change_notifier.h?rcl=0...: "New enum values should only be added to the end of the enum and no values should be modified or reused, as this is reported via UMA." > > Use your judgement on which approach you want to go with, but I'd prefer > std::distance(std::begin(kConnectionTypeNames), it) instead of plain subtraction > if you stick with the current approach. It will do the same thing, but might be > more clear to read. Obsolete. https://codereview.chromium.org/2369673004/diff/580001/net/base/network_chang... File net/base/network_change_notifier.h (right): https://codereview.chromium.org/2369673004/diff/580001/net/base/network_chang... net/base/network_change_notifier.h:410: // Parses |type| and return the equivalent ConnectionType. On 2016/10/12 21:33:34, Ryan Sturm wrote: > s/type/connection_type/ Obsolete. https://codereview.chromium.org/2369673004/diff/580001/net/nqe/network_id.h File net/nqe/network_id.h (right): https://codereview.chromium.org/2369673004/diff/580001/net/nqe/network_id.h#n... net/nqe/network_id.h:50: return type != other.type || id != other.id; On 2016/10/12 21:33:34, Ryan Sturm wrote: > maybe something like this: > > return !operator==(other); Done. https://codereview.chromium.org/2369673004/diff/580001/net/nqe/network_qualit... File net/nqe/network_qualities_prefs_manager.h (right): https://codereview.chromium.org/2369673004/diff/580001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.h:50: // Sets the persistent pref on the disk to the given value. On 2016/10/12 21:33:34, Ryan Sturm wrote: > nit: "on the disk" probably isn't necessary, I think prefs are supposed to > abstract away the concept of disk, so saying "persistent pref" should be enough. > Same below if you decide to change it. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tbansal@chromium.org changed reviewers: + bengr@chromium.org
lgtm % nits https://codereview.chromium.org/2369673004/diff/600001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service.cc (right): https://codereview.chromium.org/2369673004/diff/600001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:11: #include "base/memory/ptr_util.h" is this used? https://codereview.chromium.org/2369673004/diff/600001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service.h (right): https://codereview.chromium.org/2369673004/diff/600001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.h:10: #include "base/values.h" Where is this used? https://codereview.chromium.org/2369673004/diff/600001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.h:44: std::map<net::nqe::internal::NetworkID, move #include <map> from cc to h https://codereview.chromium.org/2369673004/diff/600001/net/nqe/network_qualit... File net/nqe/network_qualities_prefs_manager.cc (right): https://codereview.chromium.org/2369673004/diff/600001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.cc:117: // If the network ID contains a period, then return since the dictionary prefs I don't think UTF-8 actually fixes this because '.' is still '.' in UTF-8. You could do some similar stuff as UTF-8 where you put all periods as ",p" and all "," as ",,". Or something similar, but that should stay as another CL. https://codereview.chromium.org/2369673004/diff/600001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.cc:129: prefs_->MergeDictionary(&dictionary_value); Since you are doing this in the same place as creating the new DictionaryValue now, it might be easier to just call prefs_->Set(network_id_string,...); Instead of merging. https://codereview.chromium.org/2369673004/diff/600001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.h (right): https://codereview.chromium.org/2369673004/diff/600001/net/nqe/network_qualit... net/nqe/network_quality_estimator.h:244: // Notifies |this| when the persistent prefs have been read. |read_prefs| nit: s/Notifies |this| when/Called when
The CQ bit was checked by tbansal@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...
Description was changed from ========== Wire Network Quality Estimator (NQE) Prefs to Profile With this CL, prefs containing the network quality would be written to the disk if |enable_prefs| is set to true in the field trial config params. The prefs are written by PrefDelegateImpl which is profile-specific. Additionally, prefs would be read at the time of startup, and read prefs would be parsed and notified to NetworkQualityEstimator. BUG=490870 ========== to ========== Wire Network Quality Estimator (NQE) Prefs to Profile With this CL, prefs containing the network quality would be written to the disk if |enable_prefs| is set to true in the field trial config params. The prefs are written by PrefDelegateImpl which is profile-specific. Additionally, prefs would be read at the time of startup, and read prefs would be parsed and notified to NetworkQualityEstimator. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet BUG=490870 ==========
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Patchset #5 (id:640001) 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 tbansal@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...
bengr: ptal. thanks. https://codereview.chromium.org/2369673004/diff/600001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service.cc (right): https://codereview.chromium.org/2369673004/diff/600001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:11: #include "base/memory/ptr_util.h" On 2016/10/14 18:36:20, Ryan Sturm wrote: > is this used? Done. https://codereview.chromium.org/2369673004/diff/600001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service.h (right): https://codereview.chromium.org/2369673004/diff/600001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.h:10: #include "base/values.h" On 2016/10/14 18:36:20, Ryan Sturm wrote: > Where is this used? Done. https://codereview.chromium.org/2369673004/diff/600001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.h:44: std::map<net::nqe::internal::NetworkID, On 2016/10/14 18:36:20, Ryan Sturm wrote: > move #include <map> from cc to h Done. https://codereview.chromium.org/2369673004/diff/600001/net/nqe/network_qualit... File net/nqe/network_qualities_prefs_manager.cc (right): https://codereview.chromium.org/2369673004/diff/600001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.cc:117: // If the network ID contains a period, then return since the dictionary prefs On 2016/10/14 18:36:20, Ryan Sturm wrote: > I don't think UTF-8 actually fixes this because '.' is still '.' in UTF-8. You > could do some similar stuff as UTF-8 where you put all periods as ",p" and all > "," as ",,". Or something similar, but that should stay as another CL. Acknowledged. https://codereview.chromium.org/2369673004/diff/600001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.cc:129: prefs_->MergeDictionary(&dictionary_value); On 2016/10/14 18:36:21, Ryan Sturm wrote: > Since you are doing this in the same place as creating the new DictionaryValue > now, it might be easier to just call > > prefs_->Set(network_id_string,...); > > Instead of merging. Done. https://codereview.chromium.org/2369673004/diff/600001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.h (right): https://codereview.chromium.org/2369673004/diff/600001/net/nqe/network_qualit... net/nqe/network_quality_estimator.h:244: // Notifies |this| when the persistent prefs have been read. |read_prefs| On 2016/10/14 18:36:21, Ryan Sturm wrote: > nit: s/Notifies |this| when/Called when Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2369673004/diff/660001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service.cc (right): https://codereview.chromium.org/2369673004/diff/660001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:193: DCHECK(content::BrowserThread::DeleteSoon(content::BrowserThread::IO, Why is this in a DCHECK? Do you not want this to happen in release builds? https://codereview.chromium.org/2369673004/diff/660001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:199: DCHECK(content::BrowserThread::DeleteSoon(content::BrowserThread::IO, Why is this in a DCHECK? Do you not want this to happen in release builds? https://codereview.chromium.org/2369673004/diff/660001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:224: void UINetworkQualityEstimatorService::RegisterProfilePrefs( I don't like the name of this class. UI... ick. https://codereview.chromium.org/2369673004/diff/660001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service_browsertest.cc (right): https://codereview.chromium.org/2369673004/diff/660001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service_browsertest.cc:54: bool network_id_available = true; you could also do: bool network_id_available = !(net::NCN::GetConnectionType() == ...UNKNOWN || ...); https://codereview.chromium.org/2369673004/diff/660001/net/nqe/network_qualit... File net/nqe/network_qualities_prefs_manager.cc (right): https://codereview.chromium.org/2369673004/diff/660001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.cc:36: std::string effective_connection_type_string; #include <string> https://codereview.chromium.org/2369673004/diff/660001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.cc:117: // If the network ID contains a period, then return since the dictionary prefs return -> return early https://codereview.chromium.org/2369673004/diff/660001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.cc:119: // TODO(tbansal): Investigate a better way of storing network IDs with period Why can't you just escape the period? https://codereview.chromium.org/2369673004/diff/660001/net/nqe/network_qualit... File net/nqe/network_qualities_prefs_manager.h (right): https://codereview.chromium.org/2369673004/diff/660001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.h:73: std::map<nqe::internal::NetworkID, nqe::internal::CachedNetworkQuality> It might be a little more readable to typedef this map. https://codereview.chromium.org/2369673004/diff/660001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2369673004/diff/660001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:38456: + Number of times network quality prefs were read by the network quality times -> times the https://codereview.chromium.org/2369673004/diff/660001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:38465: + Count of number of network IDs in the prefs read by the network quality Count of -> Count of the https://codereview.chromium.org/2369673004/diff/660001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:38466: + estimator at the time of start up. What does "at the time of start up" mean? https://codereview.chromium.org/2369673004/diff/660001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:38474: + Number of times network quality prefs were written by the network quality times -> times the
https://codereview.chromium.org/2369673004/diff/660001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service.cc (right): https://codereview.chromium.org/2369673004/diff/660001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:193: DCHECK(content::BrowserThread::DeleteSoon(content::BrowserThread::IO, On 2016/10/14 22:37:12, bengr wrote: > Why is this in a DCHECK? Do you not want this to happen in release builds? Should we put io_observer_ and prefs_manager_ in std::unique_ptr and call release inside of DeleteSoon and stop doing x = nullptr; We keep accidentally leaking these. I think I would rather crash than leak if we mess this up.
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
ptal. thanks. https://codereview.chromium.org/2369673004/diff/660001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service.cc (right): https://codereview.chromium.org/2369673004/diff/660001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:193: DCHECK(content::BrowserThread::DeleteSoon(content::BrowserThread::IO, On 2016/10/14 22:37:12, bengr wrote: > Why is this in a DCHECK? Do you not want this to happen in release builds? Done. https://codereview.chromium.org/2369673004/diff/660001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:193: DCHECK(content::BrowserThread::DeleteSoon(content::BrowserThread::IO, On 2016/10/14 22:52:03, Ryan Sturm wrote: > On 2016/10/14 22:37:12, bengr wrote: > > Why is this in a DCHECK? Do you not want this to happen in release builds? > > Should we put io_observer_ and prefs_manager_ in std::unique_ptr and call > release inside of DeleteSoon and stop doing x = nullptr; We keep accidentally > leaking these. I think I would rather crash than leak if we mess this up. Done. https://codereview.chromium.org/2369673004/diff/660001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:199: DCHECK(content::BrowserThread::DeleteSoon(content::BrowserThread::IO, On 2016/10/14 22:37:13, bengr wrote: > Why is this in a DCHECK? Do you not want this to happen in release builds? Done. https://codereview.chromium.org/2369673004/diff/660001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:224: void UINetworkQualityEstimatorService::RegisterProfilePrefs( On 2016/10/14 22:37:12, bengr wrote: > I don't like the name of this class. UI... ick. May be we can change it in a separate CL. https://codereview.chromium.org/2369673004/diff/660001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service_browsertest.cc (right): https://codereview.chromium.org/2369673004/diff/660001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service_browsertest.cc:54: bool network_id_available = true; On 2016/10/14 22:37:13, bengr wrote: > you could also do: > > bool network_id_available = > !(net::NCN::GetConnectionType() == ...UNKNOWN || ...); I was not sure which one is more readable. The current one looked more readable to me, but I can change if you feel strongly about it. https://codereview.chromium.org/2369673004/diff/660001/net/nqe/network_qualit... File net/nqe/network_qualities_prefs_manager.cc (right): https://codereview.chromium.org/2369673004/diff/660001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.cc:36: std::string effective_connection_type_string; On 2016/10/14 22:37:13, bengr wrote: > #include <string> Done. https://codereview.chromium.org/2369673004/diff/660001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.cc:117: // If the network ID contains a period, then return since the dictionary prefs On 2016/10/14 22:37:13, bengr wrote: > return -> return early Done. https://codereview.chromium.org/2369673004/diff/660001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.cc:119: // TODO(tbansal): Investigate a better way of storing network IDs with period On 2016/10/14 22:37:13, bengr wrote: > Why can't you just escape the period? I do not think Dictionary code in base/values.h respects escape characters. It scans for '.' character in the path, and then creates a nested dictionary based on that. https://codereview.chromium.org/2369673004/diff/660001/net/nqe/network_qualit... File net/nqe/network_qualities_prefs_manager.h (right): https://codereview.chromium.org/2369673004/diff/660001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.h:73: std::map<nqe::internal::NetworkID, nqe::internal::CachedNetworkQuality> On 2016/10/14 22:37:13, bengr wrote: > It might be a little more readable to typedef this map. Done. https://codereview.chromium.org/2369673004/diff/660001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2369673004/diff/660001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:38456: + Number of times network quality prefs were read by the network quality On 2016/10/14 22:37:13, bengr wrote: > times -> times the Done. https://codereview.chromium.org/2369673004/diff/660001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:38465: + Count of number of network IDs in the prefs read by the network quality On 2016/10/14 22:37:13, bengr wrote: > Count of -> Count of the Done. https://codereview.chromium.org/2369673004/diff/660001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:38466: + estimator at the time of start up. On 2016/10/14 22:37:13, bengr wrote: > What does "at the time of start up" mean? Done. https://codereview.chromium.org/2369673004/diff/660001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:38474: + Number of times network quality prefs were written by the network quality On 2016/10/14 22:37:13, bengr wrote: > times -> times the 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: 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 tbansal@chromium.org to run a CQ dry run
Patchset #6 (id:680001) 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 #7 (id:630018) has been deleted
bengr: ping....
lgtm with nits. https://codereview.chromium.org/2369673004/diff/700001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service.cc (right): https://codereview.chromium.org/2369673004/diff/700001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:28: const char kNetworkQualityEstimatorFieldTrialName[] = "NetworkQualityEstimator"; This is only used on line 36, so you might consider using the string literal directly there. https://codereview.chromium.org/2369673004/diff/700001/net/nqe/network_qualit... File net/nqe/network_qualities_prefs_manager.cc (right): https://codereview.chromium.org/2369673004/diff/700001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.cc:118: // TODO(tbansal): Investigate a better way of storing network IDs with period Reference a bug. https://codereview.chromium.org/2369673004/diff/700001/net/nqe/network_qualit... File net/nqe/network_qualities_prefs_manager_unittest.cc (right): https://codereview.chromium.org/2369673004/diff/700001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager_unittest.cc:26: MockPrefDelegate() I'd call this "TestPrefDelegate" because it isn't a mock, but feel free to leave as is. https://codereview.chromium.org/2369673004/diff/700001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2369673004/diff/700001/net/nqe/network_qualit... net/nqe/network_quality_estimator.cc:1801: // TODO(tbansal): Incorporate the network quality into the current estimates. Reference a bug.
tbansal@chromium.org changed reviewers: + gab@chromium.org, mmenke@chromium.org, rkaplow@chromium.org
mmenke: ptal at profile_impl_io_data.cc gab: chrome/browser/prefs/browser_prefs.cc rkaplow: histograms.xml Thanks!
https://codereview.chromium.org/2369673004/diff/700001/chrome/browser/profile... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/2369673004/diff/700001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.cc:219: UINetworkQualityEstimatorServiceFactory::GetForProfile(profile_); Should this be in ChromeBrowserMainExtraPartsProfiles::EnsureBrowserContextKeyedServiceFactoriesBuilt instead, and give the ServiceFactory a ServiceIsCreatedWithBrowserContext() method that returns true?
Patchset #7 (id:730001) has been deleted
tbansal@chromium.org changed reviewers: - mmenke@chromium.org
gab: PTAL at chrome/browser/prefs/browser_prefs.cc rkaplow: histograms.xml Moved mmenke to CC list since the CL no longer affects profile_impl_io_data.cc. https://codereview.chromium.org/2369673004/diff/700001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service.cc (right): https://codereview.chromium.org/2369673004/diff/700001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:28: const char kNetworkQualityEstimatorFieldTrialName[] = "NetworkQualityEstimator"; On 2016/10/18 21:47:36, bengr wrote: > This is only used on line 36, so you might consider using the string literal > directly there. Done. https://codereview.chromium.org/2369673004/diff/700001/chrome/browser/profile... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/2369673004/diff/700001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.cc:219: UINetworkQualityEstimatorServiceFactory::GetForProfile(profile_); On 2016/10/19 17:24:57, mmenke wrote: > Should this be in > ChromeBrowserMainExtraPartsProfiles::EnsureBrowserContextKeyedServiceFactoriesBuilt > instead, and give the ServiceFactory a ServiceIsCreatedWithBrowserContext() > method that returns true? Done. I did not know about this. https://codereview.chromium.org/2369673004/diff/700001/net/nqe/network_qualit... File net/nqe/network_qualities_prefs_manager.cc (right): https://codereview.chromium.org/2369673004/diff/700001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.cc:118: // TODO(tbansal): Investigate a better way of storing network IDs with period On 2016/10/18 21:47:36, bengr wrote: > Reference a bug. I removed the TODO because I do not think this is going to be done in near-future. https://codereview.chromium.org/2369673004/diff/700001/net/nqe/network_qualit... File net/nqe/network_qualities_prefs_manager_unittest.cc (right): https://codereview.chromium.org/2369673004/diff/700001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager_unittest.cc:26: MockPrefDelegate() On 2016/10/18 21:47:36, bengr wrote: > I'd call this "TestPrefDelegate" because it isn't a mock, but feel free to leave > as is. Done. https://codereview.chromium.org/2369673004/diff/700001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2369673004/diff/700001/net/nqe/network_qualit... net/nqe/network_quality_estimator.cc:1801: // TODO(tbansal): Incorporate the network quality into the current estimates. On 2016/10/18 21:47:36, bengr wrote: > Reference a bug. Done.
The CQ bit was checked by tbansal@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 tbansal@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_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
lgtm histogram lg
Patchset #10 (id:810001) has been deleted
The CQ bit was checked by tbansal@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...
There were warnings when CQ was processing your CL: * CQ is not running the android_cronet trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the android_cronet trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the android_cronet trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the android_cronet trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
gab: ptal at chrome/browser/prefs/browser_prefs.cc. Thanks.
On 2016/10/20 20:56:49, tbansal1 wrote: > gab: ptal at chrome/browser/prefs/browser_prefs.cc. Thanks. lgtm
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ryansturm@chromium.org, bengr@chromium.org, rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/2369673004/#ps830001 (title: "More fixups")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
There were warnings when CQ was processing your CL: * CQ is not running the android_cronet trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
Message was sent while issue was closed.
Description was changed from ========== Wire Network Quality Estimator (NQE) Prefs to Profile With this CL, prefs containing the network quality would be written to the disk if |enable_prefs| is set to true in the field trial config params. The prefs are written by PrefDelegateImpl which is profile-specific. Additionally, prefs would be read at the time of startup, and read prefs would be parsed and notified to NetworkQualityEstimator. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet BUG=490870 ========== to ========== Wire Network Quality Estimator (NQE) Prefs to Profile With this CL, prefs containing the network quality would be written to the disk if |enable_prefs| is set to true in the field trial config params. The prefs are written by PrefDelegateImpl which is profile-specific. Additionally, prefs would be read at the time of startup, and read prefs would be parsed and notified to NetworkQualityEstimator. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet BUG=490870 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:830001)
Message was sent while issue was closed.
Description was changed from ========== Wire Network Quality Estimator (NQE) Prefs to Profile With this CL, prefs containing the network quality would be written to the disk if |enable_prefs| is set to true in the field trial config params. The prefs are written by PrefDelegateImpl which is profile-specific. Additionally, prefs would be read at the time of startup, and read prefs would be parsed and notified to NetworkQualityEstimator. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet BUG=490870 ========== to ========== Wire Network Quality Estimator (NQE) Prefs to Profile With this CL, prefs containing the network quality would be written to the disk if |enable_prefs| is set to true in the field trial config params. The prefs are written by PrefDelegateImpl which is profile-specific. Additionally, prefs would be read at the time of startup, and read prefs would be parsed and notified to NetworkQualityEstimator. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet BUG=490870 Committed: https://crrev.com/0d42d6c0fa774d6682b6ca90edc90b14cb94de3d Cr-Commit-Position: refs/heads/master@{#426802} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/0d42d6c0fa774d6682b6ca90edc90b14cb94de3d Cr-Commit-Position: refs/heads/master@{#426802} |
