[rac] Use stale libaddressinput data if download fails

[rac] Use stale libaddressinput data if download fails

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

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

Evan: PTAL.

[Evan Stade, 2014-01-22 01:42:12]

https://codereview.chromium.org/144353002/diff/50001/third_party/libaddressin...
File third_party/libaddressinput/chromium/cpp/src/validating_util.h (right):

https://codereview.chromium.org/144353002/diff/50001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/validating_util.h:58: static
TimestampStatus UnwrapTimestamp(std::string* data, time_t now);
I think that UnwrapTimestamp and UnwrapChecksum should be a single function:

static bool UnwrapHeader(const std::string& header, time_t* timestamp);

that returns false if the timestamp is missing or invalid or if the checksum
doesn't validate or is missing or anything, but otherwise returns true and fills
out timestamp. The Retriever can contain the logic of "how old is too old".

[please use gerrit instead, 2014-01-22 23:57:41]

Evan: PTAL Patch Set 2.

https://codereview.chromium.org/144353002/diff/50001/third_party/libaddressin...
File third_party/libaddressinput/chromium/cpp/src/validating_util.h (right):

https://codereview.chromium.org/144353002/diff/50001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/validating_util.h:58: static
TimestampStatus UnwrapTimestamp(std::string* data, time_t now);
On 2014/01/22 01:42:12, Evan Stade wrote:
> I think that UnwrapTimestamp and UnwrapChecksum should be a single function:
> 
> static bool UnwrapHeader(const std::string& header, time_t* timestamp);
> 
> that returns false if the timestamp is missing or invalid or if the checksum
> doesn't validate or is missing or anything, but otherwise returns true and
fills
> out timestamp. The Retriever can contain the logic of "how old is too old".

Refactored into:

std::string Wrapper::Wrap(const std::string& data);
bool Wrapper::Unwrap(std::string* data, double* age_in_seconds);

I think that's an improvement on what you suggested, because it hides the time_t
inside of the Wrapper implementation. The logic of "how old is too old" in
Retriever is simple now: compare age_in_seconds to the maximum allowed age.

[Evan Stade, 2014-01-23 00:07:23]

https://codereview.chromium.org/144353002/diff/50001/third_party/libaddressin...
File third_party/libaddressinput/chromium/cpp/src/validating_util.h (right):

https://codereview.chromium.org/144353002/diff/50001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/validating_util.h:58: static
TimestampStatus UnwrapTimestamp(std::string* data, time_t now);
On 2014/01/22 23:57:41, Rouslan Solomakhin wrote:
> On 2014/01/22 01:42:12, Evan Stade wrote:
> > I think that UnwrapTimestamp and UnwrapChecksum should be a single function:
> > 
> > static bool UnwrapHeader(const std::string& header, time_t* timestamp);
> > 
> > that returns false if the timestamp is missing or invalid or if the checksum
> > doesn't validate or is missing or anything, but otherwise returns true and
> fills
> > out timestamp. The Retriever can contain the logic of "how old is too old".
> 
> Refactored into:
> 
> std::string Wrapper::Wrap(const std::string& data);
> bool Wrapper::Unwrap(std::string* data, double* age_in_seconds);
> 
> I think that's an improvement on what you suggested, because it hides the
time_t
> inside of the Wrapper implementation. The logic of "how old is too old" in
> Retriever is simple now: compare age_in_seconds to the maximum allowed age.

why is double a better way of representing time than time_t?

https://codereview.chromium.org/144353002/diff/240002/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/wrapper.h (right):

https://codereview.chromium.org/144353002/diff/240002/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/wrapper.h:37: class Wrapper {
Wrapper is far too generic a name to be useful. You might as well call it class
Foo.

Why not put this in an anonymous namespace in retriever.cc[1]? If the only
answer is "testing", I think you could still test it effectively, and the tests
you wrote would even be superior in that they'd be testing behavior rather than
implementation.

[1]I know that before I said this wasn't necessary, but being unable to come up
with a decent name for the class is perhaps an indication it's not generically
useful, so should be folded into Retriever.

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

https://codereview.chromium.org/144353002/diff/50001/third_party/libaddressin...
File third_party/libaddressinput/chromium/cpp/src/validating_util.h (right):

https://codereview.chromium.org/144353002/diff/50001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/src/validating_util.h:58: static
TimestampStatus UnwrapTimestamp(std::string* data, time_t now);
On 2014/01/23 00:07:23, Evan Stade wrote:
> On 2014/01/22 23:57:41, Rouslan Solomakhin wrote:
> > On 2014/01/22 01:42:12, Evan Stade wrote:
> > > I think that UnwrapTimestamp and UnwrapChecksum should be a single
function:
> > > 
> > > static bool UnwrapHeader(const std::string& header, time_t* timestamp);
> > > 
> > > that returns false if the timestamp is missing or invalid or if the
checksum
> > > doesn't validate or is missing or anything, but otherwise returns true and
> > fills
> > > out timestamp. The Retriever can contain the logic of "how old is too
old".
> > 
> > Refactored into:
> > 
> > std::string Wrapper::Wrap(const std::string& data);
> > bool Wrapper::Unwrap(std::string* data, double* age_in_seconds);
> > 
> > I think that's an improvement on what you suggested, because it hides the
> time_t
> > inside of the Wrapper implementation. The logic of "how old is too old" in
> > Retriever is simple now: compare age_in_seconds to the maximum allowed age.
> 
> why is double a better way of representing time than time_t?

The difference between two time_t objects is always a double:

http://www.cplusplus.com/reference/ctime/difftime/

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

Evan: PTAL Patch Set 3.

https://codereview.chromium.org/144353002/diff/240002/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/wrapper.h (right):

https://codereview.chromium.org/144353002/diff/240002/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/wrapper.h:37: class Wrapper {
On 2014/01/23 00:07:23, Evan Stade wrote:
> Wrapper is far too generic a name to be useful. You might as well call it
class
> Foo.
> 
> Why not put this in an anonymous namespace in retriever.cc[1]? If the only
> answer is "testing", I think you could still test it effectively, and the
tests
> you wrote would even be superior in that they'd be testing behavior rather
than
> implementation.
> 
> [1]I know that before I said this wasn't necessary, but being unable to come
up
> with a decent name for the class is perhaps an indication it's not generically
> useful, so should be folded into Retriever.

Done.

[Evan Stade, 2014-01-23 01:44:50]

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

https://codereview.chromium.org/144353002/diff/250014/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/retriever.cc:61: bool
UnwrapHeader(const char* header_prefix,
nit: put this about Unwrap

also, s/UnwrapHeader/ExtractHeader

https://codereview.chromium.org/144353002/diff/250014/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/retriever.cc:100: std::string
Wrap(const std::string& data) {
s/Wrap/PrependTimestamp

https://codereview.chromium.org/144353002/diff/250014/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/retriever.cc:123: bool
Unwrap(std::string* data, double* age_in_seconds) {
I'd prefer if there were separate in and out params for |data| rather than an
in-out param.

https://codereview.chromium.org/144353002/diff/250014/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/retriever.cc:123: bool
Unwrap(std::string* data, double* age_in_seconds) {
s/Unwrap/VerifyAndExtractTimestamp

https://codereview.chromium.org/144353002/diff/250014/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/retriever.cc:191: if (success &&
valid_format && age_in_seconds < kStaleDataAgeInSeconds) {
if (!success) you shouldn't call Unwrap

https://codereview.chromium.org/144353002/diff/250014/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/retriever.cc:196: } else {
nit: prefer early return over else {} that goes to the end of the function.

https://codereview.chromium.org/144353002/diff/250014/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/retriever.cc:215: response =
downloaded_data;
unnecessary copy (potentially quite large)

https://codereview.chromium.org/144353002/diff/250014/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/retriever.cc:218: response =
stale_data_it->second;
unnecessary copy (potentially quite large)

https://codereview.chromium.org/144353002/diff/250014/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/retriever.cc:254: // An abandonened
request.
nit: spelling

[please use gerrit instead, 2014-01-23 23:11:04]

Evan: PTAL Patch Set 4.

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

https://codereview.chromium.org/144353002/diff/250014/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/retriever.cc:61: bool
UnwrapHeader(const char* header_prefix,
On 2014/01/23 01:44:50, Evan Stade wrote:
> nit: put this about Unwrap
> 
> also, s/UnwrapHeader/ExtractHeader

Done.

https://codereview.chromium.org/144353002/diff/250014/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/retriever.cc:100: std::string
Wrap(const std::string& data) {
On 2014/01/23 01:44:50, Evan Stade wrote:
> s/Wrap/PrependTimestamp

Done.

https://codereview.chromium.org/144353002/diff/250014/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/retriever.cc:123: bool
Unwrap(std::string* data, double* age_in_seconds) {
On 2014/01/23 01:44:50, Evan Stade wrote:
> s/Unwrap/VerifyAndExtractTimestamp

Done.

https://codereview.chromium.org/144353002/diff/250014/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/retriever.cc:123: bool
Unwrap(std::string* data, double* age_in_seconds) {
On 2014/01/23 01:44:50, Evan Stade wrote:
> I'd prefer if there were separate in and out params for |data| rather than an
> in-out param.

Done.

https://codereview.chromium.org/144353002/diff/250014/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/retriever.cc:191: if (success &&
valid_format && age_in_seconds < kStaleDataAgeInSeconds) {
On 2014/01/23 01:44:50, Evan Stade wrote:
> if (!success) you shouldn't call Unwrap

Done.

https://codereview.chromium.org/144353002/diff/250014/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/retriever.cc:196: } else {
On 2014/01/23 01:44:50, Evan Stade wrote:
> nit: prefer early return over else {} that goes to the end of the function.

Done.

https://codereview.chromium.org/144353002/diff/250014/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/retriever.cc:215: response =
downloaded_data;
On 2014/01/23 01:44:50, Evan Stade wrote:
> unnecessary copy (potentially quite large)

Done.

https://codereview.chromium.org/144353002/diff/250014/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/retriever.cc:218: response =
stale_data_it->second;
On 2014/01/23 01:44:50, Evan Stade wrote:
> unnecessary copy (potentially quite large)

Done.

https://codereview.chromium.org/144353002/diff/250014/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/retriever.cc:254: // An abandonened
request.
On 2014/01/23 01:44:50, Evan Stade wrote:
> nit: spelling

Done.

[Evan Stade, 2014-01-23 23:50:20]

lgtm

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

https://codereview.chromium.org/144353002/diff/380001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/retriever.cc:198: } else {
no else after return

[please use gerrit instead, 2014-01-23 23:54:56]

Sending to cq.

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

https://codereview.chromium.org/144353002/diff/380001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/retriever.cc:198: } else {
On 2014/01/23 23:50:20, Evan Stade wrote:
> no else after return

Done.

[commit-bot: I haz the power, 2014-01-24 00:14:59]

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

[commit-bot: I haz the power, 2014-01-24 02:49:46]

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

[commit-bot: I haz the power, 2014-01-24 04:25:25]

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

[commit-bot: I haz the power, 2014-01-24 13:00:51]

Change committed as 246860