[rac] Download all rules for a country code in libaddressinput.

[rac] Download all rules for a country code in libaddressinput.

This patch enables downloading all rules for a country code. The rules are
organized into a Ruleset tree, where nodes contain region-wide rules,
language-specific rules, and Rulesets for the sub-regions.

For example, the country code of Canada is "CA". The Ruleset for "CA"
contains the general validation rules for Canada in the default language
of the country, which is English, or "en".

One of the child nodes of "CA" is a Rule for "fr" language. This Rule
contains the general validation rules for Canada in the French language.

The rest of the child nodes of "CA" are the Ruleset objects for all of the
Canada's provinces. For example, there's a Ruleset for "BC" for British
Columbia.

Example of a Ruleset for Canada and some of its provinces:

                  CA-->fr
                  |
-------------------------------------
|        |        |        |        |
v        v        v        v        v
AB-->fr  BC-->fr  MB-->fr  NB-->fr  NL-->fr

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

[please use gerrit instead, 2013-12-13 02:46:50]

Evan: PTAL. I've avoided adding unit tests until we finalize this design, as
we've discussed before. Also, the implementation of
CountryRulesRetriever::RetrieverRules() will change after we can download all
rules in a single HTTP GET.

[please use gerrit instead, 2013-12-16 23:41:38]

Evan: PTAL Patch Set 3 with more comments. I've also updated the issue
description.

https://codereview.chromium.org/109323011/diff/10001/third_party/libaddressin...
File third_party/libaddressinput/chromium/cpp/src/address_ui.cc (left):

https://codereview.chromium.org/109323011/diff/10001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/address_ui.cc:41: return rule;
(This code is moved into Rule.cc.)

You mentioned in a different code review that we should not have static
non-PODs. However, this code is full of static data. Take
region_data_constants.cc for example:

http://src.chromium.org/viewvc/chrome/trunk/src/third_party/libaddressinput/c...
(line 994).


It has a static std::map<std::string, std::string> object.

[Evan Stade, 2013-12-18 23:54:35]

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

https://codereview.chromium.org/109323011/diff/80001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/lookup_key.h:15: // An object to
store a parsed validation rule, its identifier, and its level.
not sure why this comment is here rather than above the class it refers to

https://codereview.chromium.org/109323011/diff/80001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/lookup_key.h:58: //   
Process(administrative_area_key->GetLevel(),
not sure where Process is defined, but why not just take a single parameter that
is the LookupKey? And why not have Process be a method of LookupKey?

https://codereview.chromium.org/109323011/diff/80001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/lookup_key.h:67: class LookupKey {
call this class Ruleset?

[please use gerrit instead, 2014-01-06 20:48:41]

Evan: PTAL Patch Set 7. 

I've changed LookupKey to Ruleset that contains only rules. I've also improved
the comments for this code, because it's a bit tricky. We will need to test this
well.

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

https://codereview.chromium.org/109323011/diff/80001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/lookup_key.h:15: // An object to
store a parsed validation rule, its identifier, and its level.
On 2013/12/18 23:54:35, Evan Stade wrote:
> not sure why this comment is here rather than above the class it refers to

Moved class-specific comments near class. Using file comments only to describe
how multiple classes in the file interact.

https://codereview.chromium.org/109323011/diff/80001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/lookup_key.h:58: //   
Process(administrative_area_key->GetLevel(),
On 2013/12/18 23:54:35, Evan Stade wrote:
> not sure where Process is defined, but why not just take a single parameter
that
> is the LookupKey? And why not have Process be a method of LookupKey?

No longer applicable, as I'm using English instead of code to describe the
Ruleset does.

https://codereview.chromium.org/109323011/diff/80001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/lookup_key.h:67: class LookupKey {
On 2013/12/18 23:54:35, Evan Stade wrote:
> call this class Ruleset?

Done.

[please use gerrit instead, 2014-01-06 21:08:44]

Patch Set 9 has a few minor fixes. It may be more pleasant to review.

[Evan Stade, 2014-01-06 22:54:18]

https://codereview.chromium.org/109323011/diff/590002/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/include/libaddressinput/callback.h
(right):

https://codereview.chromium.org/109323011/diff/590002/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/callback.h:1:
// Copyright (C) 2013 Google Inc.
why the changes to this file?

https://codereview.chromium.org/109323011/diff/590002/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc
(right):

https://codereview.chromium.org/109323011/diff/590002/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc:254: new
Helper(country_code, rules_ready.Pass(), *retriever_);
this seems dangerous. How do you know Helper won't outlast |this|, and therefore
deref |retriever_| after its been freed?

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

https://codereview.chromium.org/109323011/diff/590002/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/rule.cc:165: static Rule rule;
no static non-pods

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

https://codereview.chromium.org/109323011/diff/590002/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/ruleset.cc:51:
assert(sub_regions_.find(sub_region) == sub_regions_.end());
seems like these asserts should be debug only

https://codereview.chromium.org/109323011/diff/590002/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/ruleset.h (right):

https://codereview.chromium.org/109323011/diff/590002/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/ruleset.h:53: const Rule* GetRule()
const;
return a const ref

also, why not rule() (i.e. c hacker style capitalization)

[Evan Stade, 2014-01-06 23:33:16]

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

https://codereview.chromium.org/109323011/diff/590002/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/rule.cc:165: static Rule rule;
On 2014/01/06 22:54:19, Evan Stade wrote:
> no static non-pods

In lieu of porting LeakyLazyInstance to libaddressinput, I think you can just
heap-allocate this and leak it on shutdown. Also, you could move this into
GetDefault (not sure why it's a helper fn).

[please use gerrit instead, 2014-01-06 23:44:34]

Evan: PTAL Patch Set 12 and the comments below.

https://codereview.chromium.org/109323011/diff/590002/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/include/libaddressinput/callback.h
(right):

https://codereview.chromium.org/109323011/diff/590002/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/callback.h:1:
// Copyright (C) 2013 Google Inc.
On 2014/01/06 22:54:19, Evan Stade wrote:
> why the changes to this file?

Need to enable passing response data as a scoped_ptr for the case of Ruleset:

void OnRulesetReady(bool success, const std::string& country_code,
scoped_ptr<Ruleset> ruleset);

This requirement forces the callback method to be invoked with "data.Pass()"
instead of simply "data". I did not want to unify on using scoped_ptr
everywhere, because scoped_ptr<std::string> is an overkill.

Also, it appears that all request data types are std::string. Therefore, I've
removed the request data type from the template signature.

https://codereview.chromium.org/109323011/diff/590002/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc
(right):

https://codereview.chromium.org/109323011/diff/590002/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc:254: new
Helper(country_code, rules_ready.Pass(), *retriever_);
On 2014/01/06 22:54:19, Evan Stade wrote:
> this seems dangerous. How do you know Helper won't outlast |this|, and
therefore
> deref |retriever_| after its been freed?

This helper uses the same safety measure as all other Helpers in
libaddressinput: it assumes that retriever_ will respond to all pending requests
with success=false when the retriever_ is destroyed. Upon receiving responses to
all requests, Helper will delete itself and will not dereference |retriever_|
again.

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

https://codereview.chromium.org/109323011/diff/590002/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/rule.cc:165: static Rule rule;
On 2014/01/06 22:54:19, Evan Stade wrote:
> no static non-pods

Done.

https://codereview.chromium.org/109323011/diff/590002/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/rule.cc:165: static Rule rule;
On 2014/01/06 23:33:17, Evan Stade wrote:
> On 2014/01/06 22:54:19, Evan Stade wrote:
> > no static non-pods
> 
> In lieu of porting LeakyLazyInstance to libaddressinput, I think you can just
> heap-allocate this and leak it on shutdown. Also, you could move this into
> GetDefault (not sure why it's a helper fn).

Done.

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

https://codereview.chromium.org/109323011/diff/590002/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/ruleset.cc:51:
assert(sub_regions_.find(sub_region) == sub_regions_.end());
On 2014/01/06 22:54:19, Evan Stade wrote:
> seems like these asserts should be debug only

These asserts do not trigger in Release mode (I've tried), but I haven't been
able to find the code in Chrome that specifies #define NDEBUG in Release mode.

If you want to be extra sure that these asserts do not trigger in Chrome's
release builds, we can add a LIBADDRESSINPUT_ASSERT macro. We can customize this
macro in our own libaddressinput.gyp file.

Which option do you prefer? (leave as-is vs. macro)

https://codereview.chromium.org/109323011/diff/590002/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/ruleset.h (right):

https://codereview.chromium.org/109323011/diff/590002/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/ruleset.h:53: const Rule* GetRule()
const;
On 2014/01/06 22:54:19, Evan Stade wrote:
> return a const ref
> 
> also, why not rule() (i.e. c hacker style capitalization)

Done.

[Evan Stade, 2014-01-07 00:03:14]

https://codereview.chromium.org/109323011/diff/590002/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/include/libaddressinput/callback.h
(right):

https://codereview.chromium.org/109323011/diff/590002/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/callback.h:1:
// Copyright (C) 2013 Google Inc.
On 2014/01/06 23:44:35, Rouslan Solomakhin wrote:
> On 2014/01/06 22:54:19, Evan Stade wrote:
> > why the changes to this file?
> 
> Need to enable passing response data as a scoped_ptr for the case of Ruleset:
> 
> void OnRulesetReady(bool success, const std::string& country_code,
> scoped_ptr<Ruleset> ruleset);
> 
> This requirement forces the callback method to be invoked with "data.Pass()"
> instead of simply "data". I did not want to unify on using scoped_ptr
> everywhere, because scoped_ptr<std::string> is an overkill.
> 
> Also, it appears that all request data types are std::string. Therefore, I've
> removed the request data type from the template signature.

I see. I'm a little nervous about changing param1 to always be a string. Seems
fairly likely someone will want to use an integer at some point.

https://codereview.chromium.org/109323011/diff/590002/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc
(right):

https://codereview.chromium.org/109323011/diff/590002/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc:254: new
Helper(country_code, rules_ready.Pass(), *retriever_);
On 2014/01/06 23:44:35, Rouslan Solomakhin wrote:
> On 2014/01/06 22:54:19, Evan Stade wrote:
> > this seems dangerous. How do you know Helper won't outlast |this|, and
> therefore
> > deref |retriever_| after its been freed?
> 
> This helper uses the same safety measure as all other Helpers in
> libaddressinput: it assumes that retriever_ will respond to all pending
requests
> with success=false when the retriever_ is destroyed. Upon receiving responses
to
> all requests, Helper will delete itself and will not dereference |retriever_|
> again.

Recall how I wanted to make it a rule that you only passed scoped_ptr<Callback>
to classes that you own? I was hoping to shift away from the Helper pattern.
This seems like a good place to start. Could you fold Helper into
CountryRulesRetriever?

[please use gerrit instead, 2014-01-07 01:54:37]

Evan: PTAL Patch Set 15.

https://codereview.chromium.org/109323011/diff/590002/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/include/libaddressinput/callback.h
(right):

https://codereview.chromium.org/109323011/diff/590002/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/callback.h:1:
// Copyright (C) 2013 Google Inc.
On 2014/01/07 00:03:15, Evan Stade wrote:
> On 2014/01/06 23:44:35, Rouslan Solomakhin wrote:
> > On 2014/01/06 22:54:19, Evan Stade wrote:
> > > why the changes to this file?
> > 
> > Need to enable passing response data as a scoped_ptr for the case of
Ruleset:
> > 
> > void OnRulesetReady(bool success, const std::string& country_code,
> > scoped_ptr<Ruleset> ruleset);
> > 
> > This requirement forces the callback method to be invoked with "data.Pass()"
> > instead of simply "data". I did not want to unify on using scoped_ptr
> > everywhere, because scoped_ptr<std::string> is an overkill.
> > 
> > Also, it appears that all request data types are std::string. Therefore,
I've
> > removed the request data type from the template signature.
> 
> I see. I'm a little nervous about changing param1 to always be a string. Seems
> fairly likely someone will want to use an integer at some point.


Brought back the RequestType.

https://codereview.chromium.org/109323011/diff/590002/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc
(right):

https://codereview.chromium.org/109323011/diff/590002/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc:254: new
Helper(country_code, rules_ready.Pass(), *retriever_);
On 2014/01/07 00:03:15, Evan Stade wrote:
> On 2014/01/06 23:44:35, Rouslan Solomakhin wrote:
> > On 2014/01/06 22:54:19, Evan Stade wrote:
> > > this seems dangerous. How do you know Helper won't outlast |this|, and
> > therefore
> > > deref |retriever_| after its been freed?
> > 
> > This helper uses the same safety measure as all other Helpers in
> > libaddressinput: it assumes that retriever_ will respond to all pending
> requests
> > with success=false when the retriever_ is destroyed. Upon receiving
responses
> to
> > all requests, Helper will delete itself and will not dereference
|retriever_|
> > again.
> 
> Recall how I wanted to make it a rule that you only passed
scoped_ptr<Callback>
> to classes that you own? I was hoping to shift away from the Helper pattern.
> This seems like a good place to start. Could you fold Helper into
> CountryRulesRetriever?

Done.

[Evan Stade, 2014-01-07 20:09:58]

https://codereview.chromium.org/109323011/diff/890015/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc
(right):

https://codereview.chromium.org/109323011/diff/890015/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc:152:
rulesets_.push_back(ruleset_data);
why would you need to retrieve rules for more than one country at a time?

https://codereview.chromium.org/109323011/diff/890015/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc:170:
requests_.find(key);
remove request_it now that you're done with it after this fn?

[please use gerrit instead, 2014-01-07 22:11:08]

Evan: PTAL Patch Set 16 and the comment below.

https://codereview.chromium.org/109323011/diff/890015/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc
(right):

https://codereview.chromium.org/109323011/diff/890015/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc:152:
rulesets_.push_back(ruleset_data);
On 2014/01/07 20:09:58, Evan Stade wrote:
> why would you need to retrieve rules for more than one country at a time?

The user might rapidly switch the country combobox in the UI (faster than all
Downloader requests have finished).

https://codereview.chromium.org/109323011/diff/890015/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc:170:
requests_.find(key);
On 2014/01/07 20:09:58, Evan Stade wrote:
> remove request_it now that you're done with it after this fn?

Done.

[Evan Stade, 2014-01-07 22:13:13]

https://codereview.chromium.org/109323011/diff/890015/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc
(right):

https://codereview.chromium.org/109323011/diff/890015/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc:152:
rulesets_.push_back(ruleset_data);
On 2014/01/07 22:11:08, Rouslan Solomakhin wrote:
> On 2014/01/07 20:09:58, Evan Stade wrote:
> > why would you need to retrieve rules for more than one country at a time?
> 
> The user might rapidly switch the country combobox in the UI (faster than all
> Downloader requests have finished).

in which case you could/should probably abandon the old request, no? Like if I
hold the down arrow key and it runs through all the countries, I don't really
want to have it download all rules known to mankind.

[please use gerrit instead, 2014-01-07 22:57:08]

Evan: PTAL Patch Set 17.


I've changed the code to abandon the old requests upon new requests. However,
there's now a possibility of duplicate pending requests to Retriever. For
example, if user switches between "United States"-"Canada"-"United States"
quickly, then it's possible that the abandoned downloads of data for "United
States" from the first request will still be pending when the user selects
"United States" for the second time.

I believe that your new assert that there're no duplicate requests in Retriever
will land soon. We'd have to remove that assert and behave the same way as here:
abandon an old request upon a new request.

https://codereview.chromium.org/109323011/diff/890015/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc
(right):

https://codereview.chromium.org/109323011/diff/890015/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc:152:
rulesets_.push_back(ruleset_data);
On 2014/01/07 22:13:13, Evan Stade wrote:
> On 2014/01/07 22:11:08, Rouslan Solomakhin wrote:
> > On 2014/01/07 20:09:58, Evan Stade wrote:
> > > why would you need to retrieve rules for more than one country at a time?
> > 
> > The user might rapidly switch the country combobox in the UI (faster than
all
> > Downloader requests have finished).
> 
> in which case you could/should probably abandon the old request, no? Like if I
> hold the down arrow key and it runs through all the countries, I don't really
> want to have it download all rules known to mankind.

Done.

[please use gerrit instead, 2014-01-07 23:00:41]

https://codereview.chromium.org/109323011/diff/1110001/third_party/libaddress...
File third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc
(right):

https://codereview.chromium.org/109323011/diff/1110001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc:197:
languages_.clear();
Err, copy-paste error. Fix incoming.

[please use gerrit instead, 2014-01-07 23:03:41]

Patch Set 18.

https://codereview.chromium.org/109323011/diff/1110001/third_party/libaddress...
File third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc
(right):

https://codereview.chromium.org/109323011/diff/1110001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc:197:
languages_.clear();
On 2014/01/07 23:00:42, Rouslan Solomakhin wrote:
> Err, copy-paste error. Fix incoming.

Done.

[Evan Stade, 2014-01-08 01:22:45]

Indeed, my Retriever change already landed.

https://codereview.chromium.org/109323011/diff/1260001/third_party/libaddress...
File third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc
(right):

https://codereview.chromium.org/109323011/diff/1260001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc:114:
success_ &= success;
it seems that if success is false, we should abandon everything and immediately
fail

https://codereview.chromium.org/109323011/diff/1260001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc:194:
success_ = true;
this is surprising.

[please use gerrit instead, 2014-01-08 02:04:39]

Evan: PTAL Patch Set 20. I've also updated Retriever to abandon previous
requests when there's an new incoming request for the same key.

https://codereview.chromium.org/109323011/diff/1260001/third_party/libaddress...
File third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc
(right):

https://codereview.chromium.org/109323011/diff/1260001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc:114:
success_ &= success;
On 2014/01/08 01:22:46, Evan Stade wrote:
> it seems that if success is false, we should abandon everything and
immediately
> fail

Done.

https://codereview.chromium.org/109323011/diff/1260001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc:194:
success_ = true;
On 2014/01/08 01:22:46, Evan Stade wrote:
> this is surprising.

Changed the variable description to "False if at least one request to Retriever
failed."

[lararennie, 2014-01-08 08:29:00]

On 2014/01/07 22:13:13, Evan Stade wrote:
>
https://codereview.chromium.org/109323011/diff/890015/third_party/libaddressi...
> File third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc
> (right):
> 
>
https://codereview.chromium.org/109323011/diff/890015/third_party/libaddressi...
> third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc:152:
> rulesets_.push_back(ruleset_data);
> On 2014/01/07 22:11:08, Rouslan Solomakhin wrote:
> > On 2014/01/07 20:09:58, Evan Stade wrote:
> > > why would you need to retrieve rules for more than one country at a time?
> > 
> > The user might rapidly switch the country combobox in the UI (faster than
all
> > Downloader requests have finished).
> 
> in which case you could/should probably abandon the old request, no? Like if I
> hold the down arrow key and it runs through all the countries, I don't really
> want to have it download all rules known to mankind.

Just FYI: We had it set up so that it didn't try and get rules until the value
had been selected; so holding down the down key wouldn't actually trigger 10
million look-ups. We didn't abandon old requests, because if someone is flicking
between say two countries, then we thought it possible that they want to go back
to the first one again, and if we have rules in the backend now downloaded it
improves the latency. And there was no harm in having a few more rules cached.

[Evan Stade, 2014-01-08 20:22:50]

On 2014/01/08 08:29:00, lararennie wrote:
> On 2014/01/07 22:13:13, Evan Stade wrote:
> >
>
https://codereview.chromium.org/109323011/diff/890015/third_party/libaddressi...
> > File third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/109323011/diff/890015/third_party/libaddressi...
> > third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc:152:
> > rulesets_.push_back(ruleset_data);
> > On 2014/01/07 22:11:08, Rouslan Solomakhin wrote:
> > > On 2014/01/07 20:09:58, Evan Stade wrote:
> > > > why would you need to retrieve rules for more than one country at a
time?
> > > 
> > > The user might rapidly switch the country combobox in the UI (faster than
> all
> > > Downloader requests have finished).
> > 
> > in which case you could/should probably abandon the old request, no? Like if
I
> > hold the down arrow key and it runs through all the countries, I don't
really
> > want to have it download all rules known to mankind.
> 
> Just FYI: We had it set up so that it didn't try and get rules until the value
> had been selected; so holding down the down key wouldn't actually trigger 10
> million look-ups.

If you don't have the dropdown opened, but just highlighted, then holding the
down key does actually change the selection.

> We didn't abandon old requests, because if someone is flicking
> between say two countries, then we thought it possible that they want to go
back
> to the first one again, and if we have rules in the backend now downloaded it
> improves the latency. And there was no harm in having a few more rules cached.

If a download has completed it will be stored in the Storage object, so rules
will still be cached under the right circumstances.

[Evan Stade, 2014-01-08 21:08:59]

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
File third_party/libaddressinput/chromium/cpp/src/address_validator.cc (right):

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/address_validator.cc:50:
scoped_ptr<Storage>(storage.Pass())))),
I'm not sure but I don't think you need the scoped_ptr<Storage>() here (might be
my fault for adding it)

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/address_validator.cc:56:
STLDeleteContainerPairSecondPointers(rules_.begin(), rules_.end());
STLDeleteValues

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/address_validator.cc:65:
status_.insert(std::make_pair(country_code, RULES_NOT_READY));
do you really need this to be a map? It seems like you could make it a little
simpler by checking if it exists already in rules_ (that handles the SUCCESS
case) and keeping around a std::set of outstanding requests (that handles
RULES_NOT_READY)

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/address_validator.cc:78: return
RULES_UNAVAILABLE;
add TODO to implement (so there's no confusion about whether this is a temporary
stub or permanent)

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/address_validator.cc:110: // A
mapping of a country code to all rules loaded for that country code.
Ruleset ownership comment plx

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
File third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc
(right):

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc:53: //
root ruleset at the COUNTRY level. The language-specific and sub-region
add an assert for this in the constructor?

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc:116:
(*rules_ready_)(success_, country_code_, root_.Pass());
why do you pass back partial data?

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc:127:
success_ &= rule->ParseSerializedRule(data);
I don't get why you do both this and CopyFrom above in the COUNTRY case.

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc:157:
lang_it = languages_.begin();
4 more indent

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
File third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.h
(right):

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.h:56: class
CountryRulesRetriever {
please call this something else because this name implies it implements the
Retriever interface.

Suggested name: CountryRulesAggregator

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.h:104: bool
success_;
you don't need this variable any more.

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
File third_party/libaddressinput/chromium/cpp/src/rule.cc (right):

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/rule.cc:37: static Rule*
default_rule = NULL;
why not a local static?

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
File third_party/libaddressinput/chromium/cpp/src/ruleset.cc (right):

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/ruleset.cc:39:
STLDeleteContainerPairSecondPointers(
STLDeleteValues

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/ruleset.cc:49:
sub_regions_.insert(std::make_pair(sub_region, sub_region_rules));
Any reason you can't use the easier-to-read version:

  sub_regions_[sub_region] = sub_region_rules

(below as well)

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/ruleset.cc:59: Ruleset*
Ruleset::GetSubRegion(const std::string& sub_region) const {
this is not used anywhere --- please add it when it's used and not before.

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/ruleset.cc:65: const Rule*
Ruleset::GetLanguageCode(const std::string& language_code) const {
this is not used anywhere --- please add it when it's used and not before.

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
File third_party/libaddressinput/chromium/cpp/src/ruleset.h (right):

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/ruleset.h:63: Ruleset*
GetSubRegion(const std::string& sub_region) const;
imo, call these fns Add/GetSubRegionRuleset, Add/GetLanguageCodeRule

[please use gerrit instead, 2014-01-09 00:38:10]

Evan: PTAL Patch Set 24.

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
File third_party/libaddressinput/chromium/cpp/src/address_validator.cc (right):

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/address_validator.cc:50:
scoped_ptr<Storage>(storage.Pass())))),
On 2014/01/08 21:08:59, Evan Stade wrote:
> I'm not sure but I don't think you need the scoped_ptr<Storage>() here (might
be
> my fault for adding it)

Done.

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/address_validator.cc:56:
STLDeleteContainerPairSecondPointers(rules_.begin(), rules_.end());
On 2014/01/08 21:08:59, Evan Stade wrote:
> STLDeleteValues

Done.

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/address_validator.cc:65:
status_.insert(std::make_pair(country_code, RULES_NOT_READY));
On 2014/01/08 21:08:59, Evan Stade wrote:
> do you really need this to be a map? It seems like you could make it a little
> simpler by checking if it exists already in rules_ (that handles the SUCCESS
> case) and keeping around a std::set of outstanding requests (that handles
> RULES_NOT_READY)

Done.

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/address_validator.cc:78: return
RULES_UNAVAILABLE;
On 2014/01/08 21:08:59, Evan Stade wrote:
> add TODO to implement (so there's no confusion about whether this is a
temporary
> stub or permanent)

Done.

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/address_validator.cc:110: // A
mapping of a country code to all rules loaded for that country code.
On 2014/01/08 21:08:59, Evan Stade wrote:
> Ruleset ownership comment plx

Done.

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
File third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc
(right):

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc:53: //
root ruleset at the COUNTRY level. The language-specific and sub-region
On 2014/01/08 21:08:59, Evan Stade wrote:
> add an assert for this in the constructor?

Done.

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc:116:
(*rules_ready_)(success_, country_code_, root_.Pass());
On 2014/01/08 21:08:59, Evan Stade wrote:
> why do you pass back partial data? 

Removed.

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc:127:
success_ &= rule->ParseSerializedRule(data);
On 2014/01/08 21:08:59, Evan Stade wrote:
> I don't get why you do both this and CopyFrom above in the COUNTRY case.

If a country-level rule is missing some information, like required fields, for
example, then the default information is used. I accomplish this by starting
with the default rule:

rule->CopyFrom(Rule::GetDefault());

Then I parse the serialized data into the same rule:

rule->ParseSerializedRule(data);

If the |data| does not specify the required fields, then rule will continue
using the information from Rule::GetDefault().

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.cc:157:
lang_it = languages_.begin();
On 2014/01/08 21:08:59, Evan Stade wrote:
> 4 more indent

Done.

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
File third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.h
(right):

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.h:56: class
CountryRulesRetriever {
On 2014/01/08 21:08:59, Evan Stade wrote:
> please call this something else because this name implies it implements the
> Retriever interface.
> 
> Suggested name: CountryRulesAggregator

Done.

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/country_rules_retriever.h:104: bool
success_;
On 2014/01/08 21:08:59, Evan Stade wrote:
> you don't need this variable any more.

Done.

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
File third_party/libaddressinput/chromium/cpp/src/rule.cc (right):

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/rule.cc:37: static Rule*
default_rule = NULL;
On 2014/01/08 21:08:59, Evan Stade wrote:
> why not a local static?

Done.

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
File third_party/libaddressinput/chromium/cpp/src/ruleset.cc (right):

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/ruleset.cc:39:
STLDeleteContainerPairSecondPointers(
On 2014/01/08 21:08:59, Evan Stade wrote:
> STLDeleteValues

Done.

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/ruleset.cc:49:
sub_regions_.insert(std::make_pair(sub_region, sub_region_rules));
On 2014/01/08 21:08:59, Evan Stade wrote:
> Any reason you can't use the easier-to-read version:
> 
>   sub_regions_[sub_region] = sub_region_rules
> 
> (below as well)

No reason AFAIK. Done.

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/ruleset.cc:59: Ruleset*
Ruleset::GetSubRegion(const std::string& sub_region) const {
On 2014/01/08 21:08:59, Evan Stade wrote:
> this is not used anywhere --- please add it when it's used and not before.

Done.

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/ruleset.cc:65: const Rule*
Ruleset::GetLanguageCode(const std::string& language_code) const {
On 2014/01/08 21:08:59, Evan Stade wrote:
> this is not used anywhere --- please add it when it's used and not before.

Done.

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
File third_party/libaddressinput/chromium/cpp/src/ruleset.h (right):

https://codereview.chromium.org/109323011/diff/1470001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/ruleset.h:63: Ruleset*
GetSubRegion(const std::string& sub_region) const;
On 2014/01/08 21:08:59, Evan Stade wrote:
> imo, call these fns Add/GetSubRegionRuleset, Add/GetLanguageCodeRule

Done.

[Evan Stade, 2014-01-09 01:01:39]

lgtm with nits

https://codereview.chromium.org/109323011/diff/1640001/third_party/libaddress...
File third_party/libaddressinput/chromium/cpp/src/address_validator.cc (right):

https://codereview.chromium.org/109323011/diff/1640001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/address_validator.cc:48: :
aggregator_(scoped_ptr<Retriever>(new Retriever(
I'm thinking the storage validation can go into Retriever now. Do you agree?
Could you add a TODO into whichever existing class you think it belongs in?

https://codereview.chromium.org/109323011/diff/1640001/third_party/libaddress...
File third_party/libaddressinput/chromium/cpp/src/ruleset.h (right):

https://codereview.chromium.org/109323011/diff/1640001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/ruleset.h:57: scoped_ptr<Rule>
rule);
hmm, I think the second param should be a Ruleset. Otherwise this is sort of an
ad-hoc implicit construction.

Also that allows you to make the return type void, which aligns with
AddLanguageCodeRule(). At the callsite this will be slightly more verbose, but
in the implementation it will be slightly less verbose.

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

Addressed the nits and sent to cq.

https://codereview.chromium.org/109323011/diff/1640001/third_party/libaddress...
File third_party/libaddressinput/chromium/cpp/src/address_validator.cc (right):

https://codereview.chromium.org/109323011/diff/1640001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/address_validator.cc:48: :
aggregator_(scoped_ptr<Retriever>(new Retriever(
On 2014/01/09 01:01:39, Evan Stade wrote:
> I'm thinking the storage validation can go into Retriever now. Do you agree?

Yes, the Retriever is the correct place for validation.

> Could you add a TODO into whichever existing class you think it belongs in?

Added a TODO to retriever.cc.

https://codereview.chromium.org/109323011/diff/1640001/third_party/libaddress...
File third_party/libaddressinput/chromium/cpp/src/ruleset.h (right):

https://codereview.chromium.org/109323011/diff/1640001/third_party/libaddress...
third_party/libaddressinput/chromium/cpp/src/ruleset.h:57: scoped_ptr<Rule>
rule);
On 2014/01/09 01:01:39, Evan Stade wrote:
> hmm, I think the second param should be a Ruleset. Otherwise this is sort of
an
> ad-hoc implicit construction.
> 
> Also that allows you to make the return type void, which aligns with
> AddLanguageCodeRule(). At the callsite this will be slightly more verbose, but
> in the implementation it will be slightly less verbose.

Done.

[commit-bot: I haz the power, 2014-01-09 01:27:58]

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

[commit-bot: I haz the power, 2014-01-09 04:05:11]

Change committed as 243754