Trigger data reduction proxy unreachable message via on proxy fall back.

Trigger data reduction proxy unreachable message via on proxy fall back.

Use the ChromeNetworkDelegate::OnProxyFallback() callback to keep track of number of times the data reduction proxies fall back because of network errors. Display a warning to users if there are network errors when connecting to the proxy and no successful requests through the proxy.

BUG=401244
Committed: https://crrev.com/a9a4b7da98fa7832024b4f6ae7e595a8b7e4d2b2
Cr-Commit-Position: refs/heads/master@{#296244}

[Not at Google. Contact bengr, 2014-09-12 19:38:19]

bengr:
  components/data_reduction_proxy/browser/data_reduction_proxy_params.cc
  components/data_reduction_proxy/browser/data_reduction_proxy_params.h
  components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc
  components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h
 
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc

mmenke:
  chrome/browser/net/chrome_network_delegate.cc

[Not at Google. Contact bengr, 2014-09-16 17:16:54]

kundaji@chromium.org changed reviewers:
+ bengr@chromium.org, mmenke@chromium.org

[Not at Google. Contact bengr, 2014-09-16 17:17:17]

bengr:
  components/data_reduction_proxy/browser/data_reduction_proxy_params.cc
  components/data_reduction_proxy/browser/data_reduction_proxy_params.h
  components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc
  components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h
 
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc

mmenke:
  chrome/browser/net/chrome_network_delegate.cc

[mmenke, 2014-09-16 17:19:08]

On 2014/09/16 17:17:17, kundaji wrote:
> bengr:
>   components/data_reduction_proxy/browser/data_reduction_proxy_params.cc
>   components/data_reduction_proxy/browser/data_reduction_proxy_params.h
>   components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc
>   components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h
>  
>
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc
> 
> mmenke:
>   chrome/browser/net/chrome_network_delegate.cc

chrome_network_delegate.cc LGTM

[bengr, 2014-09-16 17:22:34]

On 2014/09/16 17:19:08, mmenke wrote:
> On 2014/09/16 17:17:17, kundaji wrote:
> > bengr:
> >   components/data_reduction_proxy/browser/data_reduction_proxy_params.cc
> >   components/data_reduction_proxy/browser/data_reduction_proxy_params.h
> >  
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc
> >   components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h
> >  
> >
>
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc
> > 
> > mmenke:
> >   chrome/browser/net/chrome_network_delegate.cc
> 
> chrome_network_delegate.cc LGTM

Rohit, please rewrite the description to be clearer about what this CL is doing.

[Not at Google. Contact bengr, 2014-09-16 17:31:47]

On 2014/09/16 17:22:34, bengr1 wrote:
> On 2014/09/16 17:19:08, mmenke wrote:
> > On 2014/09/16 17:17:17, kundaji wrote:
> > > bengr:
> > >   components/data_reduction_proxy/browser/data_reduction_proxy_params.cc
> > >   components/data_reduction_proxy/browser/data_reduction_proxy_params.h
> > >  
> components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc
> > >  
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h
> > >  
> > >
> >
>
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc
> > > 
> > > mmenke:
> > >   chrome/browser/net/chrome_network_delegate.cc
> > 
> > chrome_network_delegate.cc LGTM
> 
> Rohit, please rewrite the description to be clearer about what this CL is
doing.

Discussed in person. Updated cl description.

[bengr, 2014-09-16 18:48:57]

https://codereview.chromium.org/568893002/diff/1/components/data_reduction_pr...
File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc
(right):

https://codereview.chromium.org/568893002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:82:
num_successful_requests_through_proxy_(0),
num_ is implied here and with proxy_net_errors_. I think they can be removed.

https://codereview.chromium.org/568893002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:97:
data_reduction_proxy_params_->WasDataReductionProxyUsed(request, NULL)) {
Can params be NULL? If so, check here. If not, DCHECK in the constructor.

https://codereview.chromium.org/568893002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:100:
// To account for the case when the proxy does not work for a little while
Why would it not work for a little while?

https://codereview.chromium.org/568893002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:101:
// and then works fine, we reset the counts occasionally.
How often? What triggers the reset?

https://codereview.chromium.org/568893002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:103:
num_successful_requests_through_proxy_ > 3) {
Define these constants in an anonymous namespace above.

https://codereview.chromium.org/568893002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:127:
num_successful_requests_through_proxy_ == 0);
Define these constants in an anonymous namespace above.

https://codereview.chromium.org/568893002/diff/1/components/data_reduction_pr...
File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h
(right):

https://codereview.chromium.org/568893002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:67:
void OnProxyFallback(const net::ProxyServer& bypassed_proxy,
Please add a comment that describes what this method does.

https://codereview.chromium.org/568893002/diff/1/components/data_reduction_pr...
File
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc
(right):

https://codereview.chromium.org/568893002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc:21:
using net::HostPortPair;
If you have this line here, then you don't need to preface HostPortPair with
net::. In general, I don't like using "using". Would you mind remove all of
these, and adding the namespaces into the code?

[Not at Google. Contact bengr, 2014-09-16 19:34:49]

https://codereview.chromium.org/568893002/diff/1/components/data_reduction_pr...
File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc
(right):

https://codereview.chromium.org/568893002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:82:
num_successful_requests_through_proxy_(0),
On 2014/09/16 18:48:57, bengr1 wrote:
> num_ is implied here and with proxy_net_errors_. I think they can be removed.

Prefer keeping it since it makes it more obvious.

https://codereview.chromium.org/568893002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:97:
data_reduction_proxy_params_->WasDataReductionProxyUsed(request, NULL)) {
On 2014/09/16 18:48:56, bengr1 wrote:
> Can params be NULL? If so, check here. If not, DCHECK in the constructor.

Added DCHECK in constructor.

https://codereview.chromium.org/568893002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:100:
// To account for the case when the proxy does not work for a little while
On 2014/09/16 18:48:57, bengr1 wrote:
> Why would it not work for a little while?

Done.

https://codereview.chromium.org/568893002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:101:
// and then works fine, we reset the counts occasionally.
On 2014/09/16 18:48:56, bengr1 wrote:
> How often? What triggers the reset?

Done.

https://codereview.chromium.org/568893002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:103:
num_successful_requests_through_proxy_ > 3) {
On 2014/09/16 18:48:56, bengr1 wrote:
> Define these constants in an anonymous namespace above.

Done.

https://codereview.chromium.org/568893002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:127:
num_successful_requests_through_proxy_ == 0);
On 2014/09/16 18:48:56, bengr1 wrote:
> Define these constants in an anonymous namespace above.

Done.

https://codereview.chromium.org/568893002/diff/1/components/data_reduction_pr...
File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h
(right):

https://codereview.chromium.org/568893002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:67:
void OnProxyFallback(const net::ProxyServer& bypassed_proxy,
On 2014/09/16 18:48:57, bengr1 wrote:
> Please add a comment that describes what this method does.

Done.

https://codereview.chromium.org/568893002/diff/1/components/data_reduction_pr...
File
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc
(right):

https://codereview.chromium.org/568893002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc:21:
using net::HostPortPair;
On 2014/09/16 18:48:57, bengr1 wrote:
> If you have this line here, then you don't need to preface HostPortPair with
> net::. In general, I don't like using "using". Would you mind remove all of
> these, and adding the namespaces into the code?

Done.

[Not at Google. Contact bengr, 2014-09-18 18:04:48]

Ping:)

[bengr, 2014-09-19 21:28:13]

lgtm, but please address my nits and name changes.

https://codereview.chromium.org/568893002/diff/20001/components/data_reductio...
File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc
(right):

https://codereview.chromium.org/568893002/diff/20001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:31:
const int UNAVAILABLE_THRESHOLD_NUM_PROXY_NET_ERRORS = 0;
Use chromium C++ style here and below:
kUnavailableThresholdNumProxyNetErrors;

I have no idea what these mean from the names though. How about:

const int kMinimumFailedRequestsWhenUnavailable = 1;
const int kMaximumSuccessfulRequestsWhenUnavailable = 0;
const int kMaximumSuccessfulRequestsBeforeReset = 4;

And update all your comparisons to avoid inducing any off-by-one errors with
these changes.

https://codereview.chromium.org/568893002/diff/20001/components/data_reductio...
File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h
(right):

https://codereview.chromium.org/568893002/diff/20001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:67:
// Called by |ChromeNetworkDelegate| when a proxy is put into bad proxy list.
nit: "into the"

https://codereview.chromium.org/568893002/diff/20001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:68:
// Used to track when data reduction proxy falls back.
nit: "when the"

https://codereview.chromium.org/568893002/diff/20001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:112:
// Count of successful requests through data reduction proxy.
nit: "through the"

https://codereview.chromium.org/568893002/diff/20001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:113:
unsigned long num_successful_requests_through_proxy_;
I don't like "num". If you don't like the removal of it, change to:
successful_requests_through_proxy_count_ and proxy_net_errors_count_.

[Not at Google. Contact bengr, 2014-09-22 21:46:58]

https://codereview.chromium.org/568893002/diff/20001/components/data_reductio...
File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc
(right):

https://codereview.chromium.org/568893002/diff/20001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:31:
const int UNAVAILABLE_THRESHOLD_NUM_PROXY_NET_ERRORS = 0;
On 2014/09/19 21:28:12, bengr1 wrote:
> Use chromium C++ style here and below:
> kUnavailableThresholdNumProxyNetErrors;
> 
> I have no idea what these mean from the names though. How about:
> 
> const int kMinimumFailedRequestsWhenUnavailable = 1;
> const int kMaximumSuccessfulRequestsWhenUnavailable = 0;
> const int kMaximumSuccessfulRequestsBeforeReset = 4;
> 
> And update all your comparisons to avoid inducing any off-by-one errors with
> these changes.

Done.

https://codereview.chromium.org/568893002/diff/20001/components/data_reductio...
File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h
(right):

https://codereview.chromium.org/568893002/diff/20001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:67:
// Called by |ChromeNetworkDelegate| when a proxy is put into bad proxy list.
On 2014/09/19 21:28:13, bengr1 wrote:
> nit: "into the"

Done.

https://codereview.chromium.org/568893002/diff/20001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:68:
// Used to track when data reduction proxy falls back.
On 2014/09/19 21:28:12, bengr1 wrote:
> nit: "when the"

Done.

https://codereview.chromium.org/568893002/diff/20001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:112:
// Count of successful requests through data reduction proxy.
On 2014/09/19 21:28:13, bengr1 wrote:
> nit: "through the"

Done.

https://codereview.chromium.org/568893002/diff/20001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:113:
unsigned long num_successful_requests_through_proxy_;
On 2014/09/19 21:28:12, bengr1 wrote:
> I don't like "num". If you don't like the removal of it, change to:
> successful_requests_through_proxy_count_ and proxy_net_errors_count_.

Done.

Would have preferred count_successful_requests_through_proxy_, because that way
I don't have to read the entire variable name to realize its a count. But
changing to what you suggested for now.

[Not at Google. Contact bengr, 2014-09-23 18:26:17]

The CQ bit was checked by kundaji@chromium.org

[commit-bot: I haz the power, 2014-09-23 20:31:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/568893002/80001

[commit-bot: I haz the power, 2014-09-23 21:11:05]

Committed patchset #5 (id:80001) as 4019f5a87b5acb40588fd0d8ff951eee426e2ff0

[commit-bot: I haz the power, 2014-09-23 21:12:16]

Patchset 5 (id:??) landed as
https://crrev.com/a9a4b7da98fa7832024b4f6ae7e595a8b7e4d2b2
Cr-Commit-Position: refs/heads/master@{#296244}