Stop using ScopedAllowIO in content::ResourceDispatcherHostImpl

Stop using ScopedAllowIO in content::ResourceDispatcherHostImpl

Summary:
UploadData::GetContentLengthSync() call is removed from ResourceDispatcherHostImpl.
The return type of net::URLRequest::GetUploadProgress() is changed from uint64 to net::UploadProgress.
A new member 'upload_size' is added to AutomationURLResponse for Chrome Frame.


content::ResourceDispatcherHostImpl has been using UploadData::GetContentLengthSync() which needs ScopedAllowIO.
To stop using ScopedAllowIO, there were three options considered.

 1. Use asynchronous version UploadData::GetContentLength() which does not need ScopedAllowIO.
  pros: Changes would be minimal and straight-forward.
  cons: GetContentLength() is also called in UploadDataStream::Init().  If we start reusing the value, there is no need to call the same method here.

 2. Communicate the upload data size to ResourceDispatcherHost by adding an event OnRequestBodyInitialized() to URLRequest::Delegate.
  pros: No duplicated GetContentLength() call here and in UploadDataStream::Init().
  cons: Complicated interface.

 3. Return upload data size as well as upload progress from URLRequest::GetUploadProgress().
  pros: No duplicated GetContentLength() call here and in UploadDataStream::Init().  Simple interface.  Making sense because upload progress and upload size are almost always used together (see ResourceHandler::OnUploadProgress and DOM ProgressEvent as examples).
  cons: Polling upload data size may imply that the size may change.

We've decided to go with #3.  The return type of net::URLRequest::GetUploadProgress() is changed from uint64 to net::UploadProgress, and URLRequest::GetUploadProgress() is used to get the upload size from ResourceDispatcherHostImpl instead of UploadData::GetContentLengthSync().

In Chrome Frame, URLRequestAutomationJob is used instead of URLRequestHttpJob and UploadDataStream is not initialized in the browser process.
This is problematic since we cannot know the size of upload data without UploadDataStream.
To deal with this, a new member 'upload_size' is added to AutomationURLResponse and the value is used to implement URLRequestAutomationJob::GetUploadProgress().

BUG=112607
TEST=Can upload files

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

[hashimoto, 2012-07-30 08:47:31]

jam,
Could you review this change?

[jam, 2012-07-30 15:25:09]

redirecting to Darin since he's more familiar with this code

[darin (slow to review), 2012-07-31 19:29:46]

I'm not sure this is the best change.  It is adding another asynchronous stage
to the process of loading an URL.  In this case, we only want to know the upload
size so we can generate upload progress events.  We could in theory learn about
the upload size asynchronously, and then start generating upload progress events
later.

Also, it looks like UploadDataStream::Init() also calls GetContentLengthSync()
within a ScopedAllowIO context.

It seems like a better solution would be to add the asynchronous "how big is the
upload data" stage to HttpNetworkTransaction, and then generate an event from
there that can be propagated to the URLRequest::Delegate, which
content::ResourceLoader implements.  Then, the ResourceLoader can trigger the
start of upload progress events.

http://codereview.chromium.org/10825073/diff/13001/content/browser/renderer_h...
File content/browser/renderer_host/resource_dispatcher_host_impl.cc (right):

http://codereview.chromium.org/10825073/diff/13001/content/browser/renderer_h...
content/browser/renderer_host/resource_dispatcher_host_impl.cc:881:
base::Bind(&ResourceDispatcherHostImpl::BeginRequest,
note: The ResourceDispatcherHostImpl out-lives the IO thread, so you can use
Unretained here.

[hashimoto, 2012-08-06 13:29:37]

On 2012/07/31 19:29:46, darin wrote:
> I'm not sure this is the best change.  It is adding another asynchronous stage
> to the process of loading an URL.  In this case, we only want to know the
upload
> size so we can generate upload progress events.  We could in theory learn
about
> the upload size asynchronously, and then start generating upload progress
events
> later.
> 
> Also, it looks like UploadDataStream::Init() also calls GetContentLengthSync()
> within a ScopedAllowIO context.
> 
> It seems like a better solution would be to add the asynchronous "how big is
the
> upload data" stage to HttpNetworkTransaction, and then generate an event from
> there that can be propagated to the URLRequest::Delegate, which
> content::ResourceLoader implements.  Then, the ResourceLoader can trigger the
> start of upload progress events.
Sounds good.  Since net::UploadDataStream::Init() is already calling
GetContentLength(), we can reuse the value and remove the unnecessary file IO.

The problem is, however, URLRequestAutomationJob::GetUploadProgress is relying
on the upload data size which is set with GetContentLengthSync() in
ResourceDispatcherHostImpl
(http://code.google.com/p/chromium/source/search?q=URLRequestAutomationJob::Ge...)
Since net::UploadDataStream::Init() is not called in the process where
URLRequestAutomationJob belongs, we have to pass the upload data size from
ChromeFrameAutomationClient to URLRequestAutomationJob through IPC.

So, how about returning upload data size from URLRequest::GetUploadProgress(),
instead of adding a method to URLRequest::Delegate?
I think it's easier to use than adding a method to delegates.  Also it makes
sense because upload data size and upload position are almost always used
together.

I've made a series of CLs to change the return type of GetUploadProgress()
(http://codereview.chromium.org/10834178/,
http://codereview.chromium.org/10830182/)
Though it still lacks a CL to pass the upload data size from
ChromeFrameAutomationClient to URLRequestAutomationJob through IPC.

What do you think?

> 
>
http://codereview.chromium.org/10825073/diff/13001/content/browser/renderer_h...
> File content/browser/renderer_host/resource_dispatcher_host_impl.cc (right):
> 
>
http://codereview.chromium.org/10825073/diff/13001/content/browser/renderer_h...
> content/browser/renderer_host/resource_dispatcher_host_impl.cc:881:
> base::Bind(&ResourceDispatcherHostImpl::BeginRequest,
> note: The ResourceDispatcherHostImpl out-lives the IO thread, so you can use
> Unretained here.

[darin (slow to review), 2012-08-06 22:48:51]

On Mon, Aug 6, 2012 at 6:29 AM, <hashimoto@chromium.org> wrote:

> On 2012/07/31 19:29:46, darin wrote:
>
>> I'm not sure this is the best change.  It is adding another asynchronous
>> stage
>> to the process of loading an URL.  In this case, we only want to know the
>>
> upload
>
>> size so we can generate upload progress events.  We could in theory learn
>>
> about
>
>> the upload size asynchronously, and then start generating upload progress
>>
> events
>
>> later.
>>
>
>  Also, it looks like UploadDataStream::Init() also calls
>> GetContentLengthSync()
>> within a ScopedAllowIO context.
>>
>
>  It seems like a better solution would be to add the asynchronous "how big
>> is
>>
> the
>
>> upload data" stage to HttpNetworkTransaction, and then generate an event
>> from
>> there that can be propagated to the URLRequest::Delegate, which
>> content::ResourceLoader implements.  Then, the ResourceLoader can trigger
>> the
>> start of upload progress events.
>>
> Sounds good.  Since net::UploadDataStream::Init() is already calling
> GetContentLength(), we can reuse the value and remove the unnecessary file
> IO.
>
> The problem is, however, URLRequestAutomationJob::**GetUploadProgress is
> relying
> on the upload data size which is set with GetContentLengthSync() in
> ResourceDispatcherHostImpl
> (http://code.google.com/p/**chromium/source/search?q=**
>
URLRequestAutomationJob::**GetUploadProgress<http://code.google.com/p/chromium/source/search?q=URLRequestAutomationJob::GetUploadProgress>
> )
> Since net::UploadDataStream::Init() is not called in the process where
> URLRequestAutomationJob belongs, we have to pass the upload data size from
> ChromeFrameAutomationClient to URLRequestAutomationJob through IPC.
>
> So, how about returning upload data size from
> URLRequest::GetUploadProgress(**),
> instead of adding a method to URLRequest::Delegate?
> I think it's easier to use than adding a method to delegates.  Also it
> makes
> sense because upload data size and upload position are almost always used
> together.
>

Hmm... yeah, since progress is polled, it makes sense to also poll the
upload size.  I'm not sure that I would always fetch the size and progress
values together.  Afterall, the upload size is a constant.  If
GetUploadProgress returns both progress and size, then the client might
need to worry about the upload size changing!

-Darin


>
> I've made a series of CLs to change the return type of GetUploadProgress()
>
(http://codereview.chromium.**org/10834178/<http://codereview.chromium.org/108...
> ,
>
http://codereview.chromium.**org/10830182/<http://codereview.chromium.org/108...
> )
> Though it still lacks a CL to pass the upload data size from
> ChromeFrameAutomationClient to URLRequestAutomationJob through IPC.
>
> What do you think?
>
>
>
>
> http://codereview.chromium.**org/10825073/diff/13001/**
>
content/browser/renderer_host/**resource_dispatcher_host_impl.**cc<http://codereview.chromium.org/10825073/diff/13001/content/browser/renderer_host/resource_dispatcher_host_impl.cc>
>
>> File content/browser/renderer_host/**resource_dispatcher_host_impl.**cc
>> (right):
>>
>
>
> http://codereview.chromium.**org/10825073/diff/13001/**
> content/browser/renderer_host/**resource_dispatcher_host_impl.**
>
cc#newcode881<http://codereview.chromium.org/10825073/diff/13001/content/browser/renderer_host/resource_dispatcher_host_impl.cc#newcode881>
>
>> content/browser/renderer_host/**resource_dispatcher_host_impl.**cc:881:
>> base::Bind(&**ResourceDispatcherHostImpl::**BeginRequest,
>> note: The ResourceDispatcherHostImpl out-lives the IO thread, so you can
>> use
>> Unretained here.
>>
>
>
>
http://codereview.chromium.**org/10825073/<http://codereview.chromium.org/108...
>

[hashimoto, 2012-08-07 10:38:50]

Added URLRequest::Delegate::OnRequestBodyInitialized to get the upload data
size.
This change is still not finished, there should be some chromeframe-browser IPC
fix to make URLRequestAutomationJob::GetUploadProgress work.

Darin, can I have your initial feedback about this change?

[hashimoto, 2012-08-08 09:36:02]

New patch set, now Chrome Frame works well.
PTAL

[hashimoto, 2012-08-14 00:34:42]

On 2012/08/08 09:36:02, hashimoto wrote:
> New patch set, now Chrome Frame works well.
> PTAL

ping?

[darin (slow to review), 2012-08-14 18:24:26]

http://codereview.chromium.org/10825073/diff/17001/net/url_request/url_request.h
File net/url_request/url_request.h (right):

http://codereview.chromium.org/10825073/diff/17001/net/url_request/url_reques...
net/url_request/url_request.h:239: virtual void
OnRequestBodyInitialized(URLRequest* request, uint64 size);
Does this really need to be an event?  Why not just let the caller poll for the
upload size?  Have a GetUploadSize function that returns -1 or fail if the
upload size is unknown?  I'd also be OK with GetUploadProgress having an out
parameter to report the total size:

  // Returns true if progress has been made uploading data and reports
  // the upload progress and total upload size in bytes.  Otherwise,
  // returns false if there is no upload data or if we haven't begun
  // uploading data yet.
  bool GetUploadProgress(uint64* progress, uint64* total) const;

In other words, since we don't need an edge trigger for when the upload begins,
we could very well do away with the event and just simply provide a polling
interface.

[darin (slow to review), 2012-08-14 18:27:37]

On 2012/08/14 18:24:26, darin wrote:
>
http://codereview.chromium.org/10825073/diff/17001/net/url_request/url_request.h
> File net/url_request/url_request.h (right):
> 
>
http://codereview.chromium.org/10825073/diff/17001/net/url_request/url_reques...
> net/url_request/url_request.h:239: virtual void
> OnRequestBodyInitialized(URLRequest* request, uint64 size);
> Does this really need to be an event?  Why not just let the caller poll for
the
> upload size?  Have a GetUploadSize function that returns -1 or fail if the
> upload size is unknown?  I'd also be OK with GetUploadProgress having an out
> parameter to report the total size:
> 
>   // Returns true if progress has been made uploading data and reports
>   // the upload progress and total upload size in bytes.  Otherwise,
>   // returns false if there is no upload data or if we haven't begun
>   // uploading data yet.
>   bool GetUploadProgress(uint64* progress, uint64* total) const;
> 
> In other words, since we don't need an edge trigger for when the upload
begins,
> we could very well do away with the event and just simply provide a polling
> interface.

Oh, sorry... I just realized that I am contradicting my earlier code review
feedback!  D'oh!  Sorry for that :-(

Let me think about this more and get back to you.

[darin (slow to review), 2012-08-14 18:34:02]

Yes, upon further reflection, I think GetUploadProgress that reports both
progress and total byte counts is the way to go.  Sorry for the randomization. 
I realize that you were closer to this solution earlier.

Reasons:

1-  Originally, I thought that an event on URLRequest::Delegate would allow us
to delay starting the timer that periodically updates the upload progress
reporter.  However, that is driven off of the shared timer that drives
ResourceDispatcherHostImpl::UpdateLoadStates.  So, having an edge trigger for
learning the upload size is not needed.

2-  Also, I was concerned about code having to deal with the possibility of the
total byte count changing, but in reality we already have plenty of other places
in the code that report progress and total byte counts together.  See for
example ResourceHandler::OnUploadProgress as well as the DOM ProgressEvent!

[hashimoto, 2012-08-16 11:46:13]

On 2012/08/14 18:27:37, darin wrote:
> On 2012/08/14 18:24:26, darin wrote:
> >
>
http://codereview.chromium.org/10825073/diff/17001/net/url_request/url_request.h
> > File net/url_request/url_request.h (right):
> > 
> >
>
http://codereview.chromium.org/10825073/diff/17001/net/url_request/url_reques...
> > net/url_request/url_request.h:239: virtual void
> > OnRequestBodyInitialized(URLRequest* request, uint64 size);
> > Does this really need to be an event?  Why not just let the caller poll for
> the
> > upload size?  Have a GetUploadSize function that returns -1 or fail if the
> > upload size is unknown?  I'd also be OK with GetUploadProgress having an out
> > parameter to report the total size:
> > 
> >   // Returns true if progress has been made uploading data and reports
> >   // the upload progress and total upload size in bytes.  Otherwise,
> >   // returns false if there is no upload data or if we haven't begun
> >   // uploading data yet.
> >   bool GetUploadProgress(uint64* progress, uint64* total) const;
> > 
> > In other words, since we don't need an edge trigger for when the upload
> begins,
> > we could very well do away with the event and just simply provide a polling
> > interface.
> 
> Oh, sorry... I just realized that I am contradicting my earlier code review
> feedback!  D'oh!  Sorry for that :-(
> 
> Let me think about this more and get back to you.

No problem.
I'm happy to hear that we've agreed to go with this solution :)

[hashimoto, 2012-08-16 11:47:28]

willchan@,
Do changes for net/ looks good to you?

HttpTransaction change is split as http://codereview.chromium.org/10834178/ to
avoid patch bloat.
Please look at this change too.

[willchan no longer on Chromium, 2012-08-16 19:27:15]

Can you update the changelist description with what's going on here? How are we
avoiding the ScopedAllowIO code? I see that there's a lot of discussion with
Darin on tradeoffs, it'd be great if you could summarize that in the changelist
description to explain what's going on.

Sorry for the extra roundtrip, but I'm overloaded with reviews and it'd help if
the changelist description had a more thorough explanation to help me review
more quickly.

[hashimoto, 2012-08-17 05:36:45]

On 2012/08/16 19:27:15, willchan wrote:
> Can you update the changelist description with what's going on here? How are
we
> avoiding the ScopedAllowIO code? I see that there's a lot of discussion with
> Darin on tradeoffs, it'd be great if you could summarize that in the
changelist
> description to explain what's going on.
> 
> Sorry for the extra roundtrip, but I'm overloaded with reviews and it'd help
if
> the changelist description had a more thorough explanation to help me review
> more quickly.
No problem.  Updated the description with the summary of the discussion.

[willchan no longer on Chromium, 2012-08-17 14:12:29]

lgtm

[hashimoto, 2012-08-27 18:27:04]

darin,
Could you take another look on this change?

[darin (slow to review), 2012-08-29 21:47:56]

Is this CL missing net/base/upload_progress.h?

[hashimoto, 2012-08-29 21:49:38]

On 2012/08/29 21:47:56, darin wrote:
> Is this CL missing net/base/upload_progress.h?

It's split to another change. http://codereview.chromium.org/10834178/

[darin (slow to review), 2012-08-29 22:36:16]

I'm assuming that UploadProgress can become a class that only provides methods
to read progress and size attributes.

http://codereview.chromium.org/10825073/diff/22001/content/browser/renderer_h...
File content/browser/renderer_host/resource_dispatcher_host_impl.cc (right):

http://codereview.chromium.org/10825073/diff/22001/content/browser/renderer_h...
content/browser/renderer_host/resource_dispatcher_host_impl.cc:1600: const
net::UploadProgress progress = request->GetUploadProgress();
nit: no need for 'const' here.

http://codereview.chromium.org/10825073/diff/22001/content/browser/renderer_h...
File content/browser/renderer_host/resource_loader.cc (right):

http://codereview.chromium.org/10825073/diff/22001/content/browser/renderer_h...
content/browser/renderer_host/resource_loader.cc:110: const net::UploadProgress
progress = request_->GetUploadProgress();
nit: no need for 'const' here.

http://codereview.chromium.org/10825073/diff/22001/webkit/tools/test_shell/si...
File webkit/tools/test_shell/simple_resource_loader_bridge.cc (right):

http://codereview.chromium.org/10825073/diff/22001/webkit/tools/test_shell/si...
webkit/tools/test_shell/simple_resource_loader_bridge.cc:621: const
net::UploadProgress progress = request_->GetUploadProgress();
nit: no need for 'const' here

[hashimoto, 2012-08-29 22:48:55]

Rebased onto the latest patch set of http://codereview.chromium.org/10834178/

http://codereview.chromium.org/10825073/diff/22001/content/browser/renderer_h...
File content/browser/renderer_host/resource_dispatcher_host_impl.cc (right):

http://codereview.chromium.org/10825073/diff/22001/content/browser/renderer_h...
content/browser/renderer_host/resource_dispatcher_host_impl.cc:1600: const
net::UploadProgress progress = request->GetUploadProgress();
On 2012/08/29 22:36:16, darin wrote:
> nit: no need for 'const' here.

Done.

http://codereview.chromium.org/10825073/diff/22001/content/browser/renderer_h...
File content/browser/renderer_host/resource_loader.cc (right):

http://codereview.chromium.org/10825073/diff/22001/content/browser/renderer_h...
content/browser/renderer_host/resource_loader.cc:110: const net::UploadProgress
progress = request_->GetUploadProgress();
On 2012/08/29 22:36:16, darin wrote:
> nit: no need for 'const' here.

Done.

http://codereview.chromium.org/10825073/diff/22001/webkit/tools/test_shell/si...
File webkit/tools/test_shell/simple_resource_loader_bridge.cc (right):

http://codereview.chromium.org/10825073/diff/22001/webkit/tools/test_shell/si...
webkit/tools/test_shell/simple_resource_loader_bridge.cc:621: const
net::UploadProgress progress = request_->GetUploadProgress();
On 2012/08/29 22:36:16, darin wrote:
> nit: no need for 'const' here

Done.

[darin (slow to review), 2012-08-29 22:55:27]

LGTM!

[commit-bot: I haz the power, 2012-08-30 03:54:07]

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

[commit-bot: I haz the power, 2012-08-30 05:19:53]

Try job failure for 10825073-36001 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-08-30 07:27:59]

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

[commit-bot: I haz the power, 2012-08-30 09:22:11]

Try job failure for 10825073-36001 (retry) on linux_rel for step
"interactive_ui_tests".
It's a second try, previously, step "interactive_ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-08-30 18:11:03]

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

[commit-bot: I haz the power, 2012-08-30 22:30:53]

Change committed as 154286