net: Move ownership of UploadDataStream from URLRequestHttpJob to URLRequest

net: Move ownership of UploadDataStream from URLRequestHttpJob to URLRequest

URLRequest creates and owns UploadDataStream.
URLRequest::get_upload_mutable and URLRequest::AppendBytesToUpload are removed because they can modify UploadData after UploadDataStream is created.
Return type of URLRequest::get_upload is changed from UploadData to UploadDataStream.
A number of methods are added to UploadElementReader and its subclasses to support URLRequestAutomationJob and some other users.

BUG=156574
TEST=git try

TBR=jam@chromium.org (for chrome/browser/automation, already reviewed by ananta@chromium.org)

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

[hashimoto, 2012-11-16 11:42:55]

Please review.

This patch is a temporary step before changing URLRequest::set_upload argument
type to scoped_ptr<UploadDataStream>.

[mmenke, 2012-11-19 17:21:20]

Looks good, just wondering about |disable_upload_|

Think the necessity of AsBytesReader() / AsFileReader() is a bit unfortunate,
but looks like it's needed.

https://chromiumcodereview.appspot.com/11419034/diff/33/chrome/browser/extens...
File chrome/browser/extensions/api/web_request/upload_data_presenter.h (right):

https://chromiumcodereview.appspot.com/11419034/diff/33/chrome/browser/extens...
chrome/browser/extensions/api/web_request/upload_data_presenter.h:93: // content
of such elements. If the parsing is successfull, the parsed form is
while you're here, mind correcting the spelling of successful?

https://chromiumcodereview.appspot.com/11419034/diff/33/net/url_request/url_r...
File net/url_request/url_request.cc (right):

https://chromiumcodereview.appspot.com/11419034/diff/33/net/url_request/url_r...
net/url_request/url_request.cc:282: const UploadDataStream*
URLRequest::get_upload() const {
For this function and the next, what if disable_upload_ is true?

https://chromiumcodereview.appspot.com/11419034/diff/33/net/url_request/url_r...
net/url_request/url_request.cc:781: disable_upload_ = true;
Why do we need to keep the stream around?  Since we have a response, the entire
request has already been sent, so it should no longer be needed by the stream.

We already basically do the same thing when re-initializing the UploadStream -
any further access by the old stream would be a problem.

[hashimoto, 2012-11-20 07:47:45]

On 2012/11/19 17:21:20, Matt Menke wrote:
> Looks good, just wondering about |disable_upload_|
> 
> Think the necessity of AsBytesReader() / AsFileReader() is a bit unfortunate,
> but looks like it's needed.
I don't like these down-cast methods neither.
But, as you know, it's needed to support existing code. (Chrome Frame,
WebRequest API, and some tests)
The biggest problem here is that we cannot have down-cast methods for
user-defined UploadElementReaders. (e.g. one for FileSystem files)
I think this will not be a major problem because we are not going to support
FileSystem files with Chrome Frame, and what other users really care about is
only Bytes data.
> 
>
https://chromiumcodereview.appspot.com/11419034/diff/33/chrome/browser/extens...
> File chrome/browser/extensions/api/web_request/upload_data_presenter.h
(right):
> 
>
https://chromiumcodereview.appspot.com/11419034/diff/33/chrome/browser/extens...
> chrome/browser/extensions/api/web_request/upload_data_presenter.h:93: //
content
> of such elements. If the parsing is successfull, the parsed form is
> while you're here, mind correcting the spelling of successful?
> 
>
https://chromiumcodereview.appspot.com/11419034/diff/33/net/url_request/url_r...
> File net/url_request/url_request.cc (right):
> 
>
https://chromiumcodereview.appspot.com/11419034/diff/33/net/url_request/url_r...
> net/url_request/url_request.cc:282: const UploadDataStream*
> URLRequest::get_upload() const {
> For this function and the next, what if disable_upload_ is true?
> 
>
https://chromiumcodereview.appspot.com/11419034/diff/33/net/url_request/url_r...
> net/url_request/url_request.cc:781: disable_upload_ = true;
> Why do we need to keep the stream around?  Since we have a response, the
entire
> request has already been sent, so it should no longer be needed by the stream.
> 
> We already basically do the same thing when re-initializing the UploadStream -
> any further access by the old stream would be a problem.

[hashimoto, 2012-11-20 07:48:46]

New patch set.
PTAL

http://codereview.chromium.org/11419034/diff/33/chrome/browser/extensions/api...
File chrome/browser/extensions/api/web_request/upload_data_presenter.h (right):

http://codereview.chromium.org/11419034/diff/33/chrome/browser/extensions/api...
chrome/browser/extensions/api/web_request/upload_data_presenter.h:93: // content
of such elements. If the parsing is successfull, the parsed form is
On 2012/11/19 17:21:21, Matt Menke wrote:
> while you're here, mind correcting the spelling of successful?

Done.

http://codereview.chromium.org/11419034/diff/33/net/url_request/url_request.cc
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/11419034/diff/33/net/url_request/url_request.c...
net/url_request/url_request.cc:282: const UploadDataStream*
URLRequest::get_upload() const {
On 2012/11/19 17:21:21, Matt Menke wrote:
> For this function and the next, what if disable_upload_ is true?

Removed disable_upload_.

http://codereview.chromium.org/11419034/diff/33/net/url_request/url_request.c...
net/url_request/url_request.cc:781: disable_upload_ = true;
On 2012/11/19 17:21:21, Matt Menke wrote:
> Why do we need to keep the stream around?  Since we have a response, the
entire
> request has already been sent, so it should no longer be needed by the stream.
I thought it's good to disable the stream instead of discarding it because we
can reuse it, but I agree that no one needs to reuse it.
> 
> We already basically do the same thing when re-initializing the UploadStream -
> any further access by the old stream would be a problem.
job_->GetUploadProgress() in line 803 accesses UploadDataStream so we should
keep it alive at least until then.
Also, since URLRequest::OrphanJob is not called until PrepareToRestart, I'd like
to keep the stream until then.
How does this new change look?

[mmenke, 2012-11-20 19:57:03]

LGTM

https://codereview.chromium.org/11419034/diff/34/net/url_request/url_request.cc
File net/url_request/url_request.cc (right):

https://codereview.chromium.org/11419034/diff/34/net/url_request/url_request....
net/url_request/url_request.cc:802: final_upload_progress_ =
job_->GetUploadProgress();
Can't we just move this up to before the block of code that rewrites the method,
and get rid of discard_upload?

If I'm missing something, think we should at least have a comment for the reason
discard_upload is needed.

[hashimoto, 2012-11-21 03:41:02]

https://codereview.chromium.org/11419034/diff/34/net/url_request/url_request.cc
File net/url_request/url_request.cc (right):

https://codereview.chromium.org/11419034/diff/34/net/url_request/url_request....
net/url_request/url_request.cc:802: final_upload_progress_ =
job_->GetUploadProgress();
On 2012/11/20 19:57:03, Matt Menke wrote:
> Can't we just move this up to before the block of code that rewrites the
method,
> and get rid of discard_upload?
> 
> If I'm missing something, think we should at least have a comment for the
reason
> discard_upload is needed.

Moved GetUploadProgress() and PrepareToRestart() to before the method rewriting
block.

PrepareToRestart() is also moved because, from URLRequestJob's point of view,
UploadDataStream should be alive at least until URLRequestJob::DetachRequest(),
which is called by URLRequest::OrphanJob(), is called.

[hashimoto, 2012-11-21 03:55:08]

jam@, mpcomplete@, mnissler@, marja@,
Please review as an owner of each directry.

jam: chrome/browser/automation/url_request_automation_job.cc
mpcomplete: chrome/browser/extensions/api/web_request/
mnissler: chrome/browser/policy/device_management_service_browsertest.cc
marja: chrome/browser/sessions/better_session_restore_browsertest.cc

[Mattias Nissler (ping if slow), 2012-11-21 09:41:46]

LGTM for chrome/browser/policy

[marja, 2012-11-21 09:51:48]

chrome/browser/sessions/better_session_restore_browsertest.cc LGTM

[jam, 2012-11-21 19:26:47]

owner rubberstamp (I'll defer to other reviewers for checking that the
conversion is right)

[hashimoto, 2012-11-26 06:32:31]

ananta@, mpcomplete@,
Please review as an owner of each directry.

ananta: chrome/browser/automation/url_request_automation_job.cc (on behalf of
jam)
mpcomplete: chrome/browser/extensions/api/web_request/

[Matt Perry, 2012-11-26 22:15:53]

extensions lgtm

[hashimoto, 2012-11-27 11:22:21]

ananta@, 
Please review this change as an owner
of　chrome/browser/automation/url_request_automation_job.cc (on behalf of
jam)

[hashimoto, 2012-11-28 00:57:58]

Pawel, could you review this change as an owner of chrome/browser/automation?

[ananta, 2012-11-28 02:21:18]

https://codereview.chromium.org/11419034/diff/8041/chrome/browser/automation/...
File chrome/browser/automation/url_request_automation_job.cc (right):

https://codereview.chromium.org/11419034/diff/8041/chrome/browser/automation/...
chrome/browser/automation/url_request_automation_job.cc:470: const
net::UploadDataStream* upload_data_stream = request_->get_upload();
This code seems generic enough to move to a helper function?

[hashimoto, 2012-11-28 02:34:05]

https://codereview.chromium.org/11419034/diff/8041/chrome/browser/automation/...
File chrome/browser/automation/url_request_automation_job.cc (right):

https://codereview.chromium.org/11419034/diff/8041/chrome/browser/automation/...
chrome/browser/automation/url_request_automation_job.cc:470: const
net::UploadDataStream* upload_data_stream = request_->get_upload();
On 2012/11/28 02:21:18, ananta wrote:
> This code seems generic enough to move to a helper function?

Done.

[ananta, 2012-11-28 05:29:36]

LGTM

https://codereview.chromium.org/11419034/diff/3102/chrome/browser/automation/...
File chrome/browser/automation/url_request_automation_job.cc (right):

https://codereview.chromium.org/11419034/diff/3102/chrome/browser/automation/...
chrome/browser/automation/url_request_automation_job.cc:500: const
net::UploadDataStream* upload_data_stream = request_->get_upload();
We can nuke the upload_data_stream local here and directly reference the
get_upload() member where needed.

[hashimoto, 2012-11-28 05:54:01]

https://codereview.chromium.org/11419034/diff/3102/chrome/browser/automation/...
File chrome/browser/automation/url_request_automation_job.cc (right):

https://codereview.chromium.org/11419034/diff/3102/chrome/browser/automation/...
chrome/browser/automation/url_request_automation_job.cc:500: const
net::UploadDataStream* upload_data_stream = request_->get_upload();
On 2012/11/28 05:29:37, ananta wrote:
> We can nuke the upload_data_stream local here and directly reference the
> get_upload() member where needed.

Done.

[commit-bot: I haz the power, 2012-11-28 09:09:05]

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

[commit-bot: I haz the power, 2012-11-28 09:09:21]

Presubmit check for 11419034-11047 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
You might be calling functions intended only for testing from
production code.  It is OK to ignore this warning if you know what
you are doing, as the heuristics used to detect the situation are
not perfect.  The commit queue will not block on this warning.
Email joi@chromium.org if you have questions.
  net/ocsp/nss_ocsp.cc:954
    g_ocsp_io_loop.Get().ReuseForTesting();

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/automation

Presubmit checks took 5.7s to calculate.

[hashimoto, 2012-11-28 16:07:38]

TBR-ing jam@ for chrome/browser/automation since it's already reviewed by
ananta@

[commit-bot: I haz the power, 2012-11-28 16:08:28]

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

[commit-bot: I haz the power, 2012-11-28 19:50:44]

Change committed as 170028