Make the FetchAPIRequest optional in the CacheStorage API.

For the 2 calls where the idl doesn't require a Request to be passed,
MatchAll and Keys, make the mojom call also take an optional Request.
This allows the serializer to enforce that the attributes of the
FetchAPIRequest are valid, as we can guarantee that an uninitialized
request won't be dispatched.

Change-Id: I94b6154e69341a849f57172ec016916aa6020382
Reviewed-on: https://chromium-review.googlesource.com/1060334
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Joshua Bell <jsbell@chromium.org>
Commit-Queue: Lucas Gadani <lfg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560639}

content/browser/cache_storage/cache_storage_dispatcher_host.cc

@@ -10,6 +10,7 @@
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/macros.h"
+#include "base/optional.h"
 #include "base/strings/string16.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
@@ -92,7 +93,7 @@ class CacheStorageDispatcherHost::CacheImpl
     std::move(callback).Run(blink::mojom::MatchResult::NewResponse(*response));
   }
 
-  void MatchAll(const ServiceWorkerFetchRequest& request,
+  void MatchAll(const base::Optional<ServiceWorkerFetchRequest>& request,
                 blink::mojom::QueryParamsPtr match_params,
                 MatchAllCallback callback) override {
     content::CacheStorageCache* cache = cache_handle_.value();
@@ -102,28 +103,17 @@ class CacheStorageDispatcherHost::CacheImpl
       return;
     }
 
-    if (request.url.is_empty()) {
-      cache->MatchAll(
-          nullptr, std::move(match_params),
-          base::BindOnce(&CacheImpl::OnCacheMatchAllCallback,
-                         weak_factory_.GetWeakPtr(), std::move(callback)));
-      return;
-    }
+    std::unique_ptr<ServiceWorkerFetchRequest> request_ptr;
 
-    auto scoped_request = std::make_unique<ServiceWorkerFetchRequest>(
-        request.url, request.method, request.headers, request.referrer,
-        request.is_reload);
-    if (match_params->ignore_search) {
-      cache->MatchAll(
-          std::move(scoped_request), std::move(match_params),
-          base::BindOnce(&CacheImpl::OnCacheMatchAllCallback,
-                         weak_factory_.GetWeakPtr(), std::move(callback)));
-      return;
+    if (request && !request->url.is_empty()) {
+      request_ptr = std::make_unique<ServiceWorkerFetchRequest>(
+          request->url, request->method, request->headers, request->referrer,
+          request->is_reload);
     }
 
-    cache->Match(
-        std::move(scoped_request), std::move(match_params),
-        base::BindOnce(&CacheImpl::OnCacheMatchAllCallbackAdapter,
+    cache->MatchAll(
+        std::move(request_ptr), std::move(match_params),
+        base::BindOnce(&CacheImpl::OnCacheMatchAllCallback,
                        weak_factory_.GetWeakPtr(), std::move(callback)));
   }
 
@@ -141,19 +131,7 @@ class CacheStorageDispatcherHost::CacheImpl
         blink::mojom::MatchAllResult::NewResponses(std::move(responses)));
   }
 
-  void OnCacheMatchAllCallbackAdapter(
-      blink::mojom::CacheStorageCache::MatchAllCallback callback,
-      blink::mojom::CacheStorageError error,
-      std::unique_ptr<ServiceWorkerResponse> response) {
-    std::vector<ServiceWorkerResponse> responses;
-    if (error == CacheStorageError::kSuccess) {
-      DCHECK(response);
-      responses.push_back(std::move(*response));
-    }
-    OnCacheMatchAllCallback(std::move(callback), error, std::move(responses));
-  }
-
-  void Keys(const ServiceWorkerFetchRequest& request,
+  void Keys(const base::Optional<ServiceWorkerFetchRequest>& request,
             blink::mojom::QueryParamsPtr match_params,
             KeysCallback callback) override {
     content::CacheStorageCache* cache = cache_handle_.value();
@@ -163,9 +141,13 @@ class CacheStorageDispatcherHost::CacheImpl
       return;
     }
 
-    auto request_ptr = std::make_unique<ServiceWorkerFetchRequest>(
-        request.url, request.method, request.headers, request.referrer,
-        request.is_reload);
+    std::unique_ptr<ServiceWorkerFetchRequest> request_ptr;
+
+    if (request) {
+      request_ptr = std::make_unique<ServiceWorkerFetchRequest>(
+          request->url, request->method, request->headers, request->referrer,
+          request->is_reload);
+    }
     cache->Keys(
         std::move(request_ptr), std::move(match_params),
         base::BindOnce(&CacheImpl::OnCacheKeysCallback,

third_party/blink/public/platform/modules/cache_storage/cache_storage.mojom

@@ -89,12 +89,12 @@ interface CacheStorageCache {
 
   // Returns all cached responses that match |request| according to options
   // specified on |query_params|.
-  MatchAll(blink.mojom.FetchAPIRequest request, QueryParams query_params)
+  MatchAll(blink.mojom.FetchAPIRequest? request, QueryParams query_params)
       => (MatchAllResult result);
 
   // Returns all keys (which are requests) of matching |request| and
   // |query_params|.
-  Keys(blink.mojom.FetchAPIRequest request, QueryParams query_params)
+  Keys(blink.mojom.FetchAPIRequest? request, QueryParams query_params)
       => (CacheKeysResult result);
 
   // Perform a batch of operations, used for PUT and DELETE operations.

third_party/blink/public/platform/modules/fetch/fetch_api_request.mojom

@@ -101,11 +101,7 @@ struct FetchAPIRequest {
   RequestContextType request_context_type;
   network.mojom.RequestContextFrameType frame_type;
   url.mojom.Url url;
-  // TODO(lfg): |method| shouldn't be nullable. This is allowed in order to
-  // facilitate type conversions between FetchAPIRequest and
-  // WebServiceWorkerRequest, but should be made non-nullable once the browser
-  // process is converted to use the mojo type.
-  string? method;
+  string method;
   map<string, string> headers;
   SerializedBlob? blob;
   Referrer referrer;

third_party/blink/renderer/modules/cache_storage/cache.cc

@@ -6,6 +6,7 @@
 
 #include <memory>
 #include <utility>
+#include "base/optional.h"
 #include "services/network/public/mojom/fetch_api.mojom-blink.h"
 #include "third_party/blink/public/platform/modules/cache_storage/cache_storage.mojom-blink.h"
 #include "third_party/blink/public/platform/modules/serviceworker/web_service_worker_response.h"
@@ -573,12 +574,12 @@ ScriptPromise Cache::MatchImpl(ScriptState* script_state,
 ScriptPromise Cache::MatchAllImpl(ScriptState* script_state,
                                   const Request* request,
                                   const CacheQueryOptions& options) {
-  WebServiceWorkerRequest web_request;
+  base::Optional<WebServiceWorkerRequest> web_request;
   ScriptPromiseResolver* resolver = ScriptPromiseResolver::Create(script_state);
   const ScriptPromise promise = resolver->Promise();
 
   if (request) {
-    request->PopulateWebServiceWorkerRequest(web_request);
+    request->PopulateWebServiceWorkerRequest(web_request.emplace());
 
     if (request->method() != HTTPNames::GET && !options.ignoreMethod()) {
       resolver->Resolve(HeapVector<Member<Response>>());
@@ -764,12 +765,12 @@ ScriptPromise Cache::PutImpl(ScriptState* script_state,
 ScriptPromise Cache::KeysImpl(ScriptState* script_state,
                               const Request* request,
                               const CacheQueryOptions& options) {
-  WebServiceWorkerRequest web_request;
+  base::Optional<WebServiceWorkerRequest> web_request;
   ScriptPromiseResolver* resolver = ScriptPromiseResolver::Create(script_state);
   const ScriptPromise promise = resolver->Promise();
 
   if (request) {
-    request->PopulateWebServiceWorkerRequest(web_request);
+    request->PopulateWebServiceWorkerRequest(web_request.emplace());
 
     if (request->method() != HTTPNames::GET && !options.ignoreMethod()) {
       resolver->Resolve(HeapVector<Member<Response>>());

third_party/blink/renderer/modules/cache_storage/cache_test.cc

@@ -9,6 +9,7 @@
 #include <string>
 
 #include "base/memory/ptr_util.h"
+#include "base/optional.h"
 #include "mojo/public/cpp/bindings/associated_binding.h"
 #include "services/network/public/mojom/fetch_api.mojom-blink.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -146,23 +147,24 @@ class ErrorCacheForTests : public mojom::blink::CacheStorageCache {
     result->set_status(error_);
     std::move(callback).Run(std::move(result));
   }
-  void MatchAll(const WebServiceWorkerRequest& request,
+  void MatchAll(const base::Optional<WebServiceWorkerRequest>& request,
                 mojom::blink::QueryParamsPtr query_params,
                 MatchAllCallback callback) override {
     last_error_web_cache_method_called_ = "dispatchMatchAll";
-    CheckUrlIfProvided(request.Url());
+    if (request)
+      CheckUrlIfProvided(request->Url());
     CheckQueryParamsIfProvided(query_params);
     mojom::blink::MatchAllResultPtr result =
         mojom::blink::MatchAllResult::New();
     result->set_status(error_);
     std::move(callback).Run(std::move(result));
   }
-  void Keys(const WebServiceWorkerRequest& request,
+  void Keys(const base::Optional<WebServiceWorkerRequest>& request,
             mojom::blink::QueryParamsPtr query_params,
             KeysCallback callback) override {
     last_error_web_cache_method_called_ = "dispatchKeys";
-    if (!request.Url().IsEmpty()) {
-      CheckUrlIfProvided(request.Url());
+    if (request && !request->Url().IsEmpty()) {
+      CheckUrlIfProvided(request->Url());
       CheckQueryParamsIfProvided(query_params);
     }
     mojom::blink::CacheKeysResultPtr result =
@@ -418,6 +420,13 @@ TEST_F(CacheStorageTest, BasicArguments) {
       CreateCache(fetcher, std::make_unique<NotImplementedErrorCache>());
   DCHECK(cache);
 
+  ScriptPromise match_all_result_no_arguments =
+      cache->matchAll(GetScriptState(), exception_state);
+  EXPECT_EQ("dispatchMatchAll",
+            test_cache()->GetAndClearLastErrorWebCacheMethodCalled());
+  EXPECT_EQ(kNotImplementedString,
+            GetRejectString(match_all_result_no_arguments));
+
   const String url = "http://www.cache.arguments.test/";
   test_cache()->SetExpectedUrl(&url);
 
@@ -613,7 +622,7 @@ class KeysTestCache : public NotImplementedErrorCache {
   KeysTestCache(Vector<WebServiceWorkerRequest>& requests)
       : requests_(requests) {}
 
-  void Keys(const WebServiceWorkerRequest& request,
+  void Keys(const base::Optional<WebServiceWorkerRequest>& request,
             mojom::blink::QueryParamsPtr query_params,
             KeysCallback callback) override {
     mojom::blink::CacheKeysResultPtr result =
@@ -639,7 +648,9 @@ TEST_F(CacheStorageTest, KeysResponseTest) {
 
   Vector<WebServiceWorkerRequest> web_requests(size_t(2));
   web_requests[0].SetURL(KURL(url1));
+  web_requests[0].SetMethod("GET");
   web_requests[1].SetURL(KURL(url2));
+  web_requests[1].SetMethod("GET");
 
   Cache* cache =
       CreateCache(fetcher, std::make_unique<KeysTestCache>(web_requests));
@@ -665,7 +676,7 @@ class MatchAllAndBatchTestCache : public NotImplementedErrorCache {
   MatchAllAndBatchTestCache(Vector<mojom::blink::FetchAPIResponsePtr> responses)
       : responses_(std::move(responses)) {}
 
-  void MatchAll(const WebServiceWorkerRequest& request,
+  void MatchAll(const base::Optional<WebServiceWorkerRequest>& request,
                 mojom::blink::QueryParamsPtr query_params,
                 MatchAllCallback callback) override {
     mojom::blink::MatchAllResultPtr result =

third_party/blink/renderer/modules/cache_storage/inspector_cache_storage_agent.cc

@@ -310,7 +310,8 @@ class GetCacheKeysForRequestData {
 
   void Dispatch(std::unique_ptr<GetCacheKeysForRequestData> self) {
     cache_ptr_->Keys(
-        WebServiceWorkerRequest(), mojom::blink::QueryParams::New(),
+        base::Optional<WebServiceWorkerRequest>(),
+        mojom::blink::QueryParams::New(),
         WTF::Bind(
             [](DataRequestParams params,
                std::unique_ptr<GetCacheKeysForRequestData> self,
@@ -547,6 +548,7 @@ void InspectorCacheStorageAgent::deleteEntry(
               auto& operation = batch_operations.back();
               operation->operation_type = mojom::blink::OperationType::kDelete;
               operation->request.SetURL(KURL(request));
+              operation->request.SetMethod("GET");
 
               mojom::blink::CacheStorageCacheAssociatedPtr cache_ptr;
               cache_ptr.Bind(std::move(result->get_cache()));
@@ -587,6 +589,7 @@ void InspectorCacheStorageAgent::requestCachedResponse(
   }
   WebServiceWorkerRequest request;
   request.SetURL(KURL(request_url));
+  request.SetMethod("GET");
   cache_storage->Match(
       request, mojom::blink::QueryParams::New(),
       WTF::Bind(