[rac] Download country code data in a single HTTP request.

[rac] Download country code data in a single HTTP request.

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

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

Evan: PTAL.

[Evan Stade, 2014-01-18 19:14:31]

https://codereview.chromium.org/140823005/diff/310001/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/country_rules_aggregator.cc
(right):

https://codereview.chromium.org/140823005/diff/310001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/country_rules_aggregator.cc:68: for
(std::vector<std::string>::const_iterator lang_it = languages_.begin();
please add short comments to each of these loops

https://codereview.chromium.org/140823005/diff/310001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/country_rules_aggregator.cc:77: if
(field == DEPENDENT_LOCALITY) {
why is this necessary? Wouldn't the dependent locality already not have subkeys?

https://codereview.chromium.org/140823005/diff/310001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/country_rules_aggregator.cc:100:
json_->GetJsonValueForKey(key, &value);
why did you make GetJsonValueForKey return a bool if you don't need it?

https://codereview.chromium.org/140823005/diff/310001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/country_rules_aggregator.cc:117:
std::vector<std::string> languages_;
nit: non_default_languages_? other_languages_?

https://codereview.chromium.org/140823005/diff/310001/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/util/json.h (right):

https://codereview.chromium.org/140823005/diff/310001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/util/json.h:55: virtual bool
GetJsonValueForKey(const std::string& key,
return a scoped_ptr<Json>

or if you want to stick to the outparam pattern, pass in a scoped_ptr<Json>*

[please use gerrit instead, 2014-01-21 18:43:53]

Evan: PTAL Patch Set 2.

https://codereview.chromium.org/140823005/diff/310001/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/country_rules_aggregator.cc
(right):

https://codereview.chromium.org/140823005/diff/310001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/country_rules_aggregator.cc:68: for
(std::vector<std::string>::const_iterator lang_it = languages_.begin();
On 2014/01/18 19:14:32, Evan Stade wrote:
> please add short comments to each of these loops

Done.

https://codereview.chromium.org/140823005/diff/310001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/country_rules_aggregator.cc:77: if
(field == DEPENDENT_LOCALITY) {
On 2014/01/18 19:14:32, Evan Stade wrote:
> why is this necessary? Wouldn't the dependent locality already not have
subkeys?

True. I've changed this statement to an assert(), which would be triggered only
in Debug builds.

https://codereview.chromium.org/140823005/diff/310001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/country_rules_aggregator.cc:100:
json_->GetJsonValueForKey(key, &value);
On 2014/01/18 19:14:32, Evan Stade wrote:
> why did you make GetJsonValueForKey return a bool if you don't need it?

Using the bool now.

https://codereview.chromium.org/140823005/diff/310001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/country_rules_aggregator.cc:117:
std::vector<std::string> languages_;
On 2014/01/18 19:14:32, Evan Stade wrote:
> nit: non_default_languages_? other_languages_?

non_default_languages_.

https://codereview.chromium.org/140823005/diff/310001/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/util/json.h (right):

https://codereview.chromium.org/140823005/diff/310001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/util/json.h:55: virtual bool
GetJsonValueForKey(const std::string& key,
On 2014/01/18 19:14:32, Evan Stade wrote:
> return a scoped_ptr<Json>
> 
> or if you want to stick to the outparam pattern, pass in a scoped_ptr<Json>*

Great idea! Done.

[Evan Stade, 2014-01-22 01:22:03]

lgtm

https://codereview.chromium.org/140823005/diff/460001/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/country_rules_aggregator.cc
(right):

https://codereview.chromium.org/140823005/diff/460001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/country_rules_aggregator.cc:116:
assert(field != DEPENDENT_LOCALITY || ruleset->rule().GetSubKeys().empty());
even this seems out of place to me; I might expect it somewhere in the Ruleset
class but not here. Also, here you assert, but in other places you (silently)
return early after detecting bad data. That doesn't seem consistent.

https://codereview.chromium.org/140823005/diff/460001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/country_rules_aggregator.cc:122:
subkey_it != ruleset->rule().GetSubKeys().end(); ++subkey_it) {
indent is off by one

[please use gerrit instead, 2014-01-22 01:46:21]

Will commit.

https://codereview.chromium.org/140823005/diff/460001/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/country_rules_aggregator.cc
(right):

https://codereview.chromium.org/140823005/diff/460001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/country_rules_aggregator.cc:116:
assert(field != DEPENDENT_LOCALITY || ruleset->rule().GetSubKeys().empty());
On 2014/01/22 01:22:03, Evan Stade wrote:
> even this seems out of place to me; I might expect it somewhere in the Ruleset
> class but not here. Also, here you assert, but in other places you (silently)
> return early after detecting bad data. That doesn't seem consistent.

Done.

https://codereview.chromium.org/140823005/diff/460001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/country_rules_aggregator.cc:122:
subkey_it != ruleset->rule().GetSubKeys().end(); ++subkey_it) {
On 2014/01/22 01:22:03, Evan Stade wrote:
> indent is off by one

Done.

[commit-bot: I haz the power, 2014-01-22 01:55:39]

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

[commit-bot: I haz the power, 2014-01-22 05:03:25]

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

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

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

[commit-bot: I haz the power, 2014-01-22 21:30:05]

Change committed as 246414