Reporting: Implement cache.

net/reporting/reporting_cache.h

Index: net/reporting/reporting_cache.h
diff --git a/net/reporting/reporting_cache.h b/net/reporting/reporting_cache.h
new file mode 100644
index 0000000000000000000000000000000000000000..8e699b83b3095bed4f8838959c72d4f35e330039
--- /dev/null
+++ b/net/reporting/reporting_cache.h
@@ -0,0 +1,183 @@
+// Copyright 2017 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 NET_REPORTING_REPORTING_CACHE_H_
+#define NET_REPORTING_REPORTING_CACHE_H_
+
+#include <map>
+#include <string>
+#include <vector>
+
+#include "base/macros.h"
+#include "base/time/time.h"
+#include "base/values.h"
+#include "net/base/net_export.h"
+#include "url/gurl.h"
+#include "url/origin.h"
+
+namespace net {
+
+class NET_EXPORT ReportingCache {

[jkarlin, 2017/03/15 18:54:32]

Class needs a comment. E.g., call out what in the spec this refers to (The
Storage Concept) and what its purpose is (e.g., memory-only storage of reports
and clients).

[Julia Tuttle, 2017/03/16 14:50:37]

Done.

+ public:
+  // An undelivered report.
+  class Report {

[jkarlin, 2017/03/15 18:54:32]

Seems like this should be a struct, given that all the members are public and
don't have _ suffixes

[Julia Tuttle, 2017/03/16 14:50:38]

Done.

+   public:
+    Report();

[jkarlin, 2017/03/15 18:54:32]

How about a constructor that takes all of the params?

[Julia Tuttle, 2017/03/16 14:50:38]

Done.

+    ~Report();
+
+    // The URL of the document that triggered the report.
+    GURL url;
+
+    // The endpoint group that should be used to deliver the report.
+    std::string group;
+
+    // The type of the report.
+    std::string type;
+
+    // The body of the report.
+    std::unique_ptr<const base::Value> body;
+
+    // When the report was queued.
+    base::TimeTicks queued;

[jkarlin, 2017/03/15 18:54:32]

queued_time?

[Julia Tuttle, 2017/03/16 14:50:38]

Already says "time" in the type name :)

+
+    // The number of delivery attempts made so far, not including an active
+    // attempt.
+    int attempts;

[jkarlin, 2017/03/15 18:54:32]

failed_attempts?

[Julia Tuttle, 2017/03/16 14:50:37]

The *last* one might not be failed. Also, the spec calls it "attempts".

+
+    // Whether there is currently an active delivery attempt including this
+    // report.
+    bool pending;
+
+    // Whether this report should be removed once it is no longer part of an
+    // active delivery attempt. (Should never be set unless |pending| also is.)

[jkarlin, 2017/03/15 18:54:32]

Perhaps replace the parens with, "If true, |pending| must also be true."

[Julia Tuttle, 2017/03/16 14:50:38]

Done.

+    bool doomed;
+

[jkarlin, 2017/03/15 18:54:32]

Don't forget to add a CL to keep track of the memory requirements of
net/reporting/. Talk to Helen about how to do that.

[Julia Tuttle, 2017/03/16 14:50:37]

Wilco.

+   private:
+    DISALLOW_COPY_AND_ASSIGN(Report);
+  };
+
+  // The configuration by an origin to use an endpoint for report delivery.
+  struct Client {
+    Client();
+    Client(const Client& other);
+    ~Client();
+
+    // |origin| and |endpoint| are duplicated to make the interface to the cache
+    // simpler. (If they end up using too much memory, switching them to refs to
+    // the keys in |clients_| and the elements thereof should help.)
+
+    // The origin from which reports will be delivered.
+    url::Origin origin;
+
+    // The endpoint to which reports may be delivered. (Origins may configure
+    // many.)
+    GURL endpoint;
+
+    // Whether subdomains of the host of |origin| should also be handled by this
+    // client.
+    bool subdomains;
+
+    // The endpoint group to which this client belongs.
+    std::string group;
+
+    // When this client's max-age has expired.
+    base::TimeTicks expires;

[jkarlin, 2017/03/15 18:54:32]

Perhaps rename to ttl to match the spec?

[Julia Tuttle, 2017/03/16 14:50:37]

The spec stores "creation" and "ttl", but never uses them individually, just "is
creation + ttl < now" to check if it's expired. It would be a waste of a field
to store both.

Calling the expiration time itself "ttl" isn't quite right; it's a fixed point
in time, not the total or remaining time before expiration.

+
+    // Copy and assign allowed.
+  };
+
+  ReportingCache();
+  ~ReportingCache();
+
+  // Adds a report to the cache.
+  //
+  // All parameters correspond to the desired values for the relevant fields in
+  // |Report|.
+  void AddReport(const GURL& url,

[jkarlin, 2017/03/15 18:54:32]

Why not AddReport(std::unique_ptr<Report> report)? Otherwise this is more of a
CreateAndAddReport. It looks like the implementation just makes a new Report on
the heap anyway so a unique_ptr should work fine.

[Julia Tuttle, 2017/03/16 14:50:38]

I'd rather save callers the hassle of assembling it, and keep the constructor to
Report private, to clarify that the cache owns them all.

+                 const std::string& group,
+                 const std::string& type,
+                 std::unique_ptr<const base::Value> body,
+                 base::TimeTicks queued,
+                 int attempts);
+
+  // Gets all reports in the cache. The returned pointers are valid as long as
+  // either no calls to |RemoveReports| have happened or the reports' |pending|
+  // flag has been set to true using |SetReportsPending|. Does not return
+  // doomed reports (pending reports for which removal has been requested).
+  void GetReports(std::vector<const Report*>* reports_out) const;

[jkarlin, 2017/03/15 18:54:32]

Specify that reports_out gets cleared.

[Julia Tuttle, 2017/03/16 14:50:37]

Done.

+
+  // Sets |pending| on a set of reports.
+  void SetReportsPending(const std::vector<const Report*>& reports);
+
+  // Clear |pending| on a set of reports.
+  void ClearReportsPending(const std::vector<const Report*>& reports);
+
+  // Increments |attempts| on a set of reports.
+  void IncrementReportsAttempts(const std::vector<const Report*>& reports);
+
+  // Removes a set of reports. Any reports that are pending will not be removed
+  // immediately, but rather marked doomed and removed once they are no longer
+  // pending.
+  void RemoveReports(const std::vector<const Report*>& reports);
+
+  // Creates or updates a client for a particular origin and a particular
+  // endpoint.
+  //
+  // All parameters correspond to the desired values for the fields in
+  // |Client|.
+  //
+  // |endpoint| must use a cryptographic scheme; if it doesn't, SetClient will
+  // trigger a NOTREACHED() and return without setting the client.

[jkarlin, 2017/03/15 18:54:32]

I'd leave it at |endpoint| must use a cryptographic scheme. Don't reveal the
implementation details of what will happen if it's not. Those may change and the
caller shouldn't need to know or care.

[Julia Tuttle, 2017/03/16 14:50:38]

Done.

+  void SetClient(const url::Origin& origin,
+                 const GURL& endpoint,
+                 bool subdomains,
+                 const std::string& group,
+                 base::TimeTicks expires);
+
+  // Gets all of the clients in the cache, regardless of origin or group.
+  void GetClients(std::vector<const Client*>* clients_out) const;

[jkarlin, 2017/03/15 18:54:32]

Specify that clients_out gets cleared.

[Julia Tuttle, 2017/03/16 14:50:37]

Done.

+
+  // Gets all of the clients configured for a particular origin in a particular
+  // group. The returned pointers are only guaranteed to be valid if no calls
+  // have been made to |SetClient| or |RemoveEndpoint| in between.
+  void GetClientsForOriginAndGroup(
+      const url::Origin& origin,
+      const std::string& group,
+      std::vector<const Client*>* clients_out) const;
+
+  // Removes a set of clients.
+  //
+  // May invalidate Client pointers returned by |GetClients| or
+  // |GetClientsForOriginAndGroup|.
+  void RemoveClients(const std::vector<const Client*>& clients);
+
+  // Removes a client for a particular origin and a particular endpoint.
+  void RemoveClientForOriginAndEndpoint(const url::Origin& origin,
+                                        const GURL& endpoint);
+
+  // Removes all clients whose endpoint is |endpoint|.
+  //
+  // May invalidate Client pointers returned by |GetClients| or
+  // |GetClientsForOriginAndGroup|.
+  void RemoveClientsForEndpoint(const GURL& endpoint);
+
+  // Gets the count of reports in the cache, *including* doomed reports.
+  //
+  // Needed to ensure that doomed reports are eventually deleted, since no
+  // method provides a view of *every* report in the cache, just non-doomed
+  // ones.
+  size_t GetFullReportCountForTesting() const { return reports_.size(); }
+
+ private:
+  // Map from origin to map from endpoint to client.
+  std::map<url::Origin, std::map<GURL, Client>> clients_;
+  // Map from raw pointer to unique_ptr.

[jkarlin, 2017/03/15 18:54:32]

whitespace between comment and previous line

[Julia Tuttle, 2017/03/16 14:50:38]

Done.

+  std::map<const Report*, std::unique_ptr<Report>> reports_;
+
+  DISALLOW_COPY_AND_ASSIGN(ReportingCache);
+};
+
+}  // namespace net
+
+#endif  // NET_REPORTING_REPORTING_CACHE_H_