Keep track of network error in ProxyRetryInfo.

Keep track of network error in ProxyRetryInfo.

Use it to log error type in BypassOneNetworkError UMA.
This fixes DataReductionProxy.BypassOnNetworkError UMA recording bug.

BUG=395769
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290054

[bengr, 2014-08-14 00:52:52]

I think this makes sense. I'll read it in more detail soon.

https://codereview.chromium.org/473513002/diff/40001/net/base/network_delegate.h
File net/base/network_delegate.h (right):

https://codereview.chromium.org/473513002/diff/40001/net/base/network_delegat...
net/base/network_delegate.h:67: virtual void NotifyProxyFallback(const
ProxyServer& bad_proxy,
This should not be virtual.

https://codereview.chromium.org/473513002/diff/40001/net/base/network_delegat...
net/base/network_delegate.h:137: // the network error encountered, if any.
Clarify that net_error == 0 indicates that the proxy fell back for a reason
other than a network error.

[Not at Google. Contact bengr, 2014-08-14 01:28:32]

https://codereview.chromium.org/473513002/diff/40001/net/base/network_delegate.h
File net/base/network_delegate.h (right):

https://codereview.chromium.org/473513002/diff/40001/net/base/network_delegat...
net/base/network_delegate.h:67: virtual void NotifyProxyFallback(const
ProxyServer& bad_proxy,
On 2014/08/14 00:52:52, bengr1 wrote:
> This should not be virtual.

Done

https://codereview.chromium.org/473513002/diff/40001/net/base/network_delegat...
net/base/network_delegate.h:137: // the network error encountered, if any.
On 2014/08/14 00:52:52, bengr1 wrote:
> Clarify that net_error == 0 indicates that the proxy fell back for a reason
> other than a network error.

Done.

[Not at Google. Contact bengr, 2014-08-14 01:31:42]

bengr:
  components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc
  components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h
darin:
  chrome/browser/net/chrome_network_delegate.cc
  chrome/browser/net/chrome_network_delegate.h
  google_apis/gcm/engine/connection_factory_impl.cc
  jingle/glue/proxy_resolving_client_socket.cc
  net/base/network_delegate.cc
  net/base/network_delegate.h
  net/http/http_stream_factory_impl_job.cc
  net/proxy/proxy_info.cc
  net/proxy/proxy_info.h
  net/proxy/proxy_info_unittest.cc
  net/proxy/proxy_list.cc
  net/proxy/proxy_list.h
  net/proxy/proxy_list_unittest.cc
  net/proxy/proxy_retry_info.h
  net/proxy/proxy_service.cc
  net/proxy/proxy_service.h
  net/proxy/proxy_service_unittest.cc
mef:
  chrome/browser/net/chrome_network_delegate.cc
  chrome/browser/net/chrome_network_delegate.h
  net/base/network_delegate.cc
  net/base/network_delegate.h
  net/http/http_stream_factory_impl_job.cc
  net/proxy/proxy_info.cc
  net/proxy/proxy_info.h
  net/proxy/proxy_info_unittest.cc
  net/proxy/proxy_list.cc
  net/proxy/proxy_list.h
  net/proxy/proxy_list_unittest.cc
  net/proxy/proxy_retry_info.h
  net/proxy/proxy_service.cc
  net/proxy/proxy_service.h
  net/proxy/proxy_service_unittest.cc

[mef, 2014-08-14 17:52:56]

Unfortunately I'm not going to have bandwidth for review until 8/25.

[darin (slow to review), 2014-08-14 17:59:16]

I reviewed the changes outside of net/, and those LGTM.

[Ryan Sleevi, 2014-08-14 21:14:39]

I took a quick scan, and will try to take a more detailed review later. I think
the approach is mostly reasonable (in that it doesn't introduce any new
coupling, which is always my biggest concern), but a lot can be done for
documenting things.

My biggest concern is the commenting. This code would certainly benefit from
clear comments. To be precise, it's not desirable to describe so much "what this
code does" (especially in the header, where that's often a violation of
abstraction), but more in line of how it can/should be used. Please see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
for further details on what makes a "good comment".

CL description wise, I'm a bit biased, but it "seems" your big change is to how
//net works, and that 'incidentally' fixes a bug in the DataReductionProxy code.

It's important to list both, but it's often better to list in the sense of:

Here's the big change
Here's why I did it

That can signal to reviewers less informed/contextually aware of the 'why', but
are interested in the 'big change'.

In theory, you could split this across two CLs - one that introduces the
capability (and has unit tests for it), and one that uses the capability to fix
the bug. I'm not requiring you to do this, but I can say it's saved my bacon
quite a few times when trying to understand code, so something to think about.

https://codereview.chromium.org/473513002/diff/100001/net/proxy/proxy_list.h
File net/proxy/proxy_list.h (right):

https://codereview.chromium.org/473513002/diff/100001/net/proxy/proxy_list.h#...
net/proxy/proxy_list.h:83: // there is another server available in the list.
Add a param = update comments

https://codereview.chromium.org/473513002/diff/100001/net/proxy/proxy_list.h#...
net/proxy/proxy_list.h:100: const int net_error,
Add a param = update comments

also, no const

https://codereview.chromium.org/473513002/diff/100001/net/proxy/proxy_list.h#...
net/proxy/proxy_list.h:111: const int net_error,
add a... well, you get the idea :)

https://codereview.chromium.org/473513002/diff/100001/net/proxy/proxy_retry_i...
File net/proxy/proxy_retry_info.h (right):

https://codereview.chromium.org/473513002/diff/100001/net/proxy/proxy_retry_i...
net/proxy/proxy_retry_info.h:28: // The network error we received when we tried
to use this proxy.
Word it without the pronoun and it becomes clearer and more concise (
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/NH-S6KCkr2M/yv0z3...
)

(Yes, I know of the existing uses in this file, but no reason not to have better
comments)

// The network error received when this proxy failed.

https://codereview.chromium.org/473513002/diff/100001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

https://codereview.chromium.org/473513002/diff/100001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:1252: ProxyRetryInfo proxy_retry_info = iter->second;
Unnecessary copy?

https://codereview.chromium.org/473513002/diff/100001/net/proxy/proxy_service.h
File net/proxy/proxy_service.h (right):

https://codereview.chromium.org/473513002/diff/100001/net/proxy/proxy_service...
net/proxy/proxy_service.h:171: // failed) for this request will be marked as
bad.
Add a param = update comment

[Not at Google. Contact bengr, 2014-08-14 23:45:22]

Thank you very much for the quick reviews, darin and sleevi.

https://codereview.chromium.org/473513002/diff/100001/net/proxy/proxy_list.h
File net/proxy/proxy_list.h (right):

https://codereview.chromium.org/473513002/diff/100001/net/proxy/proxy_list.h#...
net/proxy/proxy_list.h:83: // there is another server available in the list.
On 2014/08/14 21:14:39, Ryan Sleevi wrote:
> Add a param = update comments

Done. Took me a little while to figure out what you meant:)

https://codereview.chromium.org/473513002/diff/100001/net/proxy/proxy_list.h#...
net/proxy/proxy_list.h:100: const int net_error,
On 2014/08/14 21:14:39, Ryan Sleevi wrote:
> Add a param = update comments
> 
> also, no const

Done.

Removed const from net_error everywhere. Can you please clarify why I should not
use const here. Did not see anything in the style guide about this.

https://codereview.chromium.org/473513002/diff/100001/net/proxy/proxy_list.h#...
net/proxy/proxy_list.h:111: const int net_error,
On 2014/08/14 21:14:39, Ryan Sleevi wrote:
> add a... well, you get the idea :)

Done.

https://codereview.chromium.org/473513002/diff/100001/net/proxy/proxy_retry_i...
File net/proxy/proxy_retry_info.h (right):

https://codereview.chromium.org/473513002/diff/100001/net/proxy/proxy_retry_i...
net/proxy/proxy_retry_info.h:28: // The network error we received when we tried
to use this proxy.
On 2014/08/14 21:14:39, Ryan Sleevi wrote:
> Word it without the pronoun and it becomes clearer and more concise (
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/NH-S6KCkr2M/yv0z3...
> )
> 
> (Yes, I know of the existing uses in this file, but no reason not to have
better
> comments)
> 
> // The network error received when this proxy failed.

Done.

https://codereview.chromium.org/473513002/diff/100001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

https://codereview.chromium.org/473513002/diff/100001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:1252: ProxyRetryInfo proxy_retry_info = iter->second;
On 2014/08/14 21:14:39, Ryan Sleevi wrote:
> Unnecessary copy?

Fixed.

https://codereview.chromium.org/473513002/diff/100001/net/proxy/proxy_service.h
File net/proxy/proxy_service.h (right):

https://codereview.chromium.org/473513002/diff/100001/net/proxy/proxy_service...
net/proxy/proxy_service.h:171: // failed) for this request will be marked as
bad.
On 2014/08/14 21:14:39, Ryan Sleevi wrote:
> Add a param = update comment

Done.

[Ryan Sleevi, 2014-08-14 23:54:03]

https://codereview.chromium.org/473513002/diff/100001/net/proxy/proxy_list.h
File net/proxy/proxy_list.h (right):

https://codereview.chromium.org/473513002/diff/100001/net/proxy/proxy_list.h#...
net/proxy/proxy_list.h:100: const int net_error,
On 2014/08/14 23:45:22, kundaji wrote:
> On 2014/08/14 21:14:39, Ryan Sleevi wrote:
> > Add a param = update comments
> > 
> > also, no const
> 
> Done.
> 
> Removed const from net_error everywhere. Can you please clarify why I should
not
> use const here. Did not see anything in the style guide about this.

It's an int. It's always passed by value. Making it const doesn't really buy you
anything, API contract wise.

[Not at Google. Contact bengr, 2014-08-14 23:55:05]

Updated the cl description. Thanks for the information and relavant links!

[Not at Google. Contact bengr, 2014-08-14 23:59:59]

https://codereview.chromium.org/473513002/diff/100001/net/proxy/proxy_list.h
File net/proxy/proxy_list.h (right):

https://codereview.chromium.org/473513002/diff/100001/net/proxy/proxy_list.h#...
net/proxy/proxy_list.h:100: const int net_error,
On 2014/08/14 23:54:03, Ryan Sleevi wrote:
> On 2014/08/14 23:45:22, kundaji wrote:
> > On 2014/08/14 21:14:39, Ryan Sleevi wrote:
> > > Add a param = update comments
> > > 
> > > also, no const
> > 
> > Done.
> > 
> > Removed const from net_error everywhere. Can you please clarify why I should
> not
> > use const here. Did not see anything in the style guide about this.
> 
> It's an int. It's always passed by value. Making it const doesn't really buy
you
> anything, API contract wise.

It does enforce that the value will not change within the method. Some codebases
within Google enforce that anything that can be marked with const, must be. I
personally don't feel strongly one way or the other.

[Ryan Sleevi, 2014-08-15 00:02:24]

https://codereview.chromium.org/473513002/diff/160001/net/base/network_delega...
File net/base/network_delegate.h (right):

https://codereview.chromium.org/473513002/diff/160001/net/base/network_delega...
net/base/network_delegate.h:71: int net_error);
format.

https://codereview.chromium.org/473513002/diff/160001/net/base/network_delega...
net/base/network_delegate.h:142: // for a reason other than network error.
What's a reason for fallback OTHER than a network error? (Try to mention it in
terms of NetworkDelegate/ProxyService, rather than if it's specific to data
reduction proxy)

I'm guessing this is something like
"and OK if the fallback was for a reason other than a network error (e.g. a
delegate indicated to try a different proxy)" (I don't know if a delegate can do
that, but that's the sort of level of abstractness)

https://codereview.chromium.org/473513002/diff/160001/net/proxy/proxy_info.h
File net/proxy/proxy_info.h (right):

https://codereview.chromium.org/473513002/diff/160001/net/proxy/proxy_info.h#...
net/proxy/proxy_info.h:120: bool Fallback(const int net_error, const
BoundNetLog& net_log);
no const

https://codereview.chromium.org/473513002/diff/160001/net/proxy/proxy_info_un...
File net/proxy/proxy_info_unittest.cc (right):

https://codereview.chromium.org/473513002/diff/160001/net/proxy/proxy_info_un...
net/proxy/proxy_info_unittest.cc:6: #include "net/base/net_util.h"
Why do you need to introduce net_util here?

https://codereview.chromium.org/473513002/diff/160001/net/proxy/proxy_list.cc
File net/proxy/proxy_list.cc (right):

https://codereview.chromium.org/473513002/diff/160001/net/proxy/proxy_list.cc...
net/proxy/proxy_list.cc:149: const int net_error,
noconst

https://codereview.chromium.org/473513002/diff/160001/net/proxy/proxy_list.cc...
net/proxy/proxy_list.cc:187: const int net_error,
noconst

https://codereview.chromium.org/473513002/diff/160001/net/proxy/proxy_list.cc...
net/proxy/proxy_list.cc:213: const int net_error,
noconst

https://codereview.chromium.org/473513002/diff/160001/net/proxy/proxy_list.h
File net/proxy/proxy_list.h (right):

https://codereview.chromium.org/473513002/diff/160001/net/proxy/proxy_list.h#...
net/proxy/proxy_list.h:83: // should contain the network error encountered when
this proxy was tried if
"tried, if any"

https://codereview.chromium.org/473513002/diff/160001/net/proxy/proxy_list.h#...
net/proxy/proxy_list.h:85: // be passed in. Returns true if there is another
server available in the
same comment re: examples of non-error fallbacks

https://codereview.chromium.org/473513002/diff/160001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

https://codereview.chromium.org/473513002/diff/160001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:1230: OK,
This is a little weird.

When would this be called? Should we be passing network error into this? Why is
OK the right answer?

I don't have a good answer, but a little explanation would help to make sure
this is good. At the least, we should add a comment explaining that here, but
before you do so, could you explain on the CL so we can make sure it's the right
API?

https://codereview.chromium.org/473513002/diff/160001/net/proxy/proxy_service...
File net/proxy/proxy_service_unittest.cc (right):

https://codereview.chromium.org/473513002/diff/160001/net/proxy/proxy_service...
net/proxy/proxy_service_unittest.cc:210: proxy_fallback_net_error_(0) {
should this be "OK" instead of 0 ?

https://codereview.chromium.org/473513002/diff/160001/net/proxy/proxy_service...
net/proxy/proxy_service_unittest.cc:215: int net_error) OVERRIDE {
Does this fit on one line now?

[Ryan Sleevi, 2014-08-15 00:03:42]

On 2014/08/14 23:59:59, kundaji wrote:
> It does enforce that the value will not change within the method. Some
codebases
> within Google enforce that anything that can be marked with const, must be. I
> personally don't feel strongly one way or the other.

Yeah. I'm a huge fan of const. I just see it as general overkill for POD values.
Area of debate, for sure.

[bengr, 2014-08-15 01:21:40]

On 2014/08/15 00:03:47, Ryan Sleevi wrote:

lgtm once you adddress comments from sleevi@. It would be nice (but not
necessary in this CL) to track application level reasons for bypass.

[Not at Google. Contact bengr, 2014-08-15 17:27:00]

Thanks again for the quick review!

https://codereview.chromium.org/473513002/diff/160001/net/base/network_delega...
File net/base/network_delegate.h (right):

https://codereview.chromium.org/473513002/diff/160001/net/base/network_delega...
net/base/network_delegate.h:71: int net_error);
On 2014/08/15 00:02:23, Ryan Sleevi wrote:
> format.

Done.

https://codereview.chromium.org/473513002/diff/160001/net/base/network_delega...
net/base/network_delegate.h:142: // for a reason other than network error.
On 2014/08/15 00:02:23, Ryan Sleevi wrote:
> What's a reason for fallback OTHER than a network error? (Try to mention it in
> terms of NetworkDelegate/ProxyService, rather than if it's specific to data
> reduction proxy)
> 
> I'm guessing this is something like
> "and OK if the fallback was for a reason other than a network error (e.g. a
> delegate indicated to try a different proxy)" (I don't know if a delegate can
do
> that, but that's the sort of level of abstractness)

Added clarification. Please check that it provides the right amount of
information.

https://codereview.chromium.org/473513002/diff/160001/net/proxy/proxy_info.h
File net/proxy/proxy_info.h (right):

https://codereview.chromium.org/473513002/diff/160001/net/proxy/proxy_info.h#...
net/proxy/proxy_info.h:120: bool Fallback(const int net_error, const
BoundNetLog& net_log);
On 2014/08/15 00:02:23, Ryan Sleevi wrote:
> no const

Done. Removed from .cc as well.

https://codereview.chromium.org/473513002/diff/160001/net/proxy/proxy_info_un...
File net/proxy/proxy_info_unittest.cc (right):

https://codereview.chromium.org/473513002/diff/160001/net/proxy/proxy_info_un...
net/proxy/proxy_info_unittest.cc:6: #include "net/base/net_util.h"
On 2014/08/15 00:02:23, Ryan Sleevi wrote:
> Why do you need to introduce net_util here?

Done.

https://codereview.chromium.org/473513002/diff/160001/net/proxy/proxy_list.cc
File net/proxy/proxy_list.cc (right):

https://codereview.chromium.org/473513002/diff/160001/net/proxy/proxy_list.cc...
net/proxy/proxy_list.cc:149: const int net_error,
On 2014/08/15 00:02:23, Ryan Sleevi wrote:
> noconst

Done.

https://codereview.chromium.org/473513002/diff/160001/net/proxy/proxy_list.cc...
net/proxy/proxy_list.cc:187: const int net_error,
On 2014/08/15 00:02:23, Ryan Sleevi wrote:
> noconst

Done.

https://codereview.chromium.org/473513002/diff/160001/net/proxy/proxy_list.cc...
net/proxy/proxy_list.cc:213: const int net_error,
On 2014/08/15 00:02:23, Ryan Sleevi wrote:
> noconst

Done.

https://codereview.chromium.org/473513002/diff/160001/net/proxy/proxy_list.h
File net/proxy/proxy_list.h (right):

https://codereview.chromium.org/473513002/diff/160001/net/proxy/proxy_list.h#...
net/proxy/proxy_list.h:83: // should contain the network error encountered when
this proxy was tried if
On 2014/08/15 00:02:23, Ryan Sleevi wrote:
> "tried, if any"

Done.

https://codereview.chromium.org/473513002/diff/160001/net/proxy/proxy_list.h#...
net/proxy/proxy_list.h:85: // be passed in. Returns true if there is another
server available in the
On 2014/08/15 00:02:24, Ryan Sleevi wrote:
> same comment re: examples of non-error fallbacks

Done.

https://codereview.chromium.org/473513002/diff/160001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

https://codereview.chromium.org/473513002/diff/160001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:1230: OK,
On 2014/08/15 00:02:24, Ryan Sleevi wrote:
> This is a little weird.
> 
> When would this be called? Should we be passing network error into this? Why
is
> OK the right answer?
> 
> I don't have a good answer, but a little explanation would help to make sure
> this is good. At the least, we should add a comment explaining that here, but
> before you do so, could you explain on the CL so we can make sure it's the
right
> API?

Clarified in the .h file. MarkProxiesAsBadUntil is called (by components) to add
proxies to the bad proxy list to prevent them being used for reasons other than
a network error. This behavior is not being change by this cl.

https://codereview.chromium.org/473513002/diff/160001/net/proxy/proxy_service...
File net/proxy/proxy_service_unittest.cc (right):

https://codereview.chromium.org/473513002/diff/160001/net/proxy/proxy_service...
net/proxy/proxy_service_unittest.cc:210: proxy_fallback_net_error_(0) {
On 2014/08/15 00:02:24, Ryan Sleevi wrote:
> should this be "OK" instead of 0 ?

Done.

https://codereview.chromium.org/473513002/diff/160001/net/proxy/proxy_service...
net/proxy/proxy_service_unittest.cc:215: int net_error) OVERRIDE {
On 2014/08/15 00:02:24, Ryan Sleevi wrote:
> Does this fit on one line now?

Done.

[Ryan Sleevi, 2014-08-15 18:02:36]

https://codereview.chromium.org/473513002/diff/180001/net/base/network_delega...
File net/base/network_delegate.h (right):

https://codereview.chromium.org/473513002/diff/180001/net/base/network_delega...
net/base/network_delegate.h:143: // proxy to the retry list so that it is not
used).
Great, the overall comments have helped better understand how/when this is used
(which is a point of comments :D)

I think the e.g. can be simplified now

"and OK if the fallback was for a reason other than a network error (e.g. the
proxy service was explicitly directed to skip a proxy)"

https://codereview.chromium.org/473513002/diff/180001/net/proxy/proxy_info.h
File net/proxy/proxy_info.h (right):

https://codereview.chromium.org/473513002/diff/180001/net/proxy/proxy_info.h#...
net/proxy/proxy_info.h:119: // available to try in proxy list_.
Update comment :)

https://codereview.chromium.org/473513002/diff/180001/net/proxy/proxy_list.h
File net/proxy/proxy_list.h (right):

https://codereview.chromium.org/473513002/diff/180001/net/proxy/proxy_list.h#...
net/proxy/proxy_list.h:86: // is not used). Returns true if there is another
server available in the
(e.g. for reasons such as local policy)

https://codereview.chromium.org/473513002/diff/180001/net/proxy/proxy_list.h#...
net/proxy/proxy_list.h:101: // in.
I'm having trouble parsing this last sentence.

Perhaps
|net_error| should contain the network error countered when this proxy was
tried, or OK if the proxy retry info is being updated for a non-network related
reason (e.g. local policy)

https://codereview.chromium.org/473513002/diff/180001/net/proxy/proxy_list.h#...
net/proxy/proxy_list.h:116: // for a reason other than a network error, then
|OK| should be passed in.
Ditto reword

https://codereview.chromium.org/473513002/diff/180001/net/proxy/proxy_list_un...
File net/proxy/proxy_list_unittest.cc (right):

https://codereview.chromium.org/473513002/diff/180001/net/proxy/proxy_list_un...
net/proxy/proxy_list_unittest.cc:225: ERR_PROXY_CONNECTION_FAILED,
Should you also add a test to make sure your "OK" path doesn't cause any special
behaviours

[Not at Google. Contact bengr, 2014-08-15 19:56:54]

https://codereview.chromium.org/473513002/diff/180001/net/base/network_delega...
File net/base/network_delegate.h (right):

https://codereview.chromium.org/473513002/diff/180001/net/base/network_delega...
net/base/network_delegate.h:143: // proxy to the retry list so that it is not
used).
On 2014/08/15 18:02:36, Ryan Sleevi wrote:
> Great, the overall comments have helped better understand how/when this is
used
> (which is a point of comments :D)
> 
> I think the e.g. can be simplified now
> 
> "and OK if the fallback was for a reason other than a network error (e.g. the
> proxy service was explicitly directed to skip a proxy)"

Done.

https://codereview.chromium.org/473513002/diff/180001/net/proxy/proxy_info.h
File net/proxy/proxy_info.h (right):

https://codereview.chromium.org/473513002/diff/180001/net/proxy/proxy_info.h#...
net/proxy/proxy_info.h:119: // available to try in proxy list_.
On 2014/08/15 18:02:36, Ryan Sleevi wrote:
> Update comment :)

Done.

https://codereview.chromium.org/473513002/diff/180001/net/proxy/proxy_list.h
File net/proxy/proxy_list.h (right):

https://codereview.chromium.org/473513002/diff/180001/net/proxy/proxy_list.h#...
net/proxy/proxy_list.h:86: // is not used). Returns true if there is another
server available in the
On 2014/08/15 18:02:36, Ryan Sleevi wrote:
> (e.g. for reasons such as local policy)

Done.

https://codereview.chromium.org/473513002/diff/180001/net/proxy/proxy_list.h#...
net/proxy/proxy_list.h:101: // in.
On 2014/08/15 18:02:36, Ryan Sleevi wrote:
> I'm having trouble parsing this last sentence.
> 
> Perhaps
> |net_error| should contain the network error countered when this proxy was
> tried, or OK if the proxy retry info is being updated for a non-network
related
> reason (e.g. local policy)

Done.

https://codereview.chromium.org/473513002/diff/180001/net/proxy/proxy_list.h#...
net/proxy/proxy_list.h:116: // for a reason other than a network error, then
|OK| should be passed in.
On 2014/08/15 18:02:36, Ryan Sleevi wrote:
> Ditto reword

Done.

https://codereview.chromium.org/473513002/diff/180001/net/proxy/proxy_list_un...
File net/proxy/proxy_list_unittest.cc (right):

https://codereview.chromium.org/473513002/diff/180001/net/proxy/proxy_list_un...
net/proxy/proxy_list_unittest.cc:225: ERR_PROXY_CONNECTION_FAILED,
On 2014/08/15 18:02:36, Ryan Sleevi wrote:
> Should you also add a test to make sure your "OK" path doesn't cause any
special
> behaviours

Added a test for the OK case.

[Ryan Sleevi, 2014-08-15 21:38:38]

//net LGTM. Thanks.

[Not at Google. Contact bengr, 2014-08-15 22:15:10]

The CQ bit was checked by kundaji@chromium.org

[commit-bot: I haz the power, 2014-08-15 22:16:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kundaji@chromium.org/473513002/220001

[commit-bot: I haz the power, 2014-08-15 23:50:23]

Committed patchset #12 (220001) as 290054