CacheStorage: Add ignoreSearch option to cache.matchAll().

content/browser/cache_storage/cache_storage_cache.cc

Index: content/browser/cache_storage/cache_storage_cache.cc
diff --git a/content/browser/cache_storage/cache_storage_cache.cc b/content/browser/cache_storage/cache_storage_cache.cc
index 2fc74110e7e16906675d343cd560a44dea2dfb6d..6dbe01d8d44de91dbbdd9e924071c79b90c9066a 100644
--- a/content/browser/cache_storage/cache_storage_cache.cc
+++ b/content/browser/cache_storage/cache_storage_cache.cc
@@ -147,6 +147,11 @@ bool VaryMatches(const ServiceWorkerHeaderMap& request,
   return true;
 }
 
+bool URLMatchWithoutQuery(const GURL& request_url, const GURL& key) {
+  return request_url.scheme() == key.scheme() &&
+         request_url.host() == key.host() && request_url.path() == key.path();

[nhiroki, 2016/01/18 04:29:58]

This match algorithm could be insufficient, for example, when the url contains
username/password parts. According to the "Query Cache" algorithm in the sw
spec, request/cached URLs should be serialized by the "URL serializing"
algorithm (https://url.spec.whatwg.org/#url-serializing) and then be compared.

Maybe we could achieve that by creating new KURL objects without search
parameters and compares them by KURL::operator==()?

[zino, 2016/01/19 13:46:38]

I modified it but I couldn't write a test for it.
I'm not sure but Request object seems to contains no username/password parts.
The spec[1] and our implementation[2] says "if parsedURL includes credentials,
throw a TypeError".

[1] https://fetch.spec.whatwg.org/#dom-request (12-3)
[2] http://code.google.com/p/chromium/issues/detail?id=474439

[nhiroki, 2016/01/20 08:56:02]

Yes, the constructor rejects such a URL.

A Request object made by a regular network request can contain
username/password, for example, clicking "<a
href='http://username:password@example.com/'>".

"LayoutTests/http/tests/cachestorage/serviceworker/credentials.html" is an
example of credential tests.

+}
+
 void ReadMetadata(disk_cache::Entry* entry, const MetadataCallback& callback) {
   DCHECK(entry);
 
@@ -210,12 +215,16 @@ struct CacheStorageCache::OpenAllEntriesContext {
 
 // The state needed to pass between CacheStorageCache::MatchAll callbacks.
 struct CacheStorageCache::MatchAllContext {
-  explicit MatchAllContext(const CacheStorageCache::ResponsesCallback& callback)
-      : original_callback(callback),
+  explicit MatchAllContext(scoped_ptr<ServiceWorkerFetchRequest> request,

[nhiroki, 2016/01/18 04:29:58]

Remove "explicit"

[zino, 2016/01/19 13:46:38]

Done.

+                           const CacheStorageCache::ResponsesCallback& callback)

[nhiroki, 2016/01/18 04:29:58]

"CacheStorageCache::" prefix is not necessary.

[zino, 2016/01/19 13:46:38]

Done.

+      : request(std::move(request)),
+        original_callback(callback),
         out_responses(new Responses),
         out_blob_data_handles(new BlobDataHandles) {}
   ~MatchAllContext() {}
 
+  scoped_ptr<ServiceWorkerFetchRequest> request;
+
   // The callback passed to the MatchAll() function.
   ResponsesCallback original_callback;
 
@@ -328,6 +337,11 @@ void CacheStorageCache::Match(scoped_ptr<ServiceWorkerFetchRequest> request,
 }
 
 void CacheStorageCache::MatchAll(const ResponsesCallback& callback) {

[nhiroki, 2016/01/18 04:29:59]

Could you remove this indirection and directly call MatchAll(null_req,
callback)?

[zino, 2016/01/19 13:46:38]

Done.

+  MatchAll(nullptr, callback);
+}
+
+void CacheStorageCache::MatchAll(scoped_ptr<ServiceWorkerFetchRequest> request,
+                                 const ResponsesCallback& callback) {
   if (!LazyInitialize()) {
     callback.Run(CACHE_STORAGE_ERROR_STORAGE, scoped_ptr<Responses>(),
                  scoped_ptr<BlobDataHandles>());
@@ -337,9 +351,9 @@ void CacheStorageCache::MatchAll(const ResponsesCallback& callback) {
   ResponsesCallback pending_callback =
       base::Bind(&CacheStorageCache::PendingResponsesCallback,
                  weak_ptr_factory_.GetWeakPtr(), callback);
-  scheduler_->ScheduleOperation(base::Bind(&CacheStorageCache::MatchAllImpl,
-                                           weak_ptr_factory_.GetWeakPtr(),
-                                           pending_callback));
+  scheduler_->ScheduleOperation(base::Bind(
+      &CacheStorageCache::MatchAllImpl, weak_ptr_factory_.GetWeakPtr(),
+      base::Passed(std::move(request)), pending_callback));
 }
 
 void CacheStorageCache::BatchOperation(
@@ -638,7 +652,9 @@ void CacheStorageCache::MatchDidReadMetadata(
                std::move(blob_data_handle));
 }
 
-void CacheStorageCache::MatchAllImpl(const ResponsesCallback& callback) {
+void CacheStorageCache::MatchAllImpl(
+    scoped_ptr<ServiceWorkerFetchRequest> request,
+    const ResponsesCallback& callback) {
   DCHECK_NE(BACKEND_UNINITIALIZED, backend_state_);
   if (backend_state_ != BACKEND_OPEN) {
     callback.Run(CACHE_STORAGE_ERROR_STORAGE, scoped_ptr<Responses>(),
@@ -647,10 +663,12 @@ void CacheStorageCache::MatchAllImpl(const ResponsesCallback& callback) {
   }
 
   OpenAllEntries(base::Bind(&CacheStorageCache::MatchAllDidOpenAllEntries,
-                            weak_ptr_factory_.GetWeakPtr(), callback));
+                            weak_ptr_factory_.GetWeakPtr(),
+                            base::Passed(std::move(request)), callback));
 }
 
 void CacheStorageCache::MatchAllDidOpenAllEntries(
+    scoped_ptr<ServiceWorkerFetchRequest> request,
     const ResponsesCallback& callback,
     scoped_ptr<OpenAllEntriesContext> entries_context,
     CacheStorageError error) {
@@ -659,7 +677,8 @@ void CacheStorageCache::MatchAllDidOpenAllEntries(
     return;
   }
 
-  scoped_ptr<MatchAllContext> context(new MatchAllContext(callback));
+  scoped_ptr<MatchAllContext> context(
+      new MatchAllContext(std::move(request), callback));
   context->entries_context.swap(entries_context);
   Entries::iterator iter = context->entries_context->entries.begin();
   MatchAllProcessNextEntry(std::move(context), iter);
@@ -676,9 +695,16 @@ void CacheStorageCache::MatchAllProcessNextEntry(
     return;
   }
 
-  ReadMetadata(*iter, base::Bind(&CacheStorageCache::MatchAllDidReadMetadata,
-                                 weak_ptr_factory_.GetWeakPtr(),
-                                 base::Passed(std::move(context)), iter));
+  disk_cache::Entry* entry(*iter);
+  if (!context->request ||
+      URLMatchWithoutQuery(context->request->url, GURL(entry->GetKey()))) {

[nhiroki, 2016/01/18 04:29:59]

This combined condition is a bit confusing to me. How about checking the
|ignore_search| option to clarify that we're handling the case here.

// context->options() is CacheStorageCacheQueryParams passed from
CacheStorageDispatcherHost.
if (context->options().ignore_search) {
  DCHECK(context->request);
  if (!URLMatchWithoutQuery(...))
    ReadMetadata(...);
  else
    MatchAllProcessNextEntry(...);
  return;
}
DCHECK(!context->request);
ReadMetadata(...);

[zino, 2016/01/19 13:46:38]

Done.

+    ReadMetadata(*iter, base::Bind(&CacheStorageCache::MatchAllDidReadMetadata,
+                                   weak_ptr_factory_.GetWeakPtr(),
+                                   base::Passed(std::move(context)), iter));
+    return;
+  }
+
+  MatchAllProcessNextEntry(std::move(context), iter + 1);
 }
 
 void CacheStorageCache::MatchAllDidReadMetadata(