Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
809663003:
net: Add SetUploadStream method to URLFetcher. (Closed)
Descriptionnet: Add SetUploadStream method to URLFetcher.
Currently URLFetcher can only take data bytes or file path.
The CL adds a method to URLFetcher for taking UploadDataStream, which can consist of multiple elements.
BUG=442652
TEST=URLFetcherPostStreamTest.Basic
Committed: https://crrev.com/92da739316361e17f59ef9f04b7aeabf8630bff1
Cr-Commit-Position: refs/heads/master@{#312412}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 1
Patch Set 5 : #Patch Set 6 : Fix retrying. #
Total comments: 14
Patch Set 7 : Add retry test. #Patch Set 8 : #
Total comments: 4
Patch Set 9 : Address comments and fix tests #
Total comments: 4
Patch Set 10 : Fix a compiler error #
Total comments: 4
Patch Set 11 : Fixed. #
Total comments: 4
Patch Set 12 : #
Messages
Total messages: 42 (11 generated)
hirono@chromium.org changed reviewers: + mmenke@chromium.org
PTAL the CL? Thanks!
https://codereview.chromium.org/809663003/diff/60001/net/url_request/url_fetc... File net/url_request/url_fetcher.h (right): https://codereview.chromium.org/809663003/diff/60001/net/url_request/url_fetc... net/url_request/url_fetcher.h:159: scoped_ptr<net::UploadDataStream> upload_stream) = 0; Why is this needed? This is incompatible with all of the retry stuff, and I'd rather not have incompatible features unless it's really needed.
On 2014/12/18 15:38:46, mmenke wrote: > https://codereview.chromium.org/809663003/diff/60001/net/url_request/url_fetc... > File net/url_request/url_fetcher.h (right): > > https://codereview.chromium.org/809663003/diff/60001/net/url_request/url_fetc... > net/url_request/url_fetcher.h:159: scoped_ptr<net::UploadDataStream> > upload_stream) = 0; > Why is this needed? This is incompatible with all of the retry stuff, and I'd > rather not have incompatible features unless it's really needed. And just to add...Embedders that need more features can just use URLRequests directly. if this is just a one-off case, that may be the way to go.
> Why is this needed? This is incompatible with all of the retry stuff, and I'd > rather not have incompatible features unless it's really needed. The main purpose of this is letting URLFetcher send 'multipart/related' body that consists of both JSON strings and file contents. Definitely I should not destroy the retry feature. It looks I need to pass something that can generate UploadDataStream multiple times like UploadDataStreamFactory instead of UploadDataStream itself. > And just to add...Embedders that need more features can just use URLRequests > directly. if this is just a one-off case, that may be the way to go. I'm not sure if I understand correctly. You mean we should use URLRequests directly instead of adding a new method to URLFetcher? If possible, I'd like to reuse the convenient URLFetcher class. Our code have already depended on URLFetcher. And it looks URLFetcher works regardless of what is the upload contents, as long as URLFetcher can create UploadDataStream multiple times for retry.
On 2014/12/19 01:39:08, hirono wrote: > > Why is this needed? This is incompatible with all of the retry stuff, and I'd > > rather not have incompatible features unless it's really needed. > > The main purpose of this is letting URLFetcher send 'multipart/related' body > that consists of both JSON strings and file contents. > Definitely I should not destroy the retry feature. > It looks I need to pass something that can generate UploadDataStream multiple > times like UploadDataStreamFactory instead of UploadDataStream itself. > > > > And just to add...Embedders that need more features can just use URLRequests > > directly. if this is just a one-off case, that may be the way to go. > > I'm not sure if I understand correctly. You mean we should use URLRequests > directly instead of adding a new method to URLFetcher? > If possible, I'd like to reuse the convenient URLFetcher class. Our code have > already depended on URLFetcher. > And it looks URLFetcher works regardless of what is the upload contents, as long > as URLFetcher can create UploadDataStream multiple times for retry. Right, but the URLFetcher *cannot* create UploadDataStreams at all with the API you want to introduce. Instead, it takes ownership of the one you pass to it, which it passes to the first URLRequest it makes. Then when it makes another, it has no UploadDataStream to pass to it, and you're in trouble.
On 2014/12/19 02:00:10, mmenke (OOO 12-20 to 1-4) wrote: > On 2014/12/19 01:39:08, hirono wrote: > > > Why is this needed? This is incompatible with all of the retry stuff, and > I'd > > > rather not have incompatible features unless it's really needed. > > > > The main purpose of this is letting URLFetcher send 'multipart/related' body > > that consists of both JSON strings and file contents. > > Definitely I should not destroy the retry feature. > > It looks I need to pass something that can generate UploadDataStream multiple > > times like UploadDataStreamFactory instead of UploadDataStream itself. > > > > > > > And just to add...Embedders that need more features can just use URLRequests > > > directly. if this is just a one-off case, that may be the way to go. > > > > I'm not sure if I understand correctly. You mean we should use URLRequests > > directly instead of adding a new method to URLFetcher? > > If possible, I'd like to reuse the convenient URLFetcher class. Our code have > > already depended on URLFetcher. > > And it looks URLFetcher works regardless of what is the upload contents, as > long > > as URLFetcher can create UploadDataStream multiple times for retry. > > Right, but the URLFetcher *cannot* create UploadDataStreams at all with the API > you want to introduce. Instead, it takes ownership of the one you pass to it, > which it passes to the first URLRequest it makes. Then when it makes another, > it has no UploadDataStream to pass to it, and you're in trouble. Yes, I understood my first API does not work for retrying. Can I modify the API so that it takes UploadDataStreamFactory (or like this) instead of UploadDataStream?
On 2014/12/19 02:09:40, hirono wrote: > On 2014/12/19 02:00:10, mmenke (OOO 12-20 to 1-4) wrote: > > On 2014/12/19 01:39:08, hirono wrote: > > > > Why is this needed? This is incompatible with all of the retry stuff, and > > I'd > > > > rather not have incompatible features unless it's really needed. > > > > > > The main purpose of this is letting URLFetcher send 'multipart/related' body > > > that consists of both JSON strings and file contents. > > > Definitely I should not destroy the retry feature. > > > It looks I need to pass something that can generate UploadDataStream > multiple > > > times like UploadDataStreamFactory instead of UploadDataStream itself. > > > > > > > > > > And just to add...Embedders that need more features can just use > URLRequests > > > > directly. if this is just a one-off case, that may be the way to go. > > > > > > I'm not sure if I understand correctly. You mean we should use URLRequests > > > directly instead of adding a new method to URLFetcher? > > > If possible, I'd like to reuse the convenient URLFetcher class. Our code > have > > > already depended on URLFetcher. > > > And it looks URLFetcher works regardless of what is the upload contents, as > > long > > > as URLFetcher can create UploadDataStream multiple times for retry. > > > > Right, but the URLFetcher *cannot* create UploadDataStreams at all with the > API > > you want to introduce. Instead, it takes ownership of the one you pass to it, > > which it passes to the first URLRequest it makes. Then when it makes another, > > it has no UploadDataStream to pass to it, and you're in trouble. > > Yes, I understood my first API does not work for retrying. > Can I modify the API so that it takes UploadDataStreamFactory (or like this) > instead of UploadDataStream? @mmenke - I updated my CL so that it recreates UploadDataStream for each time when StartURLRequest is called. If you like this way, I also think of delegating SetUploadData and SetUploadFilePath to the new SetUploadStream. It can reduces the variables in URLFetcherCore that are used conditionally (e.g. upload_file_path_) and we can remove DCHECKs for the variables. WDYT?
Patchset #6 (id:100001) has been deleted
Patchset #6 (id:120001) has been deleted
Patchset #5 (id:80001) has been deleted
On 2014/12/22 02:12:25, hirono wrote: > On 2014/12/19 02:09:40, hirono wrote: > > On 2014/12/19 02:00:10, mmenke (OOO 12-20 to 1-4) wrote: > > > On 2014/12/19 01:39:08, hirono wrote: > > > > > Why is this needed? This is incompatible with all of the retry stuff, > and > > > I'd > > > > > rather not have incompatible features unless it's really needed. > > > > > > > > The main purpose of this is letting URLFetcher send 'multipart/related' > body > > > > that consists of both JSON strings and file contents. > > > > Definitely I should not destroy the retry feature. > > > > It looks I need to pass something that can generate UploadDataStream > > multiple > > > > times like UploadDataStreamFactory instead of UploadDataStream itself. > > > > > > > > > > > > > And just to add...Embedders that need more features can just use > > URLRequests > > > > > directly. if this is just a one-off case, that may be the way to go. > > > > > > > > I'm not sure if I understand correctly. You mean we should use URLRequests > > > > directly instead of adding a new method to URLFetcher? > > > > If possible, I'd like to reuse the convenient URLFetcher class. Our code > > have > > > > already depended on URLFetcher. > > > > And it looks URLFetcher works regardless of what is the upload contents, > as > > > long > > > > as URLFetcher can create UploadDataStream multiple times for retry. > > > > > > Right, but the URLFetcher *cannot* create UploadDataStreams at all with the > > API > > > you want to introduce. Instead, it takes ownership of the one you pass to > it, > > > which it passes to the first URLRequest it makes. Then when it makes > another, > > > it has no UploadDataStream to pass to it, and you're in trouble. > > > > Yes, I understood my first API does not work for retrying. > > Can I modify the API so that it takes UploadDataStreamFactory (or like this) > > instead of UploadDataStream? > > @mmenke - I updated my CL so that it recreates UploadDataStream for each time > when StartURLRequest is called. > If you like this way, I also think of delegating SetUploadData and > SetUploadFilePath to the new SetUploadStream. > It can reduces the variables in URLFetcherCore that are used conditionally (e.g. > upload_file_path_) and we can remove DCHECKs for the variables. WDYT? @mmenke - ping?
On 2015/01/06 00:44:39, hirono wrote: > On 2014/12/22 02:12:25, hirono wrote: > > On 2014/12/19 02:09:40, hirono wrote: > > > On 2014/12/19 02:00:10, mmenke (OOO 12-20 to 1-4) wrote: > > > > On 2014/12/19 01:39:08, hirono wrote: > > > > > > Why is this needed? This is incompatible with all of the retry stuff, > > and > > > > I'd > > > > > > rather not have incompatible features unless it's really needed. > > > > > > > > > > The main purpose of this is letting URLFetcher send 'multipart/related' > > body > > > > > that consists of both JSON strings and file contents. > > > > > Definitely I should not destroy the retry feature. > > > > > It looks I need to pass something that can generate UploadDataStream > > > multiple > > > > > times like UploadDataStreamFactory instead of UploadDataStream itself. > > > > > > > > > > > > > > > > And just to add...Embedders that need more features can just use > > > URLRequests > > > > > > directly. if this is just a one-off case, that may be the way to go. > > > > > > > > > > I'm not sure if I understand correctly. You mean we should use > URLRequests > > > > > directly instead of adding a new method to URLFetcher? > > > > > If possible, I'd like to reuse the convenient URLFetcher class. Our code > > > have > > > > > already depended on URLFetcher. > > > > > And it looks URLFetcher works regardless of what is the upload contents, > > as > > > > long > > > > > as URLFetcher can create UploadDataStream multiple times for retry. > > > > > > > > Right, but the URLFetcher *cannot* create UploadDataStreams at all with > the > > > API > > > > you want to introduce. Instead, it takes ownership of the one you pass to > > it, > > > > which it passes to the first URLRequest it makes. Then when it makes > > another, > > > > it has no UploadDataStream to pass to it, and you're in trouble. > > > > > > Yes, I understood my first API does not work for retrying. > > > Can I modify the API so that it takes UploadDataStreamFactory (or like this) > > > instead of UploadDataStream? > > > > @mmenke - I updated my CL so that it recreates UploadDataStream for each time > > when StartURLRequest is called. > > If you like this way, I also think of delegating SetUploadData and > > SetUploadFilePath to the new SetUploadStream. > > It can reduces the variables in URLFetcherCore that are used conditionally > (e.g. > > upload_file_path_) and we can remove DCHECKs for the variables. WDYT? > > @mmenke - ping? @mmenke - Sorry for hustling. But I need your advice. Do the CL still have a problem with retrying, or is the design not good? Thanks!
On 2015/01/07 02:08:25, hirono wrote: > On 2015/01/06 00:44:39, hirono wrote: > > On 2014/12/22 02:12:25, hirono wrote: > > > On 2014/12/19 02:09:40, hirono wrote: > > > > On 2014/12/19 02:00:10, mmenke (OOO 12-20 to 1-4) wrote: > > > > > On 2014/12/19 01:39:08, hirono wrote: > > > > > > > Why is this needed? This is incompatible with all of the retry > stuff, > > > and > > > > > I'd > > > > > > > rather not have incompatible features unless it's really needed. > > > > > > > > > > > > The main purpose of this is letting URLFetcher send > 'multipart/related' > > > body > > > > > > that consists of both JSON strings and file contents. > > > > > > Definitely I should not destroy the retry feature. > > > > > > It looks I need to pass something that can generate UploadDataStream > > > > multiple > > > > > > times like UploadDataStreamFactory instead of UploadDataStream itself. > > > > > > > > > > > > > > > > > > > And just to add...Embedders that need more features can just use > > > > URLRequests > > > > > > > directly. if this is just a one-off case, that may be the way to > go. > > > > > > > > > > > > I'm not sure if I understand correctly. You mean we should use > > URLRequests > > > > > > directly instead of adding a new method to URLFetcher? > > > > > > If possible, I'd like to reuse the convenient URLFetcher class. Our > code > > > > have > > > > > > already depended on URLFetcher. > > > > > > And it looks URLFetcher works regardless of what is the upload > contents, > > > as > > > > > long > > > > > > as URLFetcher can create UploadDataStream multiple times for retry. > > > > > > > > > > Right, but the URLFetcher *cannot* create UploadDataStreams at all with > > the > > > > API > > > > > you want to introduce. Instead, it takes ownership of the one you pass > to > > > it, > > > > > which it passes to the first URLRequest it makes. Then when it makes > > > another, > > > > > it has no UploadDataStream to pass to it, and you're in trouble. > > > > > > > > Yes, I understood my first API does not work for retrying. > > > > Can I modify the API so that it takes UploadDataStreamFactory (or like > this) > > > > instead of UploadDataStream? > > > > > > @mmenke - I updated my CL so that it recreates UploadDataStream for each > time > > > when StartURLRequest is called. > > > If you like this way, I also think of delegating SetUploadData and > > > SetUploadFilePath to the new SetUploadStream. > > > It can reduces the variables in URLFetcherCore that are used conditionally > > (e.g. > > > upload_file_path_) and we can remove DCHECKs for the variables. WDYT? > > > > @mmenke - ping? > > @mmenke - Sorry for hustling. But I need your advice. > Do the CL still have a problem with retrying, or is the design not good? > Thanks! I'm sorry for slowness here, but I have a bunch of stuff in my queue (Including over 10 reviews).
On 2015/01/08 15:14:21, mmenke wrote: > On 2015/01/07 02:08:25, hirono wrote: > > On 2015/01/06 00:44:39, hirono wrote: > > > On 2014/12/22 02:12:25, hirono wrote: > > > > On 2014/12/19 02:09:40, hirono wrote: > > > > > On 2014/12/19 02:00:10, mmenke (OOO 12-20 to 1-4) wrote: > > > > > > On 2014/12/19 01:39:08, hirono wrote: > > > > > > > > Why is this needed? This is incompatible with all of the retry > > stuff, > > > > and > > > > > > I'd > > > > > > > > rather not have incompatible features unless it's really needed. > > > > > > > > > > > > > > The main purpose of this is letting URLFetcher send > > 'multipart/related' > > > > body > > > > > > > that consists of both JSON strings and file contents. > > > > > > > Definitely I should not destroy the retry feature. > > > > > > > It looks I need to pass something that can generate UploadDataStream > > > > > multiple > > > > > > > times like UploadDataStreamFactory instead of UploadDataStream > itself. > > > > > > > > > > > > > > > > > > > > > > And just to add...Embedders that need more features can just use > > > > > URLRequests > > > > > > > > directly. if this is just a one-off case, that may be the way to > > go. > > > > > > > > > > > > > > I'm not sure if I understand correctly. You mean we should use > > > URLRequests > > > > > > > directly instead of adding a new method to URLFetcher? > > > > > > > If possible, I'd like to reuse the convenient URLFetcher class. Our > > code > > > > > have > > > > > > > already depended on URLFetcher. > > > > > > > And it looks URLFetcher works regardless of what is the upload > > contents, > > > > as > > > > > > long > > > > > > > as URLFetcher can create UploadDataStream multiple times for retry. > > > > > > > > > > > > Right, but the URLFetcher *cannot* create UploadDataStreams at all > with > > > the > > > > > API > > > > > > you want to introduce. Instead, it takes ownership of the one you > pass > > to > > > > it, > > > > > > which it passes to the first URLRequest it makes. Then when it makes > > > > another, > > > > > > it has no UploadDataStream to pass to it, and you're in trouble. > > > > > > > > > > Yes, I understood my first API does not work for retrying. > > > > > Can I modify the API so that it takes UploadDataStreamFactory (or like > > this) > > > > > instead of UploadDataStream? > > > > > > > > @mmenke - I updated my CL so that it recreates UploadDataStream for each > > time > > > > when StartURLRequest is called. > > > > If you like this way, I also think of delegating SetUploadData and > > > > SetUploadFilePath to the new SetUploadStream. > > > > It can reduces the variables in URLFetcherCore that are used conditionally > > > (e.g. > > > > upload_file_path_) and we can remove DCHECKs for the variables. WDYT? > > > > > > @mmenke - ping? > > > > @mmenke - Sorry for hustling. But I need your advice. > > Do the CL still have a problem with retrying, or is the design not good? > > Thanks! > > I'm sorry for slowness here, but I have a bunch of stuff in my queue (Including > over 10 reviews). That's a lot, so I'll just wait quietly. Thank you for replying!
Sorry again for slowness - a lot of reviews after the break, and I saved the ones I thought I'd be pushing back on for last (Looking at this CL, though, I'm fairly happy with the change - we could also consider switching over the other upload methods to use this method, too, rather than have a variable for each one, which really appeals to me) https://codereview.chromium.org/809663003/diff/160001/net/url_request/url_fet... File net/url_request/url_fetcher.h (right): https://codereview.chromium.org/809663003/diff/160001/net/url_request/url_fet... net/url_request/url_fetcher.h:96: // a The callback should not assign content type - that's actually done when we're setting the callback. https://codereview.chromium.org/809663003/diff/160001/net/url_request/url_fet... net/url_request/url_fetcher.h:97: // fresh upload data stream every time it's called. Nit: Merge lines https://codereview.chromium.org/809663003/diff/160001/net/url_request/url_fet... net/url_request/url_fetcher.h:162: // will be set to the length of this data). There is no |upload_stream|. Should say something about callback - most important are that it's called on the network thread, and it may be called multiple times in the case the fetcher is set to retry the request. https://codereview.chromium.org/809663003/diff/160001/net/url_request/url_fet... File net/url_request/url_fetcher_core.cc (right): https://codereview.chromium.org/809663003/diff/160001/net/url_request/url_fet... net/url_request/url_fetcher_core.cc:187: DCHECK(upload_stream_factory_.is_null()); Mind making a private method for all these shared checks? CheckIfHasNoUploadData, or DieIfHasUploadData or something. Or maybe better, get rid of upload_content_set_, and add a method to figure out if upload_content_set_ would be true if we kept it, or is_chunked_upload_ is true. Call it IsUpload or somesuch. Then DCHECK on that instead (Will still need to DCHECK on upload_content_type_.empty() everywhere, too). https://codereview.chromium.org/809663003/diff/160001/net/url_request/url_fet... File net/url_request/url_fetcher_impl_unittest.cc (right): https://codereview.chromium.org/809663003/diff/160001/net/url_request/url_fet... net/url_request/url_fetcher_impl_unittest.cc:61: new ElementsUploadDataStream(readers.Pass(), 0)).Pass(); Do we get anything from having multiple elements here? If we do, I'm fine with it, but should have a comment about why it's useful for the tests that use it. https://codereview.chromium.org/809663003/diff/160001/net/url_request/url_fet... net/url_request/url_fetcher_impl_unittest.cc:612: void URLFetcherPostStreamTest::CreateFetcher(const GURL& url) { We really should make sure the test actually retries, so it runs the callback twice (And count the times the callback is run). I think the simplest way to do this would be to modify the test server so either either the "echo" URL takes an option response code parameter, or add a new URL "echo-with-500-status-code" - former seems preferable. I'm open to other ideas.
On 2015/01/09 17:42:25, mmenke wrote: > Sorry again for slowness - a lot of reviews after the break, and I saved the > ones I thought I'd be pushing back on for last (Looking at this CL, though, I'm > fairly happy with the change - we could also consider switching over the other > upload methods to use this method, too, rather than have a variable for each > one, which really appeals to me) Just to clarify - I'm not suggesting you do that in this CL, just remarking it's an opportunity for the net stack team to do a little cleanup.
Another quick comment. https://codereview.chromium.org/809663003/diff/160001/net/url_request/url_fet... File net/url_request/url_fetcher_core.cc (right): https://codereview.chromium.org/809663003/diff/160001/net/url_request/url_fet... net/url_request/url_fetcher_core.cc:622: request_->set_upload(upload_stream_factory_.Run().Pass()); We should DCHECK that we don't get a null stream. I suppose storing it in as temporary scoped_ptr on the stack, DCHECKing, and then passing it. There is request->get_upload() which we could use instead, but I'm not a big fan of that method (And would rather it didn't exist, though it's just used for extensions, so that ship has probably sailed).
Thanks! https://codereview.chromium.org/809663003/diff/160001/net/url_request/url_fet... File net/url_request/url_fetcher.h (right): https://codereview.chromium.org/809663003/diff/160001/net/url_request/url_fet... net/url_request/url_fetcher.h:96: // a On 2015/01/09 17:42:25, mmenke wrote: > The callback should not assign content type - that's actually done when we're > setting the callback. Done. https://codereview.chromium.org/809663003/diff/160001/net/url_request/url_fet... net/url_request/url_fetcher.h:97: // fresh upload data stream every time it's called. On 2015/01/09 17:42:25, mmenke wrote: > Nit: Merge lines Done. https://codereview.chromium.org/809663003/diff/160001/net/url_request/url_fet... net/url_request/url_fetcher.h:162: // will be set to the length of this data). On 2015/01/09 17:42:24, mmenke wrote: > There is no |upload_stream|. Should say something about callback - most > important are that it's called on the network thread, and it may be called > multiple times in the case the fetcher is set to retry the request. Done. https://codereview.chromium.org/809663003/diff/160001/net/url_request/url_fet... File net/url_request/url_fetcher_core.cc (right): https://codereview.chromium.org/809663003/diff/160001/net/url_request/url_fet... net/url_request/url_fetcher_core.cc:187: DCHECK(upload_stream_factory_.is_null()); On 2015/01/09 17:42:25, mmenke wrote: > Mind making a private method for all these shared checks? > CheckIfHasNoUploadData, or DieIfHasUploadData or something. > > Or maybe better, get rid of upload_content_set_, and add a method to figure out > if upload_content_set_ would be true if we kept it, or is_chunked_upload_ is > true. Call it IsUpload or somesuch. Then DCHECK on that instead (Will still > need to DCHECK on upload_content_type_.empty() everywhere, too). I added IsUploadDataSet to check upload_content_, upload_file_path_, and upload_stream_factory_. Done https://codereview.chromium.org/809663003/diff/160001/net/url_request/url_fet... net/url_request/url_fetcher_core.cc:622: request_->set_upload(upload_stream_factory_.Run().Pass()); On 2015/01/09 18:13:51, mmenke wrote: > We should DCHECK that we don't get a null stream. I suppose storing it in as > temporary scoped_ptr on the stack, DCHECKing, and then passing it. There is > request->get_upload() which we could use instead, but I'm not a big fan of that > method (And would rather it didn't exist, though it's just used for extensions, > so that ship has probably sailed). Done. https://codereview.chromium.org/809663003/diff/160001/net/url_request/url_fet... File net/url_request/url_fetcher_impl_unittest.cc (right): https://codereview.chromium.org/809663003/diff/160001/net/url_request/url_fet... net/url_request/url_fetcher_impl_unittest.cc:61: new ElementsUploadDataStream(readers.Pass(), 0)).Pass(); On 2015/01/09 17:42:25, mmenke wrote: > Do we get anything from having multiple elements here? If we do, I'm fine with > it, but should have a comment about why it's useful for the tests that use it. No, let me simplify it. https://codereview.chromium.org/809663003/diff/160001/net/url_request/url_fet... net/url_request/url_fetcher_impl_unittest.cc:612: void URLFetcherPostStreamTest::CreateFetcher(const GURL& url) { On 2015/01/09 17:42:25, mmenke wrote: > We really should make sure the test actually retries, so it runs the callback > twice (And count the times the callback is run). > > I think the simplest way to do this would be to modify the test server so either > either the "echo" URL takes an option response code parameter, or add a new URL > "echo-with-500-status-code" - former seems preferable. I'm open to other ideas. I added status parameter to the "echo" URL. Done.
Minor comments, but looks like you're also getting test failures now. https://codereview.chromium.org/809663003/diff/200001/net/url_request/url_fet... File net/url_request/url_fetcher.h (right): https://codereview.chromium.org/809663003/diff/200001/net/url_request/url_fet... net/url_request/url_fetcher.h:161: // value will be set to the length of this data). Worth mentioning the callback may be called multiple times - can see people assuming otherwise, and then causing crashes in rare cases, which have a crash stack that make it look like a net/ bug. https://codereview.chromium.org/809663003/diff/200001/net/url_request/url_fet... File net/url_request/url_fetcher_impl_unittest.cc (right): https://codereview.chromium.org/809663003/diff/200001/net/url_request/url_fet... net/url_request/url_fetcher_impl_unittest.cc:618: EXPECT_EQ(std::string("bobsyeruncle\n"), data); nit: set::string isn't needed here.
Patchset #9 (id:220001) has been deleted
Patchset #9 (id:240001) has been deleted
Thanks! https://codereview.chromium.org/809663003/diff/200001/net/url_request/url_fet... File net/url_request/url_fetcher.h (right): https://codereview.chromium.org/809663003/diff/200001/net/url_request/url_fet... net/url_request/url_fetcher.h:161: // value will be set to the length of this data). On 2015/01/15 15:38:54, mmenke wrote: > Worth mentioning the callback may be called multiple times - can see people > assuming otherwise, and then causing crashes in rare cases, which have a crash > stack that make it look like a net/ bug. Done. https://codereview.chromium.org/809663003/diff/200001/net/url_request/url_fet... File net/url_request/url_fetcher_impl_unittest.cc (right): https://codereview.chromium.org/809663003/diff/200001/net/url_request/url_fet... net/url_request/url_fetcher_impl_unittest.cc:618: EXPECT_EQ(std::string("bobsyeruncle\n"), data); On 2015/01/15 15:38:54, mmenke wrote: > nit: set::string isn't needed here. Done.
https://codereview.chromium.org/809663003/diff/260001/net/url_request/url_fet... File net/url_request/url_fetcher_core.cc (right): https://codereview.chromium.org/809663003/diff/260001/net/url_request/url_fet... net/url_request/url_fetcher_core.cc:182: upload_content_set_ = true; Why is this still needed? https://codereview.chromium.org/809663003/diff/260001/net/url_request/url_fet... net/url_request/url_fetcher_core.cc:187: CheckIfHasNoUploadData() && upload_content_type_.empty()); Add parens before Check (Mixing || and && without them is just a bad idea) https://codereview.chromium.org/809663003/diff/280001/net/url_request/url_fet... File net/url_request/url_fetcher.h (right): https://codereview.chromium.org/809663003/diff/280001/net/url_request/url_fet... net/url_request/url_fetcher.h:162: // mutliple times to retry the request. Maybe "to retry the request" -> "if the request is retried"?
On 2015/01/16 16:47:19, mmenke wrote: > https://codereview.chromium.org/809663003/diff/260001/net/url_request/url_fet... > File net/url_request/url_fetcher_core.cc (right): > > https://codereview.chromium.org/809663003/diff/260001/net/url_request/url_fet... > net/url_request/url_fetcher_core.cc:182: upload_content_set_ = true; > Why is this still needed? > > https://codereview.chromium.org/809663003/diff/260001/net/url_request/url_fet... > net/url_request/url_fetcher_core.cc:187: CheckIfHasNoUploadData() && > upload_content_type_.empty()); > Add parens before Check (Mixing || and && without them is just a bad idea) > > https://codereview.chromium.org/809663003/diff/280001/net/url_request/url_fet... > File net/url_request/url_fetcher.h (right): > > https://codereview.chromium.org/809663003/diff/280001/net/url_request/url_fet... > net/url_request/url_fetcher.h:162: // mutliple times to retry the request. > Maybe "to retry the request" -> "if the request is retried"? Oops - accidentally made two of those comments on set 9
https://codereview.chromium.org/809663003/diff/280001/net/url_request/url_fet... File net/url_request/url_fetcher_core.cc (right): https://codereview.chromium.org/809663003/diff/280001/net/url_request/url_fet... net/url_request/url_fetcher_core.cc:971: bool URLFetcherCore::CheckIfHasNoUploadData() const { The fact that this has a "No" in it is confusing when returning a bool, since it means the bool is implicity negated, so if it returns false, we may have "no no" upload data. If we need to keep |upload_content_set_| around, suggest renaming this AssertHasNoUploadData, and move the DCHECKs in here, making it return a void.
Thanks! https://codereview.chromium.org/809663003/diff/260001/net/url_request/url_fet... File net/url_request/url_fetcher_core.cc (right): https://codereview.chromium.org/809663003/diff/260001/net/url_request/url_fet... net/url_request/url_fetcher_core.cc:182: upload_content_set_ = true; On 2015/01/16 16:47:18, mmenke wrote: > Why is this still needed? Because the class allows to set the empty upload_content_. So we cannot find out if the upload_content_ was set or not by seeing upload_content_.empty(). https://codereview.chromium.org/809663003/diff/260001/net/url_request/url_fet... net/url_request/url_fetcher_core.cc:187: CheckIfHasNoUploadData() && upload_content_type_.empty()); On 2015/01/16 16:47:18, mmenke wrote: > Add parens before Check (Mixing || and && without them is just a bad idea) Done. https://codereview.chromium.org/809663003/diff/280001/net/url_request/url_fet... File net/url_request/url_fetcher.h (right): https://codereview.chromium.org/809663003/diff/280001/net/url_request/url_fet... net/url_request/url_fetcher.h:162: // mutliple times to retry the request. On 2015/01/16 16:47:18, mmenke wrote: > Maybe "to retry the request" -> "if the request is retried"? Done. https://codereview.chromium.org/809663003/diff/280001/net/url_request/url_fet... File net/url_request/url_fetcher_core.cc (right): https://codereview.chromium.org/809663003/diff/280001/net/url_request/url_fet... net/url_request/url_fetcher_core.cc:971: bool URLFetcherCore::CheckIfHasNoUploadData() const { On 2015/01/16 16:53:29, mmenke wrote: > The fact that this has a "No" in it is confusing when returning a bool, since it > means the bool is implicity negated, so if it returns false, we may have "no no" > upload data. > > If we need to keep |upload_content_set_| around, suggest renaming this > AssertHasNoUploadData, and move the DCHECKs in here, making it return a void. Done.
LGTM https://codereview.chromium.org/809663003/diff/300001/net/url_request/url_fet... File net/url_request/url_fetcher_impl_unittest.cc (right): https://codereview.chromium.org/809663003/diff/300001/net/url_request/url_fet... net/url_request/url_fetcher_impl_unittest.cc:277: class URLFetcherPostStreamTest : public URLFetcherTest { Maybe URLFetcherSetUploadFactoryTest or just URLFetcherUploadFactoryTest? Think we should make it clear that we're testing the SetUploadStreamFactory, and I think "Upload" and "Factory" are more the more important descriptors than "Post" or "Stream". https://codereview.chromium.org/809663003/diff/300001/net/url_request/url_fet... net/url_request/url_fetcher_impl_unittest.cc:289: size_t create_stream_count_; Per Google style guide, should be private. Then can use a setter for it. Know other tests in this file break the private variables only rule, but prefer not to add rule violations.
Oh, and good catch on the fact that the upload body may be empty!
Thanks! https://codereview.chromium.org/809663003/diff/300001/net/url_request/url_fet... File net/url_request/url_fetcher_impl_unittest.cc (right): https://codereview.chromium.org/809663003/diff/300001/net/url_request/url_fet... net/url_request/url_fetcher_impl_unittest.cc:277: class URLFetcherPostStreamTest : public URLFetcherTest { On 2015/01/20 17:11:27, mmenke wrote: > Maybe URLFetcherSetUploadFactoryTest or just URLFetcherUploadFactoryTest? Think > we should make it clear that we're testing the SetUploadStreamFactory, and I > think "Upload" and "Factory" are more the more important descriptors than "Post" > or "Stream". Done. https://codereview.chromium.org/809663003/diff/300001/net/url_request/url_fet... net/url_request/url_fetcher_impl_unittest.cc:289: size_t create_stream_count_; On 2015/01/20 17:11:27, mmenke wrote: > Per Google style guide, should be private. Then can use a setter for it. Know > other tests in this file break the private variables only rule, but prefer not > to add rule violations. Done.
The CQ bit was checked by hirono@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/809663003/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/11...) win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was checked by hirono@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/809663003/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/11...) win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was checked by hirono@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/809663003/320001
Message was sent while issue was closed.
Committed patchset #12 (id:320001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/92da739316361e17f59ef9f04b7aeabf8630bff1 Cr-Commit-Position: refs/heads/master@{#312412} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
