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

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

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

BUG=423484
Committed: https://crrev.com/9913cc543c9fd1bf0fc8e77c47abb8aa271a7c1c
Cr-Commit-Position: refs/heads/master@{#419418}

[maksims (do not use this acc), 2016-09-05 13:04: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 13:04: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-09-05 13:52:53]

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

[commit-bot: I haz the power, 2016-09-05 13:52:54]

Dry run: This issue passed the CQ dry run.

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

Description was changed from

==========
appcache

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

to

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

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 04:56:16]

maksim.sisov@intel.com changed reviewers:
+ michaeln@chromium.org

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

please review

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

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 04:59:34]

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

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

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

Dry run: This issue passed the CQ dry run.

[michaeln, 2016-09-07 20:49:40]

https://codereview.chromium.org/2313693002/diff/20001/content/browser/appcach...
File content/browser/appcache/appcache_request_handler_unittest.cc (right):

https://codereview.chromium.org/2313693002/diff/20001/content/browser/appcach...
content/browser/appcache/appcache_request_handler_unittest.cc:87: 
nit: you can delete the blank line and after the dcheck below too

https://codereview.chromium.org/2313693002/diff/20001/content/browser/appcach...
content/browser/appcache/appcache_request_handler_unittest.cc:99: int
request_status() { return request_status_; }
could be const

https://codereview.chromium.org/2313693002/diff/20001/content/browser/appcach...
File content/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/2313693002/diff/20001/content/browser/appcach...
content/browser/appcache/appcache_update_job.cc:189:
OnResponseCompleted(result);
I think i'd prefer to see net::ERR_ABORTED explicitly here, i just spent a
couple minutes verifying that OK was not an expected return value.

https://codereview.chromium.org/2313693002/diff/20001/content/browser/appcach...
content/browser/appcache/appcache_update_job.cc:233:
OnResponseCompleted(result);
ditto preference for net::ERR_ABORTED

https://codereview.chromium.org/2313693002/diff/20001/content/browser/appcach...
content/browser/appcache/appcache_update_job.cc:254: net::URLRequest* request,
int bytes_read) {
maybe DCHECK_NE(net::ERR_IO_PENDING, bytes_read) for more consistency with other
overrides of this method in other classes (we'd have to change the callsite too)

https://codereview.chromium.org/2313693002/diff/20001/content/browser/appcach...
content/browser/appcache/appcache_update_job.cc:262: while (bytes_read > 0) {
somethings off here, the body of the while loop when never be entered. did you
mean to delete line 261? or maybe just make this while (true) break more
explicitly on the two conditions

while (true) {
  bytes_read = requst->Read();
  if (bytes_read <= 0)
    break;
  data_consumed = ConsumeResponseData();
  if (!data_consumed)
    break;
}

https://codereview.chromium.org/2313693002/diff/20001/content/browser/appcach...
content/browser/appcache/appcache_update_job.cc:310:
OnResponseCompleted(result);
ditto pref for net::ERR_ABORTED

https://codereview.chromium.org/2313693002/diff/20001/content/browser/appcach...
content/browser/appcache/appcache_update_job.cc:321:
OnReadCompleted(request_.get(), bytes_read);
maybe avoid calling this if byte_read == IO_PENDING, it'd be more in keeping
wither other impls of this method to be able to make that assumption/assertion

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

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

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

https://codereview.chromium.org/2313693002/diff/20001/content/browser/appcach...
File content/browser/appcache/appcache_request_handler_unittest.cc (right):

https://codereview.chromium.org/2313693002/diff/20001/content/browser/appcach...
content/browser/appcache/appcache_request_handler_unittest.cc:87: 
On 2016/09/07 20:49:39, michaeln wrote:
> nit: you can delete the blank line and after the dcheck below too

Done.

https://codereview.chromium.org/2313693002/diff/20001/content/browser/appcach...
content/browser/appcache/appcache_request_handler_unittest.cc:99: int
request_status() { return request_status_; }
On 2016/09/07 20:49:39, michaeln wrote:
> could be const

Done.

https://codereview.chromium.org/2313693002/diff/20001/content/browser/appcach...
File content/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/2313693002/diff/20001/content/browser/appcach...
content/browser/appcache/appcache_update_job.cc:189:
OnResponseCompleted(result);
On 2016/09/07 20:49:39, michaeln wrote:
> I think i'd prefer to see net::ERR_ABORTED explicitly here, i just spent a
> couple minutes verifying that OK was not an expected return value.

I can use net::ERR_ABORTED here, but if there were any other errors set for some
reason, net::ERR_ABORTED would not be set as an error. I don't know how often it
might happen, but there is a probability of this.

https://codereview.chromium.org/2313693002/diff/20001/content/browser/appcach...
content/browser/appcache/appcache_update_job.cc:233:
OnResponseCompleted(result);
On 2016/09/07 20:49:39, michaeln wrote:
> ditto preference for net::ERR_ABORTED

Done.

https://codereview.chromium.org/2313693002/diff/20001/content/browser/appcach...
content/browser/appcache/appcache_update_job.cc:254: net::URLRequest* request,
int bytes_read) {
On 2016/09/07 20:49:39, michaeln wrote:
> maybe DCHECK_NE(net::ERR_IO_PENDING, bytes_read) for more consistency with
other
> overrides of this method in other classes (we'd have to change the callsite
too)

Done.

https://codereview.chromium.org/2313693002/diff/20001/content/browser/appcach...
content/browser/appcache/appcache_update_job.cc:262: while (bytes_read > 0) {
On 2016/09/07 20:49:39, michaeln wrote:
> somethings off here, the body of the while loop when never be entered. did you
> mean to delete line 261? or maybe just make this while (true) break more
> explicitly on the two conditions
> 
> while (true) {
>   bytes_read = requst->Read();
>   if (bytes_read <= 0)
>     break;
>   data_consumed = ConsumeResponseData();
>   if (!data_consumed)
>     break;
> }

Yes, I've forgotten to remove that line. Thanks!

https://codereview.chromium.org/2313693002/diff/20001/content/browser/appcach...
content/browser/appcache/appcache_update_job.cc:310:
OnResponseCompleted(result);
On 2016/09/07 20:49:39, michaeln wrote:
> ditto pref for net::ERR_ABORTED

Done.

https://codereview.chromium.org/2313693002/diff/20001/content/browser/appcach...
content/browser/appcache/appcache_update_job.cc:321:
OnReadCompleted(request_.get(), bytes_read);
On 2016/09/07 20:49:39, michaeln wrote:
> maybe avoid calling this if byte_read == IO_PENDING, it'd be more in keeping
> wither other impls of this method to be able to make that assumption/assertion

Yes, sure. You're right. But as I noticed not all of the other impls have this
kind of logic. They don't call OnReadCompleted by themselves! But good point.
I'll take it into consideration. Thanks!

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

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 12:59:35]

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

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

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

[michaeln, 2016-09-12 20:14:02]

lgtm % typo

https://codereview.chromium.org/2313693002/diff/20001/content/browser/appcach...
File content/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/2313693002/diff/20001/content/browser/appcach...
content/browser/appcache/appcache_update_job.cc:189:
OnResponseCompleted(result);
On 2016/09/12 12:45:58, maksims wrote:
> On 2016/09/07 20:49:39, michaeln wrote:
> > I think i'd prefer to see net::ERR_ABORTED explicitly here, i just spent a
> > couple minutes verifying that OK was not an expected return value.
> 
> I can use net::ERR_ABORTED here, but if there were any other errors set for
some
> reason, net::ERR_ABORTED would not be set as an error. I don't know how often
it
> might happen, but there is a probability of this. 

Thnx

This class is explicitly choosing to abort the request for this non network
error condition. We're processing some kind of redirect response, so i don't
think the request can be in an error state, otherwise, we'd be getting an
OnResponseCompleted callback with the error instead. Even if it is, this class
is explicitly choosing to cancel for other reasons that have nothing todo with
any existing error.

https://codereview.chromium.org/2313693002/diff/40001/content/browser/appcach...
File content/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/2313693002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_update_job.cc:308: = request_->Cancel();
= typo

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

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

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

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-13 05:13: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-13 05:13: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-13 05:46:09]

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

[commit-bot: I haz the power, 2016-09-13 05:46:10]

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

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

Patchset #4 (id:60001) has been deleted

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

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

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

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-19 05:17:53]

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

[commit-bot: I haz the power, 2016-09-19 05:17:53]

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

[maksims (do not use this acc), 2016-09-19 06:08: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-19 06:08: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-19 07:12:06]

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

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

Dry run: This issue passed the CQ dry run.

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

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

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

The patchset sent to the CQ was uploaded after l-g-t-m from
michaeln@chromium.org
Link to the patchset: https://codereview.chromium.org/2313693002/#ps120001
(title: "use DCHECK_NE")

[commit-bot: I haz the power, 2016-09-19 08:11:51]

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

Description was changed from

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

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 /appcache/

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

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

[commit-bot: I haz the power, 2016-09-19 08:16:32]

Committed patchset #6 (id:120001)

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

Description was changed from

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

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 /appcache/

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

BUG=423484

Committed: https://crrev.com/9913cc543c9fd1bf0fc8e77c47abb8aa271a7c1c
Cr-Commit-Position: refs/heads/master@{#419418}
==========

[commit-bot: I haz the power, 2016-09-19 08:18:29]

Patchset 6 (id:??) landed as
https://crrev.com/9913cc543c9fd1bf0fc8e77c47abb8aa271a7c1c
Cr-Commit-Position: refs/heads/master@{#419418}