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..53f02ab6d8f25dc9408c5ca7c7bd0bc27a16d8b5 100644
--- a/content/browser/cache_storage/cache_storage_cache.cc
+++ b/content/browser/cache_storage/cache_storage_cache.cc
@@ -147,6 +147,13 @@ bool VaryMatches(const ServiceWorkerHeaderMap& request,
   return true;
 }
 
+bool URLMatchWithoutQuery(const GURL& request_url, const GURL& key) {

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

This function name sounds a bit strange to me.

This does 2 things: (1) removing query parts from given URLs and (2) comparing
them. Such mixed functions seem to make it difficult to name this. Instead, how
about making a simple "GURL RemoveQueryParam(const GURL& url)" just to remove a
query part and calling it in MatchAllProcessNextEntry() like this?:

if (RemoveQueryParam(context->request->url) !=
RemoveQueryParam(GURL(entry->GetKey())) { ... }

[zino, 2016/02/03 12:04:47]

Done.

+  url::Replacements<char> replacements;
+  replacements.ClearQuery();
+  return request_url.ReplaceComponents(replacements) ==
+         key.ReplaceComponents(replacements);
+}
+
 void ReadMetadata(disk_cache::Entry* entry, const MetadataCallback& callback) {
   DCHECK(entry);
 
@@ -210,12 +217,20 @@ struct CacheStorageCache::OpenAllEntriesContext {
 
 // The state needed to pass between CacheStorageCache::MatchAll callbacks.
 struct CacheStorageCache::MatchAllContext {
-  explicit MatchAllContext(const CacheStorageCache::ResponsesCallback& callback)
-      : original_callback(callback),
+  MatchAllContext(scoped_ptr<ServiceWorkerFetchRequest> request,
+                  const CacheStorageCacheQueryParams& match_params,
+                  const ResponsesCallback& callback)
+      : request(std::move(request)),
+        options(match_params),
+        original_callback(callback),
         out_responses(new Responses),
         out_blob_data_handles(new BlobDataHandles) {}
   ~MatchAllContext() {}
 
+  scoped_ptr<ServiceWorkerFetchRequest> request;
+
+  CacheStorageCacheQueryParams options;
+
   // The callback passed to the MatchAll() function.
   ResponsesCallback original_callback;
 
@@ -327,7 +342,10 @@ void CacheStorageCache::Match(scoped_ptr<ServiceWorkerFetchRequest> request,
                  base::Passed(std::move(request)), pending_callback));
 }
 
-void CacheStorageCache::MatchAll(const ResponsesCallback& callback) {
+void CacheStorageCache::MatchAll(
+    scoped_ptr<ServiceWorkerFetchRequest> request,
+    const CacheStorageCacheQueryParams& match_params,
+    const ResponsesCallback& callback) {
   if (!LazyInitialize()) {
     callback.Run(CACHE_STORAGE_ERROR_STORAGE, scoped_ptr<Responses>(),
                  scoped_ptr<BlobDataHandles>());
@@ -337,9 +355,12 @@ void CacheStorageCache::MatchAll(const ResponsesCallback& callback) {
   ResponsesCallback pending_callback =
       base::Bind(&CacheStorageCache::PendingResponsesCallback,
                  weak_ptr_factory_.GetWeakPtr(), callback);
+
+  scoped_ptr<MatchAllContext> context(
+      new MatchAllContext(std::move(request), match_params, pending_callback));
   scheduler_->ScheduleOperation(base::Bind(&CacheStorageCache::MatchAllImpl,
                                            weak_ptr_factory_.GetWeakPtr(),
-                                           pending_callback));
+                                           base::Passed(std::move(context))));
 }
 
 void CacheStorageCache::BatchOperation(
@@ -638,28 +659,30 @@ void CacheStorageCache::MatchDidReadMetadata(
                std::move(blob_data_handle));
 }
 
-void CacheStorageCache::MatchAllImpl(const ResponsesCallback& callback) {
+void CacheStorageCache::MatchAllImpl(scoped_ptr<MatchAllContext> context) {
   DCHECK_NE(BACKEND_UNINITIALIZED, backend_state_);
   if (backend_state_ != BACKEND_OPEN) {
-    callback.Run(CACHE_STORAGE_ERROR_STORAGE, scoped_ptr<Responses>(),
-                 scoped_ptr<BlobDataHandles>());
+    context->original_callback.Run(CACHE_STORAGE_ERROR_STORAGE,
+                                   std::move(context->out_responses),
+                                   std::move(context->out_blob_data_handles));

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

Do you have any reason to change these parameters? I think it'd be common
pattern to pass explicit empty results to an error callback as the original code
does.

[zino, 2016/02/03 12:04:47]

Done.

I thought that we need to avoid creating object duplicately (even if it's a
empty pointer object) if possible.
The out_responses and blob_data_handles were already initialized as empty
object.

On second thoughts, I was wrong and readability seems more important.

     return;
   }
 
   OpenAllEntries(base::Bind(&CacheStorageCache::MatchAllDidOpenAllEntries,
-                            weak_ptr_factory_.GetWeakPtr(), callback));
+                            weak_ptr_factory_.GetWeakPtr(),
+                            base::Passed(std::move(context))));
 }
 
 void CacheStorageCache::MatchAllDidOpenAllEntries(
-    const ResponsesCallback& callback,
+    scoped_ptr<MatchAllContext> context,
     scoped_ptr<OpenAllEntriesContext> entries_context,
     CacheStorageError error) {
   if (error != CACHE_STORAGE_OK) {
-    callback.Run(error, scoped_ptr<Responses>(), scoped_ptr<BlobDataHandles>());
+    context->original_callback.Run(error, std::move(context->out_responses),
+                                   std::move(context->out_blob_data_handles));

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

ditto.

[zino, 2016/02/03 12:04:47]

Done.

     return;
   }
 
-  scoped_ptr<MatchAllContext> context(new MatchAllContext(callback));
   context->entries_context.swap(entries_context);
   Entries::iterator iter = context->entries_context->entries.begin();
   MatchAllProcessNextEntry(std::move(context), iter);
@@ -676,6 +699,16 @@ void CacheStorageCache::MatchAllProcessNextEntry(
     return;
   }
 
+  disk_cache::Entry* entry(*iter);

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

Can you move this to line 705?

[zino, 2016/02/03 12:04:47]

Done.

+  if (context->options.ignore_search) {
+    DCHECK(context->request);
+    if (!URLMatchWithoutQuery(context->request->url, GURL(entry->GetKey()))) {
+      // In this caase, we don't need to read data.

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

s/casse/case

[zino, 2016/02/03 12:04:47]

Done.

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