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

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

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

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

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

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 12:49: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-05 13:06:07]

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

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

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

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

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 05:01:41]

Patchset #2 (id:20001) has been deleted

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

Patchset #1 (id:1) has been deleted

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

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:02:54]

Description was changed from

==========
service_worker

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

to

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

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-06 05:20:48]

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

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

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

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:27: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 09:23:04]

Patchset #1 (id:40001) has been deleted

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

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

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

please review

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

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

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

Dry run: This issue passed the CQ dry run.

[falken, 2016-09-07 02:21:50]

This looks pretty good, some comments.

https://codereview.chromium.org/2309793002/diff/60001/content/browser/service...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/2309793002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:214:
*bytes_read = net_request_->Read(buf, buf_size);
Can you change this to just return bytes_read instead of using an output param?

https://codereview.chromium.org/2309793002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:427: if
(result == net::OK)
The original code seems to accept IO_PENDING but that is probably a bug. Should
we add a DCHECK_NE(IO_PENDING)?

It may be cleaner to combine with the above if to an if/else as well.

https://codereview.chromium.org/2309793002/diff/60001/content/browser/service...
File content/browser/service_worker/service_worker_write_to_cache_job.h (right):

https://codereview.chromium.org/2309793002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.h:133:
net::Error NotifyFinishedCaching(int net_error,
Why are some functions taking net::Error and some are taking "int net_error"?
It's jarring here where the return value and params don't match. I think I'd
prefer everything that's a net error to be net::Error. I see the
net::URLRequest::Delegate interface uses "int" though.

[michaeln, 2016-09-07 20:53:17]

since falkens taking a look, i'll bow out of this review

[mmenke, 2016-09-12 16:50:38]

mmenke@chromium.org changed reviewers:
+ mmenke@chromium.org

[mmenke, 2016-09-12 16:50:39]

https://codereview.chromium.org/2309793002/diff/60001/content/browser/service...
File content/browser/service_worker/service_worker_write_to_cache_job.h (right):

https://codereview.chromium.org/2309793002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.h:133:
net::Error NotifyFinishedCaching(int net_error,
On 2016/09/07 02:21:50, falken wrote:
> Why are some functions taking net::Error and some are taking "int net_error"?
> It's jarring here where the return value and params don't match. I think I'd
> prefer everything that's a net error to be net::Error. I see the
> net::URLRequest::Delegate interface uses "int" though.

Read data methods return a single value - a net::Error if < 0, or a value > 0 on
success (0 / net::OK means a graceful completion).  Since it's not always a
net::Error, Read must return an int.  This is a pattern that appears internal to
net/, too.  It would be great if we separated out error codes from bytes read in
net/ APIs, but that's well beyond the scope of this CL.

[mmenke, 2016-09-12 16:51:07]

On 2016/09/12 16:50:39, mmenke wrote:
>
https://codereview.chromium.org/2309793002/diff/60001/content/browser/service...
> File content/browser/service_worker/service_worker_write_to_cache_job.h
(right):
> 
>
https://codereview.chromium.org/2309793002/diff/60001/content/browser/service...
> content/browser/service_worker/service_worker_write_to_cache_job.h:133:
> net::Error NotifyFinishedCaching(int net_error,
> On 2016/09/07 02:21:50, falken wrote:
> > Why are some functions taking net::Error and some are taking "int
net_error"?
> > It's jarring here where the return value and params don't match. I think I'd
> > prefer everything that's a net error to be net::Error. I see the
> > net::URLRequest::Delegate interface uses "int" though.
> 
> Read data methods return a single value - a net::Error if < 0, or a value > 0
on
> success (0 / net::OK means a graceful completion).  Since it's not always a
> net::Error, Read must return an int.  This is a pattern that appears internal
to
> net/, too.  It would be great if we separated out error codes from bytes read
in
> net/ APIs, but that's well beyond the scope of this CL.

The value > 0 being the exact number of bytes that were actually read.

[falken, 2016-09-13 06:31:35]

https://codereview.chromium.org/2309793002/diff/60001/content/browser/service...
File content/browser/service_worker/service_worker_write_to_cache_job.h (right):

https://codereview.chromium.org/2309793002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.h:133:
net::Error NotifyFinishedCaching(int net_error,
On 2016/09/12 16:50:39, mmenke wrote:
> On 2016/09/07 02:21:50, falken wrote:
> > Why are some functions taking net::Error and some are taking "int
net_error"?
> > It's jarring here where the return value and params don't match. I think I'd
> > prefer everything that's a net error to be net::Error. I see the
> > net::URLRequest::Delegate interface uses "int" though.
> 
> Read data methods return a single value - a net::Error if < 0, or a value > 0
on
> success (0 / net::OK means a graceful completion).  Since it's not always a
> net::Error, Read must return an int.  This is a pattern that appears internal
to
> net/, too.  It would be great if we separated out error codes from bytes read
in
> net/ APIs, but that's well beyond the scope of this CL.

That makes sense but it looks like the convention of > 0 to mean success isn't
being followed.

For example ServiceWorkerWriteToCacheJob::OnResponseStarted(net::URLRequest*
request, int net_error) does if (net_error != net::OK) { // fail } so it's
treating values > 0 as failures. Wouldn't it be clearer for the type of
|net_error| to be net::Error to indicate it's expecting net::OK to mean success?
Indeed the documentation of Delegate::OnResponseError says the value is 0 on
success or else a net error code.

Similarly,
ServiceWorkerWriteToCacheJob::NotifyFinishedCaching(net::URLRequestStatus
status, int net_error) does a static_cast<net::Error>(net_error) so it seems the
param should just be a net::Error.

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

On 2016/09/13 06:31:35, falken wrote:
>
https://codereview.chromium.org/2309793002/diff/60001/content/browser/service...
> File content/browser/service_worker/service_worker_write_to_cache_job.h
(right):
> 
>
https://codereview.chromium.org/2309793002/diff/60001/content/browser/service...
> content/browser/service_worker/service_worker_write_to_cache_job.h:133:
> net::Error NotifyFinishedCaching(int net_error,
> On 2016/09/12 16:50:39, mmenke wrote:
> > On 2016/09/07 02:21:50, falken wrote:
> > > Why are some functions taking net::Error and some are taking "int
> net_error"?
> > > It's jarring here where the return value and params don't match. I think
I'd
> > > prefer everything that's a net error to be net::Error. I see the
> > > net::URLRequest::Delegate interface uses "int" though.
> > 
> > Read data methods return a single value - a net::Error if < 0, or a value >
0
> on
> > success (0 / net::OK means a graceful completion).  Since it's not always a
> > net::Error, Read must return an int.  This is a pattern that appears
internal
> to
> > net/, too.  It would be great if we separated out error codes from bytes
read
> in
> > net/ APIs, but that's well beyond the scope of this CL.
> 
> That makes sense but it looks like the convention of > 0 to mean success isn't
> being followed.
> 
> For example ServiceWorkerWriteToCacheJob::OnResponseStarted(net::URLRequest*
> request, int net_error) does if (net_error != net::OK) { // fail } so it's
> treating values > 0 as failures. Wouldn't it be clearer for the type of
> |net_error| to be net::Error to indicate it's expecting net::OK to mean
success?
> Indeed the documentation of Delegate::OnResponseError says the value is 0 on
> success or else a net error code.
> 
> Similarly,
> ServiceWorkerWriteToCacheJob::NotifyFinishedCaching(net::URLRequestStatus
> status, int net_error) does a static_cast<net::Error>(net_error) so it seems
the
> param should just be a net::Error.

Yes, good point. I guess Matt wanted to say that, for example, OnReadCompleted
can have any value from -n to +n, where > 0 indicates bytes read, == 0 is
net::OK and other values are errors. But it will look ugly if on of the methods
would have net::Error (e.g. OnResponseStarted) and OnReadCompleted would just
have int in its arguments.

[maksims (do not use this acc), 2016-09-13 06:52:01]

On 2016/09/13 06:51:27, maksims wrote:
> On 2016/09/13 06:31:35, falken wrote:
> >
>
https://codereview.chromium.org/2309793002/diff/60001/content/browser/service...
> > File content/browser/service_worker/service_worker_write_to_cache_job.h
> (right):
> > 
> >
>
https://codereview.chromium.org/2309793002/diff/60001/content/browser/service...
> > content/browser/service_worker/service_worker_write_to_cache_job.h:133:
> > net::Error NotifyFinishedCaching(int net_error,
> > On 2016/09/12 16:50:39, mmenke wrote:
> > > On 2016/09/07 02:21:50, falken wrote:
> > > > Why are some functions taking net::Error and some are taking "int
> > net_error"?
> > > > It's jarring here where the return value and params don't match. I think
> I'd
> > > > prefer everything that's a net error to be net::Error. I see the
> > > > net::URLRequest::Delegate interface uses "int" though.
> > > 
> > > Read data methods return a single value - a net::Error if < 0, or a value
>
> 0
> > on
> > > success (0 / net::OK means a graceful completion).  Since it's not always
a
> > > net::Error, Read must return an int.  This is a pattern that appears
> internal
> > to
> > > net/, too.  It would be great if we separated out error codes from bytes
> read
> > in
> > > net/ APIs, but that's well beyond the scope of this CL.
> > 
> > That makes sense but it looks like the convention of > 0 to mean success
isn't
> > being followed.
> > 
> > For example ServiceWorkerWriteToCacheJob::OnResponseStarted(net::URLRequest*
> > request, int net_error) does if (net_error != net::OK) { // fail } so it's
> > treating values > 0 as failures. Wouldn't it be clearer for the type of
> > |net_error| to be net::Error to indicate it's expecting net::OK to mean
> success?
> > Indeed the documentation of Delegate::OnResponseError says the value is 0 on
> > success or else a net error code.
> > 
> > Similarly,
> > ServiceWorkerWriteToCacheJob::NotifyFinishedCaching(net::URLRequestStatus
> > status, int net_error) does a static_cast<net::Error>(net_error) so it seems
> the
> > param should just be a net::Error.
> 
> Yes, good point. I guess Matt wanted to say that, for example, OnReadCompleted
> can have any value from -n to +n, where > 0 indicates bytes read, == 0 is
> net::OK and other values are errors. But it will look ugly if on of the
methods
> would have net::Error (e.g. OnResponseStarted) and OnReadCompleted would just
> have int in its arguments.

on of the methods = all methods

[mmenke, 2016-09-13 16:49:44]

https://codereview.chromium.org/2309793002/diff/60001/content/browser/service...
File content/browser/service_worker/service_worker_write_to_cache_job.h (right):

https://codereview.chromium.org/2309793002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.h:133:
net::Error NotifyFinishedCaching(int net_error,
On 2016/09/13 06:31:35, falken wrote:
> On 2016/09/12 16:50:39, mmenke wrote:
> > On 2016/09/07 02:21:50, falken wrote:
> > > Why are some functions taking net::Error and some are taking "int
> net_error"?
> > > It's jarring here where the return value and params don't match. I think
I'd
> > > prefer everything that's a net error to be net::Error. I see the
> > > net::URLRequest::Delegate interface uses "int" though.
> > 
> > Read data methods return a single value - a net::Error if < 0, or a value >
0
> on
> > success (0 / net::OK means a graceful completion).  Since it's not always a
> > net::Error, Read must return an int.  This is a pattern that appears
internal
> to
> > net/, too.  It would be great if we separated out error codes from bytes
read
> in
> > net/ APIs, but that's well beyond the scope of this CL.
> 
> That makes sense but it looks like the convention of > 0 to mean success isn't
> being followed.
> 
> For example ServiceWorkerWriteToCacheJob::OnResponseStarted(net::URLRequest*
> request, int net_error) does if (net_error != net::OK) { // fail } so it's
> treating values > 0 as failures. Wouldn't it be clearer for the type of
> |net_error| to be net::Error to indicate it's expecting net::OK to mean
success?
> Indeed the documentation of Delegate::OnResponseError says the value is 0 on
> success or else a net error code.
> 
> Similarly,
> ServiceWorkerWriteToCacheJob::NotifyFinishedCaching(net::URLRequestStatus
> status, int net_error) does a static_cast<net::Error>(net_error) so it seems
the
> param should just be a net::Error. 

NotifyFinishedCaching is also called from
ServiceWorkerWriteToCacheJob::OnReadCompleted, where we don't necessarily have a
net error code, so we'd have to static cast here somewhere, regardless.  If you
prefer to have callers here static cast than NotifyFinishedCaching do it itself,
I'm fine with that.

For ServiceWorkerWriteToCacheJob::OnResponseStarted, my general feeling is if
you're static casting, you're doing it wrong, and I really don't want to add a
static_cast to URLRequest::NotifyResponseStarted.  I think we should refactor it
so it takes a net:Error, and can then update the API, but that's a pretty big
undertaking.

[maksims (do not use this acc), 2016-09-21 04:51:44]

https://codereview.chromium.org/2309793002/diff/60001/content/browser/service...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/2309793002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:214:
*bytes_read = net_request_->Read(buf, buf_size);
On 2016/09/07 02:21:50, falken wrote:
> Can you change this to just return bytes_read instead of using an output
param?

Done.

https://codereview.chromium.org/2309793002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:427: if
(result == net::OK)
On 2016/09/07 02:21:50, falken wrote:
> The original code seems to accept IO_PENDING but that is probably a bug.
Should
> we add a DCHECK_NE(IO_PENDING)?
> 
> It may be cleaner to combine with the above if to an if/else as well.

Done.

https://codereview.chromium.org/2309793002/diff/60001/content/browser/service...
File content/browser/service_worker/service_worker_write_to_cache_job.h (right):

https://codereview.chromium.org/2309793002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.h:133:
net::Error NotifyFinishedCaching(int net_error,
On 2016/09/07 02:21:50, falken wrote:
> Why are some functions taking net::Error and some are taking "int net_error"?
> It's jarring here where the return value and params don't match. I think I'd
> prefer everything that's a net error to be net::Error. I see the
> net::URLRequest::Delegate interface uses "int" though.

Well, it can be converted easily and there is no much difference in using
whether net::Error or just plain int. Better to use int, I guess. But I'll
discuss it within the networking team.

https://codereview.chromium.org/2309793002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.h:133:
net::Error NotifyFinishedCaching(int net_error,
So what do you think? Is it fine to static cast to net::Error in
ServiceWorkerWriteToCacheJob::NotifyFinishedCaching or??

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

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 04:53: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-21 05:23:15]

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

[commit-bot: I haz the power, 2016-09-21 05:23:16]

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-21 06:04:23]

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:04:43]

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 07:24:53]

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

[commit-bot: I haz the power, 2016-09-21 07:24:54]

Dry run: This issue passed the CQ dry run.

[falken, 2016-09-21 07:31:39]

https://codereview.chromium.org/2309793002/diff/60001/content/browser/service...
File content/browser/service_worker/service_worker_write_to_cache_job.h (right):

https://codereview.chromium.org/2309793002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.h:133:
net::Error NotifyFinishedCaching(int net_error,
On 2016/09/21 04:51:44, maksims wrote:
> So what do you think? Is it fine to static cast to net::Error in
> ServiceWorkerWriteToCacheJob::NotifyFinishedCaching or??

Sorry I didn't realize you were blocked on me. I'd prefer to have the param be a
net::Error and the callers static cast to make it clear it's a net error.

https://codereview.chromium.org/2309793002/diff/100001/content/browser/servic...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/2309793002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_write_to_cache_job.cc:212: return
bytes_read;
nit: just return the value directly without using bytes_read

https://codereview.chromium.org/2309793002/diff/100001/content/browser/servic...
File content/browser/service_worker/service_worker_write_to_cache_job.h (right):

https://codereview.chromium.org/2309793002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_write_to_cache_job.h:129: void
NotifyStartErrorHelper(int net_error, const std::string& status_message);
I prefer net::Error here too.

https://codereview.chromium.org/2309793002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_write_to_cache_job.h:132: //
additional error is found.
nit: update this comment since |status| is gone. If we go with a net:Error
param, I'd say something like  "Returns the error code, which is |net_error| or
a new one if an additional error is found."

[maksims (do not use this acc), 2016-09-21 07:52:54]

https://codereview.chromium.org/2309793002/diff/100001/content/browser/servic...
File content/browser/service_worker/service_worker_write_to_cache_job.h (right):

https://codereview.chromium.org/2309793002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_write_to_cache_job.h:129: void
NotifyStartErrorHelper(int net_error, const std::string& status_message);
On 2016/09/21 07:31:39, falken wrote:
> I prefer net::Error here too.

Why net::Error here? There are quite many calls of this method. And all of them
will have to static cast to net::Error. In my opinion, it would be better to
leave the arguments as they are and have a static cast to net::Error inside this
method (it calls NotifyFinishedCaching and NotifyStartError).

What do you think?

[falken, 2016-09-21 07:57:23]

https://codereview.chromium.org/2309793002/diff/100001/content/browser/servic...
File content/browser/service_worker/service_worker_write_to_cache_job.h (right):

https://codereview.chromium.org/2309793002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_write_to_cache_job.h:129: void
NotifyStartErrorHelper(int net_error, const std::string& status_message);
On 2016/09/21 07:52:54, maksims wrote:
> On 2016/09/21 07:31:39, falken wrote:
> > I prefer net::Error here too.
> 
> Why net::Error here? There are quite many calls of this method. And all of
them
> will have to static cast to net::Error. In my opinion, it would be better to
> leave the arguments as they are and have a static cast to net::Error inside
this
> method (it calls NotifyFinishedCaching and NotifyStartError).
> 
> What do you think?

Almost all callers are already a net::Error, no? The only one I see is in
OnResponseStarted and as mmenke said that should be refactored to a net::Error
but it's way beyond the scope of this CL.

[maksims (do not use this acc), 2016-09-21 08:30: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-21 08:30:29]

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 08:43:45]

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

[commit-bot: I haz the power, 2016-09-21 08:43:46]

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-21 11:21:23]

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 11:21:30]

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-21 11:21:41]

Patchset #4 (id:120001) has been deleted

[commit-bot: I haz the power, 2016-09-21 11:39:31]

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

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

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...)
  linux_chromium_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 12:40:02]

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 12:40:14]

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 13:40:49]

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

[commit-bot: I haz the power, 2016-09-21 13:40:50]

Dry run: This issue passed the CQ dry run.

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

On 2016/09/21 07:57:23, falken wrote:
>
https://codereview.chromium.org/2309793002/diff/100001/content/browser/servic...
> File content/browser/service_worker/service_worker_write_to_cache_job.h
(right):
> 
>
https://codereview.chromium.org/2309793002/diff/100001/content/browser/servic...
> content/browser/service_worker/service_worker_write_to_cache_job.h:129: void
> NotifyStartErrorHelper(int net_error, const std::string& status_message);
> On 2016/09/21 07:52:54, maksims wrote:
> > On 2016/09/21 07:31:39, falken wrote:
> > > I prefer net::Error here too.
> > 
> > Why net::Error here? There are quite many calls of this method. And all of
> them
> > will have to static cast to net::Error. In my opinion, it would be better to
> > leave the arguments as they are and have a static cast to net::Error inside
> this
> > method (it calls NotifyFinishedCaching and NotifyStartError).
> > 
> > What do you think?
> 
> Almost all callers are already a net::Error, no? The only one I see is in
> OnResponseStarted and as mmenke said that should be refactored to a net::Error
> but it's way beyond the scope of this CL.

Done!

[falken, 2016-09-23 00:37:36]

Sorry we had a holiday yesterday. lgtm with nits. Thanks, this is a nice patch.

https://codereview.chromium.org/2309793002/diff/160001/content/browser/servic...
File content/browser/service_worker/service_worker_script_cache_map.h (right):

https://codereview.chromium.org/2309793002/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_script_cache_map.h:40: int
net_error,
nit: make this net::Error (looks like all callers are passing a net::Error
anyway)

https://codereview.chromium.org/2309793002/diff/160001/content/browser/servic...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/2309793002/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_write_to_cache_job.cc:213: return
bytes_read;
nit: just "return net_request_->Read(buf, buf_size);"

[falken, 2016-09-23 00:39:52]

https://codereview.chromium.org/2309793002/diff/160001/content/browser/servic...
File content/browser/service_worker/service_worker_write_to_cache_job.h (right):

https://codereview.chromium.org/2309793002/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_write_to_cache_job.h:132: //
Returns an error code that is passed in through |status| or a new one if an
Ah this comment should also be updated (see previous review comments).

[maksims (do not use this acc), 2016-09-26 05:41:21]

https://codereview.chromium.org/2309793002/diff/160001/content/browser/servic...
File content/browser/service_worker/service_worker_script_cache_map.h (right):

https://codereview.chromium.org/2309793002/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_script_cache_map.h:40: int
net_error,
On 2016/09/23 00:37:35, falken wrote:
> nit: make this net::Error (looks like all callers are passing a net::Error
> anyway)

Done.

https://codereview.chromium.org/2309793002/diff/160001/content/browser/servic...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/2309793002/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_write_to_cache_job.cc:213: return
bytes_read;
On 2016/09/23 00:37:35, falken wrote:
> nit: just "return net_request_->Read(buf, buf_size);"

Done.

https://codereview.chromium.org/2309793002/diff/160001/content/browser/servic...
File content/browser/service_worker/service_worker_write_to_cache_job.h (right):

https://codereview.chromium.org/2309793002/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_write_to_cache_job.h:132: //
Returns an error code that is passed in through |status| or a new one if an
On 2016/09/23 00:39:52, falken wrote:
> Ah this comment should also be updated (see previous review comments).

Done.

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

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

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

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org
Link to the patchset: https://codereview.chromium.org/2309793002/#ps180001
(title: "nits")

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

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-26 05:51:07]

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

[commit-bot: I haz the power, 2016-09-26 05:51:10]

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

Patchset #6 (id:180001) has been deleted

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

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

[commit-bot: I haz the power, 2016-09-26 09:48:09]

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-27 10:28:48]

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

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

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org
Link to the patchset: https://codereview.chromium.org/2309793002/#ps200001
(title: "nits")

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

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-27 11:56:06]

Description was changed from

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

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

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-27 11:56:08]

Committed patchset #6 (id:200001)

[commit-bot: I haz the power, 2016-09-27 11:59:45]

Description was changed from

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

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

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

BUG=423484

Committed: https://crrev.com/c9b85a7b27cf9a07ce120b8e5303ab7672c83fb1
Cr-Commit-Position: refs/heads/master@{#421176}
==========

[commit-bot: I haz the power, 2016-09-27 11:59:46]

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