Retry downloading rules for libaddressinput.

third_party/libaddressinput/chromium/chrome_address_validator.h

 // Copyright 2014 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 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"
 #include "third_party/libaddressinput/src/cpp/include/libaddressinput/preload_supplier.h"
 
 namespace i18n {
 namespace addressinput {
 class AddressNormalizer;
 class Downloader;
 class Storage;
 struct AddressData;
 }
 }
 
 namespace autofill {
 
 class InputSuggester;
 
 // The object to be notified when loading of address validation rules is
 // finished.
 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;
 };
 
 // Interface to the libaddressinput AddressValidator for Chromium Autofill. The
 // class is named AddressValidator to simplify switching between libaddressinput
 // and this version.
 //
 // It's not possible to name this file address_validator.h because some
 // compilers do not handle multiple files with the same name (although in
 // different directories) gracefully. This class is a shim between upstream
@@ skipping 76 matching lines @@
       size_t suggestion_limit,
       std::vector< ::i18n::addressinput::AddressData>* suggestions) const;
 
   // Canonicalizes the administrative area in |address_data|. For example,
   // "texas" changes to "TX". Returns true on success, otherwise leaves
   // |address_data| alone and returns false.
   virtual bool CanonicalizeAdministrativeArea(
       ::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();
 
   // Verifies that |validator_| succeeded. Invoked by |validated_| callback.
   void Validated(bool success,
                  const ::i18n::addressinput::AddressData&,
                  const ::i18n::addressinput::FieldProblemMap&);
 
   // 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_;
 
   // Suggests addresses based on user input.
   const scoped_ptr<InputSuggester> input_suggester_;
 
   // Normalizes addresses into a canonical form.
   const scoped_ptr< ::i18n::addressinput::AddressNormalizer> normalizer_;
 
   // Validates addresses.
   const scoped_ptr<const ::i18n::addressinput::AddressValidator> validator_;
 
   // The callback that |validator_| invokes when it finished validating an
   // address.
   const scoped_ptr<const ::i18n::addressinput::AddressValidator::Callback>
       validated_;
 
   // The callback that |supplier_| invokes when it finished loading rules.
   const scoped_ptr<const ::i18n::addressinput::PreloadSupplier::Callback>
       rules_loaded_;
 
   // Not owned delegate to invoke when |suppler_| finished loading rules. Can be
   // 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);
 };
 
 }  // namespace autofill
 
 #endif  // THIRD_PARTY_LIBADDRESSINPUT_CHROMIUM_CHROME_ADDRESS_VALIDATOR_H_