AU: Manual proxy support

AU: Manual proxy support

Utilize the ChromeProxyResolver to resolve proxies in our network
requests. This means the following changes:

- HttpFetcher classes take a ProxyResolver* in their ctor. Also, a few
  useful functions in HttpFetcher to allow subclasses to iterate
  through the proxies.

- LibcurlHttpFetcher support for using the ProxyResolver. It will
  attempt to use each proxy in the order specified. If any data comes
  in from any proxy, it won't continue down the list and will continue
  to use that proxy for its lifetime.

- UpdateAttempter can choose, for a given update session, whether or
  not to use the ChromeProxyResolver or DirectProxyResolver. For now,
  the logic is: for automatic checks, 80% of the time use
  ChromeProxyResolver, 20% DirectProxyResolver. For manual checks, the
  first 19 manual checks in a row use Chrome, then once it uses
  Direct, then starts over again. The idea is that the updater doesn't
  necessarily trust Chrome, so some requests should skip it. If a
  manual check is performed, the user likely wants her proxy settings
  honored, so use them, but don't allow frequent manual checks to
  starve out usage of the DirectProxyResolver.

- Updates to tests

BUG=3167
TEST=unittests, tested on device

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=4516810

[petkov, 2010-11-19 05:37:15]

http://codereview.chromium.org/5205002/diff/1/http_fetcher.h
File http_fetcher.h (right):

http://codereview.chromium.org/5205002/diff/1/http_fetcher.h#newcode13
http_fetcher.h:13: #include <base/basictypes.h>
sort

http://codereview.chromium.org/5205002/diff/1/http_fetcher.h#newcode31
http_fetcher.h:31: explicit HttpFetcher(ProxyResolver* proxy_resolver)
a brief docstring might be useful -- can proxy_resolver be NULL?

http://codereview.chromium.org/5205002/diff/1/http_fetcher.h#newcode54
http_fetcher.h:54: if (!proxy_resolver_) {
Lots of code for a header file. I'd suggest move all the implementation into a
http_fetcher.cc.

http://codereview.chromium.org/5205002/diff/1/http_fetcher.h#newcode55
http_fetcher.h:55: LOG(ERROR) << "Missing proxy resolver!";
Why an ERROR? Maybe just default to kNoProxy. If not, did you mean to return?
Currently, you'll print an ERROR and then crash below due to NULL pointer
dereference.

http://codereview.chromium.org/5205002/diff/1/http_fetcher.h#newcode66
http_fetcher.h:66: proxies_.insert(proxies_.begin(), proxies.begin(),
proxies.end());
you could probably just proxies_.assign(begin, end). and no need to clear.

http://codereview.chromium.org/5205002/diff/1/libcurl_http_fetcher.cc
File libcurl_http_fetcher.cc (right):

http://codereview.chromium.org/5205002/diff/1/libcurl_http_fetcher.cc#newcode64
libcurl_http_fetcher.cc:64: LOG(ERROR) << "Missing proxy.";
This is a really unexpected case, right? If you end up here, you may be ending
up in a infinite loop due to a bug or something. It might make more sense to
CHECK/crash and restart into a known good state.

http://codereview.chromium.org/5205002/diff/1/libcurl_http_fetcher.cc#newcode78
libcurl_http_fetcher.cc:78: LOG(INFO) << "Set proxtype to: " << type;
Set proxy type?

http://codereview.chromium.org/5205002/diff/1/libcurl_http_fetcher.cc#newcode215
libcurl_http_fetcher.cc:215: g_timeout_add_seconds(0,
g_idle_add?

http://codereview.chromium.org/5205002/diff/1/libcurl_http_fetcher.cc#newcode256
libcurl_http_fetcher.cc:256: sent_byte_ = true;
are you sure size > 0?

http://codereview.chromium.org/5205002/diff/1/libcurl_http_fetcher.h
File libcurl_http_fetcher.h (right):

http://codereview.chromium.org/5205002/diff/1/libcurl_http_fetcher.h#newcode25
libcurl_http_fetcher.h:25: LibcurlHttpFetcher(ProxyResolver* proxy_resolver)
explicit?

http://codereview.chromium.org/5205002/diff/1/multi_http_fetcher.h
File multi_http_fetcher.h (right):

http://codereview.chromium.org/5205002/diff/1/multi_http_fetcher.h#newcode23
multi_http_fetcher.h:23: template<typename BaseHttpFetcher>
Longer term: this class might simpler if it didn't use templates and allocated
fetchers itself but rather implemented a method:

// takes ownership of |fetcher|.
AddFetcher(HttpFetcher* fetcher, off_t start, off_t size);

http://codereview.chromium.org/5205002/diff/1/multi_http_fetcher.h#newcode46
multi_http_fetcher.h:46: LOG(INFO) << "list of fetchers: " <<
(uint64_t)(it->get());
do you need this new logging? and below?

http://codereview.chromium.org/5205002/diff/1/multi_http_fetcher.h#newcode143
multi_http_fetcher.h:143: virtual void SetProxies(const
std::vector<std::string>& proxies) {
Instead of overloading this method, can't you just do SetProxies right before
calling BeginTransfer on the next fetcher? You might want to change SetProxies
to take begin/end iterators or implement set_proxies or switch deque to
vector...

http://codereview.chromium.org/5205002/diff/1/multi_http_fetcher.h#newcode146
multi_http_fetcher.h:146: it != e; ++it) {
move up?

http://codereview.chromium.org/5205002/diff/1/multi_http_fetcher.h#newcode179
multi_http_fetcher.h:179: if (fetcher != fetchers_[current_index_].get()) {
When is this true given that there's a TEST_AND_RETURN for this above?

http://codereview.chromium.org/5205002/diff/1/update_attempter.cc
File update_attempter.cc (right):

http://codereview.chromium.org/5205002/diff/1/update_attempter.cc#newcode144
update_attempter.cc:144: static const char message[] = "To help ensure updates
work, this update "
Can you do one LOG_IF(INFO, obeying_proxies_) after the whole if-statement and
avoid this message variable?

http://codereview.chromium.org/5205002/diff/1/update_attempter.cc#newcode147
update_attempter.cc:147: if (force_obey_proxies) {
does it make sense to always obey proxies on the first update check, regardless
if it's manual or automatic? you could just add || proxy_manual_checks_ == 0
here if it makes sense... not sure though.

http://codereview.chromium.org/5205002/diff/1/update_attempter.cc#newcode154
update_attempter.cc:154: force_obey_proxies = false;
no need?

http://codereview.chromium.org/5205002/diff/1/update_attempter.cc#newcode155
update_attempter.cc:155: obeying_proxies_ = true;
do you mean obeying_proxies_ = false?

http://codereview.chromium.org/5205002/diff/1/update_attempter.cc#newcode188
update_attempter.cc:188: new LibcurlHttpFetcher(GetProxyResolver())));
You should probably always obey proxies for event actions.

http://codereview.chromium.org/5205002/diff/1/update_attempter.h
File update_attempter.h (right):

http://codereview.chromium.org/5205002/diff/1/update_attempter.h#newcode60
update_attempter.h:60: // If force_obey_proxies is true, the update will likely
respect Chrome's
|force_obey_proxies| or maybe just |obey_proxies| given that it's not really
forced (given that we _may_ obey them).

http://codereview.chromium.org/5205002/diff/1/update_attempter_mock.h
File update_attempter_mock.h (right):

http://codereview.chromium.org/5205002/diff/1/update_attempter_mock.h#newcode21
update_attempter_mock.h:21: bool force_proxy));
match the name of the argument.

[petkov, 2010-11-19 17:10:04]

http://codereview.chromium.org/5205002/diff/1/multi_http_fetcher.h
File multi_http_fetcher.h (right):

http://codereview.chromium.org/5205002/diff/1/multi_http_fetcher.h#newcode23
multi_http_fetcher.h:23: template<typename BaseHttpFetcher>
On 2010/11/19 05:37:15, petkov wrote:
> Longer term: this class might simpler if it didn't use templates and allocated
> fetchers itself but rather implemented a method:
> 
> // takes ownership of |fetcher|.
> AddFetcher(HttpFetcher* fetcher, off_t start, off_t size);

In fact, we can probably simplify this to reuse the same fetcher for each range.

[adlr, 2010-11-20 02:52:29]

PTAL. thanks!

http://codereview.chromium.org/5205002/diff/1/http_fetcher.h
File http_fetcher.h (right):

http://codereview.chromium.org/5205002/diff/1/http_fetcher.h#newcode13
http_fetcher.h:13: #include <base/basictypes.h>
On 2010/11/19 05:37:15, petkov wrote:
> sort

Done.

http://codereview.chromium.org/5205002/diff/1/http_fetcher.h#newcode31
http_fetcher.h:31: explicit HttpFetcher(ProxyResolver* proxy_resolver)
On 2010/11/19 05:37:15, petkov wrote:
> a brief docstring might be useful -- can proxy_resolver be NULL?

decided to allow NULL and documented it as such.

http://codereview.chromium.org/5205002/diff/1/http_fetcher.h#newcode54
http_fetcher.h:54: if (!proxy_resolver_) {
On 2010/11/19 05:37:15, petkov wrote:
> Lots of code for a header file. I'd suggest move all the implementation into a
> http_fetcher.cc.

Done.

http://codereview.chromium.org/5205002/diff/1/http_fetcher.h#newcode55
http_fetcher.h:55: LOG(ERROR) << "Missing proxy resolver!";
On 2010/11/19 05:37:15, petkov wrote:
> Why an ERROR? Maybe just default to kNoProxy. If not, did you mean to return?
> Currently, you'll print an ERROR and then crash below due to NULL pointer
> dereference.

fixed. NULL now legal

http://codereview.chromium.org/5205002/diff/1/http_fetcher.h#newcode66
http_fetcher.h:66: proxies_.insert(proxies_.begin(), proxies.begin(),
proxies.end());
On 2010/11/19 05:37:15, petkov wrote:
> you could probably just proxies_.assign(begin, end). and no need to clear.
> 

Done.

http://codereview.chromium.org/5205002/diff/1/libcurl_http_fetcher.cc
File libcurl_http_fetcher.cc (right):

http://codereview.chromium.org/5205002/diff/1/libcurl_http_fetcher.cc#newcode64
libcurl_http_fetcher.cc:64: LOG(ERROR) << "Missing proxy.";
On 2010/11/19 05:37:15, petkov wrote:
> This is a really unexpected case, right? If you end up here, you may be ending
> up in a infinite loop due to a bug or something. It might make more sense to
> CHECK/crash and restart into a known good state.

good call. changed to CHECK

http://codereview.chromium.org/5205002/diff/1/libcurl_http_fetcher.cc#newcode78
libcurl_http_fetcher.cc:78: LOG(INFO) << "Set proxtype to: " << type;
On 2010/11/19 05:37:15, petkov wrote:
> Set proxy type?

removed this line; was kinda redundant

http://codereview.chromium.org/5205002/diff/1/libcurl_http_fetcher.cc#newcode215
libcurl_http_fetcher.cc:215: g_timeout_add_seconds(0,
On 2010/11/19 05:37:15, petkov wrote:
> g_idle_add?

Done.

http://codereview.chromium.org/5205002/diff/1/libcurl_http_fetcher.cc#newcode256
libcurl_http_fetcher.cc:256: sent_byte_ = true;
On 2010/11/19 05:37:15, petkov wrote:
> are you sure size > 0?

hadn't considered that. Should we even call anyone if we get a 0-byte write?
seems not... changed to that behavior.

http://codereview.chromium.org/5205002/diff/1/libcurl_http_fetcher.h
File libcurl_http_fetcher.h (right):

http://codereview.chromium.org/5205002/diff/1/libcurl_http_fetcher.h#newcode25
libcurl_http_fetcher.h:25: LibcurlHttpFetcher(ProxyResolver* proxy_resolver)
On 2010/11/19 05:37:15, petkov wrote:
> explicit?

Done.

http://codereview.chromium.org/5205002/diff/1/multi_http_fetcher.h
File multi_http_fetcher.h (right):

http://codereview.chromium.org/5205002/diff/1/multi_http_fetcher.h#newcode23
multi_http_fetcher.h:23: template<typename BaseHttpFetcher>
On 2010/11/19 17:10:04, petkov wrote:
> On 2010/11/19 05:37:15, petkov wrote:
> > Longer term: this class might simpler if it didn't use templates and
allocated
> > fetchers itself but rather implemented a method:
> > 
> > // takes ownership of |fetcher|.
> > AddFetcher(HttpFetcher* fetcher, off_t start, off_t size);
> 
> In fact, we can probably simplify this to reuse the same fetcher for each
range.

Yeah, we could probably reuse fetchers, but we'd need to test that. Right now,
afaik, we've never reused a fetcher object.

http://codereview.chromium.org/5205002/diff/1/multi_http_fetcher.h#newcode46
multi_http_fetcher.h:46: LOG(INFO) << "list of fetchers: " <<
(uint64_t)(it->get());
On 2010/11/19 05:37:15, petkov wrote:
> do you need this new logging? and below?

oops. yeah, when i use c-style casts i'm being quick for debugging... removed

http://codereview.chromium.org/5205002/diff/1/multi_http_fetcher.h#newcode143
multi_http_fetcher.h:143: virtual void SetProxies(const
std::vector<std::string>& proxies) {
On 2010/11/19 05:37:15, petkov wrote:
> Instead of overloading this method, can't you just do SetProxies right before
> calling BeginTransfer on the next fetcher?

We could do that. We would probably also want to do it w/ the other Set*()
methods, tho. Perhaps we should have an HttpFetcher cleanup to revamp all this?
For now I'm inclined to keep this inline w/ the other Set*() methods.

> You might want to change SetProxies
> to take begin/end iterators or implement set_proxies or switch deque to
> vector...

I tried to make SetProxies take iterators, but their type would be a template
type to the function, which makes it not able to be virtual. The nice thing
about having a deque is it handles large proxy lists better (arguably not a
likely case in practice). Just kind feels a little weird to "downgrade" to
vector...

http://codereview.chromium.org/5205002/diff/1/multi_http_fetcher.h#newcode146
multi_http_fetcher.h:146: it != e; ++it) {
On 2010/11/19 05:37:15, petkov wrote:
> move up?

Done.

http://codereview.chromium.org/5205002/diff/1/multi_http_fetcher.h#newcode179
multi_http_fetcher.h:179: if (fetcher != fetchers_[current_index_].get()) {
On 2010/11/19 05:37:15, petkov wrote:
> When is this true given that there's a TEST_AND_RETURN for this above?

i think this may have been an issue w/ a merge. removed.

http://codereview.chromium.org/5205002/diff/1/update_attempter.cc
File update_attempter.cc (right):

http://codereview.chromium.org/5205002/diff/1/update_attempter.cc#newcode144
update_attempter.cc:144: static const char message[] = "To help ensure updates
work, this update "
On 2010/11/19 05:37:15, petkov wrote:
> Can you do one LOG_IF(INFO, obeying_proxies_) after the whole if-statement and
> avoid this message variable?

Done.

http://codereview.chromium.org/5205002/diff/1/update_attempter.cc#newcode147
update_attempter.cc:147: if (force_obey_proxies) {
On 2010/11/19 05:37:15, petkov wrote:
> does it make sense to always obey proxies on the first update check,
regardless
> if it's manual or automatic? you could just add || proxy_manual_checks_ == 0
> here if it makes sense... not sure though.
> 

seems fine. added

http://codereview.chromium.org/5205002/diff/1/update_attempter.cc#newcode154
update_attempter.cc:154: force_obey_proxies = false;
On 2010/11/19 05:37:15, petkov wrote:
> no need?

good call. removed

http://codereview.chromium.org/5205002/diff/1/update_attempter.cc#newcode155
update_attempter.cc:155: obeying_proxies_ = true;
On 2010/11/19 05:37:15, petkov wrote:
> do you mean obeying_proxies_ = false?

Done.

http://codereview.chromium.org/5205002/diff/1/update_attempter.cc#newcode188
update_attempter.cc:188: new LibcurlHttpFetcher(GetProxyResolver())));
On 2010/11/19 05:37:15, petkov wrote:
> You should probably always obey proxies for event actions.

The idea was we would be consistent for a given update cycle. GetProxyResolver()
will always return the same thing for a given cycle. I think that's better
behavior, but i'm open to suggestion if you think that should be changed...

http://codereview.chromium.org/5205002/diff/1/update_attempter.h
File update_attempter.h (right):

http://codereview.chromium.org/5205002/diff/1/update_attempter.h#newcode60
update_attempter.h:60: // If force_obey_proxies is true, the update will likely
respect Chrome's
On 2010/11/19 05:37:15, petkov wrote:
> |force_obey_proxies| or maybe just |obey_proxies| given that it's not really
> forced (given that we _may_ obey them).

Done.

http://codereview.chromium.org/5205002/diff/1/update_attempter_mock.h
File update_attempter_mock.h (right):

http://codereview.chromium.org/5205002/diff/1/update_attempter_mock.h#newcode21
update_attempter_mock.h:21: bool force_proxy));
On 2010/11/19 05:37:15, petkov wrote:
> match the name of the argument.

Done.

[petkov, 2010-11-22 17:49:00]

A few more nits. The major issue is that you're missing a file, I think.

http://codereview.chromium.org/5205002/diff/1/multi_http_fetcher.h
File multi_http_fetcher.h (right):

http://codereview.chromium.org/5205002/diff/1/multi_http_fetcher.h#newcode143
multi_http_fetcher.h:143: virtual void SetProxies(const
std::vector<std::string>& proxies) {
On 2010/11/20 02:52:29, adlr wrote:
> On 2010/11/19 05:37:15, petkov wrote:
> > Instead of overloading this method, can't you just do SetProxies right
before
> > calling BeginTransfer on the next fetcher?
> 
> We could do that. We would probably also want to do it w/ the other Set*()
> methods, tho. Perhaps we should have an HttpFetcher cleanup to revamp all
this?

Yeah, we should have a cleanup. And we could rename it to HTTPFetcher :-)

> For now I'm inclined to keep this inline w/ the other Set*() methods.

That's OK. The difference vs. the other Set* methods is that this one is
implemented in the base class (and also provides a data field in it).

> 
> > You might want to change SetProxies
> > to take begin/end iterators or implement set_proxies or switch deque to
> > vector...
> 
> I tried to make SetProxies take iterators, but their type would be a template
> type to the function, which makes it not able to be virtual. The nice thing
> about having a deque is it handles large proxy lists better (arguably not a
> likely case in practice). Just kind feels a little weird to "downgrade" to
> vector...

You could also "upgrade" vector to deque :-) Doesn't matter much so feel free to
ignore.

http://codereview.chromium.org/5205002/diff/1/update_attempter.cc
File update_attempter.cc (right):

http://codereview.chromium.org/5205002/diff/1/update_attempter.cc#newcode188
update_attempter.cc:188: new LibcurlHttpFetcher(GetProxyResolver())));
On 2010/11/20 02:52:29, adlr wrote:
> On 2010/11/19 05:37:15, petkov wrote:
> > You should probably always obey proxies for event actions.
> 
> The idea was we would be consistent for a given update cycle.
GetProxyResolver()
> will always return the same thing for a given cycle. I think that's better
> behavior, but i'm open to suggestion if you think that should be changed...

Yes, makes sense. I missed the fact that the whole update cycle will use the
same resolver. Looks good as is.

http://codereview.chromium.org/5205002/diff/11001/SConstruct
File SConstruct (right):

http://codereview.chromium.org/5205002/diff/11001/SConstruct#newcode256
SConstruct:256: http_fetcher.cc
You seem to be missing this file.

http://codereview.chromium.org/5205002/diff/11001/http_fetcher.h
File http_fetcher.h (right):

http://codereview.chromium.org/5205002/diff/11001/http_fetcher.h#newcode56
http_fetcher.h:56: const std::string& CurrentProxy() const {
This should be GetCurrentProxy.

http://codereview.chromium.org/5205002/diff/11001/http_fetcher_unittest.cc
File http_fetcher_unittest.cc (right):

http://codereview.chromium.org/5205002/diff/11001/http_fetcher_unittest.cc#ne...
http_fetcher_unittest.cc:16: #include "update_engine/libcurl_http_fetcher.h"
blank line before and use <> for the headers above

[adlr, 2010-11-22 18:50:15]

I addressed all the comments in the review server and replied to the others
below:

PTAL

On Mon, Nov 22, 2010 at 9:49 AM, <petkov@chromium.org> wrote:

> A few more nits. The major issue is that you're missing a file, I think.
>
>
>
>
> http://codereview.chromium.org/5205002/diff/1/multi_http_fetcher.h
> File multi_http_fetcher.h (right):
>
>
> http://codereview.chromium.org/5205002/diff/1/multi_http_fetcher.h#newcode143
> multi_http_fetcher.h:143: virtual void SetProxies(const
> std::vector<std::string>& proxies) {
> On 2010/11/20 02:52:29, adlr wrote:
>
>> On 2010/11/19 05:37:15, petkov wrote:
>> > Instead of overloading this method, can't you just do SetProxies
>>
> right before
>
>> > calling BeginTransfer on the next fetcher?
>>
>
>  We could do that. We would probably also want to do it w/ the other
>>
> Set*()
>
>> methods, tho. Perhaps we should have an HttpFetcher cleanup to revamp
>>
> all this?
>
> Yeah, we should have a cleanup. And we could rename it to HTTPFetcher
> :-)


Okay, for now, as is.


>
>
>  For now I'm inclined to keep this inline w/ the other Set*() methods.
>>
>
> That's OK. The difference vs. the other Set* methods is that this one is
> implemented in the base class (and also provides a data field in it).


I upgraded vector to deque, as you mentioned elsewhere, which makes this Set
method a simple assignment, meaning it's eligible to be capitalized as
set_*. However, since it's overloaded in MultiHttpFetcher (as are many Set*
methods), I kept it CamelCase.


>
>
>
>  > You might want to change SetProxies
>> > to take begin/end iterators or implement set_proxies or switch deque
>>
> to
>
>> > vector...
>>
>
>  I tried to make SetProxies take iterators, but their type would be a
>>
> template
>
>> type to the function, which makes it not able to be virtual. The nice
>>
> thing
>
>> about having a deque is it handles large proxy lists better (arguably
>>
> not a
>
>> likely case in practice). Just kind feels a little weird to
>>
> "downgrade" to
>
>> vector...
>>
>
> You could also "upgrade" vector to deque :-) Doesn't matter much so feel
> free to ignore.


done.


>
>
> http://codereview.chromium.org/5205002/diff/1/update_attempter.cc
> File update_attempter.cc (right):
>
>
> http://codereview.chromium.org/5205002/diff/1/update_attempter.cc#newcode188
> update_attempter.cc:188: new LibcurlHttpFetcher(GetProxyResolver())));
> On 2010/11/20 02:52:29, adlr wrote:
>
>> On 2010/11/19 05:37:15, petkov wrote:
>> > You should probably always obey proxies for event actions.
>>
>
>  The idea was we would be consistent for a given update cycle.
>>
> GetProxyResolver()
>
>> will always return the same thing for a given cycle. I think that's
>>
> better
>
>> behavior, but i'm open to suggestion if you think that should be
>>
> changed...
>
> Yes, makes sense. I missed the fact that the whole update cycle will use
> the same resolver. Looks good as is.
>
> http://codereview.chromium.org/5205002/diff/11001/SConstruct
> File SConstruct (right):
>
> http://codereview.chromium.org/5205002/diff/11001/SConstruct#newcode256
> SConstruct:256: http_fetcher.cc
> You seem to be missing this file.
>
> http://codereview.chromium.org/5205002/diff/11001/http_fetcher.h
>
> File http_fetcher.h (right):
>
> http://codereview.chromium.org/5205002/diff/11001/http_fetcher.h#newcode56
> http_fetcher.h:56: const std::string& CurrentProxy() const {
> This should be GetCurrentProxy.
>
> http://codereview.chromium.org/5205002/diff/11001/http_fetcher_unittest.cc
> File http_fetcher_unittest.cc (right):
>
>
>
http://codereview.chromium.org/5205002/diff/11001/http_fetcher_unittest.cc#ne...
> http_fetcher_unittest.cc:16: #include
> "update_engine/libcurl_http_fetcher.h"
> blank line before and use <> for the headers above
>
>
> http://codereview.chromium.org/5205002/
>

[adlr, 2010-11-22 18:54:45]

publishing the comments

http://codereview.chromium.org/5205002/diff/11001/SConstruct
File SConstruct (right):

http://codereview.chromium.org/5205002/diff/11001/SConstruct#newcode256
SConstruct:256: http_fetcher.cc
On 2010/11/22 17:49:00, petkov wrote:
> You seem to be missing this file.

Done.

http://codereview.chromium.org/5205002/diff/11001/http_fetcher.h
File http_fetcher.h (right):

http://codereview.chromium.org/5205002/diff/11001/http_fetcher.h#newcode56
http_fetcher.h:56: const std::string& CurrentProxy() const {
On 2010/11/22 17:49:00, petkov wrote:
> This should be GetCurrentProxy.

Done.

http://codereview.chromium.org/5205002/diff/11001/http_fetcher_unittest.cc
File http_fetcher_unittest.cc (right):

http://codereview.chromium.org/5205002/diff/11001/http_fetcher_unittest.cc#ne...
http_fetcher_unittest.cc:16: #include "update_engine/libcurl_http_fetcher.h"
On 2010/11/22 17:49:00, petkov wrote:
> blank line before and use <> for the headers above

Done.

[petkov, 2010-11-22 18:56:41]

LGTM