[rac] Validate an address.

[rac] Validate an address.

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

[please use gerrit instead, 2013-12-18 21:45:39]

Evan: PTAL.

This patch validates states, localities, and dependent localities. For example,
in US, this patch will mark "CA" valid, but "XX" invalid.

The error messages are not pulled from messages.grd yet.

[Evan Stade, 2013-12-19 02:12:38]

https://codereview.chromium.org/116363003/diff/20001/third_party/libaddressin...
File third_party/libaddressinput/chromium/cpp/src/address_validator.cc (right):

https://codereview.chromium.org/116363003/diff/20001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/address_validator.cc:51: return
ADMIN_AREA;
you've now encoded this ordering twice in one CL.

https://codereview.chromium.org/116363003/diff/20001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/address_validator.cc:64: const
std::string& GetValue(const AddressData& address, AddressField field) {
didn't I read this function elsewhere in this CL?

https://codereview.chromium.org/116363003/diff/20001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/address_validator.cc:88:
filter.equal_range(field);
while this is interesting, I think the following is easier to read because it's
more idiomatic (and has no worse time complexity):

for (AddressProblemFilter::const_iterator it = filter.begin();
     it != filter.end(); ++it) {
  if (it->first == field && it->second == problem_type) {
    return true;
  }
}

https://codereview.chromium.org/116363003/diff/20001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/address_validator.cc:176:
std::map<AddressField, const Rule*> field_rule_map =
this function nee4ds some vertical whitespace somewhere.

https://codereview.chromium.org/116363003/diff/20001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/address_validator.cc:179:
field_rule_it = field_rule_map.begin();
indent by 4 more

https://codereview.chromium.org/116363003/diff/20001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/address_validator.cc:184: if (field
< DEPENDENT_LOCALITY) {
nit: invert this check and continue;, that keeps the total indentation down for
the rest of the function (indentation roughly aligns with how much context one
has to remember when reading code)

https://codereview.chromium.org/116363003/diff/20001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/address_validator.cc:187: if
(!rule.GetSubKeys().empty() && !sub_value.empty() &&
nit: easier to read with one condition per line (when you are wrapping any
conditions, wrap all conditions)

https://codereview.chromium.org/116363003/diff/20001/third_party/libaddressin...
File third_party/libaddressinput/chromium/cpp/src/lookup_key.cc (right):

https://codereview.chromium.org/116363003/diff/20001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/lookup_key.cc:38: const
std::string& GetAddressFieldValue(const AddressData& address,
this seems like it should be an AddressData method. Also, why isn't country
supported?

https://codereview.chromium.org/116363003/diff/20001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/lookup_key.cc:115:
std::map<AddressField, const Rule*> LookupKey::BuildFieldRuleMap(
this actually feels like it should be a method on AddressData, mainly because
you only use |this| once, but you use |address| 5 times (if you count the refs
in GetAddressFieldValue)

https://codereview.chromium.org/116363003/diff/20001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/lookup_key.cc:117: static const int
kNumberOfLevels  = 4;
use arraysize()

https://codereview.chromium.org/116363003/diff/20001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/lookup_key.cc:125: const LookupKey*
key = this;
does this assume |this| is a country level LookupKey? could you at least add an
assert?

https://codereview.chromium.org/116363003/diff/20001/third_party/libaddressin...
File third_party/libaddressinput/chromium/cpp/src/lookup_key.h (right):

https://codereview.chromium.org/116363003/diff/20001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/lookup_key.h:100: // Returns the
lookup key for |sub_region| or NULL. The caller does not own
explain when it may return NULL

https://codereview.chromium.org/116363003/diff/20001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/lookup_key.h:104: // Returns the
lookup key for |language_code| or NULL. The caller does not own
explain when it may return NULL

[please use gerrit instead, 2014-01-08 00:55:51]

Evan: PTAL Patch Set 2.

https://codereview.chromium.org/116363003/diff/20001/third_party/libaddressin...
File third_party/libaddressinput/chromium/cpp/src/address_validator.cc (right):

https://codereview.chromium.org/116363003/diff/20001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/address_validator.cc:51: return
ADMIN_AREA;
On 2013/12/19 02:12:38, Evan Stade wrote:
> you've now encoded this ordering twice in one CL.

Fixed.

https://codereview.chromium.org/116363003/diff/20001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/address_validator.cc:64: const
std::string& GetValue(const AddressData& address, AddressField field) {
On 2013/12/19 02:12:38, Evan Stade wrote:
> didn't I read this function elsewhere in this CL?

Moved to AddressData.

https://codereview.chromium.org/116363003/diff/20001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/address_validator.cc:88:
filter.equal_range(field);
On 2013/12/19 02:12:38, Evan Stade wrote:
> while this is interesting, I think the following is easier to read because
it's
> more idiomatic (and has no worse time complexity):
> 
> for (AddressProblemFilter::const_iterator it = filter.begin();
>      it != filter.end(); ++it) {
>   if (it->first == field && it->second == problem_type) {
>     return true;
>   }
> }

Done.

https://codereview.chromium.org/116363003/diff/20001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/address_validator.cc:176:
std::map<AddressField, const Rule*> field_rule_map =
On 2013/12/19 02:12:38, Evan Stade wrote:
> this function nee4ds some vertical whitespace somewhere.

Done.

https://codereview.chromium.org/116363003/diff/20001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/address_validator.cc:179:
field_rule_it = field_rule_map.begin();
On 2013/12/19 02:12:38, Evan Stade wrote:
> indent by 4 more

The rest of libaddressinput uses this format when "it = collection.begin()" does
not fit on the first line of the for-loop:

for (CollectionType::const_iterator
     it = collection.begin();
     it != collection.end();
     ++it) {
  ...
}

I think this makes the loop easier to reed. WYT?

https://codereview.chromium.org/116363003/diff/20001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/address_validator.cc:184: if (field
< DEPENDENT_LOCALITY) {
On 2013/12/19 02:12:38, Evan Stade wrote:
> nit: invert this check and continue;, that keeps the total indentation down
for
> the rest of the function (indentation roughly aligns with how much context one
> has to remember when reading code)

No longer applicable.

https://codereview.chromium.org/116363003/diff/20001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/address_validator.cc:187: if
(!rule.GetSubKeys().empty() && !sub_value.empty() &&
On 2013/12/19 02:12:38, Evan Stade wrote:
> nit: easier to read with one condition per line (when you are wrapping any
> conditions, wrap all conditions)

Done.

https://codereview.chromium.org/116363003/diff/20001/third_party/libaddressin...
File third_party/libaddressinput/chromium/cpp/src/lookup_key.cc (right):

https://codereview.chromium.org/116363003/diff/20001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/lookup_key.cc:38: const
std::string& GetAddressFieldValue(const AddressData& address,
On 2013/12/19 02:12:38, Evan Stade wrote:
> this seems like it should be an AddressData method. Also, why isn't country
> supported?

Done.

https://codereview.chromium.org/116363003/diff/20001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/lookup_key.cc:115:
std::map<AddressField, const Rule*> LookupKey::BuildFieldRuleMap(
On 2013/12/19 02:12:38, Evan Stade wrote:
> this actually feels like it should be a method on AddressData, mainly because
> you only use |this| once, but you use |address| 5 times (if you count the refs
> in GetAddressFieldValue)

AddressData is public API, but Rule is not. Moving this method to AddressData
will require exposing Rule as part of the public API. I don't think we should do
that.

https://codereview.chromium.org/116363003/diff/20001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/lookup_key.cc:117: static const int
kNumberOfLevels  = 4;
On 2013/12/19 02:12:38, Evan Stade wrote:
> use arraysize()

No longer applicable.

https://codereview.chromium.org/116363003/diff/20001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/lookup_key.cc:125: const LookupKey*
key = this;
On 2013/12/19 02:12:38, Evan Stade wrote:
> does this assume |this| is a country level LookupKey? could you at least add
an
> assert?

No longer applicable.

https://codereview.chromium.org/116363003/diff/20001/third_party/libaddressin...
File third_party/libaddressinput/chromium/cpp/src/lookup_key.h (right):

https://codereview.chromium.org/116363003/diff/20001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/lookup_key.h:100: // Returns the
lookup key for |sub_region| or NULL. The caller does not own
On 2013/12/19 02:12:38, Evan Stade wrote:
> explain when it may return NULL

Done.

https://codereview.chromium.org/116363003/diff/20001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/lookup_key.h:104: // Returns the
lookup key for |language_code| or NULL. The caller does not own
On 2013/12/19 02:12:38, Evan Stade wrote:
> explain when it may return NULL

Done.

[Evan Stade, 2014-01-08 21:29:33]

https://codereview.chromium.org/116363003/diff/20001/third_party/libaddressin...
File third_party/libaddressinput/chromium/cpp/src/address_validator.cc (right):

https://codereview.chromium.org/116363003/diff/20001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/address_validator.cc:179:
field_rule_it = field_rule_map.begin();
On 2014/01/08 00:55:52, Rouslan Solomakhin wrote:
> On 2013/12/19 02:12:38, Evan Stade wrote:
> > indent by 4 more
> 
> The rest of libaddressinput uses this format when "it = collection.begin()"
does
> not fit on the first line of the for-loop:
> 
> for (CollectionType::const_iterator
>      it = collection.begin();
>      it != collection.end();
>      ++it) {
>   ...
> }
> 
> I think this makes the loop easier to reed. WYT?

Why do you think it makes it easier to read? I think it makes it harder because
it's less obvious that the second line is a continuation of the first.

https://codereview.chromium.org/116363003/diff/270001/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/ruleset.cc (right):

https://codereview.chromium.org/116363003/diff/270001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/ruleset.cc:73:
std::map<AddressField, const Rule*> Ruleset::BuildRulesForAddress(
I don't really understand the purpose for this function. Why can't
AddressValidator just iterate over the Ruleset? Why does it need this map?

Also, I feel like the Ruleset or Rule should know its field.

[please use gerrit instead, 2014-01-09 19:42:00]

Evan: PTAL Patch Set 3.

https://codereview.chromium.org/116363003/diff/20001/third_party/libaddressin...
File third_party/libaddressinput/chromium/cpp/src/address_validator.cc (right):

https://codereview.chromium.org/116363003/diff/20001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/address_validator.cc:179:
field_rule_it = field_rule_map.begin();
On 2014/01/08 21:29:34, Evan Stade wrote:
> On 2014/01/08 00:55:52, Rouslan Solomakhin wrote:
> > On 2013/12/19 02:12:38, Evan Stade wrote:
> > > indent by 4 more
> > 
> > The rest of libaddressinput uses this format when "it = collection.begin()"
> does
> > not fit on the first line of the for-loop:
> > 
> > for (CollectionType::const_iterator
> >      it = collection.begin();
> >      it != collection.end();
> >      ++it) {
> >   ...
> > }
> > 
> > I think this makes the loop easier to reed. WYT?
> 
> Why do you think it makes it easier to read? I think it makes it harder
because
> it's less obvious that the second line is a continuation of the first.

Done.

https://codereview.chromium.org/116363003/diff/270001/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/ruleset.cc (right):

https://codereview.chromium.org/116363003/diff/270001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/ruleset.cc:73:
std::map<AddressField, const Rule*> Ruleset::BuildRulesForAddress(
On 2014/01/08 21:29:34, Evan Stade wrote:
> I don't really understand the purpose for this function. Why can't
> AddressValidator just iterate over the Ruleset? Why does it need this map?
> 
> Also, I feel like the Ruleset or Rule should know its field.

Done.

[Evan Stade, 2014-01-09 21:58:45]

lgtm

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

https://codereview.chromium.org/116363003/diff/550001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/address_validator.cc:110: Ruleset*
ruleset = ruleset_it->second;
constify?

https://codereview.chromium.org/116363003/diff/550001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/address_validator.cc:222:
std::map<std::string, Ruleset*> rules_;
const Ruleset*?

https://codereview.chromium.org/116363003/diff/550001/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/ruleset.cc (right):

https://codereview.chromium.org/116363003/diff/550001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/ruleset.cc:71: return it !=
language_codes_.end() ? *it->second : *rule_.get();
*rule_ (no .get())

[please use gerrit instead, 2014-01-09 22:10:53]

Addressed the comments and sent to cq.

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

https://codereview.chromium.org/116363003/diff/550001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/address_validator.cc:110: Ruleset*
ruleset = ruleset_it->second;
On 2014/01/09 21:58:46, Evan Stade wrote:
> constify?

Done.

https://codereview.chromium.org/116363003/diff/550001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/address_validator.cc:222:
std::map<std::string, Ruleset*> rules_;
On 2014/01/09 21:58:46, Evan Stade wrote:
> const Ruleset*?

Done.

https://codereview.chromium.org/116363003/diff/550001/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/ruleset.cc (right):

https://codereview.chromium.org/116363003/diff/550001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/ruleset.cc:71: return it !=
language_codes_.end() ? *it->second : *rule_.get();
On 2014/01/09 21:58:46, Evan Stade wrote:
> *rule_ (no .get())

Done.

[commit-bot: I haz the power, 2014-01-09 22:12:04]

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

[commit-bot: I haz the power, 2014-01-10 00:35:07]

Retried try job too often on linux_aura for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2014-01-10 01:41:55]

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

[commit-bot: I haz the power, 2014-01-10 02:24:31]

Change committed as 244046