net: Make UploadDataStream::Init() asynchronous.

net: Make UploadDataStream::Init() asynchronous.

Rename the existing synchronous version to InitSync() and add a new asynchronous version, Init().
However, the new asynchronous version is not used yet.
The existing code is changed to use the synchronous version.

BUG=72001
TEST=net_unittests


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

[hashimoto, 2012-09-10 21:26:22]

willchan,

Could you review this change?
This change depends on http://codereview.chromium.org/10868064/ ("net: Move data
reading functionalities from UploadElement to UploadElementReader") and will be
followed by http://codereview.chromium.org/10829188/ ("Use
net::UploadDataStream::Init() instead of InitSync()")

[willchan no longer on Chromium, 2012-09-11 19:32:40]

http://codereview.chromium.org/10913179/diff/2015/net/base/upload_data_stream.cc
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/10913179/diff/2015/net/base/upload_data_stream...
net/base/upload_data_stream.cc:131: int index,
indentation is off

http://codereview.chromium.org/10913179/diff/2015/net/base/upload_data_stream...
net/base/upload_data_stream.cc:132: int last_result) {
I suggest renaming:
s/index/start_index/ - hopefully it'll make it clearer it's the starting index.
I think the confusion between i and index may have caused the bug below.
s/last_result/previous_result/ - last isn't as clear as previous IMO, since last
may have another meaning.

http://codereview.chromium.org/10913179/diff/2015/net/base/upload_data_stream...
net/base/upload_data_stream.cc:134: 
I recommend adding a DCHECK_NE(ERR_IO_PENDING, last_result).

http://codereview.chromium.org/10913179/diff/2015/net/base/upload_data_stream...
net/base/upload_data_stream.cc:136: if (last_result != OK) {
I wonder if we should do element_readers_.clear(); at all these places where we
invoke the callback with a non-OK error code.

http://codereview.chromium.org/10913179/diff/2015/net/base/upload_data_stream...
net/base/upload_data_stream.cc:148: index + 1));
Is this a bug? Should it be i + 1? Can you write a unit test that verifies this
behavior is correct? My guess is using an UploadDataStream with a
MockUploadElementReader.

http://codereview.chromium.org/10913179/diff/2015/net/base/upload_data_stream...
net/base/upload_data_stream.cc:167: total_size += reader->GetContentLength();
Just double checking...could this block? I'm guessing not right now...but what
happens when we have a Google Drive one? How does the content length work for
that?

http://codereview.chromium.org/10913179/diff/2015/net/base/upload_file_elemen...
File net/base/upload_file_element_reader.cc (right):

http://codereview.chromium.org/10913179/diff/2015/net/base/upload_file_elemen...
net/base/upload_file_element_reader.cc:30: int* out_result) {
Should we add a ThreadRestrictions assertion here to make sure we're running on
a thread that can have sync IO?

[hashimoto, 2012-09-11 22:51:31]

http://codereview.chromium.org/10913179/diff/2015/net/base/upload_data_stream.cc
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/10913179/diff/2015/net/base/upload_data_stream...
net/base/upload_data_stream.cc:131: int index,
On 2012/09/11 19:32:40, willchan wrote:
> indentation is off

Done.

http://codereview.chromium.org/10913179/diff/2015/net/base/upload_data_stream...
net/base/upload_data_stream.cc:132: int last_result) {
On 2012/09/11 19:32:40, willchan wrote:
> I suggest renaming:
> s/index/start_index/ - hopefully it'll make it clearer it's the starting
index.
> I think the confusion between i and index may have caused the bug below.
> s/last_result/previous_result/ - last isn't as clear as previous IMO, since
last
> may have another meaning.

Done.

http://codereview.chromium.org/10913179/diff/2015/net/base/upload_data_stream...
net/base/upload_data_stream.cc:134: 
On 2012/09/11 19:32:40, willchan wrote:
> I recommend adding a DCHECK_NE(ERR_IO_PENDING, last_result).

Done.

http://codereview.chromium.org/10913179/diff/2015/net/base/upload_data_stream...
net/base/upload_data_stream.cc:136: if (last_result != OK) {
On 2012/09/11 19:32:40, willchan wrote:
> I wonder if we should do element_readers_.clear(); at all these places where
we
> invoke the callback with a non-OK error code.

Done.

http://codereview.chromium.org/10913179/diff/2015/net/base/upload_data_stream...
net/base/upload_data_stream.cc:148: index + 1));
On 2012/09/11 19:32:40, willchan wrote:
> Is this a bug? Should it be i + 1? Can you write a unit test that verifies
this
> behavior is correct? My guess is using an UploadDataStream with a
> MockUploadElementReader.

You are absolutely right.
Thank you for finding this.
Added UploadDataStreamTest.InitAsync/InitAsyncFailureAsync/InitAsyncFailureSync
to cover all three code path which runs |callback|

http://codereview.chromium.org/10913179/diff/2015/net/base/upload_data_stream...
net/base/upload_data_stream.cc:167: total_size += reader->GetContentLength();
On 2012/09/11 19:32:40, willchan wrote:
> Just double checking...could this block? I'm guessing not right now...but what
> happens when we have a Google Drive one? How does the content length work for
> that?

For Google Drive, I'm intended to fetch the content length during Init().
GetContentLength() is meant be a simple getter which returns the pre-fetched
value.

http://codereview.chromium.org/10913179/diff/2015/net/base/upload_file_elemen...
File net/base/upload_file_element_reader.cc (right):

http://codereview.chromium.org/10913179/diff/2015/net/base/upload_file_elemen...
net/base/upload_file_element_reader.cc:30: int* out_result) {
On 2012/09/11 19:32:40, willchan wrote:
> Should we add a ThreadRestrictions assertion here to make sure we're running
on
> a thread that can have sync IO?

According to the comment in base/threading/thread_restrictions.h, their practice
to decide where to put AssertIOAllowed is "It's best if you put them as close to
the disk access as possible, at the lowest level."

Since there are already AssertIOAllowed in base/platform_file_posix.cc, I think
we should not add AssertIOAllowed here.

[willchan no longer on Chromium, 2012-09-12 21:09:50]

http://codereview.chromium.org/10913179/diff/5003/net/base/upload_data_stream...
File net/base/upload_data_stream_unittest.cc (right):

http://codereview.chromium.org/10913179/diff/5003/net/base/upload_data_stream...
net/base/upload_data_stream_unittest.cc:255: scoped_ptr<UploadDataStream>
stream(new UploadDataStream(upload_data_));
Does this need to be allocated on heap? How about:
UploadDataStream stream(upload_data_);

Ditto for all these new TEST_Fs.

http://codereview.chromium.org/10913179/diff/5003/net/base/upload_data_stream...
net/base/upload_data_stream_unittest.cc:258: scoped_ptr<MockUploadElementReader>
reader;
Why do you bother making this scoped, since you always release it?

Ditto for all these new TEST_Fs.

http://codereview.chromium.org/10913179/diff/5003/net/base/upload_file_elemen...
File net/base/upload_file_element_reader.cc (right):

http://codereview.chromium.org/10913179/diff/5003/net/base/upload_file_elemen...
net/base/upload_file_element_reader.cc:40: if (file_stream.get() &&
range_offset) {
How about making this an else if (range_offset) {

Oops, I see, you just moved this code block.

http://codereview.chromium.org/10913179/diff/5003/net/base/upload_file_elemen...
net/base/upload_file_element_reader.cc:183:
file_stream_.reset(file_stream->release());
How about using scoped_ptr<T>::swap(scoped_ptr<T>& p2)?

[hashimoto, 2012-09-12 21:48:18]

http://codereview.chromium.org/10913179/diff/5003/net/base/upload_data_stream...
File net/base/upload_data_stream_unittest.cc (right):

http://codereview.chromium.org/10913179/diff/5003/net/base/upload_data_stream...
net/base/upload_data_stream_unittest.cc:255: scoped_ptr<UploadDataStream>
stream(new UploadDataStream(upload_data_));
On 2012/09/12 21:09:50, willchan wrote:
> Does this need to be allocated on heap? How about:
> UploadDataStream stream(upload_data_);
> 
> Ditto for all these new TEST_Fs.

Done.

http://codereview.chromium.org/10913179/diff/5003/net/base/upload_data_stream...
net/base/upload_data_stream_unittest.cc:258: scoped_ptr<MockUploadElementReader>
reader;
On 2012/09/12 21:09:50, willchan wrote:
> Why do you bother making this scoped, since you always release it?
> 
> Ditto for all these new TEST_Fs.

I thought using scoped_ptr is always safer than raw pointers.
Anyway, it's also true that there is no difference between scoped_ptr and raw
pointers here.

Done.

http://codereview.chromium.org/10913179/diff/5003/net/base/upload_file_elemen...
File net/base/upload_file_element_reader.cc (right):

http://codereview.chromium.org/10913179/diff/5003/net/base/upload_file_elemen...
net/base/upload_file_element_reader.cc:40: if (file_stream.get() &&
range_offset) {
On 2012/09/12 21:09:50, willchan wrote:
> How about making this an else if (range_offset) {
> 
> Oops, I see, you just moved this code block.

Done.

http://codereview.chromium.org/10913179/diff/5003/net/base/upload_file_elemen...
net/base/upload_file_element_reader.cc:183:
file_stream_.reset(file_stream->release());
On 2012/09/12 21:09:50, willchan wrote:
> How about using scoped_ptr<T>::swap(scoped_ptr<T>& p2)?

Done.

[hashimoto, 2012-09-17 14:02:15]

ping?

On 2012/09/12 21:48:18, hashimoto wrote:
>
http://codereview.chromium.org/10913179/diff/5003/net/base/upload_data_stream...
> File net/base/upload_data_stream_unittest.cc (right):
> 
>
http://codereview.chromium.org/10913179/diff/5003/net/base/upload_data_stream...
> net/base/upload_data_stream_unittest.cc:255: scoped_ptr<UploadDataStream>
> stream(new UploadDataStream(upload_data_));
> On 2012/09/12 21:09:50, willchan wrote:
> > Does this need to be allocated on heap? How about:
> > UploadDataStream stream(upload_data_);
> > 
> > Ditto for all these new TEST_Fs.
> 
> Done.
> 
>
http://codereview.chromium.org/10913179/diff/5003/net/base/upload_data_stream...
> net/base/upload_data_stream_unittest.cc:258:
scoped_ptr<MockUploadElementReader>
> reader;
> On 2012/09/12 21:09:50, willchan wrote:
> > Why do you bother making this scoped, since you always release it?
> > 
> > Ditto for all these new TEST_Fs.
> 
> I thought using scoped_ptr is always safer than raw pointers.
> Anyway, it's also true that there is no difference between scoped_ptr and raw
> pointers here.
> 
> Done.
> 
>
http://codereview.chromium.org/10913179/diff/5003/net/base/upload_file_elemen...
> File net/base/upload_file_element_reader.cc (right):
> 
>
http://codereview.chromium.org/10913179/diff/5003/net/base/upload_file_elemen...
> net/base/upload_file_element_reader.cc:40: if (file_stream.get() &&
> range_offset) {
> On 2012/09/12 21:09:50, willchan wrote:
> > How about making this an else if (range_offset) {
> > 
> > Oops, I see, you just moved this code block.
> 
> Done.
> 
>
http://codereview.chromium.org/10913179/diff/5003/net/base/upload_file_elemen...
> net/base/upload_file_element_reader.cc:183:
> file_stream_.reset(file_stream->release());
> On 2012/09/12 21:09:50, willchan wrote:
> > How about using scoped_ptr<T>::swap(scoped_ptr<T>& p2)?
> 
> Done.

[willchan no longer on Chromium, 2012-09-17 17:34:43]

lgtm

http://codereview.chromium.org/10913179/diff/6010/net/base/upload_data_stream.h
File net/base/upload_data_stream.h (right):

http://codereview.chromium.org/10913179/diff/6010/net/base/upload_data_stream...
net/base/upload_data_stream.h:92: void InitInternal(const CompletionCallback&
callback,
nit: it's more common in net/ code to have the callback be the last parameter in
the parameter list.

[hashimoto, 2012-09-18 11:07:33]

http://codereview.chromium.org/10913179/diff/6010/net/base/upload_data_stream.h
File net/base/upload_data_stream.h (right):

http://codereview.chromium.org/10913179/diff/6010/net/base/upload_data_stream...
net/base/upload_data_stream.h:92: void InitInternal(const CompletionCallback&
callback,
On 2012/09/17 17:34:43, willchan wrote:
> nit: it's more common in net/ code to have the callback be the last parameter
in
> the parameter list.

|callback| cannot be the last parameter since |previous_result| must be in that
place.
Moved |callback| to the second.

[commit-bot: I haz the power, 2012-09-18 11:07:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/10913179/18001

[commit-bot: I haz the power, 2012-09-18 14:01:55]

Change committed as 157344