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"
+#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 {
+
+// An interface to the Cache API, implemented by the embedder and passed in to blink. Blink's implementation

[dominicc (has gone to gerrit), 2014/08/11 01:40:41]

blink -> Blink; below too.

I would only write blink when you intend to refer specifically to the namespace.

[dominicc (has gone to gerrit), 2014/08/11 01:40:42]

Can this be tightened up? "An interface to the Cache Application Programming
Interface" seems duplicative; what about:

The Service Worker Cache API. The embedder provides the implementation of the
Cache to Blink. Blink uses this interface to operate on stores and entries.

(I think the bit about callbacks is self evident from good naming below.)

[gavinp, 2014/08/13 23:25:23]

Done.

[gavinp, 2014/08/13 23:25:23]

Done.

+// of the ServiceWorker spec will call these methods to perform operations on stores and the entries in stores, and
+// will expect callbacks from the embedder after operations complete.
+class WebServiceWorkerCache {
+public:
+    typedef WebCallbacks<WebServiceWorkerResponse, WebServiceWorkerCacheError> CacheMatchCallbacks;
+    typedef WebCallbacks<WebVector<WebServiceWorkerResponse>, WebServiceWorkerCacheError> CacheWithResponsesCallbacks;
+    typedef WebCallbacks<WebVector<WebServiceWorkerRequest>, WebServiceWorkerCacheError> CacheWithRequestsCallbacks;
+
+    // The ProxyInterface is used to allow a single object in blink to serve as its interface in JavaScript; so that == will
+    // always test two interfaces to the same cache as equal.
+    class ProxyInterface {

[dominicc (has gone to gerrit), 2014/08/11 01:40:41]

Not sure about naming this ...Interface; what's the thinking here?

[gavinp, 2014/09/04 19:47:33]

It holds the blink::Cache object, which must be an interface (at least that's
what Cache.idl tells me...) and the Cache object is basically a proxy around
WebServiceWorkerCache, since most calls to Cache::foo() end up being a single
call into WebServiceWorkerCache. Any ideas for a better name?

+    public:
+        virtual inline ~ProxyInterface() = 0;

[dominicc (has gone to gerrit), 2014/08/11 01:40:42]

I guess this should be abstract by virtue of this being public/ ... +1

[gavinp, 2014/08/13 23:25:23]

It should be abstract in order to guard against instantiating this totally
useless class; only a descendent of this class is reasonable, and abstract is
the concept that protects against instantiating meaningless classes. Having the
destructor be public is moot, I just put that in as boilerplate... In this case
I don't think visibility matters for the destructor. Would removing the
boilerplate help?

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

[dominicc (has gone to gerrit), 2014/08/11 01:40:42]

Regardless of how the spec has it, would it not be less unreadable to make these
positive ("includeSearch" or something) to avoid double negatives? And do the
mapping from spec-ese (which probably wants to use false as defaults for reasons
unrelated to C++, perhaps because false is similarly falsy to a missing value in
JavaScript) at a relatively early point in Blink?

[gavinp, 2014/08/13 23:25:23]

I like this suggestion. I'll follow up with an upload that does this shortly,
but the current upload lacks this.

+
+        bool ignoreSearch;
+        bool ignoreMethod;
+        bool ignoreVary;
+        bool prefixMatch;
+    };
+
+    enum WebServiceWorkerCacheOperationType {
+        WebServiceWorkerCacheOperationTypeUndefined,
+        WebServiceWorkerCacheOperationTypePut,
+        WebServiceWorkerCacheOperationTypeDelete,
+        WebServiceWorkerCacheOperationTypeLast = WebServiceWorkerCacheOperationTypeUndefined
+    };
+
+    struct BatchOperation {
+        BatchOperation() : operationType(WebServiceWorkerCacheOperationTypeUndefined) { }
+
+        WebServiceWorkerCacheOperationType operationType;
+        WebServiceWorkerRequest request;
+        WebServiceWorkerResponse response;
+        QueryParams matchParams;
+    };
+
+    WebServiceWorkerCache() : m_proxyInterface(0) { }
+    virtual ~WebServiceWorkerCache() { }
+
+    void setProxyInterface(ProxyInterface* proxyInterface) { m_proxyInterface = proxyInterface; }

[dominicc (has gone to gerrit), 2014/08/11 01:40:42]

Classes here usually fall into one of four categories:

1. Ones implemented by the embedder.

2. Ones implemented by Blink and exported to the embedder.

3. "Primitive" types like WebURL that are a bit of both (inline methods;
INSIDE_BLINK ifdefs; knows about GURL in the embedder.)

4. Ones past the limit of Dominic's knowledge.

The first type (which this is) tend strongly to have virtual methods, pure
virtual or with empty/default value returning bodies for two-sided patch
landiness, and no storage.

This should hew to that.

[gavinp, 2014/08/13 23:25:23]

I'm going to set up a VC for us to talk about this; I wonder if the proxy
interface stuff should really be implemented in the embedder?

+    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;
+
+    // Called when blink is no longer using this object.
+    virtual void notifyDone() = 0;

[dominicc (has gone to gerrit), 2014/08/11 01:40:42]

This method has a very generic name for a very specific purpose.

[gavinp, 2014/08/13 23:25:23]

And it's gone in the latest upload, as I cleared up ownership.

+
+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

[dominicc (has gone to gerrit), 2014/08/11 01:40:42]

If it's mysterious enough to warrant a comment, is it worth doing?

[gavinp, 2014/08/13 23:25:23]

I don't know. I added the comment because the last time I created one of these
types of method free abstract classes, the same question came up.

Adding a constructor to the class seems to complicate the interface and make
writing descendents harder; although I guess something similar can be done by
just adding a do-nothing protected constructor. Of course, now that requires
that descendents declare a constructor they don't need.

So I'm beat. I'll do whatever the reviewers say here, but my intuition is that
the pure virtual destructor with a body (that is in this case also inline by
keyword) is the right call.... But I'm far from sure.

[dominicc (has gone to gerrit), 2014/08/14 02:35:12]

I'm fine with the inline virtual destructor either way.

In the end a level 27 C++ Priest will come to kill you, and a level 29 C++
Priestess will come to kill *that* priest. Good luck!

+// use the inline keyword and provide an out of line definition.
+inline WebServiceWorkerCache::ProxyInterface::~ProxyInterface() { }
+
+} // namespace blink
+
+#endif // WebServiceWorkerCache_h