Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
473723002:
Update data reduction proxy statistics prefs less often on desktop (Closed)
DescriptionUpdate data reduction proxy statistics prefs less often on desktop
For platforms other than iOS and Android, the preference data is stored in
memory and written to disk at 60
minute intervals and on shutdown via the DataReductionProxyDelayedPrefService.
For iOS and Android, the DataReductionProxyDelayedPrefService takes a delay time
of zero and simply passes the prefs through to the PrefService.
BUG=399788
Committed: https://crrev.com/53f608b55172df7423ea54ab83e9fdfa8fc81f9a
Cr-Commit-Position: refs/heads/master@{#295521}
Patch Set 1 : #
Total comments: 50
Patch Set 2 : Addressing bengr comments #Patch Set 3 : Undo BUILD.gn and fix aw_browser_context #
Total comments: 62
Patch Set 4 : Addressed bengr comments #
Total comments: 36
Patch Set 5 : Addressed bengr comments #
Total comments: 3
Patch Set 6 : Addressed sgurun comment #
Total comments: 2
Patch Set 7 : Removing statistics prefs from Android WebView #
Total comments: 31
Patch Set 8 : Addressed bengr and mmenke comments #
Total comments: 2
Patch Set 9 : Addressed mmenke comments #Patch Set 10 : Fixing BUILD.gn and includes #
Total comments: 6
Patch Set 11 : Addressed bengr nits #
Total comments: 1
Patch Set 12 : Sync master and Moving statistics prefs write #Patch Set 13 : Fixing net internals bandwidth page #
Total comments: 23
Patch Set 14 : Addressed mmenke comments #Patch Set 15 : Fixing bandwidth test as discussed with mmenke #
Total comments: 3
Patch Set 16 : Fixing nits #Messages
Total messages: 59 (8 generated)
https://codereview.chromium.org/473723002/diff/20001/chrome/browser/net/chrom... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/473723002/diff/20001/chrome/browser/net/chrom... chrome/browser/net/chrome_network_delegate.cc:119: data_reduction_proxy::DataReductionProxyDelayedPrefService* pref_service) { DCHECK(pref_service) Also, after the class renaming, I would call the variable statistics_prefs. https://codereview.chromium.org/473723002/diff/20001/chrome/browser/net/chrom... chrome/browser/net/chrome_network_delegate.cc:128: if (!pref_service) Why would this ever be null? https://codereview.chromium.org/473723002/diff/20001/chrome/browser/net/chrom... chrome/browser/net/chrome_network_delegate.cc:330: // Until updated the pref values may be up to an hour behind. Please file a bug to fix this. https://codereview.chromium.org/473723002/diff/20001/chrome/browser/net/chrom... File chrome/browser/net/chrome_network_delegate.h (right): https://codereview.chromium.org/473723002/diff/20001/chrome/browser/net/chrom... chrome/browser/net/chrome_network_delegate.h:310: data_reduction_proxy_delayed_pref_service_; I would call this class DatareductionProxyStatisticsPrefs. Delay is an implementation detail. https://codereview.chromium.org/473723002/diff/20001/chrome/browser/profiles/... File chrome/browser/profiles/profile_impl.cc (right): https://codereview.chromium.org/473723002/diff/20001/chrome/browser/profiles/... chrome/browser/profiles/profile_impl.cc:647: #if defined(OS_ANDROID) || defined(OS_IOS) Rewrite as: #if defined(OS_ANDROID) || defined(OS_IOS) base::TimeDelta commit_delay = base::TimeDelta(); #else base::TimeDelta commit_delay = base::TimeDelta::FromMinutes(60) #endif data_reduction_proxy_pref_service.reset(..., commit_delay); https://codereview.chromium.org/473723002/diff/20001/chrome/browser/profiles/... File chrome/browser/profiles/profile_impl_io_data.h (right): https://codereview.chromium.org/473723002/diff/20001/chrome/browser/profiles/... chrome/browser/profiles/profile_impl_io_data.h:31: } // namespace data_reduction_proxy } // namespace... https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.cc (right): https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.cc:26: DCHECK(prefs); Also, DCHECK that delay is non-negative https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.cc:27: if (delay_ <= base::TimeDelta()) { Don't you mean >=?. Also, afaict, these don't need to be on the heap. https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.cc:38: if (!(delay_ <= base::TimeDelta())) { if (delay_ == base::TimeDelta()) return pref_service_->GetInt64(pref_path); https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.cc:45: pref_map_->insert(std::pair<const char*, int64>(pref_path, pref_value)); You can do pref_map_[pref_path] = pref_value, I think. https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.cc:53: if (!(delay_ <= base::TimeDelta())) { if (delay_ == base::TimeDelta()) ... https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.cc:60: DataReductionProxyPrefMap::iterator iter = pref_map_->find(pref_path); Can you factor out the initialization of the maps? Two ways to do this: 1) create an init function that takes a list of pref names and reads those prefs into the map. This is more efficient, but requires you to name all the prefs up front. 2) Write a function called MaybeAddPrefToMap() that reads the pref and writes to the map if necessary. https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.cc:63: pref_map_->insert(std::pair<const char*, int64>(pref_path, pref_value)); again, use [] https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.cc:79: base::ListValue* pref_value = pref_service_->GetList(pref_path)->DeepCopy(); This looks like a memory leak. Where do you deallocate this? Maybe use a scoped_ptr? https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.cc:88: for(DataReductionProxyPrefMap::iterator iter = pref_map_->begin(); for ( https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.cc:94: iter != list_pref_map_->end(); ++iter) { indent one more https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.cc:110: base::Unretained(this)), What happens after this is destroyed? I think you need to use a CancelableTaskTracker https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.cc:120: to_list->Set(i, new base::StringValue(base::Int64ToString(value))); you can combine this line with the previous. https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.h (right): https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.h:10: #include "base/memory/weak_ptr.h" #include "base/time/time.h" here and remove the forward declaration of TimeDelta below. https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.h:15: class ListValue; remove indent. https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.h:26: // variables are stored and should be read on the UI thread. should be stored and read... https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.h:32: DataReductionProxyListPrefMap; indent 4 https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.h:81: DataReductionProxyPrefMap* pref_map_; Why do these need to be pointers? https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_settings.h (right): https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_settings.h:314: base::WeakPtr<DataReductionProxyDelayedPrefService> delayed_prefs_; I think this object is owned by ProfileImplIOData, which outlives this keyed service, so just make this a raw pointer. Sorry that I previously suggested a WeakPtr.
https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.cc (right): https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.cc:110: base::Unretained(this)), On 2014/08/14 17:39:41, bengr1 wrote: > What happens after this is destroyed? I think you need to use a > CancelableTaskTracker Ignore that. I think you can just pass a WeakPtr instead of base::Unretained(this)
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/473723002/diff/20001/chrome/browser/net/chrom... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/473723002/diff/20001/chrome/browser/net/chrom... chrome/browser/net/chrome_network_delegate.cc:119: data_reduction_proxy::DataReductionProxyDelayedPrefService* pref_service) { On 2014/08/14 17:39:41, bengr1 wrote: > DCHECK(pref_service) > > Also, after the class renaming, I would call the variable statistics_prefs. Done. https://codereview.chromium.org/473723002/diff/20001/chrome/browser/net/chrom... chrome/browser/net/chrome_network_delegate.cc:128: if (!pref_service) On 2014/08/14 17:39:41, bengr1 wrote: > Why would this ever be null? Done. https://codereview.chromium.org/473723002/diff/20001/chrome/browser/net/chrom... chrome/browser/net/chrome_network_delegate.cc:330: // Until updated the pref values may be up to an hour behind. On 2014/08/14 17:39:41, bengr1 wrote: > Please file a bug to fix this. Done. https://codereview.chromium.org/473723002/diff/20001/chrome/browser/net/chrom... File chrome/browser/net/chrome_network_delegate.h (right): https://codereview.chromium.org/473723002/diff/20001/chrome/browser/net/chrom... chrome/browser/net/chrome_network_delegate.h:310: data_reduction_proxy_delayed_pref_service_; On 2014/08/14 17:39:41, bengr1 wrote: > I would call this class DatareductionProxyStatisticsPrefs. Delay is an > implementation detail. Done. https://codereview.chromium.org/473723002/diff/20001/chrome/browser/profiles/... File chrome/browser/profiles/profile_impl.cc (right): https://codereview.chromium.org/473723002/diff/20001/chrome/browser/profiles/... chrome/browser/profiles/profile_impl.cc:647: #if defined(OS_ANDROID) || defined(OS_IOS) On 2014/08/14 17:39:41, bengr1 wrote: > Rewrite as: > #if defined(OS_ANDROID) || defined(OS_IOS) > base::TimeDelta commit_delay = base::TimeDelta(); > #else > base::TimeDelta commit_delay = base::TimeDelta::FromMinutes(60) > #endif > > data_reduction_proxy_pref_service.reset(..., commit_delay); > Done. https://codereview.chromium.org/473723002/diff/20001/chrome/browser/profiles/... File chrome/browser/profiles/profile_impl_io_data.h (right): https://codereview.chromium.org/473723002/diff/20001/chrome/browser/profiles/... chrome/browser/profiles/profile_impl_io_data.h:31: } // namespace data_reduction_proxy On 2014/08/14 17:39:41, bengr1 wrote: > } // namespace... Done. https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.cc (right): https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.cc:26: DCHECK(prefs); On 2014/08/14 17:39:42, bengr1 wrote: > Also, DCHECK that delay is non-negative Done. https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.cc:27: if (delay_ <= base::TimeDelta()) { On 2014/08/14 17:39:42, bengr1 wrote: > Don't you mean >=?. Also, afaict, these don't need to be on the heap. Done. https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.cc:38: if (!(delay_ <= base::TimeDelta())) { On 2014/08/14 17:39:41, bengr1 wrote: > if (delay_ == base::TimeDelta()) > return pref_service_->GetInt64(pref_path); Done. https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.cc:45: pref_map_->insert(std::pair<const char*, int64>(pref_path, pref_value)); On 2014/08/14 17:39:42, bengr1 wrote: > You can do pref_map_[pref_path] = pref_value, I think. Done. https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.cc:53: if (!(delay_ <= base::TimeDelta())) { On 2014/08/14 17:39:41, bengr1 wrote: > if (delay_ == base::TimeDelta()) > ... Done. https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.cc:60: DataReductionProxyPrefMap::iterator iter = pref_map_->find(pref_path); On 2014/08/14 17:39:42, bengr1 wrote: > Can you factor out the initialization of the maps? Two ways to do this: > 1) create an init function that takes a list of pref names and reads those prefs > into the map. This is more efficient, but requires you to name all the prefs up > front. > 2) Write a function called MaybeAddPrefToMap() that reads the pref and writes to > the map if necessary. Done. https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.cc:63: pref_map_->insert(std::pair<const char*, int64>(pref_path, pref_value)); On 2014/08/14 17:39:41, bengr1 wrote: > again, use [] Done. https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.cc:79: base::ListValue* pref_value = pref_service_->GetList(pref_path)->DeepCopy(); On 2014/08/14 17:39:41, bengr1 wrote: > This looks like a memory leak. Where do you deallocate this? Maybe use a > scoped_ptr? Using a ScopedPtrHashMap https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.cc:79: base::ListValue* pref_value = pref_service_->GetList(pref_path)->DeepCopy(); On 2014/08/14 17:39:41, bengr1 wrote: > This looks like a memory leak. Where do you deallocate this? Maybe use a > scoped_ptr? Done. https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.cc:88: for(DataReductionProxyPrefMap::iterator iter = pref_map_->begin(); On 2014/08/14 17:39:41, bengr1 wrote: > for ( Done. https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.cc:94: iter != list_pref_map_->end(); ++iter) { On 2014/08/14 17:39:41, bengr1 wrote: > indent one more Done. https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.cc:110: base::Unretained(this)), On 2014/08/14 23:09:49, bengr1 wrote: > On 2014/08/14 17:39:41, bengr1 wrote: > > What happens after this is destroyed? I think you need to use a > > CancelableTaskTracker > > Ignore that. I think you can just pass a WeakPtr instead of > base::Unretained(this) Done. https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.cc:120: to_list->Set(i, new base::StringValue(base::Int64ToString(value))); On 2014/08/14 17:39:41, bengr1 wrote: > you can combine this line with the previous. Done. https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.h (right): https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.h:10: #include "base/memory/weak_ptr.h" On 2014/08/14 17:39:42, bengr1 wrote: > #include "base/time/time.h" here and remove the forward declaration of TimeDelta > below. Done. https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.h:15: class ListValue; On 2014/08/14 17:39:42, bengr1 wrote: > remove indent. Done. https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.h:26: // variables are stored and should be read on the UI thread. On 2014/08/14 17:39:42, bengr1 wrote: > should be stored and read... Done. https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.h:32: DataReductionProxyListPrefMap; On 2014/08/14 17:39:42, bengr1 wrote: > indent 4 Done. https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_delayed_pref_service.h:81: DataReductionProxyPrefMap* pref_map_; On 2014/08/14 17:39:42, bengr1 wrote: > Why do these need to be pointers? Done. https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_settings.h (right): https://codereview.chromium.org/473723002/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_settings.h:314: base::WeakPtr<DataReductionProxyDelayedPrefService> delayed_prefs_; On 2014/08/14 17:39:42, bengr1 wrote: > I think this object is owned by ProfileImplIOData, which outlives this keyed > service, so just make this a raw pointer. Sorry that I previously suggested a > WeakPtr. Done.
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
https://codereview.chromium.org/473723002/diff/120001/chrome/browser/net/chro... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/473723002/diff/120001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate.cc:158: data_reduction_proxy::DataReductionProxyStatisticsPrefs* pref_service) { Maybe statistics_prefs? https://codereview.chromium.org/473723002/diff/120001/chrome/browser/net/chro... File chrome/browser/net/chrome_network_delegate.h (right): https://codereview.chromium.org/473723002/diff/120001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate.h:43: } } // namespace data_reduction_proxy https://codereview.chromium.org/473723002/diff/120001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate.h:171: data_reduction_proxy::DataReductionProxyStatisticsPrefs* service) { Rename as statistics_prefs https://codereview.chromium.org/473723002/diff/120001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate.h:329: data_reduction_proxy_statistics_prefs_; indent 4 https://codereview.chromium.org/473723002/diff/120001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl.cc (right): https://codereview.chromium.org/473723002/diff/120001/chrome/browser/profiles... chrome/browser/profiles/profile_impl.cc:689: #if defined(OS_ANDROID) || defined(OS_IOS) Add a comment explaining the platform specialization. https://codereview.chromium.org/473723002/diff/120001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/473723002/diff/120001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:127: const base::FilePath& cookie_path, Fix indent down to lone 147 https://codereview.chromium.org/473723002/diff/120001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.h (right): https://codereview.chromium.org/473723002/diff/120001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.h:74: session_cookie_mode, move up with previous line https://codereview.chromium.org/473723002/diff/120001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.h:79: data_reduction_proxy_unavailable, move up with previous line https://codereview.chromium.org/473723002/diff/120001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.h:81: data_reduction_proxy_chrome_configurator, Fix indent https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_settings.h (right): https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_settings.h:120: // Sets the |statistics_prefs_| weak pointer to be used for data reduction This isn't a weak pointer https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_settings_test_utils.cc (right): https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_settings_test_utils.cc:88: statistics_prefs_ = new DataReductionProxyStatisticsPrefs( This is a memory leak https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_settings_test_utils.h (right): https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_settings_test_utils.h:166: DataReductionProxyStatisticsPrefs* statistics_prefs_; This should be a scoped_ptr<> https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc (right): https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc:36: void DataReductionProxyStatisticsPrefs::Init() { Add a comment in data_reduction_proxy_prefs to remember to update this list with any new statistics prefs. https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc:75: // need to switch ListValues to scoped_ptr to avoid memory leak They are already, right? Explain. https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc:82: DataReductionProxyPrefMap::iterator iter = pref_map_.find(pref_path); Can't you just return pref_map_[pref_path] ? Or are you worried that pref_path might not be a pref name tracked by this class? If the latter, you should probably check that you find something. https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc:123: // Only write after the first time posting the task task. https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc:124: if (!delayed_task_posted_) { if (delayed_task_posted_) WritePrefs(); else delayed_task_posted_ = true; https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h (right): https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h:28: // variables should be stored and read on the UI thread. must be https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h:36: // |pref_service|. Writes prefs to |pref_service| after |delay| amount of time Remove "amount of time" https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h:37: // and stores in |pref_map_| and |list_pref_map| between writes. If |delay| is stores them in list_pref_map_ https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h:38: // zero, writes directly to the PrefService and does not store in the maps. Somewhere in this class add a reference to a bug and say that shutdown hooks need to be added to support non-zero delays on those platforms. https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h:42: base::TimeDelta delay); const base::TimeDelta& delay https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h:51: void InitInt64Pref(const char* pref); Does this need to be public? https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h:55: void InitListPref(const char* pref); Does this need to be public? https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h:58: int64 GetInt64(const char* pref_path); Can this function be const? https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h:88: base::TimeDelta delay_; can this be const? https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h:94: } // namespace data_reduction_proxy Add a space before // https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs_unittest.cc (right): https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs_unittest.cc:15: const size_t kNumDaysInHistory = 60; Add TODO to make such constants accessible by others to avoid duplication. https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs_unittest.cc:52: #if !(defined(OS_ANDROID) || defined(OS_IOS)) Why is this here? https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs_unittest.cc:88: GetListPrefInt64Value(*delayed_list, i), Can this be moved up a line?
Patchset #4 (id:140001) has been deleted
https://codereview.chromium.org/473723002/diff/120001/chrome/browser/net/chro... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/473723002/diff/120001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate.cc:158: data_reduction_proxy::DataReductionProxyStatisticsPrefs* pref_service) { On 2014/08/27 04:53:24, bengr1 wrote: > Maybe statistics_prefs? Done. https://codereview.chromium.org/473723002/diff/120001/chrome/browser/net/chro... File chrome/browser/net/chrome_network_delegate.h (right): https://codereview.chromium.org/473723002/diff/120001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate.h:43: } On 2014/08/27 04:53:24, bengr1 wrote: > } // namespace data_reduction_proxy Done. https://codereview.chromium.org/473723002/diff/120001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate.h:171: data_reduction_proxy::DataReductionProxyStatisticsPrefs* service) { On 2014/08/27 04:53:24, bengr1 wrote: > Rename as statistics_prefs Done. https://codereview.chromium.org/473723002/diff/120001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate.h:329: data_reduction_proxy_statistics_prefs_; On 2014/08/27 04:53:24, bengr1 wrote: > indent 4 Done. https://codereview.chromium.org/473723002/diff/120001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl.cc (right): https://codereview.chromium.org/473723002/diff/120001/chrome/browser/profiles... chrome/browser/profiles/profile_impl.cc:689: #if defined(OS_ANDROID) || defined(OS_IOS) On 2014/08/27 04:53:24, bengr1 wrote: > Add a comment explaining the platform specialization. Done. https://codereview.chromium.org/473723002/diff/120001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/473723002/diff/120001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:127: const base::FilePath& cookie_path, On 2014/08/27 04:53:24, bengr1 wrote: > Fix indent down to lone 147 Done. https://codereview.chromium.org/473723002/diff/120001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.h (right): https://codereview.chromium.org/473723002/diff/120001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.h:74: session_cookie_mode, On 2014/08/27 04:53:25, bengr1 wrote: > move up with previous line Done. https://codereview.chromium.org/473723002/diff/120001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.h:79: data_reduction_proxy_unavailable, On 2014/08/27 04:53:25, bengr1 wrote: > move up with previous line Done. https://codereview.chromium.org/473723002/diff/120001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.h:81: data_reduction_proxy_chrome_configurator, On 2014/08/27 04:53:25, bengr1 wrote: > Fix indent Done. https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_settings.h (right): https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_settings.h:120: // Sets the |statistics_prefs_| weak pointer to be used for data reduction On 2014/08/27 04:53:25, bengr1 wrote: > This isn't a weak pointer Done. https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_settings_test_utils.cc (right): https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_settings_test_utils.cc:88: statistics_prefs_ = new DataReductionProxyStatisticsPrefs( On 2014/08/27 04:53:25, bengr1 wrote: > This is a memory leak Done. https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_settings_test_utils.h (right): https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_settings_test_utils.h:166: DataReductionProxyStatisticsPrefs* statistics_prefs_; On 2014/08/27 04:53:25, bengr1 wrote: > This should be a scoped_ptr<> Done. https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc (right): https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc:36: void DataReductionProxyStatisticsPrefs::Init() { On 2014/08/27 04:53:25, bengr1 wrote: > Add a comment in data_reduction_proxy_prefs to remember to update this list with > any new statistics prefs. Done. https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc:75: // need to switch ListValues to scoped_ptr to avoid memory leak On 2014/08/27 04:53:25, bengr1 wrote: > They are already, right? Explain. Whoops ya I forgot to move comment. https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc:82: DataReductionProxyPrefMap::iterator iter = pref_map_.find(pref_path); On 2014/08/27 04:53:25, bengr1 wrote: > Can't you just return pref_map_[pref_path] ? Or are you worried that pref_path > might not be a pref name tracked by this class? If the latter, you should > probably check that you find something. This method can't be const if the [] operator is used on a map. https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc:123: // Only write after the first time posting the task On 2014/08/27 04:53:25, bengr1 wrote: > task. Done. https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc:124: if (!delayed_task_posted_) { On 2014/08/27 04:53:25, bengr1 wrote: > if (delayed_task_posted_) > WritePrefs(); > else > delayed_task_posted_ = true; Done. https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h (right): https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h:28: // variables should be stored and read on the UI thread. On 2014/08/27 04:53:26, bengr1 wrote: > must be Done. https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h:36: // |pref_service|. Writes prefs to |pref_service| after |delay| amount of time On 2014/08/27 04:53:25, bengr1 wrote: > Remove "amount of time" Done. https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h:37: // and stores in |pref_map_| and |list_pref_map| between writes. If |delay| is On 2014/08/27 04:53:26, bengr1 wrote: > stores them in > > list_pref_map_ Done. https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h:38: // zero, writes directly to the PrefService and does not store in the maps. On 2014/08/27 04:53:26, bengr1 wrote: > Somewhere in this class add a reference to a bug and say that shutdown hooks > need to be added to support non-zero delays on those platforms. I added this in profile impl where I decide the commit_delay and already comment on how often we're writing. https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h:42: base::TimeDelta delay); On 2014/08/27 04:53:25, bengr1 wrote: > const base::TimeDelta& delay Done. https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h:51: void InitInt64Pref(const char* pref); On 2014/08/27 04:53:26, bengr1 wrote: > Does this need to be public? This makes it much simpler for testing purposes. Otherwise, to use Init() for the test, we have to create a test pref registry with every data reduction proxy pref. https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h:55: void InitListPref(const char* pref); On 2014/08/27 04:53:26, bengr1 wrote: > Does this need to be public? See above https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h:58: int64 GetInt64(const char* pref_path); On 2014/08/27 04:53:25, bengr1 wrote: > Can this function be const? Done. https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h:88: base::TimeDelta delay_; On 2014/08/27 04:53:25, bengr1 wrote: > can this be const? Done. https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h:94: } // namespace data_reduction_proxy On 2014/08/27 04:53:25, bengr1 wrote: > Add a space before // Done. https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs_unittest.cc (right): https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs_unittest.cc:15: const size_t kNumDaysInHistory = 60; On 2014/08/27 04:53:26, bengr1 wrote: > Add TODO to make such constants accessible by others to avoid duplication. Done. https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs_unittest.cc:52: #if !(defined(OS_ANDROID) || defined(OS_IOS)) On 2014/08/27 04:53:26, bengr1 wrote: > Why is this here? We only add the prefs to the pref map when we're not writing straight to prefs. I changed this to check the commit delay instead. https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs_unittest.cc:88: GetListPrefInt64Value(*delayed_list, i), On 2014/08/27 04:53:26, bengr1 wrote: > Can this be moved up a line? Done.
https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h (right): https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h:51: void InitInt64Pref(const char* pref); On 2014/08/28 20:44:08, megjablon wrote: > On 2014/08/27 04:53:26, bengr1 wrote: > > Does this need to be public? > > This makes it much simpler for testing purposes. Otherwise, to use Init() for > the test, we have to create a test pref registry with every data reduction proxy > pref. You can friend your tests. https://codereview.chromium.org/473723002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h:55: void InitListPref(const char* pref); On 2014/08/28 20:44:08, megjablon wrote: > On 2014/08/27 04:53:26, bengr1 wrote: > > Does this need to be public? > > See above See above. https://codereview.chromium.org/473723002/diff/160001/android_webview/browser... File android_webview/browser/aw_browser_context.cc (left): https://codereview.chromium.org/473723002/diff/160001/android_webview/browser... android_webview/browser/aw_browser_context.cc:212: user_pref_service_.get(), Does android_webview still work? I don't see a hookup for the statistics_prefs. https://codereview.chromium.org/473723002/diff/160001/chrome/browser/net/chro... File chrome/browser/net/chrome_network_delegate.h (right): https://codereview.chromium.org/473723002/diff/160001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate.h:170: void set_data_reduction_proxy_statistics_prefs( Add a comment that says that |service| must outlive this ChromeNetworkDelegate. https://codereview.chromium.org/473723002/diff/160001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl.cc (right): https://codereview.chromium.org/473723002/diff/160001/chrome/browser/profiles... chrome/browser/profiles/profile_impl.cc:707: #endif add // defined(SPDY_PROXY_AUTH_ORIGIN) https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_metrics.cc (right): https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_metrics.cc:195: const char* pref, This can be moved up a line: DailyContentLengthUpdate(const char* pref, DataReductionProxyStatisticsPrefs* pref_service) : update... { } https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_metrics.cc:264: const char* pref_original, These can probably move up a line https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_metrics.cc:478: int received_content_length, This can probably be moved up a line. https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_metrics.h (right): https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_metrics.h:9: #include "components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h" I think you should just be able to forward declare DataReductionProxyStatisticsPrefs https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_metrics_unittest.cc (right): https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_metrics_unittest.cc:35: // Test UpdateContentLengthPrefs. You need a test that the metrics are committed to prefs after commit_delay_. https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_metrics_unittest.cc:40: &pref_service_, Indentation is wrong. https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_metrics_unittest.cc:41: scoped_refptr<base::TestSimpleTaskRunner>( #include "base/memory/ref_counted.h" https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_prefs.cc (right): https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_prefs.cc:30: // Add any new data reduction proxy prefs to the |pref_map_| in Init() of This comment is wrong. You only need the comment at line 41 https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc (right): https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc:211: return 0; Why is this allowed? https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_settings.h (right): https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_settings.h:15: #include "base/memory/weak_ptr.h" Do you need this? https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_settings.h:120: // Sets the |statistics_prefs_| to be used for data reduction Move up as much of the next line as you can and still remain <= 80 chars. https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_settings_test_utils.cc (right): https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_settings_test_utils.cc:165: statistics_prefs_.get()); Can this fit on the previous line? https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc (right): https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc:121: // Only write after the first time posting the task This is still missing the period. https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs_unittest.cc (right): https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs_unittest.cc:102: #if defined(OS_ANDROID) || defined(OS_IOS) You should test both cases irrespective of platform. https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs_unittest.cc:139: #if !(defined(OS_ANDROID) || defined(OS_IOS)) This shouldn't be platform specific.
megjablon@chromium.org changed reviewers: + mmenke@chromium.org
mmenke: chrome/* https://codereview.chromium.org/473723002/diff/160001/android_webview/browser... File android_webview/browser/aw_browser_context.cc (left): https://codereview.chromium.org/473723002/diff/160001/android_webview/browser... android_webview/browser/aw_browser_context.cc:212: user_pref_service_.get(), On 2014/08/28 21:38:34, bengr1 wrote: > Does android_webview still work? I don't see a hookup for the statistics_prefs. Done. https://codereview.chromium.org/473723002/diff/160001/chrome/browser/net/chro... File chrome/browser/net/chrome_network_delegate.h (right): https://codereview.chromium.org/473723002/diff/160001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate.h:170: void set_data_reduction_proxy_statistics_prefs( On 2014/08/28 21:38:34, bengr1 wrote: > Add a comment that says that |service| must outlive this ChromeNetworkDelegate. Done. https://codereview.chromium.org/473723002/diff/160001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl.cc (right): https://codereview.chromium.org/473723002/diff/160001/chrome/browser/profiles... chrome/browser/profiles/profile_impl.cc:707: #endif On 2014/08/28 21:38:34, bengr1 wrote: > add // defined(SPDY_PROXY_AUTH_ORIGIN) Done. https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_metrics.cc (right): https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_metrics.cc:195: const char* pref, On 2014/08/28 21:38:34, bengr1 wrote: > This can be moved up a line: > > DailyContentLengthUpdate(const char* pref, > DataReductionProxyStatisticsPrefs* pref_service) > : update... { > } Done. https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_metrics.cc:264: const char* pref_original, On 2014/08/28 21:38:34, bengr1 wrote: > These can probably move up a line Done. https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_metrics.cc:478: int received_content_length, On 2014/08/28 21:38:34, bengr1 wrote: > This can probably be moved up a line. Done. https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_metrics.h (right): https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_metrics.h:9: #include "components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h" On 2014/08/28 21:38:34, bengr1 wrote: > I think you should just be able to forward declare > DataReductionProxyStatisticsPrefs Done. https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_metrics_unittest.cc (right): https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_metrics_unittest.cc:35: // Test UpdateContentLengthPrefs. On 2014/08/28 21:38:35, bengr1 wrote: > You need a test that the metrics are committed to prefs after commit_delay_. For these tests I'll just have the pref service go direct, then the statistics prefs unit tests will be for making sure the direct and delayed writing work properly. https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_metrics_unittest.cc:40: &pref_service_, On 2014/08/28 21:38:34, bengr1 wrote: > Indentation is wrong. Done. https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_metrics_unittest.cc:41: scoped_refptr<base::TestSimpleTaskRunner>( On 2014/08/28 21:38:35, bengr1 wrote: > #include "base/memory/ref_counted.h" Done. https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_prefs.cc (right): https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_prefs.cc:30: // Add any new data reduction proxy prefs to the |pref_map_| in Init() of On 2014/08/28 21:38:35, bengr1 wrote: > This comment is wrong. You only need the comment at line 41 Done. https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc (right): https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc:211: return 0; On 2014/08/28 21:38:35, bengr1 wrote: > Why is this allowed? Removed. https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_settings.h (right): https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_settings.h:15: #include "base/memory/weak_ptr.h" On 2014/08/28 21:38:35, bengr1 wrote: > Do you need this? not anymore! https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_settings.h:120: // Sets the |statistics_prefs_| to be used for data reduction On 2014/08/28 21:38:35, bengr1 wrote: > Move up as much of the next line as you can and still remain <= 80 chars. Done. https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_settings_test_utils.cc (right): https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_settings_test_utils.cc:165: statistics_prefs_.get()); On 2014/08/28 21:38:35, bengr1 wrote: > Can this fit on the previous line? Done. https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc (right): https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc:121: // Only write after the first time posting the task On 2014/08/28 21:38:35, bengr1 wrote: > This is still missing the period. Done. https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs_unittest.cc (right): https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs_unittest.cc:102: #if defined(OS_ANDROID) || defined(OS_IOS) On 2014/08/28 21:38:35, bengr1 wrote: > You should test both cases irrespective of platform. Done. https://codereview.chromium.org/473723002/diff/160001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs_unittest.cc:139: #if !(defined(OS_ANDROID) || defined(OS_IOS)) On 2014/08/28 21:38:35, bengr1 wrote: > This shouldn't be platform specific. Done.
megjablon@chromium.org changed reviewers: + sgurun@chromium.org
sgurun: android_webview/*
https://codereview.chromium.org/473723002/diff/200001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.h (right): https://codereview.chromium.org/473723002/diff/200001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.h:265: data_reduction_proxy_statistics_prefs_; This is getting a bit excessive. Can we just wrap all of these in a single class, abstracting out what does what from the embedder?
On 2014/09/02 15:15:06, mmenke wrote: > https://codereview.chromium.org/473723002/diff/200001/chrome/browser/profiles... > File chrome/browser/profiles/profile_impl_io_data.h (right): > > https://codereview.chromium.org/473723002/diff/200001/chrome/browser/profiles... > chrome/browser/profiles/profile_impl_io_data.h:265: > data_reduction_proxy_statistics_prefs_; > This is getting a bit excessive. Can we just wrap all of these in a single > class, abstracting out what does what from the embedder? That makes sense and is something I was hoping to get around to doing soon. Do you think that should happen before landing this one?
On 2014/09/02 15:27:40, bengr1 wrote: > On 2014/09/02 15:15:06, mmenke wrote: > > > https://codereview.chromium.org/473723002/diff/200001/chrome/browser/profiles... > > File chrome/browser/profiles/profile_impl_io_data.h (right): > > > > > https://codereview.chromium.org/473723002/diff/200001/chrome/browser/profiles... > > chrome/browser/profiles/profile_impl_io_data.h:265: > > data_reduction_proxy_statistics_prefs_; > > This is getting a bit excessive. Can we just wrap all of these in a single > > class, abstracting out what does what from the embedder? > > That makes sense and is something I was hoping to get around to doing soon. Do > you think that should happen before landing this one? This CL is important enough that it's probably best not to block on that, but let's not add any new hooks until that's done. Sound reasonable?
On 2014/09/02 15:31:15, mmenke wrote: > On 2014/09/02 15:27:40, bengr1 wrote: > > On 2014/09/02 15:15:06, mmenke wrote: > > > > > > https://codereview.chromium.org/473723002/diff/200001/chrome/browser/profiles... > > > File chrome/browser/profiles/profile_impl_io_data.h (right): > > > > > > > > > https://codereview.chromium.org/473723002/diff/200001/chrome/browser/profiles... > > > chrome/browser/profiles/profile_impl_io_data.h:265: > > > data_reduction_proxy_statistics_prefs_; > > > This is getting a bit excessive. Can we just wrap all of these in a single > > > class, abstracting out what does what from the embedder? > > > > That makes sense and is something I was hoping to get around to doing soon. Do > > you think that should happen before landing this one? > > This CL is important enough that it's probably best not to block on that, but > let's not add any new hooks until that's done. Sound reasonable? Yes. Thanks.
https://codereview.chromium.org/473723002/diff/200001/android_webview/browser... File android_webview/browser/aw_browser_context.cc (right): https://codereview.chromium.org/473723002/diff/200001/android_webview/browser... android_webview/browser/aw_browser_context.cc:111: g_browser_process->local_state(), does this even compile? g_browser_process is a browser concept.
https://codereview.chromium.org/473723002/diff/200001/android_webview/browser... File android_webview/browser/aw_browser_context.cc (right): https://codereview.chromium.org/473723002/diff/200001/android_webview/browser... android_webview/browser/aw_browser_context.cc:111: g_browser_process->local_state(), On 2014/09/03 01:00:38, sgurun_OOO_until_Oct5 wrote: > does this even compile? g_browser_process is a browser concept. My bad. It looks like we're using user_pref_service_ here for data reduction proxy prefs.
https://codereview.chromium.org/473723002/diff/220001/android_webview/browser... File android_webview/browser/aw_browser_context.cc (right): https://codereview.chromium.org/473723002/diff/220001/android_webview/browser... android_webview/browser/aw_browser_context.cc:111: user_pref_service_.get(), unfortunately this will not work, because we create user_pref_service_ later when AwContents is created. It will be a null here. You could call AwBrowserContext::CreateUserPrefServiceIfNecessary() here but then I think it is better to cleanup the code here a little bit so that this method is not called from AwContents anymore. will be OOO for a while so making benm@ as the reviewer for this CL.
sgurun@chromium.org changed reviewers: + benm@chromium.org - sgurun@chromium.org
https://codereview.chromium.org/473723002/diff/220001/android_webview/browser... File android_webview/browser/aw_browser_context.cc (right): https://codereview.chromium.org/473723002/diff/220001/android_webview/browser... android_webview/browser/aw_browser_context.cc:111: user_pref_service_.get(), On 2014/09/04 00:32:26, sgurun_OOO_until_Oct5 wrote: > unfortunately this will not work, because we create user_pref_service_ later > when AwContents is created. It will be a null here. You could call > AwBrowserContext::CreateUserPrefServiceIfNecessary() here but then I think it is > better to cleanup the code here a little bit so that this method is not called > from AwContents anymore. > will be OOO for a while so making benm@ as the reviewer for this CL. Instead we are just not going to instantiate DataReductionProxyStatisticsPrefs and have support for a null statistics_prefs_ in DataReductionProxySettings since webview does not use these prefs anyway.
https://codereview.chromium.org/473723002/diff/240001/android_webview/browser... File android_webview/browser/aw_browser_context.cc (right): https://codereview.chromium.org/473723002/diff/240001/android_webview/browser... android_webview/browser/aw_browser_context.cc:107: // Unlike in Chrome, DataReductionProxyStatistisPrefs is not instantiated I would change this comment to: // Compression statistics are not gathered for WebView, so // DataReductionProxyStatisticsPrefs is not instantiated // and passed to the network delegate. and I would move the comment to wherever we call other setters on the NetworkDelegate. https://codereview.chromium.org/473723002/diff/240001/chrome/browser/net/chro... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/473723002/diff/240001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate.cc:160: base::Bind(&UpdateContentLengthPrefs, Put each parameter on its own line. https://codereview.chromium.org/473723002/diff/240001/chrome/browser/net/chro... File chrome/browser/net/chrome_network_delegate.h (right): https://codereview.chromium.org/473723002/diff/240001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate.h:173: statistics_prefs_ = service; Rename "service" as "statistics_prefs" https://codereview.chromium.org/473723002/diff/240001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_metrics_unittest.cc (right): https://codereview.chromium.org/473723002/diff/240001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_metrics_unittest.cc:47: new base::TestSimpleTaskRunner()), can this fit on the previous line? https://codereview.chromium.org/473723002/diff/240001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc (right): https://codereview.chromium.org/473723002/diff/240001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc:211: return 0; What's the significance of 0? https://codereview.chromium.org/473723002/diff/240001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc:481: if (!statistics_prefs_) { Is there no way to just ensure that this function and the ones like it are never called if statistics prefs is null? If not, this is ok. https://codereview.chromium.org/473723002/diff/240001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_settings_test_utils.h (right): https://codereview.chromium.org/473723002/diff/240001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_settings_test_utils.h:170: scoped_ptr<DataReductionProxyStatisticsPrefs> statistics_prefs_; This should be forward declared.
Sorry, I wrote comments on this CL yesterday, but forgot to publish them. I'll double-check them against your newer CLs, and get back to you tomorrow.
https://codereview.chromium.org/473723002/diff/240001/chrome/browser/net/chro... File chrome/browser/net/chrome_network_delegate.h (right): https://codereview.chromium.org/473723002/diff/240001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate.h:43: } // namespace data_reduction_proxy nit: Comment not needed (End namespace comments generally aren't used for forward declarations). Please remove the domain_reliability one while you're at it. https://codereview.chromium.org/473723002/diff/240001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate.h:328: data_reduction_proxy_auth_request_handler_; While you're here, this should be indented 4 more. https://codereview.chromium.org/473723002/diff/240001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate.h:329: data_reduction_proxy::DataReductionProxyStatisticsPrefs* statistics_prefs_; data_reduction_proxy_statistics_prefs_ is more consistent with the other names. https://codereview.chromium.org/473723002/diff/240001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate.h:329: data_reduction_proxy::DataReductionProxyStatisticsPrefs* statistics_prefs_; optional: Actually, I think we don't need to know how it works. Maybe just call it DataReductionProxyStatistics? https://codereview.chromium.org/473723002/diff/240001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl.cc (right): https://codereview.chromium.org/473723002/diff/240001/chrome/browser/profiles... chrome/browser/profiles/profile_impl.cc:697: #endif Should the profile really know or care about this? Seems like this should just go in DataReductionProxyStatisticsPrefs. https://codereview.chromium.org/473723002/diff/240001/chrome/browser/profiles... chrome/browser/profiles/profile_impl.cc:704: data_reduction_proxy_statistics_prefs->Init(); Why can't the DataReductionProxyStatisticsPrefs do this on construction? https://codereview.chromium.org/473723002/diff/240001/chrome/browser/profiles... chrome/browser/profiles/profile_impl.cc:706: data_reduction_proxy_statistics_prefs.get()); Why do we need both the settings and the prefs? Can't the settings just own the statistics, and abstract out how/when the prefs are being updated behind the scenes? https://codereview.chromium.org/473723002/diff/240001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc (right): https://codereview.chromium.org/473723002/diff/240001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc:28: DCHECK(delay >= base::TimeDelta()); DCHECK_GE?
https://codereview.chromium.org/473723002/diff/240001/android_webview/browser... File android_webview/browser/aw_browser_context.cc (right): https://codereview.chromium.org/473723002/diff/240001/android_webview/browser... android_webview/browser/aw_browser_context.cc:107: // Unlike in Chrome, DataReductionProxyStatistisPrefs is not instantiated On 2014/09/04 21:18:21, bengr1 wrote: > I would change this comment to: > > // Compression statistics are not gathered for WebView, so > // DataReductionProxyStatisticsPrefs is not instantiated > // and passed to the network delegate. > > and I would move the comment to wherever we call other setters on the > NetworkDelegate. Done. https://codereview.chromium.org/473723002/diff/240001/chrome/browser/net/chro... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/473723002/diff/240001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate.cc:160: base::Bind(&UpdateContentLengthPrefs, On 2014/09/04 21:18:21, bengr1 wrote: > Put each parameter on its own line. Done. https://codereview.chromium.org/473723002/diff/240001/chrome/browser/net/chro... File chrome/browser/net/chrome_network_delegate.h (right): https://codereview.chromium.org/473723002/diff/240001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate.h:43: } // namespace data_reduction_proxy On 2014/09/05 15:23:09, mmenke wrote: > nit: Comment not needed (End namespace comments generally aren't used for > forward declarations). > > Please remove the domain_reliability one while you're at it. Good to know! Thanks https://codereview.chromium.org/473723002/diff/240001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate.h:173: statistics_prefs_ = service; On 2014/09/04 21:18:21, bengr1 wrote: > Rename "service" as "statistics_prefs" Done. https://codereview.chromium.org/473723002/diff/240001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate.h:328: data_reduction_proxy_auth_request_handler_; On 2014/09/05 15:23:09, mmenke wrote: > While you're here, this should be indented 4 more. Done. https://codereview.chromium.org/473723002/diff/240001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate.h:329: data_reduction_proxy::DataReductionProxyStatisticsPrefs* statistics_prefs_; On 2014/09/05 15:23:09, mmenke wrote: > data_reduction_proxy_statistics_prefs_ is more consistent with the other names. Agreed. https://codereview.chromium.org/473723002/diff/240001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate.h:329: data_reduction_proxy::DataReductionProxyStatisticsPrefs* statistics_prefs_; On 2014/09/05 15:23:09, mmenke wrote: > optional: Actually, I think we don't need to know how it works. Maybe just > call it DataReductionProxyStatistics? I think it makes more sense to keep Prefs on it because it operates as a pref service for these data reduction proxy prefs. https://codereview.chromium.org/473723002/diff/240001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl.cc (right): https://codereview.chromium.org/473723002/diff/240001/chrome/browser/profiles... chrome/browser/profiles/profile_impl.cc:697: #endif On 2014/09/05 15:23:09, mmenke wrote: > Should the profile really know or care about this? Seems like this should just > go in DataReductionProxyStatisticsPrefs. See comment below. https://codereview.chromium.org/473723002/diff/240001/chrome/browser/profiles... chrome/browser/profiles/profile_impl.cc:704: data_reduction_proxy_statistics_prefs->Init(); On 2014/09/05 15:23:09, mmenke wrote: > Why can't the DataReductionProxyStatisticsPrefs do this on construction? Agreed. Changed. https://codereview.chromium.org/473723002/diff/240001/chrome/browser/profiles... chrome/browser/profiles/profile_impl.cc:706: data_reduction_proxy_statistics_prefs.get()); On 2014/09/05 15:23:09, mmenke wrote: > Why do we need both the settings and the prefs? Can't the settings just own the > statistics, and abstract out how/when the prefs are being updated behind the > scenes? I'm not sure how to plumb this because this is accessed from chrome network delegate. Do you have any suggestions? https://codereview.chromium.org/473723002/diff/240001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_metrics_unittest.cc (right): https://codereview.chromium.org/473723002/diff/240001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_metrics_unittest.cc:47: new base::TestSimpleTaskRunner()), On 2014/09/04 21:18:21, bengr1 wrote: > can this fit on the previous line? Nope, too long! https://codereview.chromium.org/473723002/diff/240001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc (right): https://codereview.chromium.org/473723002/diff/240001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc:211: return 0; On 2014/09/04 21:18:21, bengr1 wrote: > What's the significance of 0? Fixed as discussed offline. https://codereview.chromium.org/473723002/diff/240001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc:481: if (!statistics_prefs_) { On 2014/09/04 21:18:21, bengr1 wrote: > Is there no way to just ensure that this function and the ones like it are never > called if statistics prefs is null? If not, this is ok. These are only called by JNI calls for Android chrome and we know this will be set for Android. https://codereview.chromium.org/473723002/diff/240001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_settings_test_utils.h (right): https://codereview.chromium.org/473723002/diff/240001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_settings_test_utils.h:170: scoped_ptr<DataReductionProxyStatisticsPrefs> statistics_prefs_; On 2014/09/04 21:18:21, bengr1 wrote: > This should be forward declared. Done. https://codereview.chromium.org/473723002/diff/240001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc (right): https://codereview.chromium.org/473723002/diff/240001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc:28: DCHECK(delay >= base::TimeDelta()); On 2014/09/05 15:23:09, mmenke wrote: > DCHECK_GE? Didn't know about that. Thanks.
LGTM, though per earlier comments, in another CL I'd really like to see this reduced to only two publicly visible classes (On on the IO thread, one on the UI thread) that take care of all the work, including posting the task to the UI thread to record stats. Think it would result in more modular, less intrusive, code. https://codereview.chromium.org/473723002/diff/240001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl.cc (right): https://codereview.chromium.org/473723002/diff/240001/chrome/browser/profiles... chrome/browser/profiles/profile_impl.cc:706: data_reduction_proxy_statistics_prefs.get()); On 2014/09/05 20:56:39, megjablon wrote: > On 2014/09/05 15:23:09, mmenke wrote: > > Why do we need both the settings and the prefs? Can't the settings just own > the > > statistics, and abstract out how/when the prefs are being updated behind the > > scenes? > > I'm not sure how to plumb this because this is accessed from chrome network > delegate. Do you have any suggestions? Had been thinking the network delegate had access to the settings, but it only has the stats and the stats_prefs, so let's just go with this for now. https://codereview.chromium.org/473723002/diff/260001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl.cc (right): https://codereview.chromium.org/473723002/diff/260001/chrome/browser/profiles... chrome/browser/profiles/profile_impl.cc:696: base::TimeDelta commit_delay = base::TimeDelta::FromMinutes(60) Missing semi-colon.
On 2014/09/08 16:29:15, mmenke wrote: > LGTM, though per earlier comments, in another CL I'd really like to see this > reduced to only two publicly visible classes (On on the IO thread, one on the UI > thread) that take care of all the work, including posting the task to the UI > thread to record stats. Think it would result in more modular, less intrusive, > code. > > https://codereview.chromium.org/473723002/diff/240001/chrome/browser/profiles... > File chrome/browser/profiles/profile_impl.cc (right): > > https://codereview.chromium.org/473723002/diff/240001/chrome/browser/profiles... > chrome/browser/profiles/profile_impl.cc:706: > data_reduction_proxy_statistics_prefs.get()); > On 2014/09/05 20:56:39, megjablon wrote: > > On 2014/09/05 15:23:09, mmenke wrote: > > > Why do we need both the settings and the prefs? Can't the settings just own > > the > > > statistics, and abstract out how/when the prefs are being updated behind the > > > scenes? > > > > I'm not sure how to plumb this because this is accessed from chrome network > > delegate. Do you have any suggestions? > > Had been thinking the network delegate had access to the settings, but it only > has the stats and the stats_prefs, so let's just go with this for now. > > https://codereview.chromium.org/473723002/diff/260001/chrome/browser/profiles... > File chrome/browser/profiles/profile_impl.cc (right): > > https://codereview.chromium.org/473723002/diff/260001/chrome/browser/profiles... > chrome/browser/profiles/profile_impl.cc:696: base::TimeDelta commit_delay = > base::TimeDelta::FromMinutes(60) > Missing semi-colon. Agreed. I'll file a bug for this.
https://codereview.chromium.org/473723002/diff/260001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl.cc (right): https://codereview.chromium.org/473723002/diff/260001/chrome/browser/profiles... chrome/browser/profiles/profile_impl.cc:696: base::TimeDelta commit_delay = base::TimeDelta::FromMinutes(60) On 2014/09/08 16:29:15, mmenke wrote: > Missing semi-colon. Done.
Patchset #10 (id:300001) has been deleted
Patchset #10 (id:320001) has been deleted
Lgtm, but please fix the nits below. Also, change the CL description to the following and limit each line to about 60 characters. Update data reduction proxy statistics prefs less often on desktop For platforms other than iOS and Android, the preference data is stored in memory and written to disk at 60 minute intervals and on shutdown via the DataReductionProxyDelayedPrefService. For iOS and Android, the DataReductionProxyDelayedPrefService takes a delay time of zero and simply passes the prefs through to the PrefService. https://codereview.chromium.org/473723002/diff/340001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc (right): https://codereview.chromium.org/473723002/diff/340001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc:37: if (delay_ != base::TimeDelta()) { change to: if (delay == base::TimeDelta() return; https://codereview.chromium.org/473723002/diff/340001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc:86: int64 pref_value) { Use int64_t here and everywhere https://codereview.chromium.org/473723002/diff/340001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h (right): https://codereview.chromium.org/473723002/diff/340001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h:92: DataReductionProxyListPrefMap list_pref_map_; Add: DISALLOW_COPY_AND_ASSIGN(DataReductionProxyStatisticsPrefs); and include "base/macros.h"
Are we going to land this CL, or the enable DRP on all platforms CL first? There'll be merge conflicts. Not major ones, but think it's worth having a second set of eyes glance over after the merge.
On 2014/09/09 14:02:46, mmenke wrote: > Are we going to land this CL, or the enable DRP on all platforms CL first? > There'll be merge conflicts. Not major ones, but think it's worth having a > second set of eyes glance over after the merge. This CL will be landed first and then it will be enabled on all platforms. kundaji and I have been in sync on what needs to be done to have the prefs working on all platforms. bengr you'll also be keeping an eye on this, correct?
https://codereview.chromium.org/473723002/diff/340001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc (right): https://codereview.chromium.org/473723002/diff/340001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc:37: if (delay_ != base::TimeDelta()) { On 2014/09/09 02:09:39, bengr1 wrote: > change to: > > if (delay == base::TimeDelta() > return; > Done. https://codereview.chromium.org/473723002/diff/340001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc:86: int64 pref_value) { On 2014/09/09 02:09:40, bengr1 wrote: > Use int64_t here and everywhere Done. https://codereview.chromium.org/473723002/diff/340001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h (right): https://codereview.chromium.org/473723002/diff/340001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h:92: DataReductionProxyListPrefMap list_pref_map_; On 2014/09/09 02:09:40, bengr1 wrote: > Add: DISALLOW_COPY_AND_ASSIGN(DataReductionProxyStatisticsPrefs); and include > "base/macros.h" Done.
/browser/net and profiles/ LGTM.
aw lgtm https://codereview.chromium.org/473723002/diff/360001/android_webview/browser... File android_webview/browser/net/aw_url_request_context_getter.cc (right): https://codereview.chromium.org/473723002/diff/360001/android_webview/browser... android_webview/browser/net/aw_url_request_context_getter.cc:241: // network delegate. Out of interest, what would it take to start gathering compression stats for WebView? Would the stats be gathered on a per-app basis or would they be aggregated based on device?
On 2014/09/10 09:26:32, benm wrote: > aw lgtm > > https://codereview.chromium.org/473723002/diff/360001/android_webview/browser... > File android_webview/browser/net/aw_url_request_context_getter.cc (right): > > https://codereview.chromium.org/473723002/diff/360001/android_webview/browser... > android_webview/browser/net/aw_url_request_context_getter.cc:241: // network > delegate. > Out of interest, what would it take to start gathering compression stats for > WebView? > > Would the stats be gathered on a per-app basis or would they be aggregated based > on device? They would be per app. All the webviews share the same profile. I don't think they track local state so we'd have to migrate the prefs, but we plan to do that anyway.
The CQ bit was checked by megjablon@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/megjablon@chromium.org/473723002/360001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2014/09/10 22:20:41, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > linux_chromium_rel_swarming on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) Hey mmenke, I had to make a couple fixes. Can you please do another review for me? I fixed bandwidth_view.js for both chrome/browser and chrome/test. I also realized that calling WritePrefs() in the destructor of statistics prefs was crashing it so I had to move it to ProfileImplIoData. Sorry, I synced and forgot to upload it in a separate CL, makes it hard to pick out my changes.
https://codereview.chromium.org/473723002/diff/400001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/473723002/diff/400001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:107: io_data_->data_reduction_proxy_statistics_prefs_->WritePrefs(); This is called on the UI thread, but the prefs live on the IO thread, no? Also, we're not guaranteed that Init() will be called, so this may be NULL. https://codereview.chromium.org/473723002/diff/400001/chrome/browser/resource... File chrome/browser/resources/net_internals/bandwidth_view.js (right): https://codereview.chromium.org/473723002/diff/400001/chrome/browser/resource... chrome/browser/resources/net_internals/bandwidth_view.js:80: sessionReceived; Why the change? We still include session prefs in kHttpReceivedContentLength, don't we? We just don't update the two as often. https://codereview.chromium.org/473723002/diff/400001/chrome/test/data/webui/... File chrome/test/data/webui/net_internals/bandwidth_view.js (right): https://codereview.chromium.org/473723002/diff/400001/chrome/test/data/webui/... chrome/test/data/webui/net_internals/bandwidth_view.js:113: this.completeIfDone(); Shouldn't onHistoricNetworkStatsChanged still be called (Possibly with 0)? https://codereview.chromium.org/473723002/diff/400001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc (right): https://codereview.chromium.org/473723002/diff/400001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc:14: #include "base/values.h" include base/logging.h for DCHECK. https://codereview.chromium.org/473723002/diff/400001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc:26: delayed_task_posted_(false){ nit: Space before "{" https://codereview.chromium.org/473723002/diff/400001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc:38: //Init all int64 prefs Space between "//" and comments, comments that are sentences should end in periods (x2) https://codereview.chromium.org/473723002/diff/400001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc:92: WritePrefsAndPost(); I hadn't looked at this code before, but checking "delayed_task_posted_" everywhere seems a bit uglier than necessary. My suggestion: Rename WritePrefsAndPost() to "DelayedWritePrefs()" or somesuch, make it do nothing if delayed_task_posted_ is true. If it's false, call WritePrefs on a timer. Have WritePrefs set delayed_task_posted_ to false (Can also have it DCHECK that it's true, in the first place). That saves a couple lines of code, and makes for slightly simpler code. https://codereview.chromium.org/473723002/diff/400001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc:158: } // namespace data_reduction_proxy nit: 2 spaces before comment. https://codereview.chromium.org/473723002/diff/400001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h (right): https://codereview.chromium.org/473723002/diff/400001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h:34: DataReductionProxyListPrefMap; These should be in the private section, since they aren't used publicly. https://codereview.chromium.org/473723002/diff/400001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h:45: virtual ~DataReductionProxyStatisticsPrefs(); virtual not needed, since nothing inherits from this class. https://codereview.chromium.org/473723002/diff/400001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h:88: scoped_refptr<base::SequencedTaskRunner> task_runner_; Should include ref_counted.h for this.
On 2014/09/15 15:11:02, mmenke wrote: > https://codereview.chromium.org/473723002/diff/400001/chrome/browser/profiles... > File chrome/browser/profiles/profile_impl_io_data.cc (right): > > https://codereview.chromium.org/473723002/diff/400001/chrome/browser/profiles... > chrome/browser/profiles/profile_impl_io_data.cc:107: > io_data_->data_reduction_proxy_statistics_prefs_->WritePrefs(); > This is called on the UI thread, but the prefs live on the IO thread, no? > > Also, we're not guaranteed that Init() will be called, so this may be NULL. > > https://codereview.chromium.org/473723002/diff/400001/chrome/browser/resource... > File chrome/browser/resources/net_internals/bandwidth_view.js (right): > > https://codereview.chromium.org/473723002/diff/400001/chrome/browser/resource... > chrome/browser/resources/net_internals/bandwidth_view.js:80: sessionReceived; > Why the change? We still include session prefs in kHttpReceivedContentLength, > don't we? We just don't update the two as often. > > https://codereview.chromium.org/473723002/diff/400001/chrome/test/data/webui/... > File chrome/test/data/webui/net_internals/bandwidth_view.js (right): > > https://codereview.chromium.org/473723002/diff/400001/chrome/test/data/webui/... > chrome/test/data/webui/net_internals/bandwidth_view.js:113: > this.completeIfDone(); > Shouldn't onHistoricNetworkStatsChanged still be called (Possibly with 0)? > > https://codereview.chromium.org/473723002/diff/400001/components/data_reducti... > File > components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc > (right): > > https://codereview.chromium.org/473723002/diff/400001/components/data_reducti... > components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc:14: > #include "base/values.h" > include base/logging.h for DCHECK. > > https://codereview.chromium.org/473723002/diff/400001/components/data_reducti... > components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc:26: > delayed_task_posted_(false){ > nit: Space before "{" > > https://codereview.chromium.org/473723002/diff/400001/components/data_reducti... > components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc:38: > //Init all int64 prefs > Space between "//" and comments, comments that are sentences should end in > periods (x2) > > https://codereview.chromium.org/473723002/diff/400001/components/data_reducti... > components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc:92: > WritePrefsAndPost(); > I hadn't looked at this code before, but checking "delayed_task_posted_" > everywhere seems a bit uglier than necessary. > > My suggestion: > > Rename WritePrefsAndPost() to "DelayedWritePrefs()" or somesuch, make it do > nothing if delayed_task_posted_ is true. If it's false, call WritePrefs on a > timer. Have WritePrefs set delayed_task_posted_ to false (Can also have it > DCHECK that it's true, in the first place). > > That saves a couple lines of code, and makes for slightly simpler code. > > https://codereview.chromium.org/473723002/diff/400001/components/data_reducti... > components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc:158: > } // namespace data_reduction_proxy > nit: 2 spaces before comment. > > https://codereview.chromium.org/473723002/diff/400001/components/data_reducti... > File > components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h > (right): > > https://codereview.chromium.org/473723002/diff/400001/components/data_reducti... > components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h:34: > DataReductionProxyListPrefMap; > These should be in the private section, since they aren't used publicly. > > https://codereview.chromium.org/473723002/diff/400001/components/data_reducti... > components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h:45: > virtual ~DataReductionProxyStatisticsPrefs(); > virtual not needed, since nothing inherits from this class. > > https://codereview.chromium.org/473723002/diff/400001/components/data_reducti... > components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h:88: > scoped_refptr<base::SequencedTaskRunner> task_runner_; > Should include ref_counted.h for this. Thanks again for reviewing this!
https://codereview.chromium.org/473723002/diff/400001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/473723002/diff/400001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:107: io_data_->data_reduction_proxy_statistics_prefs_->WritePrefs(); On 2014/09/15 15:11:01, mmenke wrote: > This is called on the UI thread, but the prefs live on the IO thread, no? > > Also, we're not guaranteed that Init() will be called, so this may be NULL. The prefs live on the UI thread. That's true about Init(), but the pref maps would just be empty. I'll check the time delta to see if its directly writing at the beginning of WritePrefs(). https://codereview.chromium.org/473723002/diff/400001/chrome/browser/resource... File chrome/browser/resources/net_internals/bandwidth_view.js (right): https://codereview.chromium.org/473723002/diff/400001/chrome/browser/resource... chrome/browser/resources/net_internals/bandwidth_view.js:80: sessionReceived; On 2014/09/15 15:11:02, mmenke wrote: > Why the change? We still include session prefs in kHttpReceivedContentLength, > don't we? We just don't update the two as often. Ah I didn't understand how historic_received_content_length was being updated. Ya, this won't work, but historic values will only be updated with the session values added in every hour. https://codereview.chromium.org/473723002/diff/400001/chrome/test/data/webui/... File chrome/test/data/webui/net_internals/bandwidth_view.js (right): https://codereview.chromium.org/473723002/diff/400001/chrome/test/data/webui/... chrome/test/data/webui/net_internals/bandwidth_view.js:113: this.completeIfDone(); On 2014/09/15 15:11:02, mmenke wrote: > Shouldn't onHistoricNetworkStatsChanged still be called (Possibly with 0)? The test times out since it waits on the historic network stats to change which only happens every hour. Any alternative suggestions on how to fix this? https://codereview.chromium.org/473723002/diff/400001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc (right): https://codereview.chromium.org/473723002/diff/400001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc:14: #include "base/values.h" On 2014/09/15 15:11:02, mmenke wrote: > include base/logging.h for DCHECK. Done. https://codereview.chromium.org/473723002/diff/400001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc:26: delayed_task_posted_(false){ On 2014/09/15 15:11:02, mmenke wrote: > nit: Space before "{" Done. https://codereview.chromium.org/473723002/diff/400001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc:38: //Init all int64 prefs On 2014/09/15 15:11:02, mmenke wrote: > Space between "//" and comments, comments that are sentences should end in > periods (x2) Done. https://codereview.chromium.org/473723002/diff/400001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc:92: WritePrefsAndPost(); On 2014/09/15 15:11:02, mmenke wrote: > I hadn't looked at this code before, but checking "delayed_task_posted_" > everywhere seems a bit uglier than necessary. > > My suggestion: > > Rename WritePrefsAndPost() to "DelayedWritePrefs()" or somesuch, make it do > nothing if delayed_task_posted_ is true. If it's false, call WritePrefs on a > timer. Have WritePrefs set delayed_task_posted_ to false (Can also have it > DCHECK that it's true, in the first place). > > That saves a couple lines of code, and makes for slightly simpler code. Thanks for this suggestion! https://codereview.chromium.org/473723002/diff/400001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc:158: } // namespace data_reduction_proxy On 2014/09/15 15:11:02, mmenke wrote: > nit: 2 spaces before comment. Done. https://codereview.chromium.org/473723002/diff/400001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h (right): https://codereview.chromium.org/473723002/diff/400001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h:34: DataReductionProxyListPrefMap; On 2014/09/15 15:11:02, mmenke wrote: > These should be in the private section, since they aren't used publicly. Done. https://codereview.chromium.org/473723002/diff/400001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h:45: virtual ~DataReductionProxyStatisticsPrefs(); On 2014/09/15 15:11:02, mmenke wrote: > virtual not needed, since nothing inherits from this class. Done. https://codereview.chromium.org/473723002/diff/400001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h:88: scoped_refptr<base::SequencedTaskRunner> task_runner_; On 2014/09/15 15:11:02, mmenke wrote: > Should include ref_counted.h for this. Done.
Sorry, meant to get back to this today. I'll do it first thing tomorrow. On 2014/09/15 18:19:24, megjablon wrote: > https://codereview.chromium.org/473723002/diff/400001/chrome/browser/profiles... > File chrome/browser/profiles/profile_impl_io_data.cc (right): > > https://codereview.chromium.org/473723002/diff/400001/chrome/browser/profiles... > chrome/browser/profiles/profile_impl_io_data.cc:107: > io_data_->data_reduction_proxy_statistics_prefs_->WritePrefs(); > On 2014/09/15 15:11:01, mmenke wrote: > > This is called on the UI thread, but the prefs live on the IO thread, no? > > > > Also, we're not guaranteed that Init() will be called, so this may be NULL. > The prefs live on the UI thread. That's true about Init(), but the pref maps > would just be empty. I'll check the time delta to see if its directly writing at > the beginning of WritePrefs(). > > https://codereview.chromium.org/473723002/diff/400001/chrome/browser/resource... > File chrome/browser/resources/net_internals/bandwidth_view.js (right): > > https://codereview.chromium.org/473723002/diff/400001/chrome/browser/resource... > chrome/browser/resources/net_internals/bandwidth_view.js:80: sessionReceived; > On 2014/09/15 15:11:02, mmenke wrote: > > Why the change? We still include session prefs in kHttpReceivedContentLength, > > don't we? We just don't update the two as often. > > Ah I didn't understand how historic_received_content_length was being updated. > Ya, this won't work, but historic values will only be updated with the session > values added in every hour. > > https://codereview.chromium.org/473723002/diff/400001/chrome/test/data/webui/... > File chrome/test/data/webui/net_internals/bandwidth_view.js (right): > > https://codereview.chromium.org/473723002/diff/400001/chrome/test/data/webui/... > chrome/test/data/webui/net_internals/bandwidth_view.js:113: > this.completeIfDone(); > On 2014/09/15 15:11:02, mmenke wrote: > > Shouldn't onHistoricNetworkStatsChanged still be called (Possibly with 0)? > The test times out since it waits on the historic network stats to change which > only happens every hour. Any alternative suggestions on how to fix this? > > https://codereview.chromium.org/473723002/diff/400001/components/data_reducti... > File > components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc > (right): > > https://codereview.chromium.org/473723002/diff/400001/components/data_reducti... > components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc:14: > #include "base/values.h" > On 2014/09/15 15:11:02, mmenke wrote: > > include base/logging.h for DCHECK. > > Done. > > https://codereview.chromium.org/473723002/diff/400001/components/data_reducti... > components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc:26: > delayed_task_posted_(false){ > On 2014/09/15 15:11:02, mmenke wrote: > > nit: Space before "{" > > Done. > > https://codereview.chromium.org/473723002/diff/400001/components/data_reducti... > components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc:38: > //Init all int64 prefs > On 2014/09/15 15:11:02, mmenke wrote: > > Space between "//" and comments, comments that are sentences should end in > > periods (x2) > > Done. > > https://codereview.chromium.org/473723002/diff/400001/components/data_reducti... > components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc:92: > WritePrefsAndPost(); > On 2014/09/15 15:11:02, mmenke wrote: > > I hadn't looked at this code before, but checking "delayed_task_posted_" > > everywhere seems a bit uglier than necessary. > > > > My suggestion: > > > > Rename WritePrefsAndPost() to "DelayedWritePrefs()" or somesuch, make it do > > nothing if delayed_task_posted_ is true. If it's false, call WritePrefs on a > > timer. Have WritePrefs set delayed_task_posted_ to false (Can also have it > > DCHECK that it's true, in the first place). > > > > That saves a couple lines of code, and makes for slightly simpler code. > > Thanks for this suggestion! > > https://codereview.chromium.org/473723002/diff/400001/components/data_reducti... > components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.cc:158: > } // namespace data_reduction_proxy > On 2014/09/15 15:11:02, mmenke wrote: > > nit: 2 spaces before comment. > > Done. > > https://codereview.chromium.org/473723002/diff/400001/components/data_reducti... > File > components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h > (right): > > https://codereview.chromium.org/473723002/diff/400001/components/data_reducti... > components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h:34: > DataReductionProxyListPrefMap; > On 2014/09/15 15:11:02, mmenke wrote: > > These should be in the private section, since they aren't used publicly. > > Done. > > https://codereview.chromium.org/473723002/diff/400001/components/data_reducti... > components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h:45: > virtual ~DataReductionProxyStatisticsPrefs(); > On 2014/09/15 15:11:02, mmenke wrote: > > virtual not needed, since nothing inherits from this class. > > Done. > > https://codereview.chromium.org/473723002/diff/400001/components/data_reducti... > components/data_reduction_proxy/browser/data_reduction_proxy_statistics_prefs.h:88: > scoped_refptr<base::SequencedTaskRunner> task_runner_; > On 2014/09/15 15:11:02, mmenke wrote: > > Should include ref_counted.h for this. > > Done.
https://codereview.chromium.org/473723002/diff/400001/chrome/test/data/webui/... File chrome/test/data/webui/net_internals/bandwidth_view.js (right): https://codereview.chromium.org/473723002/diff/400001/chrome/test/data/webui/... chrome/test/data/webui/net_internals/bandwidth_view.js:113: this.completeIfDone(); On 2014/09/15 18:19:23, megjablon wrote: > On 2014/09/15 15:11:02, mmenke wrote: > > Shouldn't onHistoricNetworkStatsChanged still be called (Possibly with 0)? > The test times out since it waits on the historic network stats to change which > only happens every hour. Any alternative suggestions on how to fix this? Why can we rely on onSessionNetworkStatsChanged being called, but not onHistoricNetworkStatsChanged? They're both only called every hour, right? We could have net-internals pull the stats directly from the data reduction proxy settings, but that seems like a bit of a pain... That having been said, if the stats only update once an hour, is there even any utility to exposing them via net-internals? May be simplest just to remove the tab (Note: If you guys still find it at all useful, I'm completely fine with keeping it. Not trying to get something useful removed, just uncertain if it is still useful).
On 2014/09/16 14:18:46, mmenke wrote: > https://codereview.chromium.org/473723002/diff/400001/chrome/test/data/webui/... > File chrome/test/data/webui/net_internals/bandwidth_view.js (right): > > https://codereview.chromium.org/473723002/diff/400001/chrome/test/data/webui/... > chrome/test/data/webui/net_internals/bandwidth_view.js:113: > this.completeIfDone(); > On 2014/09/15 18:19:23, megjablon wrote: > > On 2014/09/15 15:11:02, mmenke wrote: > > > Shouldn't onHistoricNetworkStatsChanged still be called (Possibly with 0)? > > The test times out since it waits on the historic network stats to change > which > > only happens every hour. Any alternative suggestions on how to fix this? > > Why can we rely on onSessionNetworkStatsChanged being called, but not > onHistoricNetworkStatsChanged? They're both only called every hour, right? > > We could have net-internals pull the stats directly from the data reduction > proxy settings, but that seems like a bit of a pain... That having been said, > if the stats only update once an hour, is there even any utility to exposing > them via net-internals? May be simplest just to remove the tab (Note: If you > guys still find it at all useful, I'm completely fine with keeping it. Not > trying to get something useful removed, just uncertain if it is still useful). https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/net... The historic network stats are based on the data_reduction_proxy prefs which update only every hour and on shutdown, but the session network stats are simply accumulated in memory as requests come in so those are updated real time. I still think it would be useful to keep this tab because we can see the historic values on start up and can still get the session information. Also, I have filed a bug to fix HistoricNetworkStatsInfoToValue to read from the statistics prefs which should fix all of this to update in real time.
On 2014/09/16 17:11:38, megjablon wrote: > On 2014/09/16 14:18:46, mmenke wrote: > > > https://codereview.chromium.org/473723002/diff/400001/chrome/test/data/webui/... > > File chrome/test/data/webui/net_internals/bandwidth_view.js (right): > > > > > https://codereview.chromium.org/473723002/diff/400001/chrome/test/data/webui/... > > chrome/test/data/webui/net_internals/bandwidth_view.js:113: > > this.completeIfDone(); > > On 2014/09/15 18:19:23, megjablon wrote: > > > On 2014/09/15 15:11:02, mmenke wrote: > > > > Shouldn't onHistoricNetworkStatsChanged still be called (Possibly with 0)? > > > The test times out since it waits on the historic network stats to change > > which > > > only happens every hour. Any alternative suggestions on how to fix this? > > > > Why can we rely on onSessionNetworkStatsChanged being called, but not > > onHistoricNetworkStatsChanged? They're both only called every hour, right? > > > > We could have net-internals pull the stats directly from the data reduction > > proxy settings, but that seems like a bit of a pain... That having been said, > > if the stats only update once an hour, is there even any utility to exposing > > them via net-internals? May be simplest just to remove the tab (Note: If you > > guys still find it at all useful, I'm completely fine with keeping it. Not > > trying to get something useful removed, just uncertain if it is still useful). > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/net... > > The historic network stats are based on the data_reduction_proxy prefs which > update > only every hour and on shutdown, but the session network stats are simply > accumulated > in memory as requests come in so those are updated real time. I still think it > would > be useful to keep this tab because we can see the historic values on start up > and can still get the session information. Also, I have filed a bug to fix > HistoricNetworkStatsInfoToValue to read from the statistics prefs which should > fix all of > this to update in real time. Hey mmenke! The test is good to go now. I had to switch the observer to not ignoreWhenUnchanged because otherwise it wouldn't fire when populating the values from NaN to 0. That was the key to getting the observer called.
LGTM https://codereview.chromium.org/473723002/diff/440001/chrome/test/data/webui/... File chrome/test/data/webui/net_internals/bandwidth_view.js (right): https://codereview.chromium.org/473723002/diff/440001/chrome/test/data/webui/... chrome/test/data/webui/net_internals/bandwidth_view.js:45: g_browser.addHistoricNetworkStatsObserver(this, false); Hrm...Really not sure why this is needed - we should send data to new observers even if it hasn't changed... Not going to block this CL on the issue, or require you guys to investigate...but it's strange we need to do it. https://codereview.chromium.org/473723002/diff/440001/chrome/test/data/webui/... chrome/test/data/webui/net_internals/bandwidth_view.js:123: if (this.isDone() || this.historicVerified) nit: May as well "|| this.sessionVerified" to the onSessionNetworkStatsChanged call as well - not sure if either is needed, but I think it's better to be consistent. https://codereview.chromium.org/473723002/diff/440001/chrome/test/data/webui/... chrome/test/data/webui/net_internals/bandwidth_view.js:125: console.log("checking network stats"); nit: Remove logging.
The CQ bit was checked by megjablon@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/473723002/460001
Message was sent while issue was closed.
Committed patchset #16 (id:460001) as 694212664056ce148d7700c0b5748e338c68e419
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/53f608b55172df7423ea54ab83e9fdfa8fc81f9a Cr-Commit-Position: refs/heads/master@{#295521} |
