Enable aggregate data in fake downloader.

Enable aggregate data in fake downloader.

This patch changes fake data URL from test:/// to test:///plain/ and
adds a URL for fake aggregate data: test:///aggregate/.

Example of data at test:///plain/data/ZW:

{"name": "ZIMBABWE", "key": "ZW", "id": "data/ZW"}

Example of aggregate data at test:///aggregate/data/ZW:

{"data/ZW": {"name": "ZIMBABWE", "key": "ZW", "id": "data/ZW"}}

This behavior mimics the behavior of the server URLs
https://i18napis.appspot.com/ssl-address/data/ and
https://i18napis.appspot.com/ssl-aggregate-address/data/.

R=lararennie@google.com

Committed: https://code.google.com/p/libaddressinput/source/detail?r=231

[Rouslan Solomakhin, 2014-05-20 16:11:42]

Fredrik: PTAL Patch Set 1.

[Rouslan Solomakhin, 2014-05-21 12:00:46]

Lara: Maybe you would be a more suitable reviewer for this patch.

[lararennie, 2014-05-21 13:00:46]

https://codereview.appspot.com/95560045/diff/1/cpp/test/fake_downloader.cc
File cpp/test/fake_downloader.cc (right):

https://codereview.appspot.com/95560045/diff/1/cpp/test/fake_downloader.cc#ne...
cpp/test/fake_downloader.cc:53: // The number of characters in a an aggreagete
data key.
a an -> an
aggregate

https://codereview.appspot.com/95560045/diff/1/cpp/test/fake_downloader.cc#ne...
cpp/test/fake_downloader.cc:54: const size_t kAggregateDataKeyLength =
kDataKeyPrefixLength + 2;
This +2 is a little magic to me.

https://codereview.appspot.com/95560045/diff/1/cpp/test/fake_downloader.cc#ne...
cpp/test/fake_downloader.cc:81: if (key.compare(0,
Is this just trying to do:
if (key.substr(0, kDataKeyPrefixLength) == kDataKeyPrefix) ?

Or:
if (key.find(kDataKeyPrefix) == 0)

the latter being slower I guess...

?

If so - it looks a little unreadable at the moment, I think using something else
would be preferable?

(and if not, then it needs simplifying or a comment, since I clearly
misunderstood what it does :) )

https://codereview.appspot.com/95560045/diff/1/cpp/test/fake_downloader_test.cc
File cpp/test/fake_downloader_test.cc (right):

https://codereview.appspot.com/95560045/diff/1/cpp/test/fake_downloader_test....
cpp/test/fake_downloader_test.cc:102: const std::string& key) {
indentation

https://codereview.appspot.com/95560045/diff/1/cpp/test/fake_downloader_test....
cpp/test/fake_downloader_test.cc:115: if (data.compare(data.length() -
kDataEndLength,
Consider simplifying this one too.

https://codereview.appspot.com/95560045/diff/1/cpp/test/fake_downloader_test....
cpp/test/fake_downloader_test.cc:126: // Verifies that FakeDownloader downloads
valid aggregated data for a region
Might be nice to have a test for an invalid data key?

[roubert (google), 2014-05-21 13:36:00]

https://codereview.appspot.com/95560045/diff/1/cpp/test/fake_downloader.cc
File cpp/test/fake_downloader.cc (right):

https://codereview.appspot.com/95560045/diff/1/cpp/test/fake_downloader.cc#ne...
cpp/test/fake_downloader.cc:29: const char FakeDownloader::kFakeDataUrl[] =
"test://address/";
This is not actually a URL. The part after the first :// in a URL is the
hostname, so a URL without any hostname but with a path would be
"test:///address/".

You might also want to choose some short and informative paths, like for
example:

test:///plain/
test:///aggregate/

[Rouslan Solomakhin, 2014-05-21 14:08:43]

Addressed all comments. Will commit now without a second round of comments to
unblock anyone that needs to test downloading aggregate data.

https://codereview.appspot.com/95560045/diff/1/cpp/test/fake_downloader.cc
File cpp/test/fake_downloader.cc (right):

https://codereview.appspot.com/95560045/diff/1/cpp/test/fake_downloader.cc#ne...
cpp/test/fake_downloader.cc:29: const char FakeDownloader::kFakeDataUrl[] =
"test://address/";
On 2014/05/21 13:36:00, roubert wrote:
> This is not actually a URL. The part after the first :// in a URL is the
> hostname, so a URL without any hostname but with a path would be
> "test:///address/".
> 
> You might also want to choose some short and informative paths, like for
> example:
> 
> test:///plain/
> test:///aggregate/

Done.

https://codereview.appspot.com/95560045/diff/1/cpp/test/fake_downloader.cc#ne...
cpp/test/fake_downloader.cc:53: // The number of characters in a an aggreagete
data key.
On 2014/05/21 13:00:46, lararennie wrote:
> a an -> an
> aggregate

Done.

https://codereview.appspot.com/95560045/diff/1/cpp/test/fake_downloader.cc#ne...
cpp/test/fake_downloader.cc:54: const size_t kAggregateDataKeyLength =
kDataKeyPrefixLength + 2;
On 2014/05/21 13:00:46, lararennie wrote:
> This +2 is a little magic to me.

Clarified.

https://codereview.appspot.com/95560045/diff/1/cpp/test/fake_downloader.cc#ne...
cpp/test/fake_downloader.cc:81: if (key.compare(0,
On 2014/05/21 13:00:46, lararennie wrote:
> Is this just trying to do:
> if (key.substr(0, kDataKeyPrefixLength) == kDataKeyPrefix) ?

That's it! :-)

The code did this originally, but we replaced it with the version that did not
need to calculate string length to improve performance...

I've added a clarifying comment.

https://codereview.appspot.com/95560045/diff/1/cpp/test/fake_downloader_test.cc
File cpp/test/fake_downloader_test.cc (right):

https://codereview.appspot.com/95560045/diff/1/cpp/test/fake_downloader_test....
cpp/test/fake_downloader_test.cc:102: const std::string& key) {
On 2014/05/21 13:00:46, lararennie wrote:
> indentation

Done.

https://codereview.appspot.com/95560045/diff/1/cpp/test/fake_downloader_test....
cpp/test/fake_downloader_test.cc:115: if (data.compare(data.length() -
kDataEndLength,
On 2014/05/21 13:00:46, lararennie wrote:
> Consider simplifying this one too.

Clarifying comment.

https://codereview.appspot.com/95560045/diff/1/cpp/test/fake_downloader_test....
cpp/test/fake_downloader_test.cc:126: // Verifies that FakeDownloader downloads
valid aggregated data for a region
On 2014/05/21 13:00:46, lararennie wrote:
> Might be nice to have a test for an invalid data key?

Done.

[Rouslan Solomakhin, 2014-05-21 14:11:44]

Actually, not committing until I get an el-gee, as Fredrik recommends.

[Rouslan Solomakhin, 2014-05-21 14:11:53]

Lara: PTAL Patch Set 2.

[roubert (google), 2014-05-21 14:12:49]

On 2014/05/21 14:11:44, Rouslan Solomakhin wrote:
> Actually, not committing until I get an el-gee, as Fredrik recommends.

Yes, if someone writes "it looks a little unreadable at the moment", then I
think it's a very good idea to not submit until someone decides that it is
readable after all.

[lararennie, 2014-05-21 14:46:47]

LGTM

https://codereview.appspot.com/95560045/diff/1/cpp/test/fake_downloader.cc
File cpp/test/fake_downloader.cc (right):

https://codereview.appspot.com/95560045/diff/1/cpp/test/fake_downloader.cc#ne...
cpp/test/fake_downloader.cc:81: if (key.compare(0,
On 2014/05/21 14:08:42, Rouslan Solomakhin wrote:
> On 2014/05/21 13:00:46, lararennie wrote:
> > Is this just trying to do:
> > if (key.substr(0, kDataKeyPrefixLength) == kDataKeyPrefix) ?
> 
> That's it! :-)
> 
> The code did this originally, but we replaced it with the version that did not
> need to calculate string length to improve performance...
> 
> I've added a clarifying comment.

Ah, because some strings are very big. Consider in future making it a small
private method in the anon namespace to leave the code here as readable as
possible.

(I also didn't know that for substr it had to calculate length, is that to
ensure it doesn't go over bounds?)

https://codereview.appspot.com/95560045/diff/20001/cpp/test/fake_downloader.cc
File cpp/test/fake_downloader.cc (right):

https://codereview.appspot.com/95560045/diff/20001/cpp/test/fake_downloader.c...
cpp/test/fake_downloader.cc:43: // 'test:///aggregate/'.
I don't think repeating the URL is a good idea, the reason we have the constant
is to make it easy to change it :) Same for the previous comment.

https://codereview.appspot.com/95560045/diff/20001/cpp/test/fake_downloader.c...
cpp/test/fake_downloader.cc:56: // The number of characters in an aggreagete
data key, e.g. 'data/CH'.
Not aggreagete, aggregate :)

[Rouslan Solomakhin, 2014-05-21 15:04:28]

Will commit.

https://codereview.appspot.com/95560045/diff/1/cpp/test/fake_downloader.cc
File cpp/test/fake_downloader.cc (right):

https://codereview.appspot.com/95560045/diff/1/cpp/test/fake_downloader.cc#ne...
cpp/test/fake_downloader.cc:81: if (key.compare(0,
On 2014/05/21 14:46:47, lararennie wrote:
> On 2014/05/21 14:08:42, Rouslan Solomakhin wrote:
> > On 2014/05/21 13:00:46, lararennie wrote:
> > > Is this just trying to do:
> > > if (key.substr(0, kDataKeyPrefixLength) == kDataKeyPrefix) ?
> > 
> > That's it! :-)
> > 
> > The code did this originally, but we replaced it with the version that did
not
> > need to calculate string length to improve performance...
> > 
> > I've added a clarifying comment.
> 
> Ah, because some strings are very big. Consider in future making it a small
> private method in the anon namespace to leave the code here as readable as
> possible.
> 
> (I also didn't know that for substr it had to calculate length, is that to
> ensure it doesn't go over bounds?)

It's the implicit conversion from char* kDataKeyPrefix to an std::string (for
equality check with another std::string) that calculates string length.

https://codereview.appspot.com/95560045/diff/20001/cpp/test/fake_downloader.cc
File cpp/test/fake_downloader.cc (right):

https://codereview.appspot.com/95560045/diff/20001/cpp/test/fake_downloader.c...
cpp/test/fake_downloader.cc:43: // 'test:///aggregate/'.
On 2014/05/21 14:46:47, lararennie wrote:
> I don't think repeating the URL is a good idea, the reason we have the
constant
> is to make it easy to change it :) Same for the previous comment.

Removed.

https://codereview.appspot.com/95560045/diff/20001/cpp/test/fake_downloader.c...
cpp/test/fake_downloader.cc:56: // The number of characters in an aggreagete
data key, e.g. 'data/CH'.
On 2014/05/21 14:46:47, lararennie wrote:
> Not aggreagete, aggregate :)

Done.

[Rouslan Solomakhin, 2014-05-21 15:04:56]

Committed patchset #3 manually as r231 (presubmit successful).