Retry downloading rules for libaddressinput.

Retry downloading rules for libaddressinput.

Retry downloading rules after 8, 16, 32, 64, 128, 256, and 512 seconds,
while requestAutocomplete dialog is open. (512 seconds is ~8.5 minutes.)

TEST=libaddressinput_unittests:FailingAddressValidatorTest.*
BUG=343397
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283712

[please use gerrit instead, 2014-07-16 00:09:30]

Evan: PTAL.

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

https://codereview.chromium.org/392083002/diff/80001/third_party/libaddressin...
File third_party/libaddressinput/chromium/chrome_address_validator.cc (right):

https://codereview.chromium.org/392083002/diff/80001/third_party/libaddressin...
third_party/libaddressinput/chromium/chrome_address_validator.cc:36: // The
number of seconds to wait between the first and second attempts to load
this isn't quite right. It's the amount of time between the first attempt's
failure and the second attempt's initiation, right?

https://codereview.chromium.org/392083002/diff/80001/third_party/libaddressin...
third_party/libaddressinput/chromium/chrome_address_validator.cc:38: static
const int kLoadRulesRetryPeriodSeconds = 60;
I think we should set this lower. As noted above, it already has to wait for a
request to timeout before the retry period starts. So maybe start at 8 seconds?

https://codereview.chromium.org/392083002/diff/80001/third_party/libaddressin...
third_party/libaddressinput/chromium/chrome_address_validator.cc:152: return;
nit: \n

https://codereview.chromium.org/392083002/diff/80001/third_party/libaddressin...
third_party/libaddressinput/chromium/chrome_address_validator.cc:175:
retrier_->ResetRetryCount(region_code);
why do you need to reset it here?

https://codereview.chromium.org/392083002/diff/80001/third_party/libaddressin...
File third_party/libaddressinput/chromium/chrome_address_validator.h (right):

https://codereview.chromium.org/392083002/diff/80001/third_party/libaddressin...
third_party/libaddressinput/chromium/chrome_address_validator.h:150: class
Retrier {
I don't think this class is worthwhile. Just add the functionality directly onto
ChromeAddressValidator.

https://codereview.chromium.org/392083002/diff/80001/third_party/libaddressin...
third_party/libaddressinput/chromium/chrome_address_validator.h:170:
base::WeakPtr<AddressValidator> rule_loader_;
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.

https://codereview.chromium.org/392083002/diff/80001/third_party/libaddressin...
third_party/libaddressinput/chromium/chrome_address_validator.h:178:
base::TimeDelta retry_period_;
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]

Evan: PTAL Patch Set 3.

https://codereview.chromium.org/392083002/diff/80001/third_party/libaddressin...
File third_party/libaddressinput/chromium/chrome_address_validator.cc (right):

https://codereview.chromium.org/392083002/diff/80001/third_party/libaddressin...
third_party/libaddressinput/chromium/chrome_address_validator.cc:36: // The
number of seconds to wait between the first and second attempts to load
On 2014/07/16 02:11:25, Evan Stade wrote:
> this isn't quite right. It's the amount of time between the first attempt's
> failure and the second attempt's initiation, right?

Right. Fixed.

https://codereview.chromium.org/392083002/diff/80001/third_party/libaddressin...
third_party/libaddressinput/chromium/chrome_address_validator.cc:38: static
const int kLoadRulesRetryPeriodSeconds = 60;
On 2014/07/16 02:11:25, Evan Stade wrote:
> I think we should set this lower. As noted above, it already has to wait for a
> request to timeout before the retry period starts. So maybe start at 8
seconds?

Done.

https://codereview.chromium.org/392083002/diff/80001/third_party/libaddressin...
third_party/libaddressinput/chromium/chrome_address_validator.cc:152: return;
On 2014/07/16 02:11:25, Evan Stade wrote:
> nit: \n

Done.

https://codereview.chromium.org/392083002/diff/80001/third_party/libaddressin...
third_party/libaddressinput/chromium/chrome_address_validator.cc:175:
retrier_->ResetRetryCount(region_code);
On 2014/07/16 02:11:25, Evan Stade wrote:
> why do you need to reset it here?

True, come to think of it, I don't need it. Removed.

https://codereview.chromium.org/392083002/diff/80001/third_party/libaddressin...
File third_party/libaddressinput/chromium/chrome_address_validator.h (right):

https://codereview.chromium.org/392083002/diff/80001/third_party/libaddressin...
third_party/libaddressinput/chromium/chrome_address_validator.h:150: class
Retrier {
On 2014/07/16 02:11:25, Evan Stade wrote:
> I don't think this class is worthwhile. Just add the functionality directly
onto
> ChromeAddressValidator.

Done.

https://codereview.chromium.org/392083002/diff/80001/third_party/libaddressin...
third_party/libaddressinput/chromium/chrome_address_validator.h:170:
base::WeakPtr<AddressValidator> rule_loader_;
On 2014/07/16 02:11:25, Evan Stade wrote:
> 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.

Done.

https://codereview.chromium.org/392083002/diff/80001/third_party/libaddressin...
third_party/libaddressinput/chromium/chrome_address_validator.h:178:
base::TimeDelta retry_period_;
On 2014/07/16 02:11:25, Evan Stade wrote:
> 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.

Interesting idea. I like it. Done.

[Evan Stade, 2014-07-16 23:35:06]

lgtm

https://codereview.chromium.org/392083002/diff/110001/third_party/libaddressi...
File third_party/libaddressinput/chromium/chrome_address_validator.cc (right):

https://codereview.chromium.org/392083002/diff/110001/third_party/libaddressi...
third_party/libaddressinput/chromium/chrome_address_validator.cc:38: static
const int kRetryPeriodSeconds = 8;
inline

https://codereview.chromium.org/392083002/diff/110001/third_party/libaddressi...
third_party/libaddressinput/chromium/chrome_address_validator.cc:46: static
const int kBackoffFactor = 2;
inline (and the comment is overkill)

https://codereview.chromium.org/392083002/diff/110001/third_party/libaddressi...
File third_party/libaddressinput/chromium/chrome_address_validator.h (right):

https://codereview.chromium.org/392083002/diff/110001/third_party/libaddressi...
third_party/libaddressinput/chromium/chrome_address_validator.h:148: // the
second attempt's initiation to load rules. Overridden in
nit: s/Overridden.../Exposed for testing.

[please use gerrit instead, 2014-07-16 23:45:17]

Thank you for the review! (Sending to cq.)

https://codereview.chromium.org/392083002/diff/110001/third_party/libaddressi...
File third_party/libaddressinput/chromium/chrome_address_validator.cc (right):

https://codereview.chromium.org/392083002/diff/110001/third_party/libaddressi...
third_party/libaddressinput/chromium/chrome_address_validator.cc:38: static
const int kRetryPeriodSeconds = 8;
On 2014/07/16 23:35:06, Evan Stade wrote:
> inline

Done.

https://codereview.chromium.org/392083002/diff/110001/third_party/libaddressi...
third_party/libaddressinput/chromium/chrome_address_validator.cc:46: static
const int kBackoffFactor = 2;
On 2014/07/16 23:35:06, Evan Stade wrote:
> inline (and the comment is overkill)

Done.

https://codereview.chromium.org/392083002/diff/110001/third_party/libaddressi...
File third_party/libaddressinput/chromium/chrome_address_validator.h (right):

https://codereview.chromium.org/392083002/diff/110001/third_party/libaddressi...
third_party/libaddressinput/chromium/chrome_address_validator.h:148: // the
second attempt's initiation to load rules. Overridden in
On 2014/07/16 23:35:06, Evan Stade wrote:
> nit: s/Overridden.../Exposed for testing.

Done.

[please use gerrit instead, 2014-07-16 23:47:25]

The CQ bit was checked by rouslan@chromium.org

[commit-bot: I haz the power, 2014-07-16 23:49:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rouslan@chromium.org/392083002/130001

[commit-bot: I haz the power, 2014-07-17 08:26:47]

Change committed as 283712