Add response code to the success callback of ReportSender

net/url_request/report_sender.h

 // 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_URL_REQUEST_REPORT_SENDER_H_
 #define NET_URL_REQUEST_REPORT_SENDER_H_
 
 #include <map>
 #include <memory>
 #include <string>
@@ skipping 12 matching lines @@
 
 // ReportSender asynchronously sends serialized reports to a URI.
 // It takes serialized reports as a sequence of bytes so as to be agnostic to
 // the format of the report being sent (JSON, protobuf, etc.) and the particular
 // data that it contains. Multiple reports can be in-flight at once. This class
 // owns inflight requests and cleans them up when necessary.
 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 };
 
   // Constructs a ReportSender that sends reports with the
   // given |request_context| and includes or excludes cookies based on
   // |cookies_preference|. |request_context| must outlive the
   // ReportSender.
   ReportSender(URLRequestContext* request_context,
                CookiesPreference cookies_preference);
@@ skipping 17 matching lines @@
   CookiesPreference cookies_preference_;
 
   std::map<URLRequest*, std::unique_ptr<URLRequest>> inflight_requests_;
 
   DISALLOW_COPY_AND_ASSIGN(ReportSender);
 };
 
 }  // namespace net
 
 #endif  // NET_URL_REQUEST_REPORT_SENDER_H_