Chromium Code Reviews
|
|
Chromium Code Reviews
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 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. |
DescriptionMake 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
Dependent Patchsets: Messages
Total messages: 177 (155 generated)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Patchset #6 (id:100001) has been deleted
Patchset #5 (id:80001) has been deleted
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #8 (id:200001) has been deleted
Patchset #7 (id:180001) has been deleted
Patchset #6 (id:160001) has been deleted
Patchset #5 (id:140001) has been deleted
Patchset #4 (id:120001) has been deleted
Patchset #2 (id:20001) has been deleted
Description was changed from ========== Make URLRequest::Read to return net erros or bytes read insteal of bool BUG= ========== to ========== Make URLRequest::Read to return net erros or bytes read insteal of bool. 1) OnResponseStarted and OnCompleted are overload for the transition period to easily commit all the changes. Some logic is applied in order to distinguish whether new delegate methods are overriden or not. 2) The set of these CL's will not remove URLRequestStatus completely, but I'm working towards this direction. Clients are modified here (not tested separately yet): https://codereview.chromium.org/2263673002/ https://codereview.chromium.org/2263673002/ https://codereview.chromium.org/2261103002/ https://codereview.chromium.org/2262113002/ https://codereview.chromium.org/2262093002/ https://codereview.chromium.org/2264903003/ https://codereview.chromium.org/2264973002/ https://codereview.chromium.org/2262063002/ https://codereview.chromium.org/2267643002/ https://codereview.chromium.org/2265873002/ https://codereview.chromium.org/2269523002/ https://codereview.chromium.org/2269523002/ All of them are dependent on this patch. Thus, there is no need to run different tests or commit together. BUG= ==========
maksim.sisov@intel.com changed reviewers: + mmenke@chromium.org
Hi Matt, Would you please take a first look into that? What do you think about this?
Description was changed from ========== Make URLRequest::Read to return net erros or bytes read insteal of bool. 1) OnResponseStarted and OnCompleted are overload for the transition period to easily commit all the changes. Some logic is applied in order to distinguish whether new delegate methods are overriden or not. 2) The set of these CL's will not remove URLRequestStatus completely, but I'm working towards this direction. Clients are modified here (not tested separately yet): https://codereview.chromium.org/2263673002/ https://codereview.chromium.org/2263673002/ https://codereview.chromium.org/2261103002/ https://codereview.chromium.org/2262113002/ https://codereview.chromium.org/2262093002/ https://codereview.chromium.org/2264903003/ https://codereview.chromium.org/2264973002/ https://codereview.chromium.org/2262063002/ https://codereview.chromium.org/2267643002/ https://codereview.chromium.org/2265873002/ https://codereview.chromium.org/2269523002/ https://codereview.chromium.org/2269523002/ All of them are dependent on this patch. Thus, there is no need to run different tests or commit together. BUG= ========== to ========== Make URLRequest::Read to return net erros or bytes read insteal of bool. 1) OnResponseStarted, Read and OnCompleted are overloaded for the transition period to easily commit all the changes. Some logic is applied in order to distinguish whether new delegate methods are overriden or not. 2) The set of these CL's will not remove URLRequestStatus completely, but I'm working towards this direction. Clients are modified here (not tested separately yet): https://codereview.chromium.org/2263673002/ https://codereview.chromium.org/2263673002/ https://codereview.chromium.org/2261103002/ https://codereview.chromium.org/2262113002/ https://codereview.chromium.org/2262093002/ https://codereview.chromium.org/2264903003/ https://codereview.chromium.org/2264973002/ https://codereview.chromium.org/2262063002/ https://codereview.chromium.org/2267643002/ https://codereview.chromium.org/2265873002/ https://codereview.chromium.org/2269523002/ https://codereview.chromium.org/2269523002/ All of them are dependent on this patch. Thus, there is no need to run different tests or commit together. BUG= ==========
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Make URLRequest::Read to return net erros or bytes read insteal of bool. 1) OnResponseStarted, Read and OnCompleted are overloaded for the transition period to easily commit all the changes. Some logic is applied in order to distinguish whether new delegate methods are overriden or not. 2) The set of these CL's will not remove URLRequestStatus completely, but I'm working towards this direction. Clients are modified here (not tested separately yet): https://codereview.chromium.org/2263673002/ https://codereview.chromium.org/2263673002/ https://codereview.chromium.org/2261103002/ https://codereview.chromium.org/2262113002/ https://codereview.chromium.org/2262093002/ https://codereview.chromium.org/2264903003/ https://codereview.chromium.org/2264973002/ https://codereview.chromium.org/2262063002/ https://codereview.chromium.org/2267643002/ https://codereview.chromium.org/2265873002/ https://codereview.chromium.org/2269523002/ https://codereview.chromium.org/2269523002/ All of them are dependent on this patch. Thus, there is no need to run different tests or commit together. BUG= ========== to ========== Make URLRequest::Read to return net erros or bytes read insteal of bool. 1) OnResponseStarted, Read and OnCompleted are overloaded for the transition period to easily commit all the changes. Some logic is applied in order to distinguish whether new delegate methods are overriden or not. 2) The set of these CL's will not remove URLRequestStatus completely, but I'm working towards this direction. Clients are modified here (not tested separately yet): https://codereview.chromium.org/2265873002/ <-- net/ https://codereview.chromium.org/2263673002/ https://codereview.chromium.org/2263673002/ https://codereview.chromium.org/2261103002/ https://codereview.chromium.org/2262113002/ https://codereview.chromium.org/2262093002/ 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/ https://codereview.chromium.org/2269523002/ All of them are dependent on this patch. Thus, there is no need to run different tests or commit together. BUG=423484 ==========
On 2016/08/24 12:39:36, maksims wrote: > Hi Matt, > > Would you please take a first look into that? What do you think about this? Apologies in advance, not sure when I'll get to this, working on figuring out a dev blocker. M54 branch is soon, and want to try and fix it before then.
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Make URLRequest::Read to return net erros or bytes read insteal of bool. 1) OnResponseStarted, Read and OnCompleted are overloaded for the transition period to easily commit all the changes. Some logic is applied in order to distinguish whether new delegate methods are overriden or not. 2) The set of these CL's will not remove URLRequestStatus completely, but I'm working towards this direction. Clients are modified here (not tested separately yet): https://codereview.chromium.org/2265873002/ <-- net/ https://codereview.chromium.org/2263673002/ https://codereview.chromium.org/2263673002/ https://codereview.chromium.org/2261103002/ https://codereview.chromium.org/2262113002/ https://codereview.chromium.org/2262093002/ 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/ https://codereview.chromium.org/2269523002/ All of them are dependent on this patch. Thus, there is no need to run different tests or commit together. BUG=423484 ========== to ========== Make URLRequest::Read to return net erros or bytes read insteal of bool. 1) OnResponseStarted, Read and OnCompleted are overloaded for the transition period to easily commit all the changes. Some logic is applied in order to distinguish whether new delegate methods are overriden or not. 2) The set of these CL's will not remove URLRequestStatus completely, but I'm working towards this direction. Clients are modified here (not tested separately yet): https://codereview.chromium.org/2265873002/ <-- net/ https://codereview.chromium.org/2263673002/ <-- aw/ https://codereview.chromium.org/2263673002/ https://codereview.chromium.org/2261103002/ https://codereview.chromium.org/2262113002/ https://codereview.chromium.org/2262093002/ 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/ https://codereview.chromium.org/2269523002/ All of them are dependent on this patch. Thus, there is no need to run different tests or commit together. BUG=423484 ==========
Description was changed from ========== Make URLRequest::Read to return net erros or bytes read insteal of bool. 1) OnResponseStarted, Read and OnCompleted are overloaded for the transition period to easily commit all the changes. Some logic is applied in order to distinguish whether new delegate methods are overriden or not. 2) The set of these CL's will not remove URLRequestStatus completely, but I'm working towards this direction. Clients are modified here (not tested separately yet): https://codereview.chromium.org/2265873002/ <-- net/ https://codereview.chromium.org/2263673002/ <-- aw/ https://codereview.chromium.org/2263673002/ https://codereview.chromium.org/2261103002/ https://codereview.chromium.org/2262113002/ https://codereview.chromium.org/2262093002/ 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/ https://codereview.chromium.org/2269523002/ All of them are dependent on this patch. Thus, there is no need to run different tests or commit together. BUG=423484 ========== to ========== Make URLRequest::Read to return net erros or bytes read insteal of bool. 1) OnResponseStarted, Read and OnCompleted are overloaded for the transition period to easily commit all the changes. Some logic is applied in order to distinguish whether new delegate methods are overriden or not. 2) The set of these CL's will not remove URLRequestStatus completely, but I'm working towards this direction. Clients are modified here (not tested separately yet): https://codereview.chromium.org/2265873002/ <-- net/ https://codereview.chromium.org/2262093002/ <-- content+ aw/ https://codereview.chromium.org/2263673002/ https://codereview.chromium.org/2261103002/ https://codereview.chromium.org/2262113002/ 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/ https://codereview.chromium.org/2269523002/ All of them are dependent on this patch. Thus, there is no need to run different tests or commit together. BUG=423484 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Patchset #4 (id:240001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #3 (id:220001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:1) has been deleted
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:260001) has been deleted
Description was changed from ========== Make URLRequest::Read to return net erros or bytes read insteal of bool. 1) OnResponseStarted, Read and OnCompleted are overloaded for the transition period to easily commit all the changes. Some logic is applied in order to distinguish whether new delegate methods are overriden or not. 2) The set of these CL's will not remove URLRequestStatus completely, but I'm working towards this direction. Clients are modified here (not tested separately yet): https://codereview.chromium.org/2265873002/ <-- net/ https://codereview.chromium.org/2262093002/ <-- content+ aw/ https://codereview.chromium.org/2263673002/ https://codereview.chromium.org/2261103002/ https://codereview.chromium.org/2262113002/ 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/ https://codereview.chromium.org/2269523002/ All of them are dependent on this patch. Thus, there is no need to run different tests or commit together. BUG=423484 ========== to ========== Make URLRequest::Read to return net erros or bytes read insteal of bool. 1) OnResponseStarted, Read and OnCompleted are overloaded for the transition period to easily commit all the changes. Some logic is applied in order to distinguish whether new delegate methods are overriden or not. 2) The set of these CL's will not remove URLRequestStatus completely, but I'm working towards this direction. Clients are modified here (not tested separately yet): https://codereview.chromium.org/2265873002/ <-- net/ https://codereview.chromium.org/2262093002/ <-- content+ aw/ https://codereview.chromium.org/2263673002/ https://codereview.chromium.org/2261103002/ https://codereview.chromium.org/2262113002/ 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/ <-- storage/ https://codereview.chromium.org/2269523002/ All of them are dependent on this patch. Thus, there is no need to run different tests or commit together. BUG=423484 ==========
Description was changed from ========== Make URLRequest::Read to return net erros or bytes read insteal of bool. 1) OnResponseStarted, Read and OnCompleted are overloaded for the transition period to easily commit all the changes. Some logic is applied in order to distinguish whether new delegate methods are overriden or not. 2) The set of these CL's will not remove URLRequestStatus completely, but I'm working towards this direction. Clients are modified here (not tested separately yet): https://codereview.chromium.org/2265873002/ <-- net/ https://codereview.chromium.org/2262093002/ <-- content+ aw/ https://codereview.chromium.org/2263673002/ https://codereview.chromium.org/2261103002/ https://codereview.chromium.org/2262113002/ 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/ <-- storage/ https://codereview.chromium.org/2269523002/ All of them are dependent on this patch. Thus, there is no need to run different tests or commit together. BUG=423484 ========== to ========== Make URLRequest::Read to return net erros or bytes read insteal of bool. 1) OnResponseStarted, Read and OnCompleted are overloaded for the transition period to easily commit all the changes. Some logic is applied in order to distinguish whether new delegate methods are overriden or not. 2) The set of these CL's will not remove URLRequestStatus completely, but I'm working towards this direction. Clients are modified here (not tested separately yet): https://codereview.chromium.org/2265873002/ <-- net/ https://codereview.chromium.org/2262093002/ <-- content+ aw/ https://codereview.chromium.org/2269523002/ <-- storage/ https://codereview.chromium.org/2263673002/ https://codereview.chromium.org/2261103002/ https://codereview.chromium.org/2262113002/ 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/ All of them are dependent on this patch. Thus, there is no need to run different tests or commit together. BUG=423484 ==========
Description was changed from ========== Make URLRequest::Read to return net erros or bytes read insteal of bool. 1) OnResponseStarted, Read and OnCompleted are overloaded for the transition period to easily commit all the changes. Some logic is applied in order to distinguish whether new delegate methods are overriden or not. 2) The set of these CL's will not remove URLRequestStatus completely, but I'm working towards this direction. Clients are modified here (not tested separately yet): https://codereview.chromium.org/2265873002/ <-- net/ https://codereview.chromium.org/2262093002/ <-- content+ aw/ https://codereview.chromium.org/2269523002/ <-- storage/ https://codereview.chromium.org/2263673002/ https://codereview.chromium.org/2261103002/ https://codereview.chromium.org/2262113002/ 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/ All of them are dependent on this patch. Thus, there is no need to run different tests or commit together. BUG=423484 ========== to ========== Make URLRequest::Read to return net erros or bytes read insteal of bool. 1) OnResponseStarted, Read and OnCompleted are overloaded for the transition period to easily commit all the changes. Some logic is applied in order to distinguish whether new delegate methods are overriden or not. 2) The set of these CL's will not remove URLRequestStatus completely, but I'm working towards this direction. Clients are modified here (not tested separately yet): https://codereview.chromium.org/2265873002/ <-- net/ https://codereview.chromium.org/2262093002/ <-- content+ aw/ https://codereview.chromium.org/2269523002/ <-- storage/ https://codereview.chromium.org/2261103002/ <-- components/ https://codereview.chromium.org/2262113002/ 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/ All of them are dependent on this patch. Thus, there is no need to run different tests or commit together. BUG=423484 ==========
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Make URLRequest::Read to return net erros or bytes read insteal of bool. 1) OnResponseStarted, Read and OnCompleted are overloaded for the transition period to easily commit all the changes. Some logic is applied in order to distinguish whether new delegate methods are overriden or not. 2) The set of these CL's will not remove URLRequestStatus completely, but I'm working towards this direction. Clients are modified here (not tested separately yet): https://codereview.chromium.org/2265873002/ <-- net/ https://codereview.chromium.org/2262093002/ <-- content+ aw/ https://codereview.chromium.org/2269523002/ <-- storage/ https://codereview.chromium.org/2261103002/ <-- components/ https://codereview.chromium.org/2262113002/ 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/ All of them are dependent on this patch. Thus, there is no need to run different tests or commit together. BUG=423484 ========== to ========== Make URLRequest::Read to return net erros or bytes read insteal of bool. 1) OnResponseStarted, Read and OnCompleted are overloaded for the transition period to easily commit all the changes. Some logic is applied in order to distinguish whether new delegate methods are overriden or not. 2) The set of these CL's will not remove URLRequestStatus completely, but I'm working towards this direction. Clients are modified here (not tested separately yet): https://codereview.chromium.org/2265873002/ <-- net/ https://codereview.chromium.org/2262093002/ <-- content+ aw/ https://codereview.chromium.org/2269523002/ <-- storage/ https://codereview.chromium.org/2261103002/ <-- components/ 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/ All of them are dependent on this patch. Thus, there is no need to run different tests or commit together. BUG=423484 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by maksim.sisov@intel.com
Patchset #3 (id:320001) has been deleted
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:300001) has been deleted
Patchset #1 (id:280001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/24 14:52:39, mmenke (busy) wrote: > On 2016/08/24 12:39:36, maksims wrote: > > Hi Matt, > > > > Would you please take a first look into that? What do you think about this? > > Apologies in advance, not sure when I'll get to this, working on figuring out a > dev blocker. M54 branch is soon, and want to try and fix it before then. Hi, please review when you have a time. I need your piece of advice on this. Is it right to preserve both bool and int Read and other delegate methods for a short while or there is another way of doing this? Having two OnResponseStarted methods, for example, bring some troubles as long as Delegate can be killed by a client after OnResponseStarted is completed. You can see tests with all the changes together here - https://codereview.chromium.org/2265873002/ . It is clearly seen in asan tests that heap is used after it was freed. That is what I am talking about. Your suggestions? Or should I just remove old methods and commit everything at once?
On 2016/08/26 10:10:16, maksims wrote: > On 2016/08/24 14:52:39, mmenke (busy) wrote: > > On 2016/08/24 12:39:36, maksims wrote: > > > Hi Matt, > > > > > > Would you please take a first look into that? What do you think about this? > > > > Apologies in advance, not sure when I'll get to this, working on figuring out > a > > dev blocker. M54 branch is soon, and want to try and fix it before then. > > Hi, please review when you have a time. I need your piece of advice on this. Is > it right to preserve both bool and int Read and other delegate methods for a > short while or there is another way of doing this? Having two OnResponseStarted > methods, for example, bring some troubles as long as Delegate can be killed by a > client after OnResponseStarted is completed. You can see tests with all the > changes together here - https://codereview.chromium.org/2265873002/ . > It is clearly seen in asan tests that heap is used after it was freed. That is > what I am talking about. > > Your suggestions? Or should I just remove old methods and commit everything at > once? I think it's reasonable to keep old methods around as wrappers to the new ones, until we're ready to fully remove them. I'll take a look at the failures, and (hopefully) review this CL later today.
Description was changed from ========== Make URLRequest::Read to return net erros or bytes read insteal of bool. 1) OnResponseStarted, Read and OnCompleted are overloaded for the transition period to easily commit all the changes. Some logic is applied in order to distinguish whether new delegate methods are overriden or not. 2) The set of these CL's will not remove URLRequestStatus completely, but I'm working towards this direction. Clients are modified here (not tested separately yet): https://codereview.chromium.org/2265873002/ <-- net/ https://codereview.chromium.org/2262093002/ <-- content+ aw/ https://codereview.chromium.org/2269523002/ <-- storage/ https://codereview.chromium.org/2261103002/ <-- components/ 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/ All of them are dependent on this patch. Thus, there is no need to run different tests or commit together. BUG=423484 ========== to ========== Make URLRequest::Read to return net errors or bytes read instead of a bool. 1) OnResponseStarted, Read and OnCompleted are overloaded for the transition period to easily commit all the changes. Some logic is applied in order to distinguish whether new delegate methods are overridden or not. 2) The set of these CL's will not remove URLRequestStatus completely, but I'm working towards this direction. Clients are modified here (not tested separately yet): https://codereview.chromium.org/2265873002/ <-- net/ https://codereview.chromium.org/2262093002/ <-- content+ aw/ https://codereview.chromium.org/2269523002/ <-- storage/ https://codereview.chromium.org/2261103002/ <-- components/ 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/ All of them are dependent on this patch. Thus, there is no need to run different tests or commit together. BUG=423484 ==========
Wow, looks like you've done a lot of work here, skimming over your followup CLs. I'll start reviewing them on Monday. https://codereview.chromium.org/2262653003/diff/410001/net/url_request/url_re... File net/url_request/url_request.cc (right): https://codereview.chromium.org/2262653003/diff/410001/net/url_request/url_re... net/url_request/url_request.cc:169: if (!implemented_) The issue with the failures is that the OnResponseStarted call may result in the URLRequest being deleted, so this will access freed memory. The way to do this is to get rid of this method, and remove implemented_. Instead, make the default OnResponseStarted(URLRequest* request, int net_error) implementation call OnResponseStarted(URLRequest* request), and make the default OnResponseStarted(URLRequest* request) just contain a "NOTREACHED", to prevent people from relying on both default implementations to do weird things. https://codereview.chromium.org/2262653003/diff/410001/net/url_request/url_re... net/url_request/url_request.cc:760: if (!status_.is_success()) { Can this happen, if job_->is_done() is false? (Also, if so, should remove the braces here) https://codereview.chromium.org/2262653003/diff/410001/net/url_request/url_re... net/url_request/url_request.cc:780: DCHECK(!(rv < 0) || status_.status() != URLRequestStatus::SUCCESS); Think rv >= 0 is easier to read, for the first check. https://codereview.chromium.org/2262653003/diff/410001/net/url_request/url_re... net/url_request/url_request.cc:790: DCHECK(bytes_read); Isn't everything below this the same as: int result = Read(dest, dest_size); if (result == ERR_IO_PENDING) { // set *bytes_read to 0, return 0. } if (result < ERR_IO_PENDING) { // set *bytes_read to 0, return -1. } etc. Then you can get rid of the old URLRequestJob::Read, and don't need duplicated code here. That also gets us more testing of the new URLRequest[Job]::Read methods. https://codereview.chromium.org/2262653003/diff/410001/net/url_request/url_re... File net/url_request/url_request.h (right): https://codereview.chromium.org/2262653003/diff/410001/net/url_request/url_re... net/url_request/url_request.h:559: int CancelWithError(int error); Can we just make these void? Some things that cancel will not know the final error code, in that case, but does that cause any big problems? https://codereview.chromium.org/2262653003/diff/410001/net/url_request/url_re... net/url_request/url_request.h:569: // If data is available, length and the data will be returned immediately. I'd mention net error on failure up here instead, and remove the comment about an async failure. https://codereview.chromium.org/2262653003/diff/410001/net/url_request/url_re... net/url_request/url_request.h:571: // asynchronous Read is initiated.The Read is finished when the caller Space after period. https://codereview.chromium.org/2262653003/diff/410001/net/url_request/url_re... net/url_request/url_request.h:575: // the stream. Suggest removing the last sentence, and just modifying the above the sentence to be "Unless the request was cancelled, OnReadComplete will always be called on async completion with the result of the read." Then you don't need to duplicate the description of the return value for the sync/async cases. https://codereview.chromium.org/2262653003/diff/410001/net/url_request/url_re... net/url_request/url_request.h:578: // returned immediately, otherwise OnReadComplete callback will be called with otherwise -> If an error occurs asynchronously, https://codereview.chromium.org/2262653003/diff/410001/net/url_request/url_re... net/url_request/url_request.h:579: // |net_error| set to an actual error. "|net_error| set to an actual error" -> "the net error code instead." https://codereview.chromium.org/2262653003/diff/410001/net/url_request/url_re... net/url_request/url_request.h:664: const URLRequestStatus& status() const { return status_; } Note that we could make this private, and friend current consumers. Since it looks like you're going to fix callers pretty quickly, I don't think that's necessary, but it is an option. https://codereview.chromium.org/2262653003/diff/410001/net/url_request/url_re... net/url_request/url_request.h:689: // Temporarily for testing purposes. Suggest this: // For testing purposes. // TODO(maksim): Remove this. https://codereview.chromium.org/2262653003/diff/410001/net/url_request/url_re... File net/url_request/url_request_test_util.cc (right): https://codereview.chromium.org/2262653003/diff/410001/net/url_request/url_re... net/url_request/url_request_test_util.cc:255: old_ = true; Rather than having two paths, can we just make the old OnResponseStarted call into the new one? Same for OnReadCompleted.
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2262653003/diff/410001/net/url_request/url_re... File net/url_request/url_request.cc (right): https://codereview.chromium.org/2262653003/diff/410001/net/url_request/url_re... net/url_request/url_request.cc:169: if (!implemented_) On 2016/08/26 19:36:48, mmenke wrote: > The issue with the failures is that the OnResponseStarted call may result in the > URLRequest being deleted, so this will access freed memory. > > The way to do this is to get rid of this method, and remove implemented_. > > Instead, make the default OnResponseStarted(URLRequest* request, int net_error) > implementation call OnResponseStarted(URLRequest* request), and make the default > OnResponseStarted(URLRequest* request) just contain a "NOTREACHED", to prevent > people from relying on both default implementations to do weird things. Oh, thanks! Good to know this trick! https://codereview.chromium.org/2262653003/diff/410001/net/url_request/url_re... net/url_request/url_request.cc:760: if (!status_.is_success()) { On 2016/08/26 19:36:48, mmenke wrote: > Can this happen, if job_->is_done() is false? > > (Also, if so, should remove the braces here) Yes, if URLRequestJob::NotifyStartError() is called. https://codereview.chromium.org/2262653003/diff/410001/net/url_request/url_re... net/url_request/url_request.cc:780: DCHECK(!(rv < 0) || status_.status() != URLRequestStatus::SUCCESS); On 2016/08/26 19:36:48, mmenke wrote: > Think rv >= 0 is easier to read, for the first check. Done. https://codereview.chromium.org/2262653003/diff/410001/net/url_request/url_re... net/url_request/url_request.cc:790: DCHECK(bytes_read); On 2016/08/26 19:36:48, mmenke wrote: > Isn't everything below this the same as: > > int result = Read(dest, dest_size); > if (result == ERR_IO_PENDING) { > // set *bytes_read to 0, return 0. > } > if (result < ERR_IO_PENDING) { > // set *bytes_read to 0, return -1. > } > > etc. > > Then you can get rid of the old URLRequestJob::Read, and don't need duplicated > code here. That also gets us more testing of the new URLRequest[Job]::Read > methods. Right! Thanks! https://codereview.chromium.org/2262653003/diff/410001/net/url_request/url_re... File net/url_request/url_request.h (right): https://codereview.chromium.org/2262653003/diff/410001/net/url_request/url_re... net/url_request/url_request.h:559: int CancelWithError(int error); On 2016/08/26 19:36:48, mmenke wrote: > Can we just make these void? Some things that cancel will not know the final > error code, in that case, but does that cause any big problems? Well, It might, but I'm not 100% sure.. I just want to be sure that a request is really canceled with an error which is provided by a client. But it's not really necessary, I guess. From the other hand, clients used to call Cancel() and then check the status with request_->Status(). For example, URLDownloader calls CancelWithError() (https://cs.chromium.org/chromium/src/content/browser/download/url_downloader....) and then calls ResponseCompleted() that checks the status with request_->status(). If it is assumed, the error, which is sent by the client, is always set or doesn't cause any troubles, then I'm ok with reverting back to void. But I'm suspicious it will cause troubles and bugs in the future. As an alternative, it is possible to set the error specified without checking if an error has already been set. https://codereview.chromium.org/2262653003/diff/410001/net/url_request/url_re... net/url_request/url_request.h:569: // If data is available, length and the data will be returned immediately. On 2016/08/26 19:36:48, mmenke wrote: > I'd mention net error on failure up here instead, and remove the comment about > an async failure. I didn't get it. https://codereview.chromium.org/2262653003/diff/410001/net/url_request/url_re... net/url_request/url_request.h:571: // asynchronous Read is initiated.The Read is finished when the caller On 2016/08/26 19:36:48, mmenke wrote: > Space after period. Done. https://codereview.chromium.org/2262653003/diff/410001/net/url_request/url_re... net/url_request/url_request.h:575: // the stream. On 2016/08/26 19:36:48, mmenke wrote: > Suggest removing the last sentence, and just modifying the above the sentence to > be "Unless the request was cancelled, OnReadComplete will always be called on > async completion with the result of the read." > > Then you don't need to duplicate the description of the return value for the > sync/async cases. Done. https://codereview.chromium.org/2262653003/diff/410001/net/url_request/url_re... net/url_request/url_request.h:578: // returned immediately, otherwise OnReadComplete callback will be called with On 2016/08/26 19:36:48, mmenke wrote: > otherwise -> If an error occurs asynchronously, Done. https://codereview.chromium.org/2262653003/diff/410001/net/url_request/url_re... net/url_request/url_request.h:579: // |net_error| set to an actual error. On 2016/08/26 19:36:48, mmenke wrote: > "|net_error| set to an actual error" -> "the net error code instead." Done. https://codereview.chromium.org/2262653003/diff/410001/net/url_request/url_re... net/url_request/url_request.h:664: const URLRequestStatus& status() const { return status_; } On 2016/08/26 19:36:48, mmenke wrote: > Note that we could make this private, and friend current consumers. Since it > looks like you're going to fix callers pretty quickly, I don't think that's > necessary, but it is an option. I'll consider this as well. https://codereview.chromium.org/2262653003/diff/410001/net/url_request/url_re... net/url_request/url_request.h:689: // Temporarily for testing purposes. On 2016/08/26 19:36:48, mmenke wrote: > Suggest this: > > // For testing purposes. > // TODO(maksim): Remove this. Done. https://codereview.chromium.org/2262653003/diff/410001/net/url_request/url_re... File net/url_request/url_request_test_util.cc (right): https://codereview.chromium.org/2262653003/diff/410001/net/url_request/url_re... net/url_request/url_request_test_util.cc:255: old_ = true; On 2016/08/26 19:36:48, mmenke wrote: > Rather than having two paths, can we just make the old OnResponseStarted call > into the new one? Same for OnReadCompleted. Done.
gentle reminder
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #7 (id:450001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/30 05:53:55, maksims wrote: > gentle reminder Sorry for slowness, there's a lot of weirdness here (Note: Not your fault, just questions of how to cleanly pass around the status, and how to cleanly cancel and then pass around status). I'll post some comments today, but still thinking about some of this stuff.
Sorry, earlier comment was about your *other* CL. I'm really happy with this one, now. https://codereview.chromium.org/2262653003/diff/470001/net/base/network_deleg... File net/base/network_delegate.cc (right): https://codereview.chromium.org/2262653003/diff/470001/net/base/network_deleg... net/base/network_delegate.cc:96: } Order in this file should match order in the header file. https://codereview.chromium.org/2262653003/diff/470001/net/base/network_deleg... net/base/network_delegate.cc:144: } Order in this file should match order in the header file. https://codereview.chromium.org/2262653003/diff/470001/net/base/network_deleg... File net/base/network_delegate.h (right): https://codereview.chromium.org/2262653003/diff/470001/net/base/network_deleg... net/base/network_delegate.h:308: bool implemented_; No longer used https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... File net/url_request/url_request.cc (right): https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... net/url_request/url_request.cc:509: return; Why is this needed now, and wasn't before? https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... net/url_request/url_request.cc:737: // about being called recursively. nit: Blank line below comment, since this isn't a comment about the return statement. https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... net/url_request/url_request.cc:749: // to indicate end of stream. Could you fix this comment? The old comment was incorrect before, but now it's even more incorrect. Now we're returning the error again after failure, instead of a 0-byte sync success. https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... net/url_request/url_request.cc:753: // This handles a cancel that happens while paused. By moving the status_.is_success() check above this one, this no longer handles this case. This now only handles reads after the request already completed successfully. https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... net/url_request/url_request.cc:1159: // here. nit: Add TODO to make NotifyReadCompleted return the error code on failure. https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... File net/url_request/url_request.h (right): https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... net/url_request/url_request.h:223: void NotifyOnResponseStarted(URLRequest* request, int net_error); This method doesn't exist any more. https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... net/url_request/url_request.h:230: bool implemented_; Not used. https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... net/url_request/url_request.h:558: // Cancels the request and sets the error to |error| (see net_error_list.h Let's improve this comment. How about "Cancels the request and sets the error to |error|, unless the request already failed with another error code (see net_error_list.h)." https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... net/url_request/url_request.h:559: // for values). Returns set error. "Returns set error." -> "Returns final network error code." https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... net/url_request/url_request.h:579: // callback will be called with the net error code instead. Let's merge these two paragraphs. Suggested text: Read initiates an asynchronous read from the response, and must only be called after the OnResponseStarted callback is received with a net::OK. If data is available, length and the data will be returned immediately. If the request has failed, an error code will be returned. If data is not yet available, Read returns net::ERR_IO_PENDING, and the Delegate's OnReadComplete method will be called asynchronously with the result of the read, unless the URLRequest is canceled. https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... File net/url_request/url_request_job.cc (right): https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... net/url_request/url_request_job.cc:198: return bytes_read; Suggest re-ordering these and using early returns. Think separate cases are easier to follow, and now don't need separate logic to set bytes_read and handle the return value. Something like: if (error == ERR_IO_PENDING) return ERR_IO_PENDING; if (error < 0) { ... return error; } if (bytes_read == 0) // Early return not needed here. https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... File net/url_request/url_request_test_util.cc (right): https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... net/url_request/url_request_test_util.cc:196: request_status_(1), Suggest ERR_IO_PENDING (It's the one status we should never see) https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... net/url_request/url_request_test_util.cc:257: } This method is no longer needed (Be sure to remove it from the header, too) https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... net/url_request/url_request_test_util.cc:515: } No longer needed. https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... net/url_request/url_request_test_util.cc:583: DCHECK_EQ(net_error, OK); Flip order - expected should be first value.
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Patchset #8 (id:490001) has been deleted
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2262653003/diff/470001/net/base/network_deleg... File net/base/network_delegate.cc (right): https://codereview.chromium.org/2262653003/diff/470001/net/base/network_deleg... net/base/network_delegate.cc:96: } On 2016/08/30 22:09:11, mmenke wrote: > Order in this file should match order in the header file. Done. https://codereview.chromium.org/2262653003/diff/470001/net/base/network_deleg... net/base/network_delegate.cc:144: } On 2016/08/30 22:09:11, mmenke wrote: > Order in this file should match order in the header file. Done. https://codereview.chromium.org/2262653003/diff/470001/net/base/network_deleg... File net/base/network_delegate.h (right): https://codereview.chromium.org/2262653003/diff/470001/net/base/network_deleg... net/base/network_delegate.h:308: bool implemented_; On 2016/08/30 22:09:11, mmenke wrote: > No longer used Hmm, interesting. I've seen build-bots has been failing recently when a variable is not used, but not this time. It's strange they didn't complain about that. https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... File net/url_request/url_request.cc (right): https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... net/url_request/url_request.cc:509: return; On 2016/08/30 22:09:11, mmenke wrote: > Why is this needed now, and wasn't before? Because of this - https://cs.chromium.org/chromium/src/content/browser/loader/resource_loader.c... Otherwise URLRequest::StartJob() will start and set status_ to ERR_IO_PENDING. https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... net/url_request/url_request.cc:737: // about being called recursively. On 2016/08/30 22:09:12, mmenke wrote: > nit: Blank line below comment, since this isn't a comment about the return > statement. Done. https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... net/url_request/url_request.cc:749: // to indicate end of stream. On 2016/08/30 22:09:12, mmenke wrote: > Could you fix this comment? The old comment was incorrect before, but now it's > even more incorrect. Now we're returning the error again after failure, instead > of a 0-byte sync success. Done. https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... net/url_request/url_request.cc:753: // This handles a cancel that happens while paused. On 2016/08/30 22:09:12, mmenke wrote: > By moving the status_.is_success() check above this one, this no longer handles > this case. This now only handles reads after the request already completed > successfully. Done. https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... net/url_request/url_request.cc:1159: // here. On 2016/08/30 22:09:12, mmenke wrote: > nit: Add TODO to make NotifyReadCompleted return the error code on failure. It returns errors by calling OnReadCompleted with |bytes_read| set to the actual network error code. Or do you mean to change void to int in order to tell URLRequestJob about the error instead of having it checking status()??? https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... File net/url_request/url_request.h (right): https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... net/url_request/url_request.h:223: void NotifyOnResponseStarted(URLRequest* request, int net_error); On 2016/08/30 22:09:12, mmenke wrote: > This method doesn't exist any more. Done. https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... net/url_request/url_request.h:230: bool implemented_; On 2016/08/30 22:09:12, mmenke wrote: > Not used. Done. https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... net/url_request/url_request.h:558: // Cancels the request and sets the error to |error| (see net_error_list.h On 2016/08/30 22:09:12, mmenke wrote: > Let's improve this comment. How about "Cancels the request and sets the error > to |error|, unless the request already failed with another error code (see > net_error_list.h)." Sounds reasonable! https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... net/url_request/url_request.h:559: // for values). Returns set error. On 2016/08/30 22:09:12, mmenke wrote: > "Returns set error." -> "Returns final network error code." Done. https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... net/url_request/url_request.h:579: // callback will be called with the net error code instead. On 2016/08/30 22:09:12, mmenke wrote: > Let's merge these two paragraphs. Suggested text: > > Read initiates an asynchronous read from the response, and must only be called > after the OnResponseStarted callback is received with a net::OK. If data is > available, length and the data will be returned immediately. If the request has > failed, an error code will be returned. If data is not yet available, Read > returns net::ERR_IO_PENDING, and the Delegate's OnReadComplete method will be > called asynchronously with the result of the read, unless the URLRequest is > canceled. Done. https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... File net/url_request/url_request_job.cc (right): https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... net/url_request/url_request_job.cc:198: return bytes_read; On 2016/08/30 22:09:12, mmenke wrote: > Suggest re-ordering these and using early returns. Think separate cases are > easier to follow, and now don't need separate logic to set bytes_read and handle > the return value. > > Something like: > > if (error == ERR_IO_PENDING) > return ERR_IO_PENDING; > > if (error < 0) { > ... > return error; > } > > if (bytes_read == 0) > // Early return not needed here. Done. https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... File net/url_request/url_request_test_util.cc (right): https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... net/url_request/url_request_test_util.cc:196: request_status_(1), On 2016/08/30 22:09:12, mmenke wrote: > Suggest ERR_IO_PENDING (It's the one status we should never see) Done. https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... net/url_request/url_request_test_util.cc:257: } On 2016/08/30 22:09:12, mmenke wrote: > This method is no longer needed (Be sure to remove it from the header, too) No, it's needed. Callers are not modified yet. For example, embedder calls TestDelegate::OnReponseStarted(request); https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... net/url_request/url_request_test_util.cc:583: DCHECK_EQ(net_error, OK); On 2016/08/30 22:09:12, mmenke wrote: > Flip order - expected should be first value. Done.
https://codereview.chromium.org/2262653003/diff/470001/net/base/network_deleg... File net/base/network_delegate.h (right): https://codereview.chromium.org/2262653003/diff/470001/net/base/network_deleg... net/base/network_delegate.h:308: bool implemented_; On 2016/08/31 11:16:59, maksims wrote: > On 2016/08/30 22:09:11, mmenke wrote: > > No longer used > > Hmm, interesting. I've seen build-bots has been failing recently when a variable > is not used, but not this time. It's strange they didn't complain about that. The checks don't detect class members that aren't used, just globals and locals. https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... File net/url_request/url_request.cc (right): https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... net/url_request/url_request.cc:509: return; On 2016/08/31 11:16:59, maksims wrote: > On 2016/08/30 22:09:11, mmenke wrote: > > Why is this needed now, and wasn't before? > > Because of this - > https://cs.chromium.org/chromium/src/content/browser/loader/resource_loader.c... > > Otherwise URLRequest::StartJob() will start and set status_ to ERR_IO_PENDING. I'm not following. Is something actually calling Start() after cancellation? If so, I'm not sure this is the right fix, as it will look like the request is just hanging after the start call. I'm also not sure why this is needed in this CL, but wasn't needed before. https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... net/url_request/url_request.cc:1159: // here. On 2016/08/31 11:16:59, maksims wrote: > On 2016/08/30 22:09:12, mmenke wrote: > > nit: Add TODO to make NotifyReadCompleted return the error code on failure. > > It returns errors by calling OnReadCompleted with |bytes_read| set to the actual > network error code. Or do you mean to change void to int in order to tell > URLRequestJob about the error instead of having it checking status()??? Sorry, I meant make NotifyReadCompleted take the error code as an argument on failure, rather than -1. I want to have this method be the one to set the status to error, eventually, rather than have it set before this method is invoked. How to do that? I'd like to move most of the URLRequest::NotifyDone logic to this class, eventually, and then this will call NotifyDone, on error. That slightly reduces the size of the URLRequest/URLRequestJob API, and makes the URLRequest more responsible for how it calls its consumer. I'd also like to move the callback logic from URLRequestJob::Cancel into URLRequest, other than the Kill() call at the same time...Again, just making the URLRequest responsible for implementing its own API, and decreasing the coupling between these two classes. https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... File net/url_request/url_request_test_util.cc (right): https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... net/url_request/url_request_test_util.cc:257: } On 2016/08/31 11:16:59, maksims wrote: > On 2016/08/30 22:09:12, mmenke wrote: > > This method is no longer needed (Be sure to remove it from the header, too) > > No, it's needed. Callers are not modified yet. For example, embedder calls > TestDelegate::OnReponseStarted(request); So...You're right about that, but I don't see how those tests are passing. Those tests all subclass TestDelegate, and TestDelegate overrides the new OnResponseStarted, which normally calls the old one, but TestDelegate's version of it does not. So how are any of those methods that call TestDelegate::OnReponseStarted being invoked? I'm confused.
https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... File net/url_request/url_request_test_util.cc (right): https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... net/url_request/url_request_test_util.cc:257: } On 2016/08/31 21:35:47, mmenke wrote: > On 2016/08/31 11:16:59, maksims wrote: > > On 2016/08/30 22:09:12, mmenke wrote: > > > This method is no longer needed (Be sure to remove it from the header, too) > > > > No, it's needed. Callers are not modified yet. For example, embedder calls > > TestDelegate::OnReponseStarted(request); > > So...You're right about that, but I don't see how those tests are passing. > Those tests all subclass TestDelegate, and TestDelegate overrides the new > OnResponseStarted, which normally calls the old one, but TestDelegate's version > of it does not. So how are any of those methods that call > TestDelegate::OnReponseStarted being invoked? I'm confused. Wait, aren't you updating all those callers, for just that reason? Are there any callers of TestDelegate::OnResponseStarted that you aren't already updating in this CL to use the new method?
maksim.sisov@intel.com changed reviewers: + brettw@chromium.org
https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... File net/url_request/url_request.cc (right): https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... net/url_request/url_request.cc:509: return; On 2016/08/31 21:35:47, mmenke wrote: > On 2016/08/31 11:16:59, maksims wrote: > > On 2016/08/30 22:09:11, mmenke wrote: > > > Why is this needed now, and wasn't before? > > > > Because of this - > > > https://cs.chromium.org/chromium/src/content/browser/loader/resource_loader.c... > > > > Otherwise URLRequest::StartJob() will start and set status_ to ERR_IO_PENDING. > > I'm not following. Is something actually calling Start() after cancellation? > If so, I'm not sure this is the right fix, as it will look like the request is > just hanging after the start call. > > I'm also not sure why this is needed in this CL, but wasn't needed before. yes, ResourceLoader has Resume() method which calls StartRequestInternal(). Right now it checks what status a request has and then decides to start or not start the request. It is tested in ResourceLoaderTest.ResumeCancelledRequest. It supposes that Start() doesn't trigger the URLRequestJob again if it has been cancelled. https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... net/url_request/url_request.cc:1159: // here. On 2016/08/31 21:35:47, mmenke wrote: > On 2016/08/31 11:16:59, maksims wrote: > > On 2016/08/30 22:09:12, mmenke wrote: > > > nit: Add TODO to make NotifyReadCompleted return the error code on failure. > > > > It returns errors by calling OnReadCompleted with |bytes_read| set to the > actual > > network error code. Or do you mean to change void to int in order to tell > > URLRequestJob about the error instead of having it checking status()??? > > Sorry, I meant make NotifyReadCompleted take the error code as an argument on > failure, rather than -1. I want to have this method be the one to set the > status to error, eventually, rather than have it set before this method is > invoked. > > How to do that? I'd like to move most of the URLRequest::NotifyDone logic to > this class, eventually, and then this will call NotifyDone, on error. That > slightly reduces the size of the URLRequest/URLRequestJob API, and makes the > URLRequest more responsible for how it calls its consumer. > > I'd also like to move the callback logic from URLRequestJob::Cancel into > URLRequest, other than the Kill() call at the same time...Again, just making the > URLRequest responsible for implementing its own API, and decreasing the coupling > between these two classes. Oh, I see. I'll add TODO and fix this after these CLs are committed. https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... File net/url_request/url_request_test_util.cc (right): https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... net/url_request/url_request_test_util.cc:257: } On 2016/08/31 21:39:27, mmenke wrote: > On 2016/08/31 21:35:47, mmenke wrote: > > On 2016/08/31 11:16:59, maksims wrote: > > > On 2016/08/30 22:09:12, mmenke wrote: > > > > This method is no longer needed (Be sure to remove it from the header, > too) > > > > > > No, it's needed. Callers are not modified yet. For example, embedder calls > > > TestDelegate::OnReponseStarted(request); > > > > So...You're right about that, but I don't see how those tests are passing. > > Those tests all subclass TestDelegate, and TestDelegate overrides the new > > OnResponseStarted, which normally calls the old one, but TestDelegate's > version > > of it does not. So how are any of those methods that call > > TestDelegate::OnReponseStarted being invoked? I'm confused. > > Wait, aren't you updating all those callers, for just that reason? Are there > any callers of TestDelegate::OnResponseStarted that you aren't already updating > in this CL to use the new method? Yes, I am. I'll add the embedded_test_server_unittest to this CL and remove old OnResponseStarted from Test(Network)Delegate (right now it's in another CL that is dependent on this one. That's why compilation fails here, but runs fine in the CL with net callers). https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... net/url_request/url_request_test_util.cc:515: } On 2016/08/30 22:09:12, mmenke wrote: > No longer needed. Done.
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Patchset #9 (id:530001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Did you add brettw as a reviewer for url_data_manager_backend_unittest.cc? He's a bad choice of reviewer for that file. You should use someone from content/browser/webui/OWNERS, instead of a top level content/ owner (Just consider how much stuff they own - they can't know all code in content/, and they get deluged with reviews. Should only use them as a last resort)
LGTM! https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... File net/url_request/url_request.cc (right): https://codereview.chromium.org/2262653003/diff/470001/net/url_request/url_re... net/url_request/url_request.cc:509: return; On 2016/09/01 07:13:17, maksims wrote: > On 2016/08/31 21:35:47, mmenke wrote: > > On 2016/08/31 11:16:59, maksims wrote: > > > On 2016/08/30 22:09:11, mmenke wrote: > > > > Why is this needed now, and wasn't before? > > > > > > Because of this - > > > > > > https://cs.chromium.org/chromium/src/content/browser/loader/resource_loader.c... > > > > > > Otherwise URLRequest::StartJob() will start and set status_ to > ERR_IO_PENDING. > > > > I'm not following. Is something actually calling Start() after cancellation? > > If so, I'm not sure this is the right fix, as it will look like the request is > > just hanging after the start call. > > > > I'm also not sure why this is needed in this CL, but wasn't needed before. > > yes, ResourceLoader has Resume() method which calls StartRequestInternal(). > Right now it checks what status a request has and then decides to start or not > start the request. It is tested in > ResourceLoaderTest.ResumeCancelledRequest. It supposes that Start() doesn't > trigger the URLRequestJob again if it has been cancelled. Thanks for the explanation! Seems like something else to change, in the future.
maksim.sisov@intel.com changed reviewers: + dbeam@chromium.org - brettw@chromium.org
dbeam@, would you please review url_data_manager_backend_unittest.cc ?
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Make URLRequest::Read to return net errors or bytes read instead of a bool. 1) OnResponseStarted, Read and OnCompleted are overloaded for the transition period to easily commit all the changes. Some logic is applied in order to distinguish whether new delegate methods are overridden or not. 2) The set of these CL's will not remove URLRequestStatus completely, but I'm working towards this direction. Clients are modified here (not tested separately yet): https://codereview.chromium.org/2265873002/ <-- net/ https://codereview.chromium.org/2262093002/ <-- content+ aw/ https://codereview.chromium.org/2269523002/ <-- storage/ https://codereview.chromium.org/2261103002/ <-- components/ 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/ All of them are dependent on this patch. Thus, there is no need to run different tests or commit together. BUG=423484 ========== to ========== Make URLRequest::Read to return net errors or bytes read instead of a bool. 1) OnResponseStarted, Read and OnCompleted are overloaded for the transition period to easily commit all the changes. Some logic is applied in order to distinguish whether new delegate methods are overridden or not. 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 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
Description was changed from ========== Make URLRequest::Read to return net errors or bytes read instead of a bool. 1) OnResponseStarted, Read and OnCompleted are overloaded for the transition period to easily commit all the changes. Some logic is applied in order to distinguish whether new delegate methods are overridden or not. 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 ========== to ========== 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 ==========
The CQ bit was checked by maksim.sisov@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2262653003/#ps570001 (title: "rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:570001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/0f4aa14df5c5889c37fceb98e234676913822d36 Cr-Commit-Position: refs/heads/master@{#416500}
Message was sent while issue was closed.
battre@chromium.org changed reviewers: + battre@chromium.org
Message was sent while issue was closed.
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)?
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. |
