Revert "[CVV Storage] Refactor OnCreditCardFetched function"

This reverts commit 0eca378. Reason for revert: suspect of causing breakage for `All/CardMetadataFormEventMetricsTest.LogSelectedMetrics/*` tests on https://ci.chromium.org/ui/p/chromium/builders/ci/linux-ubsan-vptr. First failure at https://ci.chromium.org/ui/p/chromium/builders/ci/linux-ubsan-vptr/25209/overview Original change's description: > [CVV Storage] Refactor OnCreditCardFetched function > > 1. Remove CVC parameter > 2. Set CVC in CreditCard Object before calling OnCreditCardFetched > function. Impacted flows: ServerCard CVC Auth, ServerCard Fido Auth, > VirtualCard Otp Auth. > 3. Retrieve CVC from CreditCard Object instead of CVV parameter > > Without CVC parameter, we are not confused with whether it's server card > cvc or local card cvc. > > 15 results of onCreditCardFethced function usage: > https://source.chromium.org/search? > q=oncreditcardfetched&ss=chromium%2Fchromium%2Fsrc&start=1 > > Will refactor CachedServerCardInfo, > VirtualCardManualFallbackBubbleOptions, > OnDeviceAuthenticationResponseForFilling in follow up CL: > https://bugs.chromium.org/p/chromium/issues/detail?id=1473481 > > Change-Id: Iea9e6c5717200c1049ea7ed163ffabf97c34e856 > Bug: 1450749 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4779745 > Reviewed-by: Vinny Persky <vinnypersky@google.com> > Reviewed-by: Sujie Zhu <sujiezhu@google.com> > Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com> > Reviewed-by: Siyu An <siyua@chromium.org> > Reviewed-by: Christoph Schwering <schwering@google.com> > Commit-Queue: Jiali Huang <jialihuang@google.com> > Cr-Commit-Position: refs/heads/main@{#1187964} Bug: 1450749 Change-Id: I533171d4307ff8c64ab8bb92afd8d963daa7f906 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4812156 Commit-Queue: Chan Li <chanli@chromium.org> Owners-Override: Chan Li <chanli@chromium.org> Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> Auto-Submit: Chan Li <chanli@chromium.org> Cr-Commit-Position: refs/heads/main@{#1188067}
chromium · Aug 24, 2023 · cfc5ed4 · cfc5ed4
1 parent a638b8b
commit cfc5ed4
Show file tree

Hide file tree

Showing 18 changed files with 72 additions and 104 deletions.
diff --git a/chrome/browser/autofill/credit_card_accessory_controller_impl.cc b/chrome/browser/autofill/credit_card_accessory_controller_impl.cc
@@ -328,7 +328,8 @@ void CreditCardAccessoryControllerImpl::OnPersonalDataChanged() {
 
 void CreditCardAccessoryControllerImpl::OnCreditCardFetched(
     CreditCardFetchResult result,
-    const CreditCard* credit_card) {
+    const CreditCard* credit_card,
+    const std::u16string& cvc) {
   if (result != CreditCardFetchResult::kSuccess)
     return;
   content::RenderFrameHost* rfh = GetWebContents().GetFocusedFrame();

diff --git a/chrome/browser/autofill/credit_card_accessory_controller_impl.h b/chrome/browser/autofill/credit_card_accessory_controller_impl.h
@@ -43,7 +43,8 @@ class CreditCardAccessoryControllerImpl
 
   // CreditCardAccessManager::Accessor:
   void OnCreditCardFetched(CreditCardFetchResult result,
-                           const CreditCard* credit_card) override;
+                           const CreditCard* credit_card,
+                           const std::u16string& cvc) override;
 
   static void CreateForWebContentsForTesting(
       content::WebContents* web_contents,

diff --git a/chrome/browser/autofill/credit_card_accessory_controller_impl_unittest.cc b/chrome/browser/autofill/credit_card_accessory_controller_impl_unittest.cc
@@ -69,7 +69,7 @@ class TestAccessManager : public CreditCardAccessManager {
 
   void FetchCreditCard(const CreditCard* card,
                        base::WeakPtr<Accessor> accessor) override {
-    accessor->OnCreditCardFetched(CreditCardFetchResult::kSuccess, card);
+    accessor->OnCreditCardFetched(CreditCardFetchResult::kSuccess, card, u"");
   }
 };
 

diff --git a/chrome/browser/autofill/mock_credit_card_accessory_controller.h b/chrome/browser/autofill/mock_credit_card_accessory_controller.h
@@ -41,7 +41,9 @@ class MockCreditCardAccessoryController
   MOCK_METHOD(void, OnPersonalDataChanged, (), (override));
   MOCK_METHOD(void,
               OnCreditCardFetched,
-              (autofill::CreditCardFetchResult, const autofill::CreditCard*),
+              (autofill::CreditCardFetchResult,
+               const autofill::CreditCard*,
+               const std::u16string&),
               (override));
 };
 

diff --git a/components/autofill/core/browser/autofill_test_utils.cc b/components/autofill/core/browser/autofill_test_utils.cc
@@ -402,12 +402,6 @@ CreditCard GetMaskedServerCard2() {
   return credit_card;
 }
 
-CreditCard GetMaskedServerCardWithCvc() {
-  CreditCard credit_card = GetMaskedServerCard();
-  credit_card.set_cvc(u"123");
-  return credit_card;
-}
-
 CreditCard GetMaskedServerCardWithLegacyId() {
   CreditCard credit_card(CreditCard::RecordType::kMaskedServerCard, "a123");
   test::SetCreditCardInfo(&credit_card, "Bonnie Parker",
@@ -737,13 +731,11 @@ void SetCreditCardInfo(CreditCard* credit_card,
                        const char* card_number,
                        const char* expiration_month,
                        const char* expiration_year,
-                       const std::string& billing_address_id,
-                       const std::u16string& cvc) {
+                       const std::string& billing_address_id) {
   check_and_set(credit_card, CREDIT_CARD_NAME_FULL, name_on_card);
   check_and_set(credit_card, CREDIT_CARD_NUMBER, card_number);
   check_and_set(credit_card, CREDIT_CARD_EXP_MONTH, expiration_month);
   check_and_set(credit_card, CREDIT_CARD_EXP_4_DIGIT_YEAR, expiration_year);
-  credit_card->set_cvc(cvc);
   credit_card->set_billing_address_id(billing_address_id);
 }
 

diff --git a/components/autofill/core/browser/autofill_test_utils.h b/components/autofill/core/browser/autofill_test_utils.h
@@ -167,7 +167,6 @@ CreditCard GetIncompleteCreditCard();
 // Returns a masked server card full of dummy info.
 CreditCard GetMaskedServerCard();
 CreditCard GetMaskedServerCard2();
-CreditCard GetMaskedServerCardWithCvc();
 CreditCard GetMaskedServerCardWithNonLegacyId();
 CreditCard GetMaskedServerCardWithLegacyId();
 CreditCard GetMaskedServerCardVisa();
@@ -279,8 +278,7 @@ void SetCreditCardInfo(CreditCard* credit_card,
                        const char* card_number,
                        const char* expiration_month,
                        const char* expiration_year,
-                       const std::string& billing_address_id,
-                       const std::u16string& cvc = u"");
+                       const std::string& billing_address_id);
 
 // TODO(isherman): We should do this automatically for all tests, not manually
 // on a per-test basis: http://crbug.com/57221

diff --git a/components/autofill/core/browser/browser_autofill_manager.cc b/components/autofill/core/browser/browser_autofill_manager.cc
@@ -1964,10 +1964,9 @@ void BrowserAutofillManager::PropagateAutofillPredictionsDeprecated(
   client().PropagateAutofillPredictionsDeprecated(&driver(), forms);
 }
 
-void BrowserAutofillManager::OnCreditCardFetched(
-    CreditCardFetchResult result,
-    const CreditCard* credit_card) {
-  const std::u16string& cvc = credit_card->cvc();
+void BrowserAutofillManager::OnCreditCardFetched(CreditCardFetchResult result,
+                                                 const CreditCard* credit_card,
+                                                 const std::u16string& cvc) {
   if (result != CreditCardFetchResult::kSuccess) {
     driver().RendererShouldClearPreviewedForm();
     return;
@@ -1994,8 +1993,6 @@ void BrowserAutofillManager::OnCreditCardFetched(
     options.masked_card_number_last_four =
         credit_card_.ObfuscatedNumberWithVisibleLastFourDigits();
     options.virtual_card = *credit_card;
-    // TODO(crbug.com/1473481): Remove CVC from
-    // VirtualCardManualFallbackBubbleOptions.
     options.virtual_card_cvc = cvc;
     options.card_image = GetCardImage(*credit_card);
     client().OnVirtualCardDataAvailable(options);

diff --git a/components/autofill/core/browser/browser_autofill_manager.h b/components/autofill/core/browser/browser_autofill_manager.h
@@ -477,7 +477,8 @@ class BrowserAutofillManager : public AutofillManager,
 
   // CreditCardAccessManager::Accessor
   void OnCreditCardFetched(CreditCardFetchResult result,
-                           const CreditCard* credit_card) override;
+                           const CreditCard* credit_card,
+                           const std::u16string& cvc) override;
 
   // Returns false if Autofill is disabled or if no Autofill data is available.
   bool RefreshDataModels();

diff --git a/components/autofill/core/browser/browser_autofill_manager_test_api.h b/components/autofill/core/browser/browser_autofill_manager_test_api.h
@@ -90,8 +90,9 @@ class BrowserAutofillManagerTestApi : public AutofillManagerTestApi {
   }
 
   void OnCreditCardFetched(CreditCardFetchResult result,
-                           const CreditCard* credit_card = nullptr) {
-    manager_->OnCreditCardFetched(result, credit_card);
+                           const CreditCard* credit_card = nullptr,
+                           const std::u16string& cvc = std::u16string()) {
+    manager_->OnCreditCardFetched(result, credit_card, cvc);
   }
 
   bool WillFillCreditCardNumber(const FormData& form,

diff --git a/components/autofill/core/browser/browser_autofill_manager_unittest.cc b/components/autofill/core/browser/browser_autofill_manager_unittest.cc
@@ -2654,13 +2654,14 @@ TEST_F(BrowserAutofillManagerTest, GetCreditCardSuggestions_NumberMissing) {
 TEST_F(BrowserAutofillManagerTest, OnCreditCardFetched_StoreInstrumentId) {
   FormData form = CreateTestCreditCardFormData(true, false);
   FormsSeen({form});
-  CreditCard credit_card = test::GetMaskedServerCardWithCvc();
+  CreditCard credit_card = test::GetMaskedServerCard();
   browser_autofill_manager_->FillOrPreviewCreditCardForm(
       mojom::AutofillActionPersistence::kFill, form, form.fields[0],
       &credit_card, {.trigger_source = AutofillTriggerSource::kPopup});
 
   test_api(*browser_autofill_manager_)
-      .OnCreditCardFetched(CreditCardFetchResult::kSuccess, &credit_card);
+      .OnCreditCardFetched(CreditCardFetchResult::kSuccess, &credit_card,
+                           /*cvc=*/u"123");
 
   ASSERT_TRUE(form_data_importer().fetched_card_instrument_id().has_value());
   EXPECT_EQ(form_data_importer().fetched_card_instrument_id().value(),

diff --git a/components/autofill/core/browser/metrics/autofill_metrics_test_base.cc b/components/autofill/core/browser/metrics/autofill_metrics_test_base.cc
@@ -191,13 +191,16 @@ void AutofillMetricsBaseTest::OnCreditCardFetchingSuccessful(
                                    ? CreditCard::RecordType::kVirtualCard
                                    : CreditCard::RecordType::kMaskedServerCard);
   credit_card_.SetNumber(real_pan);
+
   test_api(autofill_manager())
-      .OnCreditCardFetched(CreditCardFetchResult::kSuccess, &credit_card_);
+      .OnCreditCardFetched(CreditCardFetchResult::kSuccess, &credit_card_,
+                           u"123");
 }
 
 void AutofillMetricsBaseTest::OnCreditCardFetchingFailed() {
   test_api(autofill_manager())
-      .OnCreditCardFetched(CreditCardFetchResult::kPermanentError, nullptr);
+      .OnCreditCardFetched(CreditCardFetchResult::kPermanentError, nullptr,
+                           u"");
 }
 
 void AutofillMetricsBaseTest::RecreateCreditCards(

diff --git a/components/autofill/core/browser/metrics/autofill_metrics_test_base.h b/components/autofill/core/browser/metrics/autofill_metrics_test_base.h
@@ -231,7 +231,7 @@ class AutofillMetricsBaseTest {
   void CreateTestAutofillProfiles();
 
   base::test::ScopedFeatureList scoped_feature_list_async_parse_form_;
-  CreditCard credit_card_ = test::GetMaskedServerCardWithCvc();
+  CreditCard credit_card_ = test::GetMaskedServerCard();
 };
 
 }  // namespace autofill::autofill_metrics

diff --git a/components/autofill/core/browser/metrics/payments/card_metadata_metrics_unittest.cc b/components/autofill/core/browser/metrics/payments/card_metadata_metrics_unittest.cc
@@ -59,7 +59,7 @@ class CardMetadataFormEventMetricsTest
                            .action = ""});
 
     // Add a masked server card.
-    card_ = test::GetMaskedServerCardWithCvc();
+    card_ = test::GetMaskedServerCard();
     card_.set_guid(kCardGuid);
     if (registered_card_issuer_available()) {
       card_.set_issuer_id(kCapitalOneCardIssuerId);
@@ -259,7 +259,7 @@ TEST_P(CardMetadataFormEventMetricsTest, LogFilledMetrics) {
       Suggestion::BackendId(kCardGuid),
       {.trigger_source = AutofillTriggerSource::kPopup});
   test_api(autofill_manager())
-      .OnCreditCardFetched(CreditCardFetchResult::kSuccess, &card());
+      .OnCreditCardFetched(CreditCardFetchResult::kSuccess, &card(), u"123");
 
   // Verify that:
   // 1. if the card suggestion filled had metadata,
@@ -302,7 +302,7 @@ TEST_P(CardMetadataFormEventMetricsTest, LogFilledMetrics) {
 
   // Fill the suggestion again.
   test_api(autofill_manager())
-      .OnCreditCardFetched(CreditCardFetchResult::kSuccess, &card());
+      .OnCreditCardFetched(CreditCardFetchResult::kSuccess, &card(), u"123");
 
   EXPECT_THAT(
       histogram_tester.GetAllSamples("Autofill.FormEvents.CreditCard"),
@@ -335,7 +335,7 @@ TEST_P(CardMetadataFormEventMetricsTest, LogSubmitMetrics) {
       Suggestion::BackendId(kCardGuid),
       {.trigger_source = AutofillTriggerSource::kPopup});
   test_api(autofill_manager())
-      .OnCreditCardFetched(CreditCardFetchResult::kSuccess, &card());
+      .OnCreditCardFetched(CreditCardFetchResult::kSuccess, &card(), u"123");
   SubmitForm(form());
 
   // Verify that: