Add net::CertificateReportSender for handling cert report sending

net/http/certificate_report_sender.h

Index: net/http/certificate_report_sender.h
diff --git a/net/http/certificate_report_sender.h b/net/http/certificate_report_sender.h
new file mode 100644
index 0000000000000000000000000000000000000000..4ea8755328cb723fcbafc2a14c29da9f4cd16dc8
--- /dev/null
+++ b/net/http/certificate_report_sender.h
@@ -0,0 +1,31 @@
+// Copyright 2015 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_HTTP_CERTIFICATE_REPORT_SENDER_H_
+#define NET_HTTP_CERTIFICATE_REPORT_SENDER_H_
+
+#include <string>
+
+class GURL;
+
+namespace net {
+
+class URLRequestContext;
+
+// An interface for asynchronously sending serialized certificate
+// reports to a URI.

[Ryan Sleevi, 2015/06/26 20:03:46]

nit: Seems like this could be expanded on.

I guess what I'd hope for is a little context as to the motivation - what's
special about this class/this split.

For example, why isn't the format for |report| specified (it's just a sequence
of bytes), or what's "tricky" about this interface that a caller couldn't just
use URLFetcher. That sort of question that basically answers, if looking at just
this header file, "Why should this exist?"

That said, it's also desirable not to describe how it's used or specific
implementations.

Would it be easier to document this class if it was part of the
TransportSecurityState header - to clearly show how tightly coupled these are?
(This is similar to URLRequest/::Delegate). Just trying to explore that space of
guiding the reader.

[estark, 2015/07/07 21:59:28]

Done.
I can't seem to come up with a strong opinion either way on this. I originally
thought of it as a separate thing because it is used for SBER cert reports as
well as HPKP reports (so I thought of it as independent from
TransportSecurityState). But I suppose there's no reason why SBER cert reports
can't just use a CertificateReportSenderImpl even though it implements a
TSS-specific interface.

+class CertificateReportSender {
+ public:
+  // Represents whether or not to send cookies along with reports.
+  enum CookiesPreference { SEND_COOKIES, DO_NOT_SEND_COOKIES };

[Ryan Sleevi, 2015/06/26 20:03:46]

Does this really belong in the base class? It seems to be an aspect of the impl

[estark, 2015/07/07 21:59:28]

Done.

+
+  virtual ~CertificateReportSender() {}
+
+  // Send the given serialized |report| to |report_uri|.

[Ryan Sleevi, 2015/06/26 20:03:46]

Comments should be descriptive, rather than imperative -
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm...

// Sends the given serialized |report| to |report_uri|.

But it seems useful to also describe the context about what's permissible for
implementations and what's expected. Can they ignore reports? Batch reports?
Reiterate the asynchronicity.

If an implementation of a Sender isn't API-contractually bound to send a report
(e.g. because of rate throttling), maybe a different word than "Send" is
desirable here, to indicate there's some flexibility.

Outside of "s/Send/Sends/", the rest I'd clarify are nits.

[estark, 2015/07/07 21:59:28]

Done.

+  virtual void Send(const GURL& report_uri, const std::string& report) = 0;
+};
+
+}  // namespace net
+
+#endif  // NET_HTTP_CERTIFICATE_REPORT_SENDER_H_