LayoutTests/http/tests/serviceworker/service-worker-gc.html - Issue 330173003: Make ServiceWorker an ActiveDOMObject

Side by Side Diff: LayoutTests/http/tests/serviceworker/service-worker-gc.html

Issue 330173003: Make ServiceWorker an ActiveDOMObject (Closed) Base URL: svn://svn.chromium.org/blink/trunk

Patch Set: revise test Created 6 years, 6 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch | Annotate | Revision Log

« no previous file with comments | « LayoutTests/http/tests/serviceworker/fetch-event.html ('k') | LayoutTests/http/tests/serviceworker/service-worker-gc-expected.txt » ('j') | no next file with comments »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
(Empty)
	1 <!DOCTYPE html>

	2 <script src="/js-test-resources/js-test.js"></script>

	3 <body>

	4 <script>

	5 (function() {
	dominicc (has gone to gerrit) 2014/06/14 20:02:47 I think that this test would be OK without this cl I think that this test would be OK without this closure. Up to you. Saves you one level of indent. falken 2014/06/16 00:58:57 Done. Show quoted text On 2014/06/14 20:02:47, dominicc wrote: > I think that this test would be OK without this closure. Up to you. Saves you > one level of indent. Done.
	6 window.jsTestIsAsync = true;

	7 description('Test that a registered Service Worker with an event handler is not garbage collected prematurely');

	8 var worker = 'resources/empty-worker.js';

	9 var scope = 'gc';

	10 swObservation = null;

	11 failed = false;

	12

	13 unregister_and_register(worker, scope).then(onRegister);

	14

	15 function unregister_and_register(url, scope) {

	16 return navigator.serviceWorker.unregister(scope).then(function() {

	17 return navigator.serviceWorker.register(url, { scope: scope });

	18 }).catch(function(error) {

	19 debug('Could not register worker: ' + error);

	20 finishJSTest();

	21 });

	22 }

	23

	24 function fail(message) {
	dominicc (has gone to gerrit) 2014/06/14 20:02:47 Doesn't the thing that provides debug also have th Doesn't the thing that provides debug also have things like fail and the "test passed" printer? falken 2014/06/16 00:58:57 Done. Show quoted text On 2014/06/14 20:02:47, dominicc wrote: > Doesn't the thing that provides debug also have things like fail and the "test > passed" printer? Done.
	25 debug('FAIL: ' + message);

	26 failed = true;

	27 }

	28

	29 function collectGarbage(shouldBeCollected, message) {

	30 gc();

	31 if (swObservation.wasCollected != shouldBeCollected)

	32 fail(message);

	33 }

	34

	35 function onUnregister() {

	36 // FIXME: The expectation should be reversed, but our implementation cur rently fails it.
	dominicc (has gone to gerrit) 2014/06/14 20:02:47 I think you can make the FIXME clearer, something I think you can make the FIXME clearer, something like: Assert that the ServiceWorker is collected when the redundant state is implemented. falken 2014/06/16 00:58:57 Done. Show quoted text On 2014/06/14 20:02:47, dominicc wrote: > I think you can make the FIXME clearer, something like: > > Assert that the ServiceWorker is collected when the redundant state is > implemented. Done.
	37 // When properly implemented, the SW not leak here. It may be better to move this test into a separate, targeted file.

	38 collectGarbage(false, 'Service Worker should not leak after unregistrati on');

	39 if (!failed)

	40 debug('Test passed');

	41 finishJSTest();

	42 }

	43

	44 function onRegister(sw) {

	45 if (window.internals)

	46 swObservation = internals.observeGC(sw);

	47 if (!swObservation) {
	dominicc (has gone to gerrit) 2014/06/14 20:02:47 It might be simpler to test for window.internals a It might be simpler to test for window.internals at the top of the file and fail the test fast if it is missing. falken 2014/06/16 00:58:57 Done. Show quoted text On 2014/06/14 20:02:47, dominicc wrote: > It might be simpler to test for window.internals at the top of the file and fail > the test fast if it is missing. Done.
	48 fail('This test requires internals.observeGC');

	49 finishJSTest();

	50 return;

	51 }

	52

	53 sw.addEventListener('statechange', onStateChange);

	54 setTimeout(collectGarbage.bind(undefined, false, 'Service Worker should not be gc\'d after registration'), 0);
	dominicc (has gone to gerrit) 2014/06/14 20:02:47 setTimeout and the statechange events inevitably r setTimeout and the statechange events inevitably race. I think that is unavoidable and fine. If you want to slow the Service Worker down in a state transition you could do that with waitUntil and timeouts or postMessage (when we implement "redundant" it is probably worth testing that a Service Worker sharing a Message Channel does not leak.)
	55 }

	56

	57 function onStateChange(event) {

	58 collectGarbage(false, 'Service Worker should not be gc\'d in ' + event.t arget.state + ' state');
	dominicc (has gone to gerrit) 2014/06/14 20:02:47 The calls to collectGarbage are confusing in that The calls to collectGarbage are confusing in that collectGarbage(false, ...) actually collects garbage; and collectGarbage always does other stuff. I think it will be more readable to simply do two very literal lines: gc(); shouldBeFalse('serviceWorker.wasCollected'); If the test breaks it may time out; that is not ideal but it is OK. falken 2014/06/16 00:58:57 OK, done. I thought it'd be nice to avoid time out Show quoted text On 2014/06/14 20:02:47, dominicc wrote: > The calls to collectGarbage are confusing in that collectGarbage(false, ...) > actually collects garbage; and collectGarbage always does other stuff. > > I think it will be more readable to simply do two very literal lines: > > gc(); > shouldBeFalse('serviceWorker.wasCollected'); > > If the test breaks it may time out; that is not ideal but it is OK. OK, done. I thought it'd be nice to avoid time out and also print where the assert failed, but it may not be worth the complexity. Now with the generic shouldBeFalse, the race between setTimeout and statechange events doesn't matter so the code is simpler.
	59 if (event.target.state != 'active')

	60 return;

	61 navigator.serviceWorker.unregister(scope).then(onUnregister);

	62 }

	63 }());

	64 </script>

	65 </body>

OLD	NEW