[rac] Parse postal code formats and required fields in libaddressinput.

[rac] Parse postal code formats and required fields in libaddressinput.

This patch makes the Rule object parse postal code format and required
fields. This data will be used in validation logic.

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

[please use gerrit instead, 2013-12-13 23:22:51]

Evan: PTAL.

[Evan Stade, 2013-12-13 23:34:29]

https://codereview.chromium.org/106763007/diff/1/third_party/libaddressinput/...
File third_party/libaddressinput/chromium/cpp/src/rule.h (right):

https://codereview.chromium.org/106763007/diff/1/third_party/libaddressinput/...
third_party/libaddressinput/chromium/cpp/src/rule.h:72: const std::string&
GetPostalCodeExamples() const {
how is this useful?

https://codereview.chromium.org/106763007/diff/1/third_party/libaddressinput/...
third_party/libaddressinput/chromium/cpp/src/rule.h:82: const std::string&
GetPostalCodesUrl() const { return postal_codes_url_; }
how is this useful?

Please leave this out until we actually decide to use it somehow. Another
example where trying to review code in a void is very hard.

[please use gerrit instead, 2013-12-16 20:49:56]

Evan: PTAL.

https://codereview.chromium.org/106763007/diff/1/third_party/libaddressinput/...
File third_party/libaddressinput/chromium/cpp/src/rule.h (right):

https://codereview.chromium.org/106763007/diff/1/third_party/libaddressinput/...
third_party/libaddressinput/chromium/cpp/src/rule.h:72: const std::string&
GetPostalCodeExamples() const {
On 2013/12/13 23:34:29, Evan Stade wrote:
> how is this useful?

Left out until we decided to use the error messages and tooltips with postal
code examples.

https://codereview.chromium.org/106763007/diff/1/third_party/libaddressinput/...
third_party/libaddressinput/chromium/cpp/src/rule.h:82: const std::string&
GetPostalCodesUrl() const { return postal_codes_url_; }
On 2013/12/13 23:34:29, Evan Stade wrote:
> how is this useful?
> 
> Please leave this out until we actually decide to use it somehow. Another
> example where trying to review code in a void is very hard.

Left out until we decided to use the error messages and tooltips with postal
code URLs.

[Evan Stade, 2013-12-17 19:47:24]

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

https://codereview.chromium.org/106763007/diff/20001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/address_field_util.h:42: void
ParseAddressFieldsRequired(const std::string& required,
why isn't this part of Rule? what other code will ever call it?

https://codereview.chromium.org/106763007/diff/20001/third_party/libaddressin...
File third_party/libaddressinput/chromium/cpp/test/rule_test.cc (right):

https://codereview.chromium.org/106763007/diff/20001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/test/rule_test.cc:141: TEST(RuleTest,
ParsesRequiredFieldsCorrectly) {
this doesn't seem like a very thorough test. Wouldn't you want to test that it
parses multiple required fields, or what happens when the format is bad?

[please use gerrit instead, 2013-12-18 00:57:49]

Evan: PTAL Patch Set 3.

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

https://codereview.chromium.org/106763007/diff/20001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/address_field_util.h:42: void
ParseAddressFieldsRequired(const std::string& required,
On 2013/12/17 19:47:24, Evan Stade wrote:
> why isn't this part of Rule? what other code will ever call it?

This header is only every used in Rule. Moved everything there.

https://codereview.chromium.org/106763007/diff/20001/third_party/libaddressin...
File third_party/libaddressinput/chromium/cpp/test/rule_test.cc (right):

https://codereview.chromium.org/106763007/diff/20001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/test/rule_test.cc:141: TEST(RuleTest,
ParsesRequiredFieldsCorrectly) {
On 2013/12/17 19:47:24, Evan Stade wrote:
> this doesn't seem like a very thorough test. Wouldn't you want to test that it
> parses multiple required fields, or what happens when the format is bad?

Added these tests.

[Evan Stade, 2013-12-18 23:18:16]

https://codereview.chromium.org/106763007/diff/40001/third_party/libaddressin...
File third_party/libaddressinput/chromium/cpp/src/rule.h (right):

https://codereview.chromium.org/106763007/diff/40001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/rule.h:29: // An extension of
AddressField enum used only internally. The values are
"internally" sounds like "internal to Rule", but I guess you mean "internal to
libaddressinput"

https://codereview.chromium.org/106763007/diff/40001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/rule.h:32: NEWLINE = -1
comment should explain what this actually means...

also instead of using negative values, can you set it equal to the largest
addressfield value + 1 (there can be an addressfield type that is officially the
largest value)

this whole thing feels like a hack, why don't you make GetFormat() a vector of
vectors so that the whole concept of the newline can truly be internal to this
file?

[please use gerrit instead, 2013-12-19 22:43:47]

Evan: PTAL Patch Set 4.

[Evan Stade, 2013-12-19 23:00:04]

https://codereview.chromium.org/106763007/diff/60001/third_party/libaddressin...
File third_party/libaddressinput/chromium/cpp/src/rule.cc (right):

https://codereview.chromium.org/106763007/diff/60001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/rule.cc:120: static const
std::map<char, AddressField> kFields(InitFields());
no static non-PODs allowed

use switch/case for this.

https://codereview.chromium.org/106763007/diff/60001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/rule.cc:151: next = format.begin()
+ 1;
splitstring on % makes this shorter I think?

[please use gerrit instead, 2013-12-20 00:37:42]

Evan: PTAL Patch Set 5.

https://codereview.chromium.org/106763007/diff/60001/third_party/libaddressin...
File third_party/libaddressinput/chromium/cpp/src/rule.cc (right):

https://codereview.chromium.org/106763007/diff/60001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/rule.cc:120: static const
std::map<char, AddressField> kFields(InitFields());
On 2013/12/19 23:00:04, Evan Stade wrote:
> no static non-PODs allowed
> 
> use switch/case for this.

Switch/case does not work for strings. Changed to if-else.

https://codereview.chromium.org/106763007/diff/60001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/rule.cc:151: next = format.begin()
+ 1;
On 2013/12/19 23:00:04, Evan Stade wrote:
> splitstring on % makes this shorter I think?

Good guess! I tried your suggestion, but the result was -8 lines removed and +8
lines added. No net gain. Therefore, I've decided to keep the algorithm as-is.

https://codereview.chromium.org/106763007/diff/80001/third_party/libaddressin...
File third_party/libaddressinput/chromium/cpp/src/rule.cc (right):

https://codereview.chromium.org/106763007/diff/80001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/rule.cc:95: if (*current != '%') {
Inlined IsTokenPrefix().

https://codereview.chromium.org/106763007/diff/80001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/rule.cc:104: } else if (*next ==
'n') {
Inlined IsNewlineToken().

https://codereview.chromium.org/106763007/diff/80001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/rule.cc:203: static const char
kFormatKey[] = "fmt";
Moved the string constants to immediately before they're used.

[Evan Stade, 2013-12-20 03:55:12]

https://codereview.chromium.org/106763007/diff/80001/third_party/libaddressin...
File third_party/libaddressinput/chromium/cpp/src/rule.cc (right):

https://codereview.chromium.org/106763007/diff/80001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/rule.cc:57: AddressField
ParseToken(char c) {
bool ParseToken(char c, AddressField* field) {
...
}

https://codereview.chromium.org/106763007/diff/80001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/rule.cc:91: for
(std::string::const_iterator current = format.begin(),
I dunno, this looks shorter and more idiomatic to me.

fields->clear();
fields->resize(1);
std::vector<string> exploded_string;
SplitString(string, '%', &exploded_string);
for (size_t i = 0; i < exploded_string.size(); i++) {
  AddressField field = COUNTRY;
  
  if (exploded_string[i] == "n") {
    fields->push_back(std::vector<AddressField>());
  } else if (ParseToken(exploded_string[i], &field) {
    fields->back().push_back(field);
  }
}

https://codereview.chromium.org/106763007/diff/80001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/rule.cc:203: static const char
kFormatKey[] = "fmt";
On 2013/12/20 00:37:43, Rouslan Solomakhin wrote:
> Moved the string constants to immediately before they're used.

I think you can just inline them at this point.

[please use gerrit instead, 2013-12-20 22:03:32]

Evan: PTAL Patch Set 6.

https://codereview.chromium.org/106763007/diff/80001/third_party/libaddressin...
File third_party/libaddressinput/chromium/cpp/src/rule.cc (right):

https://codereview.chromium.org/106763007/diff/80001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/rule.cc:57: AddressField
ParseToken(char c) {
On 2013/12/20 03:55:13, Evan Stade wrote:
> bool ParseToken(char c, AddressField* field) {
> ...
> }

Nice! Done.

https://codereview.chromium.org/106763007/diff/80001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/rule.cc:91: for
(std::string::const_iterator current = format.begin(),
On 2013/12/20 03:55:13, Evan Stade wrote:
> I dunno, this looks shorter and more idiomatic to me.
> 
> fields->clear();
> fields->resize(1);
> std::vector<string> exploded_string;
> SplitString(string, '%', &exploded_string);
> for (size_t i = 0; i < exploded_string.size(); i++) {
>   AddressField field = COUNTRY;
>   
>   if (exploded_string[i] == "n") {
>     fields->push_back(std::vector<AddressField>());
>   } else if (ParseToken(exploded_string[i], &field) {
>     fields->back().push_back(field);
>   }
> }

I'll give you the idiomatic. Done.

This function is still not shorter if your take the corner cases into account
:-).

https://codereview.chromium.org/106763007/diff/80001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/rule.cc:203: static const char
kFormatKey[] = "fmt";
On 2013/12/20 03:55:13, Evan Stade wrote:
> On 2013/12/20 00:37:43, Rouslan Solomakhin wrote:
> > Moved the string constants to immediately before they're used.
> 
> I think you can just inline them at this point.

I prefer to avoid inlining constants that are used more than once. Each one of
these constants is used more than once.

https://codereview.chromium.org/106763007/diff/90001/third_party/libaddressin...
File third_party/libaddressinput/chromium/cpp/src/address_ui.cc (right):

https://codereview.chromium.org/106763007/diff/90001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/address_ui.cc:105: line_it->size()
== 1 ? AddressUiComponent::HINT_LONG
I like that this logic is now quite clear :-)

[Evan Stade, 2013-12-20 22:21:27]

https://codereview.chromium.org/106763007/diff/90001/third_party/libaddressin...
File third_party/libaddressinput/chromium/cpp/src/rule.cc (right):

https://codereview.chromium.org/106763007/diff/90001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/rule.cc:79: bool begin_newline =
true;
I think you should either

a) explicitly fail when handling invalid formats (i.e. if there are 2 newlines
in a row, 2 %s in a row, two non-%s in a row, an empty string, etc.). Return an
empty vector in this case.

or

b) don't define or guarantee the value of |fields| when handling an invalid
format. This also means not going out of your way to try to paper over certain
invalid cases, such as 2 successive newlines.

[please use gerrit instead, 2013-12-20 23:11:19]

Evan: PTAL Patch Set 7.

https://codereview.chromium.org/106763007/diff/90001/third_party/libaddressin...
File third_party/libaddressinput/chromium/cpp/src/rule.cc (right):

https://codereview.chromium.org/106763007/diff/90001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/rule.cc:79: bool begin_newline =
true;
On 2013/12/20 22:21:27, Evan Stade wrote:
> I think you should either
> 
> a) explicitly fail when handling invalid formats (i.e. if there are 2 newlines
> in a row, 2 %s in a row, two non-%s in a row, an empty string, etc.). Return
an
> empty vector in this case.
> 
> or
> 
> b) don't define or guarantee the value of |fields| when handling an invalid
> format. This also means not going out of your way to try to paper over certain
> invalid cases, such as 2 successive newlines.

Selected (b). Done.

[Evan Stade, 2013-12-21 00:27:12]

https://codereview.chromium.org/106763007/diff/90001/third_party/libaddressin...
File third_party/libaddressinput/chromium/cpp/src/address_ui.cc (right):

https://codereview.chromium.org/106763007/diff/90001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/address_ui.cc:105: line_it->size()
== 1 ? AddressUiComponent::HINT_LONG
On 2013/12/20 22:03:33, Rouslan Solomakhin wrote:
> I like that this logic is now quite clear :-)

what happened to SHORT_EOL or whatever we were going to call it?

https://codereview.chromium.org/106763007/diff/210001/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/rule.cc (right):

https://codereview.chromium.org/106763007/diff/210001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/rule.cc:88: for
(std::vector<std::string>::size_type i = 1;
what's wrong with size_t?

https://codereview.chromium.org/106763007/diff/210001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/rule.cc:106: for
(std::string::const_iterator token = required.begin();
nit: more idiomatic/easier to read imo:

for (size_t i = 0; i < required.size(); ++i) {
  ...
     ParseToken(required[i]);
  ...
}

you don't have to make this change, but it is consistent with general
convention. When I read the current code, it takes me a moment to realize it's
just looking at the characters in a string.

https://codereview.chromium.org/106763007/diff/210001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/rule.cc:203: 
extra newline

[please use gerrit instead, 2014-01-02 19:49:38]

Evan: PTAL.

https://codereview.chromium.org/106763007/diff/90001/third_party/libaddressin...
File third_party/libaddressinput/chromium/cpp/src/address_ui.cc (right):

https://codereview.chromium.org/106763007/diff/90001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/address_ui.cc:105: line_it->size()
== 1 ? AddressUiComponent::HINT_LONG
On 2013/12/21 00:27:13, Evan Stade wrote:
> On 2013/12/20 22:03:33, Rouslan Solomakhin wrote:
> > I like that this logic is now quite clear :-)
> 
> what happened to SHORT_EOL or whatever we were going to call it?

I've added a test that verifies that no consecutive rows in address formats have
multiple fields. Therefore, SHORT_EOL is not necessary for libaddressinput. (The
address formats are static data in region_data_constants.cc.)

https://codereview.chromium.org/106763007/diff/210001/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/rule.cc (right):

https://codereview.chromium.org/106763007/diff/210001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/rule.cc:88: for
(std::vector<std::string>::size_type i = 1;
On 2013/12/21 00:27:13, Evan Stade wrote:
> what's wrong with size_t?

You're right, size_t is better in this case. size_type is better only when
comparing a variable to std::string::npos.

https://codereview.chromium.org/106763007/diff/210001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/rule.cc:106: for
(std::string::const_iterator token = required.begin();
On 2013/12/21 00:27:13, Evan Stade wrote:
> nit: more idiomatic/easier to read imo:
> 
> for (size_t i = 0; i < required.size(); ++i) {
>   ...
>      ParseToken(required[i]);
>   ...
> }
> 
> you don't have to make this change, but it is consistent with general
> convention. When I read the current code, it takes me a moment to realize it's
> just looking at the characters in a string.

Done.

[Evan Stade, 2014-01-02 22:05:11]

lgtm

[commit-bot: I haz the power, 2014-01-02 22:52:56]

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

[commit-bot: I haz the power, 2014-01-02 22:53:04]

Failed to apply patch for third_party/libaddressinput/libaddressinput.gyp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file third_party/libaddressinput/libaddressinput.gyp
  Hunk #1 succeeded at 67 (offset 2 lines).
  Hunk #2 FAILED at 113.
  1 out of 2 hunks FAILED -- saving rejects to file
third_party/libaddressinput/libaddressinput.gyp.rej

Patch:       third_party/libaddressinput/libaddressinput.gyp
Index: third_party/libaddressinput/libaddressinput.gyp
diff --git a/third_party/libaddressinput/libaddressinput.gyp
b/third_party/libaddressinput/libaddressinput.gyp
index
e76285fcb87a2253f1ccfca64eec0e0a1884d5b0..10997c5bc6ab6620571723498216c70d06ae29a6
100644
--- a/third_party/libaddressinput/libaddressinput.gyp
+++ b/third_party/libaddressinput/libaddressinput.gyp
@@ -65,8 +65,6 @@
        
'<(libaddressinput_dir)/cpp/include/libaddressinput/util/internal/template_util.h',
         '<(libaddressinput_dir)/cpp/include/libaddressinput/util/scoped_ptr.h',
         '<(libaddressinput_dir)/cpp/src/address_field.cc',
-        '<(libaddressinput_dir)/cpp/src/address_field_util.cc',
-        '<(libaddressinput_dir)/cpp/src/address_field_util.h',
         '<(libaddressinput_dir)/cpp/src/address_problem.cc',
         '<(libaddressinput_dir)/cpp/src/address_ui.cc',
         '<(libaddressinput_dir)/cpp/src/address_validator.cc',
@@ -115,7 +113,6 @@
         '<(SHARED_INTERMEDIATE_DIR)/libaddressinput/',
       ],
       'sources': [
-        '<(libaddressinput_dir)/cpp/test/address_field_util_test.cc',
         '<(libaddressinput_dir)/cpp/test/address_ui_test.cc',
         '<(libaddressinput_dir)/cpp/test/fake_downloader.cc',
         '<(libaddressinput_dir)/cpp/test/fake_downloader.h',

[commit-bot: I haz the power, 2014-01-02 23:25:40]

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

[commit-bot: I haz the power, 2014-01-03 00:12:38]

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

[commit-bot: I haz the power, 2014-01-03 17:34:05]

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

[commit-bot: I haz the power, 2014-01-03 18:15:21]

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

[commit-bot: I haz the power, 2014-01-03 19:06:25]

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

[commit-bot: I haz the power, 2014-01-03 19:41:30]

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

[commit-bot: I haz the power, 2014-01-03 22:47:18]

Change committed as 242951