Add response code to the success callback of ReportSender

Add response code to the success callback of ReportSender

We are suspecting that some error reports don't get sent properly
even when the success callback runs. For that reason, we'll check
the response code and classify the upload as failure if the response
code isn't as expected.

BUG=682933
Review-Url: https://codereview.chromium.org/2648713002
Cr-Commit-Position: refs/heads/master@{#467772}
Committed: https://chromium.googlesource.com/chromium/src/+/5d4dc5a9a8c92aef2ebda576be00795677b8d625

[meacer, 2017-01-20 02:08:57]

The CQ bit was checked by meacer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 02:09:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[meacer, 2017-01-20 02:10:37]

meacer@chromium.org changed reviewers:
+ nparker@chromium.org

[meacer, 2017-01-20 02:10:38]

PTAL? I'm not fully convinced that we need response_code for error callbacks -
there will always be a net_error instead.

[commit-bot: I haz the power, 2017-01-20 03:07:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-20 03:07:29]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[meacer, 2017-01-20 19:26:08]

On 2017/01/20 02:10:38, Mustafa Emre Acer wrote:
> PTAL? I'm not fully convinced that we need response_code for error callbacks -
> there will always be a net_error instead.

Looks like this won't work since the response code is not available yet in
OnResponseStarted.

[Nathan Parker, 2017-01-23 23:42:37]

The OnResponseStarted() interface says:
    // At this point,
    // meta data about the response is available, including for example HTTP
    // response headers if this is a request for a HTTP resource

So it seems like it should have HTTP error codes then, ya?  

You're not convinced... do you think the error code captures the case where the
network connection succeeded but the HTTP request failed? That's the part I'm
concerned about. There could be an intervening proxy, or (more seriously) the
backend could be rejecting some requests (> some size limit, or some that are
malformed) or all requests (because it's misconfigured).

[meacer, 2017-02-02 02:07:32]

Okay, I'm now enlightened and convinced that we should do this. Will take
another shot at it.

[Nathan Parker, 2017-02-07 01:33:01]

lgtm

Thanks Mustafa

https://codereview.chromium.org/2648713002/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/certificate_reporting_service.cc (right):

https://codereview.chromium.org/2648713002/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/certificate_reporting_service.cc:149:
RecordUMAOnFailure(error);
How about also logging the response_code to UMA now?  Or do you want to do that
in a different CL?

https://codereview.chromium.org/2648713002/diff/1/net/url_request/url_request...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/2648713002/diff/1/net/url_request/url_request...
net/url_request/url_request_unittest.cc:718: void Send(const GURL& report_uri,
holey smokes this file is rediculously large.

[Nathan Parker, 2017-04-12 19:41:16]

lgtm

Is this CL going to land, or is it abandoned?

[meacer, 2017-04-12 20:07:31]

On 2017/04/12 19:41:16, Nathan Parker wrote:
> lgtm
> 
> Is this CL going to land, or is it abandoned?

Not abandoned, but I need to fix the builds before landing it.

[meacer, 2017-04-24 22:17:21]

The CQ bit was checked by meacer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-24 22:18:05]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-24 23:05:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-24 23:05:35]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[meacer, 2017-04-25 00:47:44]

The CQ bit was checked by meacer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 00:48:19]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-25 02:19:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-25 02:19:31]

Dry run: This issue passed the CQ dry run.

[meacer, 2017-04-25 18:09:54]

Description was changed from

==========
Add response code to error and success callbacks of ReportSender

BUG=682933
==========

to

==========
Add response code to the success callback of ReportSender

We are suspecting that some error reports don't get sent properly
even when the success callback runs. For that reason, we'll check
the response code and classify the upload as failure if the response
code isn't as expected.

BUG=682933
==========

[meacer, 2017-04-25 18:10:00]

meacer@chromium.org changed reviewers:
+ eroman@chromium.org, estark@chromium.org

[meacer, 2017-04-25 18:10:01]

eroman, estark: Can you PTAL?

eroman: net/
estark: components/certificate_reporting/

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

https://codereview.chromium.org/2648713002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/certificate_reporting_service.h (right):

https://codereview.chromium.org/2648713002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/certificate_reporting_service.h:130: void
SuccessCallback(int report_id, int response_code);
can you document what this means, or give it a more specific name like
http_response_code ?

https://codereview.chromium.org/2648713002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/notification_image_reporter.cc (right):

https://codereview.chromium.org/2648713002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/notification_image_reporter.cc:53: void
LogReportSuccessCallback(int response_code) {
Is it intentional that |response_code| is unused? If you are trying to track
down failures you may want to histogram |response_code / 100|, or a sparse
histogram of the exact HTTP code.

https://codereview.chromium.org/2648713002/diff/40001/components/certificate_...
File components/certificate_reporting/error_reporter_unittest.cc (right):

https://codereview.chromium.org/2648713002/diff/40001/components/certificate_...
components/certificate_reporting/error_reporter_unittest.cc:130: kDummyReport,
base::Callback<void(int)>(),
Are there any tests for the HTTP code?

https://codereview.chromium.org/2648713002/diff/40001/net/url_request/report_...
File net/url_request/report_sender.h (right):

https://codereview.chromium.org/2648713002/diff/40001/net/url_request/report_...
net/url_request/report_sender.h:33: using SuccessCallback =
base::Callback<void(int /* response_code */)>;
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]

Adding two comments before I address the rest.

https://codereview.chromium.org/2648713002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/notification_image_reporter.cc (right):

https://codereview.chromium.org/2648713002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/notification_image_reporter.cc:53: void
LogReportSuccessCallback(int response_code) {
On 2017/04/25 21:02:39, eroman wrote:
> Is it intentional that |response_code| is unused? If you are trying to track
> down failures you may want to histogram |response_code / 100|, or a sparse
> histogram of the exact HTTP code.

I intentionally left it unused here, but I think SafeBrowsing wants to use it
later. nparker, is that correct?

I'm going to use the response code in CertificateReportingService and record the
exact code in a sparse histogram in a separate CL.

https://codereview.chromium.org/2648713002/diff/40001/net/url_request/report_...
File net/url_request/report_sender.h (right):

https://codereview.chromium.org/2648713002/diff/40001/net/url_request/report_...
net/url_request/report_sender.h:33: using SuccessCallback =
base::Callback<void(int /* response_code */)>;
On 2017/04/25 21:02:39, eroman wrote:
> 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?)

Yes, that's the current semantics of the success callback.

> 
> 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).

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]

https://codereview.chromium.org/2648713002/diff/40001/net/url_request/report_...
File net/url_request/report_sender.h (right):

https://codereview.chromium.org/2648713002/diff/40001/net/url_request/report_...
net/url_request/report_sender.h:33: using SuccessCallback =
base::Callback<void(int /* response_code */)>;
On 2017/04/25 22:19:07, Mustafa Emre Acer wrote:
> On 2017/04/25 21:02:39, eroman wrote:
> > 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?)
> 
> Yes, that's the current semantics of the success callback.
> 
> > 
> > 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).
> 
> 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).

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]

https://codereview.chromium.org/2648713002/diff/40001/net/url_request/report_...
File net/url_request/report_sender.h (right):

https://codereview.chromium.org/2648713002/diff/40001/net/url_request/report_...
net/url_request/report_sender.h:33: using SuccessCallback =
base::Callback<void(int /* response_code */)>;
On 2017/04/25 22:41:55, eroman wrote:
> On 2017/04/25 22:19:07, Mustafa Emre Acer wrote:
> > On 2017/04/25 21:02:39, eroman wrote:
> > > 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?)
> > 
> > Yes, that's the current semantics of the success callback.
> > 
> > > 
> > > 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).
> > 
> > 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).
> 
> 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.

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.

[eroman, 2017-04-25 23:02:50]

My proposal was to change the signature for ErrorCallback by adding a third
parameter:

  (url, net_error, http_response_code)

So existing histogramming code could disregard the new parameter, and would only
have the difference that now it may additionally log a net_error of 0.

[meacer, 2017-04-25 23:06:49]

On 2017/04/25 23:02:50, eroman wrote:
> My proposal was to change the signature for ErrorCallback by adding a third
> parameter:
> 
>   (url, net_error, http_response_code)
> 
> So existing histogramming code could disregard the new parameter, and would
only
> have the difference that now it may additionally log a net_error of 0.

I see, yes that makes sense.

[meacer, 2017-04-26 20:13:36]

The CQ bit was checked by meacer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 20:14:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-26 22:24:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-26 22:24:49]

Dry run: This issue passed the CQ dry run.

[meacer, 2017-04-26 22:27:06]

Addressed comments. Can you please take another look?

https://codereview.chromium.org/2648713002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/certificate_reporting_service.h (right):

https://codereview.chromium.org/2648713002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/certificate_reporting_service.h:130: void
SuccessCallback(int report_id, int response_code);
On 2017/04/25 21:02:39, eroman wrote:
> can you document what this means, or give it a more specific name like
> http_response_code ?

Renamed to http_response_code and documented the callbacks in
transport_security_state.h.

https://codereview.chromium.org/2648713002/diff/40001/components/certificate_...
File components/certificate_reporting/error_reporter_unittest.cc (right):

https://codereview.chromium.org/2648713002/diff/40001/components/certificate_...
components/certificate_reporting/error_reporter_unittest.cc:130: kDummyReport,
base::Callback<void(int)>(),
On 2017/04/25 21:02:39, eroman wrote:
> Are there any tests for the HTTP code?

Added one to report_sender_unittest.cc

[eroman, 2017-04-26 22:52:28]

lgtm!

https://codereview.chromium.org/2648713002/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/certificate_reporting_service.cc (right):

https://codereview.chromium.org/2648713002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/certificate_reporting_service.cc:158: int error,
ditto

https://codereview.chromium.org/2648713002/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/certificate_reporting_service.h (right):

https://codereview.chromium.org/2648713002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/certificate_reporting_service.h:131: int error,
I suggested updating this to "net_error" to make it clear what the domain is.

https://codereview.chromium.org/2648713002/diff/60001/net/http/transport_secu...
File net/http/transport_security_state.h (right):

https://codereview.chromium.org/2648713002/diff/60001/net/http/transport_secu...
net/http/transport_security_state.h:291: // error callback's
|http_response_code| is zero.
I suggest making it default to -1 instead (as that is what URLRequest will
return when it doesn't have an HTTP status)

https://codereview.chromium.org/2648713002/diff/60001/net/url_request/report_...
File net/url_request/report_sender.cc (right):

https://codereview.chromium.org/2648713002/diff/60001/net/url_request/report_...
net/url_request/report_sender.cc:95:
callback_info->error_callback().Run(request->url(), net_error, 0);
I suggest using -1 instead of 0 (as that is what GetResponseCode() returns in
cases where it doesn't have one).

[estark, 2017-04-27 00:35:58]

https://codereview.chromium.org/2648713002/diff/60001/components/certificate_...
File components/certificate_reporting/error_reporter.h (right):

https://codereview.chromium.org/2648713002/diff/60001/components/certificate_...
components/certificate_reporting/error_reporter.h:63: virtual void
SendExtendedReportingReport(
nit: can you document what the |error_callback| arguments are?

https://codereview.chromium.org/2648713002/diff/60001/components/certificate_...
File components/certificate_reporting/error_reporter_unittest.cc (right):

https://codereview.chromium.org/2648713002/diff/60001/components/certificate_...
components/certificate_reporting/error_reporter_unittest.cc:42:
EXPECT_NE(net::HTTP_OK, http_response_code);
Is there a test that exercises this?

https://codereview.chromium.org/2648713002/diff/60001/components/certificate_...
components/certificate_reporting/error_reporter_unittest.cc:197: // Tests that
an UMA histogram is recorded if a report is successfully sent.
oof, copy/paste fail, thanks for fixing

[meacer, 2017-04-27 00:56:42]

https://codereview.chromium.org/2648713002/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/certificate_reporting_service.cc (right):

https://codereview.chromium.org/2648713002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/certificate_reporting_service.cc:158: int error,
On 2017/04/26 22:52:28, eroman wrote:
> ditto

Done.

https://codereview.chromium.org/2648713002/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/certificate_reporting_service.h (right):

https://codereview.chromium.org/2648713002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/certificate_reporting_service.h:131: int error,
On 2017/04/26 22:52:28, eroman wrote:
> I suggested updating this to "net_error" to make it clear what the domain is.

Done.

https://codereview.chromium.org/2648713002/diff/60001/components/certificate_...
File components/certificate_reporting/error_reporter.h (right):

https://codereview.chromium.org/2648713002/diff/60001/components/certificate_...
components/certificate_reporting/error_reporter.h:63: virtual void
SendExtendedReportingReport(
On 2017/04/27 00:35:57, estark wrote:
> nit: can you document what the |error_callback| arguments are?

Done.

https://codereview.chromium.org/2648713002/diff/60001/components/certificate_...
File components/certificate_reporting/error_reporter_unittest.cc (right):

https://codereview.chromium.org/2648713002/diff/60001/components/certificate_...
components/certificate_reporting/error_reporter_unittest.cc:42:
EXPECT_NE(net::HTTP_OK, http_response_code);
On 2017/04/27 00:35:57, estark wrote:
> Is there a test that exercises this?

I reverted this change as this test will always get net error because of
MockCertificateReportSender. I think the non net-error case is sufficiently
tested in net/url_request/report_sender_unittest.cc and doing the same here
would duplicate the code there. WDYT?

https://codereview.chromium.org/2648713002/diff/60001/net/http/transport_secu...
File net/http/transport_security_state.h (right):

https://codereview.chromium.org/2648713002/diff/60001/net/http/transport_secu...
net/http/transport_security_state.h:291: // error callback's
|http_response_code| is zero.
On 2017/04/26 22:52:28, eroman wrote:
> I suggest making it default to -1 instead (as that is what URLRequest will
> return when it doesn't have an HTTP status)

Done.

https://codereview.chromium.org/2648713002/diff/60001/net/url_request/report_...
File net/url_request/report_sender.cc (right):

https://codereview.chromium.org/2648713002/diff/60001/net/url_request/report_...
net/url_request/report_sender.cc:95:
callback_info->error_callback().Run(request->url(), net_error, 0);
On 2017/04/26 22:52:28, eroman wrote:
> I suggest using -1 instead of 0 (as that is what GetResponseCode() returns in
> cases where it doesn't have one).

Done.

[estark, 2017-04-27 01:06:50]

components/certificate_reporting lgtm

[meacer, 2017-04-27 17:33:13]

The CQ bit was checked by meacer@chromium.org

[meacer, 2017-04-27 17:33:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
nparker@chromium.org, eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2648713002/#ps80001
(title: "eroman and estark comments")

[commit-bot: I haz the power, 2017-04-27 17:33:35]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-27 18:00:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-27 18:00:36]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[meacer, 2017-04-27 19:01:20]

The CQ bit was checked by meacer@chromium.org

[commit-bot: I haz the power, 2017-04-27 19:02:28]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[robbybrewer49, 2017-04-27 19:08:42]

robbybrewer49@gmail.com changed reviewers:
+ robbybrewer49@gmail.com

[commit-bot: I haz the power, 2017-04-27 20:37:51]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493319680557860,
"parent_rev": "d807be2872bb8eb583989d5a578f8a50d2631c68", "commit_rev":
"5d4dc5a9a8c92aef2ebda576be00795677b8d625"}

[commit-bot: I haz the power, 2017-04-27 20:37:59]

Description was changed from

==========
Add response code to the success callback of ReportSender

We are suspecting that some error reports don't get sent properly
even when the success callback runs. For that reason, we'll check
the response code and classify the upload as failure if the response
code isn't as expected.

BUG=682933
==========

to

==========
Add response code to the success callback of ReportSender

We are suspecting that some error reports don't get sent properly
even when the success callback runs. For that reason, we'll check
the response code and classify the upload as failure if the response
code isn't as expected.

BUG=682933

Review-Url: https://codereview.chromium.org/2648713002
Cr-Commit-Position: refs/heads/master@{#467772}
Committed:
https://chromium.googlesource.com/chromium/src/+/5d4dc5a9a8c92aef2ebda576be00...
==========

[commit-bot: I haz the power, 2017-04-27 20:38:00]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/5d4dc5a9a8c92aef2ebda576be00...