net: Make UploadData::GetContentLength() asynchronous.

net: Make UploadData::GetContentLength() asynchronous.

However, the asynchronous version is not used yet.
The synchronous version is kept as GetContentLengthSync().
The existing code is changed to use the synchronous version.

TEST=net_unittests
BUG=72001, 112607
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=121411

[satorux1, 2012-02-04 01:13:50]

+robertshield for chrome_frame changes.

[robertshield, 2012-02-04 05:39:03]

http://codereview.chromium.org/9321003/diff/4003/chrome_frame/plugin_url_requ...
File chrome_frame/plugin_url_request.cc (right):

http://codereview.chromium.org/9321003/diff/4003/chrome_frame/plugin_url_requ...
chrome_frame/plugin_url_request.cc:46: post_data_len_ =
upload_data->GetContentLengthSyncHack();
This code runs inside Internet Explorer, not Chrome. Does it really require the
Hack() version that disables the thread io restrictions? Code in Chrome Frame
should not be subject to Chrome's thread io restrictions.

http://codereview.chromium.org/9321003/diff/4003/net/base/upload_data.cc
File net/base/upload_data.cc (right):

http://codereview.chromium.org/9321003/diff/4003/net/base/upload_data.cc#newc...
net/base/upload_data.cc:223: *content_length +=
elements_.at(i).GetContentLength();
prefer using iterator syntax:
std::vector<Element>::iterator iter(elements_.begin());
for (; iter != elements_.end(); ++iter) { ...

http://codereview.chromium.org/9321003/diff/4003/net/base/upload_data.h
File net/base/upload_data.h (right):

http://codereview.chromium.org/9321003/diff/4003/net/base/upload_data.h#newco...
net/base/upload_data.h:111: // Once called, this function will always return the
same value.
it looks like this last sentence is only true if type_ is TYPE_FILE?

http://codereview.chromium.org/9321003/diff/4003/net/base/upload_data.h#newco...
net/base/upload_data.h:172: void GetContentLength(const ContentLengthCallback&
callback);
This only hits the IO thread when there are elements of TYPE_FILE afaict. It
seems odd to force all clients to use a (slightly more complicated) async
interface to GetContentLength when only some types of UploadData really require
it. 

Would you consider keeping a GetContentLengthSync() method that works on
UploadData instances that don't refer to file data?

http://codereview.chromium.org/9321003/diff/4003/net/base/upload_data.h#newco...
net/base/upload_data.h:176: // TODO(satorux): Remove this once clients are gone.
Are you planning on attempting to convert the Urlmon client of this in Chrome
Frame to using the asynchronous interface? That seems like it would be hard to
get right. My understanding was that the synchronous interface would remain.

[satorux1, 2012-02-04 07:18:32]

Robert, thank you for the comments. Hope you'll like the new version.

http://codereview.chromium.org/9321003/diff/4003/chrome_frame/plugin_url_requ...
File chrome_frame/plugin_url_request.cc (right):

http://codereview.chromium.org/9321003/diff/4003/chrome_frame/plugin_url_requ...
chrome_frame/plugin_url_request.cc:46: post_data_len_ =
upload_data->GetContentLengthSyncHack();
On 2012/02/04 05:39:04, robertshield wrote:
> This code runs inside Internet Explorer, not Chrome. Does it really require
the
> Hack() version that disables the thread io restrictions? Code in Chrome Frame
> should not be subject to Chrome's thread io restrictions.

That's good to know that. I thought the thread io restrictions were effective
here. Remove Hack() suffix from here.

http://codereview.chromium.org/9321003/diff/4003/net/base/upload_data.cc
File net/base/upload_data.cc (right):

http://codereview.chromium.org/9321003/diff/4003/net/base/upload_data.cc#newc...
net/base/upload_data.cc:223: *content_length +=
elements_.at(i).GetContentLength();
On 2012/02/04 05:39:04, robertshield wrote:
> prefer using iterator syntax:
> std::vector<Element>::iterator iter(elements_.begin());
> for (; iter != elements_.end(); ++iter) { ...

It's more verbose, and I don't see advantage here, but let's not build a bike
shed here. :)

http://codereview.chromium.org/9321003/diff/4003/net/base/upload_data.h
File net/base/upload_data.h (right):

http://codereview.chromium.org/9321003/diff/4003/net/base/upload_data.h#newco...
net/base/upload_data.h:111: // Once called, this function will always return the
same value.
On 2012/02/04 05:39:04, robertshield wrote:
> it looks like this last sentence is only true if type_ is TYPE_FILE?

You are right. The caching works only for TYPE_FILE. Removed the last sentence
as it's just an implementation detail that clients should not care.

http://codereview.chromium.org/9321003/diff/4003/net/base/upload_data.h#newco...
net/base/upload_data.h:172: void GetContentLength(const ContentLengthCallback&
callback);
On 2012/02/04 05:39:04, robertshield wrote:
> This only hits the IO thread when there are elements of TYPE_FILE afaict. It
> seems odd to force all clients to use a (slightly more complicated) async
> interface to GetContentLength when only some types of UploadData really
require
> it. 
>
> Would you consider keeping a GetContentLengthSync() method that works on
> UploadData instances that don't refer to file data?

Sounds reasonable. I originally thought it'd be simpler to provide only the
async version but since Chrome Frame will need the sync version, and the sync
version is useful in tests (FWIW, ResourceDispatcherHost and UploadDataStream
are the only non-test clients in Chrome), I'd keep GetContentLengthSync() 
without the thread IO restrictions allowed.

http://codereview.chromium.org/9321003/diff/4003/net/base/upload_data.h#newco...
net/base/upload_data.h:176: // TODO(satorux): Remove this once clients are gone.
On 2012/02/04 05:39:04, robertshield wrote:
> Are you planning on attempting to convert the Urlmon client of this in Chrome
> Frame to using the asynchronous interface? That seems like it would be hard to
> get right. My understanding was that the synchronous interface would remain.

No. I don't plan to make a major change to chrome_frame code. Besides, since
Chrome Frame clients are not affected by the thread io restrictions, there is no
need to make them async.

So I changed the code to have two sync versions:


GetContentLengthSync() - without AllowIO. used by chrome frame and tests
GetContentLengthSyncHack() - with AllowIO. will be removed.

[grt (UTC plus 2), 2012-02-04 19:33:55]

drive-by STL remark

http://codereview.chromium.org/9321003/diff/4003/net/base/upload_data.cc
File net/base/upload_data.cc (right):

http://codereview.chromium.org/9321003/diff/4003/net/base/upload_data.cc#newc...
net/base/upload_data.cc:223: *content_length +=
elements_.at(i).GetContentLength();
On 2012/02/04 07:18:32, satorux1 wrote:
> On 2012/02/04 05:39:04, robertshield wrote:
> > prefer using iterator syntax:
> > std::vector<Element>::iterator iter(elements_.begin());
> > for (; iter != elements_.end(); ++iter) { ...
> 
> It's more verbose, and I don't see advantage here, but let's not build a bike
> shed here. :)

If you prefer to use an index, use operator[] rather than at().  The latter does
bounds checking and throws exceptions.  You clearly don't need the former since
your loop won't read out of bounds, and you don't want the latter since we don't
do exceptions in Chromium.  Also, if you prefer an index, cache the result of
size() rather than fetching it each time through the loop.

FWIW: I like iterators, but I won't try to convince you to like them too.

Oh, and mauve is an excellent shed color.

[robertshield, 2012-02-04 21:10:56]

lgtm

http://codereview.chromium.org/9321003/diff/4003/net/base/upload_data.cc
File net/base/upload_data.cc (right):

http://codereview.chromium.org/9321003/diff/4003/net/base/upload_data.cc#newc...
net/base/upload_data.cc:223: *content_length +=
elements_.at(i).GetContentLength();
On 2012/02/04 19:33:56, grt wrote:
> On 2012/02/04 07:18:32, satorux1 wrote:
> > On 2012/02/04 05:39:04, robertshield wrote:
> > > prefer using iterator syntax:
> > > std::vector<Element>::iterator iter(elements_.begin());
> > > for (; iter != elements_.end(); ++iter) { ...
> > 
> > It's more verbose, and I don't see advantage here, but let's not build a
bike
> > shed here. :)

Sure, no worries :-) Do note that grt@'s advice on such matters is always worth
heeding.

Except for the color; burgundy is a much better shed color!

> 
> If you prefer to use an index, use operator[] rather than at().  The latter
does
> bounds checking and throws exceptions.  You clearly don't need the former
since
> your loop won't read out of bounds, and you don't want the latter since we
don't
> do exceptions in Chromium.  Also, if you prefer an index, cache the result of
> size() rather than fetching it each time through the loop.
> 
> FWIW: I like iterators, but I won't try to convince you to like them too.
> 
> Oh, and mauve is an excellent shed color.

http://codereview.chromium.org/9321003/diff/4003/net/base/upload_data.h
File net/base/upload_data.h (right):

http://codereview.chromium.org/9321003/diff/4003/net/base/upload_data.h#newco...
net/base/upload_data.h:176: // TODO(satorux): Remove this once clients are gone.
On 2012/02/04 07:18:32, satorux1 wrote:
> On 2012/02/04 05:39:04, robertshield wrote:
> > Are you planning on attempting to convert the Urlmon client of this in
Chrome
> > Frame to using the asynchronous interface? That seems like it would be hard
to
> > get right. My understanding was that the synchronous interface would remain.
> 
> No. I don't plan to make a major change to chrome_frame code. Besides, since
> Chrome Frame clients are not affected by the thread io restrictions, there is
no
> need to make them async.
> 
> So I changed the code to have two sync versions:
> 
> 
> GetContentLengthSync() - without AllowIO. used by chrome frame and tests
> GetContentLengthSyncHack() - with AllowIO. will be removed.

Thanks! That looks good to me.

[satorux1, 2012-02-04 22:40:56]

On 2012/02/04 19:33:55, grt wrote:
> drive-by STL remark
> 
> http://codereview.chromium.org/9321003/diff/4003/net/base/upload_data.cc
> File net/base/upload_data.cc (right):
> 
>
http://codereview.chromium.org/9321003/diff/4003/net/base/upload_data.cc#newc...
> net/base/upload_data.cc:223: *content_length +=
> elements_.at(i).GetContentLength();
> On 2012/02/04 07:18:32, satorux1 wrote:
> > On 2012/02/04 05:39:04, robertshield wrote:
> > > prefer using iterator syntax:
> > > std::vector<Element>::iterator iter(elements_.begin());
> > > for (; iter != elements_.end(); ++iter) { ...
> > 
> > It's more verbose, and I don't see advantage here, but let's not build a
bike
> > shed here. :)
> 
> If you prefer to use an index, use operator[] rather than at().  The latter
does
> bounds checking and throws exceptions.  You clearly don't need the former
since
> your loop won't read out of bounds, and you don't want the latter since we
don't
> do exceptions in Chromium.  Also, if you prefer an index, cache the result of
> size() rather than fetching it each time through the loop.

Thanks. at() was replaced with [] in patch set 3. size() should be inlined, so
i'd keep it as-is.

> 
> FWIW: I like iterators, but I won't try to convince you to like them too.
> 
> Oh, and mauve is an excellent shed color.

[grt (UTC plus 2), 2012-02-05 01:24:38]

On 2012/02/04 22:40:56, satorux1 wrote:
> Thanks. at() was replaced with [] in patch set 3. size() should be inlined, so
> i'd keep it as-is.

True.  On the other hand, the optimizer may not have the smarts to know that the
size doesn't change on each iteration, so (end - begin) may be computed each
time through the loop.  IMO, it's worth the trivial added code to cache the size
so that we know we have a good loop across all of our compilers.  This may not
be universal, so I won't push it.  Just food for thought.

I'll bow out now.  Good luck!

[satorux1, 2012-02-08 17:03:24]

willchan@, could you take a look?

[willchan no longer on Chromium, 2012-02-08 20:44:02]

http://codereview.chromium.org/9321003/diff/19001/net/base/upload_data.cc
File net/base/upload_data.cc (right):

http://codereview.chromium.org/9321003/diff/19001/net/base/upload_data.cc#new...
net/base/upload_data.cc:176: base::Bind(&UploadData::DoGetContentLength, this,
result),
I have to think about this more, but I thought I'd point it out here so you can
think about it too while I am in meetings. This is extending the lifetime of
UploadData. It's possible that all consumers of UploadData are gone and have
released all their references and expect UploadData to go away (and delete the
UploadData::Elements and their FileStreams and what not). I think it's safe to
extend the lifetime a bit (if the WorkerPool task is still outstanding), but
thought I should point out that there's a change in lifetime behavior here that
has some implication.

I pondered suggesting using a base::WeakPtrFactory to avoid extending the
lifetime, but I think it's problematic for us to delete UploadData's Elements
while the DoGetContentLength() operation is underway.

Anyway, some food for thought. I'll think more about it later.

[satorux1, 2012-02-08 21:02:19]

http://codereview.chromium.org/9321003/diff/19001/net/base/upload_data.cc
File net/base/upload_data.cc (right):

http://codereview.chromium.org/9321003/diff/19001/net/base/upload_data.cc#new...
net/base/upload_data.cc:176: base::Bind(&UploadData::DoGetContentLength, this,
result),
On 2012/02/08 20:44:03, willchan wrote:
> I have to think about this more, but I thought I'd point it out here so you
can
> think about it too while I am in meetings. This is extending the lifetime of
> UploadData. It's possible that all consumers of UploadData are gone and have
> released all their references and expect UploadData to go away (and delete the
> UploadData::Elements and their FileStreams and what not). I think it's safe to
> extend the lifetime a bit (if the WorkerPool task is still outstanding), but
> thought I should point out that there's a change in lifetime behavior here
that
> has some implication.
> 
> I pondered suggesting using a base::WeakPtrFactory to avoid extending the
> lifetime, but I think it's problematic for us to delete UploadData's Elements
> while the DoGetContentLength() operation is underway.
> 
> Anyway, some food for thought. I'll think more about it later.

Thank you for sharing the thought.

I think one way to do this without extending the life time is to  compute length
of non-file payloads now, and then compute length of file payloads in the
background. For the latter, we just need file names which can easily be passed
to a callback without referencing 'this'.

However, UploadData::Element::GetContentLength() does some caching not to reopen
the same file twice, which makes it difficult for us to go with the
"non-life-extending" idea. I'd argue that caching here is a premature
optimization and we can remove this behavior. Caching makes code a bit hard to
follow and is one of reasons why a memory leak was introduced
(crrev.com/120952).

[satorux1, 2012-02-09 00:45:47]

Uploaded a new patch with a simple change:

GetContentLengthSyncHack() is removed.
Instead, GetContentLengthSync() is used with explicit
ThreadRestrictions::ScopedIO.

[willchan no longer on Chromium, 2012-02-09 19:15:04]

LGTM

I'm willing to believe that caching is premature optimization. I'd have to think
more and I'd rather unblock you first by approving this changelist, since I
think the lifetime change is benign. If you think you're right, can you file a
bug for it so we can consider a refactor in the future?

http://codereview.chromium.org/9321003/diff/27001/net/base/upload_data_unitte...
File net/base/upload_data_unittest.cc (right):

http://codereview.chromium.org/9321003/diff/27001/net/base/upload_data_unitte...
net/base/upload_data_unittest.cc:57: class UploadDataTest : public PlatformTest
{
You should probably move the protected keyword up here

http://codereview.chromium.org/9321003/diff/27001/net/base/upload_data_unitte...
net/base/upload_data_unittest.cc:146: // The lengh is returned asynchronously.
length

[satorux1, 2012-02-09 19:25:22]

Thanks. I'll submit this for now, and rethink about the caching behavior. I'm
guessing the caching makes it harder to make UploadDataStream::Read()
asynchronous. If that's the case, I'll have to get rid of it. :)

http://codereview.chromium.org/9321003/diff/27001/net/base/upload_data_unitte...
File net/base/upload_data_unittest.cc (right):

http://codereview.chromium.org/9321003/diff/27001/net/base/upload_data_unitte...
net/base/upload_data_unittest.cc:57: class UploadDataTest : public PlatformTest
{
On 2012/02/09 19:15:04, willchan wrote:
> You should probably move the protected keyword up here

Done.

http://codereview.chromium.org/9321003/diff/27001/net/base/upload_data_unitte...
net/base/upload_data_unittest.cc:146: // The lengh is returned asynchronously.
On 2012/02/09 19:15:04, willchan wrote:
> length

Done.

[satorux1, 2012-02-09 23:07:11]

needed to get tony's approval for webkit/ changes

[tony, 2012-02-09 23:11:50]

LGTM

[satorux1, 2012-02-09 23:13:49]

and from jam@ for content/ changes...

[jam, 2012-02-10 00:30:11]

content lgtm