[OBSOLETE] Reporting: Initial implementation.

components/reporting/core/browser/reporting_cache.h

Index: components/reporting/core/browser/reporting_cache.h
diff --git a/components/reporting/core/browser/reporting_cache.h b/components/reporting/core/browser/reporting_cache.h
new file mode 100644
index 0000000000000000000000000000000000000000..fb3422e1adb97362821412d714bcacb8fca2e3bd
--- /dev/null
+++ b/components/reporting/core/browser/reporting_cache.h
@@ -0,0 +1,49 @@
+// Copyright 2016 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 COMPONENTS_REPORTING_CORE_BROWSER_REPORTING_CACHE_H_
+#define COMPONENTS_REPORTING_CORE_BROWSER_REPORTING_CACHE_H_
+
+#include <map>
+#include <unordered_set>
+
+#include "components/reporting/core/browser/reporting_endpoint.h"
+#include "components/reporting/core/browser/reporting_report.h"
+#include "components/reporting/core/common/reporting_export.h"
+#include "url/gurl.h"
+
+namespace reporting {
+
+// Section 2.4

[Randy Smith (Not in Mondays), 2016/10/21 20:15:11]

I presume this is a reference into the spec, but I think files should be
understandable without context, so I'd include the full reference.

Also, a comment summarizing the goal/service this class is vending would be
valuable (both for future readers and for your current reviewer :-}).  Requiring
people to read the spec to understand the code isn't completely friendly; there
are times when it's the right move, but this doesn't strike me as one.  "Storage
for active endpoints and queued reports." would do it here, for me.

[Julia Tuttle, 2016/11/02 20:44:39]

Done.

+class REPORTING_EXPORT ReportingCache {

[Randy Smith (Not in Mondays), 2016/10/21 20:15:11]

Not actionable: From a purely code analysis POV, the functionality this class
provides doesn't strike me as justifying a separate class.  The queue of
endpoints is conceptually separate from the queue of reports, and both strike me
as pretty easily managed by just having an EndpointMap/ReportSet in whatever
object owns this one.  I do understand that the spec calls out the concept of a
ReportingCache, but specs don't actually specify implementation (usually) and
what I take that section as saying is "This is the information that the
implementation is responsible for storing over time (between header reception,
report queueing, and report delivery, specifically)."  So as I do the rest of
the review I may want to nuke this class and have a comment somewhere explaining
how the Reporting Cache concept from the spec is implemented.  Mentioning it now
so that you can argue with the philosophy up front if you want.

[Julia Tuttle, 2016/11/02 20:44:39]

I will end up making the cache able to serialize and unserialize itself
eventually, at which point it may not exist until later, and will have more code
than just trivial map/set manipulation.

That said, a part of me feels like flattening Reporting{Cache,Manager,Service}
would simplify things quite a bit.

Thoughts?

+ public:
+  // EndpointMap would be an unordered_map, but there's no hash for GURL.
+  using EndpointMap = std::map<GURL, std::unique_ptr<ReportingEndpoint>>;
+  using ReportSet = std::unordered_set<std::unique_ptr<ReportingReport>>;
+
+  ReportingCache();
+  ~ReportingCache();
+
+  void InsertEndpoint(std::unique_ptr<ReportingEndpoint> endpoints);
+  const std::unique_ptr<ReportingEndpoint>* GetEndpoint(const GURL& url) const;

[Randy Smith (Not in Mondays), 2016/10/21 20:15:11]

Why return a const pointer to a std::unique_ptr<>?  That would seem to say "Look
but don't touch" but where is there about the owning pointer that's useful if
you don't touch?  I.e. why not return a const ReportingEndpoint*?  I think of
passing around unique_ptrs only when ownership is being transferred.

[Julia Tuttle, 2016/11/02 20:44:39]

I can't find a good way to erase from a vector<unique_ptr<T>> given a const T&
in O(1) -- there's no way to compare the two. Am I missing something? (I hope I
am.)

+  const EndpointMap& GetEndpoints() const;
+  void RemoveEndpoint(const std::unique_ptr<ReportingEndpoint>& endpoint);
+
+  void EnqueueReport(std::unique_ptr<ReportingReport> report);
+  const ReportSet& GetReports() const;
+  void DequeueReport(const std::unique_ptr<ReportingReport>& report);
+
+  void Clear();
+
+ private:
+  // Map from endpoint.url to endpoint.
+  EndpointMap endpoints_;
+  ReportSet reports_;
+
+  DISALLOW_COPY_AND_ASSIGN(ReportingCache);
+};
+
+}  // namespace reporting
+
+#endif  // COMPONENTS_REPORTING_CORE_BROWSER_REPORTING_CACHE_H_