Skip to content

GitLab

T
tangled
Commit ab050037 authored by Vidhan Jain's avatar Vidhan Jain Committed by Commit Bot
Browse files
Options

Revert "[Autofill][States] Implemented StateNameMapUpdater" 

This reverts commit bd6beb55.

Reason for revert: Open comments on CL

Original change's description:
> [Autofill][States] Implemented StateNameMapUpdater
>
> StateNameMapUpdater is a helper class for StateNameMapUpdater
> that reads the data from the state data files and loads them to the
> map.
>
> Bug: 1111960
> Change-Id: I8add7215a1ec450e0a31ef999b2adfa6dade138b
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2471337
> Reviewed-by: Matthias Körber <koerber@google.com>
> Reviewed-by: Dominic Battré <battre@chromium.org>
> Commit-Queue: Vidhan Jain <vidhanj@google.com>
> Cr-Commit-Position: refs/heads/master@{#822187}

TBR=battre@chromium.org,koerber@google.com,schwering@google.com,vidhanj@google.com

Change-Id: I5a47671505371abfccfd5ba5ebbe43002e2db4f3
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1111960
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2507751Reviewed-by: default avatarMatthias Körber <koerber@google.com>
Commit-Queue: Vidhan Jain <vidhanj@google.com>
Cr-Commit-Position: refs/heads/master@{#822257}
parent 34befa23
Hide whitespace changes
Inline Side-by-side
Showing with 7 additions and 467 deletions
components/autofill/core/browser/BUILD.gn
View file @ ab050037
...@@ -155,8 +155,6 @@ static_library("browser") { ...@@ -155,8 +155,6 @@ static_library("browser") {
"geo/address_i18n.h", "geo/address_i18n.h",
"geo/alternative_state_name_map.cc", "geo/alternative_state_name_map.cc",
"geo/alternative_state_name_map.h", "geo/alternative_state_name_map.h",
"geo/alternative_state_name_map_updater.cc",
"geo/alternative_state_name_map_updater.h",
"geo/autofill_country.cc", "geo/autofill_country.cc",
"geo/autofill_country.h", "geo/autofill_country.h",
"geo/country_data.cc", "geo/country_data.cc",
...@@ -635,7 +633,6 @@ source_set("unit_tests") { ...@@ -635,7 +633,6 @@ source_set("unit_tests") {
"form_structure_unittest.cc", "form_structure_unittest.cc",
"geo/address_i18n_unittest.cc", "geo/address_i18n_unittest.cc",
"geo/alternative_state_name_map_unittest.cc", "geo/alternative_state_name_map_unittest.cc",
"geo/alternative_state_name_map_updater_unittest.cc",
"geo/autofill_country_unittest.cc", "geo/autofill_country_unittest.cc",
"geo/country_names_for_locale_unittest.cc", "geo/country_names_for_locale_unittest.cc",
"geo/country_names_unittest.cc", "geo/country_names_unittest.cc",
......
components/autofill/core/browser/geo/alternative_state_name_map_test_utils.cc
View file @ ab050037
...@@ -11,8 +11,8 @@ namespace autofill { ...@@ -11,8 +11,8 @@ namespace autofill {
namespace test { namespace test {
void PopulateStateEntry(const TestStateEntry& test_state_entry, void CreateFakeStateEntry(const TestStateEntry& test_state_entry,
StateEntry* state_entry) { StateEntry* state_entry) {
state_entry->set_canonical_name(test_state_entry.canonical_name); state_entry->set_canonical_name(test_state_entry.canonical_name);
for (const auto& abbr : test_state_entry.abbreviations) for (const auto& abbr : test_state_entry.abbreviations)
state_entry->add_abbreviations(abbr); state_entry->add_abbreviations(abbr);
...@@ -31,7 +31,7 @@ void PopulateAlternativeStateNameMapForTesting( ...@@ -31,7 +31,7 @@ void PopulateAlternativeStateNameMapForTesting(
std::vector<TestStateEntry> test_state_entries) { std::vector<TestStateEntry> test_state_entries) {
for (const auto& test_state_entry : test_state_entries) { for (const auto& test_state_entry : test_state_entries) {
StateEntry state_entry; StateEntry state_entry;
PopulateStateEntry(test_state_entry, &state_entry); CreateFakeStateEntry(test_state_entry, &state_entry);
std::vector<AlternativeStateNameMap::StateName> alternatives; std::vector<AlternativeStateNameMap::StateName> alternatives;
AlternativeStateNameMap::CanonicalStateName canonical_state_name = AlternativeStateNameMap::CanonicalStateName canonical_state_name =
AlternativeStateNameMap::CanonicalStateName( AlternativeStateNameMap::CanonicalStateName(
...@@ -52,16 +52,5 @@ void PopulateAlternativeStateNameMapForTesting( ...@@ -52,16 +52,5 @@ void PopulateAlternativeStateNameMapForTesting(
} }
} }
std::string CreateStatesProtoAsString(std::string country_code) {
StatesInCountry states_data;
states_data.set_country_code(std::move(country_code));
StateEntry* entry = states_data.add_states();
PopulateStateEntry(TestStateEntry(), entry);
std::string serialized_output;
DCHECK(states_data.SerializeToString(&serialized_output));
return serialized_output;
}
} // namespace test } // namespace test
} // namespace autofill } // namespace autofill
components/autofill/core/browser/geo/alternative_state_name_map_test_utils.h
View file @ ab050037
...@@ -23,22 +23,19 @@ struct TestStateEntry { ...@@ -23,22 +23,19 @@ struct TestStateEntry {
using TestStateEntry = internal::TestStateEntry<>; using TestStateEntry = internal::TestStateEntry<>;
// Populates |state_entry| with the data in |test_state_entry|. // Creates a fake |StateEntry|.
void PopulateStateEntry(const TestStateEntry& test_state_entry, void CreateFakeStateEntry(const TestStateEntry& test_state_entry,
StateEntry* state_entry); StateEntry* state_entry);
// Clears the map for testing purposes. // Clears the map for testing purposes.
void ClearAlternativeStateNameMapForTesting(); void ClearAlternativeStateNameMapForTesting();
// Inserts a StateEntry instance into AlternativeStateNameMap for testing. // Inserts a fake |StateEntry| object into AlternativeStateNameMap for testing.
void PopulateAlternativeStateNameMapForTesting( void PopulateAlternativeStateNameMapForTesting(
std::string country_code = "DE", std::string country_code = "DE",
std::string key = "Bavaria", std::string key = "Bavaria",
std::vector<TestStateEntry> test_state_entries = {TestStateEntry()}); std::vector<TestStateEntry> test_state_entries = {TestStateEntry()});
// Returns a StateEntry instance serialized as string.
std::string CreateStatesProtoAsString(std::string country_code = "DE");
} // namespace test } // namespace test
} // namespace autofill } // namespace autofill
......
components/autofill/core/browser/geo/alternative_state_name_map_updater.cc deleted 100644 → 0
View file @ 34befa23
// Copyright 2020 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/autofill/core/browser/geo/alternative_state_name_map_updater.h"
#include <memory>
#include <string>
#include <vector>
#include "base/bind.h"
#include "base/bind_helpers.h"
#include "base/files/file_path.h"
#include "base/files/file_util.h"
#include "base/logging.h"
#include "base/ranges/algorithm.h"
#include "base/stl_util.h"
#include "base/strings/string_util.h"
#include "base/strings/utf_string_conversions.h"
#include "base/task/post_task.h"
#include "base/task/task_traits.h"
#include "base/task/thread_pool.h"
#include "base/task_runner_util.h"
#include "components/autofill/core/browser/geo/country_data.h"
#include "components/autofill/core/common/autofill_l10n_util.h"
#include "components/autofill/core/common/autofill_prefs.h"
#include "components/prefs/pref_service.h"
namespace autofill {
namespace {
// Returns data read from the file specified in |file|.
std::string LoadDataFromFile(const base::FilePath& file) {
DCHECK(!file.empty());
std::string data;
if (!base::PathExists(file)) {
VLOG(1) << "File does not exist: " << file;
return std::string();
}
if (!base::ReadFileToString(file, &data)) {
VLOG(1) << "Failed reading from file: " << file;
return std::string();
}
return data;
}
} // namespace
AlternativeStateNameMapUpdater::AlternativeStateNameMapUpdater()
: task_runner_(base::ThreadPool::CreateSequencedTaskRunner(
{base::MayBlock(), base::TaskPriority::BEST_EFFORT})) {}
AlternativeStateNameMapUpdater::~AlternativeStateNameMapUpdater() = default;
// static
bool AlternativeStateNameMapUpdater::ContainsState(
const std::vector<AlternativeStateNameMap::StateName>&
stripped_alternative_state_names,
const AlternativeStateNameMap::StateName&
stripped_state_values_from_profile) {
l10n::CaseInsensitiveCompare compare;
// Returns true if |str1| is same as |str2| in a case-insensitive comparison.
return base::ranges::any_of(
stripped_alternative_state_names,
[&](const AlternativeStateNameMap::StateName& text) {
return compare.StringsEqual(text.value(),
stripped_state_values_from_profile.value());
});
}
void AlternativeStateNameMapUpdater::LoadStatesData(
const CountryToStateNamesListMapping& country_to_state_names_map,
PrefService* pref_service,
base::OnceClosure done_callback) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
// Get the states data installation path from |pref_service|.
const std::string data_download_path =
pref_service->GetString(prefs::kAutofillStatesDataDir);
// If the installed directory path is empty, it means that the component is
// not ready for use yet.
if (data_download_path.empty()) {
std::move(done_callback).Run();
return;
}
const std::vector<std::string>& country_codes =
CountryDataMap::GetInstance()->country_codes();
// The |country_to_state_names_map| maps country_code names to a vector of
// state names that are associated with this corresponding country.
for (const auto& entry : country_to_state_names_map) {
const AlternativeStateNameMap::CountryCode& country_code = entry.first;
const std::vector<AlternativeStateNameMap::StateName>& states =
entry.second;
// This is a security check to ensure that we only attempt to read files
// that match to known countries.
if (!base::Contains(country_codes, country_code.value()))
continue;
// country_code is used as the filename.
// Example -> File "DE" contains the geographical states data of Germany.
// |data_download_path| is set by the component updater once it downloads
// the states data and should be safe to use.
const base::FilePath file_path =
base::FilePath::FromUTF8Unsafe(data_download_path)
.AppendASCII(country_code.value());
++number_pending_init_tasks_;
pending_init_done_callbacks_.push_back(std::move(done_callback));
base::PostTaskAndReplyWithResult(
task_runner_.get(), FROM_HERE,
base::BindOnce(&LoadDataFromFile, file_path),
base::BindOnce(
&AlternativeStateNameMapUpdater::ProcessLoadedStateFileContent,
weak_ptr_factory_.GetWeakPtr(), states));
}
}
void AlternativeStateNameMapUpdater::ProcessLoadedStateFileContent(
const std::vector<AlternativeStateNameMap::StateName>&
stripped_state_values_from_profiles,
const std::string& data) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK_GT(number_pending_init_tasks_, 0);
--number_pending_init_tasks_;
StatesInCountry states_data;
if (!data.empty() && states_data.ParseFromString(data)) {
DCHECK(states_data.has_country_code());
AlternativeStateNameMap::CountryCode country_code =
AlternativeStateNameMap::CountryCode(states_data.country_code());
// Boolean flags that denote in |match_found[i]| whether the match has been
// found for |stripped_state_values_from_profiles[i]|.
std::vector<bool> match_found(stripped_state_values_from_profiles.size(),
false);
// Iterates over the states data loaded from the file and builds a list of
// the state names and its variations. For each value v in
// |stripped_state_values_from_profiles|, v is compared with the values in
// the above created states list (if a match is not found for v yet). If the
// comparison results in a match, the corresponding entry is added to the
// |AlternativeStateNameMap|.
for (const auto& state_entry : states_data.states()) {
DCHECK(state_entry.has_canonical_name());
AlternativeStateNameMap::CanonicalStateName state_canonical_name =
AlternativeStateNameMap::CanonicalStateName(
base::UTF8ToUTF16(state_entry.canonical_name()));
// Build a list of all the names of the state (including its
// abbreviations) in |state_names|.
const std::vector<AlternativeStateNameMap::StateName>& state_names =
ExtractAllStateNames(state_entry);
for (size_t i = 0; i < stripped_state_values_from_profiles.size(); i++) {
if (match_found[i])
continue;
// If |stripped_state_values_from_profile[i] is in the set of names of
// the state under consideration, add it to the AlternativeStateNameMap.
if (ContainsState(state_names,
stripped_state_values_from_profiles[i])) {
AlternativeStateNameMap::GetInstance()->AddEntry(
country_code, stripped_state_values_from_profiles[i], state_entry,
state_names, &state_canonical_name);
match_found[i] = true;
}
}
}
for (size_t i = 0; i < stripped_state_values_from_profiles.size(); i++) {
// In case, no match is found, insert an |empty_state_entry| object
// to the map.
if (!match_found[i]) {
StateEntry empty_state_entry;
AlternativeStateNameMap::GetInstance()->AddEntry(
country_code, stripped_state_values_from_profiles[i],
empty_state_entry, {}, nullptr);
}
}
}
// When all pending tasks are completed, trigger and clear the pending
// callbacks.
if (number_pending_init_tasks_ == 0) {
for (auto& callback : std::exchange(pending_init_done_callbacks_, {}))
std::move(callback).Run();
}
}
std::vector<AlternativeStateNameMap::StateName>
AlternativeStateNameMapUpdater::ExtractAllStateNames(
const StateEntry& state_entry) {
DCHECK(state_entry.has_canonical_name());
std::vector<AlternativeStateNameMap::StateName> state_names;
state_names.reserve(1u + state_entry.abbreviations_size() +
state_entry.alternative_names_size());
state_names.emplace_back(AlternativeStateNameMap::NormalizeStateName(
AlternativeStateNameMap::StateName(
base::UTF8ToUTF16(state_entry.canonical_name()))));
for (const auto& abbr : state_entry.abbreviations()) {
state_names.emplace_back(AlternativeStateNameMap::NormalizeStateName(
AlternativeStateNameMap::StateName(base::UTF8ToUTF16(abbr))));
}
for (const auto& alternative_name : state_entry.alternative_names()) {
state_names.emplace_back(AlternativeStateNameMap::NormalizeStateName(
AlternativeStateNameMap::StateName(
base::UTF8ToUTF16(alternative_name))));
}
return state_names;
}
} // namespace autofill
components/autofill/core/browser/geo/alternative_state_name_map_updater.h deleted 100644 → 0
View file @ 34befa23
// Copyright 2020 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.
#ifndef COMPONENTS_AUTOFILL_CORE_BROWSER_GEO_ALTERNATIVE_STATE_NAME_MAP_UPDATER_H_
#define COMPONENTS_AUTOFILL_CORE_BROWSER_GEO_ALTERNATIVE_STATE_NAME_MAP_UPDATER_H_
#include <memory>
#include <string>
#include <vector>
#include "base/callback.h"
#include "base/memory/weak_ptr.h"
#include "base/sequence_checker.h"
#include "components/autofill/core/browser/geo/alternative_state_name_map.h"
class PrefService;
namespace autofill {
using CountryToStateNamesListMapping =
std::map<AlternativeStateNameMap::CountryCode,
std::vector<AlternativeStateNameMap::StateName>>;
// The AlternativeStateNameMap is a singleton to map between canonical state
// names and alternative representations. This class encapsulates all aspects
// about loading state data from disk and adding it to the
// AlternativeStateNameMap.
class AlternativeStateNameMapUpdater {
public:
AlternativeStateNameMapUpdater();
~AlternativeStateNameMapUpdater();
AlternativeStateNameMapUpdater(const AlternativeStateNameMapUpdater&) =
delete;
AlternativeStateNameMapUpdater& operator=(
const AlternativeStateNameMapUpdater&) = delete;
// Compares |stripped_state_value_from_profile| with the entries in
// |stripped_state_alternative_names| and returns true if a match is found.
static bool ContainsState(
const std::vector<AlternativeStateNameMap::StateName>&
stripped_alternative_state_names,
const AlternativeStateNameMap::StateName&
stripped_state_values_from_profile);
// Creates and posts jobs to the |task_runner_| for reading the state data
// files and populating AlternativeStateNameMap. Once all files are read and
// the data is incorporated into AlternativeStateNameMap, |done_callback| is
// fired. |country_to_state_names_map| specifies which state data of which
// countries to load.
// Each call to LoadStatesData triggers loading state data files, so requests
// should be batched up.
void LoadStatesData(
const CountryToStateNamesListMapping& country_to_state_names_map,
PrefService* pref_service,
base::OnceClosure done_callback);
#if defined(UNIT_TEST)
// A wrapper around |ProcessLoadedStateFileContent| used for testing purposes.
void ProcessLoadedStateFileContentForTesting(
const std::vector<AlternativeStateNameMap::StateName>&
state_values_from_profiles,
const std::string& data) {
number_pending_init_tasks_++;
ProcessLoadedStateFileContent(state_values_from_profiles, data);
}
#endif
private:
// Each entry in |state_values_from_profiles| is compared with the states
// |data| read from the files and then inserted into the
// AlternativeStateNameMap.
void ProcessLoadedStateFileContent(
const std::vector<AlternativeStateNameMap::StateName>&
state_values_from_profiles,
const std::string& data);
// Builds and returns a list of all the names of the state (including its
// abbreviations) from the |state_entry| into |state_names|.
std::vector<AlternativeStateNameMap::StateName> ExtractAllStateNames(
const StateEntry& state_entry);
// TaskRunner for reading files from disk.
scoped_refptr<base::SequencedTaskRunner> task_runner_;
// In case of concurrent requests to load states data, the callbacks are
// queued in |pending_init_done_callbacks_| and triggered once the
// |number_pending_init_tasks_| returns to 0.
std::vector<base::OnceClosure> pending_init_done_callbacks_;
int number_pending_init_tasks_ = 0;
SEQUENCE_CHECKER(sequence_checker_);
// base::WeakPtr ensures that the callback bound to the object is canceled
// when that object is destroyed.
base::WeakPtrFactory<AlternativeStateNameMapUpdater> weak_ptr_factory_{this};
};
} // namespace autofill
#endif // COMPONENTS_AUTOFILL_CORE_BROWSER_GEO_ALTERNATIVE_STATE_NAME_MAP_UPDATER_H_
components/autofill/core/browser/geo/alternative_state_name_map_updater_unittest.cc deleted 100644 → 0
View file @ 34befa23
// Copyright 2020 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/autofill/core/browser/geo/alternative_state_name_map_updater.h"
#include "base/files/file_path.h"
#include "base/files/file_util.h"
#include "base/files/scoped_temp_dir.h"
#include "base/optional.h"
#include "base/strings/utf_string_conversions.h"
#include "base/test/task_environment.h"
#include "components/autofill/core/browser/autofill_test_utils.h"
#include "components/autofill/core/browser/geo/alternative_state_name_map.h"
#include "components/autofill/core/browser/geo/alternative_state_name_map_test_utils.h"
#include "components/autofill/core/common/autofill_prefs.h"
#include "components/prefs/testing_pref_service.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
using base::ASCIIToUTF16;
using base::UTF8ToUTF16;
namespace autofill {
class AlternativeStateNameMapUpdaterTest : public ::testing::Test {
public:
AlternativeStateNameMapUpdaterTest()
: pref_service_(test::PrefServiceForTesting()) {}
void SetUp() override {
ASSERT_TRUE(data_install_dir_.CreateUniqueTempDir());
}
const base::FilePath& GetPath() const { return data_install_dir_.GetPath(); }
void WritePathToPref(const base::FilePath& file_path) {
pref_service_->SetFilePath(autofill::prefs::kAutofillStatesDataDir,
file_path);
}
protected:
base::test::TaskEnvironment task_environment_;
AlternativeStateNameMapUpdater alternative_state_name_map_updater;
std::unique_ptr<PrefService> pref_service_;
base::ScopedTempDir data_install_dir_;
};
// Tests that the states data is added to AlternativeStateNameMap.
TEST_F(AlternativeStateNameMapUpdaterTest, EntryAddedToStateMap) {
test::ClearAlternativeStateNameMapForTesting();
std::string states_data = test::CreateStatesProtoAsString();
std::vector<AlternativeStateNameMap::StateName> test_strings = {
AlternativeStateNameMap::StateName(ASCIIToUTF16("Bavaria")),
AlternativeStateNameMap::StateName(ASCIIToUTF16("Bayern")),
AlternativeStateNameMap::StateName(ASCIIToUTF16("B.Y")),
AlternativeStateNameMap::StateName(ASCIIToUTF16("Bav-aria")),
AlternativeStateNameMap::StateName(UTF8ToUTF16("amapá")),
AlternativeStateNameMap::StateName(ASCIIToUTF16("Broen")),
AlternativeStateNameMap::StateName(ASCIIToUTF16("Bavaria is in Germany")),
AlternativeStateNameMap::StateName(ASCIIToUTF16("BA is in Germany"))};
std::vector<bool> state_data_present = {true, true, true, true,
false, false, false, false};
alternative_state_name_map_updater.ProcessLoadedStateFileContentForTesting(
test_strings, states_data);
AlternativeStateNameMap* alternative_state_name_map =
AlternativeStateNameMap::GetInstance();
DCHECK(!alternative_state_name_map->IsLocalisedStateNamesMapEmpty());
for (size_t i = 0; i < test_strings.size(); i++) {
SCOPED_TRACE(test_strings[i]);
EXPECT_EQ(alternative_state_name_map->GetCanonicalStateName(
AlternativeStateNameMap::CountryCode("DE"),
test_strings[i]) != base::nullopt,
state_data_present[i]);
}
}
// Tests that the AlternativeStateNameMap is populated when
// |StateNameMapUpdater::LoadStatesData()| is called.
TEST_F(AlternativeStateNameMapUpdaterTest, TestLoadStatesData) {
test::ClearAlternativeStateNameMapForTesting();
base::WriteFile(GetPath().AppendASCII("DE"),
test::CreateStatesProtoAsString());
WritePathToPref(GetPath());
base::RunLoop run_loop;
alternative_state_name_map_updater.LoadStatesData(
{{AlternativeStateNameMap::CountryCode("DE"),
{AlternativeStateNameMap::StateName(ASCIIToUTF16("Bavaria"))}}},
pref_service_.get(), run_loop.QuitClosure());
run_loop.Run();
EXPECT_NE(
AlternativeStateNameMap::GetInstance()->GetCanonicalStateName(
AlternativeStateNameMap::CountryCode("DE"),
AlternativeStateNameMap::StateName(base::ASCIIToUTF16("Bavaria"))),
base::nullopt);
}
// Tests the |StateNameMapUpdater::ContainsState()| functionality.
TEST_F(AlternativeStateNameMapUpdaterTest, ContainsState) {
EXPECT_TRUE(AlternativeStateNameMapUpdater::ContainsState(
{AlternativeStateNameMap::StateName(base::ASCIIToUTF16("Bavaria")),
AlternativeStateNameMap::StateName(base::ASCIIToUTF16("Bayern")),
AlternativeStateNameMap::StateName(base::ASCIIToUTF16("BY"))},
AlternativeStateNameMap::StateName(base::ASCIIToUTF16("Bavaria"))));
EXPECT_FALSE(AlternativeStateNameMapUpdater::ContainsState(
{AlternativeStateNameMap::StateName(base::ASCIIToUTF16("Bavaria")),
AlternativeStateNameMap::StateName(base::ASCIIToUTF16("Bayern")),
AlternativeStateNameMap::StateName(base::ASCIIToUTF16("BY"))},
AlternativeStateNameMap::StateName(base::ASCIIToUTF16("California"))));
}
} // namespace autofill
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment