Reporting: Implement delivery agent.

net/reporting/reporting_delivery_agent.h

Index: net/reporting/reporting_delivery_agent.h
diff --git a/net/reporting/reporting_delivery_agent.h b/net/reporting/reporting_delivery_agent.h
new file mode 100644
index 0000000000000000000000000000000000000000..510944ad248769cb24835aaf4e0c2672265095ef
--- /dev/null
+++ b/net/reporting/reporting_delivery_agent.h
@@ -0,0 +1,100 @@
+// 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_DELIVERY_AGENT_H_
+#define NET_REPORTING_REPORTING_DELIVERY_AGENT_H_
+
+#include <memory>
+#include <set>
+#include <string>
+#include <utility>
+
+#include "base/macros.h"
+#include "base/memory/weak_ptr.h"
+#include "net/base/backoff_entry.h"
+#include "net/base/net_export.h"
+#include "net/reporting/reporting_endpoint_manager.h"
+#include "net/reporting/reporting_uploader.h"
+#include "url/gurl.h"
+#include "url/origin.h"
+
+namespace base {
+class TickClock;
+}  // namespace base
+
+namespace net {
+
+class ReportingCache;
+
+// Takes reports from the ReportingCache, assembles reports into deliveries to
+// endpoints, and sends those deliveries using ReportingUploader.
+//
+// Since the Reporting spec is completely silent on issues of concurrency, the
+// delivery agent handles it as so:
+//
+// 1. An individual report can only be included in one delivery at once -- if
+//    SendReports is called again while a report is being delivered, it won't
+//    be included in another delivery during that call to SendReports. (This is,
+//    in fact, made redundant by rule 3, but it's included anyway in case rule 3
+//    changes.)
+//
+// 2. An endpoint can only be the target of one delivery at once -- if
+//    SendReports is called again with reports that could be delivered to that
+//    endpoint, it won't be chosen. This may mean that the new reports are not

[Randy Smith (Not in Mondays), 2017/04/05 19:02:38]

The antecedent of "it" isn't clear to me in this sentence.  Does it refer to the
reports that could be delivered?  (This is a request for a comment
clarification.)

[Julia Tuttle, 2017/04/06 17:03:14]

Done.

+//    delivered anywhere during that call to SendReports.
+//
+// 3. Reports for an (origin, group) tuple can only be included in one delivery
+//    at once -- if SendReports is called again with reports in that (origin,
+//    group), they won't be included in any delivery during that call to
+//    SendReports. (This prevents the agent from getting around rule 2 by using
+//    other endpoints.)
+//
+// 4. Reports for the same origin *can* be included in multiple parallel
+//    deliveries if they are in different groups within that origin.
+//
+// (Note that a single delivery can contain an infinite number of reports.)
+//
+// TODO(juliatuttle): Consider capping the maximum number of reports per
+// delivery attempt.
+class NET_EXPORT ReportingDeliveryAgent {
+ public:
+  // |clock|, |cache|, |uploader|, and |endpoint_backoff_policy| must all
+  // outlive the ReportingDeliveryAgent.
+  ReportingDeliveryAgent(base::TickClock* clock,
+                         ReportingCache* cache,
+                         ReportingUploader* uploader,
+                         const BackoffEntry::Policy* endpoint_backoff_policy);
+  ~ReportingDeliveryAgent();
+
+  // Tries to deliver all of the reports in the cache. Reports that are already
+  // being delivered will not be attempted a second time, and reports that do
+  // not have a viable endpoint will be neither attempted nor removed.
+  void SendReports();

[Randy Smith (Not in Mondays), 2017/04/05 19:02:38]

Why is this controlled by a consumer rather than being on a timer or triggered
by the cache notifying the agent of changes?   I'm not certain that would be a
better design, but the problem of this type of salami CL approach is that it's
hard to evaluate, so I figured I'd ask.

[Julia Tuttle, 2017/04/06 17:03:14]

It'll be doing exactly that eventually, but I wanted to land the basic delivery
code first.

+
+ private:
+  class Delivery;
+
+  using OriginGroup = std::pair<url::Origin, std::string>;
+
+  void OnUploadComplete(const std::unique_ptr<Delivery>& delivery,
+                        ReportingUploader::Outcome outcome);
+
+  base::TickClock* clock_;
+  ReportingCache* cache_;
+  ReportingUploader* uploader_;
+
+  ReportingEndpointManager endpoint_manager_;
+
+  // Tracks OriginGroup tuples for which there is a pending delivery running.
+  // (Would be an unordered_set, but there's no hash on pair.)

[Randy Smith (Not in Mondays), 2017/04/05 19:02:38]

Suggestion: I'd erase this last sentence.  It invites a bikeshed, which starts
with what I was about to write, which is "You could totally define your own
hash", to which the right response is "This is totally not a design choice that
matters in any way shape or form", which would then be responded to with "So is
the parenthetical note worth the comment space?"  :-}

[Julia Tuttle, 2017/04/06 17:03:14]

The parenthetical note is there because otherwise Shivani (and presumably
others) will wonder why I didn't use unordered_set. Adding a hash is Not My
Problem, but using the best data structure available in my code is.

+  std::set<OriginGroup> pending_origin_groups_;
+
+  base::WeakPtrFactory<ReportingDeliveryAgent> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(ReportingDeliveryAgent);
+};
+
+}  // namespace net
+
+#endif  // NET_REPORTING_REPORTING_DELIVERY_AGENT_H_