Add response code to the success callback of ReportSender

net/url_request/report_sender.h

Index: net/url_request/report_sender.h
diff --git a/net/url_request/report_sender.h b/net/url_request/report_sender.h
index a230f0603b1a9b5a769ade2f65b3e133eb509664..8334366119a61503ee06fd8494462a9825e5b0bd 100644
--- a/net/url_request/report_sender.h
+++ b/net/url_request/report_sender.h
@@ -30,8 +30,8 @@ class NET_EXPORT ReportSender
     : public URLRequest::Delegate,
       public TransportSecurityState::ReportSenderInterface {
  public:
-  using SuccessCallback = base::Callback<void()>;
-  using ErrorCallback = base::Callback<void(const GURL&, int)>;
+  using SuccessCallback = base::Callback<void(int /* response_code */)>;

[eroman, 2017/04/25 21:02:39]

Please document the expectations.

- what is the range for response_code
- what determines success vs failure? (presumably "success" means network level
success, but may include HTTP-level failures as in non-200 response code?)

From an API perspective I would say that this should either:
  (1) Have a combined callback "ReportSentCallback" and require callers to
determine success/failure based on their policies
   (2) Internalize the policy that non-200 http return codes are failures, and
have ReportSender call ErrorCallback (with the http code) in such a case

Given the current code I would say modify as (2), unless you believe there are
any callers which want to use a different notion of success (but given that none
to today, doesn't seem necessary).

[meacer, 2017/04/25 22:19:07]

Yes, that's the current semantics of the success callback.
I'm a bit wary of calling error callback in case of non-200 return codes because
each caller will need to exclude non-200 codes in order to keep their existing
histograms the same.

I can go ahead and fix them as part of this CL, but it seems a bit extra work to
first combine non-200 codes with net errors, then separate them in the error
callback for some of the callers.

estark, nparker: WDYT? You both own histograms that keep count of net errors
(SSL.ExpectCTReportFailure2 and
SafeBrowsing.NotificationImageReporter.NetError).

[eroman, 2017/04/25 22:41:55]

My understanding of this bug is that right now they are essentially "wrong" in
that they undercount any HTTP level failures.

I would say the histograms can safely be updated such that the "bad http status"
code logs as error=0  (net::OK). The two histogram owners can refine from there
if they want, but it doesn't fundamentally change the interpretation of existing
histograms, just adds one more error bucket.

[meacer, 2017/04/25 22:54:54]

I might have misunderstood your original suggestion, so let me know if this is
what you meant:

void ReportSender::OnResponseStarted(URLRequest* request, int net_error) {
  ...
  if (net_error != OK) {
    callback_info->error_callback().Run(request->url(), net_error);
    return;
  }
  if (request->GetResponseCode() != HTTP_OK) {
    callback_info->error_callback().Run(request->url(),
request->GetResponseCode());
    return;
  }
  callback_info->success_callback().Run();
}


The caller can then decide whether to ignore non-200 response codes (net_errors
are always negative so there is no confusion).

Or do you mean to simply invoke error callback with net::OK?

void ReportSender::OnResponseStarted(URLRequest* request, int net_error) {
  ...
  if (net_error != OK) {
    callback_info->error_callback().Run(request->url(), net_error);
    return;
  }
  if (request->GetResponseCode() != HTTP_OK) {
    callback_info->error_callback().Run(request->url(), OK);
    return;
  }
  callback_info->success_callback().Run();
}

I'm going to need the first version in CertificateReportingService as I'll be
recording the actual HTTP response 
code in the histogram.

+  using ErrorCallback = base::Callback<void(const GURL&, int /* net_error */)>;
 
   // Represents whether or not to send cookies along with reports.
   enum CookiesPreference { SEND_COOKIES, DO_NOT_SEND_COOKIES };