net: Move data reading functionalities from UploadElement to UploadElementReader

net: Move data reading functionalities from UploadElement to UploadElementReader

Add a new interface net::UploadElementReader and its implementations for bytes and file.
Rewrite UploadDataStream using UploadElementReader.
UploadElement::SetContentLength is replaced with UploadFileElementReader::ScopedOverridingContentLengthForTests.
HttpNetworkTransactionSpdy(2|3)Test.UnreadableUploadFileAfterAuthRestart is bit modified since a new FileStream is created every time when UploadDataStream is initialized.

BUG=145329
TEST=net_unittests

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

[hashimoto, 2012-08-28 20:28:56]

willchan,

Could you review this change?
This change depends on my previous change,
http://codereview.chromium.org/10878082/

[satorux1, 2012-08-28 20:42:35]

Drive-by comments. Two meta comments:

1) 72001 is abused. please file sub issues for divided tasks including this one.
That'd make it easier to track the progress and get help from some other
engineers, if needed.

2) by having the Reader interface, we'll be able to read from a URLFetcher
downloading a file from Drive, and this is the motivation of having the Reader
interface. Am I understanding right?

http://codereview.chromium.org/10868064/diff/18001/net/base/upload_data_strea...
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/10868064/diff/18001/net/base/upload_data_strea...
net/base/upload_data_stream.cc:32: UploadElementReader* reader = NULL;
maybe

UploadElementReader* reader = UploadElementReader::Create(element);

http://codereview.chromium.org/10868064/diff/18001/net/http/http_network_tran...
File net/http/http_network_transaction_spdy3_unittest.cc (right):

http://codereview.chromium.org/10868064/diff/18001/net/http/http_network_tran...
net/http/http_network_transaction_spdy3_unittest.cc:6638: "Content-Length:
0\r\n"
why was this changed?

[hashimoto, 2012-08-28 21:13:44]

On 2012/08/28 20:42:35, satorux1 wrote:
> Drive-by comments. Two meta comments:
> 
> 1) 72001 is abused. please file sub issues for divided tasks including this
one.
> That'd make it easier to track the progress and get help from some other
> engineers, if needed.
Filed crbug.com/145329
> 
> 2) by having the Reader interface, we'll be able to read from a URLFetcher
> downloading a file from Drive, and this is the motivation of having the Reader
> interface. Am I understanding right?
Right, and another motivation of this change is to make it easier to convert
data reading code to asynchronous.
To process asynchronous data reading safely, it is desirable to have a more
encapsulated classes to well manage the lifetime and the consistency of the
object which is performing the data reading.

> 
>
http://codereview.chromium.org/10868064/diff/18001/net/base/upload_data_strea...
> File net/base/upload_data_stream.cc (right):
> 
>
http://codereview.chromium.org/10868064/diff/18001/net/base/upload_data_strea...
> net/base/upload_data_stream.cc:32: UploadElementReader* reader = NULL;
> maybe
> 
> UploadElementReader* reader = UploadElementReader::Create(element);
> 
>
http://codereview.chromium.org/10868064/diff/18001/net/http/http_network_tran...
> File net/http/http_network_transaction_spdy3_unittest.cc (right):
> 
>
http://codereview.chromium.org/10868064/diff/18001/net/http/http_network_tran...
> net/http/http_network_transaction_spdy3_unittest.cc:6638: "Content-Length:
> 0\r\n"
> why was this changed?

[hashimoto, 2012-08-28 21:41:20]

http://codereview.chromium.org/10868064/diff/18001/net/base/upload_data_strea...
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/10868064/diff/18001/net/base/upload_data_strea...
net/base/upload_data_stream.cc:32: UploadElementReader* reader = NULL;
On 2012/08/28 20:42:35, satorux1 wrote:
> maybe
> 
> UploadElementReader* reader = UploadElementReader::Create(element);

Done.

http://codereview.chromium.org/10868064/diff/18001/net/http/http_network_tran...
File net/http/http_network_transaction_spdy3_unittest.cc (right):

http://codereview.chromium.org/10868064/diff/18001/net/http/http_network_tran...
net/http/http_network_transaction_spdy3_unittest.cc:6638: "Content-Length:
0\r\n"
On 2012/08/28 20:42:35, satorux1 wrote:
> why was this changed?

This was changed because the content length is now cached in UploadDataStream,
not in UploadElement.

Before this change, the content length is cached in UploadElement and the value
is reused between different UploadDataStream instances.
But, now the content length is not cached in UploadElement, only in
UploadDataStream (UploadDataStream::size() returns the cached value).
It means that the content length is calculated every time UploadDataStream is
initialized so the second content length here becomes zero.

I think this change is OK since the change which introduced this test was for
fixing freeze and the freeze does not occur even after this change.
(http://codereview.chromium.org/1250002)

[willchan no longer on Chromium, 2012-09-01 01:40:17]

Sorry, didn't finish this. Will look later.

http://codereview.chromium.org/10868064/diff/36001/net/base/upload_data_strea...
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/10868064/diff/36001/net/base/upload_data_strea...
net/base/upload_data_stream.cc:84: base::ThreadRestrictions::ScopedAllowIO
allow_io;
This seems to make sense, although I'm a bit confused why it's new. Can you
explain what's going on here? I see the ReadSync(), so that makes sense I guess.

[hashimoto, 2012-09-01 01:51:53]

http://codereview.chromium.org/10868064/diff/36001/net/base/upload_data_strea...
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/10868064/diff/36001/net/base/upload_data_strea...
net/base/upload_data_stream.cc:84: base::ThreadRestrictions::ScopedAllowIO
allow_io;
On 2012/09/01 01:40:18, willchan wrote:
> This seems to make sense, although I'm a bit confused why it's new. Can you
> explain what's going on here? I see the ReadSync(), so that makes sense I
guess.

Sorry for missing description.

This ScopedAllowIO was actually moved from UploadElement::ReadFromFileSync.
I know moving it to UploadFileElementReader::ReadSync may be more natural than
this, but I think it is a good timing to move it to the upper level.

I think ScopedAllowIO should be placed in upper level functions (e.g.
UploadDataStream::Read) rather than low level ones (e.g.
UploadElementReader::ReadSync) since the latter may result in future abuse more
easily.

[willchan no longer on Chromium, 2012-09-05 02:55:59]

http://codereview.chromium.org/10868064/diff/36002/net/base/upload_data_strea...
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/10868064/diff/36002/net/base/upload_data_strea...
net/base/upload_data_stream.cc:36: base::ThreadRestrictions::ScopedAllowIO
allow_io;
Shouldn't this be moved to the specific UploadElementReader's destructor? Does
it need to apply to all of them?

http://codereview.chromium.org/10868064/diff/36002/net/base/upload_data_strea...
net/base/upload_data_stream.cc:84: base::ThreadRestrictions::ScopedAllowIO
allow_io;
Can't this also be moved to the specific UploadElementReader implementation?
It's not true for all of them, is it?

http://codereview.chromium.org/10868064/diff/36002/net/base/upload_element_re...
File net/base/upload_element_reader.h (right):

http://codereview.chromium.org/10868064/diff/36002/net/base/upload_element_re...
net/base/upload_element_reader.h:22: class NET_EXPORT UploadElementReader {
Why is this NET_EXPORT instead of NET_EXPORT_PRIVATE?

http://codereview.chromium.org/10868064/diff/36002/net/base/upload_element_re...
net/base/upload_element_reader.h:55: class NET_EXPORT UploadBytesElementReader :
public UploadElementReader {
Ditto on NET_EXPORT.

http://codereview.chromium.org/10868064/diff/36002/net/base/upload_element_re...
net/base/upload_element_reader.h:76: class NET_EXPORT UploadFileElementReader :
public UploadElementReader {
Ditto on NET_EXPORT

http://codereview.chromium.org/10868064/diff/36002/net/base/upload_element_re...
File net/base/upload_element_reader_unittest.cc (right):

http://codereview.chromium.org/10868064/diff/36002/net/base/upload_element_re...
net/base/upload_element_reader_unittest.cc:110: const size_t kTooLargeSize =
bytes_.size()*2;
Another nit is that you should do add spaces between the operands:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Horizo...,
like you do for division elsewhere in this file.

[kinuko, 2012-09-05 03:24:32]

(Drive-by)

http://codereview.chromium.org/10868064/diff/36002/net/base/upload_element_re...
File net/base/upload_element_reader.h (right):

http://codereview.chromium.org/10868064/diff/36002/net/base/upload_element_re...
net/base/upload_element_reader.h:22: class NET_EXPORT UploadElementReader {
On 2012/09/05 02:55:59, willchan wrote:
> Why is this NET_EXPORT instead of NET_EXPORT_PRIVATE?

I guess it's because we might want to implement in different modules, like in
content or in webkit?

http://codereview.chromium.org/10868064/diff/36002/net/base/upload_element_re...
net/base/upload_element_reader.h:55: class NET_EXPORT UploadBytesElementReader :
public UploadElementReader {
It might be nicer to have these implementations in its own file?

[hashimoto, 2012-09-05 22:00:55]

http://codereview.chromium.org/10868064/diff/36002/net/base/upload_data_strea...
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/10868064/diff/36002/net/base/upload_data_strea...
net/base/upload_data_stream.cc:36: base::ThreadRestrictions::ScopedAllowIO
allow_io;
On 2012/09/05 02:55:59, willchan wrote:
> Shouldn't this be moved to the specific UploadElementReader's destructor? Does
> it need to apply to all of them?

I thought if we placed ScopedAllowIO in UploadFileElementReader's dtor, anyone
could be a user of the synchronous operation, while placing ScopedAllowIO here
prohibits abuse of UploadFileElementReader's synchrnous operation.
Anyway, it donen't matter if we prefer abuse prohibition to narrower scope or
not, since we are going to remove these ScopedAllowIO soon. (they will be moved
to Chrome Frame code at some timing)

Done.

http://codereview.chromium.org/10868064/diff/36002/net/base/upload_data_strea...
net/base/upload_data_stream.cc:84: base::ThreadRestrictions::ScopedAllowIO
allow_io;
On 2012/09/05 02:55:59, willchan wrote:
> Can't this also be moved to the specific UploadElementReader implementation?
> It's not true for all of them, is it?

Done.

http://codereview.chromium.org/10868064/diff/36002/net/base/upload_element_re...
File net/base/upload_element_reader.h (right):

http://codereview.chromium.org/10868064/diff/36002/net/base/upload_element_re...
net/base/upload_element_reader.h:22: class NET_EXPORT UploadElementReader {
On 2012/09/05 03:24:32, kinuko wrote:
> On 2012/09/05 02:55:59, willchan wrote:
> > Why is this NET_EXPORT instead of NET_EXPORT_PRIVATE?
> 
> I guess it's because we might want to implement in different modules, like in
> content or in webkit?

Yes, it should be overridable from non-net modules.

http://codereview.chromium.org/10868064/diff/36002/net/base/upload_element_re...
net/base/upload_element_reader.h:55: class NET_EXPORT UploadBytesElementReader :
public UploadElementReader {
On 2012/09/05 02:55:59, willchan wrote:
> Ditto on NET_EXPORT.

Done.

http://codereview.chromium.org/10868064/diff/36002/net/base/upload_element_re...
net/base/upload_element_reader.h:55: class NET_EXPORT UploadBytesElementReader :
public UploadElementReader {
On 2012/09/05 03:24:32, kinuko wrote:
> It might be nicer to have these implementations in its own file?

Done.

http://codereview.chromium.org/10868064/diff/36002/net/base/upload_element_re...
net/base/upload_element_reader.h:76: class NET_EXPORT UploadFileElementReader :
public UploadElementReader {
On 2012/09/05 02:55:59, willchan wrote:
> Ditto on NET_EXPORT

Done.

http://codereview.chromium.org/10868064/diff/36002/net/base/upload_element_re...
File net/base/upload_element_reader_unittest.cc (right):

http://codereview.chromium.org/10868064/diff/36002/net/base/upload_element_re...
net/base/upload_element_reader_unittest.cc:110: const size_t kTooLargeSize =
bytes_.size()*2;
On 2012/09/05 02:55:59, willchan wrote:
> Another nit is that you should do add spaces between the operands:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Horizo...,
> like you do for division elsewhere in this file.

Done.

[willchan no longer on Chromium, 2012-09-06 01:06:01]

http://codereview.chromium.org/10868064/diff/36002/net/base/upload_data_strea...
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/10868064/diff/36002/net/base/upload_data_strea...
net/base/upload_data_stream.cc:36: base::ThreadRestrictions::ScopedAllowIO
allow_io;
On 2012/09/05 22:00:55, hashimoto wrote:
> On 2012/09/05 02:55:59, willchan wrote:
> > Shouldn't this be moved to the specific UploadElementReader's destructor?
Does
> > it need to apply to all of them?
> 
> I thought if we placed ScopedAllowIO in UploadFileElementReader's dtor, anyone
> could be a user of the synchronous operation, while placing ScopedAllowIO here
> prohibits abuse of UploadFileElementReader's synchrnous operation.
> Anyway, it donen't matter if we prefer abuse prohibition to narrower scope or
> not, since we are going to remove these ScopedAllowIO soon. (they will be
moved
> to Chrome Frame code at some timing)
> 
> Done.

OIC. Yes, you're right then.

http://codereview.chromium.org/10868064/diff/39004/net/base/upload_bytes_elem...
File net/base/upload_bytes_element_reader_unittest.cc (right):

http://codereview.chromium.org/10868064/diff/39004/net/base/upload_bytes_elem...
net/base/upload_bytes_element_reader_unittest.cc:18: bytes_.assign(kData, kData
+ arraysize(kData));
you need basictypes.h for arraysize

http://codereview.chromium.org/10868064/diff/39004/net/base/upload_data_strea...
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/10868064/diff/39004/net/base/upload_data_strea...
net/base/upload_data_stream.cc:57: // Initialize readers for newly appended
chunks.
This just occurred to me. I'm not sure how big a deal it is, and I don't believe
it's a regression, but do we ever free up old
UploadElementReaders/UploadElements? I figure that if the size gets really large
and split up among many UploadElements, it'd be nicer to free up the old memory
as we go. Just a thought.

http://codereview.chromium.org/10868064/diff/39004/net/base/upload_data_strea...
net/base/upload_data_stream.cc:64: DCHECK(element.type() ==
UploadElement::TYPE_BYTES);
DCHECK_EQ?

http://codereview.chromium.org/10868064/diff/39004/net/base/upload_element_re...
File net/base/upload_element_reader.h (right):

http://codereview.chromium.org/10868064/diff/39004/net/base/upload_element_re...
net/base/upload_element_reader.h:38: // Reads up to |buf_length| bytes
synchronously. Returns the number of bytes
Is it valid to read 0 bytes? What happens when BytesRemaining() == 0 and
ReadSync() is called?

http://codereview.chromium.org/10868064/diff/39004/net/base/upload_file_eleme...
File net/base/upload_file_element_reader.cc (right):

http://codereview.chromium.org/10868064/diff/39004/net/base/upload_file_eleme...
net/base/upload_file_element_reader.cc:79: // modification time from WebKit is
based on time_t precision. So we
I don't fully understand this comment. Is this moved from somewhere else, or is
it new? If it's new, I sort of object, because it looks like a layering
violation.

http://codereview.chromium.org/10868064/diff/39004/net/base/upload_file_eleme...
File net/base/upload_file_element_reader.h (right):

http://codereview.chromium.org/10868064/diff/39004/net/base/upload_file_eleme...
net/base/upload_file_element_reader.h:45: struct NET_EXPORT
ScopedOverridingContentLengthForTests {
Types need to be declared at the beginning of the access control section, above
the member variables.

[hashimoto, 2012-09-06 01:55:39]

http://codereview.chromium.org/10868064/diff/39004/net/base/upload_bytes_elem...
File net/base/upload_bytes_element_reader_unittest.cc (right):

http://codereview.chromium.org/10868064/diff/39004/net/base/upload_bytes_elem...
net/base/upload_bytes_element_reader_unittest.cc:18: bytes_.assign(kData, kData
+ arraysize(kData));
On 2012/09/06 01:06:01, willchan wrote:
> you need basictypes.h for arraysize

Done.

http://codereview.chromium.org/10868064/diff/39004/net/base/upload_data_strea...
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/10868064/diff/39004/net/base/upload_data_strea...
net/base/upload_data_stream.cc:57: // Initialize readers for newly appended
chunks.
On 2012/09/06 01:06:01, willchan wrote:
> This just occurred to me. I'm not sure how big a deal it is, and I don't
believe
> it's a regression, but do we ever free up old
> UploadElementReaders/UploadElements? I figure that if the size gets really
large
> and split up among many UploadElements, it'd be nicer to free up the old
memory
> as we go. Just a thought.
This is not a regression.
Currently, appended chunks are never freed up.
Filed a bug. http://crbug.com/146697

http://codereview.chromium.org/10868064/diff/39004/net/base/upload_data_strea...
net/base/upload_data_stream.cc:64: DCHECK(element.type() ==
UploadElement::TYPE_BYTES);
On 2012/09/06 01:06:01, willchan wrote:
> DCHECK_EQ?

Done.

http://codereview.chromium.org/10868064/diff/39004/net/base/upload_element_re...
File net/base/upload_element_reader.h (right):

http://codereview.chromium.org/10868064/diff/39004/net/base/upload_element_re...
net/base/upload_element_reader.h:38: // Reads up to |buf_length| bytes
synchronously. Returns the number of bytes
On 2012/09/06 01:06:01, willchan wrote:
> Is it valid to read 0 bytes? What happens when BytesRemaining() == 0 and
> ReadSync() is called?
No behavior change from the current code.

Reading 0 bytes results in returning 0 and no-op.
When BytesRemaining() == 0 and ReadSync() is called, it also results in
returning 0 and no-op.

http://codereview.chromium.org/10868064/diff/39004/net/base/upload_file_eleme...
File net/base/upload_file_element_reader.cc (right):

http://codereview.chromium.org/10868064/diff/39004/net/base/upload_file_eleme...
net/base/upload_file_element_reader.cc:79: // modification time from WebKit is
based on time_t precision. So we
On 2012/09/06 01:06:01, willchan wrote:
> I don't fully understand this comment. Is this moved from somewhere else, or
is
> it new? If it's new, I sort of object, because it looks like a layering
> violation.
This comment is moved from UploadDataStream::Init().
Agree, this part is really ugly.

http://codereview.chromium.org/10868064/diff/39004/net/base/upload_file_eleme...
File net/base/upload_file_element_reader.h (right):

http://codereview.chromium.org/10868064/diff/39004/net/base/upload_file_eleme...
net/base/upload_file_element_reader.h:45: struct NET_EXPORT
ScopedOverridingContentLengthForTests {
On 2012/09/06 01:06:01, willchan wrote:
> Types need to be declared at the beginning of the access control section,
above
> the member variables.

Done.

[hashimoto, 2012-09-07 17:27:44]

ping?

[willchan no longer on Chromium, 2012-09-10 21:32:13]

lgtm

[commit-bot: I haz the power, 2012-09-10 21:42:14]

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

[commit-bot: I haz the power, 2012-09-10 21:42:24]

Presubmit check for 10868064-42002 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Banned functions were used.
    net/base/upload_file_element_reader.cc:37:
      New code should not use ScopedAllowIO. Post a task to the blocking
      pool or the FILE thread instead.
    net/base/upload_file_element_reader.cc:44:
      New code should not use ScopedAllowIO. Post a task to the blocking
      pool or the FILE thread instead.
    net/base/upload_file_element_reader.cc:106:
      New code should not use ScopedAllowIO. Post a task to the blocking
      pool or the FILE thread instead.

Presubmit checks took 1.9s to calculate.

[hashimoto, 2012-09-11 03:38:11]

On 2012/09/10 21:42:24, I haz the power (commit-bot) wrote:
> Presubmit check for 10868064-42002 failed and returned exit status 1.
> 
> 
> Running presubmit commit checks ...
> 
> ** Presubmit ERRORS **
> Banned functions were used.
>     net/base/upload_file_element_reader.cc:37:
>       New code should not use ScopedAllowIO. Post a task to the blocking
>       pool or the FILE thread instead.
>     net/base/upload_file_element_reader.cc:44:
>       New code should not use ScopedAllowIO. Post a task to the blocking
>       pool or the FILE thread instead.
>     net/base/upload_file_element_reader.cc:106:
>       New code should not use ScopedAllowIO. Post a task to the blocking
>       pool or the FILE thread instead.
> 
> Presubmit checks took 1.9s to calculate.

Going to bypass hooks since ScopedAllowIO in this change is just moved from one
place to another.

[willchan no longer on Chromium, 2012-09-11 18:46:33]

SGTM


On Mon, Sep 10, 2012 at 8:38 PM, <hashimoto@chromium.org> wrote:

> On 2012/09/10 21:42:24, I haz the power (commit-bot) wrote:
>
>> Presubmit check for 10868064-42002 failed and returned exit status 1.
>>
>
>
>  Running presubmit commit checks ...
>>
>
>  ** Presubmit ERRORS **
>> Banned functions were used.
>>      net/base/upload_file_element_**reader.cc:37:
>>        New code should not use ScopedAllowIO. Post a task to the blocking
>>        pool or the FILE thread instead.
>>      net/base/upload_file_element_**reader.cc:44:
>>        New code should not use ScopedAllowIO. Post a task to the blocking
>>        pool or the FILE thread instead.
>>      net/base/upload_file_element_**reader.cc:106:
>>        New code should not use ScopedAllowIO. Post a task to the blocking
>>        pool or the FILE thread instead.
>>
>
>  Presubmit checks took 1.9s to calculate.
>>
>
> Going to bypass hooks since ScopedAllowIO in this change is just moved
> from one
> place to another.
>
>
http://codereview.chromium.**org/10868064/<http://codereview.chromium.org/108...
>