Add SetUploadDataStream method to URLFetcher.

Add SetUploadDataStream method to URLFetcher.

BUG=169551
TEST=net_unittests

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

[mattm, 2013-01-11 02:14:25]

mmenke: review
hashimoto: FYI

[hashimoto, 2013-01-11 06:07:38]

Just out of curiosity, what is the expected use case of this new method?

https://codereview.chromium.org/11843003/diff/2002/net/url_request/test_url_f...
File net/url_request/test_url_fetcher_factory.cc (right):

https://codereview.chromium.org/11843003/diff/2002/net/url_request/test_url_f...
net/url_request/test_url_fetcher_factory.cc:64: upload_data_ = upload_content;
Having both upload_data_stream_ and upload_data_ seems confusing.
How about completely replacing upload_data_ with upload_data_stream_, or mark
SetUploadDataStream as NOTIMPLEMENTED?

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

https://codereview.chromium.org/11843003/diff/2002/net/url_request/url_fetche...
net/url_request/url_fetcher.h:126: scoped_ptr<UploadDataStream> upload_content)
= 0;
nit: blank line here?

https://codereview.chromium.org/11843003/diff/2002/net/url_request/url_fetche...
net/url_request/url_fetcher.h:132: 
nit: No need to have a blank line here.

https://codereview.chromium.org/11843003/diff/2002/net/url_request/url_fetche...
File net/url_request/url_fetcher_core.cc (right):

https://codereview.chromium.org/11843003/diff/2002/net/url_request/url_fetche...
net/url_request/url_fetcher_core.cc:1020: int64 current =
request_->GetUploadProgress().position();
You can use request_->GetUploadProgress().size() for |total|.

https://codereview.chromium.org/11843003/diff/2002/net/url_request/url_fetche...
net/url_request/url_fetcher_core.cc:1025: total =
static_cast<int64>(request_->get_upload()->size());
This change introduces a little difference to the behavior where |total| is zero
until UploadDataStream::Init gets called during the Http transaction.
Perhaps, we should not inform delegate about progress when !is_chunked_upload_
&& total==0.

[mmenke, 2013-01-11 16:25:40]

Please reference a bug number in the description.  If there isn't one, please
file one.  Thanks!

https://codereview.chromium.org/11843003/diff/2002/net/url_request/url_fetche...
File net/url_request/url_fetcher_core.cc (right):

https://codereview.chromium.org/11843003/diff/2002/net/url_request/url_fetche...
net/url_request/url_fetcher_core.cc:351: DCHECK(!is_chunked_upload_);
DCHECK(!upload_content);

Also maybe DCHECK(upload_content_type_.empty()); like in SetChunkedUpload?

[mattm, 2013-01-11 21:39:31]

On 2013/01/11 06:07:38, hashimoto wrote:
> Just out of curiosity, what is the expected use case of this new method?

It is for the safebrowsing download feedback feature I'm starting
(crbug.com/169557), which will want to upload files to the safebrowsing service.


On 2013/01/11 16:25:40, Matt Menke wrote:
> Please reference a bug number in the description.  If there isn't one, please
> file one.  Thanks!

done.

[mattm, 2013-01-11 21:42:02]

https://codereview.chromium.org/11843003/diff/2002/net/url_request/test_url_f...
File net/url_request/test_url_fetcher_factory.cc (right):

https://codereview.chromium.org/11843003/diff/2002/net/url_request/test_url_f...
net/url_request/test_url_fetcher_factory.cc:64: upload_data_ = upload_content;
On 2013/01/11 06:07:38, hashimoto wrote:
> Having both upload_data_stream_ and upload_data_ seems confusing.
> How about completely replacing upload_data_ with upload_data_stream_, 

Thought about that, but wasn't entirely sure what the desired use case of this
class is.  If the test code actually wants to be able to distinguish whether the
caller used SetUploadData or SetUploadDataStream, then having them separately
would make sense.  Also this would make implementing the upload_data() accessor
harder.

>or mark
> SetUploadDataStream as NOTIMPLEMENTED?

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

https://codereview.chromium.org/11843003/diff/2002/net/url_request/url_fetche...
net/url_request/url_fetcher.h:126: scoped_ptr<UploadDataStream> upload_content)
= 0;
On 2013/01/11 06:07:38, hashimoto wrote:
> nit: blank line here?

Done.

https://codereview.chromium.org/11843003/diff/2002/net/url_request/url_fetche...
net/url_request/url_fetcher.h:132: 
On 2013/01/11 06:07:38, hashimoto wrote:
> nit: No need to have a blank line here.

Done.

https://codereview.chromium.org/11843003/diff/2002/net/url_request/url_fetche...
File net/url_request/url_fetcher_core.cc (right):

https://codereview.chromium.org/11843003/diff/2002/net/url_request/url_fetche...
net/url_request/url_fetcher_core.cc:351: DCHECK(!is_chunked_upload_);
On 2013/01/11 16:25:40, Matt Menke wrote:
> DCHECK(!upload_content);
> 
> Also maybe DCHECK(upload_content_type_.empty()); like in SetChunkedUpload?

Done.

https://codereview.chromium.org/11843003/diff/2002/net/url_request/url_fetche...
net/url_request/url_fetcher_core.cc:1020: int64 current =
request_->GetUploadProgress().position();
On 2013/01/11 06:07:38, hashimoto wrote:
> You can use request_->GetUploadProgress().size() for |total|.

Done.

https://codereview.chromium.org/11843003/diff/2002/net/url_request/url_fetche...
net/url_request/url_fetcher_core.cc:1025: total =
static_cast<int64>(request_->get_upload()->size());
On 2013/01/11 06:07:38, hashimoto wrote:
> This change introduces a little difference to the behavior where |total| is
zero
> until UploadDataStream::Init gets called during the Http transaction.
> Perhaps, we should not inform delegate about progress when !is_chunked_upload_
> && total==0.

Done.

[hashimoto, 2013-01-15 06:59:28]

lgtm

https://codereview.chromium.org/11843003/diff/2002/net/url_request/test_url_f...
File net/url_request/test_url_fetcher_factory.cc (right):

https://codereview.chromium.org/11843003/diff/2002/net/url_request/test_url_f...
net/url_request/test_url_fetcher_factory.cc:64: upload_data_ = upload_content;
On 2013/01/11 21:42:02, mattm wrote:
> On 2013/01/11 06:07:38, hashimoto wrote:
> > Having both upload_data_stream_ and upload_data_ seems confusing.
> > How about completely replacing upload_data_ with upload_data_stream_, 
> 
> Thought about that, but wasn't entirely sure what the desired use case of this
> class is.  If the test code actually wants to be able to distinguish whether
the
> caller used SetUploadData or SetUploadDataStream, then having them separately
> would make sense.  Also this would make implementing the upload_data()
accessor
> harder.
> 
> >or mark
> > SetUploadDataStream as NOTIMPLEMENTED?
> 
I see.  Seems all users of this method just want to verify the correct value is
set as the upload data.
(http://code.google.com/p/chromium/source/search?q=\bupload_data\()

https://codereview.chromium.org/11843003/diff/11001/net/url_request/url_fetch...
File net/url_request/url_fetcher_core.cc (right):

https://codereview.chromium.org/11843003/diff/11001/net/url_request/url_fetch...
net/url_request/url_fetcher_core.cc:1022: int64 current =
request_->GetUploadProgress().position();
nit: How about replacing |current| and |total| with one net::UploadProgress?

[mmenke, 2013-01-15 15:06:39]

LGTM as well.

On 2013/01/15 06:59:28, hashimoto wrote:
> lgtm
> 
>
https://codereview.chromium.org/11843003/diff/2002/net/url_request/test_url_f...
> File net/url_request/test_url_fetcher_factory.cc (right):
> 
>
https://codereview.chromium.org/11843003/diff/2002/net/url_request/test_url_f...
> net/url_request/test_url_fetcher_factory.cc:64: upload_data_ = upload_content;
> On 2013/01/11 21:42:02, mattm wrote:
> > On 2013/01/11 06:07:38, hashimoto wrote:
> > > Having both upload_data_stream_ and upload_data_ seems confusing.
> > > How about completely replacing upload_data_ with upload_data_stream_, 
> > 
> > Thought about that, but wasn't entirely sure what the desired use case of
this
> > class is.  If the test code actually wants to be able to distinguish whether
> the
> > caller used SetUploadData or SetUploadDataStream, then having them
separately
> > would make sense.  Also this would make implementing the upload_data()
> accessor
> > harder.
> > 
> > >or mark
> > > SetUploadDataStream as NOTIMPLEMENTED?
> > 
> I see.  Seems all users of this method just want to verify the correct value
is
> set as the upload data.
> (http://code.google.com/p/chromium/source/search?q=%5Cbupload_data%5C%28)
> 
>
https://codereview.chromium.org/11843003/diff/11001/net/url_request/url_fetch...
> File net/url_request/url_fetcher_core.cc (right):
> 
>
https://codereview.chromium.org/11843003/diff/11001/net/url_request/url_fetch...
> net/url_request/url_fetcher_core.cc:1022: int64 current =
> request_->GetUploadProgress().position();
> nit: How about replacing |current| and |total| with one net::UploadProgress?

[mattm, 2013-01-24 02:35:45]

https://codereview.chromium.org/11843003/diff/11001/net/url_request/url_fetch...
File net/url_request/url_fetcher_core.cc (right):

https://codereview.chromium.org/11843003/diff/11001/net/url_request/url_fetch...
net/url_request/url_fetcher_core.cc:1022: int64 current =
request_->GetUploadProgress().position();
On 2013/01/15 06:59:28, hashimoto wrote:
> nit: How about replacing |current| and |total| with one net::UploadProgress?

Tried it out, but I think it just makes things a more verbose.  Also with the
handling of |total| being different for chunked or not it gets a little ugly.

[commit-bot: I haz the power, 2013-01-24 02:36:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/11843003/19001

[commit-bot: I haz the power, 2013-01-24 07:10:23]

Change committed as 178535