Cache API should not match() HEAD requests

Cache API should not match() HEAD requests

Per spec issue, the match()/matchAll() methods should not match HEAD
requests, yielding undefined (or an empty result set).

[1] https://github.com/slightlyoff/ServiceWorker/issues/710#issuecomment-236641802

BUG=633358
Committed: https://crrev.com/b482718f3ced3088afbd8bfdd7ff51fdfdc9bc8e
Cr-Commit-Position: refs/heads/master@{#411172}

[jsbell, 2016-08-01 22:44:52]

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

[commit-bot: I haz the power, 2016-08-01 22:45:21]

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

[jsbell, 2016-08-01 22:46:00]

jsbell@chromium.org changed reviewers:
+ cmumford@chromium.org, jkarlin@chromium.org

[jsbell, 2016-08-01 22:46:01]

jkarlin@, cmumford@ - please take a look?

It may be cleaner to do this on the back end - it may be a one-line change
there.

[commit-bot: I haz the power, 2016-08-02 00:40:18]

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

[commit-bot: I haz the power, 2016-08-02 00:40:19]

Dry run: This issue passed the CQ dry run.

[cmumford, 2016-08-02 13:12:59]

lgtm

[jkarlin, 2016-08-02 13:37:34]

What about CacheStorage::Match requests?

[jsbell, 2016-08-02 16:52:06]

On 2016/08/02 13:37:34, jkarlin wrote:
> What about CacheStorage::Match requests?

Oh duh, of course.

Thoughts on blink vs. chromium-side for the change?

[jkarlin, 2016-08-02 17:55:00]

On 2016/08/02 16:52:06, jsbell wrote:
> On 2016/08/02 13:37:34, jkarlin wrote:
> > What about CacheStorage::Match requests?
> 
> Oh duh, of course.
> 
> Thoughts on blink vs. chromium-side for the change?

CacheStorageCache::Match would be a good place for it, but you'd need to return
a new error type as I don't think we have one for malformed requests. You'd also
have to be sure that CacheStorage::Match handled the error correctly.

[jsbell, 2016-08-03 00:13:23]

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

[commit-bot: I haz the power, 2016-08-03 00:14:18]

Dry run: 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-08-03 01:36:15]

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

[commit-bot: I haz the power, 2016-08-03 01:36:16]

Dry run: This issue passed the CQ dry run.

[jsbell, 2016-08-03 17:34:01]

jkarlin@ - please take a look?

I went with CacheStorageCache::MatchImpl() rather than Match() to keep the
processing model the same (i.e. this query is processed in sequence with other
requests and just yields no results, not an early error).

CacheStorage operations route through CacheStorageCache::Match() so no changes
were necessary there. The MatchAll() special case was necessary (and took a
while to figure out that I needed ignore_search to trigger that code path!)

[jkarlin, 2016-08-04 14:50:40]

https://codereview.chromium.org/2204683002/diff/40001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage_cache.cc (right):

https://codereview.chromium.org/2204683002/diff/40001/content/browser/cache_s...
content/browser/cache_storage/cache_storage_cache.cc:800: if (request &&
request->method == "HEAD") {
The updated spec reads that if it's a HEAD and options ignore_method is false,
return empty. We need to check ignore_method here.

[jkarlin, 2016-08-04 14:52:18]

https://codereview.chromium.org/2204683002/diff/40001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage_cache.cc (right):

https://codereview.chromium.org/2204683002/diff/40001/content/browser/cache_s...
content/browser/cache_storage/cache_storage_cache.cc:800: if (request &&
request->method == "HEAD") {
On 2016/08/04 14:50:40, jkarlin wrote:
> The updated spec reads that if it's a HEAD and options ignore_method is false,
> return empty. We need to check ignore_method here.

This should really be a check to make sure that it's a GET.

[jsbell, 2016-08-04 23:47:48]

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

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

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

[jsbell, 2016-08-04 23:49:17]

This should match the spec updates, but I need to add tests for the keys() and
other methods that now bail on non-GET requests.

Many checks moved back to the blink side which seems (maybe?) to the spec text
w/r/t sequencing of operations.

Anyway, take a look if you want but I will probably iterate more.

https://codereview.chromium.org/2204683002/diff/40001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage_cache.cc (right):

https://codereview.chromium.org/2204683002/diff/40001/content/browser/cache_s...
content/browser/cache_storage/cache_storage_cache.cc:800: if (request &&
request->method == "HEAD") {
On 2016/08/04 14:52:18, jkarlin wrote:
> On 2016/08/04 14:50:40, jkarlin wrote:
> > The updated spec reads that if it's a HEAD and options ignore_method is
false,
> > return empty. We need to check ignore_method here.
> 
> This should really be a check to make sure that it's a GET.

Done and done.

[commit-bot: I haz the power, 2016-08-05 01:44:36]

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

[commit-bot: I haz the power, 2016-08-05 01:44:37]

Dry run: This issue passed the CQ dry run.

[jsbell, 2016-08-09 23:39:02]

Okay, I think this is ready for a look-see. Added cache-keys tests (did we
really not have those?) and even though we don't pass all (due to
https://crbug.com/474866) they turned up issues with my impl, so yay!

jkarlin@ - please take a look?

[jkarlin, 2016-08-10 13:22:45]

Looking good!

https://codereview.chromium.org/2204683002/diff/80001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage_cache.cc (right):

https://codereview.chromium.org/2204683002/diff/80001/content/browser/cache_s...
content/browser/cache_storage/cache_storage_cache.cc:598:
callback.Run(CACHE_STORAGE_OK, base::WrapUnique(new QueryCacheResults(
base::MakeUnique<QueryCacheResults>(std::move(request), options, callback) is
preferred over WrapUnique(new ...) according to
https://cs.chromium.org/chromium/src/base/memory/ptr_util.h?q=makeunique&sq=p...

https://codereview.chromium.org/2204683002/diff/80001/content/browser/cache_s...
content/browser/cache_storage/cache_storage_cache.cc:702: if (request->method ==
"HEAD") {
Should this be "if (request->method != "GET")" ?

https://codereview.chromium.org/2204683002/diff/80001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage_cache_unittest.cc (right):

https://codereview.chromium.org/2204683002/diff/80001/content/browser/cache_s...
content/browser/cache_storage/cache_storage_cache_unittest.cc:968: //
TODO(jsbell): Test with ignore_method = true when supported.
It looks like it's supported to me, at least on the browser side. Good enough
for unit testing.

https://codereview.chromium.org/2204683002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-keys.js
(right):

https://codereview.chromium.org/2204683002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-keys.js:1:
if (self.importScripts) {
You're going to have rebase issues.

https://codereview.chromium.org/1719103002/ landed yesterday and added a
cache-keys.js

https://codereview.chromium.org/2204683002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-keys.js:39:
assert_equals(requests.length, 1);
This test fails. Why not expect 2 here?

https://codereview.chromium.org/2204683002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-keys.js:51:
assert_equals(requests.length, 1);
Ditto

https://codereview.chromium.org/2204683002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/cachestorage/window/cache-keys-expected.txt
(right):

https://codereview.chromium.org/2204683002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/cachestorage/window/cache-keys-expected.txt:2:
PASS Cache.keys() called on an empty cache
We shouldn't have any failures here right? There shouldn't be a
cache-keys-expected.txt file.

https://codereview.chromium.org/2204683002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/cachestorage/window/cache-match-expected.txt
(left):

https://codereview.chromium.org/2204683002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/cachestorage/window/cache-match-expected.txt:15:
FAIL Cache.match with POST Request assert_equals: Cache.match should not find a
match expected (undefined) undefined but got (object) object "[object Response]"
Woot!

[jsbell, 2016-08-10 15:50:10]

https://codereview.chromium.org/2204683002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-keys.js
(right):

https://codereview.chromium.org/2204683002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-keys.js:1:
if (self.importScripts) {
On 2016/08/10 13:22:45, jkarlin wrote:
> You're going to have rebase issues.
> 
> https://codereview.chromium.org/1719103002/ landed yesterday and added a
> cache-keys.js

Hah, great timing. Yeah, that renders some of this moot...

https://codereview.chromium.org/2204683002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-keys.js:39:
assert_equals(requests.length, 1);
On 2016/08/10 13:22:45, jkarlin wrote:
> This test fails. Why not expect 2 here?

keys(test_url1) should not match test_url2 so should be 1... hopefully this will
pass with that other CL in.

[jsbell, 2016-08-10 17:17:29]

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

[commit-bot: I haz the power, 2016-08-10 17:18:02]

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

[jsbell, 2016-08-10 17:18:09]

Yet another look, jkarlin@ ?

Thanks for your patience!

https://codereview.chromium.org/2204683002/diff/80001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage_cache.cc (right):

https://codereview.chromium.org/2204683002/diff/80001/content/browser/cache_s...
content/browser/cache_storage/cache_storage_cache.cc:598:
callback.Run(CACHE_STORAGE_OK, base::WrapUnique(new QueryCacheResults(
On 2016/08/10 13:22:45, jkarlin wrote:
> base::MakeUnique<QueryCacheResults>(std::move(request), options, callback) is
> preferred over WrapUnique(new ...) according to
>
https://cs.chromium.org/chromium/src/base/memory/ptr_util.h?q=makeunique&sq=p...

Done.

https://codereview.chromium.org/2204683002/diff/80001/content/browser/cache_s...
content/browser/cache_storage/cache_storage_cache.cc:702: if (request->method ==
"HEAD") {
On 2016/08/10 13:22:44, jkarlin wrote:
> Should this be "if (request->method != "GET")" ?

Done.

https://codereview.chromium.org/2204683002/diff/80001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage_cache_unittest.cc (right):

https://codereview.chromium.org/2204683002/diff/80001/content/browser/cache_s...
content/browser/cache_storage/cache_storage_cache_unittest.cc:968: //
TODO(jsbell): Test with ignore_method = true when supported.
On 2016/08/10 13:22:45, jkarlin wrote:
> It looks like it's supported to me, at least on the browser side. Good enough
> for unit testing.

Done.

https://codereview.chromium.org/2204683002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/cachestorage/window/cache-keys-expected.txt
(right):

https://codereview.chromium.org/2204683002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/cachestorage/window/cache-keys-expected.txt:2:
PASS Cache.keys() called on an empty cache
On 2016/08/10 13:22:45, jkarlin wrote:
> We shouldn't have any failures here right? There shouldn't be a
> cache-keys-expected.txt file.

The FAILs were for the unsupported request param, but now that that's supported
I can remove those cases when the tests merge. So yes - all PASS now. \o/

[jkarlin, 2016-08-10 17:30:04]

lgtm!

https://codereview.chromium.org/2204683002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-keys.js
(right):

https://codereview.chromium.org/2204683002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-keys.js:39:
assert_equals(requests.length, 1);
On 2016/08/10 15:50:10, jsbell wrote:
> On 2016/08/10 13:22:45, jkarlin wrote:
> > This test fails. Why not expect 2 here?
> 
> keys(test_url1) should not match test_url2 so should be 1... hopefully this
will
> pass with that other CL in.

Acknowledged.

https://codereview.chromium.org/2204683002/diff/100001/content/browser/cache_...
File content/browser/cache_storage/cache_storage_cache_unittest.cc (right):

https://codereview.chromium.org/2204683002/diff/100001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache_unittest.cc:960:
TEST_P(CacheStorageCacheTestP, MatchAll_HEAD) {
For consistency, can you change to MatchAll_Head

[commit-bot: I haz the power, 2016-08-10 19:20:36]

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

[commit-bot: I haz the power, 2016-08-10 19:20:37]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jsbell, 2016-08-10 20:10:49]

https://codereview.chromium.org/2204683002/diff/100001/content/browser/cache_...
File content/browser/cache_storage/cache_storage_cache_unittest.cc (right):

https://codereview.chromium.org/2204683002/diff/100001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache_unittest.cc:960:
TEST_P(CacheStorageCacheTestP, MatchAll_HEAD) {
On 2016/08/10 17:30:04, jkarlin wrote:
> For consistency, can you change to MatchAll_Head

Done.

[jsbell, 2016-08-10 20:11:04]

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

[jsbell, 2016-08-10 20:11:08]

The CQ bit was checked by jsbell@chromium.org

[jsbell, 2016-08-10 20:11:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
cmumford@chromium.org, jkarlin@chromium.org
Link to the patchset: https://codereview.chromium.org/2204683002/#ps120001
(title: "HEAD -> Head")

[commit-bot: I haz the power, 2016-08-10 20:11:40]

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-08-10 21:01:02]

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

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

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[jsbell, 2016-08-10 21:18:20]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2016-08-10 21:19:28]

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-08-10 22:47:53]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-08-10 22:50:13]

Description was changed from

==========
Cache API should not match() HEAD requests

Per spec issue, the match()/matchAll() methods should not match HEAD
requests, yielding undefined (or an empty result set).

[1]
https://github.com/slightlyoff/ServiceWorker/issues/710#issuecomment-236641802

BUG=633358
==========

to

==========
Cache API should not match() HEAD requests

Per spec issue, the match()/matchAll() methods should not match HEAD
requests, yielding undefined (or an empty result set).

[1]
https://github.com/slightlyoff/ServiceWorker/issues/710#issuecomment-236641802

BUG=633358

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

[commit-bot: I haz the power, 2016-08-10 22:50:14]

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