LayoutTests/http/tests/serviceworker/unregister.html - Issue 362123003: ServiceWorker: unregister's scope argument should be optional

Side by Side Diff: LayoutTests/http/tests/serviceworker/unregister.html

Issue 362123003: ServiceWorker: unregister's scope argument should be optional (Closed) Base URL: svn://svn.chromium.org/blink/trunk

Patch Set: Created 6 years, 5 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

OLD	NEW
1 <!DOCTYPE html>	1 <!DOCTYPE html>

2 <script src="../resources/testharness.js"></script>	2 <script src="../resources/testharness.js"></script>

3 <script src="../resources/testharnessreport.js"></script>	3 <script src="../resources/testharnessreport.js"></script>

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

4 <script>	5 <script>

5 (function() {	6 (function() {

6 var t = async_test('Unregistering out of origin');	7 var t = async_test('Unregistering out of origin');

7 navigator.serviceWorker.unregister('http://example.com/*').then(	8 navigator.serviceWorker.unregister('http://example.com/*').then(

8 t.step_func(function() {	9 t.step_func(function() {

9 assert_unreached('unregistering out of origin should fail');	10 assert_unreached('unregistering out of origin should fail');

10 }),	11 }),

11 t.step_func(function(reason) {	12 t.step_func(function(reason) {

12 assert_equals(reason.name, 'SecurityError',	13 assert_equals(reason.name, 'SecurityError',

13 'unregistering out of origins should fail');	14 'unregistering out of origins should fail');

(...skipping 35 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
49 assert_equals(value, undefined,	50 assert_equals(value, undefined,

50 'successful unregistration should succeed ' +	51 'successful unregistration should succeed ' +

51 'with no value');	52 'with no value');

52 t.done();	53 t.done();

53 },	54 },

54 t.step_func(function(reason) {	55 t.step_func(function(reason) {

55 assert_unreached('unregistration should succeed.');	56 assert_unreached('unregistration should succeed.');

56 })	57 })

57 );	58 );

58 }());	59 }());

	60

	61 async_test(function(t) {

	62 function contains(string, substring) {

	63 return string.indexOf(substring) !== -1;

	64 }

	65

	66 service_worker_unregister_and_register(t, 'resources/simple-intercept-worker .js', '/*')
	dominicc (has gone to gerrit) 2014/07/02 02:10:51 The line wrapping in this addition is inconsistent The line wrapping in this addition is inconsistent; it mostly adheres to 80 columns, except where it doesn't. jsbell 2014/07/02 18:44:22 Done. Show quoted text On 2014/07/02 02:10:51, dominicc wrote: > The line wrapping in this addition is inconsistent; it mostly adheres to 80 > columns, except where it doesn't. Done.
	67 .then(t.step_func(function() {
	dominicc (has gone to gerrit) 2014/07/02 02:10:51 I think we should not indent here. asanka wrote: I think we should not indent here. asanka wrote: mumble(). then( which also works out OK when you get used to the . and then being split up. Maybe we should adopt his style? He also made abstemious use of step_func FWIW, and I am inclined to think that's more readable. We could make with_iframe return a function if it doesn't already. jsbell 2014/07/02 18:44:22 Hrm. Putting the '.' on the leading line is fine. Show quoted text On 2014/07/02 02:10:51, dominicc wrote: > I think we should not indent here. > > asanka wrote: > > mumble(). > then( > > which also works out OK when you get used to the . and then being split up. > Maybe we should adopt his style? Hrm. Putting the '.' on the leading line is fine. Without the indent there's nothing that makes it apparent this is a promise chain - it's much less readable to me. Show quoted text > He also made abstemious use of step_func FWIW, and I am inclined to think that's > more readable. I agree it's more readable without, but leaving out step_func is just asking for pain later. For example, when running the IDB tests submitted to the W3C there were a few tests Chrome failed due to timeouts. It took grovelling through the test to find the missing step_func wrappers to get a meaningful failure from the test. If we leave out step_func then we're making more work for those who want to use our tests to validate their impls.
	68 return with_iframe('resources/simple.html');

	69 }))

	70 .then(t.step_func(function(iframe) {

	71 assert_true(contains(iframe.contentDocument.body.innerHTML, 'interce pted'),
	dominicc (has gone to gerrit) 2014/07/02 02:10:51 Maybe putting that as the whole title would make t Maybe putting that as the whole title would make this smaller (no contains; document.title instead of document.body.innerHTML)
	72 'registered worker should override content');

	73 return navigator.serviceWorker.unregister();

	74 }))

	75 .then(t.step_func(function(value) {

	76 assert_equals(value, undefined,

	77 'unregister with default scope should succeed');

	78 }))

	79 .then(t.step_func(function() {

	80 return with_iframe('resources/simple.html');

	81 }))

	82 .then(t.step_func(function(iframe) {

	83 assert_false(contains(iframe.contentDocument.body.innerHTML, 'interc epted'),

	84 'unregistered worker should not override content');
	michaeln 2014/07/01 21:14:03 i think this tests may be hitting on much more tha i think this tests may be hitting on much more than the default param value, i think its hitting on things that aren't yet implemented, about unregistering sw that are inuse vs notinuse (kinda complicated things). a different way to verify unreg understand the default as "/" could be to verify that .state transitions to 'deprecated' in the sw instance returned by register? jsbell* 2014/07/02 00:18:50 Makes sense - I'll give that a whirl. Show quoted text On 2014/07/01 21:14:03, michaeln wrote: > i think this tests may be hitting on much more than the default param value, i > think its hitting on things that aren't yet implemented, about unregistering sw > that are inuse vs notinuse (kinda complicated things). > > a different way to verify unreg understand the default as "/" could be to > verify that .state transitions to 'deprecated' in the sw instance returned by > register? Makes sense - I'll give that a whirl. jsbell* 2014/07/02 18:44:22 It appears that our [[Unregister]] implementation Show quoted text On 2014/07/02 00:18:50, jsbell wrote: > On 2014/07/01 21:14:03, michaeln wrote: > > i think this tests may be hitting on much more than the default param value, i > > think its hitting on things that aren't yet implemented, about unregistering > sw > > that are inuse vs notinuse (kinda complicated things). > > > > a different way to verify unreg understand the default as "/" could be to > > verify that .state transitions to 'deprecated' in the sw instance returned by > > register? > > Makes sense - I'll give that a whirl. It appears that our [[Unregister]] implementation matches the spec in that the [[Registration]]'s activeVersion is not set to "redundant". That seems like a spec bug, but maybe there's some subtlety I'm missing. The latest patch on this issue works if I make this addition on the Chromium side in service_worker_unregister_job.cc: + if (registration->active_version()) { + registration->active_version()->SetStatus( + ServiceWorkerVersion::REDUNDANT); + } ... but there's no-one around at the moment to bounce this off of. :P michaeln* 2014/07/02 19:03:33 hmmm, i certainly had the expectation that any ver Show quoted text > It appears that our [[Unregister]] implementation matches the spec in that the > [[Registration]]'s activeVersion is not set to "redundant". > > That seems like a spec bug, but maybe there's some subtlety I'm missing. hmmm, i certainly had the expectation that any versions associated with an unregisterted registration would claim to be redundant thereafter, that doesn't mean they shouldn't continue to function for pages that are 'controlled' by them, just that the .state should change to 'redundant'. wdyt, open an issue to possibly tweak that in the spec?
	85 t.done();

	86 }));

	87 }, 'Unregistering with default scope');

59 </script>	88 </script>

OLD	NEW

« no previous file with comments | « no previous file | Source/modules/serviceworkers/ServiceWorkerContainer.h » ('j') | no next file with comments »