Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
425413002:
[ServiceWorker] Tests for Cache (Closed)
|
Created:
6 years, 4 months ago by asanka Modified:
6 years, 2 months ago CC:
blink-reviews, michaeln, jsbell+serviceworker_chromium.org, kenjibaheux+watch_chromium.org, tzik, serviceworker-reviews, nhiroki, falken, kinuko+serviceworker, horo+watch_chromium.org, alecflett+watch_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Project:
blink Visibility:
Public. |
Description[ServiceWorker] Tests for Cache
These tests exercise the following methods on Cache objects:
* add
* addAll
* put
* match
* matchAll
BUG=374802
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184242
Patch Set 1 #
Total comments: 6
Patch Set 2 : Address comments and update tests with spec. #Patch Set 3 : More tests. #
Total comments: 53
Patch Set 4 : Address comments. #
Total comments: 8
Patch Set 5 : Comments + 'Vary' header tests + refactoring #Patch Set 6 : Fix typo #Patch Set 7 : Update interfaces test #Patch Set 8 : #Patch Set 9 : Merge with ToT #Patch Set 10 : Add test expectations. #Patch Set 11 : #
Total comments: 2
Patch Set 12 : #
Total comments: 1
Patch Set 13 : CacheStorage.{create => open} #
Total comments: 19
Patch Set 14 : Updates tests to comply with spec changes #
Total comments: 20
Patch Set 15 : Enable more tests #Messages
Total messages: 46 (10 generated)
Quick skim; someone more familiar with cache API should review. https://codereview.chromium.org/425413002/diff/1/LayoutTests/http/tests/servi... File LayoutTests/http/tests/serviceworker/resources/cache-batch-test-worker.js (right): https://codereview.chromium.org/425413002/diff/1/LayoutTests/http/tests/servi... LayoutTests/http/tests/serviceworker/resources/cache-batch-test-worker.js:37: assert_true(e instanceof TypeError, There's a repeated pattern here: <operation> .then(t.unreached_func(...)) .catch(t.step_func(function(e) { assert_true(e instanceof TypeError, ...); })); Maybe we should introduce a helper similar to assert_throws? https://codereview.chromium.org/425413002/diff/1/LayoutTests/http/tests/servi... File LayoutTests/http/tests/serviceworker/resources/cache-put-test-worker.js (right): https://codereview.chromium.org/425413002/diff/1/LayoutTests/http/tests/servi... LayoutTests/http/tests/serviceworker/resources/cache-put-test-worker.js:6: 'Hello, world!', { This line break is weird. https://codereview.chromium.org/425413002/diff/1/LayoutTests/http/tests/servi... File LayoutTests/http/tests/serviceworker/resources/interfaces-worker.js (right): https://codereview.chromium.org/425413002/diff/1/LayoutTests/http/tests/servi... LayoutTests/http/tests/serviceworker/resources/interfaces-worker.js:45: put: 'function', delete: 'function', Split this line?
https://codereview.chromium.org/425413002/diff/1/LayoutTests/http/tests/servi... File LayoutTests/http/tests/serviceworker/resources/cache-batch-test-worker.js (right): https://codereview.chromium.org/425413002/diff/1/LayoutTests/http/tests/servi... LayoutTests/http/tests/serviceworker/resources/cache-batch-test-worker.js:37: assert_true(e instanceof TypeError, On 2014/08/01 00:02:25, jsbell wrote: > There's a repeated pattern here: > > <operation> > .then(t.unreached_func(...)) > .catch(t.step_func(function(e) { > assert_true(e instanceof TypeError, ...); > })); > > Maybe we should introduce a helper similar to assert_throws? > I added a convenience method for the .catch(...<check for exception>) pattern. https://codereview.chromium.org/425413002/diff/1/LayoutTests/http/tests/servi... File LayoutTests/http/tests/serviceworker/resources/cache-put-test-worker.js (right): https://codereview.chromium.org/425413002/diff/1/LayoutTests/http/tests/servi... LayoutTests/http/tests/serviceworker/resources/cache-put-test-worker.js:6: 'Hello, world!', { On 2014/08/01 00:02:25, jsbell wrote: > This line break is weird. Indeed. Fixed. https://codereview.chromium.org/425413002/diff/1/LayoutTests/http/tests/servi... File LayoutTests/http/tests/serviceworker/resources/interfaces-worker.js (right): https://codereview.chromium.org/425413002/diff/1/LayoutTests/http/tests/servi... LayoutTests/http/tests/serviceworker/resources/interfaces-worker.js:45: put: 'function', delete: 'function', On 2014/08/01 00:02:25, jsbell wrote: > Split this line? Done.
Note: this is dependent on the 'expect_promise_throws' function over in https://codereview.chromium.org/430993002/
looking pretty good. I'm worried about the churn in https://github.com/slightlyoff/ServiceWorker/issues/372 I haven't tried running this to see how badly we fail yet. :) Are you planning to add cases for delete() in this CL or another one? https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/resources/cache-add-test-worker.js (right): https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-add-test-worker.js:15: 'Cache.add should throw a TypeError when more than one argument is ' + Interesting... it's unusual for a Web API to throw on extra args. It's in the spec, but I wonder why. I filed https://github.com/slightlyoff/ServiceWorker/issues/417 to track this. https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-add-test-worker.js:21: .then(t.step_func(function(result) { t.step_func should be unnecessary here, since a throw inside a then() will turn into a reject which promise_test rethrows. https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-add-test-worker.js:25: 'Cache.add should not fulfil until response ' + Nit: 'fulfill' ? Apparently this is an American vs. British thing; I'm Canadian so I'm fine either way. :) https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-add-test-worker.js:47: // Assumes the existence of simple.txt and blank.html in the same directory as nit: line length (80 cols) https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-add-test-worker.js:51: .then(t.step_func(verify_response_array_against_urls.bind(undefined, urls))); t.step_func should be unnecessary https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-add-test-worker.js:55: // Assumes the existence of simple.txt and blank.html in the same directory as nit: line length (80 cols) https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-add-test-worker.js:62: .then(t.step_func(verify_response_array_against_urls.bind(undefined, urls))); nit: line length (80 cols) https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-add-test-worker.js:62: .then(t.step_func(verify_response_array_against_urls.bind(undefined, urls))); t.step_func should be unnecessary https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-add-test-worker.js:76: nit: remove blank line https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-add-test-worker.js:80: .then(t.step_func(function(result) { t.step_func should be unnecessary https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-add-test-worker.js:117: function absolute_url(url) { This can just be: return new URL(url, self.location.href).toString(); https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/resources/cache-match-test-worker.js (right): https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-match-test-worker.js:55: { headers: { 'Cookies': 'is-for-cookie' } }), That's good enough for me! https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-match-test-worker.js:57: { headers: { 'Vary': 'Cookies' } }) But actually.. per style guide, no space after { or before } for single-line initializers. https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-match-test-worker.js:90: var cache = new Cache(); Just noticing that the constructor isn't actually in the spec, but https://github.com/slightlyoff/ServiceWorker/issues/372 says there will be one. Hopefully it stays! https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-match-test-worker.js:97: .then(t.step_func(function(result) { t.step_func shouldn't be necessary in a promise_test (here and below) https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-match-test-worker.js:102: .then(function() { Since these cases are independent, can we split this up rather than having it be one big promise_test? If something fails (or the impl is incomplete) it'd be nice to see 'pass / pass / pass / fail (reason #1) / pass / fail (reason #2)' in the output rather than just 'fail (reason #1)' https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-match-test-worker.js:162: 'Cache.matchAll should treat query as a URL and not just a string fragment.'); nit: line length (80 cols) https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-match-test-worker.js:166: return cache.matchAll('http://example.com/cat', { prefixMatch: true }); nit: line length (80 cols) https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-match-test-worker.js:171: [ entries.cat.response, nit: break after [ and before ] for multi-line initializers, per style guide https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-match-test-worker.js:178: return cache.matchAll('http://example.com/cat/', { prefixMatch: true }); nit: line length (80 cols) https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-match-test-worker.js:190: function put_all_requests_in_cache(cache) { Could be written as: return Promise.all(entries.map(function(e) { return cache.put(e.request, e.response); })); This also avoids using for...in to enumerate properties, which (while fine here) is one of those fragile bits of JS. https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-match-test-worker.js:199: function assert_array_equivalent(actual, expected, description) { Can you add a comment that this is comparing the contents of arrays ignoring order, since the match result order may be indeterminate? https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-match-test-worker.js:199: function assert_array_equivalent(actual, expected, description) { For consistency with assert_array_equals(), rename to: assert_array_equivalent() ? https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-match-test-worker.js:203: assert_greater_than_equal(actual.indexOf(expected[index]), 0, description); This could use assert_in_array() https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/resources/cache-put-test-worker.js (right): https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-put-test-worker.js:19: .then(t.step_func(function(result) { t.step_func should be unnecessary in promise_test (here and below) https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-put-test-worker.js:37: 'Cache.put should replace existing response with ' + Should this have an extra step that does a match() to verify the replacement? https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-put-test-worker.js:58: }, 'Cache.put with a request string'); Add another case with a relative URL string?
Hrm... we don't typically name the workers <base>-test-worker.js, just <base>-worker.js fetch-event-test-worker.js is the only exception. Are we setting a new naming standard, or should we drop the '-test' infix from these? (and in the other patch)
I renamed the workers to be cache-foo-worker.js. https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/resources/cache-add-test-worker.js (right): https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-add-test-worker.js:21: .then(t.step_func(function(result) { On 2014/08/15 at 20:49:33, jsbell wrote: > t.step_func should be unnecessary here, since a throw inside a then() will turn into a reject which promise_test rethrows. Done. https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-add-test-worker.js:25: 'Cache.add should not fulfil until response ' + On 2014/08/15 at 20:49:33, jsbell wrote: > Nit: 'fulfill' ? > > Apparently this is an American vs. British thing; I'm Canadian so I'm fine either way. :) Changed to fulfill :) https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-add-test-worker.js:47: // Assumes the existence of simple.txt and blank.html in the same directory as On 2014/08/15 at 20:49:33, jsbell wrote: > nit: line length (80 cols) Done. https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-add-test-worker.js:51: .then(t.step_func(verify_response_array_against_urls.bind(undefined, urls))); On 2014/08/15 at 20:49:32, jsbell wrote: > t.step_func should be unnecessary Removed. https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-add-test-worker.js:55: // Assumes the existence of simple.txt and blank.html in the same directory as On 2014/08/15 at 20:49:33, jsbell wrote: > nit: line length (80 cols) Done. https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-add-test-worker.js:62: .then(t.step_func(verify_response_array_against_urls.bind(undefined, urls))); On 2014/08/15 at 20:49:32, jsbell wrote: > t.step_func should be unnecessary Done. Also fixed line length. https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-add-test-worker.js:76: On 2014/08/15 at 20:49:32, jsbell wrote: > nit: remove blank line Done. https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-add-test-worker.js:80: .then(t.step_func(function(result) { On 2014/08/15 at 20:49:33, jsbell wrote: > t.step_func should be unnecessary Removed. https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-add-test-worker.js:117: function absolute_url(url) { On 2014/08/15 at 20:49:33, jsbell wrote: > This can just be: > > return new URL(url, self.location.href).toString(); Done. I wasn't sure whether to use URL since it's a bit new. https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/resources/cache-match-test-worker.js (right): https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-match-test-worker.js:90: var cache = new Cache(); On 2014/08/15 at 20:49:33, jsbell wrote: > Just noticing that the constructor isn't actually in the spec, but https://github.com/slightlyoff/ServiceWorker/issues/372 says there will be one. Hopefully it stays! I replaced the Cache() constructor with a helper that creates a new temporary cache. https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-match-test-worker.js:97: .then(t.step_func(function(result) { On 2014/08/15 at 20:49:33, jsbell wrote: > t.step_func shouldn't be necessary in a promise_test (here and below) Removed. https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-match-test-worker.js:102: .then(function() { On 2014/08/15 20:49:33, jsbell wrote: > Since these cases are independent, can we split this up rather than having it be > one big promise_test? > > If something fails (or the impl is incomplete) it'd be nice to see 'pass / pass > / pass / fail (reason #1) / pass / fail (reason #2)' in the output rather than > just 'fail (reason #1)' Done. https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-match-test-worker.js:162: 'Cache.matchAll should treat query as a URL and not just a string fragment.'); On 2014/08/15 at 20:49:34, jsbell wrote: > nit: line length (80 cols) Done. https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-match-test-worker.js:166: return cache.matchAll('http://example.com/cat', { prefixMatch: true }); On 2014/08/15 at 20:49:34, jsbell wrote: > nit: line length (80 cols) Done. https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-match-test-worker.js:171: [ entries.cat.response, On 2014/08/15 at 20:49:33, jsbell wrote: > nit: break after [ and before ] for multi-line initializers, per style guide Done. https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-match-test-worker.js:178: return cache.matchAll('http://example.com/cat/', { prefixMatch: true }); On 2014/08/15 at 20:49:34, jsbell wrote: > nit: line length (80 cols) Done. https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-match-test-worker.js:190: function put_all_requests_in_cache(cache) { On 2014/08/15 at 20:49:34, jsbell wrote: > Could be written as: > > return Promise.all(entries.map(function(e) { > return cache.put(e.request, e.response); > })); > > This also avoids using for...in to enumerate properties, which (while fine here) is one of those fragile bits of JS. Ah. Thanks! Changed. https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-match-test-worker.js:199: function assert_array_equivalent(actual, expected, description) { On 2014/08/15 at 20:49:34, jsbell wrote: > Can you add a comment that this is comparing the contents of arrays ignoring order, since the match result order may be indeterminate? Added a comment. https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-match-test-worker.js:199: function assert_array_equivalent(actual, expected, description) { On 2014/08/15 20:49:34, jsbell wrote: > For consistency with assert_array_equals(), rename to: assert_array_equivalent() > ? Hmm. Were you going to suggest a different name? https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-match-test-worker.js:203: assert_greater_than_equal(actual.indexOf(expected[index]), 0, description); On 2014/08/15 at 20:49:34, jsbell wrote: > This could use assert_in_array() Done. https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/resources/cache-put-test-worker.js (right): https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-put-test-worker.js:19: .then(t.step_func(function(result) { On 2014/08/15 20:49:35, jsbell wrote: > t.step_func should be unnecessary in promise_test (here and below) Done. https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-put-test-worker.js:37: 'Cache.put should replace existing response with ' + On 2014/08/15 20:49:34, jsbell wrote: > Should this have an extra step that does a match() to verify the replacement? Done. https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-put-test-worker.js:58: }, 'Cache.put with a request string'); On 2014/08/15 20:49:34, jsbell wrote: > Add another case with a relative URL string? Done.
lgtm, with just a few nits https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/resources/cache-add-test-worker.js (right): https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-add-test-worker.js:117: function absolute_url(url) { On 2014/08/20 03:11:58, asanka wrote: > On 2014/08/15 at 20:49:33, jsbell wrote: > > This can just be: > > > > return new URL(url, self.location.href).toString(); > > Done. I wasn't sure whether to use URL since it's a bit new. Paranoia is good. :) But any browser that supports ServiceWorker/Cache should support URL, so we might as well use the new hotness. https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/resources/cache-match-test-worker.js (right): https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-match-test-worker.js:90: var cache = new Cache(); On 2014/08/20 03:11:58, asanka wrote: > On 2014/08/15 at 20:49:33, jsbell wrote: > > Just noticing that the constructor isn't actually in the spec, but > https://github.com/slightlyoff/ServiceWorker/issues/372 says there will be one. > Hopefully it stays! > > I replaced the Cache() constructor with a helper that creates a new temporary > cache. Great idea! https://codereview.chromium.org/425413002/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-match-test-worker.js:199: function assert_array_equivalent(actual, expected, description) { On 2014/08/20 03:11:58, asanka wrote: > On 2014/08/15 20:49:34, jsbell wrote: > > For consistency with assert_array_equals(), rename to: > assert_array_equivalent() > > ? > > Hmm. Were you going to suggest a different name? I must have been misreading it. Name is fine, sorry! https://codereview.chromium.org/425413002/diff/60001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/resources/cache-add-worker.js (right): https://codereview.chromium.org/425413002/diff/60001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-add-worker.js:20: 'Cache.add should throw a TypeError when more than one argument ' + Per https://github.com/slightlyoff/ServiceWorker/issues/417 this is no longer valid. This case can be removed (or we could flip the expectation, but *any* operation can accept/ignore extra args, nothing special about add()) https://codereview.chromium.org/425413002/diff/60001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-add-worker.js:98: nit: remove blank line https://codereview.chromium.org/425413002/diff/60001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-add-worker.js:143: return self.location.href.replace(/[^/]+$/, url); Hrm, looks like it's still not using URL()? https://codereview.chromium.org/425413002/diff/60001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/resources/worker-test-harness.js (right): https://codereview.chromium.org/425413002/diff/60001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/worker-test-harness.js:111: function create_temporary_cache(test, base_name) { This is great!
Note: this will need the -expected.txt files added, since most of the tests will fail at the moment.
Thanks! This is waiting on https://codereview.chromium.org/430993002/. In the meantime I added a few tests that exercise handling of the 'Vary' header and also changed the tests to not be nested at the expense of constructing more caches. https://codereview.chromium.org/425413002/diff/60001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/resources/cache-add-worker.js (right): https://codereview.chromium.org/425413002/diff/60001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-add-worker.js:20: 'Cache.add should throw a TypeError when more than one argument ' + On 2014/08/20 18:18:08, jsbell wrote: > Per https://github.com/slightlyoff/ServiceWorker/issues/417 this is no longer > valid. This case can be removed (or we could flip the expectation, but *any* > operation can accept/ignore extra args, nothing special about add()) Makes sense. Removed. https://codereview.chromium.org/425413002/diff/60001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-add-worker.js:98: On 2014/08/20 18:18:08, jsbell wrote: > nit: remove blank line Done. https://codereview.chromium.org/425413002/diff/60001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/cache-add-worker.js:143: return self.location.href.replace(/[^/]+$/, url); On 2014/08/20 18:18:08, jsbell wrote: > Hrm, looks like it's still not using URL()? Done. Now I'll have to go hunt down what I remember changing. Hmm. https://codereview.chromium.org/425413002/diff/60001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/resources/worker-test-harness.js (right): https://codereview.chromium.org/425413002/diff/60001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/worker-test-harness.js:111: function create_temporary_cache(test, base_name) { On 2014/08/20 18:18:08, jsbell wrote: > This is great! Thanks!
BTW, since the C++ Cache constructor is not implemented yet, converting the interfaces-worker.js usage over to use create_temporary_cache() might be a good idea.
On 2014/08/21 18:31:47, jsbell wrote: > BTW, since the C++ Cache constructor is not implemented yet, converting the > interfaces-worker.js usage over to use create_temporary_cache() might be a good > idea. Done.
asanka@chromium.org changed reviewers: + jkarlin@chromium.org
jkarlin: Here are the tests for Cache. Can you run them and check how they perform with your patches? I can land this with pass/fail expectations if there's nothing obviously wrong with the tests.
Everything but the first test in each fails right now. You can patch in https://codereview.chromium.org/580063003/ if you want to try it out yourself. I'd say go ahead and land and we can fix later if we find bugs. On Mon, Sep 29, 2014 at 3:19 PM, <asanka@chromium.org> wrote: > Reviewers: jsbell, gavinp, jkarlin, > > Message: > jkarlin: Here are the tests for Cache. Can you run them and check how they > perform with your patches? I can land this with pass/fail expectations if > there's nothing obviously wrong with the tests. > > > Description: > [ServiceWorker] Tests for Cache > > BUG=374802 > > Please review this at https://codereview.chromium.org/425413002/ > > SVN Base: svn://svn.chromium.org/blink/trunk > > Affected files (+680, -9 lines): > A + LayoutTests/http/tests/serviceworker/cache-add.html > A + LayoutTests/http/tests/serviceworker/cache-match.html > A + LayoutTests/http/tests/serviceworker/cache-put.html > A LayoutTests/http/tests/serviceworker/resources/cache-add-worker.js > A LayoutTests/http/tests/serviceworker/resources/cache-match-worker.js > A LayoutTests/http/tests/serviceworker/resources/cache-put-worker.js > M LayoutTests/http/tests/serviceworker/resources/interfaces-worker.js > M LayoutTests/http/tests/serviceworker/resources/worker-test-harness.js > > > To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On 2014/09/30 11:31:34, jkarlin wrote: > Everything but the first test in each fails right now. You can patch in > https://codereview.chromium.org/580063003/ if you want to try it out > yourself. I'd say go ahead and land and we can fix later if we find bugs. Ok. I've set expectations sans your patch since that's how it's going to land. Will flip the commit bit once the try bots clear.
The CQ bit was checked by asanka@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/425413002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/29599)
On 2014/10/01 00:16:44, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > win_blink_rel on tryserver.blink > (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/29599) Hmm. Looks like cache-match and cache-storage-keys are crashing on Windows. I'll take a look when I get in tomorrow.
https://codereview.chromium.org/425413002/diff/200001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/resources/cache-put-worker.js (right): https://codereview.chromium.org/425413002/diff/200001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/cache-put-worker.js:23: assert_equals(result, assert_equals does a === comparison which will fail here. The responses returned from these functions are to different but equivalent objects.
Patchset #12 (id:220001) has been deleted
Could you take another look? I had to make some substantial changes. https://codereview.chromium.org/425413002/diff/200001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/resources/cache-put-worker.js (right): https://codereview.chromium.org/425413002/diff/200001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/cache-put-worker.js:23: assert_equals(result, On 2014/10/03 15:10:52, jkarlin wrote: > assert_equals does a === comparison which will fail here. The responses > returned from these functions are to different but equivalent objects. Should be okay now. https://codereview.chromium.org/425413002/diff/240001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/cache-put-expected.txt (right): https://codereview.chromium.org/425413002/diff/240001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/cache-put-expected.txt:7: FAIL Cache.put with a relative URL assert_equals: Cache.put should accept a relative URL as the request. :[object].url expected "" but got "http://127.0.0.1:8000/serviceworker/resources/relative-url" Looks like https://github.com/slightlyoff/ServiceWorker/issues/469
Updated to use CacheStorage.open. I can haz review?
overall lgtm, a few nits/suggestions. https://codereview.chromium.org/425413002/diff/260001/LayoutTests/http/tests/... File LayoutTests/http/tests/resources/testharness-helpers.js (right): https://codereview.chromium.org/425413002/diff/260001/LayoutTests/http/tests/... LayoutTests/http/tests/resources/testharness-helpers.js:18: // only need to be wrapped in test.step()s if the promise chain contains test.step_func() ? https://codereview.chromium.org/425413002/diff/260001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/resources/cache-add-worker.js (right): https://codereview.chromium.org/425413002/diff/260001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/cache-add-worker.js:8: cache.add(''), This is an empty string, not "no arguments"; can you clarify what's being tested here? (If this changed to `caches.add()` I'd expect the test to PASS, like addAll does.) https://codereview.chromium.org/425413002/diff/260001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/cache-add-worker.js:55: .then(verify_response_array_against_urls.bind(undefined, urls)); You could move this .then out a level in the promise chain, i.e. .then(function(cache) { return cache.addAll(url); }) .then(verify....); https://codereview.chromium.org/425413002/diff/260001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/cache-add-worker.js:69: .then(verify_response_array_against_urls.bind(undefined, urls)); Ditto. https://codereview.chromium.org/425413002/diff/260001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/cache-add-worker.js:75: // resource does not. Grammar nit: "are existing ... does not." -> "are existing ... is not." Or "are existing ... does not." -> "exist. ... does not." https://codereview.chromium.org/425413002/diff/260001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/resources/cache-match-worker.js (right): https://codereview.chromium.org/425413002/diff/260001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/cache-match-worker.js:224: }, 'Cache.matchAll with string fragment \'http\' as query'); Fine as-is, but also feel free to use " here to avoid escaping. https://codereview.chromium.org/425413002/diff/260001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/cache-match-worker.js:360: return Promise.resolve(cache); You should just be able to return `cache` here. https://codereview.chromium.org/425413002/diff/260001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/resources/cache-put-worker.js (right): https://codereview.chromium.org/425413002/diff/260001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/cache-put-worker.js:19: return create_temporary_cache(t) Consider adding a helper: cache_test(function(t, cache) { ... }) That'd avoid a few lines of boilerplate and a few levels of indentation. https://codereview.chromium.org/425413002/diff/260001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/resources/worker-testharness.js (right): https://codereview.chromium.org/425413002/diff/260001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/worker-testharness.js:81: function create_temporary_cache(test, base_name) { Is it worth having the `base_name` param now, since there are no uses (that I saw)?
Patchset #14 (id:280001) has been deleted
https://codereview.chromium.org/425413002/diff/260001/LayoutTests/http/tests/... File LayoutTests/http/tests/resources/testharness-helpers.js (right): https://codereview.chromium.org/425413002/diff/260001/LayoutTests/http/tests/... LayoutTests/http/tests/resources/testharness-helpers.js:18: // only need to be wrapped in test.step()s if the promise chain contains On 2014/10/20 18:20:40, jsbell wrote: > test.step_func() ? Done. https://codereview.chromium.org/425413002/diff/260001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/resources/cache-add-worker.js (right): https://codereview.chromium.org/425413002/diff/260001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/cache-add-worker.js:8: cache.add(''), On 2014/10/20 18:20:40, jsbell wrote: > This is an empty string, not "no arguments"; can you clarify what's being tested > here? > > (If this changed to `caches.add()` I'd expect the test to PASS, like addAll > does.) Changed to cache.add(), but shouldn't that be equivalent to cache.addAll([undefined]) ? https://codereview.chromium.org/425413002/diff/260001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/cache-add-worker.js:55: .then(verify_response_array_against_urls.bind(undefined, urls)); On 2014/10/20 18:20:40, jsbell wrote: > You could move this .then out a level in the promise chain, i.e. > > .then(function(cache) { > return cache.addAll(url); > }) > .then(verify....); Transitioned to a cache_test() helper function which, I believe, resolves this. https://codereview.chromium.org/425413002/diff/260001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/cache-add-worker.js:69: .then(verify_response_array_against_urls.bind(undefined, urls)); On 2014/10/20 18:20:40, jsbell wrote: > Ditto. Yup. But obsolete due to the introduction of cache_test() helper. https://codereview.chromium.org/425413002/diff/260001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/cache-add-worker.js:75: // resource does not. On 2014/10/20 18:20:41, jsbell wrote: > Grammar nit: "are existing ... does not." -> "are existing ... is not." > Or "are existing ... does not." -> "exist. ... does not." Done. https://codereview.chromium.org/425413002/diff/260001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/resources/cache-match-worker.js (right): https://codereview.chromium.org/425413002/diff/260001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/cache-match-worker.js:224: }, 'Cache.matchAll with string fragment \'http\' as query'); On 2014/10/20 18:20:41, jsbell wrote: > Fine as-is, but also feel free to use " here to avoid escaping. Done. https://codereview.chromium.org/425413002/diff/260001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/cache-match-worker.js:360: return Promise.resolve(cache); On 2014/10/20 18:20:41, jsbell wrote: > You should just be able to return `cache` here. Done. https://codereview.chromium.org/425413002/diff/260001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/resources/cache-put-worker.js (right): https://codereview.chromium.org/425413002/diff/260001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/cache-put-worker.js:19: return create_temporary_cache(t) On 2014/10/20 18:20:41, jsbell wrote: > Consider adding a helper: cache_test(function(t, cache) { ... }) > > That'd avoid a few lines of boilerplate and a few levels of indentation. Good suggestion. Done. https://codereview.chromium.org/425413002/diff/260001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/resources/worker-testharness.js (right): https://codereview.chromium.org/425413002/diff/260001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/worker-testharness.js:81: function create_temporary_cache(test, base_name) { On 2014/10/20 18:20:41, jsbell wrote: > Is it worth having the `base_name` param now, since there are no uses (that I > saw)? Removed. https://codereview.chromium.org/425413002/diff/300001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/resources/cache-add-worker.js (right): https://codereview.chromium.org/425413002/diff/300001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/cache-add-worker.js:5: return assert_promise_rejects( There are a few places where https://github.com/slightlyoff/ServiceWorker/issues/523 is relevant. I.e. it's not clear whether the cache.add() call is going to throw or whether the returned promise is going to be rejected. https://codereview.chromium.org/425413002/diff/300001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/resources/cache-match-worker.js (right): https://codereview.chromium.org/425413002/diff/300001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/cache-match-worker.js:129: '[https://fetch.spec.whatwg.org/#concept-body-used-flag] ' + I've annotated assertions which are referring to specs other than service-worker.
https://codereview.chromium.org/425413002/diff/260001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/resources/cache-add-worker.js (right): https://codereview.chromium.org/425413002/diff/260001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/cache-add-worker.js:8: cache.add(''), On 2014/10/22 22:35:31, asanka wrote: > On 2014/10/20 18:20:40, jsbell wrote: > > This is an empty string, not "no arguments"; can you clarify what's being > tested > > here? > > > > (If this changed to `caches.add()` I'd expect the test to PASS, like addAll > > does.) > > Changed to cache.add(), but shouldn't that be equivalent to > cache.addAll([undefined]) ? No - the spec prose about delegating to addAll() applies after the arity checks implied by the Web IDL method signature.
lgtm! https://codereview.chromium.org/425413002/diff/300001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/cache-add-expected.txt (right): https://codereview.chromium.org/425413002/diff/300001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/cache-add-expected.txt:3: FAIL Cache.add with no arguments Failed to execute 'add' on 'Cache': 1 argument required, but only 0 present.(stack: TypeError: Failed to execute 'add' on 'Cache': 1 argument required, but only 0 present. Weird - is it actually *throwing* instead of rejecting? I'll dig in - the bindings code should be taking care of this automagically. https://codereview.chromium.org/425413002/diff/300001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/cache-add-expected.txt:9: FAIL Cache.add with a used Request object assert_throws: Cache.add with a Request object with a used body should reject with a TypeError. function "function () { throw e; }" threw object "NotFoundError: Entry was not found." ("NotFoundError") expected object "TypeError" ("TypeError") Any idea why this is NotFoundError instead of NotSupportedError (not implemented) ? https://codereview.chromium.org/425413002/diff/300001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/resources/cache-add-worker.js (right): https://codereview.chromium.org/425413002/diff/300001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/cache-add-worker.js:5: return assert_promise_rejects( On 2014/10/22 22:35:32, asanka wrote: > There are a few places where > https://github.com/slightlyoff/ServiceWorker/issues/523 is relevant. I.e. it's > not clear whether the cache.add() call is going to throw or whether the returned > promise is going to be rejected. The bindings layer should be doing the arity check and rejecting instead of throwing, since the behavior here is defined in Web IDL (not the SW spec - it happens before it hits the prose). https://codereview.chromium.org/425413002/diff/300001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/resources/cache-put-worker.js (right): https://codereview.chromium.org/425413002/diff/300001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/cache-put-worker.js:122: // crash is resolved. http://crbug.com/426153 Fix posted - thanks for catching this!
https://codereview.chromium.org/425413002/diff/300001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/resources/cache-put-worker.js (right): https://codereview.chromium.org/425413002/diff/300001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/cache-put-worker.js:122: // crash is resolved. http://crbug.com/426153 On 2014/10/23 00:15:31, jsbell wrote: > Fix posted - thanks for catching this! Well, fix posted crbug.com/426153 (i.e. put(..., string)) but that doesn't actually affect this crash, does it? I'm hitting the same crash as crbug.com/426150 when I try this one.
Sorry for the churn, just want to make sure everything is documented. https://codereview.chromium.org/425413002/diff/300001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/resources/cache-put-worker.js (right): https://codereview.chromium.org/425413002/diff/300001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/cache-put-worker.js:96: // crash is resolved. http://crbug.com/426150 Testing locally: crash resolved by https://codereview.chromium.org/672733002 (but the test fails) https://codereview.chromium.org/425413002/diff/300001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/cache-put-worker.js:122: // crash is resolved. http://crbug.com/426153 Testing locally: resolved by https://codereview.chromium.org/672733002 https://codereview.chromium.org/425413002/diff/300001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/cache-put-worker.js:173: // crash is resolved. http://crbug.com/426153 Testing locally: resolved by https://codereview.chromium.org/676563002 https://codereview.chromium.org/425413002/diff/300001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/cache-put-worker.js:220: // crash is resolved. http://crbug.com/426153 Testing locally: resolved by https://codereview.chromium.org/676563002
https://codereview.chromium.org/425413002/diff/300001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/resources/cache-add-worker.js (right): https://codereview.chromium.org/425413002/diff/300001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/cache-add-worker.js:5: return assert_promise_rejects( Ah, the bindings code has a known issue: if the method is overloaded (like add() is) and returns a promise (like add() is) we don't properly reject instead of throw. http://crbug.com/359386 I'll poke it now that the webidl issue has been resolved.
On 2014/10/22 23:54:44, jsbell wrote: > https://codereview.chromium.org/425413002/diff/260001/LayoutTests/http/tests/... > File LayoutTests/http/tests/serviceworker/resources/cache-add-worker.js (right): > > https://codereview.chromium.org/425413002/diff/260001/LayoutTests/http/tests/... > LayoutTests/http/tests/serviceworker/resources/cache-add-worker.js:8: > cache.add(''), > On 2014/10/22 22:35:31, asanka wrote: > > On 2014/10/20 18:20:40, jsbell wrote: > > > This is an empty string, not "no arguments"; can you clarify what's being > > tested > > > here? > > > > > > (If this changed to `caches.add()` I'd expect the test to PASS, like addAll > > > does.) > > > > Changed to cache.add(), but shouldn't that be equivalent to > > cache.addAll([undefined]) ? > > No - the spec prose about delegating to addAll() applies after the arity checks > implied by the Web IDL method signature. Right! Thanks for the clarification.
https://codereview.chromium.org/425413002/diff/300001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/cache-add-expected.txt (right): https://codereview.chromium.org/425413002/diff/300001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/cache-add-expected.txt:3: FAIL Cache.add with no arguments Failed to execute 'add' on 'Cache': 1 argument required, but only 0 present.(stack: TypeError: Failed to execute 'add' on 'Cache': 1 argument required, but only 0 present. On 2014/10/23 00:15:31, jsbell wrote: > Weird - is it actually *throwing* instead of rejecting? I'll dig in - the > bindings code should be taking care of this automagically. Acknowledged. https://codereview.chromium.org/425413002/diff/300001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/cache-add-expected.txt:3: FAIL Cache.add with no arguments Failed to execute 'add' on 'Cache': 1 argument required, but only 0 present.(stack: TypeError: Failed to execute 'add' on 'Cache': 1 argument required, but only 0 present. On 2014/10/23 00:15:31, jsbell wrote: > Weird - is it actually *throwing* instead of rejecting? I'll dig in - the > bindings code should be taking care of this automagically. Acknowledged. https://codereview.chromium.org/425413002/diff/300001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/cache-add-expected.txt:9: FAIL Cache.add with a used Request object assert_throws: Cache.add with a Request object with a used body should reject with a TypeError. function "function () { throw e; }" threw object "NotFoundError: Entry was not found." ("NotFoundError") expected object "TypeError" ("TypeError") On 2014/10/23 00:15:31, jsbell wrote: > Any idea why this is NotFoundError instead of NotSupportedError (not > implemented) ? Couldn't dig into this one yet. https://codereview.chromium.org/425413002/diff/300001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/resources/cache-add-worker.js (right): https://codereview.chromium.org/425413002/diff/300001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/cache-add-worker.js:5: return assert_promise_rejects( On 2014/10/23 00:39:24, jsbell wrote: > Ah, the bindings code has a known issue: if the method is overloaded (like add() > is) and returns a promise (like add() is) we don't properly reject instead of > throw. > > http://crbug.com/359386 > > I'll poke it now that the webidl issue has been resolved. Acknowledged. https://codereview.chromium.org/425413002/diff/300001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/resources/cache-put-worker.js (right): https://codereview.chromium.org/425413002/diff/300001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/cache-put-worker.js:96: // crash is resolved. http://crbug.com/426150 On 2014/10/23 00:30:32, jsbell wrote: > Testing locally: crash resolved by https://codereview.chromium.org/672733002 > > (but the test fails) Enabling test and updating expectations. https://codereview.chromium.org/425413002/diff/300001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/cache-put-worker.js:122: // crash is resolved. http://crbug.com/426153 On 2014/10/23 00:23:51, jsbell wrote: > On 2014/10/23 00:15:31, jsbell wrote: > > Fix posted - thanks for catching this! > > Well, fix posted crbug.com/426153 (i.e. put(..., string)) but that doesn't > actually affect this crash, does it? I'm hitting the same crash as > crbug.com/426150 when I try this one. Whoops. Bug annotations was wrong. This should be the same crash as crbug.com/426150. Enabling this test as well. https://codereview.chromium.org/425413002/diff/300001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/cache-put-worker.js:173: // crash is resolved. http://crbug.com/426153 On 2014/10/23 00:30:32, jsbell wrote: > Testing locally: resolved by https://codereview.chromium.org/676563002 Ack. If this lands before that CL, I'll enable the test in a followup. https://codereview.chromium.org/425413002/diff/300001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/cache-put-worker.js:220: // crash is resolved. http://crbug.com/426153 On 2014/10/23 00:30:32, jsbell wrote: > Testing locally: resolved by https://codereview.chromium.org/676563002 Acknowledged.
The CQ bit was checked by asanka@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/425413002/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was checked by asanka@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/425413002/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was checked by asanka@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/425413002/320001
Message was sent while issue was closed.
Committed patchset #15 (id:320001) as 184242 |
