Move form_parsed_timestamp out of autofill_metrics and put in form_structure.
This will issue of using FormInteractionsUkmLogger in AutofillManager: AutofillManager.Reset() may be called before logging the metrics, which will make FormParsedTimeStamp null when logging the metrics.
This fix will keep FormParsedTimeStamp in form_structure and explicitly pass the FormParsedTimeStamp in the callback.
We have to make full_card_request take form_parsed_timestamp and pass it back to autofill_manager.OnFullCardRequestSucceeded. Since the time stamp may be destructed or overrided by another form interaction.
Bug: 736495
Change-Id: I0068db7bb8c3681937aac9736c1bad0d15b43928
TBR=jochen
Change-Id: I0068db7bb8c3681937aac9736c1bad0d15b43928
Reviewed-on: https://chromium-review.googlesource.com/549026
Commit-Queue: Shanfeng Zhang <[email protected]>
Reviewed-by: Mathieu Perreault <[email protected]>
Reviewed-by: mahmadi <[email protected]>
Reviewed-by: Sebastien Seguin-Gagnon <[email protected]>
Reviewed-by: Roger McFarlane <[email protected]>
Cr-Commit-Position: refs/heads/master@{#488558}diff --git a/components/payments/core/BUILD.gn b/components/payments/core/BUILD.gn
index ed5343e6..f762260 100644
--- a/components/payments/core/BUILD.gn
+++ b/components/payments/core/BUILD.gn
@@ -78,6 +78,7 @@
"//components/autofill/core/browser:test_support",
"//components/pref_registry",
"//components/prefs",
+ "//net:test_support",
]
}
@@ -109,6 +110,7 @@
"//components/strings:components_strings_grit",
"//components/ukm",
"//components/ukm:test_support",
+ "//net:test_support",
"//testing/gmock",
"//testing/gtest",
"//third_party/libaddressinput:test_support",
diff --git a/components/payments/core/DEPS b/components/payments/core/DEPS
index 8325aae1..59baa08 100644
--- a/components/payments/core/DEPS
+++ b/components/payments/core/DEPS
@@ -7,6 +7,7 @@
"+components/prefs",
"+components/pref_registry",
"+components/strings",
+ "+net",
"+services/metrics/public",
"+third_party/libaddressinput",
"+third_party/libphonenumber",
diff --git a/components/payments/core/autofill_payment_instrument.cc b/components/payments/core/autofill_payment_instrument.cc
index 86d98f0277..8673e6ce 100644
--- a/components/payments/core/autofill_payment_instrument.cc
+++ b/components/payments/core/autofill_payment_instrument.cc
@@ -162,6 +162,7 @@
}
void AutofillPaymentInstrument::OnFullCardRequestSucceeded(
+ const autofill::payments::FullCardRequest& /* full_card_request */,
const autofill::CreditCard& card,
const base::string16& cvc) {
DCHECK(delegate_);
diff --git a/components/payments/core/autofill_payment_instrument.h b/components/payments/core/autofill_payment_instrument.h
index 3600918..726ac16 100644
--- a/components/payments/core/autofill_payment_instrument.h
+++ b/components/payments/core/autofill_payment_instrument.h
@@ -54,8 +54,10 @@
const std::vector<std::string>& supported_networks) const override;
// autofill::payments::FullCardRequest::ResultDelegate:
- void OnFullCardRequestSucceeded(const autofill::CreditCard& card,
- const base::string16& cvc) override;
+ void OnFullCardRequestSucceeded(
+ const autofill::payments::FullCardRequest& full_card_request,
+ const autofill::CreditCard& card,
+ const base::string16& cvc) override;
void OnFullCardRequestFailed() override;
// AddressNormalizer::Delegate:
diff --git a/components/payments/core/autofill_payment_instrument_unittest.cc b/components/payments/core/autofill_payment_instrument_unittest.cc
index 117d587..328163b 100644
--- a/components/payments/core/autofill_payment_instrument_unittest.cc
+++ b/components/payments/core/autofill_payment_instrument_unittest.cc
@@ -10,9 +10,15 @@
#include "components/autofill/core/browser/autofill_profile.h"
#include "components/autofill/core/browser/autofill_test_utils.h"
#include "components/autofill/core/browser/credit_card.h"
+#include "components/autofill/core/browser/payments/full_card_request.h"
+#include "components/autofill/core/browser/payments/payments_client.h"
+#include "components/autofill/core/browser/personal_data_manager.h"
+#include "components/autofill/core/browser/test_autofill_client.h"
+#include "components/autofill/core/browser/test_personal_data_manager.h"
#include "components/payments/core/address_normalizer.h"
#include "components/payments/core/test_payment_request_delegate.h"
#include "components/strings/grit/components_strings.h"
+#include "net/url_request/url_request_test_util.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "ui/base/l10n/l10n_util.h"
@@ -78,10 +84,20 @@
AddressNormalizer::Delegate* requester_;
};
-class FakePaymentRequestDelegate : public PaymentRequestDelegate {
+class FakePaymentRequestDelegate
+ : public PaymentRequestDelegate,
+ public autofill::payments::PaymentsClientDelegate {
public:
FakePaymentRequestDelegate()
- : locale_("en-US"), last_committed_url_("https://shop.com") {}
+ : locale_("en-US"),
+ last_committed_url_("https://shop.com"),
+ personal_data_("en-US"),
+ request_context_(new net::TestURLRequestContextGetter(
+ base::ThreadTaskRunnerHandle::Get())),
+ payments_client_(request_context_.get(), this),
+ full_card_request_(&autofill_client_,
+ &payments_client_,
+ &personal_data_) {}
void ShowDialog(PaymentRequest* request) override {}
void CloseDialog() override {}
@@ -120,7 +136,7 @@
void CompleteFullCardRequest() {
full_card_result_delegate_->OnFullCardRequestSucceeded(
- full_card_request_card_, base::ASCIIToUTF16("123"));
+ full_card_request_, full_card_request_card_, base::ASCIIToUTF16("123"));
}
autofill::RegionDataLoader* GetRegionDataLoader() override { return nullptr; }
@@ -131,7 +147,11 @@
std::string locale_;
const GURL last_committed_url_;
FakeAddressNormalizer address_normalizer_;
-
+ autofill::PersonalDataManager personal_data_;
+ scoped_refptr<net::TestURLRequestContextGetter> request_context_;
+ autofill::TestAutofillClient autofill_client_;
+ autofill::payments::PaymentsClient payments_client_;
+ autofill::payments::FullCardRequest full_card_request_;
autofill::CreditCard full_card_request_card_;
base::WeakPtr<autofill::payments::FullCardRequest::ResultDelegate>
full_card_result_delegate_;
@@ -328,7 +348,9 @@
// the billing address has been normalized and the card has been unmasked.
TEST_F(AutofillPaymentInstrumentTest,
InvokePaymentApp_NormalizationBeforeUnmask) {
- TestPaymentRequestDelegate delegate(/*personal_data_manager=*/nullptr);
+ auto personal_data_manager =
+ base::MakeUnique<autofill::TestPersonalDataManager>();
+ TestPaymentRequestDelegate delegate(personal_data_manager.get());
delegate.DelayFullCardRequestCompletion();
delegate.test_address_normalizer()->DelayNormalization();
@@ -357,7 +379,9 @@
// the billing address has been normalized and the card has been unmasked.
TEST_F(AutofillPaymentInstrumentTest,
InvokePaymentApp_UnmaskBeforeNormalization) {
- TestPaymentRequestDelegate delegate(/*personal_data_manager=*/nullptr);
+ auto personal_data_manager =
+ base::MakeUnique<autofill::TestPersonalDataManager>();
+ TestPaymentRequestDelegate delegate(personal_data_manager.get());
delegate.DelayFullCardRequestCompletion();
delegate.test_address_normalizer()->DelayNormalization();
diff --git a/components/payments/core/test_payment_request_delegate.cc b/components/payments/core/test_payment_request_delegate.cc
index 6679419..d379997 100644
--- a/components/payments/core/test_payment_request_delegate.cc
+++ b/components/payments/core/test_payment_request_delegate.cc
@@ -12,7 +12,12 @@
autofill::PersonalDataManager* personal_data_manager)
: personal_data_manager_(personal_data_manager),
locale_("en-US"),
- last_committed_url_("https://shop.com") {}
+ last_committed_url_("https://shop.com"),
+ request_context_(new TestURLRequestContextGetter(loop_.task_runner())),
+ payments_client_(request_context_.get(), &payments_cleint_delegate_),
+ full_card_request_(&autofill_client_,
+ &payments_client_,
+ personal_data_manager) {}
TestPaymentRequestDelegate::~TestPaymentRequestDelegate() {}
@@ -42,7 +47,7 @@
base::WeakPtr<autofill::payments::FullCardRequest::ResultDelegate>
result_delegate) {
if (instantaneous_full_card_request_result_) {
- result_delegate->OnFullCardRequestSucceeded(credit_card,
+ result_delegate->OnFullCardRequestSucceeded(full_card_request_, credit_card,
base::ASCIIToUTF16("123"));
return;
}
@@ -74,7 +79,7 @@
void TestPaymentRequestDelegate::CompleteFullCardRequest() {
DCHECK(instantaneous_full_card_request_result_ == false);
full_card_result_delegate_->OnFullCardRequestSucceeded(
- full_card_request_card_, base::ASCIIToUTF16("123"));
+ full_card_request_, full_card_request_card_, base::ASCIIToUTF16("123"));
}
std::string TestPaymentRequestDelegate::GetAuthenticatedEmail() const {
@@ -85,4 +90,40 @@
return nullptr;
}
+TestPaymentsClientDelegate::TestPaymentsClientDelegate() {}
+
+TestPaymentsClientDelegate::~TestPaymentsClientDelegate() {}
+
+void TestPaymentsClientDelegate::OnDidGetRealPan(
+ autofill::AutofillClient::PaymentsRpcResult result,
+ const std::string& real_pan) {}
+
+IdentityProvider* TestPaymentsClientDelegate::GetIdentityProvider() {
+ return nullptr;
+}
+
+void TestPaymentsClientDelegate::OnDidGetUploadDetails(
+ autofill::AutofillClient::PaymentsRpcResult result,
+ const base::string16& context_token,
+ std::unique_ptr<base::DictionaryValue> legal_message) {}
+
+void TestPaymentsClientDelegate::OnDidUploadCard(
+ autofill::AutofillClient::PaymentsRpcResult result,
+ const std::string& server_id) {}
+
+TestURLRequestContextGetter::TestURLRequestContextGetter(
+ scoped_refptr<base::SingleThreadTaskRunner> task_runner)
+ : task_runner_(task_runner) {}
+
+TestURLRequestContextGetter::~TestURLRequestContextGetter() {}
+
+net::URLRequestContext* TestURLRequestContextGetter::GetURLRequestContext() {
+ return nullptr;
+}
+
+scoped_refptr<base::SingleThreadTaskRunner>
+TestURLRequestContextGetter::GetNetworkTaskRunner() const {
+ return task_runner_;
+}
+
} // namespace payments
diff --git a/components/payments/core/test_payment_request_delegate.h b/components/payments/core/test_payment_request_delegate.h
index e0697a7..7bca98f 100644
--- a/components/payments/core/test_payment_request_delegate.h
+++ b/components/payments/core/test_payment_request_delegate.h
@@ -7,11 +7,48 @@
#include <string>
+#include "base/message_loop/message_loop.h"
+#include "components/autofill/core/browser/payments/full_card_request.h"
+#include "components/autofill/core/browser/payments/payments_client.h"
+#include "components/autofill/core/browser/test_autofill_client.h"
#include "components/payments/core/payment_request_delegate.h"
#include "components/payments/core/test_address_normalizer.h"
+#include "net/url_request/url_request_test_util.h"
namespace payments {
+class TestPaymentsClientDelegate
+ : public autofill::payments::PaymentsClientDelegate {
+ public:
+ TestPaymentsClientDelegate();
+ ~TestPaymentsClientDelegate();
+
+ private:
+ void OnDidGetRealPan(autofill::AutofillClient::PaymentsRpcResult result,
+ const std::string& real_pan) override;
+ IdentityProvider* GetIdentityProvider() override;
+ void OnDidGetUploadDetails(
+ autofill::AutofillClient::PaymentsRpcResult result,
+ const base::string16& context_token,
+ std::unique_ptr<base::DictionaryValue> legal_message) override;
+ void OnDidUploadCard(autofill::AutofillClient::PaymentsRpcResult result,
+ const std::string& server_id) override;
+};
+
+class TestURLRequestContextGetter : public net::URLRequestContextGetter {
+ public:
+ explicit TestURLRequestContextGetter(
+ scoped_refptr<base::SingleThreadTaskRunner> task_runner);
+ net::URLRequestContext* GetURLRequestContext() override;
+ scoped_refptr<base::SingleThreadTaskRunner> GetNetworkTaskRunner()
+ const override;
+
+ private:
+ ~TestURLRequestContextGetter() override;
+
+ scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+};
+
class TestPaymentRequestDelegate : public PaymentRequestDelegate {
public:
TestPaymentRequestDelegate(
@@ -42,10 +79,16 @@
void CompleteFullCardRequest();
private:
+ base::MessageLoop loop_;
+ TestPaymentsClientDelegate payments_cleint_delegate_;
autofill::PersonalDataManager* personal_data_manager_;
std::string locale_;
const GURL last_committed_url_;
TestAddressNormalizer address_normalizer_;
+ scoped_refptr<TestURLRequestContextGetter> request_context_;
+ autofill::TestAutofillClient autofill_client_;
+ autofill::payments::PaymentsClient payments_client_;
+ autofill::payments::FullCardRequest full_card_request_;
bool instantaneous_full_card_request_result_ = true;
autofill::CreditCard full_card_request_card_;
