1271069 - caches.match({name: 'badName' }) should resolve undefined, not reject

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

We're changing the spec not to reject caches.match() if the named cache is not found.  See:

https://github.com/slightlyoff/ServiceWorker/issues/891

This just involves changing a line or two in dom/cache/DBCache.cpp and updating a few tests.

Comment 1

[Tom Tung [:tt, :ttung]]

I'd like to take this bug.

Comment 2

[Tom Tung [:tt, :ttung]]

In my naive guess, just change the return value [1] to NS_OK and tests which is related to it(test_cache_match*)?

[1] https://dxr.mozilla.org/mozilla-central/source/dom/cache/DBSchema.cpp#904

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Essentially yes.  There are probably some wpt tests that exercise this to.

I think we should also add to these tests a caches.has() call after the caches.match() to demonstrate that the Cache object is not created as a side effect.

Does that make sense?

Comment 4

[Tom Tung [:tt, :ttung]]

Yes, thank you for your feedback! I'll resolve it as soon as possible.

Comment 5

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1271069 - caches.match with wrong name should resolce to undefined.

Do you mind helping me to review this patch, Ben? Thanks! 

In this patch, I changed a line of code to make caches.match resolves to undefined as CacheName is wrong. 

I just modify the return value to NS_OK when the cache name is not be found. Besides, three existing tests which are related to this modification are changed. I modify those tests from expected for rejected to resolved to undefined as caches.match with unopened CacheName. I also add tests to check whether the cache is create after caches.match resolves to undefined as suggestion in comment 3. 

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8e7aabc8caa

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8751178 [details] [diff] [review]
Bug 1271069 - caches.match with wrong name should resolce to undefined.

Review of attachment 8751178 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!  r=me with the one comment addressed.

Thanks!

::: testing/web-platform/tests/service-workers/cache-storage/script-tests/cache-storage-match.js
@@ +111,5 @@
>            return self.caches.match(transaction.request, {cacheName: 'foo'});
>          })
>        .then(function(response) {
> +          assert_equals(response, undefined,
> +                        'The match with bad cache name should reject.');

Please fix this assert message to say "should resolve to undefined".

Comment 7

[Tom Tung [:tt, :ttung]]

Attachment: [Final] Bug 1271069 - caches.match with wrong name should resolve to undefined. r=bkelly

Thanks for your review and your time, Ben! I addressed the comment.

Comment 8

[Tom Tung [:tt, :ttung]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5554d1e3b797

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d493625548f

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5d493625548f