Retry downloading rules for libaddressinput.

third_party/libaddressinput/chromium/chrome_address_validator.h

Index: third_party/libaddressinput/chromium/chrome_address_validator.h
diff --git a/third_party/libaddressinput/chromium/chrome_address_validator.h b/third_party/libaddressinput/chromium/chrome_address_validator.h
index 49875c8272a1071c56baa1e309cc126285c06d66..64a05c8d2d921dfd0c97015b721de75e33b97f3e 100644
--- a/third_party/libaddressinput/chromium/chrome_address_validator.h
+++ b/third_party/libaddressinput/chromium/chrome_address_validator.h
@@ -5,12 +5,15 @@
 #ifndef THIRD_PARTY_LIBADDRESSINPUT_CHROMIUM_CHROME_ADDRESS_VALIDATOR_H_
 #define THIRD_PARTY_LIBADDRESSINPUT_CHROMIUM_CHROME_ADDRESS_VALIDATOR_H_
 
-#include <cstddef>
+#include <map>
 #include <string>
 #include <vector>
 
+#include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+#include "base/time/time.h"
 #include "third_party/libaddressinput/src/cpp/include/libaddressinput/address_field.h"
 #include "third_party/libaddressinput/src/cpp/include/libaddressinput/address_validator.h"
 #include "third_party/libaddressinput/src/cpp/include/libaddressinput/callback.h"
@@ -35,14 +38,14 @@ class LoadRulesListener {
  public:
   virtual ~LoadRulesListener() {}
 
-  // Called when the validation rules for the |country_code| have been loaded.
-  // The validation rules include the generic rules for the |country_code| and
+  // Called when the validation rules for the |region_code| have been loaded.
+  // The validation rules include the generic rules for the |region_code| and
   // specific rules for the country's administrative areas, localities, and
   // dependent localities. If a country has language-specific validation rules,
   // then these are also loaded.
   //
   // The |success| parameter is true when the rules were loaded successfully.
-  virtual void OnAddressValidationRulesLoaded(const std::string& country_code,
+  virtual void OnAddressValidationRulesLoaded(const std::string& region_code,
                                               bool success) = 0;
 };
 
@@ -139,8 +142,44 @@ class AddressValidator {
       ::i18n::addressinput::AddressData* address) const;
 
  private:
+  friend class FailingAddressValidatorTest;
   friend class MockAddressValidator;
 
+  // Encapsulates the logic for retrying to load rules with exponential backoff
+  // and a maxium number of attempts.
+  class Retrier {

[Evan Stade, 2014/07/16 02:11:25]

I don't think this class is worthwhile. Just add the functionality directly onto
ChromeAddressValidator.

[please use gerrit instead, 2014/07/16 23:22:03]

Done.

+   public:
+    explicit Retrier(const base::WeakPtr<AddressValidator>& rule_loader);
+    ~Retrier();
+
+    // Resets the number of attempts to enable retrying loading rules again.
+    void ResetRetryCount(const std::string& region_code);
+
+    // Retries loading rules after a delay, unless the maximum number of
+    // attempts is exceeded.
+    void RetryLoadRules(const std::string& region_code);
+
+    // Sets the period of time to wait between the first and second attempts to
+    // load rules, if the first attempt failed.
+    void set_retry_period(const base::TimeDelta& retry_period) {
+      retry_period_ = retry_period;
+    }
+
+   private:
+    // Rule loader used when retrying to load rules.
+    base::WeakPtr<AddressValidator> rule_loader_;

[Evan Stade, 2014/07/16 02:11:25]

this member is only necessary because of the private class. You only actually
need attempts_number_. So you've created a class that replaces a single member
var with an instance of itself. Net change to number of member vars on
ChromeAddressValidator is zero.

[please use gerrit instead, 2014/07/16 23:22:03]

Done.

+
+    // A mapping of region codes to the number of attempts to retry loading
+    // rules for the region code.
+    std::map<std::string, int> attempts_number_;
+
+    // The period of time to wait between the first and second attempts to load
+    // rules.
+    base::TimeDelta retry_period_;

[Evan Stade, 2014/07/16 02:11:25]

This isn't very useful. It's actually a static value. Why does it need to be
stored as a member var?

I see that it's used for testing. I think the better way to allow a test to
override the delay is to create a virtual method called GetBaseRetryPeriod and a
testing class that overrides it. Then you don't have this extra cruft in the
production code.

[please use gerrit instead, 2014/07/16 23:22:03]

Interesting idea. I like it. Done.

+
+    DISALLOW_COPY_AND_ASSIGN(Retrier);
+  };
+
   // Constructor used only for MockAddressValidator.
   AddressValidator();
 
@@ -151,7 +190,10 @@ class AddressValidator {
 
   // Invokes the |load_rules_listener_|, if it's not NULL. Called by
   // |rules_loaded_| callback.
-  void RulesLoaded(bool success, const std::string& country_code, int);
+  void RulesLoaded(bool success, const std::string& region_code, int);
+
+  // Retries loading rules without resetting the retry counter.
+  void RetryLoadRules(const std::string& region_code);
 
   // Loads and stores aggregate rules at COUNTRY level.
   const scoped_ptr< ::i18n::addressinput::PreloadSupplier> supplier_;
@@ -178,6 +220,14 @@ class AddressValidator {
   // NULL.
   LoadRulesListener* const load_rules_listener_;
 
+  // Retries loading rules after a failed attempt.
+  scoped_ptr<Retrier> retrier_;
+
+  // Member variables should appear before the WeakPtrFactory, to ensure that
+  // any WeakPtrs to AddressValidator are invalidated before its members
+  // variable's destructors are executed, rendering them invalid.
+  base::WeakPtrFactory<AddressValidator> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(AddressValidator);
 };