CacheStorage: Expand cache.keys() method.

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 6dc652e7d3d00d2807912fb8ed3ab82d673e9854..caff0ac893174cb5b10631797e93c6197c343489 100644
--- a/content/browser/cache_storage/cache_storage_cache.cc
+++ b/content/browser/cache_storage/cache_storage_cache.cc
@@ -451,7 +451,9 @@ void CacheStorageCache::BatchDidAllOperations(
   callback->Run(CACHE_STORAGE_OK);
 }
 
-void CacheStorageCache::Keys(const RequestsCallback& callback) {
+void CacheStorageCache::Keys(std::unique_ptr<ServiceWorkerFetchRequest> request,
+                             const CacheStorageCacheQueryParams& options,
+                             const RequestsCallback& callback) {
   if (backend_state_ == BACKEND_CLOSED) {
     callback.Run(CACHE_STORAGE_ERROR_STORAGE, std::unique_ptr<Requests>());
     return;
@@ -459,6 +461,7 @@ void CacheStorageCache::Keys(const RequestsCallback& callback) {
 
   scheduler_->ScheduleOperation(
       base::Bind(&CacheStorageCache::KeysImpl, weak_ptr_factory_.GetWeakPtr(),
+                 base::Passed(std::move(request)), options,
                  scheduler_->WrapCallbackToRunNext(callback)));
 }
 
@@ -625,11 +628,18 @@ void CacheStorageCache::QueryCacheProcessNextEntry(
     return;
   }
 
-  if (query_cache_results->options.ignore_search) {
-    DCHECK(query_cache_results->request);
+  if (query_cache_results->request &&
+      !query_cache_results->request->url.is_empty()) {

[nhiroki, 2016/08/02 07:31:06]

Hm... I assumed that the diskcache always returns a valid request and request's
url is not empty. Did you hit DCHECK(query_cache_results->request) in the
original code?

If request is null or url is empty, I think we should immediately proceed to the
next entry instead of trying to read metadata.

[zino, 2016/08/02 17:37:42]

Yes, diskcache always returns a valid request and request's url is not empty but
The query_cache_results->request is a optional argument of
keys(request, options) or other things. So, it can be null or empty.
If the name(query_cache_results->request) is confusing, we can rename it.

In the original code, we asserted that the request is not null if only
ignore_search is true.
But it could still be null or empty url if ignore_search is false.

Also, the spec says like this (e.g. keys()):
  ...
  2. If the optional argument request is omitted, then:
    1. For each fetching record entry of its request to response map,
       in key insertion order:
      1. Add entry.[[key]] to resultArray.
  3. Else:
    ...
    4. Let requestResponseArray be the result of running Query Cache algorithm
       passing a Request object that represents r and options as the arguments.

In terms of implementation, I thought that the number 2(for each) could merge
into 
the number 4(Query Cache) in order to reduce repeated codes unnecessarily.
So, I thought we should read metadata if request is null or url is empty.
What do you think?
If you are agree with me, I'll leave comments here.
If I'm wrong, please let me know better way :)

Thank you!

[nhiroki, 2016/08/08 12:40:54]

Oh, I wrongly assumed |query_cache_results->request| is returned from a disk
cache. I think current naming is appropriate and you don't have to rename it, I
misread it though :p
Looks good. Thank you for the clarification :)

     disk_cache::Entry* entry(*iter);
-    if (RemoveQueryParam(query_cache_results->request->url) !=
-        RemoveQueryParam(GURL(entry->GetKey()))) {
+    GURL requestURL = query_cache_results->request->url;
+    GURL cachedURL = GURL(entry->GetKey());
+
+    if (query_cache_results->options.ignore_search) {
+      requestURL = RemoveQueryParam(requestURL);
+      cachedURL = RemoveQueryParam(cachedURL);
+    }
+
+    if (cachedURL != requestURL) {
       QueryCacheProcessNextEntry(std::move(query_cache_results), iter + 1);
       return;
     }
@@ -1266,16 +1276,17 @@ void CacheStorageCache::DeleteDidOpenEntry(
   callback.Run(CACHE_STORAGE_OK);
 }
 
-void CacheStorageCache::KeysImpl(const RequestsCallback& callback) {
+void CacheStorageCache::KeysImpl(
+    std::unique_ptr<ServiceWorkerFetchRequest> request,
+    const CacheStorageCacheQueryParams& options,
+    const RequestsCallback& callback) {
   DCHECK_NE(BACKEND_UNINITIALIZED, backend_state_);
   if (backend_state_ != BACKEND_OPEN) {
     callback.Run(CACHE_STORAGE_ERROR_STORAGE, std::unique_ptr<Requests>());
     return;
   }
 
-  std::unique_ptr<ServiceWorkerFetchRequest> request(
-      new ServiceWorkerFetchRequest);
-  QueryCache(std::move(request), CacheStorageCacheQueryParams(),
+  QueryCache(std::move(request), options,
              base::Bind(&CacheStorageCache::KeysDidQueryCache,
                         weak_ptr_factory_.GetWeakPtr(), callback));
 }