Adjust callers and networking delegates in chrome/ to modified APIs

Adjust callers and networking delegates in chrome/ to modified APIs

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

This CL dependents on the following CL as well -
https://codereview.chromium.org/2264973002/

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

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

Description was changed from

==========
chrome

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

to

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

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

[maksims (do not use this acc), 2016-08-22 06:51:01]

Patchset #2 (id:20001) has been deleted

[maksims (do not use this acc), 2016-08-22 06:51:35]

Patchset #1 (id:1) has been deleted

[maksims (do not use this acc), 2016-09-05 08:16:01]

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 08:16:13]

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-05 08:16:58]

Description was changed from

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

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

to

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

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

This dependents on the following CL as well - 
https://codereview.chromium.org/2264973002/

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

[commit-bot: I haz the power, 2016-09-05 08:28:20]

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

[commit-bot: I haz the power, 2016-09-05 08:28:21]

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

[maksims (do not use this acc), 2016-09-05 08:33:37]

maksim.sisov@intel.com changed reviewers:
+ dewittj@chromium.org, mmenke@chromium.org, mtomasz@chromium.org,
pasko@chromium.org, sky@chromium.org

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

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 08:37:15]

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-05 08:37:20]

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

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

Patchset #5 (id:120001) has been deleted

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

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

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

Patchset #4 (id:100001) has been deleted

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

Patchset #3 (id:80001) has been deleted

[commit-bot: I haz the power, 2016-09-05 10:50:55]

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-05 10:50:57]

Patchset #2 (id:60001) has been deleted

[maksims (do not use this acc), 2016-09-05 10:51:08]

Description was changed from

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

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

This dependents on the following CL as well - 
https://codereview.chromium.org/2264973002/

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

to

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

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

This CL dependents on the following CL as well - 
https://codereview.chromium.org/2264973002/

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

[maksims (do not use this acc), 2016-09-05 10:51:15]

Patchset #1 (id:40001) has been deleted

[commit-bot: I haz the power, 2016-09-05 11:00:31]

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

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

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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 12:23:41]

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

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

Patchset #2 (id:160001) has been deleted

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

Patchset #1 (id:140001) has been deleted

[commit-bot: I haz the power, 2016-09-06 12:23:54]

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 12:38:05]

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

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

Dry run: Try jobs failed on following builders:
  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 17:25:29]

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 17:26:08]

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 17:38:22]

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

[commit-bot: I haz the power, 2016-09-06 17:38:23]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_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-09-06 18:14:00]

Patchset #2 (id:200001) has been deleted

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

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 18:14:35]

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 18:32:38]

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

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

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_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 18:39:16]

Patchset #1 (id:180001) has been deleted

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

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

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

Patchset #1 (id:220001) has been deleted

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

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 20:08:25]

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

[commit-bot: I haz the power, 2016-09-06 20:08:26]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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-21 06:38:06]

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

[commit-bot: I haz the power, 2016-09-21 06:38: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-21 06:51:41]

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

[commit-bot: I haz the power, 2016-09-21 06:51:42]

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

[pasko, 2016-09-21 18:14:02]

https://codereview.chromium.org/2264903003/diff/260001/chrome/browser/predict...
File chrome/browser/predictors/resource_prefetcher.cc (right):

https://codereview.chromium.org/2264903003/diff/260001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetcher.cc:173: } else if (bytes_read < 0)
{
why checking the condition again?

this equivalent code would look more readable:
if (bytes_read >= 0) {
  ...
else if (bytes_read != net::ERR_IO_PENDING) {
  ...
}

https://codereview.chromium.org/2264903003/diff/260001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetcher.cc:221:
DCHECK_NE(net::ERR_IO_PENDING, net_error);
In what circumstances is net::ERR_IO_PENDING provided with OnResponseStarted()?
Never? If so, why DCHECK in every client while it could be checked on the API
level?

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

https://codereview.chromium.org/2264903003/diff/260001/chrome/browser/predict...
File chrome/browser/predictors/resource_prefetcher.cc (right):

https://codereview.chromium.org/2264903003/diff/260001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetcher.cc:221:
DCHECK_NE(net::ERR_IO_PENDING, net_error);
On 2016/09/21 18:14:02, pasko wrote:
> In what circumstances is net::ERR_IO_PENDING provided with
OnResponseStarted()?
> Never? If so, why DCHECK in every client while it could be checked on the API
> level?

Because it was done like this in some clients. What is more, it helps to avoid
misusing of these delegate methods.

[dewittj, 2016-09-22 16:27:06]

https://codereview.chromium.org/2264903003/diff/260001/chrome/browser/android...
File chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc
(right):

https://codereview.chromium.org/2264903003/diff/260001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc:105:
DCHECK_NE(net::ERR_IO_PENDING, net_error);
This DCHECK seems wrong.  I can see two cases here:
* I'm likely to get an IO_PENDING message for normal usage.  In that case we
should just return early.
* I'm not likely to get an IO_PENDING message; then the net::OK check seems
fine.

Neither case should require a DCHECK, right?

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

Description was changed from

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

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

This CL dependents on the following CL as well - 
https://codereview.chromium.org/2264973002/

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

to

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

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

This CL dependents on the following CL as well - 
https://codereview.chromium.org/2264973002/

BUG=423484

dewittj, mtomasz, mmenke, sky, pasko

chromium-reviews, extensions-reviews@chromium.org, cbentzel+watch@chromium.org,
tzik, shishir+watch@chromium.org, tburkard+watch@chromium.org, nhiroki (ooo
until Sep. 30), gavinp+prer@chromium.org, oshima+watch@chromium.org,
chromium-apps-reviews@chromium.org, kinuko+fileapi, davemoore+watch@chromium.org
==========

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

maksim.sisov@intel.com changed reviewers:
- dewittj@chromium.org, mmenke@chromium.org, mtomasz@chromium.org,
pasko@chromium.org, sky@chromium.org

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

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

[commit-bot: I haz the power, 2016-09-28 08:18:21]

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-28 08:39:32]

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

[commit-bot: I haz the power, 2016-09-28 08:39:33]

Dry run: Try jobs failed on following builders:
  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-09-29 05:49:44]

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

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

Patchset #3 (id:280001) has been deleted

[commit-bot: I haz the power, 2016-09-29 05:50:00]

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-29 06:06:26]

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

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

Dry run: Try jobs failed on following builders:
  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-09-29 06:27:24]

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

[commit-bot: I haz the power, 2016-09-29 06:27:40]

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-29 07:06:36]

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

[commit-bot: I haz the power, 2016-09-29 07:06:37]

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-29 09:05:51]

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

[commit-bot: I haz the power, 2016-09-29 09:06: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-29 09:47:54]

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

[commit-bot: I haz the power, 2016-09-29 09:47:55]

Dry run: This issue passed the CQ dry run.

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

maksim.sisov@intel.com changed reviewers:
+ battre@chromium.org, dewittj@chromium.org, mmenke@chromium.org,
pasko@chromium.org, pkasting@chromium.org, thestig@chromium.org

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

please review

https://codereview.chromium.org/2264903003/diff/260001/chrome/browser/android...
File chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc
(right):

https://codereview.chromium.org/2264903003/diff/260001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc:105:
DCHECK_NE(net::ERR_IO_PENDING, net_error);
On 2016/09/22 16:27:06, dewittj wrote:
> This DCHECK seems wrong.  I can see two cases here:
> * I'm likely to get an IO_PENDING message for normal usage.  In that case we
> should just return early.
> * I'm not likely to get an IO_PENDING message; then the net::OK check seems
> fine.
> 
> Neither case should require a DCHECK, right?

Actually, IO_PENDING must not arrive here. It's just a check.

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

Description was changed from

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

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

This CL dependents on the following CL as well - 
https://codereview.chromium.org/2264973002/

BUG=423484

dewittj, mtomasz, mmenke, sky, pasko

chromium-reviews, extensions-reviews@chromium.org, cbentzel+watch@chromium.org,
tzik, shishir+watch@chromium.org, tburkard+watch@chromium.org, nhiroki (ooo
until Sep. 30), gavinp+prer@chromium.org, oshima+watch@chromium.org,
chromium-apps-reviews@chromium.org, kinuko+fileapi, davemoore+watch@chromium.org
==========

to

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

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

This CL dependents on the following CL as well - 
https://codereview.chromium.org/2264973002/

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

[battre, 2016-09-29 13:24:29]

https://codereview.chromium.org/2264903003/diff/340001/chrome/browser/extensi...
File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc
(left):

https://codereview.chromium.org/2264903003/diff/340001/chrome/browser/extensi...
chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:490:
EXPECT_EQ(net::ERR_ABORTED, request->status().error());
Doesn't a canceled request have some status?

[pasko, 2016-09-29 13:51:40]

https://codereview.chromium.org/2264903003/diff/260001/chrome/browser/predict...
File chrome/browser/predictors/resource_prefetcher.cc (right):

https://codereview.chromium.org/2264903003/diff/260001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetcher.cc:173: } else if (bytes_read < 0)
{
On 2016/09/21 18:14:02, pasko wrote:
> why checking the condition again?
> 
> this equivalent code would look more readable:
> if (bytes_read >= 0) {
>   ...
> else if (bytes_read != net::ERR_IO_PENDING) {
>   ...
> }

please respond to this comment

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

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

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

https://codereview.chromium.org/2264903003/diff/260001/chrome/browser/predict...
File chrome/browser/predictors/resource_prefetcher.cc (right):

https://codereview.chromium.org/2264903003/diff/260001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetcher.cc:173: } else if (bytes_read < 0)
{
On 2016/09/29 13:51:40, pasko wrote:
> On 2016/09/21 18:14:02, pasko wrote:
> > why checking the condition again?
> > 
> > this equivalent code would look more readable:
> > if (bytes_read >= 0) {
> >   ...
> > else if (bytes_read != net::ERR_IO_PENDING) {
> >   ...
> > }
> 
> please respond to this comment

Sorry. Right, done!

https://codereview.chromium.org/2264903003/diff/340001/chrome/browser/extensi...
File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc
(left):

https://codereview.chromium.org/2264903003/diff/340001/chrome/browser/extensi...
chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:490:
EXPECT_EQ(net::ERR_ABORTED, request->status().error());
On 2016/09/29 13:24:29, battre wrote:
> Doesn't a canceled request have some status?

Yes, indeed, it has. But the status is returned through callbacks for which we
are heading for right now. It would be completely not possible to check the
status of the request during it's job being done.

Basically, the cancelled status is passed through NetworkDelegate::OnComplete
callback which is then passed all the way through to
ExtensionsWebRequestEventRouter::OnErrorOccurred or through
Delegate::OnResponseStarted or onReadCompleted (if job is not pending) taking
into account whether a response has already been handled or not.

Basically, I can extract the net error from the event message at this stage.

Or just int net_error = request->Cancel();
It is guaranteed that request will be cancelled and job will be stopped with an
appropriate error set.

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

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-30 06:08:48]

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

[commit-bot: I haz the power, 2016-09-30 06:08:49]

Dry run: Try jobs failed on following builders:
  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-30 06:49:25]

Patchset #7 (id:380001) has been deleted

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

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

[commit-bot: I haz the power, 2016-09-30 06:49:53]

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

[battre, 2016-09-30 06:53:24]

chrome/browser/extensions/api/web_request/web_request_api_unittest.cc and
chrome/browser/net/* LGTM

https://codereview.chromium.org/2264903003/diff/340001/chrome/browser/extensi...
File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc
(left):

https://codereview.chromium.org/2264903003/diff/340001/chrome/browser/extensi...
chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:490:
EXPECT_EQ(net::ERR_ABORTED, request->status().error());
On 2016/09/30 06:06:07, maksims wrote:
> On 2016/09/29 13:24:29, battre wrote:
> > Doesn't a canceled request have some status?
> 
> Yes, indeed, it has. But the status is returned through callbacks for which we
> are heading for right now. It would be completely not possible to check the
> status of the request during it's job being done.
> 
> Basically, the cancelled status is passed through NetworkDelegate::OnComplete
> callback which is then passed all the way through to
> ExtensionsWebRequestEventRouter::OnErrorOccurred or through
> Delegate::OnResponseStarted or onReadCompleted (if job is not pending) taking
> into account whether a response has already been handled or not.
> 
> Basically, I can extract the net error from the event message at this stage.
> 
> Or just int net_error = request->Cancel();
> It is guaranteed that request will be cancelled and job will be stopped with
an
> appropriate error set. 

ok, I see. I think the important line of this test is 491 (checking that the
redirect did not happen).

[maksims (do not use this acc), 2016-09-30 07:00:49]

On 2016/09/30 06:53:24, battre wrote:
> chrome/browser/extensions/api/web_request/web_request_api_unittest.cc and
> chrome/browser/net/* LGTM
> 
>
https://codereview.chromium.org/2264903003/diff/340001/chrome/browser/extensi...
> File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc
> (left):
> 
>
https://codereview.chromium.org/2264903003/diff/340001/chrome/browser/extensi...
> chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:490:
> EXPECT_EQ(net::ERR_ABORTED, request->status().error());
> On 2016/09/30 06:06:07, maksims wrote:
> > On 2016/09/29 13:24:29, battre wrote:
> > > Doesn't a canceled request have some status?
> > 
> > Yes, indeed, it has. But the status is returned through callbacks for which
we
> > are heading for right now. It would be completely not possible to check the
> > status of the request during it's job being done.
> > 
> > Basically, the cancelled status is passed through
NetworkDelegate::OnComplete
> > callback which is then passed all the way through to
> > ExtensionsWebRequestEventRouter::OnErrorOccurred or through
> > Delegate::OnResponseStarted or onReadCompleted (if job is not pending)
taking
> > into account whether a response has already been handled or not.
> > 
> > Basically, I can extract the net error from the event message at this stage.
> > 
> > Or just int net_error = request->Cancel();
> > It is guaranteed that request will be cancelled and job will be stopped with
> an
> > appropriate error set. 
> 
> ok, I see. I think the important line of this test is 491 (checking that the
> redirect did not happen).

Thanks!

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

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

[commit-bot: I haz the power, 2016-09-30 07:05: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_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[pasko, 2016-09-30 10:56:22]

https://codereview.chromium.org/2264903003/diff/400001/chrome/browser/predict...
File chrome/browser/predictors/resource_prefetcher.cc (right):

https://codereview.chromium.org/2264903003/diff/400001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetcher.cc:150: } else if (bytes_read !=
net::ERR_IO_PENDING) {
This condition is never true because bytes_read is >= 0 and net::ERR_IO_PENDING
is equal to -1.

Please let me know if this code passes all the tests, I would get worried and
would consider adding more tests.

[maksims (do not use this acc), 2016-09-30 17:45:04]

On 2016/09/30 10:56:22, pasko wrote:
>
https://codereview.chromium.org/2264903003/diff/400001/chrome/browser/predict...
> File chrome/browser/predictors/resource_prefetcher.cc (right):
> 
>
https://codereview.chromium.org/2264903003/diff/400001/chrome/browser/predict...
> chrome/browser/predictors/resource_prefetcher.cc:150: } else if (bytes_read !=
> net::ERR_IO_PENDING) {
> This condition is never true because bytes_read is >= 0 and
net::ERR_IO_PENDING
> is equal to -1.
> 
> Please let me know if this code passes all the tests, I would get worried and
> would consider adding more tests.

Ah, sorry. Haven't noticed. I've been to busy and just urgently copy pasted
wrong things.

[maksims (do not use this acc), 2016-09-30 17:49:44]

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

[commit-bot: I haz the power, 2016-09-30 17:50:17]

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-30 18:34:54]

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

[commit-bot: I haz the power, 2016-09-30 18:34:55]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2016-09-30 20:25:23]

I got looped in as a reviewer here.  There are many files and many reviewers.

Please say specifically which files and in what role you want each person to
review, on any CL where you have multiple reviewers.  For example, "a: Review
everything but foo.cc; b: OWNERS for dir/; c: Review + OWNER for foo.cc".

I'll I was added as c/b/ui/ OWNER.  LGTM on the comment additions there.

[maksims (do not use this acc), 2016-10-03 18:32:34]

On 2016/09/30 10:56:22, pasko wrote:
>
https://codereview.chromium.org/2264903003/diff/400001/chrome/browser/predict...
> File chrome/browser/predictors/resource_prefetcher.cc (right):
> 
>
https://codereview.chromium.org/2264903003/diff/400001/chrome/browser/predict...
> chrome/browser/predictors/resource_prefetcher.cc:150: } else if (bytes_read !=
> net::ERR_IO_PENDING) {
> This condition is never true because bytes_read is >= 0 and
net::ERR_IO_PENDING
> is equal to -1.
> 
> Please let me know if this code passes all the tests, I would get worried and
> would consider adding more tests.

pasko@, please check

[pasko, 2016-10-04 11:18:42]

https://codereview.chromium.org/2264903003/diff/420001/chrome/browser/predict...
File chrome/browser/predictors/resource_prefetcher.cc (right):

https://codereview.chromium.org/2264903003/diff/420001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetcher.cc:149: } else if (bytes_read !=
net::ERR_IO_PENDING) {
So if we get net::ERR_IO_PENDING, this will turn into a busy loop never updating
the |status|. Not good.

[maksims (do not use this acc), 2016-10-04 11:51:25]

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

[maksims (do not use this acc), 2016-10-04 11:51:35]

https://codereview.chromium.org/2264903003/diff/420001/chrome/browser/predict...
File chrome/browser/predictors/resource_prefetcher.cc (right):

https://codereview.chromium.org/2264903003/diff/420001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetcher.cc:149: } else if (bytes_read !=
net::ERR_IO_PENDING) {
On 2016/10/04 11:18:42, pasko wrote:
> So if we get net::ERR_IO_PENDING, this will turn into a busy loop never
updating
> the |status|. Not good.

Basically, previous implementation has this error as well.
is_success() returned true if the error code was net::OK or net::ERR_IO_PENDING;

[commit-bot: I haz the power, 2016-10-04 11:51:37]

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

[pasko, 2016-10-04 12:14:16]

chrome/browser/predictors and chrome/browser/prerender lgtm

https://codereview.chromium.org/2264903003/diff/420001/chrome/browser/predict...
File chrome/browser/predictors/resource_prefetcher.cc (right):

https://codereview.chromium.org/2264903003/diff/420001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetcher.cc:149: } else if (bytes_read !=
net::ERR_IO_PENDING) {
On 2016/10/04 11:51:35, maksims wrote:
> On 2016/10/04 11:18:42, pasko wrote:
> > So if we get net::ERR_IO_PENDING, this will turn into a busy loop never
> updating
> > the |status|. Not good.
> 
> Basically, previous implementation has this error as well.
> is_success() returned true if the error code was net::OK or
net::ERR_IO_PENDING;

I noticed you fixed it. Thank you!

Feel free to revert back to the previous version (and notifying me) in case it
does not pass tests for some reason. The previous version was looking good as a
refactoring.

[maksims (do not use this acc), 2016-10-04 12:38:05]

On 2016/10/04 12:14:16, pasko wrote:
> chrome/browser/predictors and chrome/browser/prerender lgtm
> 
>
https://codereview.chromium.org/2264903003/diff/420001/chrome/browser/predict...
> File chrome/browser/predictors/resource_prefetcher.cc (right):
> 
>
https://codereview.chromium.org/2264903003/diff/420001/chrome/browser/predict...
> chrome/browser/predictors/resource_prefetcher.cc:149: } else if (bytes_read !=
> net::ERR_IO_PENDING) {
> On 2016/10/04 11:51:35, maksims wrote:
> > On 2016/10/04 11:18:42, pasko wrote:
> > > So if we get net::ERR_IO_PENDING, this will turn into a busy loop never
> > updating
> > > the |status|. Not good.
> > 
> > Basically, previous implementation has this error as well.
> > is_success() returned true if the error code was net::OK or
> net::ERR_IO_PENDING;
> 
> I noticed you fixed it. Thank you!
> 
> Feel free to revert back to the previous version (and notifying me) in case it
> does not pass tests for some reason. The previous version was looking good as
a
> refactoring.

Thanks! Basically, that confused me.

[commit-bot: I haz the power, 2016-10-04 12:38:25]

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

[commit-bot: I haz the power, 2016-10-04 12:38:26]

Dry run: This issue passed the CQ dry run.

[maksims (do not use this acc), 2016-10-04 12:46:07]

maksim.sisov@intel.com changed reviewers:
- dewittj@chromium.org, thestig@chromium.org

[maksims (do not use this acc), 2016-10-05 05:27:47]

maksim.sisov@intel.com changed reviewers:
+ dimich@chromium.org, sky@chromium.org, thestig@chromium.org

[maksims (do not use this acc), 2016-10-05 05:27:48]

dimich@: Please review changes in 
chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc

thestig@ or sky@: Please review changes in 
chrome/browser/extensions/extension_protocols_unittest.cc	chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc
I couldn't find persons who own these files.

[Lei Zhang, 2016-10-05 07:21:36]

On 2016/10/05 05:27:48, maksims wrote:
> thestig@ or sky@: Please review changes in 
>
chrome/browser/extensions/extension_protocols_unittest.cc	chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc
> I couldn't find persons who own these files.

Umm, the c/b/{extensions,loader}/OWNERS files exist. Why do you say you have
trouble finding owners?

[maksims (do not use this acc), 2016-10-05 09:27:23]

On 2016/10/05 07:21:36, Lei Zhang wrote:
> On 2016/10/05 05:27:48, maksims wrote:
> > thestig@ or sky@: Please review changes in 
> >
>
chrome/browser/extensions/extension_protocols_unittest.cc	chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc
> > I couldn't find persons who own these files.
> 
> Umm, the c/b/{extensions,loader}/OWNERS files exist. Why do you say you have
> trouble finding owners?

Because it says - per-file
data_reduction_proxy_resource_throttle_android.*=sgurun@chromium.org

That person doesn't seem to be responsible for the files I have.

The same thing with c/b/extensions - 
per-file *networking*=stevenjb@chromium.org
per-file *networking*=tbarzic@chromium.org

They are not responsible for extension_protocols_unittest.cc

[mmenke, 2016-10-05 14:25:06]

On 2016/10/05 09:27:23, maksims wrote:
> On 2016/10/05 07:21:36, Lei Zhang wrote:
> > On 2016/10/05 05:27:48, maksims wrote:
> > > thestig@ or sky@: Please review changes in 
> > >
> >
>
chrome/browser/extensions/extension_protocols_unittest.cc	chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc
> > > I couldn't find persons who own these files.
> > 
> > Umm, the c/b/{extensions,loader}/OWNERS files exist. Why do you say you have
> > trouble finding owners?
> 
> Because it says - per-file
> mailto:data_reduction_proxy_resource_throttle_android.*=sgurun@chromium.org
> 
> That person doesn't seem to be responsible for the files I have.
> 
> The same thing with c/b/extensions - 
> per-file mailto:*networking*=stevenjb@chromium.org
> per-file mailto:*networking*=tbarzic@chromium.org
> 
> They are not responsible for extension_protocols_unittest.cc

The line "file://extensions/OWNERS" means that the people in extensions/OWNERS
own everything in c/b/extensions, so you should take a look there.

c/b/loader/ LGTM.

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

maksim.sisov@intel.com changed reviewers:
+ asargent@chromium.org
- sky@chromium.org

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

maksim.sisov@intel.com changed reviewers:
+ petewil@chromium.org
- dimich@chromium.org

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

petewil@, please review
chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc

asargent@, please review
chrome/browser/extensions/extension_protocols_unittest.cc

[Pete Williamson, 2016-10-06 16:40:48]

chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc
changes lgtm

[asargent_no_longer_on_chrome, 2016-10-06 23:47:58]

chrome/browser/extensions/ lgtm

[maksims (do not use this acc), 2016-10-07 04:49:46]

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

[maksims (do not use this acc), 2016-10-07 04:49:47]

The patchset sent to the CQ was uploaded after l-g-t-m from battre@chromium.org,
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2264903003/#ps440001
(title: "fix while loop")

[commit-bot: I haz the power, 2016-10-07 04:50:06]

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-10-07 06:07:49]

Description was changed from

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

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

This CL dependents on the following CL as well - 
https://codereview.chromium.org/2264973002/

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

to

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

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

This CL dependents on the following CL as well - 
https://codereview.chromium.org/2264973002/

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

[commit-bot: I haz the power, 2016-10-07 06:07:51]

Committed patchset #9 (id:440001)

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

Description was changed from

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

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

This CL dependents on the following CL as well - 
https://codereview.chromium.org/2264973002/

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

to

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

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

This CL dependents on the following CL as well -
https://codereview.chromium.org/2264973002/

BUG=423484

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

[commit-bot: I haz the power, 2016-10-07 06:09:28]

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