Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(407)

Issue 2262653003: Make URLRequest::Read to return net errors or bytes read instead of a bool (Closed)

Created:
4 years, 4 months ago by maksims (do not use this acc)
Modified:
4 years, 2 months ago
Reviewers:
Dan Beam, mmenke, battre
CC:
chromium-reviews, cbentzel+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Make URLRequest::Read to return net errors or bytes read instead of a bool. 1) OnResponseStarted, and OnCompleted are overloaded for the transition period to easily commit all the changes. Some logic is applied in order to be able to server clients which use old Read method. 2) The set of these CL's will not remove URLRequestStatus completely, but I'm working towards this direction. Dependant changes: https://codereview.chromium.org/2265873002/ https://codereview.chromium.org/2262093002/ https://codereview.chromium.org/2269523002/ https://codereview.chromium.org/2261103002/ https://codereview.chromium.org/2264903003/ https://codereview.chromium.org/2264973002/ https://codereview.chromium.org/2262063002/ https://codereview.chromium.org/2267643002/ https://codereview.chromium.org/2269523002/ BUG=423484 Committed: https://crrev.com/0f4aa14df5c5889c37fceb98e234676913822d36 Cr-Commit-Position: refs/heads/master@{#416500}

Patch Set 1 #

Patch Set 2 : fixing compilation #

Patch Set 3 : ... #

Patch Set 4 : ... #

Patch Set 5 : more fixes #

Total comments: 26

Patch Set 6 : Matt's comments #

Patch Set 7 : rebased #

Total comments: 45

Patch Set 8 : Matt's comments #

Patch Set 9 : Remove TestDelegate::OnResponseStarted(URLRequest* request) #

Patch Set 10 : rebased #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+262 lines, -175 lines) Patch
M content/browser/webui/url_data_manager_backend_unittest.cc View 1 chunk +3 lines, -3 lines 0 comments Download
M net/base/layered_network_delegate.h View 1 2 3 4 5 1 chunk +3 lines, -2 lines 1 comment Download
M net/base/layered_network_delegate.cc View 1 2 3 4 5 2 chunks +8 lines, -6 lines 0 comments Download
M net/base/layered_network_delegate_unittest.cc View 1 2 3 3 chunks +4 lines, -4 lines 0 comments Download
M net/base/network_delegate.h View 1 2 3 4 5 6 7 3 chunks +10 lines, -2 lines 0 comments Download
M net/base/network_delegate.cc View 1 2 3 4 5 6 7 4 chunks +29 lines, -4 lines 0 comments Download
M net/base/network_delegate_impl.h View 1 2 3 4 5 2 chunks +6 lines, -0 lines 0 comments Download
M net/base/network_delegate_impl.cc View 1 2 3 4 5 6 1 chunk +14 lines, -4 lines 0 comments Download
M net/test/embedded_test_server/embedded_test_server_unittest.cc View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -2 lines 0 comments Download
M net/url_request/url_request.h View 1 2 3 4 5 6 7 7 chunks +37 lines, -33 lines 0 comments Download
M net/url_request/url_request.cc View 1 2 3 4 5 6 7 8 10 chunks +62 lines, -39 lines 0 comments Download
M net/url_request/url_request_file_dir_job_unittest.cc View 1 chunk +1 line, -1 line 0 comments Download
M net/url_request/url_request_job.h View 1 2 3 4 5 6 1 chunk +3 lines, -3 lines 0 comments Download
M net/url_request/url_request_job.cc View 1 2 3 4 5 6 7 1 chunk +14 lines, -15 lines 0 comments Download
M net/url_request/url_request_simple_job_unittest.cc View 1 chunk +4 lines, -3 lines 0 comments Download
M net/url_request/url_request_test_util.h View 1 2 3 4 5 6 7 8 4 chunks +7 lines, -3 lines 0 comments Download
M net/url_request/url_request_test_util.cc View 1 2 3 4 5 6 7 8 7 chunks +47 lines, -41 lines 0 comments Download
M net/url_request/url_request_unittest.cc View 2 chunks +8 lines, -10 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 177 (155 generated)
maksims (do not use this acc)
Hi Matt, Would you please take a first look into that? What do you think ...
4 years, 4 months ago (2016-08-24 12:39:36 UTC) #56
mmenke
On 2016/08/24 12:39:36, maksims wrote: > Hi Matt, > > Would you please take a ...
4 years, 4 months ago (2016-08-24 14:52:39 UTC) #61
maksims (do not use this acc)
On 2016/08/24 14:52:39, mmenke (busy) wrote: > On 2016/08/24 12:39:36, maksims wrote: > > Hi ...
4 years, 3 months ago (2016-08-26 10:10:16 UTC) #117
mmenke
On 2016/08/26 10:10:16, maksims wrote: > On 2016/08/24 14:52:39, mmenke (busy) wrote: > > On ...
4 years, 3 months ago (2016-08-26 14:36:44 UTC) #118
mmenke
Wow, looks like you've done a lot of work here, skimming over your followup CLs. ...
4 years, 3 months ago (2016-08-26 19:36:48 UTC) #120
maksims (do not use this acc)
https://codereview.chromium.org/2262653003/diff/410001/net/url_request/url_request.cc File net/url_request/url_request.cc (right): https://codereview.chromium.org/2262653003/diff/410001/net/url_request/url_request.cc#newcode169 net/url_request/url_request.cc:169: if (!implemented_) On 2016/08/26 19:36:48, mmenke wrote: > The ...
4 years, 3 months ago (2016-08-29 12:30:36 UTC) #129
maksims (do not use this acc)
gentle reminder
4 years, 3 months ago (2016-08-30 05:53:55 UTC) #130
mmenke
On 2016/08/30 05:53:55, maksims wrote: > gentle reminder Sorry for slowness, there's a lot of ...
4 years, 3 months ago (2016-08-30 20:17:52 UTC) #136
mmenke
Sorry, earlier comment was about your *other* CL. I'm really happy with this one, now. ...
4 years, 3 months ago (2016-08-30 22:09:12 UTC) #137
maksims (do not use this acc)
https://codereview.chromium.org/2262653003/diff/470001/net/base/network_delegate.cc File net/base/network_delegate.cc (right): https://codereview.chromium.org/2262653003/diff/470001/net/base/network_delegate.cc#newcode96 net/base/network_delegate.cc:96: } On 2016/08/30 22:09:11, mmenke wrote: > Order in ...
4 years, 3 months ago (2016-08-31 11:17:00 UTC) #147
mmenke
https://codereview.chromium.org/2262653003/diff/470001/net/base/network_delegate.h File net/base/network_delegate.h (right): https://codereview.chromium.org/2262653003/diff/470001/net/base/network_delegate.h#newcode308 net/base/network_delegate.h:308: bool implemented_; On 2016/08/31 11:16:59, maksims wrote: > On ...
4 years, 3 months ago (2016-08-31 21:35:47 UTC) #148
mmenke
https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_request_test_util.cc File net/url_request/url_request_test_util.cc (right): https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_request_test_util.cc#newcode257 net/url_request/url_request_test_util.cc:257: } On 2016/08/31 21:35:47, mmenke wrote: > On 2016/08/31 ...
4 years, 3 months ago (2016-08-31 21:39:27 UTC) #149
maksims (do not use this acc)
https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_request.cc File net/url_request/url_request.cc (right): https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_request.cc#newcode509 net/url_request/url_request.cc:509: return; On 2016/08/31 21:35:47, mmenke wrote: > On 2016/08/31 ...
4 years, 3 months ago (2016-09-01 07:13:17 UTC) #151
mmenke
Did you add brettw as a reviewer for url_data_manager_backend_unittest.cc? He's a bad choice of reviewer ...
4 years, 3 months ago (2016-09-01 14:54:40 UTC) #157
mmenke
LGTM! https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_request.cc File net/url_request/url_request.cc (right): https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_request.cc#newcode509 net/url_request/url_request.cc:509: return; On 2016/09/01 07:13:17, maksims wrote: > On ...
4 years, 3 months ago (2016-09-01 14:57:28 UTC) #158
maksims (do not use this acc)
dbeam@, would you please review url_data_manager_backend_unittest.cc ?
4 years, 3 months ago (2016-09-02 04:43:31 UTC) #160
Dan Beam
lgtm
4 years, 3 months ago (2016-09-02 22:38:23 UTC) #166
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2262653003/570001
4 years, 3 months ago (2016-09-05 04:46:11 UTC) #170
commit-bot: I haz the power
Committed patchset #10 (id:570001)
4 years, 3 months ago (2016-09-05 05:55:46 UTC) #172
commit-bot: I haz the power
Patchset 10 (id:??) landed as https://crrev.com/0f4aa14df5c5889c37fceb98e234676913822d36 Cr-Commit-Position: refs/heads/master@{#416500}
4 years, 3 months ago (2016-09-05 05:57:17 UTC) #174
battre
https://codereview.chromium.org/2262653003/diff/570001/net/base/layered_network_delegate.h File net/base/layered_network_delegate.h (right): https://codereview.chromium.org/2262653003/diff/570001/net/base/layered_network_delegate.h#newcode63 net/base/layered_network_delegate.h:63: void OnResponseStarted(URLRequest* request, int net_error) final; Shouldn't this be ...
4 years, 2 months ago (2016-09-29 13:07:29 UTC) #176
battre
4 years, 2 months ago (2016-09-29 13:13:50 UTC) #177
Message was sent while issue was closed.
On 2016/09/29 13:07:29, battre wrote:
>
https://codereview.chromium.org/2262653003/diff/570001/net/base/layered_netwo...
> File net/base/layered_network_delegate.h (right):
> 
>
https://codereview.chromium.org/2262653003/diff/570001/net/base/layered_netwo...
> net/base/layered_network_delegate.h:63: void OnResponseStarted(URLRequest*
> request, int net_error) final;
> Shouldn't this be "net::Error net_error" (also in all other cases)?

Ok...I saw that net::Error only covers internal errors. So probably not.

Powered by Google App Engine
This is Rietveld 408576698