Move SetUserData of BlobDataHandles from UploadDataStreamBuilder to ResourceDispatcherHostImpl.

Move SetUserData of BlobDataHandles from UploadDataStreamBuilder to ResourceDispatcherHostImpl.

BUG=402387
Committed: https://crrev.com/82fd6e6f80b2e35a1ace01dd25d4a8dcc40b4d75
Cr-Commit-Position: refs/heads/master@{#292174}

[horo, 2014-08-20 12:19:35]

michaeln@
Could you please review this?
This is related to your comments in
https://codereview.chromium.org/475333002/#msg9.

Thank you.

[michaeln, 2014-08-20 18:49:23]

lgtm, thnx

[horo, 2014-08-20 23:42:46]

mmenke@
Could you please review this cl?

[mmenke, 2014-08-21 15:54:07]

On 2014/08/20 23:42:46, horo wrote:
> mmenke@
> Could you please review this cl?

Will do.  May not be ready to sign off on this until tomorrow (Don't really know
anything about blobs, so want to take the time to understand how they're hooked
up, first).

[mmenke, 2014-08-21 15:55:40]

On 2014/08/21 15:54:07, mmenke wrote:
> On 2014/08/20 23:42:46, horo wrote:
> > mmenke@
> > Could you please review this cl?
> 
> Will do.  May not be ready to sign off on this until tomorrow (Don't really
know
> anything about blobs, so want to take the time to understand how they're
hooked
> up, first).

If you don't hear back from me by Monday afternoon, feel free to ping (Since
this is just a refactoring, and doesn't seem to be blocking anything, not giving
it a high priority, though don't want it to take too long, of course.  If I'm
mistaken in that assumption, please correct me).

[horo, 2014-08-22 00:05:50]

On 2014/08/21 15:55:40, mmenke wrote:
> On 2014/08/21 15:54:07, mmenke wrote:
> > On 2014/08/20 23:42:46, horo wrote:
> > > mmenke@
> > > Could you please review this cl?
> > 
> > Will do.  May not be ready to sign off on this until tomorrow (Don't really
> know
> > anything about blobs, so want to take the time to understand how they're
> hooked
> > up, first).
> 
> If you don't hear back from me by Monday afternoon, feel free to ping (Since
> this is just a refactoring, and doesn't seem to be blocking anything, not
giving
> it a high priority, though don't want it to take too long, of course.  If I'm
> mistaken in that assumption, please correct me).

Yes, it is just a refactoring.
And it is not high priority.

[mmenke, 2014-08-25 18:49:03]

https://codereview.chromium.org/492603002/diff/80001/content/browser/loader/u...
File content/browser/loader/upload_data_stream_builder.cc (right):

https://codereview.chromium.org/492603002/diff/80001/content/browser/loader/u...
content/browser/loader/upload_data_stream_builder.cc:72:
blob_context->GetBlobDataFromUUID(element.blob_uuid());
I know this works, due to refcounting, but but should we really be creating a
new handle here?  Seems like we should be using a raw pointer instead of
creating short-lived handles.  My suggestion is just to make
UploadDataStreamBuilder::Build take in an optional BlobData*, and mention the
lifetime dependency in its documentation.

[horo, 2014-08-26 00:31:53]

https://chromiumcodereview.appspot.com/492603002/diff/80001/content/browser/l...
File content/browser/loader/upload_data_stream_builder.cc (right):

https://chromiumcodereview.appspot.com/492603002/diff/80001/content/browser/l...
content/browser/loader/upload_data_stream_builder.cc:72:
blob_context->GetBlobDataFromUUID(element.blob_uuid());
On 2014/08/25 18:49:02, mmenke wrote:
> I know this works, due to refcounting, but but should we really be creating a
> new handle here?  Seems like we should be using a raw pointer instead of
> creating short-lived handles.  My suggestion is just to make
> UploadDataStreamBuilder::Build take in an optional BlobData*, and mention the
> lifetime dependency in its documentation.

If my understanding is correct, we resolve the blob into the other type (BYTES,
FILE, FILE_SYSTEM) items here to avoid code complexity.
If we don't resolve here we have to implement UploadElementReader for Blob.

So we need to get this BlobDataHandle from the uuid in order to resolve the
blob.

[mmenke, 2014-08-26 18:09:15]

https://codereview.chromium.org/492603002/diff/80001/content/browser/loader/u...
File content/browser/loader/upload_data_stream_builder.cc (right):

https://codereview.chromium.org/492603002/diff/80001/content/browser/loader/u...
content/browser/loader/upload_data_stream_builder.cc:72:
blob_context->GetBlobDataFromUUID(element.blob_uuid());
On 2014/08/26 00:31:53, horo wrote:
> On 2014/08/25 18:49:02, mmenke wrote:
> > I know this works, due to refcounting, but but should we really be creating
a
> > new handle here?  Seems like we should be using a raw pointer instead of
> > creating short-lived handles.  My suggestion is just to make
> > UploadDataStreamBuilder::Build take in an optional BlobData*, and mention
the
> > lifetime dependency in its documentation.
> 
> If my understanding is correct, we resolve the blob into the other type
(BYTES,
> FILE, FILE_SYSTEM) items here to avoid code complexity.
> If we don't resolve here we have to implement UploadElementReader for Blob.
> 
> So we need to get this BlobDataHandle from the uuid in order to resolve the
> blob.

Think it's at least worth a comment the caller is responsible for making sure
the BlobData outlives the returned value of UploadDataStream, maybe in the docs
for UploadDataStreamBuilder::Build?

[michaeln, 2014-08-26 20:34:08]

https://codereview.chromium.org/492603002/diff/80001/content/browser/loader/u...
File content/browser/loader/upload_data_stream_builder.cc (right):

https://codereview.chromium.org/492603002/diff/80001/content/browser/loader/u...
content/browser/loader/upload_data_stream_builder.cc:72:
blob_context->GetBlobDataFromUUID(element.blob_uuid());
> Think it's at least worth a comment the caller is responsible for making sure
> the BlobData outlives the returned value of UploadDataStream, maybe in the
docs
> for UploadDataStreamBuilder::Build?

Thats a good point.

I asked horo to hoist the SetUserData() calls into RDH because there's a 2nd
chunk of code, also invoked by RDH, that's relying on that behavior as well.
Another option could be to have both UploadDataStreamBuilder and the other
consumer (serviceworker thing) independently use SetUserData() to take care of
themselves (so to speak).

[horo, 2014-08-27 01:38:16]

https://codereview.chromium.org/492603002/diff/80001/content/browser/loader/u...
File content/browser/loader/upload_data_stream_builder.cc (right):

https://codereview.chromium.org/492603002/diff/80001/content/browser/loader/u...
content/browser/loader/upload_data_stream_builder.cc:72:
blob_context->GetBlobDataFromUUID(element.blob_uuid());
On 2014/08/26 18:09:14, mmenke wrote:
> On 2014/08/26 00:31:53, horo wrote:
> > On 2014/08/25 18:49:02, mmenke wrote:
> > > I know this works, due to refcounting, but but should we really be
creating
> a
> > > new handle here?  Seems like we should be using a raw pointer instead of
> > > creating short-lived handles.  My suggestion is just to make
> > > UploadDataStreamBuilder::Build take in an optional BlobData*, and mention
> the
> > > lifetime dependency in its documentation.
> > 
> > If my understanding is correct, we resolve the blob into the other type
> (BYTES,
> > FILE, FILE_SYSTEM) items here to avoid code complexity.
> > If we don't resolve here we have to implement UploadElementReader for Blob.
> > 
> > So we need to get this BlobDataHandle from the uuid in order to resolve the
> > blob.
> 
> Think it's at least worth a comment the caller is responsible for making sure
> the BlobData outlives the returned value of UploadDataStream, maybe in the
docs
> for UploadDataStreamBuilder::Build?

Added comments in upload_data_stream_builder.h.

[mmenke, 2014-08-27 14:31:09]

On 2014/08/27 01:38:16, horo wrote:
>
https://codereview.chromium.org/492603002/diff/80001/content/browser/loader/u...
> File content/browser/loader/upload_data_stream_builder.cc (right):
> 
>
https://codereview.chromium.org/492603002/diff/80001/content/browser/loader/u...
> content/browser/loader/upload_data_stream_builder.cc:72:
> blob_context->GetBlobDataFromUUID(element.blob_uuid());
> On 2014/08/26 18:09:14, mmenke wrote:
> > On 2014/08/26 00:31:53, horo wrote:
> > > On 2014/08/25 18:49:02, mmenke wrote:
> > > > I know this works, due to refcounting, but but should we really be
> creating
> > a
> > > > new handle here?  Seems like we should be using a raw pointer instead of
> > > > creating short-lived handles.  My suggestion is just to make
> > > > UploadDataStreamBuilder::Build take in an optional BlobData*, and
mention
> > the
> > > > lifetime dependency in its documentation.
> > > 
> > > If my understanding is correct, we resolve the blob into the other type
> > (BYTES,
> > > FILE, FILE_SYSTEM) items here to avoid code complexity.
> > > If we don't resolve here we have to implement UploadElementReader for
Blob.
> > > 
> > > So we need to get this BlobDataHandle from the uuid in order to resolve
the
> > > blob.
> > 
> > Think it's at least worth a comment the caller is responsible for making
sure
> > the BlobData outlives the returned value of UploadDataStream, maybe in the
> docs
> > for UploadDataStreamBuilder::Build?
> 
> Added comments in upload_data_stream_builder.h.

LGTM

[horo, 2014-08-27 15:24:33]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-08-27 15:25:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/horo@chromium.org/492603002/120001

[commit-bot: I haz the power, 2014-08-27 16:24:33]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-27 17:08:21]

Committed patchset #3 (id:120001) as 19e49f6c20b165120ba75eb3fb0a0d6fc7a9a7e4

[commit-bot: I haz the power, 2014-09-10 02:52:25]

Patchset 3 (id:??) landed as
https://crrev.com/82fd6e6f80b2e35a1ace01dd25d4a8dcc40b4d75
Cr-Commit-Position: refs/heads/master@{#292174}