Introducing the WebServiceWorkerCache.

public/platform/WebServiceWorkerCache.h

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef WebServiceWorkerCache_h
+#define WebServiceWorkerCache_h
+
+#include "WebCommon.h"

[tkent, 2014/09/18 23:35:56]

should be "public/platform/WebCommon.h"

[gavinp, 2014/09/22 16:05:24]

Done.

+#include "public/platform/WebCallbacks.h"
+#include "public/platform/WebServiceWorkerCacheError.h"
+#include "public/platform/WebServiceWorkerRequest.h"
+#include "public/platform/WebServiceWorkerResponse.h"
+#include "public/platform/WebString.h"
+#include "public/platform/WebVector.h"
+
+namespace blink {
+
+// The Service Worker Cache API. The embedder provides the implementation of the Cache to Blink. Blink uses the interface
+// to operate on entries.
+// This object is owned by Blink, and should be destroyed as each Cache instance is no longer in use.
+class WebServiceWorkerCache {
+public:
+    typedef WebCallbacks<WebServiceWorkerResponse, WebServiceWorkerCacheError> CacheMatchCallbacks;
+    typedef WebCallbacks<WebVector<WebServiceWorkerResponse>, WebServiceWorkerCacheError> CacheWithResponsesCallbacks;
+    typedef WebCallbacks<WebVector<WebServiceWorkerRequest>, WebServiceWorkerCacheError> CacheWithRequestsCallbacks;
+
+    virtual ~WebServiceWorkerCache() { }

[gavinp, 2014/09/22 16:05:24]

So.... how scary is it that this destructor is inlined, given that this class is
not trivial? It has m_proxyInterface as a member, but that's just a pointer,
with no destructor behaviour, so I _think_ this is safe and not going to
generate tons of code. Is this still really bad? Should we add a .cpp file?

+
+    // Blink can store its internal interface to this object as a ProxyInterface in the WebServiceWorkerCache. This allows

[jsbell, 2014/09/18 21:45:00]

I'm not completely thrilled with the naming here, but don't have great
suggestions.

I'm surprised we don't have this pattern elsewhere but I'm not spotting it. I
think in most other places where the embedder (chromium) provides an impl to
blink on demand it doesn't re-use the object if it's re-requested - e.g. because
blink satisfies the request on its own, or because there's no
identity-preservation requirement, so this link-up isn't required.

There's a similar use in WebServiceWorkerProxy. That and other examples indicate
it's acceptable do this in a typesafe way, using a forward declaration of
blink::Cache rather than an abstract interface. It'd also be nice to factor this
pattern out similar to the WebPrivatePtr/WebPrivateOwnPtr which could make it
typesafe and assert that it's assigned exactly once. But that can probably be
done in a follow-up.

We'll need an OWNERS review for the public API changes, who may have more input.

[tkent, 2014/09/18 23:35:56]

I have the same impression.  Can we make WebServiceWorkerCache when Cache needs
it?
Platform classes shouldn't refer to modules classes.

[jsbell, 2014/09/19 16:22:47]

It is happening on demand; Blink has asked Chromium for a cache, and Chromium's
impl of WSWC is what's being returned. Blink then mints a Cache and associates
it with the WSWC before handing the Cache off to script. Conceptually, the
ProxyInterface is implementing a "WeakMap" allowing a WSWC->Cache lookup. 

Within Blink we might use Supplementable for this.
That's what I thought... but it looks like some references have snuck in from
various modules. :( Maybe the secret cabal of API owners should do a quick
audit. Or maybe I'm just misinterpreting what I'm seeing.

+    // Blink to quickly find the correct internal interface when it is passed the same WebServiceWorkerCache object.
+    class ProxyInterface {
+    public:
+        virtual inline ~ProxyInterface() = 0;

[tkent, 2014/09/18 23:35:56]

Is |virtual inline| helpful?

[gavinp, 2014/09/22 16:05:24]

Yes. It's a trivial class, with no members, so inlining the destructor doesn't
have any cost. This is very similar to virtual ~Foo() {}, which occurs all over.
The only difference is that it's a pure virtual function; it's pure virtual to
make the class abstract, which seems important because it communicates that this
must be subclassed to be useful. The syntactic problem is that you can't provide
a body together with a pure virtual declaration, so it's done using the inline
keyword (see the end of this header to see the provided body).

The destructor requires a body, since the destructor is called using non-virtual
calling conventions from the destructors of classes that inherit from this. It's
always legal to provide a body for a pure virtual function, but in this case
it's required.

[jamesr, 2014/09/22 16:36:03]

No.

Just write this as:

virtual ~ProxyInterface() { }

the interface is not pure virtual because there are no pure virtual functions on
it, so declaring it as such is not useful.  The d'tor is empty so inlining it
means nothing.

In the Blink API, we normally call this an ExtraData type:

https://code.google.com/p/chromium/codesearch#search/&q=extradata%20file:thir...

+    };
+
+    // Options that affect the scope of searches.
+    struct QueryParams {
+        QueryParams() : ignoreSearch(false), ignoreMethod(false), ignoreVary(false), prefixMatch(false) { }
+
+        bool ignoreSearch;
+        bool ignoreMethod;
+        bool ignoreVary;
+        bool prefixMatch;
+        WebString cacheName;
+    };
+
+    enum WebServiceWorkerCacheOperationType {

[tkent, 2014/09/18 23:35:56]

nit: This is an inner enum, and the name can be shorter.  |OperationType| would
be enough.

[gavinp, 2014/09/22 16:05:24]

Done.

+        WebServiceWorkerCacheOperationTypeUndefined,
+        WebServiceWorkerCacheOperationTypePut,
+        WebServiceWorkerCacheOperationTypeDelete,
+        WebServiceWorkerCacheOperationTypeLast = WebServiceWorkerCacheOperationTypeDelete
+    };
+
+    struct BatchOperation {
+        BatchOperation() : operationType(WebServiceWorkerCacheOperationTypeUndefined) { }
+
+        WebServiceWorkerCacheOperationType operationType;
+        WebServiceWorkerRequest request;
+        WebServiceWorkerResponse response;
+        QueryParams matchParams;
+    };
+
+    WebServiceWorkerCache() : m_proxyInterface(0) { }
+
+    void setProxyInterface(ProxyInterface* proxyInterface) { m_proxyInterface = proxyInterface; }
+    ProxyInterface* proxyInterface() { return m_proxyInterface; }
+
+    // Ownership of the Cache*Callbacks methods passes to the WebServiceWorkerCache instance, which will delete it after
+    // calling onSuccess or onFailure.
+    virtual void dispatchMatch(CacheMatchCallbacks*, const WebServiceWorkerRequest&, const QueryParams&) = 0;
+    virtual void dispatchMatchAll(CacheWithResponsesCallbacks*, const WebServiceWorkerRequest&, const QueryParams&) = 0;
+    virtual void dispatchKeys(CacheWithRequestsCallbacks*, const WebServiceWorkerRequest*, const QueryParams&) = 0;
+    virtual void dispatchBatch(CacheWithResponsesCallbacks*, const WebVector<BatchOperation>&) = 0;
+
+private:
+    ProxyInterface* m_proxyInterface;
+};
+
+// The pure virtual destructor makes the interface abstract, but we still want an inline destructor for descendents to call. Thus we
+// use the inline keyword and provide an out of line definition.
+inline WebServiceWorkerCache::ProxyInterface::~ProxyInterface() { }
+
+} // namespace blink
+
+#endif // WebServiceWorkerCache_h