Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
669163003:
Handle disabling data reduction proxy for all platforms. (Closed)
|
Created:
6 years, 1 month ago by Not at Google. Contact bengr Modified:
6 years, 1 month ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionHandle disabling data reduction proxy for all platforms.
Disable data reduction proxy by deleting user level proxy prefs only if
the proxy is known to be a data_reduction_proxy. Remove duplicate
implementation in off_the_record_profile_impl.cc.
BUG=426220
Committed: https://crrev.com/ef948c023b46e6a006d15c186e56740a2d7a9034
Cr-Commit-Position: refs/heads/master@{#302156}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Addressed bengr's comments #
Total comments: 8
Patch Set 3 : Use IsDataReductionProxy()/ #
Total comments: 10
Patch Set 4 : Addressed bengr's changes #
Total comments: 18
Patch Set 5 : Sync to head #Patch Set 6 : Clarified comments. Added kEverythingAllowed. #
Total comments: 2
Patch Set 7 : Renamed to kAllowAllProxyConfigurations. #Patch Set 8 : Sync to head #
Messages
Total messages: 19 (3 generated)
kundaji@chromium.org changed reviewers: + bengr@chromium.org
rlp: chrome/browser/profiles/off_the_record_profile_impl.cc bengr: chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc
https://codereview.chromium.org/669163003/diff/1/chrome/browser/net/spdyproxy... File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc (right): https://codereview.chromium.org/669163003/diff/1/chrome/browser/net/spdyproxy... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc:70: dict->SetBoolean("is_data_reduction_proxy", true); How about "use_with_off_the_record", false https://codereview.chromium.org/669163003/diff/1/chrome/browser/net/spdyproxy... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc:93: dict->GetBoolean("is_data_reduction_proxy", &is_data_reduction_proxy); Please verify (and add a test) that this will get erased if the user specifies a proxy. https://codereview.chromium.org/669163003/diff/1/chrome/browser/net/spdyproxy... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc:94: if (is_data_reduction_proxy) { if (!is_data_reduction_proxy) return; https://codereview.chromium.org/669163003/diff/1/chrome/browser/profiles/off_... File chrome/browser/profiles/off_the_record_profile_impl.cc (right): https://codereview.chromium.org/669163003/diff/1/chrome/browser/profiles/off_... chrome/browser/profiles/off_the_record_profile_impl.cc:139: data_reduction_proxy_configurator->Disable(); Could this be a static function called: void DataReductionproxyConfigurator::DisableOnOffTheRecord( const PrefService* prefs); Otherwise, it's a shame that you need to instantiate an object where the DRP will never be used.
https://codereview.chromium.org/669163003/diff/1/chrome/browser/net/spdyproxy... File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc (right): https://codereview.chromium.org/669163003/diff/1/chrome/browser/net/spdyproxy... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc:70: dict->SetBoolean("is_data_reduction_proxy", true); On 2014/10/27 23:28:01, bengr wrote: > How about "use_with_off_the_record", false Discussed in person. Odd to use a label which is not specific to this file. https://codereview.chromium.org/669163003/diff/1/chrome/browser/net/spdyproxy... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc:93: dict->GetBoolean("is_data_reduction_proxy", &is_data_reduction_proxy); On 2014/10/27 23:28:01, bengr wrote: > Please verify (and add a test) that this will get erased if the user specifies a > proxy. This approach does not work since the dictionary does not necessarily reset when user prefs are set. Switched to using params directly. Added test. Unable to test manually on linux. https://codereview.chromium.org/669163003/diff/1/chrome/browser/net/spdyproxy... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc:94: if (is_data_reduction_proxy) { On 2014/10/27 23:28:01, bengr wrote: > if (!is_data_reduction_proxy) > return; Done. https://codereview.chromium.org/669163003/diff/1/chrome/browser/profiles/off_... File chrome/browser/profiles/off_the_record_profile_impl.cc (right): https://codereview.chromium.org/669163003/diff/1/chrome/browser/profiles/off_... chrome/browser/profiles/off_the_record_profile_impl.cc:139: data_reduction_proxy_configurator->Disable(); On 2014/10/27 23:28:01, bengr wrote: > Could this be a static function called: > void DataReductionproxyConfigurator::DisableOnOffTheRecord( > const PrefService* prefs); > > Otherwise, it's a shame that you need to instantiate an object where the DRP > will never be used. Done.
https://codereview.chromium.org/669163003/diff/20001/chrome/browser/net/spdyp... File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc (right): https://codereview.chromium.org/669163003/diff/20001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc:38: return; I prefer using curly braces when the if clause spans multiple lines. https://codereview.chromium.org/669163003/diff/20001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc:138: bool DataReductionProxyChromeConfigurator::ContainsDataReductionProxy( Why can't you use DRPParams::IsDataReductionProxy? https://codereview.chromium.org/669163003/diff/20001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc:148: bool DataReductionProxyChromeConfigurator::HasProxy(const std::string& server, I don't think you'll need this if you use the method in DRPParams, but if you do, I'd prefer you work with ProxyServers or HostPortPairs instead of strings. https://codereview.chromium.org/669163003/diff/20001/chrome/browser/net/spdyp... File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.h (right): https://codereview.chromium.org/669163003/diff/20001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.h:35: static void Disable(PrefService* prefs); Don't overload the name. Maybe call this DisableInProxyConfigPref, or something like that.
https://codereview.chromium.org/669163003/diff/20001/chrome/browser/net/spdyp... File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc (right): https://codereview.chromium.org/669163003/diff/20001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc:38: return; On 2014/10/28 21:03:41, bengr wrote: > I prefer using curly braces when the if clause spans multiple lines. Done. https://codereview.chromium.org/669163003/diff/20001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc:138: bool DataReductionProxyChromeConfigurator::ContainsDataReductionProxy( On 2014/10/28 21:03:41, bengr wrote: > Why can't you use DRPParams::IsDataReductionProxy? Done. https://codereview.chromium.org/669163003/diff/20001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc:148: bool DataReductionProxyChromeConfigurator::HasProxy(const std::string& server, On 2014/10/28 21:03:41, bengr wrote: > I don't think you'll need this if you use the method in DRPParams, but if you > do, I'd prefer you work with ProxyServers or HostPortPairs instead of strings. Done. https://codereview.chromium.org/669163003/diff/20001/chrome/browser/net/spdyp... File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.h (right): https://codereview.chromium.org/669163003/diff/20001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.h:35: static void Disable(PrefService* prefs); On 2014/10/28 21:03:41, bengr wrote: > Don't overload the name. Maybe call this DisableInProxyConfigPref, or something > like that. Done.
https://codereview.chromium.org/669163003/diff/40001/chrome/browser/net/spdyp... File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc (right): https://codereview.chromium.org/669163003/diff/40001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc:143: data_reduction_proxy::DataReductionProxyParams params(0); How does this work? Nothing is allowed, so IsDataReductionProxy would always be false. https://codereview.chromium.org/669163003/diff/40001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc:160: if (http_proxy_list && !http_proxy_list->IsEmpty() && // It is sufficient to check only the first proxy on the proxy list, because all data reduction proxy configurations start with a data reduction proxy. https://codereview.chromium.org/669163003/diff/40001/chrome/browser/net/spdyp... File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.h (right): https://codereview.chromium.org/669163003/diff/40001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.h:35: static void DisableInProxyConfigPref(PrefService* prefs); Add a comment. https://codereview.chromium.org/669163003/diff/40001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.h:66: static bool ContainsDataReductionProxy(const std::string& server); Add a comment describing what this does. This function would be clearer if it took a ProxyConfig::ProxyRules. https://codereview.chromium.org/669163003/diff/40001/chrome/browser/profiles/... File chrome/browser/profiles/off_the_record_profile_impl.cc (right): https://codereview.chromium.org/669163003/diff/40001/chrome/browser/profiles/... chrome/browser/profiles/off_the_record_profile_impl.cc:135: DataReductionProxyChromeConfigurator::DisableInProxyConfigPref(prefs_); Add a comment to explain what this does.
https://codereview.chromium.org/669163003/diff/40001/chrome/browser/net/spdyp... File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc (right): https://codereview.chromium.org/669163003/diff/40001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc:143: data_reduction_proxy::DataReductionProxyParams params(0); On 2014/10/29 20:58:46, bengr wrote: > How does this work? Nothing is allowed, so IsDataReductionProxy would always be > false. You are right. Added the correct parameters. This was working because allowed() is not checked in the IsDataReductionProxy() method. I added the checks for allowed() as well as alternative_allowed() to IsDataReductionProxy(). https://codereview.chromium.org/669163003/diff/40001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc:160: if (http_proxy_list && !http_proxy_list->IsEmpty() && On 2014/10/29 20:58:46, bengr wrote: > // It is sufficient to check only the first proxy on the proxy list, because all > data reduction proxy configurations start with a data reduction proxy. Done. https://codereview.chromium.org/669163003/diff/40001/chrome/browser/net/spdyp... File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.h (right): https://codereview.chromium.org/669163003/diff/40001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.h:35: static void DisableInProxyConfigPref(PrefService* prefs); On 2014/10/29 20:58:46, bengr wrote: > Add a comment. Done. https://codereview.chromium.org/669163003/diff/40001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.h:66: static bool ContainsDataReductionProxy(const std::string& server); On 2014/10/29 20:58:46, bengr wrote: > Add a comment describing what this does. > > This function would be clearer if it took a ProxyConfig::ProxyRules. Done. Good idea on passing in proxy_rules. https://codereview.chromium.org/669163003/diff/40001/chrome/browser/profiles/... File chrome/browser/profiles/off_the_record_profile_impl.cc (right): https://codereview.chromium.org/669163003/diff/40001/chrome/browser/profiles/... chrome/browser/profiles/off_the_record_profile_impl.cc:135: DataReductionProxyChromeConfigurator::DisableInProxyConfigPref(prefs_); On 2014/10/29 20:58:46, bengr wrote: > Add a comment to explain what this does. Done.
https://codereview.chromium.org/669163003/diff/60001/chrome/browser/net/spdyp... File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc (right): https://codereview.chromium.org/669163003/diff/60001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc:33: base::DictionaryValue* dict = update.Get(); #include "base/values.h" https://codereview.chromium.org/669163003/diff/60001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc:40: if (mode != ProxyModeToString(ProxyPrefs::MODE_FIXED_SERVERS) Add comment: The data reduction proxy uses MODE_FIXED_SERVERS https://codereview.chromium.org/669163003/diff/60001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc:142: bool DataReductionProxyChromeConfigurator::ContainsDataReductionProxy( Again, talk to megjablon@ https://codereview.chromium.org/669163003/diff/60001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc:144: data_reduction_proxy::DataReductionProxyParams params( Suggestion (not required): Add a flag for kEverythingAllowed. https://codereview.chromium.org/669163003/diff/60001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc:150: if (proxy_rules.type != net::ProxyConfig::ProxyRules::TYPE_PROXY_PER_SCHEME) { Curly braces not needed. https://codereview.chromium.org/669163003/diff/60001/chrome/browser/net/spdyp... File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.h (right): https://codereview.chromium.org/669163003/diff/60001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.h:35: // Set the proxy entry in browser preference to use the default system prefs I wouldn't be so specific. How about: // Removes the data reduction proxy configuration from the proxy // preference. This disables use of the data reduction proxy. // This method is public to disable the proxy on incognito. // Disable() should be used otherwise. https://codereview.chromium.org/669163003/diff/60001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.h:68: // Check whether the |proxy_rules| contain any of the data reduction proxies. Coordinate with megjablon@ who is also querying ProxyConfigs. https://codereview.chromium.org/669163003/diff/60001/chrome/browser/profiles/... File chrome/browser/profiles/off_the_record_profile_impl.cc (right): https://codereview.chromium.org/669163003/diff/60001/chrome/browser/profiles/... chrome/browser/profiles/off_the_record_profile_impl.cc:135: // Clear the proxy prefs if and only if the data reduction proxy is specified. proxy pref https://codereview.chromium.org/669163003/diff/60001/components/data_reductio... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc (right): https://codereview.chromium.org/669163003/diff/60001/components/data_reductio... components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:318: if (allowed() && Why is this now necessary?
https://codereview.chromium.org/669163003/diff/60001/chrome/browser/net/spdyp... File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc (right): https://codereview.chromium.org/669163003/diff/60001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc:33: base::DictionaryValue* dict = update.Get(); On 2014/10/29 23:41:25, bengr wrote: > #include "base/values.h" Done. https://codereview.chromium.org/669163003/diff/60001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc:40: if (mode != ProxyModeToString(ProxyPrefs::MODE_FIXED_SERVERS) On 2014/10/29 23:41:25, bengr wrote: > Add comment: > The data reduction proxy uses MODE_FIXED_SERVERS Done. https://codereview.chromium.org/669163003/diff/60001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc:142: bool DataReductionProxyChromeConfigurator::ContainsDataReductionProxy( On 2014/10/29 23:41:25, bengr wrote: > Again, talk to megjablon@ Done. https://codereview.chromium.org/669163003/diff/60001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc:144: data_reduction_proxy::DataReductionProxyParams params( On 2014/10/29 23:41:25, bengr wrote: > Suggestion (not required): Add a flag for kEverythingAllowed. Done. https://codereview.chromium.org/669163003/diff/60001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc:150: if (proxy_rules.type != net::ProxyConfig::ProxyRules::TYPE_PROXY_PER_SCHEME) { On 2014/10/29 23:41:25, bengr wrote: > Curly braces not needed. Done. https://codereview.chromium.org/669163003/diff/60001/chrome/browser/net/spdyp... File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.h (right): https://codereview.chromium.org/669163003/diff/60001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.h:35: // Set the proxy entry in browser preference to use the default system prefs On 2014/10/29 23:41:25, bengr wrote: > I wouldn't be so specific. How about: > > // Removes the data reduction proxy configuration from the proxy > // preference. This disables use of the data reduction proxy. > // This method is public to disable the proxy on incognito. > // Disable() should be used otherwise. Done. https://codereview.chromium.org/669163003/diff/60001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.h:68: // Check whether the |proxy_rules| contain any of the data reduction proxies. On 2014/10/29 23:41:25, bengr wrote: > Coordinate with megjablon@ who is also querying ProxyConfigs. Done. https://codereview.chromium.org/669163003/diff/60001/chrome/browser/profiles/... File chrome/browser/profiles/off_the_record_profile_impl.cc (right): https://codereview.chromium.org/669163003/diff/60001/chrome/browser/profiles/... chrome/browser/profiles/off_the_record_profile_impl.cc:135: // Clear the proxy prefs if and only if the data reduction proxy is specified. On 2014/10/29 23:41:25, bengr wrote: > proxy pref Done. https://codereview.chromium.org/669163003/diff/60001/components/data_reductio... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc (right): https://codereview.chromium.org/669163003/diff/60001/components/data_reductio... components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:318: if (allowed() && On 2014/10/29 23:41:26, bengr wrote: > Why is this now necessary? Discussed in person. Was always needed for correctness. Without this the allowed_ flag is not checked, so no point having it.
kundaji@chromium.org changed reviewers: + rlp@chromium.org
rlp: chrome/browser/profiles/off_the_record_profile_impl.cc bengr: chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.h chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator_unittest.cc components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc components/data_reduction_proxy/core/common/data_reduction_proxy_params.h
profiles lgtm
lgtm, but please address this last comment. https://codereview.chromium.org/669163003/diff/100001/components/data_reducti... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.h (right): https://codereview.chromium.org/669163003/diff/100001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:55: static const unsigned int kEverythingAllowed = kAllowed | kFallbackAllowed | Why does kEverythingAllowed not include everything? Either include everything or renames as kAllowAllProxyConfigurations
https://codereview.chromium.org/669163003/diff/100001/components/data_reducti... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.h (right): https://codereview.chromium.org/669163003/diff/100001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:55: static const unsigned int kEverythingAllowed = kAllowed | kFallbackAllowed | On 2014/10/30 18:13:48, bengr wrote: > Why does kEverythingAllowed not include everything? Either include everything or > renames as kAllowAllProxyConfigurations Done.
The CQ bit was checked by kundaji@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/669163003/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/ef948c023b46e6a006d15c186e56740a2d7a9034 Cr-Commit-Position: refs/heads/master@{#302156} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
