Recover from bad proxy settings pointing to non-proxy servers that reply anyway.

Recover from bad proxy settings pointing to non-proxy servers that reply anyway.

Added tests for proxy error recovery.

BUG=chromium-os:20775
TEST=DeviceManagementServiceTest.*. Steps described in the bug now work as expected.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=103172

[Joao da Silva, 2011-09-27 15:01:56]

Please review, thanks!

@mnissler: main review, policy/ changes
@willchan: content/ changes, mainly [Test]URLFetcher
@akalin: owner review for net/tools/testserver/

[willchan no longer on Chromium, 2011-09-27 19:04:27]

Eric, thoughts here? I'm worried about creep into URLFetcher, since it's already
a huge mess, but this seems mostly benign. I'm leaning towards letting it in.
What say you?

[akalin, 2011-09-27 19:07:54]

Willchan should be sufficient for net/tools/testserver OWNERS (note that the
net/tools/testserver/OWNERS is additive -- I'm only in there for sync changes).

Removing myself as reviewer as I'm not too familiar with the non-sync parts of
the testserver code (Willchan should suffice).

[eroman, 2011-09-28 01:56:21]

@willchan: i am fine with the changes to url_fetcher

[willchan no longer on Chromium, 2011-09-28 02:02:11]

OK, LGTM.

On Tue, Sep 27, 2011 at 6:56 PM, <eroman@chromium.org> wrote:

> @willchan: i am fine with the changes to url_fetcher
>
>
>
http://codereview.chromium.**org/8054013/<http://codereview.chromium.org/8054...
>

[Mattias Nissler (ping if slow), 2011-09-28 09:06:17]

LGTM with some nits and a request for parental guidance. Will, Eric, can you
weigh in on the question I put in the last comment regarding passing refcounted
stuff to functions?

http://codereview.chromium.org/8054013/diff/1/chrome/browser/policy/device_ma...
File chrome/browser/policy/device_management_service.cc (right):

http://codereview.chromium.org/8054013/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service.cc:246: LOG(WARNING) << "Got bad
mime-type in response from dmserver that was "
Please also put a comment here explaining what problem we're trying to solve.

http://codereview.chromium.org/8054013/diff/1/chrome/browser/policy/device_ma...
File chrome/browser/policy/device_management_service_unittest.cc (right):

http://codereview.chromium.org/8054013/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service_unittest.cc:555:
EXPECT_TRUE((fetcher->load_flags() & net::LOAD_BYPASS_PROXY) != 0);
you can drop the != 0

http://codereview.chromium.org/8054013/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service_unittest.cc:559: // No calls
should have been made to the mock.
Other code uses EXPECT_CALL().Times(0) instead of StrictMock to achieve this,
and the gmock documentation advises against using StrictMock if you can achieve
what you want with regular mocks and expectations.

http://codereview.chromium.org/8054013/diff/1/content/common/net/url_fetcher.cc
File content/common/net/url_fetcher.cc (right):

http://codereview.chromium.org/8054013/diff/1/content/common/net/url_fetcher....
content/common/net/url_fetcher.cc:1011: scoped_refptr<net::HttpResponseHeaders>
headers) {
Pass a const ref in order to avoid an unnecessary copy?

I thought we had the general rule to pass a refcounted X to functions as plain
pointers, since the caller needs to hold a reference anyway. Lately, I've seen
more and more code declaring parameters as const scoped_refptr<X>&. Since Will
and Eric are on this review: What's the rule?

[willchan no longer on Chromium, 2011-09-28 18:51:28]

http://codereview.chromium.org/8054013/diff/1/content/common/net/url_fetcher.cc
File content/common/net/url_fetcher.cc (right):

http://codereview.chromium.org/8054013/diff/1/content/common/net/url_fetcher....
content/common/net/url_fetcher.cc:1011: scoped_refptr<net::HttpResponseHeaders>
headers) {
On 2011/09/28 09:06:18, Mattias Nissler wrote:
> Pass a const ref in order to avoid an unnecessary copy?
> 
> I thought we had the general rule to pass a refcounted X to functions as plain
> pointers, since the caller needs to hold a reference anyway. Lately, I've seen
> more and more code declaring parameters as const scoped_refptr<X>&. Since Will
> and Eric are on this review: What's the rule?

The rule is to pass in a raw pointer, never const scoped_refptr<T>&.

[Joao da Silva, 2011-09-28 19:20:27]

Thanks all for reviewing

http://codereview.chromium.org/8054013/diff/1/chrome/browser/policy/device_ma...
File chrome/browser/policy/device_management_service.cc (right):

http://codereview.chromium.org/8054013/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service.cc:246: LOG(WARNING) << "Got bad
mime-type in response from dmserver that was "
On 2011/09/28 09:06:18, Mattias Nissler wrote:
> Please also put a comment here explaining what problem we're trying to solve.

Done.

http://codereview.chromium.org/8054013/diff/1/chrome/browser/policy/device_ma...
File chrome/browser/policy/device_management_service_unittest.cc (right):

http://codereview.chromium.org/8054013/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service_unittest.cc:555:
EXPECT_TRUE((fetcher->load_flags() & net::LOAD_BYPASS_PROXY) != 0);
On 2011/09/28 09:06:18, Mattias Nissler wrote:
> you can drop the != 0

Done.

http://codereview.chromium.org/8054013/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service_unittest.cc:559: // No calls
should have been made to the mock.
On 2011/09/28 09:06:18, Mattias Nissler wrote:
> Other code uses EXPECT_CALL().Times(0) instead of StrictMock to achieve this,
> and the gmock documentation advises against using StrictMock if you can
achieve
> what you want with regular mocks and expectations.

Done.

[commit-bot: I haz the power, 2011-09-28 19:20:41]

CQ is trying tha patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/8054013/2002

[akalin, 2011-09-28 19:24:09]

>
> The rule is to pass in a raw pointer, never const scoped_refptr<T>&.


Hmm, did you change your mind about this?  I could have sworn you've
recommended const scoped_refptr<T>& before.

I disagree with passing raw pointers around.  That could easily lead to
danging raw pointers (if the callee is modified to, e.g. store it or
something).

[willchan no longer on Chromium, 2011-09-28 19:28:38]

On Wed, Sep 28, 2011 at 12:24 PM, Fred Akalin <akalin@chromium.org> wrote:

> The rule is to pass in a raw pointer, never const scoped_refptr<T>&.
>
>
> Hmm, did you change your mind about this?  I could have sworn you've
> recommended const scoped_refptr<T>& before.
>

I'm all for passing const scoped_refptr<T>&. I was outvoted on chromium-dev.
At least that was my takeaway from that chromium-dev article.


>
> I disagree with passing raw pointers around.  That could easily lead to
> danging raw pointers (if the callee is modified to, e.g. store it or
> something).
>

[willchan no longer on Chromium, 2011-09-28 19:28:49]

On Wed, Sep 28, 2011 at 12:28 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> On Wed, Sep 28, 2011 at 12:24 PM, Fred Akalin <akalin@chromium.org> wrote:
>
>> The rule is to pass in a raw pointer, never const scoped_refptr<T>&.
>>
>>
>> Hmm, did you change your mind about this?  I could have sworn you've
>> recommended const scoped_refptr<T>& before.
>>
>
> I'm all for passing const scoped_refptr<T>&. I was outvoted on
> chromium-dev. At least that was my takeaway from that chromium-dev article.
>

Er, email thread.


>
>
>>
>> I disagree with passing raw pointers around.  That could easily lead to
>> danging raw pointers (if the callee is modified to, e.g. store it or
>> something).
>>
>
>

[commit-bot: I haz the power, 2011-09-28 20:46:25]

Change committed as 103172