Use modified URLRequest::Read() and delegate methods in components/

Use modified URLRequest::Read() and delegate methods in components/

This CL modifies clients that use URLRequest::Read() method
or implement net::Delegate/net::NetworkDelegate methods
and makes them to use returned networking errors instead,
which are returned in term of int.

Old implementation of URLRequest::Read() used to set its
status inside the URLRequest object itself and clients
used to check the status by calling request->status().

The main CL that changes URLRequest::Read and delegate
methods is here - https://codereview.chromium.org/2262653003

BUG=423484
Committed: https://crrev.com/1faf924e0dc99a797084918fc04bc6a688e2cd4c
Cr-Commit-Position: refs/heads/master@{#419413}

[maksims (do not use this acc), 2016-08-22 07:36:53]

Description was changed from

==========
djust callers and networking delegates in components/ to modified APIs

BUG=
==========

to

==========
Adjust callers and networking delegates in components/ to modified APIs

BUG=
==========

[maksims (do not use this acc), 2016-08-25 12:05:40]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 12:05:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 12:10:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-25 12:10:39]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[maksims (do not use this acc), 2016-08-25 12:21:31]

Patchset #1 (id:1) has been deleted

[maksims (do not use this acc), 2016-08-25 12:26:02]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[maksims (do not use this acc), 2016-08-25 12:26:15]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-08-25 12:26:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 12:36:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-25 12:36:16]

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...)

[maksims (do not use this acc), 2016-08-25 12:41:35]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 12:41:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 12:48:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-25 12:48:51]

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...)

[maksims (do not use this acc), 2016-09-05 11:19:19]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-05 11:19:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-05 11:27:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-05 11:27:07]

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_...)

[maksims (do not use this acc), 2016-09-05 13:03:53]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-05 13:04:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-05 13:12:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-05 13:12:40]

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_...)

[maksims (do not use this acc), 2016-09-06 05:06:25]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-06 05:06:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[maksims (do not use this acc), 2016-09-06 05:06:42]

Description was changed from

==========
Adjust callers and networking delegates in components/ to modified APIs

BUG=
==========

to

==========
Use modified URLRequest::Read() and delegate methods in components/

Use modified Read and delegate methods from the following 
CL - https://codereview.chromium.org/2262653003/

BUG=423484
==========

[maksims (do not use this acc), 2016-09-06 05:06:49]

Patchset #4 (id:100001) has been deleted

[maksims (do not use this acc), 2016-09-06 05:06:59]

Patchset #3 (id:80001) has been deleted

[maksims (do not use this acc), 2016-09-06 05:07:06]

Patchset #2 (id:60001) has been deleted

[maksims (do not use this acc), 2016-09-06 05:07:12]

Patchset #1 (id:40001) has been deleted

[commit-bot: I haz the power, 2016-09-06 05:15:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-06 05:15:34]

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...)

[maksims (do not use this acc), 2016-09-06 08:36:31]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-06 08:36:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-06 08:44:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-06 08:44:12]

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_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[maksims (do not use this acc), 2016-09-06 10:26:12]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-06 10:26:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-06 10:32:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-06 10:32:30]

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...)

[maksims (do not use this acc), 2016-09-06 10:50:20]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-06 10:50:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-06 11:32:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-06 11:32:07]

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...)

[maksims (do not use this acc), 2016-09-06 12:18:18]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-06 12:18:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[maksims (do not use this acc), 2016-09-06 12:24:14]

Patchset #4 (id:180001) has been deleted

[maksims (do not use this acc), 2016-09-06 12:24:24]

Patchset #3 (id:160001) has been deleted

[maksims (do not use this acc), 2016-09-06 12:24:32]

Patchset #2 (id:140001) has been deleted

[maksims (do not use this acc), 2016-09-06 12:24:40]

Patchset #1 (id:120001) has been deleted

[commit-bot: I haz the power, 2016-09-06 12:59:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-06 12:59:51]

Dry run: This issue passed the CQ dry run.

[maksims (do not use this acc), 2016-09-06 14:31:00]

PTAL

rsleevi@:
components/certificate_transparency/log_proof_fetcher.cc 

mmenke@:
components/cronet/android/*

bengr@:
components/data_reduction_proxy/core/*

davidben@:
components/domain_reliability/*

[maksims (do not use this acc), 2016-09-06 14:32:47]

maksim.sisov@intel.com changed reviewers:
+ bengr@chromium.org, davidben@chromium.org, mmenke@chromium.org,
rsleevi@chromium.org

[maksims (do not use this acc), 2016-09-06 14:33:06]

PTAL

rsleevi@:
components/certificate_transparency/log_proof_fetcher.cc 

mmenke@:
components/cronet/android/*

bengr@:
components/data_reduction_proxy/core/*

davidben@:
components/domain_reliability/*

[Ryan Sleevi, 2016-09-06 16:15:34]

Please update the commit description to better reflect what this code does.

A good primer on this is http://chris.beams.io/posts/git-commit/

[maksims (do not use this acc), 2016-09-06 16:35:14]

Description was changed from

==========
Use modified URLRequest::Read() and delegate methods in components/

Use modified Read and delegate methods from the following 
CL - https://codereview.chromium.org/2262653003/

BUG=423484
==========

to

==========
Use modified URLRequest::Read() and delegate methods in components/

This CL modifies clients that use URLRequest::Read() method
or implement net::Delegate/net::NetworkDelegate methods
and makes them to use returned networking errors instead,
which are returned in term of int.

Old implementation of URLRequest::Read() used to set its  
status inside the URLRequest object itself and clients
used to check the status by calling request->status().

The main CL that changes URLRequest::Read and delegate
methods is here - https://codereview.chromium.org/2262653003

BUG=423484
==========

[maksims (do not use this acc), 2016-09-06 16:35:25]

On 2016/09/06 16:15:34, Ryan Sleevi (slow) wrote:
> Please update the commit description to better reflect what this code does.
> 
> A good primer on this is http://chris.beams.io/posts/git-commit/

please check

[davidben, 2016-09-06 17:07:28]

https://codereview.chromium.org/2261103002/diff/200001/components/domain_reli...
File components/domain_reliability/monitor.cc (right):

https://codereview.chromium.org/2261103002/diff/200001/components/domain_reli...
components/domain_reliability/monitor.cc:173: // TODO(maksims): Make this take
net_error.
It does not appear this CL does not actually do anything to this directory
except lose the logic in URLRequestStatusToNetError. I think this TODO needs to
be resolved at the same time as converting this directory. So either remove the
changes to this directory and leave it for later or fully route the net_error
out of URLRequest correctly.

https://codereview.chromium.org/2261103002/diff/200001/components/domain_reli...
File components/domain_reliability/monitor.h (right):

https://codereview.chromium.org/2261103002/diff/200001/components/domain_reli...
components/domain_reliability/monitor.h:147: explicit RequestInfo(const
net::URLRequest& request, int net_error);
No need for 'explicit' with two arguments.

[mmenke, 2016-09-06 17:57:58]

Cronet LGTM

https://codereview.chromium.org/2261103002/diff/200001/components/cronet/andr...
File components/cronet/android/cronet_url_request_adapter.cc (right):

https://codereview.chromium.org/2261103002/diff/200001/components/cronet/andr...
components/cronet/android/cronet_url_request_adapter.cc:274:
ReportError(request, bytes_read);
Early return is generally preferred, and it's generally easiest to follow code
that handles errors before success.

https://codereview.chromium.org/2261103002/diff/200001/components/cronet/andr...
components/cronet/android/cronet_url_request_adapter.cc:354: if (bytes_read ==
net::ERR_IO_PENDING)
nit:  bytes_read -> result.

https://codereview.chromium.org/2261103002/diff/200001/components/cronet/andr...
components/cronet/android/cronet_url_request_adapter.cc:371:
DCHECK_NE(net::ERR_IO_PENDING, net_error);
Suggest also DCHECK_LT(new_error, 0);

https://codereview.chromium.org/2261103002/diff/200001/components/cronet/andr...
File components/cronet/android/url_request_adapter.cc (right):

https://codereview.chromium.org/2261103002/diff/200001/components/cronet/andr...
components/cronet/android/url_request_adapter.cc:244: int bytes_read =
url_request_->Read(read_buffer_.get(), kReadBufferSize);
nit:  bytes_read -> result

https://codereview.chromium.org/2261103002/diff/200001/components/cronet/andr...
components/cronet/android/url_request_adapter.cc:254: bool
URLRequestAdapter::HandleReadResult(int bytes_read) {
bytes_read -> result

https://codereview.chromium.org/2261103002/diff/200001/components/cronet/andr...
components/cronet/android/url_request_adapter.cc:260: } else if (bytes_read ==
0) {
pre-existing issue, but while you're here, mind removing this else?

https://codereview.chromium.org/2261103002/diff/200001/components/cronet/andr...
components/cronet/android/url_request_adapter.cc:306: void
URLRequestAdapter::OnRequestFailed(int net_error) {
Suggest adding:

DCHECK_LE(net_error, 0);
DCHECK_NE(ERR_IO_PENDING, net_error);

[Ryan Sleevi, 2016-09-07 18:48:08]

lgtm

https://codereview.chromium.org/2261103002/diff/200001/components/certificate...
File components/certificate_transparency/log_proof_fetcher.cc (right):

https://codereview.chromium.org/2261103002/diff/200001/components/certificate...
components/certificate_transparency/log_proof_fetcher.cc:149: // Check for an
error condition.
You can drop this line; it no longer grammatically fits with the rest of the
code (and probably shouldn't have been included on 137 to begin with)

[bengr, 2016-09-08 00:28:23]

components/data_reduction_proxy/* lgtm

[maksims (do not use this acc), 2016-09-12 05:58:57]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 05:59:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[maksims (do not use this acc), 2016-09-12 06:19:00]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 06:19:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-12 06:52:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-12 06:52:33]

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...)

[maksims (do not use this acc), 2016-09-12 07:17:57]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 07:18:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-12 08:06:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-12 08:06:41]

Dry run: This issue passed the CQ dry run.

[maksims (do not use this acc), 2016-09-12 12:11:54]

https://codereview.chromium.org/2261103002/diff/200001/components/cronet/andr...
File components/cronet/android/cronet_url_request_adapter.cc (right):

https://codereview.chromium.org/2261103002/diff/200001/components/cronet/andr...
components/cronet/android/cronet_url_request_adapter.cc:274:
ReportError(request, bytes_read);
On 2016/09/06 17:57:58, mmenke wrote:
> Early return is generally preferred, and it's generally easiest to follow code
> that handles errors before success.

Done.

https://codereview.chromium.org/2261103002/diff/200001/components/cronet/andr...
components/cronet/android/cronet_url_request_adapter.cc:354: if (bytes_read ==
net::ERR_IO_PENDING)
On 2016/09/06 17:57:58, mmenke wrote:
> nit:  bytes_read -> result.

Done.

https://codereview.chromium.org/2261103002/diff/200001/components/cronet/andr...
components/cronet/android/cronet_url_request_adapter.cc:371:
DCHECK_NE(net::ERR_IO_PENDING, net_error);
On 2016/09/06 17:57:58, mmenke wrote:
> Suggest also DCHECK_LT(new_error, 0);

Done.

https://codereview.chromium.org/2261103002/diff/200001/components/cronet/andr...
File components/cronet/android/url_request_adapter.cc (right):

https://codereview.chromium.org/2261103002/diff/200001/components/cronet/andr...
components/cronet/android/url_request_adapter.cc:260: } else if (bytes_read ==
0) {
On 2016/09/06 17:57:58, mmenke wrote:
> pre-existing issue, but while you're here, mind removing this else?
Do you mean to do this - 

if (result == 0) {
  ...
  return false;
}

??? Or to remove completely?

https://codereview.chromium.org/2261103002/diff/200001/components/cronet/andr...
components/cronet/android/url_request_adapter.cc:306: void
URLRequestAdapter::OnRequestFailed(int net_error) {
On 2016/09/06 17:57:58, mmenke wrote:
> Suggest adding:
> 
> DCHECK_LE(net_error, 0);
> DCHECK_NE(ERR_IO_PENDING, net_error);

Done.

https://codereview.chromium.org/2261103002/diff/200001/components/domain_reli...
File components/domain_reliability/monitor.cc (right):

https://codereview.chromium.org/2261103002/diff/200001/components/domain_reli...
components/domain_reliability/monitor.cc:173: // TODO(maksims): Make this take
net_error.
On 2016/09/06 17:07:28, davidben wrote:
> It does not appear this CL does not actually do anything to this directory
> except lose the logic in URLRequestStatusToNetError. I think this TODO needs
to
> be resolved at the same time as converting this directory. So either remove
the
> changes to this directory and leave it for later or fully route the net_error
> out of URLRequest correctly.

Thank you for your feedback. I'll leave this for second iteration then after all
the other changes land, otherwise I'll have to change quite a lot, becuase this
method is overridden by other clients as well.

[maksims (do not use this acc), 2016-09-12 12:12:48]

mmenke@, can I commit this or better wait? what do you think?

[mmenke, 2016-09-12 15:33:30]

On 2016/09/12 12:12:48, maksims wrote:
> mmenke@, can I commit this or better wait? what do you think?

It's up to you.  I'd try not landing it at the same time as the others, as you
incrementally see if the crash will reappear, but I think you're fine to land at
your leisure.

[maksims (do not use this acc), 2016-09-16 11:27:27]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-16 11:27:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-16 12:06:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-16 12:06:41]

Dry run: This issue passed the CQ dry run.

[maksims (do not use this acc), 2016-09-19 04:22:01]

The CQ bit was checked by maksim.sisov@intel.com

[maksims (do not use this acc), 2016-09-19 04:22:01]

The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org,
mmenke@chromium.org, rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/2261103002/#ps260001
(title: "comments")

[commit-bot: I haz the power, 2016-09-19 04:22:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-19 04:54:18]

Description was changed from

==========
Use modified URLRequest::Read() and delegate methods in components/

This CL modifies clients that use URLRequest::Read() method
or implement net::Delegate/net::NetworkDelegate methods
and makes them to use returned networking errors instead,
which are returned in term of int.

Old implementation of URLRequest::Read() used to set its  
status inside the URLRequest object itself and clients
used to check the status by calling request->status().

The main CL that changes URLRequest::Read and delegate
methods is here - https://codereview.chromium.org/2262653003

BUG=423484
==========

to

==========
Use modified URLRequest::Read() and delegate methods in components/

This CL modifies clients that use URLRequest::Read() method
or implement net::Delegate/net::NetworkDelegate methods
and makes them to use returned networking errors instead,
which are returned in term of int.

Old implementation of URLRequest::Read() used to set its  
status inside the URLRequest object itself and clients
used to check the status by calling request->status().

The main CL that changes URLRequest::Read and delegate
methods is here - https://codereview.chromium.org/2262653003

BUG=423484
==========

[commit-bot: I haz the power, 2016-09-19 04:54:19]

Committed patchset #4 (id:260001)

[commit-bot: I haz the power, 2016-09-19 04:56:29]

Description was changed from

==========
Use modified URLRequest::Read() and delegate methods in components/

This CL modifies clients that use URLRequest::Read() method
or implement net::Delegate/net::NetworkDelegate methods
and makes them to use returned networking errors instead,
which are returned in term of int.

Old implementation of URLRequest::Read() used to set its  
status inside the URLRequest object itself and clients
used to check the status by calling request->status().

The main CL that changes URLRequest::Read and delegate
methods is here - https://codereview.chromium.org/2262653003

BUG=423484
==========

to

==========
Use modified URLRequest::Read() and delegate methods in components/

This CL modifies clients that use URLRequest::Read() method
or implement net::Delegate/net::NetworkDelegate methods
and makes them to use returned networking errors instead,
which are returned in term of int.

Old implementation of URLRequest::Read() used to set its
status inside the URLRequest object itself and clients
used to check the status by calling request->status().

The main CL that changes URLRequest::Read and delegate
methods is here - https://codereview.chromium.org/2262653003

BUG=423484

Committed: https://crrev.com/1faf924e0dc99a797084918fc04bc6a688e2cd4c
Cr-Commit-Position: refs/heads/master@{#419413}
==========

[commit-bot: I haz the power, 2016-09-19 04:56:31]

Patchset 4 (id:??) landed as
https://crrev.com/1faf924e0dc99a797084918fc04bc6a688e2cd4c
Cr-Commit-Position: refs/heads/master@{#419413}