Remove SupportsUserData from ResourceRequestBodyImpl.

Remove SupportsUserData from ResourceRequestBodyImpl.

ResourceRequestBodyImpl is sent between processes via IPC and Mojo, and
since we can't send the UserData along with it, it doesn't make sense to
inherit from SupportsUserData. It was only being used to stash upload blob
handles, so this CL juggles around ownership of those.

BUG=737191
Review-Url: https://codereview.chromium.org/2960023002
Cr-Commit-Position: refs/heads/master@{#483140}
Committed: https://chromium.googlesource.com/chromium/src/+/ed44e6c201f6e31ab4dc71b7914628545a551eda

[mmenke, 2017-06-27 20:33:58]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-27 20:34:17]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-27 20:44:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-27 20:44:46]

Dry run: Try jobs failed on following builders:
  linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mmenke, 2017-06-27 20:59:24]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-27 20:59:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mmenke, 2017-06-27 21:14:40]

The CQ bit was unchecked by mmenke@chromium.org

[mmenke, 2017-06-27 21:24:09]

mmenke@chromium.org changed reviewers:
+ dmurph@chromium.org

[mmenke, 2017-06-27 21:24:10]

[dmurph]:  Is this reasonable?  I'm open to other ideas.

We're working on a new network service (in content/network), which uses the same
mojo API as RDH.  Eventually we'll have to figure out how it should handle
blobs, but for now, I want to move the NetworkService Mojo API and its
dependencies over to content/public/common, and blob-related fields will be
ignored by the network service.

This is just a small cleanup to reduce the warts on the API we're exposing.

[dmurph, 2017-06-27 21:29:07]

This is great - I'd love to ditch the unique_ptr for just std::move-ing the
vector.

https://codereview.chromium.org/2960023002/diff/20001/content/browser/blob_st...
File content/browser/blob_storage/chrome_blob_storage_context.h (right):

https://codereview.chromium.org/2960023002/diff/20001/content/browser/blob_st...
content/browser/blob_storage/chrome_blob_storage_context.h:99:
std::unique_ptr<BlobHandles> GetBodyBlobDataHandles(
Can you have this just return the vector? You can just std::move it out.

https://codereview.chromium.org/2960023002/diff/20001/content/browser/loader/...
File content/browser/loader/resource_request_info_impl.h (right):

https://codereview.chromium.org/2960023002/diff/20001/content/browser/loader/...
content/browser/loader/resource_request_info_impl.h:246:
std::unique_ptr<BlobHandles> blob_handles_;
Can you make all of these just BlobHandles instead of unique_ptr? and then
std::move the vector?

[mmenke, 2017-06-27 21:32:23]

https://codereview.chromium.org/2960023002/diff/20001/content/browser/blob_st...
File content/browser/blob_storage/chrome_blob_storage_context.h (right):

https://codereview.chromium.org/2960023002/diff/20001/content/browser/blob_st...
content/browser/blob_storage/chrome_blob_storage_context.h:99:
std::unique_ptr<BlobHandles> GetBodyBlobDataHandles(
On 2017/06/27 21:29:07, dmurph wrote:
> Can you have this just return the vector? You can just std::move it out.

We need a way to fail.  The only reason I used a unique_ptr was so that we could
return nullptr.  I could return a bool and take a BlobHandles* as an argument. 
Open to other ideas.

[dmurph, 2017-06-27 21:35:53]

https://codereview.chromium.org/2960023002/diff/20001/content/browser/blob_st...
File content/browser/blob_storage/chrome_blob_storage_context.h (right):

https://codereview.chromium.org/2960023002/diff/20001/content/browser/blob_st...
content/browser/blob_storage/chrome_blob_storage_context.h:99:
std::unique_ptr<BlobHandles> GetBodyBlobDataHandles(
On 2017/06/27 21:32:22, mmenke wrote:
> On 2017/06/27 21:29:07, dmurph wrote:
> > Can you have this just return the vector? You can just std::move it out.
> 
> We need a way to fail.  The only reason I used a unique_ptr was so that we
could
> return nullptr.  I could return a bool and take a BlobHandles* as an argument.

> Open to other ideas.

What about providing the 'out' vector in an argument, and returning a bool?

[mmenke, 2017-06-27 21:48:07]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-27 21:48:42]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mmenke, 2017-06-27 21:48:44]

Thanks for the feedback!

https://codereview.chromium.org/2960023002/diff/20001/content/browser/blob_st...
File content/browser/blob_storage/chrome_blob_storage_context.h (right):

https://codereview.chromium.org/2960023002/diff/20001/content/browser/blob_st...
content/browser/blob_storage/chrome_blob_storage_context.h:99:
std::unique_ptr<BlobHandles> GetBodyBlobDataHandles(
On 2017/06/27 21:35:53, dmurph wrote:
> On 2017/06/27 21:32:22, mmenke wrote:
> > On 2017/06/27 21:29:07, dmurph wrote:
> > > Can you have this just return the vector? You can just std::move it out.
> > 
> > We need a way to fail.  The only reason I used a unique_ptr was so that we
> could
> > return nullptr.  I could return a bool and take a BlobHandles* as an
argument.
> 
> > Open to other ideas.
> 
> What about providing the 'out' vector in an argument, and returning a bool?

Works for me, done.

[commit-bot: I haz the power, 2017-06-27 22:07:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-27 22:07:41]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dmurph, 2017-06-27 22:18:09]

thanks for the patch! Cleans things up a bit.

lgtm

[mmenke, 2017-06-27 22:18:51]

On 2017/06/27 22:18:09, dmurph wrote:
> thanks for the patch! Cleans things up a bit.
> 
> lgtm

No problem.  Thanks for the speedy review!

[mmenke, 2017-06-27 22:18:57]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2017-06-27 22:19:25]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-27 22:28:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-27 22:28:04]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[mmenke, 2017-06-27 22:31:51]

mmenke@chromium.org changed reviewers:
+ jam@chromium.org

[mmenke, 2017-06-27 22:31:52]

[+jam]:  Please review content/common/resource_request_body_impl.h

[kinuko, 2017-06-28 01:43:27]

kinuko@chromium.org changed reviewers:
+ kinuko@chromium.org

[kinuko, 2017-06-28 01:43:28]

Drive-by, this looks great! lgtm

https://codereview.chromium.org/2960023002/diff/40001/content/browser/loader/...
File content/browser/loader/resource_request_info_impl.h (right):

https://codereview.chromium.org/2960023002/diff/40001/content/browser/loader/...
content/browser/loader/resource_request_info_impl.h:245: // Keeps blobs alive
for the duration of the request.
Could we also briefly note that it's for uploading?

[mmenke, 2017-06-28 17:31:37]

https://codereview.chromium.org/2960023002/diff/40001/content/browser/loader/...
File content/browser/loader/resource_request_info_impl.h (right):

https://codereview.chromium.org/2960023002/diff/40001/content/browser/loader/...
content/browser/loader/resource_request_info_impl.h:245: // Keeps blobs alive
for the duration of the request.
On 2017/06/28 01:43:28, kinuko wrote:
> Could we also briefly note that it's for uploading?

Done.

[mmenke, 2017-06-28 18:02:38]

Description was changed from

==========
Remove SupportsUserData from ResourceRequestBodyImpl.

ResourceRequestBodyImpl is sent between processes via IPC and Mojo, and
since we can't set the UserData along with it, it doesn't make sense to
inherit from it. It was only being used to stash blob handles, so this
CL juggles around ownership of those.

BUG=737191
==========

to

==========
Remove SupportsUserData from ResourceRequestBodyImpl.

ResourceRequestBodyImpl is sent between processes via IPC and Mojo, and
since we can't send the UserData along with it, it doesn't make sense to
inherit from SupportsUserData. It was only being used to stash blob
handles, so this CL juggles around ownership of those.

BUG=737191
==========

[mmenke, 2017-06-28 18:02:51]

Description was changed from

==========
Remove SupportsUserData from ResourceRequestBodyImpl.

ResourceRequestBodyImpl is sent between processes via IPC and Mojo, and
since we can't send the UserData along with it, it doesn't make sense to
inherit from SupportsUserData. It was only being used to stash blob
handles, so this CL juggles around ownership of those.

BUG=737191
==========

to

==========
Remove SupportsUserData from ResourceRequestBodyImpl.

ResourceRequestBodyImpl is sent between processes via IPC and Mojo, and
since we can't send the UserData along with it, it doesn't make sense to
inherit from SupportsUserData. It was only being used to stash upload blob
handles, so this CL juggles around ownership of those.

BUG=737191
==========

[jam, 2017-06-28 19:44:03]

lgtm

[mmenke, 2017-06-28 19:50:07]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2017-06-28 19:50:08]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
dmurph@chromium.org
Link to the patchset: https://codereview.chromium.org/2960023002/#ps60001
(title: "Merge, response to comment")

[commit-bot: I haz the power, 2017-06-28 19:50:44]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-28 21:13:34]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1498679407990970,
"parent_rev": "d2a722f6bd23eaf0f791b19378c639dce6b3c2c0", "commit_rev":
"ed44e6c201f6e31ab4dc71b7914628545a551eda"}

[commit-bot: I haz the power, 2017-06-28 21:13:50]

Description was changed from

==========
Remove SupportsUserData from ResourceRequestBodyImpl.

ResourceRequestBodyImpl is sent between processes via IPC and Mojo, and
since we can't send the UserData along with it, it doesn't make sense to
inherit from SupportsUserData. It was only being used to stash upload blob
handles, so this CL juggles around ownership of those.

BUG=737191
==========

to

==========
Remove SupportsUserData from ResourceRequestBodyImpl.

ResourceRequestBodyImpl is sent between processes via IPC and Mojo, and
since we can't send the UserData along with it, it doesn't make sense to
inherit from SupportsUserData. It was only being used to stash upload blob
handles, so this CL juggles around ownership of those.

BUG=737191

Review-Url: https://codereview.chromium.org/2960023002
Cr-Commit-Position: refs/heads/master@{#483140}
Committed:
https://chromium.googlesource.com/chromium/src/+/ed44e6c201f6e31ab4dc71b79146...
==========

[commit-bot: I haz the power, 2017-06-28 21:13:52]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/ed44e6c201f6e31ab4dc71b79146...