Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1006583002:
Reset unreachable data reduction proxy message when data savings is toggled. (Closed)
|
Created:
5 years, 9 months ago by Not at Google. Contact bengr Modified:
5 years, 7 months ago Reviewers:
bengr CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReset data reduction proxy state when data savings is switched off.
Reset unreachable data reduction proxy message when data savings is
switched off. Clear counts of successful requests through proxy and
net errors.
Reset bad proxy list so that the data reduction proxies are retried when Data
Saver is turned back on.
BUG=436326, 438694
Committed: https://crrev.com/5bb90a929d8242a610db5a3dd741a02bb3d10590
Cr-Commit-Position: refs/heads/master@{#330157}
Patch Set 1 #Patch Set 2 : Add tests #Patch Set 3 : Use TestingPrefServiceSimple return type. #Patch Set 4 : Shutdown on UI thread. #
Total comments: 7
Patch Set 5 : Sync to head #Patch Set 6 : Reimplemented for new code pattern. #
Total comments: 4
Patch Set 7 : Reset bypass state #
Total comments: 6
Patch Set 8 : Added test. Reused context in io_data. #Patch Set 9 : Sync to head #
Total comments: 16
Patch Set 10 : Addressed comments #Patch Set 11 : Whitespace #Patch Set 12 : Added comment with bug to rename SetProxyPrefs. #Patch Set 13 : Use vector #Patch Set 14 : Vector initialization. #
Messages
Total messages: 26 (5 generated)
kundaji@chromium.org changed reviewers: + bengr@chromium.org
bengr: components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_protocol_unittest.cc components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.cc components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.h components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats_unittest.cc
kundaji@chromium.org changed reviewers: + sclittle@chromium.org - bengr@chromium.org
sclittle: components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_protocol_unittest.cc components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.cc components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.h components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats_unittest.cc
https://codereview.chromium.org/1006583002/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/1006583002/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.cc:250: void DataReductionProxyUsageStats::OnDataReductionProxyEnabledChanged() { DRPSettings already listens to pref changes, could it just fix it's own unreachable state? Then this could just post ClearRequestCounts on the IO thread. https://codereview.chromium.org/1006583002/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.cc:255: ClearRequestCountsAndNotifyFromUIThread(); Seems like lots of thread jumping here, from UI to IO back to UI. Could this be simplified? https://codereview.chromium.org/1006583002/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.h (right): https://codereview.chromium.org/1006583002/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.h:58: PrefService* pref_service); It seems weird to have UsageStats depend on PrefService, since PrefService is primarily UI-related. Instead of having DRPUsageStats listen to pref changes, it might be better to give DRPSettings a pointer to UsageStats or something, and have it post usage_stats->ClearRequestCounts on the IO thread. What do you think?
https://codereview.chromium.org/1006583002/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/1006583002/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.cc:250: void DataReductionProxyUsageStats::OnDataReductionProxyEnabledChanged() { On 2015/03/13 01:22:46, sclittle wrote: > DRPSettings already listens to pref changes, could it just fix it's own > unreachable state? Then this could just post ClearRequestCounts on the IO > thread. DRPSettings isn't currently listening to pref changes. It does have a reference to PrefService. However, DRPSettings does not have a reference to UsageStats. UsageStats runs on the io thread and initialized by drp_io_data. https://codereview.chromium.org/1006583002/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.cc:255: ClearRequestCountsAndNotifyFromUIThread(); On 2015/03/13 01:22:46, sclittle wrote: > Seems like lots of thread jumping here, from UI to IO back to UI. Could this be > simplified? This class runs on the IO thread whereas pref changes are on UI thread. One option is to move the counts to the UI thread. UI thread is where the counts are actually used. This will get rid of unavailable_ and prev_unavailable since we can directly calculate this from the counts. This is how this class originally used to be. Ben had changed it, and I could consult with him to see if theres a reason to have the current structure. I think thread jumping is fine. We aren't creating new threads, just posting to existing ones, so this should not be inefficient. https://codereview.chromium.org/1006583002/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.h (right): https://codereview.chromium.org/1006583002/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.h:58: PrefService* pref_service); On 2015/03/13 01:22:46, sclittle wrote: > It seems weird to have UsageStats depend on PrefService, since PrefService is > primarily UI-related. Instead of having DRPUsageStats listen to pref changes, it > might be better to give DRPSettings a pointer to UsageStats or something, and > have it post usage_stats->ClearRequestCounts on the IO thread. > > What do you think? How would you provide a DRP_Usage_Stats reference to DRP_Settings? The data gathered by usage stats is used on UI thread. The data used to come from IO thread, but now it comes from both IO thread and UI thread.
https://codereview.chromium.org/1006583002/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/1006583002/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.cc:255: ClearRequestCountsAndNotifyFromUIThread(); On 2015/03/13 17:02:08, kundaji wrote: > On 2015/03/13 01:22:46, sclittle wrote: > > Seems like lots of thread jumping here, from UI to IO back to UI. Could this > be > > simplified? > > This class runs on the IO thread whereas pref changes are on UI thread. One > option is to move the counts to the UI thread. UI thread is where the counts are > actually used. This will get rid of unavailable_ and prev_unavailable since we > can directly calculate this from the counts. This is how this class originally > used to be. Ben had changed it, and I could consult with him to see if theres a > reason to have the current structure. > > I think thread jumping is fine. We aren't creating new threads, just posting to > existing ones, so this should not be inefficient. Thinking about this some more, perhaps the current structure is better since updates to the counts on the IO thread are a lot more frequent than pref changes. So in most cases the jump across threads only happens when 'unavailable_' changes.
kundaji@chromium.org changed reviewers: + bengr@chromium.org - sclittle@chromium.org
On 2015/03/16 16:44:12, kundaji wrote: I think we should do the following: 1) Write a class that tells observers (see ObserverListThreadSafe) when the Data Reduction Proxy is enabled and disabled. The class would be a thin wrapper around a BooleanPrefMember that also checks the command line flag. This class would also have a synchronous interface, e.g., IsDataReductionProxyEnabled. 2) Have this class be owned by DRPIOData and pass a raw pointer to it via InitDataReductionProxySettings. The pref is currently used by UsageStats, NetworkDelegate, and Settings, so be sure to register them as observers where appropriate. Note DRPIOData is guaranteed to be constructed before the call to InitDataReductionProxySettings, and destroyed after DRPSettings is destroyed.
Code structure has changed since last comment. Piggy backing off of pref value change in drp_io_data instead of subscribing to it. PTAL.
Coordinate so that all other state that should be reset is reset. https://codereview.chromium.org/1006583002/diff/90001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats.h (right): https://codereview.chromium.org/1006583002/diff/90001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats.h:83: // Clear request counts unconditionally and notify unavailability if it has Clears request... and notifies... https://codereview.chromium.org/1006583002/diff/90001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc (right): https://codereview.chromium.org/1006583002/diff/90001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:237: bypass_stats_->ClearRequestCountsAndNotifyUnavailability(); Add a comment that says why we do this.
lgtm, modulo the nits.
Implemented resetting bypass state as well. PTAL. https://codereview.chromium.org/1006583002/diff/90001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats.h (right): https://codereview.chromium.org/1006583002/diff/90001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats.h:83: // Clear request counts unconditionally and notify unavailability if it has On 2015/04/24 23:42:05, bengr wrote: > Clears request... and notifies... Done. https://codereview.chromium.org/1006583002/diff/90001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc (right): https://codereview.chromium.org/1006583002/diff/90001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:237: bypass_stats_->ClearRequestCountsAndNotifyUnavailability(); On 2015/04/24 23:42:05, bengr wrote: > Add a comment that says why we do this. Done.
https://codereview.chromium.org/1006583002/diff/110001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc (right): https://codereview.chromium.org/1006583002/diff/110001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:222: const scoped_refptr<net::URLRequestContextGetter>& context_getter) { Why does this need to be passed in? DRPIOData has a url_request_context_getter_. https://codereview.chromium.org/1006583002/diff/110001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:232: proxy_service->ClearBadProxiesCache(); This needs a test. https://codereview.chromium.org/1006583002/diff/110001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:233: bypass_stats_->ClearRequestCountsAndNotifyUnavailability(); This method just calls two methods. Why not just expose those two methods here?
Also, fix the typo in the CL description.
https://codereview.chromium.org/1006583002/diff/110001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc (right): https://codereview.chromium.org/1006583002/diff/110001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:222: const scoped_refptr<net::URLRequestContextGetter>& context_getter) { On 2015/05/07 14:42:34, bengr wrote: > Why does this need to be passed in? DRPIOData has a url_request_context_getter_. Done. https://codereview.chromium.org/1006583002/diff/110001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:232: proxy_service->ClearBadProxiesCache(); On 2015/05/07 14:42:34, bengr wrote: > This needs a test. Done. https://codereview.chromium.org/1006583002/diff/110001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:233: bypass_stats_->ClearRequestCountsAndNotifyUnavailability(); On 2015/05/07 14:42:33, bengr wrote: > This method just calls two methods. Why not just expose those two methods here? Done.
https://codereview.chromium.org/1006583002/diff/150001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats.h (right): https://codereview.chromium.org/1006583002/diff/150001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats.h:82: // Clears request counts unconditionally. Add a blank line above. Also, what are request counts? https://codereview.chromium.org/1006583002/diff/150001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats.h:85: // Checks if the availability status of the data reduction proxy has changed, capitalize Data Reduction Proxy. https://codereview.chromium.org/1006583002/diff/150001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc (right): https://codereview.chromium.org/1006583002/diff/150001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:218: void DataReductionProxyIOData::SetProxyPrefs(bool enabled, This method does nothing to proxy prefs. Please rename it appropriately. https://codereview.chromium.org/1006583002/diff/150001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc (right): https://codereview.chromium.org/1006583002/diff/150001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc:163: drp_test_context->pref_service()->SetBoolean( #include pref_service.h https://codereview.chromium.org/1006583002/diff/150001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc:169: net::ProxyService* proxy_service = #include proxy_service.h https://codereview.chromium.org/1006583002/diff/150001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc:172: net::ProxyInfo proxy_info; #include proxy_info.h https://codereview.chromium.org/1006583002/diff/150001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc:174: net::BoundNetLog bound_net_log; Does this need to be a BoundNetLog?
https://codereview.chromium.org/1006583002/diff/150001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats.h (right): https://codereview.chromium.org/1006583002/diff/150001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats.h:82: // Clears request counts unconditionally. On 2015/05/13 17:48:50, bengr wrote: > Add a blank line above. Also, what are request counts? Done. https://codereview.chromium.org/1006583002/diff/150001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats.h:85: // Checks if the availability status of the data reduction proxy has changed, On 2015/05/13 17:48:50, bengr wrote: > capitalize Data Reduction Proxy. Done. https://codereview.chromium.org/1006583002/diff/150001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc (right): https://codereview.chromium.org/1006583002/diff/150001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:218: void DataReductionProxyIOData::SetProxyPrefs(bool enabled, On 2015/05/13 17:48:50, bengr wrote: > This method does nothing to proxy prefs. Please rename it appropriately. Out of scope of this cl? This method wasn't introduced in this cl and is used in many files and tests which this cl does not touch: https://cs.corp.google.com/#search/&q=SetProxyPrefs%20package:%5Eclankium$&ty.... https://codereview.chromium.org/1006583002/diff/150001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc (right): https://codereview.chromium.org/1006583002/diff/150001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc:163: drp_test_context->pref_service()->SetBoolean( On 2015/05/13 17:48:50, bengr wrote: > #include pref_service.h Done. https://codereview.chromium.org/1006583002/diff/150001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc:169: net::ProxyService* proxy_service = On 2015/05/13 17:48:50, bengr wrote: > #include proxy_service.h Done. https://codereview.chromium.org/1006583002/diff/150001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc:172: net::ProxyInfo proxy_info; On 2015/05/13 17:48:50, bengr wrote: > #include proxy_info.h Done. https://codereview.chromium.org/1006583002/diff/150001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc:174: net::BoundNetLog bound_net_log; On 2015/05/13 17:48:50, bengr wrote: > Does this need to be a BoundNetLog? Yes. MarkProxiesAsBadUntil takes const BoundNetLog&.
lgtm https://codereview.chromium.org/1006583002/diff/150001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc (right): https://codereview.chromium.org/1006583002/diff/150001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:218: void DataReductionProxyIOData::SetProxyPrefs(bool enabled, On 2015/05/13 20:09:16, kundaji wrote: > On 2015/05/13 17:48:50, bengr wrote: > > This method does nothing to proxy prefs. Please rename it appropriately. > > Out of scope of this cl? This method wasn't introduced in this cl and is used in > many files and tests which this cl does not touch: > https://cs.corp.google.com/#search/&q=SetProxyPrefs%20package:%5Eclankium$&ty.... > Fine. Please file a bug so we don't forget to do it.
https://codereview.chromium.org/1006583002/diff/150001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc (right): https://codereview.chromium.org/1006583002/diff/150001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:218: void DataReductionProxyIOData::SetProxyPrefs(bool enabled, On 2015/05/14 18:32:37, bengr wrote: > On 2015/05/13 20:09:16, kundaji wrote: > > On 2015/05/13 17:48:50, bengr wrote: > > > This method does nothing to proxy prefs. Please rename it appropriately. > > > > Out of scope of this cl? This method wasn't introduced in this cl and is used > in > > many files and tests which this cl does not touch: > > > https://cs.corp.google.com/#search/&q=SetProxyPrefs%20package:%5Eclankium$&ty.... > > > > Fine. Please file a bug so we don't forget to do it. Done.
The CQ bit was checked by kundaji@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org Link to the patchset: https://codereview.chromium.org/1006583002/#ps250001 (title: "Vector initialization.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1006583002/250001
Message was sent while issue was closed.
Committed patchset #14 (id:250001)
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/5bb90a929d8242a610db5a3dd741a02bb3d10590 Cr-Commit-Position: refs/heads/master@{#330157} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
