Split net::UploadData into two: for IPC and for upload handling

Split net::UploadData into two: for resource request IPC and for upload handling

Introducing webkit_glue::ResourceRequestBody as a content-level abstraction corresponding to WebHTTPBody and as an alternative of net::UploadData in ResourceRequest.

This interface can contain content-level objects like Blob (or FileSystem URL in later patches) while net::UploadData should NOT.

This patch also removes Blob support in net::UploadData.

BUG=110119
TEST=existing tests

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

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

[kinuko, 2012-08-13 15:52:09]

http://codereview.chromium.org/10834289/diff/9001/webkit/glue/webupload_data.h
File webkit/glue/webupload_data.h (right):

http://codereview.chromium.org/10834289/diff/9001/webkit/glue/webupload_data....
webkit/glue/webupload_data.h:24: class WEBKIT_GLUE_EXPORT WebUploadData
Having this in webkit/glue is probably not ideal

[kinuko, 2012-08-13 16:35:52]

This is a CL split out from http://codereview.chromium.org/10828252/ (support
FileSystem URL in File), adding a different data structure for IPC serialization
of upload data and removing Blob type from original net::Upload.

This is still a bit rough but can you take a look to see if it's in a good
direction?  Thanks so much.

[darin (slow to review), 2012-08-13 16:48:49]

http://codereview.chromium.org/10834289/diff/9001/webkit/glue/webupload_data.h
File webkit/glue/webupload_data.h (right):

http://codereview.chromium.org/10834289/diff/9001/webkit/glue/webupload_data....
webkit/glue/webupload_data.h:24: class WEBKIT_GLUE_EXPORT WebUploadData
On 2012/08/13 15:52:09, kinuko wrote:
> Having this in webkit/glue is probably not ideal

Seems like it could live in content/public/common/ or content/common/.  If it is
not needed by webkit/glue/, then don't put it there ;-)

Alternatively, if you think it would make sense for WebURLLoaderImpl to create
WebUploadData and pass that to ResourceLoaderBridge (instead of having all of
those Append* functions on ResourceLoaderBridge), then it could make sense for
WebUploadData to live here.

[darin (slow to review), 2012-08-13 20:47:36]

http://codereview.chromium.org/10834289/diff/9001/webkit/glue/webupload_data.h
File webkit/glue/webupload_data.h (right):

http://codereview.chromium.org/10834289/diff/9001/webkit/glue/webupload_data....
webkit/glue/webupload_data.h:24: class WEBKIT_GLUE_EXPORT WebUploadData
On 2012/08/13 16:48:49, darin wrote:
> On 2012/08/13 15:52:09, kinuko wrote:
> > Having this in webkit/glue is probably not ideal
> 
> Seems like it could live in content/public/common/ or content/common/.  If it
is
> not needed by webkit/glue/, then don't put it there ;-)
> 
> Alternatively, if you think it would make sense for WebURLLoaderImpl to create
> WebUploadData and pass that to ResourceLoaderBridge (instead of having all of
> those Append* functions on ResourceLoaderBridge), then it could make sense for
> WebUploadData to live here.

Also, it might be good to use a prefix of "Resource" to be consistent with other
related classes in the content module.  ResourceUploadData or
ResourceRequestBody?  We already have ResourceLoaderBridge::RequestInfo and
ResourceResponse.  I think we might end up with a content::ResourceRequest in
the future.  content::ResourceRequestBody seems to complement that well.

[kinuko, 2012-08-14 15:33:31]

http://codereview.chromium.org/10834289/diff/9001/webkit/glue/webupload_data.h
File webkit/glue/webupload_data.h (right):

http://codereview.chromium.org/10834289/diff/9001/webkit/glue/webupload_data....
webkit/glue/webupload_data.h:24: class WEBKIT_GLUE_EXPORT WebUploadData
On 2012/08/13 20:47:36, darin wrote:
> On 2012/08/13 16:48:49, darin wrote:
> > On 2012/08/13 15:52:09, kinuko wrote:
> > > Having this in webkit/glue is probably not ideal
> > 
> > Seems like it could live in content/public/common/ or content/common/.  If
it
> is
> > not needed by webkit/glue/, then don't put it there ;-)
> > 
> > Alternatively, if you think it would make sense for WebURLLoaderImpl to
create
> > WebUploadData and pass that to ResourceLoaderBridge (instead of having all
of
> > those Append* functions on ResourceLoaderBridge), then it could make sense
for
> > WebUploadData to live here.

I think I like the latter idea (since it can kill more code), so I took the way.

> Also, it might be good to use a prefix of "Resource" to be consistent with
other
> related classes in the content module.  ResourceUploadData or
> ResourceRequestBody?  We already have ResourceLoaderBridge::RequestInfo and
> ResourceResponse.  I think we might end up with a content::ResourceRequest in
> the future.  content::ResourceRequestBody seems to complement that well.

Sounds good! Done.

[kinuko, 2012-08-15 04:45:01]

http://codereview.chromium.org/10834289/diff/9013/content/common/resource_dis...
File content/common/resource_dispatcher.h (right):

http://codereview.chromium.org/10834289/diff/9013/content/common/resource_dis...
content/common/resource_dispatcher.h:25: }
will remove this

[kinuko, 2012-08-15 07:27:27]

(+willchan, cc-ing hashimoto@)

Now tests look ok and we have less data copies.
Darin: could you do one more review?
Willchan: can you take a look in net/base changes?

Thanks!

[darin (slow to review), 2012-08-15 17:49:55]

Overall, this looks great!

http://codereview.chromium.org/10834289/diff/9013/chrome/browser/policy/devic...
File chrome/browser/policy/device_management_service_browsertest.cc (right):

http://codereview.chromium.org/10834289/diff/9013/chrome/browser/policy/devic...
chrome/browser/policy/device_management_service_browsertest.cc:66: void
ConstructResponse(const void* request_data,
nit: |const char*| instead since that's what UploadData::Element::bytes()
returns?

http://codereview.chromium.org/10834289/diff/9013/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host_impl.cc (right):

http://codereview.chromium.org/10834289/diff/9013/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host_impl.cc:86: #include
"webkit/glue/resource_request_body.h"
nit: sort

http://codereview.chromium.org/10834289/diff/9013/content/common/resource_dis...
File content/common/resource_dispatcher.cc (right):

http://codereview.chromium.org/10834289/diff/9013/content/common/resource_dis...
content/common/resource_dispatcher.cc:25: #include
"webkit/glue/resource_request_body.h"
nit: sort

http://codereview.chromium.org/10834289/diff/9013/content/common/resource_dis...
content/common/resource_dispatcher.cc:29: using
webkit_glue::ResourceRequestBody;
nit: sort

http://codereview.chromium.org/10834289/diff/9013/content/public/common/commo...
File content/public/common/common_param_traits.cc (right):

http://codereview.chromium.org/10834289/diff/9013/content/public/common/commo...
content/public/common/common_param_traits.cc:131: struct
ParamTraits<net::UploadData::Element> {
Who still relies on UploadData serialization?

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

http://codereview.chromium.org/10834289/diff/9013/net/base/upload_data.h#newc...
net/base/upload_data.h:66: const char* bytes() const { return bytes_start_ ?
bytes_start_ : &buf_[0]; }
maybe you should just ensure that bytes_start_ is always set?

http://codereview.chromium.org/10834289/diff/9013/net/base/upload_data.h#newc...
net/base/upload_data.h:81: void SetToSharedBytes(const char* bytes, int
bytes_len) {
You should probably add a warning here about memory management.  The trick about
having to use UserData to ensure that the lifetime of this pointer remains valid
for the lifetime of the UploadData object is not so obvious.  A comment about
that might help.

http://codereview.chromium.org/10834289/diff/9013/webkit/blob/blob_storage_co...
File webkit/blob/blob_storage_controller.cc (right):

http://codereview.chromium.org/10834289/diff/9013/webkit/blob/blob_storage_co...
webkit/blob/blob_storage_controller.cc:174: void
BlobStorageController::ResolveBlobReferencesInResourceRequestBody(
nit: I'd be OK dropping the "Resource" in this function name just as you did on
ResourceLoaderBridge with the SetRequestBody method.

http://codereview.chromium.org/10834289/diff/9013/webkit/glue/resource_reques...
File webkit/glue/resource_request_body.cc (right):

http://codereview.chromium.org/10834289/diff/9013/webkit/glue/resource_reques...
webkit/glue/resource_request_body.cc:49: upload_data->SetUserData(
Oh, I see!  You should add a comment about how this relates to the
SetToSharedData usage.

http://codereview.chromium.org/10834289/diff/9013/webkit/glue/resource_reques...
File webkit/glue/resource_request_body.h (right):

http://codereview.chromium.org/10834289/diff/9013/webkit/glue/resource_reques...
webkit/glue/resource_request_body.h:22: namespace webkit_glue {
nit: add a new line below here

[willchan no longer on Chromium, 2012-08-16 01:51:39]

I'm a bit disappointed that we are taking something quite ugly (UploadData) and
mostly duplicating it. What's the long term plan here? In another thread, Darin
mentioned abstracting UploadDataStream. That sounds great to me. I'd hope that
we'd be able to kill off UploadData and replace it with different subclasses of
an UploadDataStream.

[willchan no longer on Chromium, 2012-08-16 01:55:04]

To clarify, I'm OK with this change as an intermediate step, but I don't
like it as a long-term solution. I'd like to hear what the plans are going
forward.


On Wed, Aug 15, 2012 at 6:51 PM, <willchan@chromium.org> wrote:

> I'm a bit disappointed that we are taking something quite ugly
> (UploadData) and
> mostly duplicating it. What's the long term plan here? In another thread,
> Darin
> mentioned abstracting UploadDataStream. That sounds great to me. I'd hope
> that
> we'd be able to kill off UploadData and replace it with different
> subclasses of
> an UploadDataStream.
>
>
http://codereview.chromium.**org/10834289/<http://codereview.chromium.org/108...
>

[darin (slow to review), 2012-08-16 04:14:17]

Yes, that's the plan.  See the other email thread where Kinuko and I were
excitedly talking about that as well as Kinuko's revelation that we can
perhaps also eliminate the BlobData class.

-Darin

On Wed, Aug 15, 2012 at 6:55 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> To clarify, I'm OK with this change as an intermediate step, but I don't
> like it as a long-term solution. I'd like to hear what the plans are going
> forward.
>
>
> On Wed, Aug 15, 2012 at 6:51 PM, <willchan@chromium.org> wrote:
>
>> I'm a bit disappointed that we are taking something quite ugly
>> (UploadData) and
>> mostly duplicating it. What's the long term plan here? In another thread,
>> Darin
>> mentioned abstracting UploadDataStream. That sounds great to me. I'd hope
>> that
>> we'd be able to kill off UploadData and replace it with different
>> subclasses of
>> an UploadDataStream.
>>
>>
http://codereview.chromium.**org/10834289/<http://codereview.chromium.org/108...
>>
>
>

[kinuko, 2012-08-16 07:13:00]

Will, yes, I believe our goal is to kill UploadData but not to make it
proliferated.

http://codereview.chromium.org/10834289/diff/9013/content/public/common/commo...
File content/public/common/common_param_traits.cc (right):

http://codereview.chromium.org/10834289/diff/9013/content/public/common/commo...
content/public/common/common_param_traits.cc:131: struct
ParamTraits<net::UploadData::Element> {
On 2012/08/15 17:49:55, darin wrote:
> Who still relies on UploadData serialization?

Looks like AutomationURLRequest still sends this over IPC, which seems to be
used for ChromeFrame to intercept URLRequest. I was looking into if I could kill
the code but it's not obvious to me :(  This worries me since it may hinder our
long-term plan.

One uncomfortable option is maybe to have a specialized ctor which creates
ResourceRequestBody back from UploadData (or UploadDataStream in the future) for
serialization?

[kinuko, 2012-08-16 08:14:59]

http://codereview.chromium.org/10834289/diff/9013/chrome/browser/policy/devic...
File chrome/browser/policy/device_management_service_browsertest.cc (right):

http://codereview.chromium.org/10834289/diff/9013/chrome/browser/policy/devic...
chrome/browser/policy/device_management_service_browsertest.cc:66: void
ConstructResponse(const void* request_data,
On 2012/08/15 17:49:55, darin wrote:
> nit: |const char*| instead since that's what UploadData::Element::bytes()
> returns?

Done.

http://codereview.chromium.org/10834289/diff/9013/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host_impl.cc (right):

http://codereview.chromium.org/10834289/diff/9013/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host_impl.cc:86: #include
"webkit/glue/resource_request_body.h"
On 2012/08/15 17:49:55, darin wrote:
> nit: sort

Done.

http://codereview.chromium.org/10834289/diff/9013/content/common/resource_dis...
File content/common/resource_dispatcher.cc (right):

http://codereview.chromium.org/10834289/diff/9013/content/common/resource_dis...
content/common/resource_dispatcher.cc:25: #include
"webkit/glue/resource_request_body.h"
On 2012/08/15 17:49:55, darin wrote:
> nit: sort

Done.

http://codereview.chromium.org/10834289/diff/9013/content/common/resource_dis...
content/common/resource_dispatcher.cc:29: using
webkit_glue::ResourceRequestBody;
On 2012/08/15 17:49:55, darin wrote:
> nit: sort

Done.

http://codereview.chromium.org/10834289/diff/9013/content/public/common/commo...
File content/public/common/common_param_traits.cc (right):

http://codereview.chromium.org/10834289/diff/9013/content/public/common/commo...
content/public/common/common_param_traits.cc:131: struct
ParamTraits<net::UploadData::Element> {
On 2012/08/16 07:13:01, kinuko wrote:
> On 2012/08/15 17:49:55, darin wrote:
> > Who still relies on UploadData serialization?
> 
> Looks like AutomationURLRequest still sends this over IPC, which seems to be
> used for ChromeFrame to intercept URLRequest. I was looking into if I could
kill
> the code but it's not obvious to me :(  This worries me since it may hinder
our
> long-term plan.
> 
> One uncomfortable option is maybe to have a specialized ctor which creates
> ResourceRequestBody back from UploadData (or UploadDataStream in the future)
for
> serialization?

...or if we could intercept before we create URLRequest (like in RDH) for
ChromeFrame.  I'm not sure how stupid this idea could be though.

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

http://codereview.chromium.org/10834289/diff/9013/net/base/upload_data.h#newc...
net/base/upload_data.h:66: const char* bytes() const { return bytes_start_ ?
bytes_start_ : &buf_[0]; }
On 2012/08/15 17:49:55, darin wrote:
> maybe you should just ensure that bytes_start_ is always set?

To do so we need to allocate memory by ourselves and have a flag to free it
(since Element is copyable), and I thought having wrapper getter might be
easier.

http://codereview.chromium.org/10834289/diff/9013/net/base/upload_data.h#newc...
net/base/upload_data.h:81: void SetToSharedBytes(const char* bytes, int
bytes_len) {
On 2012/08/15 17:49:55, darin wrote:
> You should probably add a warning here about memory management.  The trick
about
> having to use UserData to ensure that the lifetime of this pointer remains
valid
> for the lifetime of the UploadData object is not so obvious.  A comment about
> that might help.

Done.

http://codereview.chromium.org/10834289/diff/9013/webkit/blob/blob_storage_co...
File webkit/blob/blob_storage_controller.cc (right):

http://codereview.chromium.org/10834289/diff/9013/webkit/blob/blob_storage_co...
webkit/blob/blob_storage_controller.cc:174: void
BlobStorageController::ResolveBlobReferencesInResourceRequestBody(
On 2012/08/15 17:49:55, darin wrote:
> nit: I'd be OK dropping the "Resource" in this function name just as you did
on
> ResourceLoaderBridge with the SetRequestBody method.

Done.

http://codereview.chromium.org/10834289/diff/9013/webkit/glue/resource_reques...
File webkit/glue/resource_request_body.cc (right):

http://codereview.chromium.org/10834289/diff/9013/webkit/glue/resource_reques...
webkit/glue/resource_request_body.cc:49: upload_data->SetUserData(
On 2012/08/15 17:49:55, darin wrote:
> Oh, I see!  You should add a comment about how this relates to the
> SetToSharedData usage.

Done.

http://codereview.chromium.org/10834289/diff/9013/webkit/glue/resource_reques...
File webkit/glue/resource_request_body.h (right):

http://codereview.chromium.org/10834289/diff/9013/webkit/glue/resource_reques...
webkit/glue/resource_request_body.h:22: namespace webkit_glue {
On 2012/08/15 17:49:55, darin wrote:
> nit: add a new line below here

Done.

[willchan no longer on Chromium, 2012-08-16 20:39:42]

net/ LGTM

Do we need a test for the new ResourceRequestBody?

I look forward to UploadData going away. It makes me sad.

I'm a bit disappointed to see ResourceRequestBody be refcounted. Is this because
we copy the IPC Message objects? Can we use scoped_ptr's move operator instead?
Don't worry about fixing that if it's a pain, since you're essentially just
replicating the existing badness of UploadData which is going on.

[kinuko, 2012-08-17 10:13:57]

Thanks! Added a test for ResourceRequestBody.

scoped_refptr makes it easier to be attached as UserData, but I don't think it's
brilliant either. Let me try fixing more when I (or Ryo) have next refactoring
patch.

[kinuko, 2012-08-20 01:53:36]

Adding jam@ and tony@ for content/ and webkit/.  John, Tony can you take a look?
 Thanks!

[tony, 2012-08-20 18:43:10]

webkit LGTM

http://codereview.chromium.org/10834289/diff/12052/webkit/blob/blob_storage_c...
File webkit/blob/blob_storage_controller_unittest.cc (right):

http://codereview.chromium.org/10834289/diff/12052/webkit/blob/blob_storage_c...
webkit/blob/blob_storage_controller_unittest.cc:9: #include
"net/base/upload_data.h"
Nit: Can you remove the upload_data.h include?

[jam, 2012-08-20 20:20:55]

lgtm

[kinuko, 2012-08-21 14:26:20]

Resubmitted after rebasing onto http://crrev.com/152481 and moved
ResolveBlobReference code from webkit_blob to webkit_glue to fix dependency
violation. (I'll have a follow-up patch for you to review/complain)