Add helper functions for converting int64_t/TimeDelta/Time to base::Value

This CL adds helper functions for converting Time values into base::Values
via string representation. As a side effect we also get TimeDelta and
int64_t conversions for "free" as they have identical data representations.

This change is to dedupe common logic between PrefService,
AppBannerSettingsBanner and a future CL to add the same conversion logic
in a web_app::install_metrics namespace.

Change-Id: Ie5758a6e395292eb738f89a395df3763ee674739
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1596149Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Reviewed-by: Dominic Battré <battre@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Commit-Queue: Dominick Ng <dominickn@chromium.org>
Auto-Submit: Alan Cutter <alancutter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#658727}

base/util/BUILD.gn

@@ -7,6 +7,7 @@ import("//testing/test.gni")
 test("base_util_unittests") {
   deps = [
     "type-safety:tests",
+    "values:unittests",
     "//base/test:run_all_base_unittests",
   ]
 }

base/util/values/BUILD.gn

+# Copyright 2019 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.
+
+source_set("values_util") {
+  sources = [
+    "values_util.cc",
+    "values_util.h",
+  ]
+
+  deps = [
+    "//base:base",
+  ]
+}
+
+source_set("unittests") {
+  testonly = true
+  sources = [
+    "values_util_unittest.cc",
+  ]
+
+  deps = [
+    ":values_util",
+    "//testing/gtest",
+  ]
+}

base/util/values/OWNERS

+alancutter@chromium.org
+jdoerrie@chromium.org

base/util/values/values_util.cc

+// Copyright 2019 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 "base/util/values/values_util.h"
+
+#include "base/strings/string_number_conversions.h"
+
+namespace util {
+
+base::Value Int64ToValue(int64_t integer) {
+  return base::Value(base::NumberToString(integer));
+}
+
+base::Optional<int64_t> ValueToInt64(const base::Value* value) {
+  return value ? ValueToInt64(*value) : base::nullopt;
+}
+
+base::Optional<int64_t> ValueToInt64(const base::Value& value) {
+  if (!value.is_string())
+    return base::nullopt;
+
+  int64_t integer;
+  if (!base::StringToInt64(value.GetString(), &integer))
+    return base::nullopt;
+
+  return integer;
+}
+
+base::Value TimeDeltaToValue(base::TimeDelta time_delta) {
+  return Int64ToValue(time_delta.InMicroseconds());
+}
+
+base::Optional<base::TimeDelta> ValueToTimeDelta(const base::Value* value) {
+  return value ? ValueToTimeDelta(*value) : base::nullopt;
+}
+
+base::Optional<base::TimeDelta> ValueToTimeDelta(const base::Value& value) {
+  base::Optional<int64_t> integer = ValueToInt64(value);
+  if (!integer)
+    return base::nullopt;
+  return base::TimeDelta::FromMicroseconds(*integer);
+}
+
+base::Value TimeToValue(base::Time time) {
+  return TimeDeltaToValue(time.ToDeltaSinceWindowsEpoch());
+}
+
+base::Optional<base::Time> ValueToTime(const base::Value* value) {
+  return value ? ValueToTime(*value) : base::nullopt;
+}
+
+base::Optional<base::Time> ValueToTime(const base::Value& value) {
+  base::Optional<base::TimeDelta> time_delta = ValueToTimeDelta(value);
+  if (!time_delta)
+    return base::nullopt;
+  return base::Time::FromDeltaSinceWindowsEpoch(*time_delta);
+}
+
+}  // namespace util

base/util/values/values_util.h

+// Copyright 2019 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.
+
+#ifndef BASE_UTIL_VALUES_VALUES_UTIL_H_
+#define BASE_UTIL_VALUES_VALUES_UTIL_H_
+
+#include "base/optional.h"
+#include "base/time/time.h"
+#include "base/values.h"
+
+namespace util {
+
+// Simple helper functions for converting int64_t, base::TimeDelta and
+// base::Time to numeric string base::Values.
+// Because base::TimeDelta and base::Time share the same internal representation
+// as int64_t they are stored using the exact same numeric string format.
+
+// Stores the int64_t as a string.
+base::Value Int64ToValue(int64_t integer);
+base::Optional<int64_t> ValueToInt64(const base::Value* value);
+base::Optional<int64_t> ValueToInt64(const base::Value& value);
+
+// Converts the TimeDelta to an int64_t of microseconds.
+base::Value TimeDeltaToValue(base::TimeDelta time_delta);
+base::Optional<base::TimeDelta> ValueToTimeDelta(const base::Value* value);
+base::Optional<base::TimeDelta> ValueToTimeDelta(const base::Value& value);
+
+// Converts the Time to a TimeDelta from the Windows epoch.
+base::Value TimeToValue(base::Time time);
+base::Optional<base::Time> ValueToTime(const base::Value* value);
+base::Optional<base::Time> ValueToTime(const base::Value& value);
+
+}  // namespace util
+
+#endif  // BASE_UTIL_VALUES_VALUES_UTIL_H_

base/util/values/values_util_unittest.cc

+// Copyright 2019 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 <limits>
+
+#include "base/util/values/values_util.h"
+
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace util {
+
+namespace {
+
+TEST(ValuesUtilTest, BasicLimits) {
+  struct {
+    int64_t input;
+    const char* expected;
+  } test_cases[] = {
+      {0, "0"},
+      {-1234, "-1234"},
+      {5678, "5678"},
+      {std::numeric_limits<int64_t>::lowest(), "-9223372036854775808"},
+      {std::numeric_limits<int64_t>::max(), "9223372036854775807"},
+  };
+  for (const auto& test_case : test_cases) {
+    int64_t input = test_case.input;
+    base::TimeDelta time_delta_input = base::TimeDelta::FromMicroseconds(input);
+    base::Time time_input =
+        base::Time::FromDeltaSinceWindowsEpoch(time_delta_input);
+    base::Value expected(test_case.expected);
+    SCOPED_TRACE(testing::Message()
+                 << "input: " << input << ", expected: " << expected);
+
+    EXPECT_EQ(Int64ToValue(input), expected);
+    EXPECT_EQ(*ValueToInt64(&expected), input);
+
+    EXPECT_EQ(TimeDeltaToValue(time_delta_input), expected);
+    EXPECT_EQ(*ValueToTimeDelta(&expected), time_delta_input);
+
+    EXPECT_EQ(TimeToValue(time_input), expected);
+    EXPECT_EQ(*ValueToTime(&expected), time_input);
+  }
+}
+
+TEST(ValuesUtilTest, InvalidValues) {
+  std::unique_ptr<base::Value> test_cases[] = {
+      nullptr,
+      std::make_unique<base::Value>(),
+      std::make_unique<base::Value>(0),
+      std::make_unique<base::Value>(1234),
+      std::make_unique<base::Value>(true),
+      std::make_unique<base::Value>(base::Value::Type::BINARY),
+      std::make_unique<base::Value>(base::Value::Type::LIST),
+      std::make_unique<base::Value>(base::Value::Type::DICTIONARY),
+      std::make_unique<base::Value>(""),
+      std::make_unique<base::Value>("abcd"),
+      std::make_unique<base::Value>("1234.0"),
+      std::make_unique<base::Value>("1234a"),
+      std::make_unique<base::Value>("a1234"),
+  };
+  for (const auto& test_case : test_cases) {
+    EXPECT_FALSE(ValueToInt64(test_case.get()));
+    EXPECT_FALSE(ValueToTimeDelta(test_case.get()));
+    EXPECT_FALSE(ValueToTime(test_case.get()));
+  }
+}
+
+}  // namespace
+
+}  // namespace util

chrome/browser/BUILD.gn

@@ -1825,6 +1825,7 @@ jumbo_split_static_library("browser") {
     ":resource_prefetch_predictor_proto",
     "//base:i18n",
     "//base/allocator:buildflags",
+    "//base/util/values:values_util",
     "//cc",
     "//chrome:extra_resources",
     "//chrome:resources",

chrome/browser/banners/app_banner_settings_helper.cc

@@ -14,6 +14,7 @@
 #include "base/command_line.h"
 #include "base/optional.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/util/values/values_util.h"
 #include "chrome/browser/banners/app_banner_manager.h"
 #include "chrome/browser/banners/app_banner_metrics.h"
 #include "chrome/browser/browser_process.h"
@@ -182,21 +183,6 @@ bool WasEventWithinPeriod(AppBannerSettingsHelper::AppBannerEvent event,
   return (now - event_time < period);
 }
 
-base::Optional<base::TimeDelta> ParseTimeDelta(const base::Value* value) {
-  std::string delta_string;
-  if (!value || !value->GetAsString(&delta_string))
-    return base::nullopt;
-
-  int64_t delta_int64;
-  if (!base::StringToInt64(delta_string, &delta_int64))
-    return base::nullopt;
-  return base::TimeDelta::FromMicroseconds(delta_int64);
-}
-
-base::Value SerializeTimeDelta(const base::TimeDelta& delta) {
-  return base::Value(base::NumberToString(delta.InMicroseconds()));
-}
-
 // Dictionary of time information for how long to wait before showing the
 // "Install" text slide animation again.
 // Data format: {"last_shown": timestamp, "delay": duration}
@@ -230,18 +216,17 @@ base::Optional<NextInstallTextAnimation> NextInstallTextAnimation::Get(
   if (!next_dict || !next_dict->is_dict())
     return base::nullopt;
 
-  base::Optional<base::TimeDelta> last_shown_since_epoch =
-      ParseTimeDelta(next_dict->FindKey(kLastShownKey));
-  if (!last_shown_since_epoch)
+  base::Optional<base::Time> last_shown =
+      util::ValueToTime(next_dict->FindKey(kLastShownKey));
+  if (!last_shown)
     return base::nullopt;
 
   base::Optional<base::TimeDelta> delay =
-      ParseTimeDelta(next_dict->FindKey(kDelayKey));
+      util::ValueToTimeDelta(next_dict->FindKey(kDelayKey));
   if (!delay)
     return base::nullopt;
 
-  return NextInstallTextAnimation{
-      base::Time::FromDeltaSinceWindowsEpoch(*last_shown_since_epoch), *delay};
+  return NextInstallTextAnimation{*last_shown, *delay};
 }
 
 void NextInstallTextAnimation::RecordToPrefs(content::WebContents* web_contents,
@@ -251,9 +236,8 @@ void NextInstallTextAnimation::RecordToPrefs(content::WebContents* web_contents,
     return;
 
   base::Value next_dict(base::Value::Type::DICTIONARY);
-  next_dict.SetKey(kLastShownKey,
-                   SerializeTimeDelta(last_shown.ToDeltaSinceWindowsEpoch()));
-  next_dict.SetKey(kDelayKey, SerializeTimeDelta(delay));
+  next_dict.SetKey(kLastShownKey, util::TimeToValue(last_shown));
+  next_dict.SetKey(kDelayKey, util::TimeDeltaToValue(delay));
   app_prefs.dict()->SetKey(kNextInstallTextAnimation, std::move(next_dict));
   app_prefs.Save();
 }

components/prefs/BUILD.gn

@@ -52,6 +52,7 @@ component("prefs") {
 
   deps = [
     "//base",
+    "//base/util/values:values_util",
   ]
 }
 

components/prefs/pref_service.cc

@@ -21,6 +21,7 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "base/util/values/values_util.h"
 #include "base/value_conversions.h"
 #include "build/build_config.h"
 #include "components/prefs/default_pref_store.h"
@@ -491,22 +492,14 @@ void PrefService::SetFilePath(const std::string& path,
 
 void PrefService::SetInt64(const std::string& path, int64_t value) {
   SetUserPrefValue(path,
-                   std::make_unique<base::Value>(base::NumberToString(value)));
+                   base::Value::ToUniquePtrValue(util::Int64ToValue(value)));
 }
 
 int64_t PrefService::GetInt64(const std::string& path) const {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-
   const base::Value* value = GetPreferenceValueChecked(path);
-  if (!value)
-    return 0;
-  std::string result("0");
-  bool rv = value->GetAsString(&result);
-  DCHECK(rv);
-
-  int64_t val;
-  base::StringToInt64(result, &val);
-  return val;
+  base::Optional<int64_t> integer = util::ValueToInt64(value);
+  DCHECK(integer);
+  return integer.value_or(0);
 }
 
 void PrefService::SetUint64(const std::string& path, uint64_t value) {
@@ -530,20 +523,27 @@ uint64_t PrefService::GetUint64(const std::string& path) const {
 }
 
 void PrefService::SetTime(const std::string& path, base::Time value) {
-  SetInt64(path, value.ToDeltaSinceWindowsEpoch().InMicroseconds());
+  SetUserPrefValue(path,
+                   base::Value::ToUniquePtrValue(util::TimeToValue(value)));
 }
 
 base::Time PrefService::GetTime(const std::string& path) const {
-  return base::Time::FromDeltaSinceWindowsEpoch(
-      base::TimeDelta::FromMicroseconds(GetInt64(path)));
+  const base::Value* value = GetPreferenceValueChecked(path);
+  base::Optional<base::Time> time = util::ValueToTime(value);
+  DCHECK(time);
+  return time.value_or(base::Time());
 }
 
 void PrefService::SetTimeDelta(const std::string& path, base::TimeDelta value) {
-  SetInt64(path, value.InMicroseconds());
+  SetUserPrefValue(
+      path, base::Value::ToUniquePtrValue(util::TimeDeltaToValue(value)));
 }
 
 base::TimeDelta PrefService::GetTimeDelta(const std::string& path) const {
-  return base::TimeDelta::FromMicroseconds(GetInt64(path));
+  const base::Value* value = GetPreferenceValueChecked(path);
+  base::Optional<base::TimeDelta> time_delta = util::ValueToTimeDelta(value);
+  DCHECK(time_delta);
+  return time_delta.value_or(base::TimeDelta());
 }
 
 base::Value* PrefService::GetMutableUserPref(const std::string& path,