AU: MultiHttpFetcher, an HttpFetcher for specific byte ranges

AU: MultiHttpFetcher, an HttpFetcher for specific byte ranges

MultiHttpFetcher takes an HttpFetcher class via template parameter,
and a collection of byte ranges. It hits up the URL multiple times,
once per range specified. For each time, it uses a new HttpFetcher of
the type specified and fast-forwards to the offset requested, and
aborting after enough bytes have been downloaded. Any range many
specify a length of -1, which means until the end of the file (as
dictated by the server). Thus, a single range of [0, -1] makes
MultiHttpFetcher a pass-through.

HttpFetcher change: ability to supply an offset.

LibcurlHttpFetcher changes: offset support (from HttpFetcher API),
ability to be terminted in a write-callback.

test_http_fetcher: support for failures in write() on the socket (at
least in the /big url case).

BUG=7391
TEST=unittests

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

[petkov, 2010-10-08 01:41:24]

some quick comments, we'll look more later...

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

http://codereview.chromium.org/3591018/diff/1/4#newcode304
libcurl_http_fetcher.cc:304: long http_response_code = 0;
set http_response_code_ to 0... otherwise it's unitialized/unchanged on getinfo
failure.

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

http://codereview.chromium.org/3591018/diff/1/5#newcode149
libcurl_http_fetcher.h:149: // not to resuming a download across a reboot.
not to resuming an interrupted download.

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

http://codereview.chromium.org/3591018/diff/1/7#newcode1
multi_http_fetcher.h:1: // Copyright (c) 2010 The Chromium OS Authors. All
rights reserved.
This seems like a lot of implementation code for a header file. Can you move at
least some of it to a .cc?

http://codereview.chromium.org/3591018/diff/1/7#newcode22
multi_http_fetcher.h:22: template<typename BaseHttpFetcher>
do you need this to be a template? given that you have virtual methods, etc...

http://codereview.chromium.org/3591018/diff/1/7#newcode36
multi_http_fetcher.h:36: // fetchers_[0] =
std::tr1::shared_ptr<BaseHttpFetcher>(new BaseHttpFetcher);
remove? or 80 chars...

http://codereview.chromium.org/3591018/diff/1/7#newcode37
multi_http_fetcher.h:37: for (typename
std::vector<std::tr1::shared_ptr<BaseHttpFetcher> >::iterator it =
fetchers_.begin(),
80 chars.

http://codereview.chromium.org/3591018/diff/1/7#newcode134
multi_http_fetcher.h:134: && (bytes_received_this_fetcher_ >=
ranges_[current_index_].second)) {
move && to the upper line and remove extra ()

[adlr, 2010-10-08 02:50:09]

address your comments, hopefully... ready for another look.

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

http://codereview.chromium.org/3591018/diff/1/4#newcode304
libcurl_http_fetcher.cc:304: long http_response_code = 0;
On 2010/10/08 01:41:24, petkov wrote:
> set http_response_code_ to 0... otherwise it's unitialized/unchanged on
getinfo
> failure.
> 

it's already initialized to 0 in BeginTransfer() and HttpFetcher's ctor. This
method is intended to not clobber an existing response code on failure

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

http://codereview.chromium.org/3591018/diff/1/5#newcode149
libcurl_http_fetcher.h:149: // not to resuming a download across a reboot.
On 2010/10/08 01:41:24, petkov wrote:
> not to resuming an interrupted download.
> 

Done.

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

http://codereview.chromium.org/3591018/diff/1/7#newcode1
multi_http_fetcher.h:1: // Copyright (c) 2010 The Chromium OS Authors. All
rights reserved.
On 2010/10/08 01:41:24, petkov wrote:
> This seems like a lot of implementation code for a header file. Can you move
at
> least some of it to a .cc?
> 

Templated classes have to be defined in headers, afaik. See below for why I made
it a templated class.

http://codereview.chromium.org/3591018/diff/1/7#newcode22
multi_http_fetcher.h:22: template<typename BaseHttpFetcher>
On 2010/10/08 01:41:24, petkov wrote:
> do you need this to be a template? given that you have virtual methods, etc...
> 

It doesn't need to be a template, but I figured I'd just make it generic. It
seems cleaner for things to depend on an HttpFetcher API rather than a
LibcurlHttpFetcher API... anyway, that's why I made it a template.

http://codereview.chromium.org/3591018/diff/1/7#newcode36
multi_http_fetcher.h:36: // fetchers_[0] =
std::tr1::shared_ptr<BaseHttpFetcher>(new BaseHttpFetcher);
On 2010/10/08 01:41:24, petkov wrote:
> remove? or 80 chars...
> 
> 

Done.

http://codereview.chromium.org/3591018/diff/1/7#newcode37
multi_http_fetcher.h:37: for (typename
std::vector<std::tr1::shared_ptr<BaseHttpFetcher> >::iterator it =
fetchers_.begin(),
On 2010/10/08 01:41:24, petkov wrote:
> 80 chars.
> 

Done.

http://codereview.chromium.org/3591018/diff/1/7#newcode134
multi_http_fetcher.h:134: && (bytes_received_this_fetcher_ >=
ranges_[current_index_].second)) {
On 2010/10/08 01:41:24, petkov wrote:
> move && to the upper line and remove extra ()
> 

Done.

[petkov, 2010-10-08 03:01:23]

LGTM -- let's get this in and integrate it with the rest of the code to see if
things work. We can refactor and cleanup later, if needed.

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

http://codereview.chromium.org/3591018/diff/1/7#newcode22
multi_http_fetcher.h:22: template<typename BaseHttpFetcher>
On 2010/10/08 02:50:10, adlr wrote:
> On 2010/10/08 01:41:24, petkov wrote:
> > do you need this to be a template? given that you have virtual methods,
etc...
> > 
> 
> It doesn't need to be a template, but I figured I'd just make it generic. It
> seems cleaner for things to depend on an HttpFetcher API rather than a
> LibcurlHttpFetcher API... anyway, that's why I made it a template.

Ah, I think the answer I was looking for was because you're actually
constructing the fetchers in this class. In general, I don't like templates --
they duplicate code, are hard to read, etc. I would have use a callback or
something like that. Anyway, it's OK given the time frame...