ServiceWorker: Change worker script fetch error code(2/3).

ServiceWorker: Change worker script fetch error code.

Change worker script fetch error code in "Update" algorithm to align
with spec:
Non 2xx response will be rejected with NetworkError;
Redirected response will be rejected with SecurityError;
Reponse with Wrong MIME type will be rejected with SecurityError.

(1/3): https://codereview.chromium.org/546353003/
(2/3): this
(3/3): Layout test & Blink side cleanup

BUG=408421
TEST=http/tests/serviceworker

Committed: https://crrev.com/1987a8265654383711fc0bba427339f7eead944f
Cr-Commit-Position: refs/heads/master@{#295940}

[xiang, 2014-09-12 05:08:20]

xiang.long@intel.com changed reviewers:
+ michaeln@chromium.org, nhiroki@chromium.org

[xiang, 2014-09-12 05:08:32]

PTAL, thanks!

[michaeln, 2014-09-13 01:21:35]

https://codereview.chromium.org/567903002/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/567903002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_version.h:223: void
NotifyStartWorkerURLRequestError(ServiceWorkerStatusCode status);
The 'start callbacks' are already being called in these cases. Adding a special
case code paths to include extra error info doesn't seem needed. Can we keep the
existing control flow, but have start callback detect the more nuanced error
condition?

https://codereview.chromium.org/567903002/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/567903002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_write_to_cache_job.cc:283:
version_->NotifyStartWorkerURLRequestError(SERVICE_WORKER_ERROR_SECURITY);
This class is used for more than the main resource, its used for imports as
well. This class deals with a single resource load, the starting of a worker is
more involved than a single resource load. I don't this class can/should make
this call.

https://codereview.chromium.org/567903002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_write_to_cache_job.cc:386:
version_->script_cache_map()->NotifyFinishedCaching(
As an alternative approach.

Currently the ServiceWorkerScriptCacheMap doesn't store much about errors
observed during initial script caching. It could keep a map of urls to
swspecific status codes. ServiceWorkerRegisterJob::OnStartWorkerFinished could
query the ScriptCacheMap for the error observed during the caching of the main
script resource and provide the downstream client with the appropriate status
code based on that.

wdyt?

[xiang, 2014-09-16 06:24:31]

Thanks for the review, CL update.
Would you take a look again?

https://codereview.chromium.org/567903002/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/567903002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_version.h:223: void
NotifyStartWorkerURLRequestError(ServiceWorkerStatusCode status);
On 2014/09/13 01:21:34, michaeln wrote:
> The 'start callbacks' are already being called in these cases. Adding a
special
> case code paths to include extra error info doesn't seem needed. Can we keep
the
> existing control flow, but have start callback detect the more nuanced error
> condition?

Done.

https://codereview.chromium.org/567903002/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/567903002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_write_to_cache_job.cc:283:
version_->NotifyStartWorkerURLRequestError(SERVICE_WORKER_ERROR_SECURITY);
On 2014/09/13 01:21:34, michaeln wrote:
> This class is used for more than the main resource, its used for imports as
> well. This class deals with a single resource load, the starting of a worker
is
> more involved than a single resource load. I don't this class can/should make
> this call.

I see. However from the spec seems the imports redirect should be permitted, I
changed the code to only fail when load main script redirected.

https://codereview.chromium.org/567903002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_write_to_cache_job.cc:386:
version_->script_cache_map()->NotifyFinishedCaching(
On 2014/09/13 01:21:34, michaeln wrote:
> As an alternative approach.
> 
> Currently the ServiceWorkerScriptCacheMap doesn't store much about errors
> observed during initial script caching. It could keep a map of urls to
> swspecific status codes. ServiceWorkerRegisterJob::OnStartWorkerFinished could
> query the ScriptCacheMap for the error observed during the caching of the main
> script resource and provide the downstream client with the appropriate status
> code based on that.
> 
> wdyt?

Your suggestion seems much better. As only main script loading status will
influence the registration success, I just keep its request error code.

[michaeln, 2014-09-16 19:42:40]

https://codereview.chromium.org/567903002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/567903002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_write_to_cache_job.cc:71: url_,
net::URLRequestStatus());
please report a status that doesn't indicate success
URLRequestStatus(URLRequestStatus::FAILED, net::ERR_ABORTED)

https://codereview.chromium.org/567903002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_write_to_cache_job.cc:281: if
(version_->script_url() == url_) {
Lets add support for redirection of imports in a different cl. I don't think
this code change is sufficient for that and we'll absolutely need tests for this
case too.

[xiang, 2014-09-17 01:55:46]

Patchset #3 (id:40001) has been deleted

[xiang, 2014-09-17 01:56:06]

Patchset #3 (id:60001) has been deleted

[xiang, 2014-09-17 02:39:26]

PTAL, thanks!

https://codereview.chromium.org/567903002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/567903002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_write_to_cache_job.cc:71: url_,
net::URLRequestStatus());
On 2014/09/16 19:42:40, michaeln wrote:
> please report a status that doesn't indicate success
> URLRequestStatus(URLRequestStatus::FAILED, net::ERR_ABORTED) 

Done. Sorry for this mistake.

https://codereview.chromium.org/567903002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_write_to_cache_job.cc:281: if
(version_->script_url() == url_) {
On 2014/09/16 19:42:40, michaeln wrote:
> Lets add support for redirection of imports in a different cl. I don't think
> this code change is sufficient for that and we'll absolutely need tests for
this
> case too.

OK. Done.

[michaeln1, 2014-09-17 19:34:46]

michaeln@google.com changed reviewers:
+ michaeln@google.com

[michaeln1, 2014-09-17 19:34:47]

lgtm, thanx for working on this!

[michaeln, 2014-09-17 19:35:35]

lgtm 2 (doh, i keep doing that)

[xiang, 2014-09-22 07:54:13]

Patchset #4 (id:100001) has been deleted

[xiang, 2014-09-22 07:56:16]

The CQ bit was checked by xiang.long@intel.com

[commit-bot: I haz the power, 2014-09-22 07:56:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/567903002/120001

[commit-bot: I haz the power, 2014-09-22 08:49:12]

Committed patchset #4 (id:120001) as ef03516f1af815ef0d99a8dd302a73c566bc9741

[commit-bot: I haz the power, 2014-09-22 08:49:59]

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

[horo, 2014-09-24 12:55:31]

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

[horo, 2014-09-24 12:55:32]

https://codereview.chromium.org/567903002/diff/120001/content/browser/service...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/567903002/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:360: NOTREACHED();
Why don't we handle other error types?