Don't speculatively start ServiceWorker if it doesn't have FetchEvent handler

Don't speculatively start ServiceWorker if it doesn't have FetchEvent handler

ServiceWorkers without FetchEvent handlers should not be started up on navigation, see https://crbug.com/616502#c38

BUG=616502, 640132
Committed: https://crrev.com/c2ace594258d2912d8732b263aabb13594fed912
Cr-Commit-Position: refs/heads/master@{#422796}

[horo, 2016-10-04 01:42:43]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-04 01:43:11]

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

[horo, 2016-10-04 03:08:08]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-04 03:08:11]

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

[horo, 2016-10-04 04:26:48]

Description was changed from

==========
Don't speculatively start ServiceWorker if it doesn't have FetchEvent handler

BUG=616502
==========

to

==========
Don't speculatively start ServiceWorker if it doesn't have FetchEvent handler

BUG=616502,640132
==========

[horo, 2016-10-04 04:27:28]

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

[horo, 2016-10-04 04:27:29]

falken@
Could you please review this?

[commit-bot: I haz the power, 2016-10-04 04:29:21]

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

[commit-bot: I haz the power, 2016-10-04 04:29:22]

Dry run: This issue passed the CQ dry run.

[falken, 2016-10-04 04:50:51]

Maybe a brief justification in the CL description would help for future
maintainers ("Service workers without fetch event handlers should not started up
on navigation, see bug 605844.")

https://codereview.chromium.org/2393503002/diff/20001/chrome/browser/chrome_s...
File chrome/browser/chrome_service_worker_browsertest.cc (right):

https://codereview.chromium.org/2393503002/diff/20001/chrome/browser/chrome_s...
chrome/browser/chrome_service_worker_browsertest.cc:554: }
nit: add newline

https://codereview.chromium.org/2393503002/diff/20001/chrome/browser/chrome_s...
chrome/browser/chrome_service_worker_browsertest.cc:573: 0 /* SERVICE_WORKER_OK
*/, 1);
nit: how about using SERVICE_WORKER_OK directly?

https://codereview.chromium.org/2393503002/diff/20001/chrome/browser/chrome_s...
chrome/browser/chrome_service_worker_browsertest.cc:576:
embedded_test_server()->base_url());
Since StopWorker is async, it seems like this function should have a callback
and we wait until it's done. No?

https://codereview.chromium.org/2393503002/diff/20001/chrome/browser/chrome_s...
chrome/browser/chrome_service_worker_browsertest.cc:584: "DOWN",
nit: Can you preserve this string on one line for greppability? i.e., make a
const string. (and below)

https://codereview.chromium.org/2393503002/diff/20001/chrome/browser/chrome_s...
chrome/browser/chrome_service_worker_browsertest.cc:611: 0 /* SERVICE_WORKER_OK
*/, 1);
const string and SERVICE_WORKER_OK here too

https://codereview.chromium.org/2393503002/diff/20001/chrome/browser/chrome_s...
chrome/browser/chrome_service_worker_browsertest.cc:616:
WriteFile(FILE_PATH_LITERAL("sw.js"), "// no fetch event handler.");
This doesn't need mock headers with javascript content type?

https://codereview.chromium.org/2393503002/diff/20001/chrome/browser/chrome_s...
chrome/browser/chrome_service_worker_browsertest.cc:625: "DOWN",
const string

[horo, 2016-10-04 06:48:26]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-04 06:48:36]

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

[horo, 2016-10-04 06:53:30]

Patchset #3 (id:40001) has been deleted

[horo, 2016-10-04 06:53:39]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

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

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

[horo, 2016-10-04 06:54:07]

https://codereview.chromium.org/2393503002/diff/20001/chrome/browser/chrome_s...
File chrome/browser/chrome_service_worker_browsertest.cc (right):

https://codereview.chromium.org/2393503002/diff/20001/chrome/browser/chrome_s...
chrome/browser/chrome_service_worker_browsertest.cc:554: }
On 2016/10/04 04:50:51, falken wrote:
> nit: add newline

Done.

https://codereview.chromium.org/2393503002/diff/20001/chrome/browser/chrome_s...
chrome/browser/chrome_service_worker_browsertest.cc:573: 0 /* SERVICE_WORKER_OK
*/, 1);
On 2016/10/04 04:50:51, falken wrote:
> nit: how about using SERVICE_WORKER_OK directly?

Unfortunately SERVICE_WORKER_OK is not available under /chrome/ folder.

https://codereview.chromium.org/2393503002/diff/20001/chrome/browser/chrome_s...
chrome/browser/chrome_service_worker_browsertest.cc:576:
embedded_test_server()->base_url());
On 2016/10/04 04:50:51, falken wrote:
> Since StopWorker is async, it seems like this function should have a callback
> and we wait until it's done. No?

Even if the worker is still stopping, we record the same
UMA("ServiceWorker.StartWorker.StatusByPurpose_NAVIGATION_HINT_LINK_MOUSE_DOWN"),
So there is no problem.

https://codereview.chromium.org/2393503002/diff/20001/chrome/browser/chrome_s...
chrome/browser/chrome_service_worker_browsertest.cc:584: "DOWN",
On 2016/10/04 04:50:51, falken wrote:
> nit: Can you preserve this string on one line for greppability? i.e., make a
> const string. (and below)

Done.

https://codereview.chromium.org/2393503002/diff/20001/chrome/browser/chrome_s...
chrome/browser/chrome_service_worker_browsertest.cc:611: 0 /* SERVICE_WORKER_OK
*/, 1);
On 2016/10/04 04:50:51, falken wrote:
> const string and SERVICE_WORKER_OK here too

Done.

https://codereview.chromium.org/2393503002/diff/20001/chrome/browser/chrome_s...
chrome/browser/chrome_service_worker_browsertest.cc:616:
WriteFile(FILE_PATH_LITERAL("sw.js"), "// no fetch event handler.");
On 2016/10/04 04:50:51, falken wrote:
> This doesn't need mock headers with javascript content type?

Ah, we don't need scope.html.mock-http-headers.

embedded_test_server supports both application/javascript and text/html.
https://cs.chromium.org/chromium/src/net/test/embedded_test_server/request_ha...

https://codereview.chromium.org/2393503002/diff/20001/chrome/browser/chrome_s...
chrome/browser/chrome_service_worker_browsertest.cc:625: "DOWN",
On 2016/10/04 04:50:51, falken wrote:
> const string

Done.

[horo, 2016-10-04 06:57:03]

Description was changed from

==========
Don't speculatively start ServiceWorker if it doesn't have FetchEvent handler

BUG=616502,640132
==========

to

==========
Don't speculatively start ServiceWorker if it doesn't have FetchEvent handler

ServiceWorkers without fetch event handlers should not started up on navigation,
see https://crbug.com/616502#c38


BUG=616502,640132
==========

[horo, 2016-10-04 06:57:42]

Description was changed from

==========
Don't speculatively start ServiceWorker if it doesn't have FetchEvent handler

ServiceWorkers without fetch event handlers should not started up on navigation,
see https://crbug.com/616502#c38


BUG=616502,640132
==========

to

==========
Don't speculatively start ServiceWorker if it doesn't have FetchEvent handler

ServiceWorkers without FetchEvent handlers should not started up on navigation,
see https://crbug.com/616502#c38


BUG=616502,640132
==========

[falken, 2016-10-04 07:23:52]

lgtm

typo in CL Description "not started up" -> "not be started up"

[horo, 2016-10-04 08:05:22]

Description was changed from

==========
Don't speculatively start ServiceWorker if it doesn't have FetchEvent handler

ServiceWorkers without FetchEvent handlers should not started up on navigation,
see https://crbug.com/616502#c38


BUG=616502,640132
==========

to

==========
Don't speculatively start ServiceWorker if it doesn't have FetchEvent handler

ServiceWorkers without FetchEvent handlers should not be started up on
navigation, see https://crbug.com/616502#c38


BUG=616502,640132
==========

[commit-bot: I haz the power, 2016-10-04 08:10:55]

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

[commit-bot: I haz the power, 2016-10-04 08:10:56]

Dry run: This issue passed the CQ dry run.

[horo, 2016-10-04 08:18:12]

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

[horo, 2016-10-04 08:18:12]

jochen@
Could you please review chrome/browser/chrome_service_worker_browsertest.cc?

[jochen (gone - plz use gerrit), 2016-10-04 14:49:51]

lgtm

[horo, 2016-10-04 15:04:30]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2016-10-04 15:04:54]

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-04 15:09:02]

Description was changed from

==========
Don't speculatively start ServiceWorker if it doesn't have FetchEvent handler

ServiceWorkers without FetchEvent handlers should not be started up on
navigation, see https://crbug.com/616502#c38


BUG=616502,640132
==========

to

==========
Don't speculatively start ServiceWorker if it doesn't have FetchEvent handler

ServiceWorkers without FetchEvent handlers should not be started up on
navigation, see https://crbug.com/616502#c38


BUG=616502,640132
==========

[commit-bot: I haz the power, 2016-10-04 15:09:03]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-10-04 15:11:07]

Description was changed from

==========
Don't speculatively start ServiceWorker if it doesn't have FetchEvent handler

ServiceWorkers without FetchEvent handlers should not be started up on
navigation, see https://crbug.com/616502#c38


BUG=616502,640132
==========

to

==========
Don't speculatively start ServiceWorker if it doesn't have FetchEvent handler

ServiceWorkers without FetchEvent handlers should not be started up on
navigation, see https://crbug.com/616502#c38

BUG=616502,640132

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

[commit-bot: I haz the power, 2016-10-04 15:11:08]

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