[rac] Download all rules for a country code in libaddressinput.

third_party/libaddressinput/chromium/cpp/src/address_validator.cc

Index: third_party/libaddressinput/chromium/cpp/src/address_validator.cc
diff --git a/third_party/libaddressinput/chromium/cpp/src/address_validator.cc b/third_party/libaddressinput/chromium/cpp/src/address_validator.cc
index 0732826b9562917ab6b6c7bad98351cb347e3afe..6b426c7029cd32506cfec11a6ddfd871a7f53f7c 100644
--- a/third_party/libaddressinput/chromium/cpp/src/address_validator.cc
+++ b/third_party/libaddressinput/chromium/cpp/src/address_validator.cc
@@ -18,33 +18,112 @@
 #include <libaddressinput/load_rules_delegate.h>
 #include <libaddressinput/localization.h>
 #include <libaddressinput/storage.h>
+#include <libaddressinput/util/basictypes.h>
+#include <libaddressinput/util/scoped_ptr.h>
 
+#include <cassert>
+#include <map>
 #include <string>
+#include <utility>
 
+#include "country_rules_retriever.h"
 #include "retriever.h"
+#include "ruleset.h"
+#include "util/stl_util.h"
 
 namespace i18n {
 namespace addressinput {
 
-AddressValidator::AddressValidator(scoped_ptr<const Downloader> downloader,
-                                   scoped_ptr<Storage> storage,
-                                   LoadRulesDelegate* load_rules_delegate)
-    : retriever_(new Retriever(
+namespace {
+
+// Validates AddressData structure.
+class AddressValidatorImpl : public AddressValidator {
+ public:
+  // Takes ownership of |downloader| and |storage|. Does not take ownership of
+  // |load_rules_delegate|.
+  AddressValidatorImpl(scoped_ptr<const Downloader> downloader,
+                       scoped_ptr<Storage> storage,
+                       LoadRulesDelegate* load_rules_delegate)
+    : retriever_(scoped_ptr<Retriever>(new Retriever(
           VALIDATION_DATA_URL,
           downloader.Pass(),
-          scoped_ptr<Storage>(storage.Pass()))),
-      load_rules_delegate_(load_rules_delegate) {}
+          scoped_ptr<Storage>(storage.Pass())))),

[Evan Stade, 2014/01/08 21:08:59]

I'm not sure but I don't think you need the scoped_ptr<Storage>() here (might be
my fault for adding it)

[please use gerrit instead, 2014/01/09 00:38:11]

Done.

+      load_rules_delegate_(load_rules_delegate),
+      status_(),
+      rules_() {}
 
-AddressValidator::~AddressValidator() {}
+  virtual ~AddressValidatorImpl() {
+    STLDeleteContainerPairSecondPointers(rules_.begin(), rules_.end());

[Evan Stade, 2014/01/08 21:08:59]

STLDeleteValues

[please use gerrit instead, 2014/01/09 00:38:11]

Done.

+  }
+
+  // AddressValidator implementation.
+  virtual void LoadRules(const std::string& country_code) {
+    std::map<std::string, Status>::const_iterator status_it =
+        status_.find(country_code);
+    if (status_it == status_.end()) {
+      assert(rules_.find(country_code) == rules_.end());
+      status_.insert(std::make_pair(country_code, RULES_NOT_READY));

[Evan Stade, 2014/01/08 21:08:59]

do you really need this to be a map? It seems like you could make it a little
simpler by checking if it exists already in rules_ (that handles the SUCCESS
case) and keeping around a std::set of outstanding requests (that handles
RULES_NOT_READY)

[please use gerrit instead, 2014/01/09 00:38:11]

Done.

+      retriever_.RetrieveRules(
+          country_code,
+          BuildScopedPtrCallback(this, &AddressValidatorImpl::OnRulesLoaded));
+    }
+  }
+
+  // AddressValidator implementation.
+  virtual Status ValidateAddress(
+      const AddressData& address,
+      const AddressProblemFilter& filter,
+      const Localization& localization,
+      AddressProblems* problems) const {
+    return RULES_UNAVAILABLE;

[Evan Stade, 2014/01/08 21:08:59]

add TODO to implement (so there's no confusion about whether this is a temporary
stub or permanent)

[please use gerrit instead, 2014/01/09 00:38:11]

Done.

+  }
+
+ private:
+  // Called when CountryRulesRetriever::RetrieveRules finishes loading all rules
+  // for the |country_code|.
+  void OnRulesLoaded(bool success,
+                     const std::string& country_code,
+                     scoped_ptr<Ruleset> ruleset) {
+    assert(rules_.find(country_code) == rules_.end());
+    if (success) {
+      status_[country_code] = SUCCESS;
+      rules_.insert(std::make_pair(country_code, ruleset.release()));
+    } else {
+      status_.erase(country_code);
+    }
+    if (load_rules_delegate_) {
+      load_rules_delegate_->OnAddressValidationRulesLoaded(
+          country_code, success);
+    }
+  }
+
+  // Retrieves all rules for a country code.
+  CountryRulesRetriever retriever_;
 
-void AddressValidator::LoadRules(const std::string& country_code) {}
+  // An optional delegate to be invoked when rules for a country code finish
+  // loading.
+  LoadRulesDelegate* load_rules_delegate_;
+
+  // A mapping of a country code to the status of its rule download.
+  std::map<std::string, Status> status_;
+
+  // A mapping of a country code to all rules loaded for that country code.

[Evan Stade, 2014/01/08 21:08:59]

Ruleset ownership comment plx

[please use gerrit instead, 2014/01/09 00:38:11]

Done.

+  std::map<std::string, Ruleset*> rules_;
+
+  DISALLOW_COPY_AND_ASSIGN(AddressValidatorImpl);
+};
+
+}  // namespace
+
+AddressValidator::~AddressValidator() {}
 
-AddressValidator::Status AddressValidator::ValidateAddress(
-    const AddressData& address,
-    const AddressProblemFilter& filter,
-    const Localization& localization,
-    AddressProblems* problems) const {
-  return RULES_UNAVAILABLE;
+// static
+scoped_ptr<AddressValidator> AddressValidator::Build(
+    scoped_ptr<const Downloader> downloader,
+    scoped_ptr<Storage> storage,
+    LoadRulesDelegate* load_rules_delegate) {
+  return scoped_ptr<AddressValidator>(new AddressValidatorImpl(
+      downloader.Pass(), storage.Pass(), load_rules_delegate));
 }
 
 }  // namespace addressinput