[rac] Add AddressValidator API.

[rac] Add AddressValidator API.

This patch adds the AddressValidator API without the implementation. It is
intended to start integration into Chrome in parallel with implementing
the API in the library. This is not the final API, as it may change as
integration into Chrome progresses.

BUG=327046
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=240488

[please use gerrit instead, 2013-12-10 23:24:06]

Evan: PTAL the AddressValidator API. Will this work for Chrome?

I've also tested copying these changes to the libaddressinput repository---it
build successfully and passes all unit tests there, too.

[Evan Stade, 2013-12-10 23:59:43]

the unit test building and the addressvalidator interface don't seem like
sufficiently related changes to warrant being part of the same CL.

https://codereview.chromium.org/106863006/diff/60001/third_party/libaddressin...
File
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h
(right):

https://codereview.chromium.org/106863006/diff/60001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h:44:
//        validator_->PreloadRules("US", this);
nit: this is out of date?

https://codereview.chromium.org/106863006/diff/60001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h:71:
struct AddressValidator {
class

https://codereview.chromium.org/106863006/diff/60001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h:73:
// The status of address validation.
what would be the status if you haven't called LoadRules yet?

https://codereview.chromium.org/106863006/diff/60001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h:81:
LOAD_RULES_FAIL,
nit: RULES_UNAVAILABLE

https://codereview.chromium.org/106863006/diff/60001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h:85:
LOADING_RULES
nit: RULES_NOT_READY

https://codereview.chromium.org/106863006/diff/60001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h:103:
bool force,
hmmm, I can't remember why this was necessary in our previous design discussion.
We don't need/want to reload rules if they successfully loaded already, right?
Only if they didn't successfully load. Leave this out for now IMO, we can add it
back if we decide we need it. Make LoadRules do nothing if you already have
rules for the country, and retry to download them if you don't have them or
loading previously failed.

https://codereview.chromium.org/106863006/diff/60001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h:104:
LoadRulesObserver* observer);
I would make the observer

a) a delegate (0-1 exist)
b) passed in the constructor
c) not required

https://codereview.chromium.org/106863006/diff/60001/third_party/libaddressin...
File third_party/libaddressinput/chromium/cpp/test/address_validator_test.cc
(right):

https://codereview.chromium.org/106863006/diff/60001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/test/address_validator_test.cc:41: //
TODO(rouslan): Add tests when the API is implemented.
IMO don't add this file till it does something useful. This doesn't assist in
integration.

[please use gerrit instead, 2013-12-11 00:56:44]

Evan: PTAL Patch Set 7.

https://codereview.chromium.org/106863006/diff/60001/third_party/libaddressin...
File
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h
(right):

https://codereview.chromium.org/106863006/diff/60001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h:44:
//        validator_->PreloadRules("US", this);
On 2013/12/10 23:59:43, Evan Stade wrote:
> nit: this is out of date?

Yes, I've updated the comment now.

https://codereview.chromium.org/106863006/diff/60001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h:71:
struct AddressValidator {
On 2013/12/10 23:59:43, Evan Stade wrote:
> class

Done.

https://codereview.chromium.org/106863006/diff/60001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h:73:
// The status of address validation.
On 2013/12/10 23:59:43, Evan Stade wrote:
> what would be the status if you haven't called LoadRules yet?

We should assert(false) if we haven't called LoadRules() yet. That's a
programmer mistake.

https://codereview.chromium.org/106863006/diff/60001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h:81:
LOAD_RULES_FAIL,
On 2013/12/10 23:59:43, Evan Stade wrote:
> nit: RULES_UNAVAILABLE

Done.

https://codereview.chromium.org/106863006/diff/60001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h:85:
LOADING_RULES
On 2013/12/10 23:59:43, Evan Stade wrote:
> nit: RULES_NOT_READY

Done.

https://codereview.chromium.org/106863006/diff/60001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h:103:
bool force,
On 2013/12/10 23:59:43, Evan Stade wrote:
> hmmm, I can't remember why this was necessary in our previous design
discussion.
> We don't need/want to reload rules if they successfully loaded already, right?
> Only if they didn't successfully load. Leave this out for now IMO, we can add
it
> back if we decide we need it. Make LoadRules do nothing if you already have
> rules for the country, and retry to download them if you don't have them or
> loading previously failed.

Done.

https://codereview.chromium.org/106863006/diff/60001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h:104:
LoadRulesObserver* observer);
On 2013/12/10 23:59:43, Evan Stade wrote:
> I would make the observer
> 
> a) a delegate (0-1 exist)
> b) passed in the constructor
> c) not required

Done.

https://codereview.chromium.org/106863006/diff/60001/third_party/libaddressin...
File third_party/libaddressinput/chromium/cpp/test/address_validator_test.cc
(right):

https://codereview.chromium.org/106863006/diff/60001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/test/address_validator_test.cc:41: //
TODO(rouslan): Add tests when the API is implemented.
On 2013/12/10 23:59:43, Evan Stade wrote:
> IMO don't add this file till it does something useful. This doesn't assist in
> integration.

Removed for now.

[Evan Stade, 2013-12-11 01:21:49]

https://codereview.chromium.org/106863006/diff/60001/third_party/libaddressin...
File
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h
(right):

https://codereview.chromium.org/106863006/diff/60001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h:73:
// The status of address validation.
On 2013/12/11 00:56:45, Rouslan Solomakhin wrote:
> On 2013/12/10 23:59:43, Evan Stade wrote:
> > what would be the status if you haven't called LoadRules yet?
> 
> We should assert(false) if we haven't called LoadRules() yet. That's a
> programmer mistake.

uhhhh, I suppose that's one solution. But there's also the case where you've
called LoadRules, just not for the correct country (i.e. the country in the
address in ValidateAddress). Client can be careful to avoid letting this happen,
but I can also half-imagine scenarios where it's extra work to make sure this
doesn't happen. Crashing seems like a harsh penalty. Can we return
RULES_UNAVAILABLE in that case? The distinction between "load failed" and "load
not tried" could be left up to the client to track based on LoadRulesObserver
calls.

https://codereview.chromium.org/106863006/diff/140001/third_party/libaddressi...
File
third_party/libaddressinput/chromium/cpp/include/libaddressinput/load_rules_delegate.h
(right):

https://codereview.chromium.org/106863006/diff/140001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/load_rules_delegate.h:28:
virtual void OnAddressValidationRulesLoaded(const std::string& country_code,
docs.

https://codereview.chromium.org/106863006/diff/140001/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/address_validator.cc (right):

https://codereview.chromium.org/106863006/diff/140001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/address_validator.cc:51:
assert(problems != NULL);
Typically I think it's nicer to allow null outparams. You might just use this to
check if the rules are ready, for some reason...

[please use gerrit instead, 2013-12-11 02:10:11]

Evan: PTAL Patch Set 8.

https://codereview.chromium.org/106863006/diff/140001/third_party/libaddressi...
File
third_party/libaddressinput/chromium/cpp/include/libaddressinput/load_rules_delegate.h
(right):

https://codereview.chromium.org/106863006/diff/140001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/load_rules_delegate.h:28:
virtual void OnAddressValidationRulesLoaded(const std::string& country_code,
On 2013/12/11 01:21:49, Evan Stade wrote:
> docs.

Done.

https://codereview.chromium.org/106863006/diff/140001/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/address_validator.cc (right):

https://codereview.chromium.org/106863006/diff/140001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/address_validator.cc:51:
assert(problems != NULL);
On 2013/12/11 01:21:49, Evan Stade wrote:
> Typically I think it's nicer to allow null outparams. You might just use this
to
> check if the rules are ready, for some reason...

Done.

[Evan Stade, 2013-12-11 03:22:10]

lgtm

https://codereview.chromium.org/106863006/diff/160001/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/address_validator.cc (right):

https://codereview.chromium.org/106863006/diff/160001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/address_validator.cc:43:
load_rules_delegate_->OnAddressValidationRulesLoaded(country_code, false);
nit: as far as a stub implementation is concerned, I think simply doing nothing
is nicer, because the naive implementation of OnAddressValidationRulesLoaded()
might be to just call LoadRules again, which would lead to an infinite loop with
this particular stub impl.

[lararennie, 2013-12-11 08:25:37]

https://codereview.chromium.org/106863006/diff/160001/third_party/libaddressi...
File
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h
(right):

https://codereview.chromium.org/106863006/diff/160001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h:95:
// localities. A typical data size is 10KB. The largest is 250KB. If a country
The largest is "currently" 250KB....

Maybe point them at the data so they can always go and see themselves if they
are interested...

[roubert, 2013-12-11 17:38:25]

https://codereview.chromium.org/106863006/diff/160001/third_party/libaddressi...
File
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h
(right):

https://codereview.chromium.org/106863006/diff/160001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h:73:
enum Result {
Maybe a different name, eg. Status, might help highlight the fact that this
isn't actually the validation result; an invalid address would have
ValidateAddress() return SUCCESS if it was successful in determining that the
address was invalid.

https://codereview.chromium.org/106863006/diff/160001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h:93:
// Loads the generic validation rules for |country_code| and specific rules
Is this for pre-loading data for countries that are anticipated to be used? It
would be good if you here could comment on when and why anyone would want to
call this method.

https://codereview.chromium.org/106863006/diff/160001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h:99:
// |load_rules_delegate| when the load finishes.
"the load" -> "the loading"?

https://codereview.chromium.org/106863006/diff/160001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h:109:
// If the |problems| parameter is NULL, then checks whether the validation
This seems like a strange API to me. Would it not be more straightforward to
here require problems to be non-NULL and then provide a separate method to check
whether validation rules are available? (When do you envision that a user of
this library would want to check that?)

https://codereview.chromium.org/106863006/diff/160001/third_party/libaddressi...
File
third_party/libaddressinput/chromium/cpp/include/libaddressinput/load_rules_delegate.h
(right):

https://codereview.chromium.org/106863006/diff/160001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/load_rules_delegate.h:36:
virtual void OnAddressValidationRulesLoaded(const std::string& country_code,
Is this not just a callback? Why have this special base class that a user of the
library must inherit from, instead of treating it just like all other callbacks
so far?

https://codereview.chromium.org/106863006/diff/160001/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/address_validator.cc (right):

https://codereview.chromium.org/106863006/diff/160001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/address_validator.cc:17: #include
<libaddressinput/downloader.h>
You're not actually using this header file.

https://codereview.chromium.org/106863006/diff/160001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/address_validator.cc:19: #include
<libaddressinput/storage.h>
You're not actually using this header file.

https://codereview.chromium.org/106863006/diff/160001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/address_validator.cc:21: #include
<cassert>
You're not actually using this header file.

[please use gerrit instead, 2013-12-11 22:23:56]

I will keep these comments in mind for the upstreaming, thank you!

On 2013/12/11 17:38:25, roubert wrote:
>
https://codereview.chromium.org/106863006/diff/160001/third_party/libaddressi...
> File
>
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h
> (right):
> 
>
https://codereview.chromium.org/106863006/diff/160001/third_party/libaddressi...
>
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h:73:
> enum Result {
> Maybe a different name, eg. Status, might help highlight the fact that this
> isn't actually the validation result; an invalid address would have
> ValidateAddress() return SUCCESS if it was successful in determining that the
> address was invalid.
> 
>
https://codereview.chromium.org/106863006/diff/160001/third_party/libaddressi...
>
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h:93:
> // Loads the generic validation rules for |country_code| and specific rules
> Is this for pre-loading data for countries that are anticipated to be used? It
> would be good if you here could comment on when and why anyone would want to
> call this method.
> 
>
https://codereview.chromium.org/106863006/diff/160001/third_party/libaddressi...
>
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h:99:
> // |load_rules_delegate| when the load finishes.
> "the load" -> "the loading"?
> 
>
https://codereview.chromium.org/106863006/diff/160001/third_party/libaddressi...
>
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h:109:
> // If the |problems| parameter is NULL, then checks whether the validation
> This seems like a strange API to me. Would it not be more straightforward to
> here require problems to be non-NULL and then provide a separate method to
check
> whether validation rules are available? (When do you envision that a user of
> this library would want to check that?)
> 
>
https://codereview.chromium.org/106863006/diff/160001/third_party/libaddressi...
> File
>
third_party/libaddressinput/chromium/cpp/include/libaddressinput/load_rules_delegate.h
> (right):
> 
>
https://codereview.chromium.org/106863006/diff/160001/third_party/libaddressi...
>
third_party/libaddressinput/chromium/cpp/include/libaddressinput/load_rules_delegate.h:36:
> virtual void OnAddressValidationRulesLoaded(const std::string& country_code,
> Is this not just a callback? Why have this special base class that a user of
the
> library must inherit from, instead of treating it just like all other
callbacks
> so far?
> 
>
https://codereview.chromium.org/106863006/diff/160001/third_party/libaddressi...
> File third_party/libaddressinput/chromium/cpp/src/address_validator.cc
(right):
> 
>
https://codereview.chromium.org/106863006/diff/160001/third_party/libaddressi...
> third_party/libaddressinput/chromium/cpp/src/address_validator.cc:17: #include
> <libaddressinput/downloader.h>
> You're not actually using this header file.
> 
>
https://codereview.chromium.org/106863006/diff/160001/third_party/libaddressi...
> third_party/libaddressinput/chromium/cpp/src/address_validator.cc:19: #include
> <libaddressinput/storage.h>
> You're not actually using this header file.
> 
>
https://codereview.chromium.org/106863006/diff/160001/third_party/libaddressi...
> third_party/libaddressinput/chromium/cpp/src/address_validator.cc:21: #include
> <cassert>
> You're not actually using this header file.

[Evan Stade, 2013-12-12 06:31:36]

https://codereview.chromium.org/106863006/diff/180001/third_party/libaddressi...
File
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h
(right):

https://codereview.chromium.org/106863006/diff/180001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h:71:
class AddressValidator {
By the way, I think it would be cleaner if we convert the public-facing API to
an interface, and create an AddressValidatorImpl for the implementation.

But this is a change that's easy enough to make later on if we judge it will
substantially improve readability or ease of testing, so I'm happy to leave it
as a TODO for now.

[please use gerrit instead, 2013-12-12 17:50:24]

On 2013/12/12 06:31:36, Evan Stade wrote:
>
https://codereview.chromium.org/106863006/diff/180001/third_party/libaddressi...
> File
>
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h
> (right):
> 
>
https://codereview.chromium.org/106863006/diff/180001/third_party/libaddressi...
>
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h:71:
> class AddressValidator {
> By the way, I think it would be cleaner if we convert the public-facing API to
> an interface, and create an AddressValidatorImpl for the implementation.

I was planning to do this, because I suspect that our basictypes.h will conflict
with Chrome's own file. The interface can include this method:

static AddressValidator* AddressValidator::Build(
    const Downloader* downloader,
    Storage* storage) {
  return new AddressValidatorImpl(downloader, storage);
}

> But this is a change that's easy enough to make later on if we judge it will
> substantially improve readability or ease of testing, so I'm happy to leave it
> as a TODO for now.

[commit-bot: I haz the power, 2013-12-12 21:20:39]

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

[commit-bot: I haz the power, 2013-12-12 23:56:53]

Change committed as 240488