[OBSOLETE] Reporting: Initial implementation.

net/reporting/reporting_report.h

Index: net/reporting/reporting_report.h
diff --git a/net/reporting/reporting_report.h b/net/reporting/reporting_report.h
new file mode 100644
index 0000000000000000000000000000000000000000..7bea214190586e664f77c7841c71041f6bfea95a
--- /dev/null
+++ b/net/reporting/reporting_report.h
@@ -0,0 +1,42 @@
+// 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 NET_REPORTING_REPORTING_REPORT_H_
+#define NET_REPORTING_REPORTING_REPORT_H_
+
+#include "base/time/time.h"
+#include "base/values.h"
+#include "url/gurl.h"
+
+namespace net {
+
+// A report queued by another component to be delivered by Reporting.
+struct ReportingReport {

[Ryan Sleevi, 2017/01/03 21:28:13]

From a design standpoint, this feels more like a class (or will evolve into one)
than a struct, per
https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes

[Julia Tuttle, 2017/01/25 20:27:46]

My gut feeling is that it's a struct, but I'm making it a class since it's got a
unique_ptr member and (therefore, and since I don't *need* to copy or assign it)
I'm adding DISALLOW_COPY_OR_ASSIGN.

+ public:
+  ReportingReport();
+  ~ReportingReport();

[Ryan Sleevi, 2017/01/03 21:28:13]

Rule of three applies here - in general, if you provide a dtor, you should
provide copy and assignment.

It would seem you're not doing so here because you're only concerned about
squelching warnings about complex ctors/dtors (and the code-size implication
they have), but the same applies here to copy/assignment.

Further, the copy/assignment is complex (because of line 22) - so it deserves
either a DISALLOW_COPY_AND_ASSIGN (which further deflates the "is-a struct"
argument) or explicit management (which I'm unclear of what the right answer is)

[Julia Tuttle, 2017/01/25 20:27:46]

Done.

+
+  std::string ToString() const;

[Ryan Sleevi, 2017/01/03 21:28:13]

From reading the header: Why does this exist? What does this do? Is it for
debugging? GTEst? Is it the serializing of the class?

[Julia Tuttle, 2017/01/25 20:27:46]

It was for testing, but I'm gonna remove it for now.

+
+  std::unique_ptr<base::Value> body;

[Ryan Sleevi, 2017/01/03 21:28:13]

What is this? Why is it a base::Value versus another structure? Does anything
guarantee any form of consistency (or non-mutability)?

[Julia Tuttle, 2017/01/25 20:27:45]

It's the report body, which is any JSON.

+  // URL of content that triggered report.

[Ryan Sleevi, 2017/01/03 21:28:13]

nelines between 22-23

[Julia Tuttle, 2017/01/25 20:27:46]

Done.

+  GURL url;
+  // Origin of content that triggered report. Should be url.GetOrigin(), but the
+  // spec doesn't guarantee this, and allows callers to provide URL and origin
+  // separately, so they are separate fields for now.

[Ryan Sleevi, 2017/01/03 21:28:13]

Line break between 24-25 and 29-30

As comments go, I think this leaves more questions than answers. For example, is
there a bug filed on this? Why does the spec allow callers to separate them?
What's an example of when you should or should not? What happens if origin isn't
valid at all - what do you do?

[Julia Tuttle, 2017/01/25 20:27:46]

There is a bug filed, at https://github.com/WICG/reporting/issues/25. If the
origin isn't valid at all, the report will never get picked up for delivery, and
will expire after an hour or so.

+  GURL origin;
+  std::string group;
+  std::string type;
+
+  // Creation timestamp.
+  base::TimeTicks timestamp;

[Ryan Sleevi, 2017/01/03 21:28:13]

naming: "timestamp" is self-describing without adding new information. Should
this be base::TimeTicks creation_time?

Does this require monotonicity? Civil time conversion? How is this used? All
unclear.

[Julia Tuttle, 2017/01/25 20:27:45]

Renamed to "queued".
Added a comment to specify how it's used. It's reported to the endpoint as an
age, and reports aren't persisted across sessions, so I *think* TimeTicks is
preferable to Time.

+  // Number of delivery attempts made.
+  int attempts;

[Ryan Sleevi, 2017/01/03 21:28:13]

size_t for counts -
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

[Julia Tuttle, 2017/01/25 20:27:45]

Done.

+  // Whether this report is currently part of a pending delivery attempt.
+  bool pending;

[Ryan Sleevi, 2017/01/03 21:28:13]

This feels like an interface bleed - why should the report keep track of whether
it itself is pending? This seems to leak in details about how the report is
used, rather than what a report is.

The same applies to |attempts|. Both of these seem to represent a state about-a
report, not state that is-a report.

[Julia Tuttle, 2017/01/25 20:27:46]

Obsolete; I'm making Report an inner, private class of ReportingService, so
callers don't have to deal with it at all.

+};
+
+}  // namespace net
+
+#endif  // NET_REPORTING_REPORTING_REPORT_H_