Views: Check results of type conversions from strings

This CL changed the way how we handle conversion failures from strings.
Instead of using the default values, this CL returns 'false' to indicate
the conversion errors. This returned result is useful in case we need to
handle more complex type conversions such base::optional type.

Also, this CL simplify the type converter function definitions since we
only need to and from string conversions, not between arbitrary two
types.

BUG=938501

Change-Id: I1e6d8510c39a9a240161aa0b36c2141ab62ed0e8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1606772
Commit-Queue: Wei Li <weili@chromium.org>
Reviewed-by: Allen Bauer <kylixrd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#659250}

ui/views/metadata/property_metadata.h

@@ -26,7 +26,7 @@ class ClassPropertyReadOnlyMetaData : public MemberMetaDataBase {
   ~ClassPropertyReadOnlyMetaData() override = default;
 
   base::string16 GetValueAsString(void* obj) const override {
-    return Convert<TValue, base::string16>((static_cast<TClass*>(obj)->*Get)());
+    return ConvertToString<TValue>((static_cast<TClass*>(obj)->*Get)());
   }
 
   PropertyFlags GetPropertyFlags() const override {
@@ -53,8 +53,9 @@ class ClassPropertyMetaData
   ~ClassPropertyMetaData() override = default;
 
   void SetValueAsString(void* obj, const base::string16& new_value) override {
-    (static_cast<TClass*>(obj)->*Set)(Convert<base::string16, TValue>(
-        new_value, (static_cast<TClass*>(obj)->*Get)()));
+    TValue result;
+    if (ConvertFromString<TValue>(new_value, &result))
+      (static_cast<TClass*>(obj)->*Set)(result);
   }
 
   PropertyFlags GetPropertyFlags() const override {

ui/views/metadata/type_conversion.cc

@@ -17,12 +17,6 @@ namespace metadata {
 
 /***** String Conversions *****/
 
-template <>
-base::string16 ConvertToString<base::string16>(
-    const base::string16& source_value) {
-  return source_value;
-}
-
 template <>
 base::string16 ConvertToString<int8_t>(int8_t source_value) {
   return base::NumberToString16(source_value);
@@ -85,110 +79,131 @@ base::string16 ConvertToString<gfx::Size>(const gfx::Size& source_value) {
 }
 
 template <>
-int8_t ConvertFromString<int8_t>(const base::string16& source_value,
-                                 int8_t default_value) {
-  int32_t ret = 0;
-  return base::StringToInt(source_value, &ret) &&
-                 base::IsValueInRangeForNumericType<int8_t>(ret)
-             ? static_cast<int8_t>(ret)
-             : default_value;
+base::string16 ConvertToString<base::string16>(
+    const base::string16& source_value) {
+  return source_value;
 }
 
 template <>
-int16_t ConvertFromString<int16_t>(const base::string16& source_value,
-                                   int16_t default_value) {
+bool ConvertFromString<int8_t>(const base::string16& source_value,
+                               int8_t* dst_value) {
   int32_t ret = 0;
-  return base::StringToInt(source_value, &ret) &&
-                 base::IsValueInRangeForNumericType<int16_t>(ret)
-             ? static_cast<int16_t>(ret)
-             : default_value;
+  if (base::StringToInt(source_value, &ret) &&
+      base::IsValueInRangeForNumericType<int8_t>(ret)) {
+    *dst_value = static_cast<int8_t>(ret);
+    return true;
+  }
+  return false;
 }
 
 template <>
-int32_t ConvertFromString<int32_t>(const base::string16& source_value,
-                                   int32_t default_value) {
+bool ConvertFromString<int16_t>(const base::string16& source_value,
+                                int16_t* dst_value) {
   int32_t ret = 0;
-  return base::StringToInt(source_value, &ret) ? static_cast<int32_t>(ret)
-                                               : default_value;
+  if (base::StringToInt(source_value, &ret) &&
+      base::IsValueInRangeForNumericType<int16_t>(ret)) {
+    *dst_value = static_cast<int16_t>(ret);
+    return true;
+  }
+  return false;
 }
 
 template <>
-int64_t ConvertFromString<int64_t>(const base::string16& source_value,
-                                   int64_t default_value) {
-  int64_t ret = 0;
-  return base::StringToInt64(source_value, &ret) ? ret : default_value;
+bool ConvertFromString<int32_t>(const base::string16& source_value,
+                                int32_t* dst_value) {
+  return base::StringToInt(source_value, dst_value);
 }
 
 template <>
-uint8_t ConvertFromString<uint8_t>(const base::string16& source_value,
-                                   uint8_t default_value) {
-  uint32_t ret = 0;
-  return base::StringToUint(source_value, &ret) &&
-                 base::IsValueInRangeForNumericType<uint8_t>(ret)
-             ? static_cast<uint8_t>(ret)
-             : default_value;
+bool ConvertFromString<int64_t>(const base::string16& source_value,
+                                int64_t* dst_value) {
+  return base::StringToInt64(source_value, dst_value);
 }
 
 template <>
-uint16_t ConvertFromString<uint16_t>(const base::string16& source_value,
-                                     uint16_t default_value) {
+bool ConvertFromString<uint8_t>(const base::string16& source_value,
+                                uint8_t* dst_value) {
   uint32_t ret = 0;
-  return base::StringToUint(source_value, &ret) &&
-                 base::IsValueInRangeForNumericType<uint16_t>(ret)
-             ? static_cast<uint16_t>(ret)
-             : default_value;
+  if (base::StringToUint(source_value, &ret) &&
+      base::IsValueInRangeForNumericType<uint8_t>(ret)) {
+    *dst_value = static_cast<uint8_t>(ret);
+    return true;
+  }
+  return false;
 }
 
 template <>
-uint32_t ConvertFromString<uint32_t>(const base::string16& source_value,
-                                     uint32_t default_value) {
+bool ConvertFromString<uint16_t>(const base::string16& source_value,
+                                 uint16_t* dst_value) {
   uint32_t ret = 0;
-  return base::StringToUint(source_value, &ret) ? static_cast<uint32_t>(ret)
-                                                : default_value;
+  if (base::StringToUint(source_value, &ret) &&
+      base::IsValueInRangeForNumericType<uint16_t>(ret)) {
+    *dst_value = static_cast<uint16_t>(ret);
+    return true;
+  }
+  return false;
+}
+
+template <>
+bool ConvertFromString<uint32_t>(const base::string16& source_value,
+                                 uint32_t* dst_value) {
+  return base::StringToUint(source_value, dst_value);
 }
 
 template <>
-uint64_t ConvertFromString<uint64_t>(const base::string16& source_value,
-                                     uint64_t default_value) {
-  uint64_t ret = 0;
-  return base::StringToUint64(source_value, &ret) ? ret : default_value;
+bool ConvertFromString<uint64_t>(const base::string16& source_value,
+                                 uint64_t* dst_value) {
+  return base::StringToUint64(source_value, dst_value);
 }
 
 template <>
-float ConvertFromString<float>(const base::string16& source_value,
-                               float default_value) {
-  return static_cast<float>(
-      ConvertFromString<double>(source_value, default_value));
+bool ConvertFromString<float>(const base::string16& source_value,
+                              float* dst_value) {
+  double temp;
+  if (ConvertFromString<double>(source_value, &temp)) {
+    *dst_value = static_cast<float>(temp);
+    return true;
+  }
+  return false;
 }
 
 template <>
-double ConvertFromString<double>(const base::string16& source_value,
-                                 double default_value) {
-  double ret = 0;
-  return base::StringToDouble(base::UTF16ToUTF8(source_value), &ret)
-             ? ret
-             : default_value;
+bool ConvertFromString<double>(const base::string16& source_value,
+                               double* dst_value) {
+  return base::StringToDouble(base::UTF16ToUTF8(source_value), dst_value);
 }
 
 template <>
 bool ConvertFromString<bool>(const base::string16& source_value,
-                             bool default_value) {
-  if (source_value == base::ASCIIToUTF16("false"))
-    return false;
-  return source_value == base::ASCIIToUTF16("true") || default_value;
+                             bool* dst_value) {
+  if (source_value == base::ASCIIToUTF16("true") ||
+      source_value == base::ASCIIToUTF16("false")) {
+    *dst_value = source_value == base::ASCIIToUTF16("true");
+    return true;
+  }
+  return false;
 }
 
 template <>
-gfx::Size ConvertFromString<gfx::Size>(const base::string16& source_value,
-                                       const gfx::Size& default_value) {
+bool ConvertFromString<gfx::Size>(const base::string16& source_value,
+                                  gfx::Size* dst_value) {
   const auto values =
       base::SplitStringPiece(source_value, base::ASCIIToUTF16("{,}"),
                              base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY);
   int width, height;
-  return (values.size() == 2) && base::StringToInt(values[0], &width) &&
-                 base::StringToInt(values[1], &height)
-             ? gfx::Size(width, height)
-             : default_value;
+  if ((values.size() == 2) && base::StringToInt(values[0], &width) &&
+      base::StringToInt(values[1], &height)) {
+    *dst_value = gfx::Size(width, height);
+    return true;
+  }
+  return false;
+}
+
+template <>
+bool ConvertFromString<base::string16>(const base::string16& source_value,
+                                       base::string16* dst_value) {
+  *dst_value = source_value;
+  return true;
 }
 
 }  // namespace metadata

ui/views/metadata/type_conversion_unittest.cc

@@ -9,30 +9,29 @@
 #include "testing/platform_test.h"
 #include "ui/gfx/geometry/rect.h"
 
-namespace VM = views::metadata;
+namespace TC = views::metadata;
 
 using TypeConversionTest = PlatformTest;
 
 TEST_F(TypeConversionTest, TestConversion_IntToString) {
   int from_int = 5;
-  base::string16 to_string = VM::Convert<int, base::string16>(from_int);
+  base::string16 to_string = TC::ConvertToString<int>(from_int);
 
   EXPECT_EQ(to_string, base::ASCIIToUTF16("5"));
 }
 
 TEST_F(TypeConversionTest, TestConversion_StringToInt) {
   base::string16 from_string = base::ASCIIToUTF16("10");
-  int to_int = VM::Convert<base::string16, int>(from_string, 0);
+  int to_int = 0;
+  EXPECT_TRUE(TC::ConvertFromString<int>(from_string, &to_int));
 
   EXPECT_EQ(to_int, 10);
 }
 
 // This tests whether the converter handles a bogus input string, in which case
-// the default value should be returned.
-TEST_F(TypeConversionTest, TestConversion_StringToIntDefault) {
-  const int default_value = 1000;
+// the return value should be false.
+TEST_F(TypeConversionTest, TestConversion_BogusStringToInt) {
   base::string16 from_string = base::ASCIIToUTF16("Foo");
-  int to_int = VM::Convert<base::string16, int>(from_string, default_value);
-
-  EXPECT_EQ(to_int, default_value);
+  int to_int = 0;
+  EXPECT_FALSE(TC::ConvertFromString<int>(from_string, &to_int));
 }