Support range uploading of a file.

Support range uploading of a file.

URLFetcher currently supports uploading std::string or a whole file.
With this CL, URLFetcher also supports a part of file, which will be used
to upload a file from Drive file system.

BUG=234178
TEST=Ran unit_tests

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

[hidehiko, 2013-04-30 10:23:03]

Hello Matt,

could you kindly review this CL?
I'm working on google_apis code, and would like to use file uploading function
of UrlFetcher. Currently, we can upload whole a file, but I also would like to
support uploading a part of the file for resuming the upload operation
(restarting to uploading the remaining part of the file) on drive API.

Ryo, Kazuhiro, could you kindly review, too?

I'd appreciate your help,
- hidehiko

[hashimoto, 2013-04-30 10:27:36]

https://codereview.chromium.org/14578004/diff/1/net/url_request/url_fetcher.h
File net/url_request/url_fetcher.h (right):

https://codereview.chromium.org/14578004/diff/1/net/url_request/url_fetcher.h...
net/url_request/url_fetcher.h:151: uint64 range_length,
Instead of adding a new method, can we add these range parameters to
SetUploadFilePath?
The method is used by only one user, two_phase_uploader.cc.

[hidehiko, 2013-04-30 11:30:14]

Thank you for your review, and sorry, my local env was somehow broken. Fixed
compile errors.

https://codereview.chromium.org/14578004/diff/1/net/url_request/url_fetcher.h
File net/url_request/url_fetcher.h (right):

https://codereview.chromium.org/14578004/diff/1/net/url_request/url_fetcher.h...
net/url_request/url_fetcher.h:151: uint64 range_length,
On 2013/04/30 10:27:36, hashimoto wrote:
> Instead of adding a new method, can we add these range parameters to
> SetUploadFilePath?
> The method is used by only one user, two_phase_uploader.cc.

We can, but should we do that?
I'm thinking that setting (0, kuint64max) at client side (to upload whole the
file) may not good stuff because it's just the implementation details. WDYT?

[hashimoto, 2013-05-01 06:39:21]

https://codereview.chromium.org/14578004/diff/1/net/url_request/url_fetcher.h
File net/url_request/url_fetcher.h (right):

https://codereview.chromium.org/14578004/diff/1/net/url_request/url_fetcher.h...
net/url_request/url_fetcher.h:151: uint64 range_length,
On 2013/04/30 11:30:14, hidehiko wrote:
> On 2013/04/30 10:27:36, hashimoto wrote:
> > Instead of adding a new method, can we add these range parameters to
> > SetUploadFilePath?
> > The method is used by only one user, two_phase_uploader.cc.
> 
> We can, but should we do that?
> I'm thinking that setting (0, kuint64max) at client side (to upload whole the
> file) may not good stuff because it's just the implementation details. WDYT?

I don't think we should maintain two unpopular methods at the same time which
has only one user for each.

Historically, SetUploadFilePath was introduced because the attempt to add more
general SetUploadDataStream method had failed due to the problem around retry.
(http://crbug.com/169551)
I think setting kuint64max is not beautiful but acceptable because
SetUploadFilePath is meant to be an alternative for manual setup of
UploadFileElementReader.

[hidehiko, 2013-05-01 13:51:49]

Thank you for your review, Ryo. PTAL?

Matt (mmenke@), could you review for files under net/? 

Matt (mattm@), could you review for
chrome/browser/safe_browsing/two_phase_uploader.cc?

Thank you for your review in advance,
- hidehiko

https://codereview.chromium.org/14578004/diff/1/net/url_request/url_fetcher.h
File net/url_request/url_fetcher.h (right):

https://codereview.chromium.org/14578004/diff/1/net/url_request/url_fetcher.h...
net/url_request/url_fetcher.h:151: uint64 range_length,
On 2013/05/01 06:39:21, hashimoto wrote:
> On 2013/04/30 11:30:14, hidehiko wrote:
> > On 2013/04/30 10:27:36, hashimoto wrote:
> > > Instead of adding a new method, can we add these range parameters to
> > > SetUploadFilePath?
> > > The method is used by only one user, two_phase_uploader.cc.
> > 
> > We can, but should we do that?
> > I'm thinking that setting (0, kuint64max) at client side (to upload whole
the
> > file) may not good stuff because it's just the implementation details. WDYT?
> 
> I don't think we should maintain two unpopular methods at the same time which
> has only one user for each.
> 
> Historically, SetUploadFilePath was introduced because the attempt to add more
> general SetUploadDataStream method had failed due to the problem around retry.
> (http://crbug.com/169551)
> I think setting kuint64max is not beautiful but acceptable because
> SetUploadFilePath is meant to be an alternative for manual setup of
> UploadFileElementReader.

Ok. Done.

[mmenke, 2013-05-01 15:37:01]

Just nits

https://codereview.chromium.org/14578004/diff/13003/net/url_request/url_fetch...
File net/url_request/url_fetcher_impl_unittest.cc (right):

https://codereview.chromium.org/14578004/diff/13003/net/url_request/url_fetch...
net/url_request/url_fetcher_impl_unittest.cc:585: range_length_ = 100;
I think it's a little weird to make these variables but set them to random
values in the constructor.

I think it's actually simpler just to modify URLFetcherPostFileTest instead of
creating a new test fixture.  Setting these to 0 and kuint64max respectively in
the constructor.

https://codereview.chromium.org/14578004/diff/13003/net/url_request/url_fetch...
net/url_request/url_fetcher_impl_unittest.cc:585: range_length_ = 100;
Also, these should be set in initializer lists.

[hidehiko, 2013-05-01 17:38:12]

Thank you for your review. PTAL?

https://chromiumcodereview.appspot.com/14578004/diff/13003/net/url_request/ur...
File net/url_request/url_fetcher_impl_unittest.cc (right):

https://chromiumcodereview.appspot.com/14578004/diff/13003/net/url_request/ur...
net/url_request/url_fetcher_impl_unittest.cc:585: range_length_ = 100;
On 2013/05/01 15:37:01, mmenke wrote:
> Also, these should be set in initializer lists.

I see. So how about this? Merged two test fixtures, instead added a setter for
range_offset and range_length, and set them in TEST_F code.

[mmenke, 2013-05-01 18:10:22]

https://codereview.chromium.org/14578004/diff/18002/net/url_request/url_fetch...
File net/url_request/url_fetcher_impl_unittest.cc (right):

https://codereview.chromium.org/14578004/diff/18002/net/url_request/url_fetch...
net/url_request/url_fetcher_impl_unittest.cc:247: const base::FilePath& path() {
return path_; }
nit:  "... path() const { ...", if you want to keep it after responding to the
comment below.

https://codereview.chromium.org/14578004/diff/18002/net/url_request/url_fetch...
net/url_request/url_fetcher_impl_unittest.cc:1090: uint64 range_length =
base::RandGenerator(size - range_offset);
We generally avoid deliberate randomization in tests.

Tests that are flaky generally get ignores - suppose it only fails when
range_offset and range_length are both 0.

Also, it's difficult to reproduce failures.  I suggest just using hard-coded
values like you were before.

[hidehiko, 2013-05-01 18:30:04]

Thank you for your review. Could you take another look?

https://chromiumcodereview.appspot.com/14578004/diff/18002/net/url_request/ur...
File net/url_request/url_fetcher_impl_unittest.cc (right):

https://chromiumcodereview.appspot.com/14578004/diff/18002/net/url_request/ur...
net/url_request/url_fetcher_impl_unittest.cc:247: const base::FilePath& path() {
return path_; }
On 2013/05/01 18:10:22, mmenke wrote:
> nit:  "... path() const { ...", if you want to keep it after responding to the
> comment below.

Just removed.

https://chromiumcodereview.appspot.com/14578004/diff/18002/net/url_request/ur...
net/url_request/url_fetcher_impl_unittest.cc:1090: uint64 range_length =
base::RandGenerator(size - range_offset);
On 2013/05/01 18:10:22, mmenke wrote:
> We generally avoid deliberate randomization in tests.
> 
> Tests that are flaky generally get ignores - suppose it only fails when
> range_offset and range_length are both 0.
> 
> Also, it's difficult to reproduce failures.  I suggest just using hard-coded
> values like you were before.

I see. Sorry that I misunderstood that I should set the actual random value.
Fixed.

[mmenke, 2013-05-01 18:34:48]

LGTM!

https://chromiumcodereview.appspot.com/14578004/diff/18002/net/url_request/ur...
File net/url_request/url_fetcher_impl_unittest.cc (right):

https://chromiumcodereview.appspot.com/14578004/diff/18002/net/url_request/ur...
net/url_request/url_fetcher_impl_unittest.cc:1090: uint64 range_length =
base::RandGenerator(size - range_offset);
On 2013/05/01 18:30:04, hidehiko wrote:
> On 2013/05/01 18:10:22, mmenke wrote:
> > We generally avoid deliberate randomization in tests.
> > 
> > Tests that are flaky generally get ignores - suppose it only fails when
> > range_offset and range_length are both 0.
> > 
> > Also, it's difficult to reproduce failures.  I suggest just using hard-coded
> > values like you were before.
> 
> I see. Sorry that I misunderstood that I should set the actual random value.
> Fixed.

Oh, I'm the one who should have been clearer.  Sorry about that.

[mattm, 2013-05-01 19:52:35]

two_phase_uploader lgtm

https://codereview.chromium.org/14578004/diff/28001/net/url_request/url_fetch...
File net/url_request/url_fetcher.h (right):

https://codereview.chromium.org/14578004/diff/28001/net/url_request/url_fetch...
net/url_request/url_fetcher.h:139: // to be uploaded. To upload while the file,
(0, kuint64max) can be used.
s/while the/the whole/  ?

[kinaba, 2013-05-01 23:55:31]

lgtm

[hidehiko, 2013-05-02 00:59:50]

Thank you for your review!

https://codereview.chromium.org/14578004/diff/28001/net/url_request/url_fetch...
File net/url_request/url_fetcher.h (right):

https://codereview.chromium.org/14578004/diff/28001/net/url_request/url_fetch...
net/url_request/url_fetcher.h:139: // to be uploaded. To upload while the file,
(0, kuint64max) can be used.
On 2013/05/01 19:52:36, mattm wrote:
> s/while the/the whole/  ?

Done.

[hashimoto, 2013-05-02 04:45:35]

lgtm

https://codereview.chromium.org/14578004/diff/28002/net/url_request/url_fetch...
File net/url_request/url_fetcher.h (right):

https://codereview.chromium.org/14578004/diff/28002/net/url_request/url_fetch...
net/url_request/url_fetcher.h:138: // |range_offset| and |range_length|
specifies the range of the part
nit: s/specifies/specify/

[hidehiko, 2013-05-02 05:26:06]

Thank you all for your review. Sending to CQ.

https://codereview.chromium.org/14578004/diff/28002/net/url_request/url_fetch...
File net/url_request/url_fetcher.h (right):

https://codereview.chromium.org/14578004/diff/28002/net/url_request/url_fetch...
net/url_request/url_fetcher.h:138: // |range_offset| and |range_length|
specifies the range of the part
On 2013/05/02 04:45:35, hashimoto wrote:
> nit: s/specifies/specify/

Good catch. Done.

[commit-bot: I haz the power, 2013-05-02 05:26:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hidehiko@chromium.org/14578004/24002

[commit-bot: I haz the power, 2013-05-02 05:49:02]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-02 05:49:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hidehiko@chromium.org/14578004/24002

[commit-bot: I haz the power, 2013-05-02 05:51:14]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[hidehiko, 2013-05-02 06:20:51]

Oops, my code wasn't portable... Fixed and sending to CQ again.

[commit-bot: I haz the power, 2013-05-02 06:21:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hidehiko@chromium.org/14578004/54001

[commit-bot: I haz the power, 2013-05-02 09:42:09]

Change committed as 197878