net: Change argument of URLRequest::set_upload from UploadData to UploadDataStream

net: Change argument of URLRequest::set_upload from UploadData to UploadDataStream

Users of URLRequest are now responsible to create UploadDataStream.
UploadOwnedBytesElementReader is moved from upload_data_stream.cc to upload_bytes_element_reader.h.

BUG=156574
TEST=net_unittests and git try
TBR=abodenha@chromium.org for cloud_print/service/service_state.cc

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

[hashimoto, 2012-12-11 15:07:38]

kinuko@: Please review src/webkit.
mmenke@: Please review other parts.

[mmenke, 2012-12-11 18:53:19]

https://codereview.chromium.org/11439008/diff/24001/cloud_print/service/servi...
File cloud_print/service/service_state.cc (right):

https://codereview.chromium.org/11439008/diff/24001/cloud_print/service/servi...
cloud_print/service/service_state.cc:186: request.set_upload(new
net::UploadDataStream(&element_readers, 0));
Egads...this function is ugly, though suppose that's more than a little beyond
the scope of this CL.

This should all be done before we create the request - think it's best to avoid
destroying an UploadDataStream after its underlying data has been destroyed,
even if it technically won't cause any problems.

https://codereview.chromium.org/11439008/diff/24001/net/base/upload_bytes_ele...
File net/base/upload_bytes_element_reader.h (right):

https://codereview.chromium.org/11439008/diff/24001/net/base/upload_bytes_ele...
net/base/upload_bytes_element_reader.h:48:
UploadOwnedBytesElementReader(std::vector<char>* data);
Think it's worth mentioning that we clear |data|.  It's implied by the fact we
take it as a pointer, but prefer to be explicit.

https://codereview.chromium.org/11439008/diff/24001/net/base/upload_bytes_ele...
net/base/upload_bytes_element_reader.h:48:
UploadOwnedBytesElementReader(std::vector<char>* data);
Speaking of which...this function should also be explicit.

https://codereview.chromium.org/11439008/diff/24001/net/base/upload_bytes_ele...
net/base/upload_bytes_element_reader.h:52: std::vector<char> data_;
May make life easier to make UploadOwnedBytesElementReader take a string rather
than a vector, as we seem to be doing a lot of converting between the two.

Or could just give it a constructor that takes a string (And optionally switch
to storing a string as well, but keeping the vector constructor).

https://codereview.chromium.org/11439008/diff/24001/net/url_request/url_fetch...
File net/url_request/url_fetcher_core.cc (right):

https://codereview.chromium.org/11439008/diff/24001/net/url_request/url_fetch...
net/url_request/url_fetcher_core.cc:710: request_->set_upload(new
UploadDataStream(&element_readers, 0));
Life may be a bit simple if we have the UploadDataStream and
UploadElementReaders implement the same interface, and just have the URLRequest
use that.  A significant enough change that it's probably not reasonable to do
in this CL.

All classes pretty much implement the same interface, except for the chunked
stuff.  Could even have a separate class for the chunked encoding class and the
normal multiple element reader case, though not sure that would get us much.

Suppose this idea would cause issues with the classes that use AsBytesReader() /
AsFileReader().

https://codereview.chromium.org/11439008/diff/24001/net/url_request/url_reque...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/11439008/diff/24001/net/url_request/url_reque...
net/url_request/url_request_unittest.cc:1962: r.set_upload(new
UploadDataStream(&element_readers, 0));
Looks to me like you can just use CreateSimpleUploadData(uploadBytes) here.

https://codereview.chromium.org/11439008/diff/24001/webkit/glue/resource_requ...
File webkit/glue/resource_request_body.h (right):

https://codereview.chromium.org/11439008/diff/24001/webkit/glue/resource_requ...
webkit/glue/resource_request_body.h:47: net::UploadDataStream*
ResolveElementsAndCreateUploadData(
Maybe AndCreateUploadStream?  AndCreateUploadStreamData?

While it is indeed upload data, since we also have an UploadData class, is
perhaps a bit confusing.

[kinuko, 2012-12-12 01:19:16]

https://codereview.chromium.org/11439008/diff/24001/net/base/upload_bytes_ele...
File net/base/upload_bytes_element_reader.h (right):

https://codereview.chromium.org/11439008/diff/24001/net/base/upload_bytes_ele...
net/base/upload_bytes_element_reader.h:16: class NET_EXPORT
UploadBytesElementReader : public UploadElementReader {
Can we add a comment to note that this doesn't copy the given bytes or take the
ownership of the data? (The existence of ...Owned... version implies it, but
it's not obvious just by looking at this class's interface)

https://codereview.chromium.org/11439008/diff/24001/net/url_request/url_reque...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/11439008/diff/24001/net/url_request/url_reque...
net/url_request/url_request.h:415: void set_upload(UploadDataStream* upload);
Could this take scoped_ptr<UploadDataStream> to make the ownership relationship
more explicit?

https://codereview.chromium.org/11439008/diff/24001/webkit/glue/resource_requ...
File webkit/glue/resource_request_body_unittest.cc (right):

https://codereview.chromium.org/11439008/diff/24001/webkit/glue/resource_requ...
webkit/glue/resource_request_body_unittest.cc:24: bool CompareElement(const
net::UploadElementReader& reader,
naming-nit. This function name makes me feel that this should return -1, 0 or 1
as a comparator.  AreElementsEqual() or something maybe?

https://codereview.chromium.org/11439008/diff/24001/webkit/glue/resource_requ...
webkit/glue/resource_request_body_unittest.cc:34: element.bytes() +
element.length());
Can we use std::equal?

[hashimoto, 2012-12-12 10:29:07]

https://codereview.chromium.org/11439008/diff/24001/cloud_print/service/servi...
File cloud_print/service/service_state.cc (right):

https://codereview.chromium.org/11439008/diff/24001/cloud_print/service/servi...
cloud_print/service/service_state.cc:186: request.set_upload(new
net::UploadDataStream(&element_readers, 0));
On 2012/12/11 18:53:21, Matt Menke wrote:
> Egads...this function is ugly, though suppose that's more than a little beyond
> the scope of this CL.
> 
> This should all be done before we create the request - think it's best to
avoid
> destroying an UploadDataStream after its underlying data has been destroyed,
> even if it technically won't cause any problems.

Should we switch to UploadOwnedBytesElementReader as a safer choice?
I think it doesn't cost much since the size of post_body is some KBs.

https://codereview.chromium.org/11439008/diff/24001/net/base/upload_bytes_ele...
File net/base/upload_bytes_element_reader.h (right):

https://codereview.chromium.org/11439008/diff/24001/net/base/upload_bytes_ele...
net/base/upload_bytes_element_reader.h:16: class NET_EXPORT
UploadBytesElementReader : public UploadElementReader {
On 2012/12/12 01:19:17, kinuko wrote:
> Can we add a comment to note that this doesn't copy the given bytes or take
the
> ownership of the data? (The existence of ...Owned... version implies it, but
> it's not obvious just by looking at this class's interface)

Done.

https://codereview.chromium.org/11439008/diff/24001/net/base/upload_bytes_ele...
net/base/upload_bytes_element_reader.h:48:
UploadOwnedBytesElementReader(std::vector<char>* data);
On 2012/12/11 18:53:21, Matt Menke wrote:
> Speaking of which...this function should also be explicit.

Oops, it seems presubmit check does not help for missing 'explicit' in src/net,
while it does in src/chrome.

https://codereview.chromium.org/11439008/diff/24001/net/base/upload_bytes_ele...
net/base/upload_bytes_element_reader.h:48:
UploadOwnedBytesElementReader(std::vector<char>* data);
On 2012/12/11 18:53:21, Matt Menke wrote:
> Think it's worth mentioning that we clear |data|.  It's implied by the fact we
> take it as a pointer, but prefer to be explicit.

Done.

https://codereview.chromium.org/11439008/diff/24001/net/base/upload_bytes_ele...
net/base/upload_bytes_element_reader.h:52: std::vector<char> data_;
On 2012/12/11 18:53:21, Matt Menke wrote:
> May make life easier to make UploadOwnedBytesElementReader take a string
rather
> than a vector, as we seem to be doing a lot of converting between the two.
Since yet there are only two places which uses UploadOwnedBytesReader with
string (chrome_fraudlent_certificate_reporter.cc and download_manager_impl.cc),
can we go with vector<char> now?
I think vector<char> is little bit purer than std::string as a way to own bytes.
> 
> Or could just give it a constructor that takes a string (And optionally switch
> to storing a string as well, but keeping the vector constructor).

Having a string ctor while keeping the vector does not work because:

UploadOwnedBytesElementReader::UploadOwnedBytesElementReader(
    const std::string& string)
  : UploadBytesElementReader(string.data(), string.length()),
    data_(string.begin(), string.end()) {
 // &data_[0] is different from this->bytes() now!
}

How about having UploadOwnedBytesElementReader::CreateFromString?

https://codereview.chromium.org/11439008/diff/24001/net/url_request/url_fetch...
File net/url_request/url_fetcher_core.cc (right):

https://codereview.chromium.org/11439008/diff/24001/net/url_request/url_fetch...
net/url_request/url_fetcher_core.cc:710: request_->set_upload(new
UploadDataStream(&element_readers, 0));
On 2012/12/11 18:53:21, Matt Menke wrote:
> Life may be a bit simple if we have the UploadDataStream and
> UploadElementReaders implement the same interface, and just have the
URLRequest
> use that.  A significant enough change that it's probably not reasonable to do
> in this CL.
Since the main user (webkit_glue::ResourceRequestBody) uses
ResourceRequestBody-Element model which is similar to
UploadDataStream-UploadElmentReader relationship, I think having a helper
function to construct a UploadDataStream from one UploadElementReader (or,
std::string or std::vector<char>) does suffice.
> 
> All classes pretty much implement the same interface, except for the chunked
> stuff.  Could even have a separate class for the chunked encoding class and
the
> normal multiple element reader case, though not sure that would get us much.
Separating chunked data code to a separate class sounds interesting. Though I'm
not sure if it's really worth doing neither.
> 
> Suppose this idea would cause issues with the classes that use AsBytesReader()
/
> AsFileReader().

https://codereview.chromium.org/11439008/diff/24001/net/url_request/url_reque...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/11439008/diff/24001/net/url_request/url_reque...
net/url_request/url_request.h:415: void set_upload(UploadDataStream* upload);
On 2012/12/12 01:19:17, kinuko wrote:
> Could this take scoped_ptr<UploadDataStream> to make the ownership
relationship
> more explicit?

Done.

https://codereview.chromium.org/11439008/diff/24001/net/url_request/url_reque...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/11439008/diff/24001/net/url_request/url_reque...
net/url_request/url_request_unittest.cc:1962: r.set_upload(new
UploadDataStream(&element_readers, 0));
On 2012/12/11 18:53:21, Matt Menke wrote:
> Looks to me like you can just use CreateSimpleUploadData(uploadBytes) here.

Done.

https://codereview.chromium.org/11439008/diff/24001/webkit/glue/resource_requ...
File webkit/glue/resource_request_body.h (right):

https://codereview.chromium.org/11439008/diff/24001/webkit/glue/resource_requ...
webkit/glue/resource_request_body.h:47: net::UploadDataStream*
ResolveElementsAndCreateUploadData(
On 2012/12/11 18:53:21, Matt Menke wrote:
> Maybe AndCreateUploadStream?  AndCreateUploadStreamData?
> 
> While it is indeed upload data, since we also have an UploadData class, is
> perhaps a bit confusing.

Renamed to ResolveElementsAndCreateUploadDataStream.

https://codereview.chromium.org/11439008/diff/24001/webkit/glue/resource_requ...
File webkit/glue/resource_request_body_unittest.cc (right):

https://codereview.chromium.org/11439008/diff/24001/webkit/glue/resource_requ...
webkit/glue/resource_request_body_unittest.cc:24: bool CompareElement(const
net::UploadElementReader& reader,
On 2012/12/12 01:19:17, kinuko wrote:
> naming-nit. This function name makes me feel that this should return -1, 0 or
1
> as a comparator.  AreElementsEqual() or something maybe?

Done.

https://codereview.chromium.org/11439008/diff/24001/webkit/glue/resource_requ...
webkit/glue/resource_request_body_unittest.cc:34: element.bytes() +
element.length());
On 2012/12/12 01:19:17, kinuko wrote:
> Can we use std::equal?

Done.

[mmenke, 2012-12-12 19:26:41]

Just a couple minor comments.

https://codereview.chromium.org/11439008/diff/24001/cloud_print/service/servi...
File cloud_print/service/service_state.cc (right):

https://codereview.chromium.org/11439008/diff/24001/cloud_print/service/servi...
cloud_print/service/service_state.cc:186: request.set_upload(new
net::UploadDataStream(&element_readers, 0));
On 2012/12/12 10:29:07, hashimoto wrote:
> On 2012/12/11 18:53:21, Matt Menke wrote:
> > Egads...this function is ugly, though suppose that's more than a little
beyond
> > the scope of this CL.
> > 
> > This should all be done before we create the request - think it's best to
> avoid
> > destroying an UploadDataStream after its underlying data has been destroyed,
> > even if it technically won't cause any problems.
> 
> Should we switch to UploadOwnedBytesElementReader as a safer choice?
> I think it doesn't cost much since the size of post_body is some KBs.

I think that's a good idea.

https://codereview.chromium.org/11439008/diff/24001/net/base/upload_bytes_ele...
File net/base/upload_bytes_element_reader.h (right):

https://codereview.chromium.org/11439008/diff/24001/net/base/upload_bytes_ele...
net/base/upload_bytes_element_reader.h:52: std::vector<char> data_;
On 2012/12/12 10:29:07, hashimoto wrote:
> On 2012/12/11 18:53:21, Matt Menke wrote:
> > May make life easier to make UploadOwnedBytesElementReader take a string
> rather
> > than a vector, as we seem to be doing a lot of converting between the two.
> Since yet there are only two places which uses UploadOwnedBytesReader with
> string (chrome_fraudlent_certificate_reporter.cc and
download_manager_impl.cc),
> can we go with vector<char> now?
> I think vector<char> is little bit purer than std::string as a way to own
bytes.

I admit to a bit of a bias against vectors used in this way because I prefer
data.data() to "&(*data)[0]".  And passing strings around is a bit prettier,
too.

> > 
> > Or could just give it a constructor that takes a string (And optionally
switch
> > to storing a string as well, but keeping the vector constructor).
> 
> Having a string ctor while keeping the vector does not work because:
> 
> UploadOwnedBytesElementReader::UploadOwnedBytesElementReader(
>     const std::string& string)
>   : UploadBytesElementReader(string.data(), string.length()),
>     data_(string.begin(), string.end()) {
>  // &data_[0] is different from this->bytes() now!
> }
> 
> How about having UploadOwnedBytesElementReader::CreateFromString?

I think that's fine.

https://codereview.chromium.org/11439008/diff/24001/net/url_request/url_fetch...
File net/url_request/url_fetcher_core.cc (right):

https://codereview.chromium.org/11439008/diff/24001/net/url_request/url_fetch...
net/url_request/url_fetcher_core.cc:710: request_->set_upload(new
UploadDataStream(&element_readers, 0));
On 2012/12/12 10:29:07, hashimoto wrote:
> On 2012/12/11 18:53:21, Matt Menke wrote:
> > Life may be a bit simple if we have the UploadDataStream and
> > UploadElementReaders implement the same interface, and just have the
> URLRequest
> > use that.  A significant enough change that it's probably not reasonable to
do
> > in this CL.
> Since the main user (webkit_glue::ResourceRequestBody) uses
> ResourceRequestBody-Element model which is similar to
> UploadDataStream-UploadElmentReader relationship, I think having a helper
> function to construct a UploadDataStream from one UploadElementReader (or,
> std::string or std::vector<char>) does suffice.

Think your two new static methods do a lot to remove the boilerplate code that
was bothering me.  Still think it would be nice to we could make them all
implement the same interface, but given how many times we have to dig into their
guts, that may not be practical.

> > 
> > All classes pretty much implement the same interface, except for the chunked
> > stuff.  Could even have a separate class for the chunked encoding class and
> the
> > normal multiple element reader case, though not sure that would get us much.
> Separating chunked data code to a separate class sounds interesting. Though
I'm
> not sure if it's really worth doing neither.
> > 
> > Suppose this idea would cause issues with the classes that use
AsBytesReader()
> /
> > AsFileReader().
>

https://codereview.chromium.org/11439008/diff/43008/webkit/glue/resource_requ...
File webkit/glue/resource_request_body.cc (right):

https://codereview.chromium.org/11439008/diff/43008/webkit/glue/resource_requ...
webkit/glue/resource_request_body.cc:39: class FileElementReader : public
net::UploadFileElementReader {
This class isn't needed, is it?  UploadFileElementReader copies everything it
needs.

[kinuko, 2012-12-13 00:07:55]

https://codereview.chromium.org/11439008/diff/24001/net/base/upload_bytes_ele...
File net/base/upload_bytes_element_reader.h (right):

https://codereview.chromium.org/11439008/diff/24001/net/base/upload_bytes_ele...
net/base/upload_bytes_element_reader.h:52: std::vector<char> data_;
As for the use of &(*data)[0] I think vector_as_array (defined in stl_util.h) is
more preferable? (By the way could we get an empty data here?)

[mmenke, 2012-12-13 00:43:47]

https://codereview.chromium.org/11439008/diff/24001/net/base/upload_bytes_ele...
File net/base/upload_bytes_element_reader.h (right):

https://codereview.chromium.org/11439008/diff/24001/net/base/upload_bytes_ele...
net/base/upload_bytes_element_reader.h:52: std::vector<char> data_;
On 2012/12/13 00:07:55, kinuko wrote:
> As for the use of &(*data)[0] I think vector_as_array (defined in stl_util.h)
is
> more preferable? (By the way could we get an empty data here?)

Whether we could get empty data or not depends on the callers.  Seems best to
support it.  I wasn't aware of vector_as_array - seems like a good idea, to me.

[hashimoto, 2012-12-13 03:58:46]

https://codereview.chromium.org/11439008/diff/24001/cloud_print/service/servi...
File cloud_print/service/service_state.cc (right):

https://codereview.chromium.org/11439008/diff/24001/cloud_print/service/servi...
cloud_print/service/service_state.cc:186: request.set_upload(new
net::UploadDataStream(&element_readers, 0));
On 2012/12/12 19:26:41, Matt Menke wrote:
> On 2012/12/12 10:29:07, hashimoto wrote:
> > On 2012/12/11 18:53:21, Matt Menke wrote:
> > > Egads...this function is ugly, though suppose that's more than a little
> beyond
> > > the scope of this CL.
> > > 
> > > This should all be done before we create the request - think it's best to
> > avoid
> > > destroying an UploadDataStream after its underlying data has been
destroyed,
> > > even if it technically won't cause any problems.
> > 
> > Should we switch to UploadOwnedBytesElementReader as a safer choice?
> > I think it doesn't cost much since the size of post_body is some KBs.
> 
> I think that's a good idea.

Done.

https://codereview.chromium.org/11439008/diff/24001/net/base/upload_bytes_ele...
File net/base/upload_bytes_element_reader.h (right):

https://codereview.chromium.org/11439008/diff/24001/net/base/upload_bytes_ele...
net/base/upload_bytes_element_reader.h:52: std::vector<char> data_;
On 2012/12/13 00:43:47, Matt Menke wrote:
> On 2012/12/13 00:07:55, kinuko wrote:
> > As for the use of &(*data)[0] I think vector_as_array (defined in
stl_util.h)
> is
> > more preferable? (By the way could we get an empty data here?)
> 
> Whether we could get empty data or not depends on the callers.  Seems best to
> support it.  I wasn't aware of vector_as_array - seems like a good idea, to
me.
Good point, it seems cleaner to support empty data, thanks.
Replaced &(*data)[0] with vector_as_array(data).

https://codereview.chromium.org/11439008/diff/43008/webkit/glue/resource_requ...
File webkit/glue/resource_request_body.cc (right):

https://codereview.chromium.org/11439008/diff/43008/webkit/glue/resource_requ...
webkit/glue/resource_request_body.cc:39: class FileElementReader : public
net::UploadFileElementReader {
On 2012/12/12 19:26:41, Matt Menke wrote:
> This class isn't needed, is it?  UploadFileElementReader copies everything it
> needs.

This class is needed to keep BlobData attached to this ResourceRequestBody
alive.
(http://code.google.com/searchframe#OAMlx_jo-ck/src/webkit/glue/resource_reque...)
Without owning ResourceRequestBody (and BlobData), the ShareableFileReference
attached to the BlobData would be lost.

SetUserData is making the world complicated…

[kinuko, 2012-12-13 06:05:47]

https://codereview.chromium.org/11439008/diff/43008/webkit/glue/resource_requ...
File webkit/glue/resource_request_body.cc (right):

https://codereview.chromium.org/11439008/diff/43008/webkit/glue/resource_requ...
webkit/glue/resource_request_body.cc:39: class FileElementReader : public
net::UploadFileElementReader {
On 2012/12/13 03:58:47, hashimoto wrote:
> On 2012/12/12 19:26:41, Matt Menke wrote:
> > This class isn't needed, is it?  UploadFileElementReader copies everything
it
> > needs.
> 
> This class is needed to keep BlobData attached to this ResourceRequestBody
> alive.
>
(http://code.google.com/searchframe#OAMlx_jo-ck/src/webkit/glue/resource_reque...)
> Without owning ResourceRequestBody (and BlobData), the ShareableFileReference
> attached to the BlobData would be lost.
> 
> SetUserData is making the world complicated…

Can we have a comment about that?

[hashimoto, 2012-12-13 06:17:50]

https://codereview.chromium.org/11439008/diff/43008/webkit/glue/resource_requ...
File webkit/glue/resource_request_body.cc (right):

https://codereview.chromium.org/11439008/diff/43008/webkit/glue/resource_requ...
webkit/glue/resource_request_body.cc:39: class FileElementReader : public
net::UploadFileElementReader {
On 2012/12/13 06:05:47, kinuko wrote:
> On 2012/12/13 03:58:47, hashimoto wrote:
> > On 2012/12/12 19:26:41, Matt Menke wrote:
> > > This class isn't needed, is it?  UploadFileElementReader copies everything
> it
> > > needs.
> > 
> > This class is needed to keep BlobData attached to this ResourceRequestBody
> > alive.
> >
>
(http://code.google.com/searchframe#OAMlx_jo-ck/src/webkit/glue/resource_reque...)
> > Without owning ResourceRequestBody (and BlobData), the
ShareableFileReference
> > attached to the BlobData would be lost.
> > 
> > SetUserData is making the world complicated…
> 
> Can we have a comment about that?

Done.

[kinuko, 2012-12-13 06:24:52]

webkit/ changes lgtm (though I'm not an owner)

[mmenke, 2012-12-13 16:44:39]

LGTM (Though I did not look at the webkit unit tests)

https://codereview.chromium.org/11439008/diff/61002/net/ocsp/nss_ocsp.cc
File net/ocsp/nss_ocsp.cc (right):

https://codereview.chromium.org/11439008/diff/61002/net/ocsp/nss_ocsp.cc#newc...
net/ocsp/nss_ocsp.cc:422: URLRequest* request_;           // The actual request
this wraps
Think it's worth mentioning that if upload_content_ is non-NULL, then this owns
request_'s upload data, it's not safe to delete until the request has finished
uploading data.

[hashimoto, 2012-12-13 17:39:12]

https://codereview.chromium.org/11439008/diff/61002/net/ocsp/nss_ocsp.cc
File net/ocsp/nss_ocsp.cc (right):

https://codereview.chromium.org/11439008/diff/61002/net/ocsp/nss_ocsp.cc#newc...
net/ocsp/nss_ocsp.cc:422: URLRequest* request_;           // The actual request
this wraps
On 2012/12/13 16:44:40, Matt Menke wrote:
> Think it's worth mentioning that if upload_content_ is non-NULL, then this
owns
> request_'s upload data, it's not safe to delete until the request has finished
> uploading data.
Seems DCHECK(!request_) in the dtor guarantees that upload_content_ outlives
request_.
Added a comment here and another DCHECK(!request_) to SetPostData.

[mmenke, 2012-12-13 17:42:11]

Still LGTM.

https://codereview.chromium.org/11439008/diff/61002/net/ocsp/nss_ocsp.cc
File net/ocsp/nss_ocsp.cc (right):

https://codereview.chromium.org/11439008/diff/61002/net/ocsp/nss_ocsp.cc#newc...
net/ocsp/nss_ocsp.cc:422: URLRequest* request_;           // The actual request
this wraps
On 2012/12/13 17:39:12, hashimoto wrote:
> On 2012/12/13 16:44:40, Matt Menke wrote:
> > Think it's worth mentioning that if upload_content_ is non-NULL, then this
> owns
> > request_'s upload data, it's not safe to delete until the request has
finished
> > uploading data.
> Seems DCHECK(!request_) in the dtor guarantees that upload_content_ outlives
> request_.
> Added a comment here and another DCHECK(!request_) to SetPostData.

You're right that it currently does - I'm just being paranoid about that
changing, resulting in a regression in code I'm not terribly familiar with.

Seems likely that it won't change, but request_ is not actually owned by
|this|...And I'm paranoid.

[hashimoto, 2012-12-13 17:45:25]

tony@, abodenha@, mpcomplete@, sky@, could you review this change as an owner of
each directory?

tony@: webkit/
abodenha@: cloud_print/
mpcomplete@: chrome/browser/extensions/api/web_request/
sky@: content/browser/

[tony, 2012-12-13 18:12:46]

webkit LGTM

[Matt Perry, 2012-12-13 18:28:29]

ext lgtm

[sky, 2012-12-13 20:19:20]

LGTM

[hashimoto, 2012-12-15 02:03:20]

TBR-ing abodenha@chromium.org for cloud_print/service/service_state.cc

[commit-bot: I haz the power, 2012-12-15 02:06:37]

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

[commit-bot: I haz the power, 2012-12-15 02:25:38]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-12-15 03:51:32]

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

[commit-bot: I haz the power, 2012-12-15 04:01:21]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[hashimoto, 2012-12-15 05:11:03]

Fixed net/test/spawner_communicator.cc which is used only by android build.

[commit-bot: I haz the power, 2012-12-15 05:11:25]

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

[commit-bot: I haz the power, 2012-12-15 08:52:26]

Retried try job too often on win_rel for step(s) browser_tests

[commit-bot: I haz the power, 2012-12-15 18:09:34]

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

[commit-bot: I haz the power, 2012-12-15 20:28:31]

Change committed as 173318