From 57cc98b317b1efee3c0fa580d3f7fdb961f73373 Mon Sep 17 00:00:00 2001 From: Siyu An Date: Thu, 29 Jul 2021 22:36:16 +0000 Subject: [PATCH] [VCN] Add image usage in VCN flow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Made some explorations and found the image fetcher works pretty well without a disk cache in the AutofillTable. listed three options https://docs.google.com/document/d/1tSz7puztdgn-AJExKg0RRu4yNfelqw0-024toE9L3RY/edit?resourcekey=0-kpj35lGmSEWOImPeDFjbMw Now we are actually preferring option 2, which gets rid of the Autofill Table storage completely. This would save much integration work and will avoid unnecessary decoding of the image. Previously submitted CL crrev.com/c/3048555 was reverted so figured this would be a good time to redo the work. Bug: 1196021 Change-Id: I7d1da29b39c43eee2b270295fdbda0ffb0950d67 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3054839 Reviewed-by: Jared Saul Reviewed-by: Siddharth Shah Reviewed-by: Dominic Battré Commit-Queue: Siyu An Cr-Commit-Position: refs/heads/master@{#906880} --- .../browser/autofill_suggestion_generator.cc | 14 ++-- .../core/browser/browser_autofill_manager.cc | 7 +- .../data_model/credit_card_art_image.cc | 9 +-- .../data_model/credit_card_art_image.h | 23 ++---- .../core/browser/personal_data_manager.cc | 73 ++++++++++++++++- .../core/browser/personal_data_manager.h | 30 ++++++- .../browser/personal_data_manager_unittest.cc | 65 +++++++++++++++ .../core/browser/ui/autofill_image_fetcher.cc | 35 ++++---- .../core/browser/ui/autofill_image_fetcher.h | 34 ++++---- .../ui/autofill_image_fetcher_unittest.cc | 81 +++++++++---------- .../core/browser/webdata/autofill_table.cc | 55 ------------- .../core/browser/webdata/autofill_table.h | 8 +- .../webdata/autofill_table_unittest.cc | 31 ------- .../webdata/autofill_wallet_sync_bridge.cc | 11 +-- .../webdata/autofill_wallet_sync_bridge.h | 2 + .../core/common/autofill_payments_features.cc | 7 ++ .../core/common/autofill_payments_features.h | 2 + 17 files changed, 277 insertions(+), 210 deletions(-) diff --git a/components/autofill/core/browser/autofill_suggestion_generator.cc b/components/autofill/core/browser/autofill_suggestion_generator.cc index 723b9d21ec3c9..e256070898776 100644 --- a/components/autofill/core/browser/autofill_suggestion_generator.cc +++ b/components/autofill/core/browser/autofill_suggestion_generator.cc @@ -176,16 +176,17 @@ Suggestion AutofillSuggestionGenerator::CreateCreditCardSuggestion( suggestion.match = prefix_matched_suggestion ? Suggestion::PREFIX_MATCH : Suggestion::SUBSTRING_MATCH; - std::string server_id_for_virtual_card_option; + GURL card_art_url_for_virtual_card_option; if (virtual_card_option && credit_card.record_type() == CreditCard::MASKED_SERVER_CARD) { - server_id_for_virtual_card_option = credit_card.server_id(); + card_art_url_for_virtual_card_option = credit_card.card_art_url(); } else if (virtual_card_option && credit_card.record_type() == CreditCard::LOCAL_CARD) { const CreditCard* server_duplicate_card = GetServerCardForLocalCard(&credit_card); DCHECK(server_duplicate_card); - server_id_for_virtual_card_option = server_duplicate_card->server_id(); + card_art_url_for_virtual_card_option = + server_duplicate_card->card_art_url(); backend_id = server_duplicate_card->guid(); } suggestion.backend_id = backend_id; @@ -251,9 +252,12 @@ Suggestion AutofillSuggestionGenerator::CreateCreditCardSuggestion( feature_engagement::kIPHKeyboardAccessoryPaymentVirtualCardFeature.name; #endif // OS_ANDROID - // TODO(crbug.com/1196021): Populate custom_icon with card art if available - // (use server_id_for_virtual_card_option). suggestion.frontend_id = POPUP_ITEM_ID_VIRTUAL_CREDIT_CARD_ENTRY; + + gfx::Image* image = personal_data_->GetCreditCardArtImageForUrl( + card_art_url_for_virtual_card_option); + if (image) + suggestion.custom_icon = *image; } return suggestion; diff --git a/components/autofill/core/browser/browser_autofill_manager.cc b/components/autofill/core/browser/browser_autofill_manager.cc index 47e4bffa7d84f..7c34877d159f1 100644 --- a/components/autofill/core/browser/browser_autofill_manager.cc +++ b/components/autofill/core/browser/browser_autofill_manager.cc @@ -1484,9 +1484,10 @@ void BrowserAutofillManager::OnCreditCardFetched(CreditCardFetchResult result, // If synced down card is a virtual card, let the client know so that it can // show the UI to help user to manually fill the form, if needed. if (credit_card->record_type() == CreditCard::VIRTUAL_CARD) { - // TODO(crbug.com/1196021): Pass in real card image. - client()->OnVirtualCardDataAvailable(credit_card, cvc, - /*card_image=*/gfx::Image()); + gfx::Image* card_art_image = personal_data_->GetCreditCardArtImageForUrl( + credit_card->card_art_url()); + client()->OnVirtualCardDataAvailable( + credit_card, cvc, card_art_image ? *card_art_image : gfx::Image()); } FillCreditCardForm(credit_card_query_id_, credit_card_form_, diff --git a/components/autofill/core/browser/data_model/credit_card_art_image.cc b/components/autofill/core/browser/data_model/credit_card_art_image.cc index e63ac3a8bab06..97725d8ee6ed0 100644 --- a/components/autofill/core/browser/data_model/credit_card_art_image.cc +++ b/components/autofill/core/browser/data_model/credit_card_art_image.cc @@ -8,13 +8,8 @@ namespace autofill { CreditCardArtImage::CreditCardArtImage() = default; -CreditCardArtImage::CreditCardArtImage(const std::string& id, - int64_t instrument_id, - std::vector card_art_image) { - this->id = id; - this->instrument_id = instrument_id; - this->card_art_image = std::move(card_art_image); -} +CreditCardArtImage::CreditCardArtImage(const CreditCardArtImage& other) = + default; CreditCardArtImage::~CreditCardArtImage() = default; diff --git a/components/autofill/core/browser/data_model/credit_card_art_image.h b/components/autofill/core/browser/data_model/credit_card_art_image.h index 4ae25d201544c..7f5fc5950b5fc 100644 --- a/components/autofill/core/browser/data_model/credit_card_art_image.h +++ b/components/autofill/core/browser/data_model/credit_card_art_image.h @@ -5,30 +5,23 @@ #ifndef COMPONENTS_AUTOFILL_CORE_BROWSER_DATA_MODEL_CREDIT_CARD_ART_IMAGE_H_ #define COMPONENTS_AUTOFILL_CORE_BROWSER_DATA_MODEL_CREDIT_CARD_ART_IMAGE_H_ -#include -#include +#include "ui/gfx/image/image.h" +#include "url/gurl.h" namespace autofill { -// Represents an rich card art image for the server credit card. +// Represents an rich card art image for the card art url. struct CreditCardArtImage { public: CreditCardArtImage(); - CreditCardArtImage(const std::string& id, - int64_t instrument_id, - std::vector card_art_image); + CreditCardArtImage(const CreditCardArtImage& other); ~CreditCardArtImage(); - CreditCardArtImage(const CreditCardArtImage&) = delete; - CreditCardArtImage& operator=(const CreditCardArtImage&) = delete; - // The server id for the related credit card. - std::string id; + // The url to fetch the card art image. + GURL card_art_url; - // The instrument id for the related credit card. - int64_t instrument_id; - - // The customized card art image. Stored as an raw PNG-encoded data. - std::vector card_art_image; + // The customized card art image. + gfx::Image card_art_image; }; } // namespace autofill diff --git a/components/autofill/core/browser/personal_data_manager.cc b/components/autofill/core/browser/personal_data_manager.cc index 8bdb9da1de47b..9a63b5e2fe61c 100644 --- a/components/autofill/core/browser/personal_data_manager.cc +++ b/components/autofill/core/browser/personal_data_manager.cc @@ -34,6 +34,7 @@ #include "components/autofill/core/browser/autofill_metrics.h" #include "components/autofill/core/browser/autofill_profile_save_strike_database.h" #include "components/autofill/core/browser/data_model/autofill_profile_comparator.h" +#include "components/autofill/core/browser/data_model/credit_card_art_image.h" #include "components/autofill/core/browser/data_model/phone_number.h" #include "components/autofill/core/browser/form_structure.h" #include "components/autofill/core/browser/geo/address_i18n.h" @@ -152,6 +153,9 @@ class PersonalDatabaseHelper explicit PersonalDatabaseHelper(PersonalDataManager* personal_data_manager) : personal_data_manager_(personal_data_manager) {} + PersonalDatabaseHelper(const PersonalDatabaseHelper&) = delete; + PersonalDatabaseHelper& operator=(const PersonalDatabaseHelper&) = delete; + void Init(scoped_refptr profile_database, scoped_refptr account_database) { profile_database_ = profile_database; @@ -252,8 +256,6 @@ class PersonalDatabaseHelper scoped_refptr server_database_; PersonalDataManager* personal_data_manager_; - - DISALLOW_COPY_AND_ASSIGN(PersonalDatabaseHelper); }; PersonalDataManager::PersonalDataManager( @@ -478,6 +480,7 @@ void PersonalDataManager::OnWebDataServiceRequestDone( ReceiveLoadedDbValues(h, result.get(), &pending_server_creditcards_query_, &server_credit_cards_); + OnServerCreditCardsRefreshed(); } break; case AUTOFILL_CLOUDTOKEN_RESULT: @@ -961,6 +964,7 @@ void PersonalDataManager::ClearAllServerData() { payments_customer_data_.reset(); server_credit_card_cloud_token_data_.clear(); autofill_offer_data_.clear(); + credit_card_art_images_.clear(); } void PersonalDataManager::ClearAllLocalData() { @@ -1227,6 +1231,27 @@ std::vector PersonalDataManager::GetAutofillOffers() const { return result; } +gfx::Image* PersonalDataManager::GetCreditCardArtImageForUrl( + const GURL& card_art_url) const { + if (!IsAutofillWalletImportEnabled()) + return nullptr; + + if (!card_art_url.is_valid()) + return nullptr; + + auto images_iterator = credit_card_art_images_.find(card_art_url); + + // Found an image and return it. + if (images_iterator != credit_card_art_images_.end()) { + gfx::Image* image = images_iterator->second.get(); + if (!image->IsEmpty()) + return image; + } + + FetchImagesForUrls({card_art_url}); + return nullptr; +} + void PersonalDataManager::Refresh() { LoadProfiles(); LoadCreditCards(); @@ -1504,6 +1529,16 @@ const ProfileValidityMap& PersonalDataManager::GetProfileValidityByGUID( return empty_validity_map; } +void PersonalDataManager::FetchImagesForUrls( + const std::vector& updated_urls) const { + if (!image_fetcher_) + return; + + image_fetcher_->FetchImagesForUrls( + updated_urls, base::BindOnce(&PersonalDataManager::OnCardArtImagesFetched, + weak_factory_.GetWeakPtr())); +} + const std::string& PersonalDataManager::GetDefaultCountryCodeForNewAddress() const { if (default_country_code_.empty()) @@ -2099,6 +2134,16 @@ void PersonalDataManager::OnAutofillProfileChanged( OnProfileChangeDone(guid); } +void PersonalDataManager::OnCardArtImagesFetched( + std::vector> art_images) { + for (auto& art_image : art_images) { + if (!art_image->card_art_image.IsEmpty()) { + credit_card_art_images_[art_image->card_art_url] = + std::make_unique(art_image->card_art_image); + } + } +} + void PersonalDataManager::LogServerCardLinkClicked() const { AutofillMetrics::LogServerCardLinkClicked(GetSyncSigninState()); } @@ -2375,4 +2420,28 @@ scoped_refptr PersonalDataManager::GetLocalDatabase() { return database_helper_->GetLocalDatabase(); } +void PersonalDataManager::OnServerCreditCardsRefreshed() { + ProcessVirtualCardMetadataChanges(); +} + +void PersonalDataManager::ProcessVirtualCardMetadataChanges() { + std::vector updated_urls; + for (auto& card : server_credit_cards_) { + // If this card is not enrolled for virtual cards or the url is not valid, + // continue. + if (card->virtual_card_enrollment_state() != CreditCard::ENROLLED || + !card->card_art_url().is_valid()) { + continue; + } + + // Otherwise try to find the old entry with the same url. + auto it = credit_card_art_images_.find(card->card_art_url()); + // No existing entry found. + if (it == credit_card_art_images_.end()) + updated_urls.emplace_back(card->card_art_url()); + } + if (!updated_urls.empty()) + FetchImagesForUrls(updated_urls); +} + } // namespace autofill diff --git a/components/autofill/core/browser/personal_data_manager.h b/components/autofill/core/browser/personal_data_manager.h index 5c55e2ebde406..458e76309d80c 100644 --- a/components/autofill/core/browser/personal_data_manager.h +++ b/components/autofill/core/browser/personal_data_manager.h @@ -15,7 +15,6 @@ #include #include "base/gtest_prod_util.h" -#include "base/macros.h" #include "base/observer_list.h" #include "base/scoped_observation.h" #include "build/build_config.h" @@ -55,6 +54,7 @@ class RemoveAutofillTester; namespace autofill { class AutofillImageFetcher; class AutofillInteractiveTest; +struct CreditCardArtImage; class PersonalDataManagerObserver; class PersonalDataManagerFactory; class PersonalDatabaseHelper; @@ -85,6 +85,8 @@ class PersonalDataManager : public KeyedService, explicit PersonalDataManager(const std::string& app_locale); PersonalDataManager(const std::string& app_locale, const std::string& country_code); + PersonalDataManager(const PersonalDataManager&) = delete; + PersonalDataManager& operator=(const PersonalDataManager&) = delete; ~PersonalDataManager() override; // Kicks off asynchronous loading of profiles and credit cards. @@ -286,6 +288,10 @@ class PersonalDataManager : public KeyedService, // Returns autofill offer data, including card-linked and promo code offers. virtual std::vector GetAutofillOffers() const; + // Returns the customized credit card art image for the |card_art_url|. + virtual gfx::Image* GetCreditCardArtImageForUrl( + const GURL& card_art_url) const; + // Updates the validity states of |profiles| according to server validity map. void UpdateProfilesServerValidityMapsIfNeeded( const std::vector& profiles); @@ -471,6 +477,8 @@ class PersonalDataManager : public KeyedService, FRIEND_TEST_ALL_PREFIXES(PersonalDataManagerTest, AddCreditCard_CrazyCharacters); FRIEND_TEST_ALL_PREFIXES(PersonalDataManagerTest, AddCreditCard_Invalid); + FRIEND_TEST_ALL_PREFIXES(PersonalDataManagerTest, + AddAndGetCreditCardArtImage); FRIEND_TEST_ALL_PREFIXES(PersonalDataManagerTest, DedupeProfiles_ProfilesToDelete); FRIEND_TEST_ALL_PREFIXES(PersonalDataManagerTest, @@ -629,6 +637,9 @@ class PersonalDataManager : public KeyedService, // Get the profiles fields validity map by |guid|. const ProfileValidityMap& GetProfileValidityByGUID(const std::string& guid); + // Asks AutofillImageFetcher to fetch images. + virtual void FetchImagesForUrls(const std::vector& updated_urls) const; + // Decides which database type to use for server and local cards. std::unique_ptr database_helper_; @@ -659,6 +670,9 @@ class PersonalDataManager : public KeyedService, // Offer data for user's credit cards. std::vector> autofill_offer_data_; + // The customized card art images for the URL. + std::map> credit_card_art_images_; + // When the manager makes a request from WebDataServiceBase, the database // is queried on another sequence, we record the query handle until we // get called back. We store handles for both profile and credit card queries @@ -747,6 +761,11 @@ class PersonalDataManager : public KeyedService, // Triggered when a profile is added/updated/removed on db. void OnAutofillProfileChanged(const AutofillProfileDeepChange& change); + // Triggered when all the card art image fetches have been completed, + // regardless of whether all of them succeeded. + void OnCardArtImagesFetched( + std::vector> art_images); + // Look at the next profile change for profile with guid = |guid|, and handle // it. void HandleNextProfileChange(const std::string& guid); @@ -774,6 +793,13 @@ class PersonalDataManager : public KeyedService, // Returns the database that is used for storing local data. scoped_refptr GetLocalDatabase(); + // Invoked when server credit card cache is refreshed. + void OnServerCreditCardsRefreshed(); + + // Checks whether any virtual card metadata for server cards is new and makes + // corresponding changes. + void ProcessVirtualCardMetadataChanges(); + // Stores the |app_locale| supplied on construction. const std::string app_locale_; @@ -860,8 +886,6 @@ class PersonalDataManager : public KeyedService, history_service_observation_{this}; base::WeakPtrFactory weak_factory_{this}; - - DISALLOW_COPY_AND_ASSIGN(PersonalDataManager); }; } // namespace autofill diff --git a/components/autofill/core/browser/personal_data_manager_unittest.cc b/components/autofill/core/browser/personal_data_manager_unittest.cc index 2ad385ee5f34d..5f91b12fecbb8 100644 --- a/components/autofill/core/browser/personal_data_manager_unittest.cc +++ b/components/autofill/core/browser/personal_data_manager_unittest.cc @@ -39,6 +39,7 @@ #include "components/autofill/core/browser/data_model/autofill_profile.h" #include "components/autofill/core/browser/data_model/autofill_profile_comparator.h" #include "components/autofill/core/browser/data_model/autofill_structured_address_utils.h" +#include "components/autofill/core/browser/data_model/credit_card_art_image.h" #include "components/autofill/core/browser/field_types.h" #include "components/autofill/core/browser/form_structure.h" #include "components/autofill/core/browser/personal_data_manager_observer.h" @@ -70,6 +71,7 @@ #include "services/network/test/test_url_loader_factory.h" #include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" +#include "ui/gfx/image/image_unittest_util.h" namespace autofill { @@ -113,6 +115,11 @@ class PersonalDataManagerMock : public PersonalDataManager { void OnValidatedPDM(const AutofillProfile* profile) { PersonalDataManager::OnValidated(profile); } + + MOCK_METHOD(void, + FetchImagesForUrls, + ((const std::vector& updated_urls)), + (const override)); }; template @@ -711,6 +718,19 @@ class PersonalDataManagerMockTest : public PersonalDataManagerTestBase, prefs::kAutofillLastVersionValidated); } + // Verifies the credit card art image fetching should begin. + void WaitForFetchImagesForUrls(const std::vector& updated_urls) { + base::RunLoop run_loop; + EXPECT_CALL(personal_data_observer_, OnPersonalDataFinishedProfileTasks()) + .Times(testing::AnyNumber()); + EXPECT_CALL(personal_data_observer_, OnPersonalDataChanged()) + .Times(testing::AnyNumber()); + EXPECT_CALL(*personal_data_, FetchImagesForUrls(updated_urls)) + .Times(1) + .WillOnce(QuitMessageLoop(&run_loop)); + run_loop.Run(); + } + std::unique_ptr personal_data_; }; @@ -1275,6 +1295,51 @@ TEST_F(PersonalDataManagerTest, AddCreditCard_Invalid) { ASSERT_EQ(card, *personal_data_->GetCreditCards()[0]); } +#if !defined(OS_IOS) +TEST_F(PersonalDataManagerTest, AddAndGetCreditCardArtImage) { + gfx::Image expected_image = gfx::test::CreateImage(32, 20); + std::unique_ptr credit_card_art_image = + std::make_unique(); + credit_card_art_image->card_art_url = GURL("https://www.example.com"); + credit_card_art_image->card_art_image = expected_image; + std::vector> images; + images.emplace_back(std::move(credit_card_art_image)); + + personal_data_->OnCardArtImagesFetched(std::move(images)); + + gfx::Image* actual_image = personal_data_->GetCreditCardArtImageForUrl( + GURL("https://www.example.com")); + ASSERT_TRUE(actual_image); + EXPECT_TRUE(gfx::test::AreImagesEqual(expected_image, *actual_image)); +} + +TEST_F(PersonalDataManagerMockTest, ProcessVirtualCardMetadataChanges) { + CreditCard card = test::GetFullServerCard(); + card.set_virtual_card_enrollment_state( + CreditCard::VirtualCardEnrollmentState::UNENROLLED); + card.set_server_id("card_server_id"); + personal_data_->AddFullServerCreditCard(card); + WaitForOnPersonalDataChanged(); + + card.set_virtual_card_enrollment_state( + CreditCard::VirtualCardEnrollmentState::ENROLLED); + card.set_server_id("card_server_id"); + card.set_card_art_url(GURL("https://www.example.com/card1")); + std::vector updated_urls; + updated_urls.emplace_back(GURL("https://www.example.com/card1")); + + personal_data_->AddFullServerCreditCard(card); + WaitForFetchImagesForUrls(updated_urls); + + card.set_card_art_url(GURL("https://www.example.com/card2")); + updated_urls.clear(); + updated_urls.emplace_back(GURL("https://www.example.com/card2")); + + personal_data_->AddFullServerCreditCard(card); + WaitForFetchImagesForUrls(updated_urls); +} +#endif + TEST_F(PersonalDataManagerTest, UpdateUnverifiedProfilesAndCreditCards) { // Start with unverified data. AutofillProfile profile(base::GenerateGUID(), "https://www.example.com/"); diff --git a/components/autofill/core/browser/ui/autofill_image_fetcher.cc b/components/autofill/core/browser/ui/autofill_image_fetcher.cc index e3629128fcb61..3310fa1f7a823 100644 --- a/components/autofill/core/browser/ui/autofill_image_fetcher.cc +++ b/components/autofill/core/browser/ui/autofill_image_fetcher.cc @@ -5,6 +5,8 @@ #include "components/autofill/core/browser/ui/autofill_image_fetcher.h" #include "components/autofill/core/browser/autofill_metrics.h" +#include "components/autofill/core/browser/data_model/credit_card_art_image.h" +#include "components/autofill/core/common/autofill_payments_features.h" #include "components/image_fetcher/core/image_decoder.h" #include "components/image_fetcher/core/image_fetcher_impl.h" #include "components/image_fetcher/core/request_metadata.h" @@ -52,14 +54,18 @@ ImageFetchOperation::ImageFetchOperation(size_t image_count, : pending_request_count_(image_count), all_fetches_complete_callback_(std::move(callback)) {} -void ImageFetchOperation::ImageFetched(const std::string& card_server_id, +void ImageFetchOperation::ImageFetched(const GURL& card_art_url, const gfx::Image& card_art_image) { AutofillMetrics::LogImageFetchResult(/*succeeded=*/!card_art_image.IsEmpty()); - pending_request_count_--; - if (!card_art_image.IsEmpty()) - fetched_card_art_images_[card_server_id] = card_art_image; + if (!card_art_image.IsEmpty()) { + auto credit_card_art_image = std::make_unique(); + credit_card_art_image->card_art_url = card_art_url; + credit_card_art_image->card_art_image = card_art_image; + + fetched_card_art_images_.emplace_back(std::move(credit_card_art_image)); + } if (pending_request_count_ == 0U) { std::move(all_fetches_complete_callback_) @@ -81,7 +87,7 @@ AutofillImageFetcher::AutofillImageFetcher( AutofillImageFetcher::~AutofillImageFetcher() = default; void AutofillImageFetcher::FetchImagesForUrls( - const std::map& card_server_ids_and_art_urls, + const std::vector& card_art_urls, CardArtImagesFetchedCallback callback) { if (!image_fetcher_) { std::move(callback).Run({}); @@ -89,40 +95,39 @@ void AutofillImageFetcher::FetchImagesForUrls( } auto image_fetcher_operation = base::MakeRefCounted( - card_server_ids_and_art_urls.size(), std::move(callback)); + card_art_urls.size(), std::move(callback)); - for (const auto& card_server_id_and_art_url : card_server_ids_and_art_urls) { - FetchImageForUrl(image_fetcher_operation, card_server_id_and_art_url.first, - card_server_id_and_art_url.second); - } + for (const auto& card_art_url : card_art_urls) + FetchImageForUrl(image_fetcher_operation, card_art_url); } void AutofillImageFetcher::FetchImageForUrl( const scoped_refptr& operation, - const std::string& card_server_id, const GURL& card_art_url) { if (!card_art_url.is_valid()) { - OnCardArtImageFetched(operation, card_server_id, gfx::Image(), + OnCardArtImageFetched(operation, card_art_url, gfx::Image(), image_fetcher::RequestMetadata()); return; } image_fetcher::ImageFetcherParams params(kCardArtImageTrafficAnnotation, kUmaClientName); + params.set_hold_for_expiration_interval(base::TimeDelta::FromMinutes( + features::kAutofillImageFetcherDiskCacheExpirationInMinutes.Get())); image_fetcher_->FetchImage( card_art_url, base::BindOnce(&AutofillImageFetcher::OnCardArtImageFetched, operation, - card_server_id), + card_art_url), std::move(params)); } // static void AutofillImageFetcher::OnCardArtImageFetched( const scoped_refptr& operation, - const std::string& card_server_id, + const GURL& card_art_url, const gfx::Image& card_art_image, const image_fetcher::RequestMetadata& metadata) { - operation->ImageFetched(card_server_id, card_art_image); + operation->ImageFetched(card_art_url, card_art_image); } } // namespace autofill diff --git a/components/autofill/core/browser/ui/autofill_image_fetcher.h b/components/autofill/core/browser/ui/autofill_image_fetcher.h index 4f269fb1480b2..863c8da994bc0 100644 --- a/components/autofill/core/browser/ui/autofill_image_fetcher.h +++ b/components/autofill/core/browser/ui/autofill_image_fetcher.h @@ -8,6 +8,7 @@ #include #include #include +#include #include "base/callback.h" #include "base/memory/scoped_refptr.h" @@ -31,8 +32,10 @@ class GURL; namespace autofill { +struct CreditCardArtImage; + using CardArtImagesFetchedCallback = base::OnceCallback& card_art_images)>; + std::vector> card_art_images)>; // The image fetching operation instance. It tracks the state of the paired // request. Will be created when the AutofillImageFetcher receives a request to @@ -45,8 +48,7 @@ class ImageFetchOperation : public base::RefCounted { ImageFetchOperation& operator=(const ImageFetchOperation&) = delete; // Invoked when an image fetch is complete and data is returned. - void ImageFetched(const std::string& card_server_id, - const gfx::Image& card_art_image); + void ImageFetched(const GURL& card_art_url, const gfx::Image& card_art_image); private: friend class base::RefCounted; @@ -56,8 +58,8 @@ class ImageFetchOperation : public base::RefCounted { // The number of images that should be fetched before completion. size_t pending_request_count_ = 0; - // The mapping of each credit card's server id to its card art image. - std::map fetched_card_art_images_; + // The vector of the fetched CreditCardArtImages. + std::vector> fetched_card_art_images_; // Callback function to be invoked when fetching is finished. CardArtImagesFetchedCallback all_fetches_complete_callback_; @@ -74,21 +76,19 @@ class AutofillImageFetcher : public KeyedService { AutofillImageFetcher& operator=(const AutofillImageFetcher&) = delete; // Once invoked, the |image_fetcher_| will start fetching images based on the - // urls. |card_server_ids_and_art_urls| is a map with credit cards' server id - // as the key and its card art image url as the value. |callback| will be - // invoked when all the requests have been completed. The callback will - // receive a map of card server IDs to images, for (only) those cards for - // which the AutofillImageFetcher could successfully fetch the image. - void FetchImagesForUrls( - const std::map& card_server_ids_and_art_urls, - CardArtImagesFetchedCallback callback); + // urls. |card_art_urls| is a vector with credit cards' card art image url. + // |callback| will be invoked when all the requests have been completed. The + // callback will receive a vector of CreditCardArtImage, for (only) those + // cards for which the AutofillImageFetcher could successfully fetch the + // image. + void FetchImagesForUrls(const std::vector& card_art_urls, + CardArtImagesFetchedCallback callback); protected: - // Helper function to fetch art image for card with |card_server_id| given the - // |card_art_url|, for the specific |operation| instance. + // Helper function to fetch art image for card given the |card_art_url|, for + // the specific |operation| instance. virtual void FetchImageForUrl( const scoped_refptr& operation, - const std::string& card_server_id, const GURL& card_art_url); // The image fetcher implementation. @@ -100,7 +100,7 @@ class AutofillImageFetcher : public KeyedService { // Called when an image is fetched for the |operation| instance. static void OnCardArtImageFetched( const scoped_refptr& operation, - const std::string& card_server_id, + const GURL& card_art_url, const gfx::Image& card_art_image, const image_fetcher::RequestMetadata& metadata); }; diff --git a/components/autofill/core/browser/ui/autofill_image_fetcher_unittest.cc b/components/autofill/core/browser/ui/autofill_image_fetcher_unittest.cc index 40d7b3149b036..ba338e9d556c4 100644 --- a/components/autofill/core/browser/ui/autofill_image_fetcher_unittest.cc +++ b/components/autofill/core/browser/ui/autofill_image_fetcher_unittest.cc @@ -8,6 +8,7 @@ #include "base/test/bind.h" #include "base/test/metrics/histogram_tester.h" #include "components/autofill/core/browser/data_model/credit_card.h" +#include "components/autofill/core/browser/data_model/credit_card_art_image.h" #include "components/image_fetcher/core/image_decoder.h" #include "components/image_fetcher/core/mock_image_fetcher.h" #include "components/image_fetcher/core/request_metadata.h" @@ -34,10 +35,8 @@ class TestAutofillImageFetcher : public AutofillImageFetcher { ~TestAutofillImageFetcher() override = default; void FetchImageForUrl(const scoped_refptr& operation, - const std::string& card_server_id, const GURL& card_art_url) override { - AutofillImageFetcher::FetchImageForUrl(operation, card_server_id, - card_art_url); + AutofillImageFetcher::FetchImageForUrl(operation, card_art_url); current_operation_ = operation; } @@ -58,15 +57,14 @@ class AutofillImageFetcherTest : public testing::Test { std::make_unique(std::move(image_fetcher)); } - void SimulateOnImageFetched(const std::string& server_id, - const gfx::Image& image) { + void SimulateOnImageFetched(const GURL& url, const gfx::Image& image) { TestAutofillImageFetcher::OnCardArtImageFetched( - autofill_image_fetcher()->current_operation(), server_id, image, + autofill_image_fetcher()->current_operation(), url, image, image_fetcher::RequestMetadata()); } - void ValidateResult(std::map received_images, - std::map expected_images) { + void ValidateResult(std::map received_images, + std::map expected_images) { ASSERT_EQ(expected_images.size(), received_images.size()); for (const auto& expected_pair : expected_images) { const gfx::Image& expected_image = expected_pair.second; @@ -97,30 +95,32 @@ TEST_F(AutofillImageFetcherTest, FetchImage_Success) { gfx::Image fake_image2 = ui::ResourceBundle::GetSharedInstance().GetNativeImageNamed( IDR_DEFAULT_FAVICON_DARK); - std::map expected_images = { - {"server_id1", fake_image1}, {"server_id2", fake_image2}}; + GURL fake_url1 = GURL("http://www.example.com/fake_image1"); + GURL fake_url2 = GURL("http://www.example.com/fake_image2"); + + std::map expected_images = {{fake_url1, fake_image1}, + {fake_url2, fake_image2}}; // Expect callback to be called with some received images. - std::map received_images; + std::map received_images; auto callback = base::BindLambdaForTesting( - [&](const std::map& card_art_image_map) { - received_images = card_art_image_map; + [&](std::vector> card_art_images) { + for (auto& entry : card_art_images) + received_images[entry->card_art_url] = entry->card_art_image; }); base::HistogramTester histogram_tester; // Expect to be called twice. EXPECT_CALL(*mock_image_fetcher(), FetchImageAndData_(_, _, _, _)).Times(2); - std::map url_map = { - {"server_id1", GURL("http://www.example.com/fake_image1")}, - {"server_id2", GURL("http://www.example.com/fake_image2")}}; - autofill_image_fetcher()->FetchImagesForUrls(url_map, callback); + std::vector urls = {fake_url1, fake_url2}; + autofill_image_fetcher()->FetchImagesForUrls(urls, callback); // Simulate successful image fetching (for image with URL) -> expect the // callback to be called. - SimulateOnImageFetched("server_id1", fake_image1); - SimulateOnImageFetched("server_id2", fake_image2); + SimulateOnImageFetched(fake_url1, fake_image1); + SimulateOnImageFetched(fake_url2, fake_image2); - ValidateResult(received_images, expected_images); + ValidateResult(std::move(received_images), expected_images); histogram_tester.ExpectBucketCount("Autofill.ImageFetcher.Result", true, 2); } @@ -128,55 +128,54 @@ TEST_F(AutofillImageFetcherTest, FetchImage_InvalidUrlFailure) { gfx::Image fake_image1 = ui::ResourceBundle::GetSharedInstance().GetNativeImageNamed( IDR_DEFAULT_FAVICON); - std::map expected_images = { - {"server_id1", fake_image1}}; + GURL fake_url1 = GURL("http://www.example.com/fake_image1"); + std::map expected_images = {{fake_url1, fake_image1}}; - // Expect callback to be called with expected images. - std::map received_images; + std::map received_images; auto callback = base::BindLambdaForTesting( - [&](const std::map& card_art_image_map) { - received_images = card_art_image_map; + [&](std::vector> card_art_images) { + for (auto& entry : card_art_images) + received_images[entry->card_art_url] = entry->card_art_image; }); base::HistogramTester histogram_tester; // Expect to be called once with one invalid url. EXPECT_CALL(*mock_image_fetcher(), FetchImageAndData_(_, _, _, _)).Times(1); - std::map url_map = { - {"server_id1", GURL("http://www.example.com/fake_image1")}, - {"server_id2", GURL("")}}; - autofill_image_fetcher()->FetchImagesForUrls(url_map, callback); + std::vector urls = {fake_url1, GURL("")}; + autofill_image_fetcher()->FetchImagesForUrls(urls, callback); // Simulate successful image fetching (for image with URL) -> expect the // callback to be called. - SimulateOnImageFetched("server_id1", fake_image1); + SimulateOnImageFetched(fake_url1, fake_image1); - ValidateResult(received_images, expected_images); + ValidateResult(std::move(received_images), expected_images); histogram_tester.ExpectBucketCount("Autofill.ImageFetcher.Result", true, 1); histogram_tester.ExpectBucketCount("Autofill.ImageFetcher.Result", false, 1); } TEST_F(AutofillImageFetcherTest, FetchImage_ServerFailure) { - std::map expected_images = {}; + GURL fake_url1 = GURL("http://www.example.com/fake_image1"); + std::map expected_images; // Expect callback to be called with some received images. - std::map received_images; + std::map received_images; auto callback = base::BindLambdaForTesting( - [&](const std::map& card_art_image_map) { - received_images = card_art_image_map; + [&](std::vector> card_art_images) { + for (auto& entry : card_art_images) + received_images[entry->card_art_url] = entry->card_art_image; }); base::HistogramTester histogram_tester; // Expect to be called once. EXPECT_CALL(*mock_image_fetcher(), FetchImageAndData_(_, _, _, _)).Times(1); - std::map url_map = { - {"server_id1", GURL("http://www.example.com/fake_image1")}}; - autofill_image_fetcher()->FetchImagesForUrls(url_map, callback); + std::vector urls = {fake_url1}; + autofill_image_fetcher()->FetchImagesForUrls(urls, callback); // Simulate failed image fetching (for image with URL) -> expect the // callback to be called. - SimulateOnImageFetched("server_id1", gfx::Image()); + SimulateOnImageFetched(fake_url1, gfx::Image()); - ValidateResult(received_images, expected_images); + ValidateResult(std::move(received_images), expected_images); histogram_tester.ExpectBucketCount("Autofill.ImageFetcher.Result", false, 1); } diff --git a/components/autofill/core/browser/webdata/autofill_table.cc b/components/autofill/core/browser/webdata/autofill_table.cc index 26042dda7274e..b93f0bc13ab57 100644 --- a/components/autofill/core/browser/webdata/autofill_table.cc +++ b/components/autofill/core/browser/webdata/autofill_table.cc @@ -29,7 +29,6 @@ #include "components/autofill/core/browser/data_model/autofill_offer_data.h" #include "components/autofill/core/browser/data_model/autofill_profile.h" #include "components/autofill/core/browser/data_model/credit_card.h" -#include "components/autofill/core/browser/data_model/credit_card_art_image.h" #include "components/autofill/core/browser/data_model/credit_card_cloud_token_data.h" #include "components/autofill/core/browser/geo/autofill_country.h" #include "components/autofill/core/browser/payments/payments_customer_data.h" @@ -2024,60 +2023,6 @@ bool AutofillTable::GetCreditCardCloudTokenData( return s.Succeeded(); } -bool AutofillTable::AddCreditCardArtImage( - const CreditCardArtImage& credit_card_art_image) { - sql::Transaction transaction(db_); - if (!transaction.Begin()) - return false; - - sql::Statement s(db_->GetUniqueStatement( - "INSERT INTO credit_card_art_images(id, instrument_id, card_art_image)" - "VALUES (?,?,?)")); - s.BindString(0, credit_card_art_image.id); - s.BindInt64(1, credit_card_art_image.instrument_id); - s.BindBlob(2, credit_card_art_image.card_art_image); - s.Run(); - - return transaction.Commit(); -} - -bool AutofillTable::GetCreditCardArtImages( - std::vector>* credit_card_art_images) { - credit_card_art_images->clear(); - - sql::Statement s( - db_->GetUniqueStatement("SELECT " - "id, " // 0 - "instrument_id, " // 1 - "card_art_image " // 2 - "FROM credit_card_art_images")); - - while (s.Step()) { - std::vector card_art_image; - if (s.ColumnBlobAsVector(2, &card_art_image)) { - std::unique_ptr data = - std::make_unique( - s.ColumnString(0), s.ColumnInt64(1), std::move(card_art_image)); - credit_card_art_images->push_back(std::move(data)); - } - } - - return s.Succeeded(); -} - -bool AutofillTable::ClearCreditCardArtImage(const std::string& id) { - sql::Transaction transaction(db_); - if (!transaction.Begin()) - return false; - - sql::Statement s(db_->GetUniqueStatement( - "DELETE FROM credit_card_art_images WHERE id = ?")); - s.BindString(0, id); - s.Run(); - - return transaction.Commit(); -} - void AutofillTable::SetPaymentsCustomerData( const PaymentsCustomerData* customer_data) { sql::Transaction transaction(db_); diff --git a/components/autofill/core/browser/webdata/autofill_table.h b/components/autofill/core/browser/webdata/autofill_table.h index 223991cdd7c4d..79c7e8777c71e 100644 --- a/components/autofill/core/browser/webdata/autofill_table.h +++ b/components/autofill/core/browser/webdata/autofill_table.h @@ -35,7 +35,6 @@ class AutofillProfile; class AutofillTableEncryptor; class AutofillTableTest; class CreditCard; -struct CreditCardArtImage; struct CreditCardCloudTokenData; struct FormFieldData; struct PaymentsCustomerData; @@ -429,6 +428,7 @@ struct PaymentsCustomerData; // offer_id in the offer_data table. // merchant_domain List of full origins for merchant websites on which // this offer would apply. +// TODO(crbug.com/1196021): Remove unused table. // credit_card_art_images // Contains the card art image for the server credit card. // @@ -596,12 +596,6 @@ class AutofillTable : public WebDatabaseTable, std::vector>* credit_card_cloud_token_data); - // Setters and getters related to the credit card art images. - bool AddCreditCardArtImage(const CreditCardArtImage& credit_card_art_image); - bool GetCreditCardArtImages( - std::vector>* credit_card_art_images); - bool ClearCreditCardArtImage(const std::string& id); - // Setters and getters related to the Google Payments customer data. // Passing null to the setter will clear the data. void SetPaymentsCustomerData(const PaymentsCustomerData* customer_data); diff --git a/components/autofill/core/browser/webdata/autofill_table_unittest.cc b/components/autofill/core/browser/webdata/autofill_table_unittest.cc index c077283c742c8..97cb1c6dddaf2 100644 --- a/components/autofill/core/browser/webdata/autofill_table_unittest.cc +++ b/components/autofill/core/browser/webdata/autofill_table_unittest.cc @@ -27,7 +27,6 @@ #include "components/autofill/core/browser/data_model/autofill_offer_data.h" #include "components/autofill/core/browser/data_model/autofill_profile.h" #include "components/autofill/core/browser/data_model/credit_card.h" -#include "components/autofill/core/browser/data_model/credit_card_art_image.h" #include "components/autofill/core/browser/data_model/credit_card_cloud_token_data.h" #include "components/autofill/core/browser/payments/payments_customer_data.h" #include "components/autofill/core/browser/webdata/autofill_change.h" @@ -3768,34 +3767,4 @@ TEST_F(AutofillTableTest, SetAndGetCreditCardOfferData) { } } -TEST_F(AutofillTableTest, SetAndGetAndClearCreditCardArtImage) { - CreditCardArtImage image1("image1", 1, {UINT8_MAX}); - table_->AddCreditCardArtImage(image1); - CreditCardArtImage image2("image2", 2, {UINT8_MAX}); - table_->AddCreditCardArtImage(image2); - - std::vector> output; - EXPECT_TRUE(table_->GetCreditCardArtImages(&output)); - EXPECT_EQ(2U, output.size()); - EXPECT_EQ("image1", output[0]->id); - EXPECT_EQ(1, output[0]->instrument_id); - EXPECT_EQ(UINT8_MAX, output[0]->card_art_image[0]); - EXPECT_EQ("image2", output[1]->id); - EXPECT_EQ(2, output[1]->instrument_id); - EXPECT_EQ(UINT8_MAX, output[1]->card_art_image[0]); - - EXPECT_TRUE(table_->ClearCreditCardArtImage("image1")); - output.clear(); - EXPECT_TRUE(table_->GetCreditCardArtImages(&output)); - EXPECT_EQ(1U, output.size()); - EXPECT_EQ("image2", output[0]->id); - EXPECT_EQ(2, output[0]->instrument_id); - EXPECT_EQ(UINT8_MAX, output[0]->card_art_image[0]); - - EXPECT_TRUE(table_->ClearAllServerData()); - output.clear(); - EXPECT_TRUE(table_->GetCreditCardArtImages(&output)); - EXPECT_EQ(0U, output.size()); -} - } // namespace autofill diff --git a/components/autofill/core/browser/webdata/autofill_wallet_sync_bridge.cc b/components/autofill/core/browser/webdata/autofill_wallet_sync_bridge.cc index e8598a8172b1b..f55343bdc7abb 100644 --- a/components/autofill/core/browser/webdata/autofill_wallet_sync_bridge.cc +++ b/components/autofill/core/browser/webdata/autofill_wallet_sync_bridge.cc @@ -547,11 +547,9 @@ void AutofillWalletSyncBridge::ProcessVirtualCardMetadataChanges( const std::vector& new_data) { std::vector updated_server_ids; for (const CreditCard& new_card : new_data) { - // If this new card is not enrolled for virtual cards, clear the table entry - // for it if any and continue. + // If this new card is not enrolled for virtual cards, continue. if (new_card.virtual_card_enrollment_state() != CreditCard::VirtualCardEnrollmentState::ENROLLED) { - GetAutofillTable()->ClearCreditCardArtImage(new_card.server_id()); continue; } @@ -567,20 +565,15 @@ void AutofillWalletSyncBridge::ProcessVirtualCardMetadataChanges( updated_server_ids.push_back(new_card.server_id()); // log the newly-synced card. AutofillMetrics::LogVirtualCardMetadataSynced(/*existing_card=*/false); - // The actual card image will be added later once it has actually been - // fetched. continue; } // If the virtual card metadata has changed from the old card to the new - // cards, change the table data and log the updated sync. + // cards, log the updated sync. if ((*old_data_iterator)->virtual_card_enrollment_state() != new_card.virtual_card_enrollment_state() || (*old_data_iterator)->card_art_url() != new_card.card_art_url()) { updated_server_ids.push_back(new_card.server_id()); - // Any existing card art image is not valid anymore. A new image will be - // added later. - GetAutofillTable()->ClearCreditCardArtImage(new_card.server_id()); AutofillMetrics::LogVirtualCardMetadataSynced(/*existing_card=*/true); } } diff --git a/components/autofill/core/browser/webdata/autofill_wallet_sync_bridge.h b/components/autofill/core/browser/webdata/autofill_wallet_sync_bridge.h index b6329ecc58f89..80d7c9c1e2fe8 100644 --- a/components/autofill/core/browser/webdata/autofill_wallet_sync_bridge.h +++ b/components/autofill/core/browser/webdata/autofill_wallet_sync_bridge.h @@ -134,6 +134,8 @@ class AutofillWalletSyncBridge : public base::SupportsUserData::Data, // processor so that it can start tracking changes. void LoadMetadata(); + // TODO(crbug/com/1196021): Clean up duplicate functions and use it for + // logging only. // Checks whether any virtual card metadata for new_data is new and make // corresponding changes. void ProcessVirtualCardMetadataChanges( diff --git a/components/autofill/core/common/autofill_payments_features.cc b/components/autofill/core/common/autofill_payments_features.cc index c8bf1cbff64cc..c4ade7d697656 100644 --- a/components/autofill/core/common/autofill_payments_features.cc +++ b/components/autofill/core/common/autofill_payments_features.cc @@ -112,6 +112,13 @@ const base::Feature kAutofillEnableVirtualCard{ const base::Feature kAutofillFixOfferInIncognito{ "AutofillFixOfferInIncognito", base::FEATURE_DISABLED_BY_DEFAULT}; +// The merchant bound virtual card feature introduces new customized card art +// images. This parameter defines the expiration of the fetched image in the +// disk cache of the image fetcher. +const base::FeatureParam kAutofillImageFetcherDiskCacheExpirationInMinutes{ + &kAutofillEnableMerchantBoundVirtualCards, + "autofill_image_fetcher_disk_cache_expiration_in_minutes", 10}; + // When enabled, Autofill will attempt to find merchant promo/coupon/gift code // fields when parsing forms. const base::Feature kAutofillParseMerchantPromoCodeFields{ diff --git a/components/autofill/core/common/autofill_payments_features.h b/components/autofill/core/common/autofill_payments_features.h index 8a4cce194ba03..4d3ace874b9b2 100644 --- a/components/autofill/core/common/autofill_payments_features.h +++ b/components/autofill/core/common/autofill_payments_features.h @@ -32,6 +32,8 @@ extern const base::Feature kAutofillEnableStickyManualFallbackForCards; extern const base::Feature kAutofillEnableToolbarStatusChip; extern const base::Feature kAutofillEnableVirtualCard; extern const base::Feature kAutofillFixOfferInIncognito; +extern const base::FeatureParam + kAutofillImageFetcherDiskCacheExpirationInMinutes; extern const base::Feature kAutofillParseMerchantPromoCodeFields; extern const base::Feature kAutofillSaveCardDismissOnNavigation; extern const base::Feature kAutofillSaveCardInfobarEditSupport;