Allow BackoffEntry to be serialized and deserialized.

net/base/backoff_entry_serializer.cc

Index: net/base/backoff_entry_serializer.cc
diff --git a/net/base/backoff_entry_serializer.cc b/net/base/backoff_entry_serializer.cc
new file mode 100644
index 0000000000000000000000000000000000000000..3d04e10b0afd23fcb5db4fef0bf28c5222a62306
--- /dev/null
+++ b/net/base/backoff_entry_serializer.cc
@@ -0,0 +1,70 @@
+// Copyright (c) 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "net/base/backoff_entry_serializer.h"
+
+#include "base/time/tick_clock.h"
+#include "base/values.h"
+#include "net/base/backoff_entry.h"
+
+namespace {
+const int SERIALIZATION_VERSION_NUMBER = 1;
+}  // namespace
+
+namespace net {
+
+scoped_ptr<base::ListValue> BackoffEntrySerializer::SerializeToValue(
+    const BackoffEntry& entry, base::Time time_now) {
+  scoped_ptr<base::ListValue> serialized(new base::ListValue());
+  serialized->AppendInteger(SERIALIZATION_VERSION_NUMBER);
+  serialized->AppendInteger(entry.failure_count());
+  // Can't use entry.GetTimeUntilRelease as it doesn't allow negative deltas.
+  base::TimeDelta time_until_release =
+      entry.GetReleaseTime() - entry.clock_->NowTicks();
+  // We redundantly store both the remaining time delta and the absolute time.

[mmenke, 2015/05/05 15:47:52]

Avoid using "we" in comments.

[johnme, 2015/05/06 12:46:47]

Done.

+  // This will let us work around some cases where wall clock time changes.

[mmenke, 2015/05/05 15:47:51]

Avoid using "us" in comments.

[johnme, 2015/05/06 12:46:47]

Done.

+  serialized->AppendDouble(time_until_release.InSecondsF());
+  base::Time absolute_release_time = time_until_release + time_now;
+  serialized->AppendDouble(absolute_release_time.ToDoubleT());

[mmenke, 2015/05/05 15:47:52]

Do we really need these both?  Seems like we could just record
absolute_release_time, and if it's greater than the value of
CalculateReleaseTime(), just ignore absolute_release_time.

Doesn't quite catch as much...but do we care?

[johnme, 2015/05/06 12:46:47]

I added serialization of the time_until_release (now called backoff_duration) in
response to rsleevi's comment[1] that "Serializing time, rather than time
deltas, is almost always a Big Bad Thing". This way we're safe against the
system (wall) clock jumping backwards, which seems a useful protection
(otherwise, if the system clock jumps backwards a lot, the best we could do is
to set release time to now + maximum_backoff_ms, which may be excessively
large).

I'm not sure what you mean by "the value of CalculateReleaseTime()", since
during deserialization it's up to us to set that value. Perhaps you mean "the
value of CalculateReleaseTime() after calling InformOfRequest(false)
failure_count times", but that calculated release time might be wrong, since
BackoffEntry has a public SetCustomReleaseTime method that could have been used
to override the release time.

[1]: https://codereview.chromium.org/1023473003/#msg11

[mmenke, 2015/05/06 17:34:47]

Ah, right.  Forgot about SetCustomReleaseTime.

+  return serialized;
+}
+
+scoped_ptr<BackoffEntry> BackoffEntrySerializer::DeserializeFromValue(
+    const base::ListValue& serialized, const BackoffEntry::Policy* policy,
+    base::TickClock* tick_clock, base::Time time_now) {
+  if (serialized.GetSize() != 4)
+    return nullptr;
+  int version_number;
+  if (!serialized.GetInteger(0, &version_number) ||
+      version_number != SERIALIZATION_VERSION_NUMBER)
+    return nullptr;

[mmenke, 2015/05/05 15:47:51]

Use braces when the condition of an if takes up more than 1 line.

[johnme, 2015/05/06 12:46:46]

Done.

+  int failure_count;
+  if (!serialized.GetInteger(1, &failure_count))

[mmenke, 2015/05/05 15:47:51]

We should fail if this is negative.

[johnme, 2015/05/06 12:46:47]

Done.

+    return nullptr;
+  double original_time_until_release_double;
+  if (!serialized.GetDouble(2, &original_time_until_release_double))
+    return nullptr;
+  double absolute_release_time_double;
+  if (!serialized.GetDouble(3, &absolute_release_time_double))

[mmenke, 2015/05/05 15:47:51]

We should fail if this is negative.

[johnme, 2015/05/06 12:46:47]

Done.

+    return nullptr;
+  scoped_ptr<BackoffEntry> entry(new BackoffEntry(policy, tick_clock));
+  entry->failure_count_ = failure_count;

[mmenke, 2015/05/05 15:47:52]

Erm...Could we just call InformOfRequest(false) failure_count times, make
entry->failure_count() public, and then have BackoffEntry de-friend us?

The loop isn't lovely, but not a big fan of friend classes unless we really need
them.

[johnme, 2015/05/06 12:46:47]

Hmm, ok done, but note that I had to make TimeUntilReleaseToReleaseTime (now
called BackoffDurationToReleaseTime) and tick_clock public (in addition to the
expensive loop).

+  base::TimeDelta original_time_until_release =
+      base::TimeDelta::FromSecondsD(original_time_until_release_double);
+  base::Time absolute_release_time =
+      base::Time::FromDoubleT(absolute_release_time_double);
+  base::TimeDelta time_until_release =
+      absolute_release_time - time_now;
+  // In cases where the system wall clock is rewound, use the redundant
+  // original_time_until_release to ensure the backoff duration isn't longer
+  // than it was before serializing (note that it's not possible to protect
+  // against the clock being wound forward).
+  if (time_until_release > original_time_until_release)
+    time_until_release = original_time_until_release;
+  entry->SetCustomReleaseTime(
+      entry->TimeUntilReleaseToReleaseTime(time_until_release));
+  return entry;

[mmenke, 2015/05/05 15:47:52]

Think this method could use a couple blank lines, to make it more readable.

[johnme, 2015/05/06 12:46:47]

Done.

+}
+
+}  // namespace net