Introduce CertificateReportingService class to handle certificate reports.

chrome/browser/ssl/certificate_reporting_service.h

+// 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 CHROME_BROWSER_SSL_CERTIFICATE_REPORTING_SERVICE_H_
+#define CHROME_BROWSER_SSL_CERTIFICATE_REPORTING_SERVICE_H_
+
+#include <map>
+#include <memory>
+#include <string>
+#include <vector>
+
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/threading/thread_checker.h"
+#include "base/time/time.h"
+#include "components/certificate_reporting/error_reporter.h"
+#include "components/keyed_service/core/keyed_service.h"
+
+namespace base {
+class Clock;
+}
+
+// This service initiates uploads invalid certificate reports and retries any

[estark, 2016/11/17 14:51:59]

extra word in this sentence?

[meacer, 2016/11/17 20:09:08]

Missing, in fact :)

+// failed uploads.
+class CertificateReportingService : public KeyedService {
+ public:
+  // Represent a report to be sent.
+  struct Report {
+    int report_id;
+    base::Time creation_time;
+    std::string serialized_report;
+    Report() {}

[estark, 2016/11/17 14:51:59]

do you need this constructor?

[meacer, 2016/11/17 20:09:08]

[] operator of std::map needs it, but I modified the code to avoid using it.
Slightly more code, but got rid of this constructor.

+    Report(int report_id,
+           base::Time creation_time,
+           const std::string& serialized_report)
+        : report_id(report_id),
+          creation_time(creation_time),
+          serialized_report(serialized_report) {}
+  };
+
+  // This class contains a number of reports, sorted by the first time the
+  // report was to be sent. Oldest reports are at the end of the list. The
+  // number of reports are bounded by |max_size|. The implementation sorts all
+  // items in the list whenever a new item is added. This should be fine for
+  // small values of |max_size| (e.g. fewer than 100 items). In case this is not
+  // sufficient in the future, an array based implementation should be
+  // considered where the array is maintained as a heap.
+  class BoundedReportList {
+   public:
+    explicit BoundedReportList(size_t max_size);
+    ~BoundedReportList();
+
+    void Add(const Report& report);
+    void Clear();
+
+    const std::vector<Report>& items() const;
+
+   private:
+    // Maximum number of reports in the list. A newly added item is compared
+    // to the items in the list and only added when it's newer than the
+    // oldest item in the list.

[estark, 2016/11/17 14:51:59]

This sentence "A newly added item is compared..." only applies when the number
of items == max_size_, right?

[meacer, 2016/11/17 20:09:08]

Correct, clarified the comment a bit.

+    const size_t max_size_;
+
+    std::vector<Report> items_;
+    base::ThreadChecker thread_checker_;
+  };
+
+  // A class to observe events by the service. Used for testing.

[estark, 2016/11/17 14:51:59]

True confessions, I haven't read the tests yet... or the .cc file... so I'm not
sure yet how this will be used. But it seems unfortunate that we need a whole
interface just for testing. And also a little fragile, maybe -- like I can
imagine that the code could change in such a way that reports don't get sent but
the OnSendAttempt event still fires.

Is it possible to test this in any other way? Like a URLRequestInterceptor that
intercepts reports and keeps track of them?

[meacer, 2016/11/17 20:09:08]

Looks like this isn't actually used yet, so I removed it.

I'm most likely going to reintroduce it in the follow up CLs though. I have
browser tests that have an interceptor for follow up CLs, but the interceptor
can't determine the report ids from the URLRequests since we no longer associate
report ids with the URLRequests. For that reason, the event observer will still
be necessary. I hope the next CL will make it clear why we need it, but I agree
it's confusing to have it here.

+  class EventObserver {
+   public:
+    EventObserver() {}
+    virtual ~EventObserver() {}
+
+    // Called when sending of a report is attempted. If attempt was cancelled,
+    // |sent| is false. Otherwise, it's true.

[estark, 2016/11/17 14:51:59]

sent => completed

[meacer, 2016/11/17 20:09:08]

Done.

+    virtual void OnSendAttempt(bool completed) {}
+    // Called when sending a report is completed. If attempt was successful,
+    // |success| is true. Otherwise, it's false.
+    virtual void OnSendComplete(bool success) {}
+    // Called when reporter is created. This can happen when changing
+    // SafeBrowsing or extended reporting preferences.
+    virtual void OnCreated() {}
+    // The service is being reset because SafeBrowsing preferences have changed.
+    virtual void OnReset() {}
+    // The service is being reset because SafeBrowsing preferences have changed.
+    virtual void OnInitialized() {}
+  };
+
+  // Class that handles report uploads and implements the upload retry logic.
+  class Reporter : public base::RefCountedThreadSafe<Reporter> {

[estark, 2016/11/17 14:51:59]

ALERT ALERT REFCOUNTED ALERT

Glanced at the .cc file and it looks like this only needs to be refcounted for
the cross-thread EventObserver interface, is that true?

If indeed the EventObserver interface is necessary (see comment above) and this
needs to be refcounted, might be worth documenting here what should be called on
what thread.

[meacer, 2016/11/17 20:09:08]

Yeah, this was no longer necessary and I removed it in follow up CLs. Sorry that
this is a bit old patchset, removed it from here as well.

+   public:
+    Reporter(
+        std::unique_ptr<certificate_reporting::ErrorReporter> error_reporter_,
+        std::unique_ptr<BoundedReportList> retry_list,
+        base::Clock* clock,
+        base::TimeDelta max_item_age,
+        EventObserver* observer);
+
+    // Sends a report. If the send fails, the report will be added to the retry
+    // list.
+    void Send(const std::string& serialized_report);
+
+    // Sends all pending reports. Skips reports older than max_item_age_. Failed

[estark, 2016/11/17 14:51:59]

Tiny nit: referring to a private member on a public method's documentation seems
weird. Maybe "older than the |max_item_age| provided in the constructor"?

+    // reports will be added to the retry list.
+    void SendPending();

[estark, 2016/11/17 14:51:59]

No code change necessary, but for my own understanding, can you explain a little
how this will be called? Periodically? Will there be any kind of back-off?

[meacer, 2016/11/17 20:09:08]

It will be called periodically by the Metrics service. They do have their own
scheduling and back-off code:
https://cs.chromium.org/chromium/src/components/metrics/metrics_service.cc?sq...

+
+    // Getter and setters for testing:
+    size_t inflight_report_count_for_testing() const;
+    BoundedReportList* GetQueueForTesting() const;
+    void SetEventObserverForTesting(EventObserver* observer);
+
+   private:
+    ~Reporter();
+    friend class base::RefCountedThreadSafe<Reporter>;
+
+    void SendInternal(const Report& report);
+    void ErrorCallback(int report_id, const GURL& url, int error);
+    void SuccessCallback(int report_id);
+    void OnSendComplete(bool success);
+
+    std::unique_ptr<certificate_reporting::ErrorReporter> error_reporter_;
+    std::unique_ptr<BoundedReportList> retry_list_;
+    base::Clock* test_clock_;
+    const base::TimeDelta max_item_age_;
+    EventObserver* event_observer_;
+    int current_report_id_;
+
+    std::map<int, Report> inflight_reports_;
+
+    DISALLOW_IMPLICIT_CONSTRUCTORS(Reporter);
+  };
+};
+
+#endif  // CHROME_BROWSER_SSL_CERTIFICATE_REPORTING_SERVICE_H_