[Payments] Record navigations that close the Payment Request as aborts.

components/payments/core/journey_logger.cc

 // Copyright 2017 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.
 
 #include "components/payments/core/journey_logger.h"
 
 #include <algorithm>
 
 #include "base/metrics/histogram_functions.h"
 #include "base/metrics/histogram_macros.h"
@@ skipping 92 matching lines @@
 void JourneyLogger::SetShowCalled() {
   was_show_called_ = true;
 }
 
 void JourneyLogger::SetEventOccurred(Event event) {
   events_ |= event;
 }
 
 void JourneyLogger::RecordJourneyStatsHistograms(
     CompletionStatus completion_status) {
-  DCHECK(!has_recorded_);
+  if (has_recorded_)

[Mathieu, 2017/05/09 18:12:19]

We should enforce this better, so I agree with the DCHECK

[sebsg, 2017/05/10 14:58:48]

My fear is that we'll be missing some logs. With the if, if we make good tests
we can make sure it's fine. If we don't we may be missing some error logs and
not know it. 

We could add an abort reason instead. So we'll be missing some journey logs but
if we look at the abort reasons and see a lot of Abort_reason_uknown we can
investigate more. 

What do you think?

[Mathieu, 2017/05/10 17:04:20]

In the destructor we could add a DCHECK(has_recorded_), and here have
DCHECK(!has_recorded_). A silent return like this is exactly how bugs are
created, no matter how good the tests... With a silent return, you are actually
counting on an order of operations thing, which you can't guarantee won't change
over time, and as this class is used on other platforms!

[sebsg, 2017/05/10 18:20:26]

Sure, I understand that, it's a good point so I had put back the DCHECK in the
other patch. Good idea for the DCHECK in the destructor.

But I don't think a DCHECK is enough. The sites in the wild will do all kinds of
weird things we don't think about, and if we don't log something in that case,
we'll be totally blind to that. So what do you think about my proposition about
logging and abort reason instead of using the journey logger (mentioned in my
previous comment in this thread)?

+    return;
   has_recorded_ = true;
 
   RecordSectionSpecificStats(completion_status);
 
   // Record the CanMakePayment metrics based on whether the transaction was
   // completed or aborted by the user (UserAborted) or otherwise (OtherAborted).
   RecordCanMakePaymentStats(completion_status);
 
   RecordUrlKeyedMetrics(completion_status);
 }
@@ skipping 104 matching lines @@
   int32_t source_id = ukm_service_->GetNewSourceID();
   ukm_service_->UpdateSourceURL(source_id, url_);
   std::unique_ptr<ukm::UkmEntryBuilder> builder = ukm_service_->GetEntryBuilder(
       source_id, internal::kUKMCheckoutEventsEntryName);
   builder->AddMetric(internal::kUKMCompletionStatusMetricName,
                      completion_status);
   builder->AddMetric(internal::kUKMEventsMetricName, events_);
 }
 
 }  // namespace payments