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

components/payments/core/journey_logger.cc

Index: components/payments/core/journey_logger.cc
diff --git a/components/payments/core/journey_logger.cc b/components/payments/core/journey_logger.cc
index 11665b770cd09012252f156e45b1e64bf866dc45..fd28f68fe787863db6850a6473f8efe232707574 100644
--- a/components/payments/core/journey_logger.cc
+++ b/components/payments/core/journey_logger.cc
@@ -110,7 +110,8 @@ void JourneyLogger::SetEventOccurred(Event 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);