[rAc - libaddressinput] slay a Helper class.

[rAc - libaddressinput] slay a Helper class.

For better memory ownership.

BUG=none

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

[Evan Stade, 2014-01-07 01:31:15]

question before you start the review. What do you think should happen if the
retriever is asked to retrieve the same thing twice in a row? Should it (a)
reject the second request (b) immediately fail the first callback, replace with
new callback (c) respect both requests (i.e. the map needs to be a multimap)?

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

On 2014/01/07 01:31:15, Evan Stade wrote:
> question before you start the review. What do you think should happen if the
> retriever is asked to retrieve the same thing twice in a row? Should it (a)
> reject the second request (b) immediately fail the first callback, replace
with
> new callback (c) respect both requests (i.e. the map needs to be a multimap)?

(a) reject, because AddressValidator::LoadRules() will return RULES_NOT_READY
for pending downloads. Therefore, all pending requests to Retriever will be
unique.

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

On 2014/01/07 01:38:08, Rouslan Solomakhin wrote:
> On 2014/01/07 01:31:15, Evan Stade wrote:
> > question before you start the review. What do you think should happen if the
> > retriever is asked to retrieve the same thing twice in a row? Should it (a)
> > reject the second request (b) immediately fail the first callback, replace
> with
> > new callback (c) respect both requests (i.e. the map needs to be a
multimap)?
> 
> (a) reject, because AddressValidator::LoadRules() will return RULES_NOT_READY
> for pending downloads. Therefore, all pending requests to Retriever will be
> unique.

great, that's the easiest choice.

[Evan Stade, 2014-01-07 02:06:44]

updated.

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

https://codereview.chromium.org/115523011/diff/110003/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/retriever.cc (right):

https://codereview.chromium.org/115523011/diff/110003/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/retriever.cc:51:
(*retrieved)(false, key, std::string());
I recommend using (requests_.find(key) != requests_.end()). The find() method
has the benefit of stopping the search after finding the first element. If you
ever change |requests_| to be a data structure that can contain duplicates (e.g.
a vector a multimap), then count() would be checking all elements every time.

nit:

I think assert(requests_.find(key) == requests_.end()); would work better here,
because this condition should never happen. If we let it happen, I foresee weird
bugs where some addresses do not get validated due to user clicking the "submit"
button twice, for example.

https://codereview.chromium.org/115523011/diff/110003/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/retriever.cc:55:
requests_.insert(std::make_pair(key, retrieved.release()));
nit:

#include <utility> 

(std::make_pair is in the <utility> header.)

[Evan Stade, 2014-01-07 20:23:32]

https://codereview.chromium.org/115523011/diff/110003/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/retriever.cc (right):

https://codereview.chromium.org/115523011/diff/110003/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/retriever.cc:51:
(*retrieved)(false, key, std::string());
On 2014/01/07 02:37:28, Rouslan Solomakhin wrote:
> I recommend using (requests_.find(key) != requests_.end()). The find() method
> has the benefit of stopping the search after finding the first element. If you
> ever change |requests_| to be a data structure that can contain duplicates
(e.g.
> a vector a multimap), then count() would be checking all elements every time.
> 
> nit:
> 
> I think assert(requests_.find(key) == requests_.end()); would work better
here,
> because this condition should never happen. If we let it happen, I foresee
weird
> bugs where some addresses do not get validated due to user clicking the
"submit"
> button twice, for example.

Done.

https://codereview.chromium.org/115523011/diff/110003/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/retriever.cc:55:
requests_.insert(std::make_pair(key, retrieved.release()));
On 2014/01/07 02:37:28, Rouslan Solomakhin wrote:
> nit:
> 
> #include <utility> 
> 
> (std::make_pair is in the <utility> header.)

Done.

[please use gerrit instead, 2014-01-07 22:04:47]

lgtm

[commit-bot: I haz the power, 2014-01-07 22:19:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/115523011/170001

[commit-bot: I haz the power, 2014-01-08 00:10:29]

Change committed as 243440