[Telemetry SWX] Introduce ConvertPtr that checks null input

Introduce ConvertPtr template that checks whether input is null.

Since ConvertPtr is a template we can test it with any valid type.

Bug: b:158658869
Change-Id: I78d5f7c984303cbd9e34b187c07e1f08a0a891eb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2273065
Commit-Queue: Oleh Lamzin <lamzin@google.com>
Reviewed-by: Mahmoud Gawad <mgawad@google.com>
Reviewed-by: Roland Bock <rbock@google.com>
Cr-Commit-Position: refs/heads/master@{#786203}

chromeos/components/telemetry_extension_ui/probe_service.cc

@@ -28,7 +28,7 @@ void ProbeService::ProbeTelemetryInfo(
           [](health::mojom::ProbeService::ProbeTelemetryInfoCallback callback,
              cros_healthd::mojom::TelemetryInfoPtr ptr) {
             std::move(callback).Run(
-                probe_service_converters::Convert(std::move(ptr)));
+                probe_service_converters::ConvertPtr(std::move(ptr)));
           },
           std::move(callback)));
 }

chromeos/components/telemetry_extension_ui/probe_service_converters.cc

@@ -30,56 +30,21 @@ cros_healthd::mojom::ProbeCategoryEnum Convert(
 
 }  // namespace
 
-health::mojom::ErrorType Convert(cros_healthd::mojom::ErrorType input) {
-  switch (input) {
-    case cros_healthd::mojom::ErrorType::kFileReadError:
-      return health::mojom::ErrorType::kFileReadError;
-    case cros_healthd::mojom::ErrorType::kParseError:
-      return health::mojom::ErrorType::kParseError;
-    case cros_healthd::mojom::ErrorType::kSystemUtilityError:
-      return health::mojom::ErrorType::kSystemUtilityError;
-  }
-  NOTREACHED();
-}
+namespace unchecked {
 
-health::mojom::ProbeErrorPtr Convert(cros_healthd::mojom::ProbeErrorPtr input) {
-  if (!input) {
-    return nullptr;
-  }
+health::mojom::ProbeErrorPtr UncheckedConvertPtr(
+    cros_healthd::mojom::ProbeErrorPtr input) {
   return health::mojom::ProbeError::New(Convert(input->type),
                                         std::move(input->msg));
 }
 
-health::mojom::DoubleValuePtr Convert(double input) {
-  return health::mojom::DoubleValue::New(input);
-}
-
-health::mojom::Int64ValuePtr Convert(int64_t input) {
-  return health::mojom::Int64Value::New(input);
-}
-
-health::mojom::UInt32ValuePtr Convert(uint32_t input) {
-  return health::mojom::UInt32Value::New(input);
-}
-
-health::mojom::UInt64ValuePtr Convert(uint64_t input) {
-  return health::mojom::UInt64Value::New(input);
-}
-
-health::mojom::UInt64ValuePtr Convert(
+health::mojom::UInt64ValuePtr UncheckedConvertPtr(
     cros_healthd::mojom::UInt64ValuePtr input) {
-  if (!input) {
-    return nullptr;
-  }
   return health::mojom::UInt64Value::New(input->value);
 }
 
-health::mojom::BatteryInfoPtr Convert(
+health::mojom::BatteryInfoPtr UncheckedConvertPtr(
     cros_healthd::mojom::BatteryInfoPtr input) {
-  if (!input) {
-    return nullptr;
-  }
-
   return health::mojom::BatteryInfo::New(
       Convert(input->cycle_count), Convert(input->voltage_now),
       std::move(input->vendor), std::move(input->serial_number),
@@ -88,27 +53,23 @@ health::mojom::BatteryInfoPtr Convert(
       Convert(input->charge_now), Convert(input->current_now),
       std::move(input->technology), std::move(input->status),
       std::move(input->manufacture_date),
-      Convert(std::move(input->temperature)));
+      ConvertPtr(std::move(input->temperature)));
 }
 
-health::mojom::BatteryResultPtr Convert(
+health::mojom::BatteryResultPtr UncheckedConvertPtr(
     cros_healthd::mojom::BatteryResultPtr input) {
-  if (!input) {
-    return nullptr;
-  }
-
   switch (input->which()) {
     case cros_healthd::mojom::BatteryResult::Tag::BATTERY_INFO:
       return health::mojom::BatteryResult::NewBatteryInfo(
-          Convert(std::move(input->get_battery_info())));
+          ConvertPtr(std::move(input->get_battery_info())));
     case cros_healthd::mojom::BatteryResult::Tag::ERROR:
       return health::mojom::BatteryResult::NewError(
-          Convert(std::move(input->get_error())));
+          ConvertPtr(std::move(input->get_error())));
   }
   NOTREACHED();
 }
 
-health::mojom::NonRemovableBlockDeviceInfoPtr Convert(
+health::mojom::NonRemovableBlockDeviceInfoPtr UncheckedConvertPtr(
     cros_healthd::mojom::NonRemovableBlockDeviceInfoPtr input) {
   return health::mojom::NonRemovableBlockDeviceInfo::New(
       std::move(input->path), Convert(input->size), std::move(input->type),
@@ -119,15 +80,11 @@ health::mojom::NonRemovableBlockDeviceInfoPtr Convert(
       Convert(input->read_time_seconds_since_last_boot),
       Convert(input->write_time_seconds_since_last_boot),
       Convert(input->io_time_seconds_since_last_boot),
-      Convert(std::move(input->discard_time_seconds_since_last_boot)));
+      ConvertPtr(std::move(input->discard_time_seconds_since_last_boot)));
 }
 
-health::mojom::NonRemovableBlockDeviceResultPtr Convert(
+health::mojom::NonRemovableBlockDeviceResultPtr UncheckedConvertPtr(
     cros_healthd::mojom::NonRemovableBlockDeviceResultPtr input) {
-  if (!input) {
-    return nullptr;
-  }
-
   switch (input->which()) {
     case cros_healthd::mojom::NonRemovableBlockDeviceResult::Tag::
         BLOCK_DEVICE_INFO:
@@ -136,47 +93,65 @@ health::mojom::NonRemovableBlockDeviceResultPtr Convert(
               std::move(input->get_block_device_info())));
     case cros_healthd::mojom::NonRemovableBlockDeviceResult::Tag::ERROR:
       return health::mojom::NonRemovableBlockDeviceResult::NewError(
-          Convert(std::move(input->get_error())));
+          ConvertPtr(std::move(input->get_error())));
   }
   NOTREACHED();
 }
 
-health::mojom::CachedVpdInfoPtr Convert(
+health::mojom::CachedVpdInfoPtr UncheckedConvertPtr(
     cros_healthd::mojom::CachedVpdInfoPtr input) {
-  if (!input) {
-    return nullptr;
-  }
-
   return health::mojom::CachedVpdInfo::New(std::move(input->sku_number));
 }
 
-health::mojom::CachedVpdResultPtr Convert(
+health::mojom::CachedVpdResultPtr UncheckedConvertPtr(
     cros_healthd::mojom::CachedVpdResultPtr input) {
-  if (!input) {
-    return nullptr;
-  }
-
   switch (input->which()) {
     case cros_healthd::mojom::CachedVpdResult::Tag::VPD_INFO:
       return health::mojom::CachedVpdResult::NewVpdInfo(
-          Convert(std::move(input->get_vpd_info())));
+          ConvertPtr(std::move(input->get_vpd_info())));
     case cros_healthd::mojom::CachedVpdResult::Tag::ERROR:
       return health::mojom::CachedVpdResult::NewError(
-          Convert(std::move(input->get_error())));
+          ConvertPtr(std::move(input->get_error())));
   }
   NOTREACHED();
 }
 
-health::mojom::TelemetryInfoPtr Convert(
+health::mojom::TelemetryInfoPtr UncheckedConvertPtr(
     cros_healthd::mojom::TelemetryInfoPtr input) {
-  if (!input) {
-    return nullptr;
+  return health::mojom::TelemetryInfo::New(
+      ConvertPtr(std::move(input->battery_result)),
+      ConvertPtr(std::move(input->block_device_result)),
+      ConvertPtr(std::move(input->vpd_result)));
+}
+
+}  // namespace unchecked
+
+health::mojom::ErrorType Convert(cros_healthd::mojom::ErrorType input) {
+  switch (input) {
+    case cros_healthd::mojom::ErrorType::kFileReadError:
+      return health::mojom::ErrorType::kFileReadError;
+    case cros_healthd::mojom::ErrorType::kParseError:
+      return health::mojom::ErrorType::kParseError;
+    case cros_healthd::mojom::ErrorType::kSystemUtilityError:
+      return health::mojom::ErrorType::kSystemUtilityError;
   }
+  NOTREACHED();
+}
 
-  return health::mojom::TelemetryInfo::New(
-      Convert(std::move(input->battery_result)),
-      Convert(std::move(input->block_device_result)),
-      Convert(std::move(input->vpd_result)));
+health::mojom::DoubleValuePtr Convert(double input) {
+  return health::mojom::DoubleValue::New(input);
+}
+
+health::mojom::Int64ValuePtr Convert(int64_t input) {
+  return health::mojom::Int64Value::New(input);
+}
+
+health::mojom::UInt32ValuePtr Convert(uint32_t input) {
+  return health::mojom::UInt32Value::New(input);
+}
+
+health::mojom::UInt64ValuePtr Convert(uint64_t input) {
+  return health::mojom::UInt64Value::New(input);
 }
 
 std::vector<cros_healthd::mojom::ProbeCategoryEnum> ConvertCategoryVector(

chromeos/components/telemetry_extension_ui/probe_service_converters.h

@@ -22,48 +22,63 @@ namespace probe_service_converters {
 // This file contains helper functions used by ProbeService to convert its
 // types to/from cros_healthd ProbeService types.
 
-health::mojom::ErrorType Convert(cros_healthd::mojom::ErrorType type);
-
-health::mojom::ProbeErrorPtr Convert(cros_healthd::mojom::ProbeErrorPtr input);
-
-health::mojom::DoubleValuePtr Convert(double input);
+namespace unchecked {
 
-health::mojom::Int64ValuePtr Convert(int64_t input);
+// Functions in unchecked namespace do not verify whether input pointer is
+// nullptr, they should be called only via ConvertPtr wrapper that checks
+// whether input pointer is nullptr.
 
-health::mojom::UInt32ValuePtr Convert(uint32_t input);
+health::mojom::ProbeErrorPtr UncheckedConvertPtr(
+    cros_healthd::mojom::ProbeErrorPtr input);
 
-health::mojom::UInt64ValuePtr Convert(uint64_t input);
-
-health::mojom::UInt64ValuePtr Convert(
+health::mojom::UInt64ValuePtr UncheckedConvertPtr(
     cros_healthd::mojom::UInt64ValuePtr input);
 
-health::mojom::BatteryInfoPtr Convert(
+health::mojom::BatteryInfoPtr UncheckedConvertPtr(
     cros_healthd::mojom::BatteryInfoPtr input);
 
-health::mojom::BatteryResultPtr Convert(
+health::mojom::BatteryResultPtr UncheckedConvertPtr(
     cros_healthd::mojom::BatteryResultPtr input);
 
-health::mojom::NonRemovableBlockDeviceInfoPtr Convert(
+health::mojom::NonRemovableBlockDeviceInfoPtr UncheckedConvertPtr(
     cros_healthd::mojom::NonRemovableBlockDeviceInfoPtr input);
 
-health::mojom::NonRemovableBlockDeviceResultPtr Convert(
+health::mojom::NonRemovableBlockDeviceResultPtr UncheckedConvertPtr(
     cros_healthd::mojom::NonRemovableBlockDeviceResultPtr input);
 
-health::mojom::CachedVpdInfoPtr Convert(
+health::mojom::CachedVpdInfoPtr UncheckedConvertPtr(
     cros_healthd::mojom::CachedVpdInfoPtr input);
 
-health::mojom::CachedVpdResultPtr Convert(
+health::mojom::CachedVpdResultPtr UncheckedConvertPtr(
     cros_healthd::mojom::CachedVpdResultPtr input);
 
-health::mojom::TelemetryInfoPtr Convert(
+health::mojom::TelemetryInfoPtr UncheckedConvertPtr(
     cros_healthd::mojom::TelemetryInfoPtr input);
 
+}  // namespace unchecked
+
+health::mojom::ErrorType Convert(cros_healthd::mojom::ErrorType type);
+
+health::mojom::DoubleValuePtr Convert(double input);
+
+health::mojom::Int64ValuePtr Convert(int64_t input);
+
+health::mojom::UInt32ValuePtr Convert(uint32_t input);
+
+health::mojom::UInt64ValuePtr Convert(uint64_t input);
+
+template <class InputT>
+auto ConvertPtr(InputT input) {
+  return (!input.is_null()) ? unchecked::UncheckedConvertPtr(std::move(input))
+                            : nullptr;
+}
+
 template <class OutputT, class InputT>
 std::vector<OutputT> ConvertPtrVector(std::vector<InputT> input) {
   std::vector<OutputT> output;
   for (auto&& element : input) {
     DCHECK(!element.is_null());
-    output.push_back(Convert(std::move(element)));
+    output.push_back(unchecked::UncheckedConvertPtr(std::move(element)));
   }
   return output;
 }

chromeos/components/telemetry_extension_ui/probe_service_converters_unittest.cc

@@ -30,6 +30,12 @@ TEST(ProbeServiceConvertors, ConvertCategoryVector) {
           cros_healthd::mojom::ProbeCategoryEnum::kCachedVpdData));
 }
 
+// Tests that |ConvertPtr| function returns nullptr if input is nullptr.
+// ConvertPtr is a template, so we can test this function with any valid type.
+TEST(ProbeServiceConvertors, ConvertPtrTakesNullPtr) {
+  EXPECT_TRUE(ConvertPtr(cros_healthd::mojom::ProbeErrorPtr()).is_null());
+}
+
 TEST(ProbeServiceConvertors, ErrorType) {
   EXPECT_EQ(Convert(cros_healthd::mojom::ErrorType::kFileReadError),
             health::mojom::ErrorType::kFileReadError);
@@ -41,13 +47,9 @@ TEST(ProbeServiceConvertors, ErrorType) {
             health::mojom::ErrorType::kSystemUtilityError);
 }
 
-TEST(ProbeServiceConvertors, ProbeErrorPtrNull) {
-  EXPECT_TRUE(Convert(cros_healthd::mojom::ProbeErrorPtr()).is_null());
-}
-
 TEST(ProbeServiceConvertors, ProbeErrorPtr) {
   constexpr char kMsg[] = "file not found";
-  EXPECT_EQ(Convert(cros_healthd::mojom::ProbeError::New(
+  EXPECT_EQ(ConvertPtr(cros_healthd::mojom::ProbeError::New(
                 cros_healthd::mojom::ErrorType::kFileReadError, kMsg)),
             health::mojom::ProbeError::New(
                 health::mojom::ErrorType::kFileReadError, kMsg));
@@ -68,20 +70,12 @@ TEST(ProbeServiceConvertors, UInt64Value) {
   EXPECT_EQ(Convert(kValue), health::mojom::UInt64Value::New(kValue));
 }
 
-TEST(ProbeServiceConvertors, UInt64ValuePtrNull) {
-  EXPECT_TRUE(Convert(cros_healthd::mojom::UInt64ValuePtr()).is_null());
-}
-
 TEST(ProbeServiceConvertors, UInt64ValuePtr) {
   constexpr uint64_t kValue = 100500;
-  EXPECT_EQ(Convert(cros_healthd::mojom::UInt64Value::New(kValue)),
+  EXPECT_EQ(ConvertPtr(cros_healthd::mojom::UInt64Value::New(kValue)),
             health::mojom::UInt64Value::New(kValue));
 }
 
-TEST(ProbeServiceConvertors, BatteryInfoPtrNull) {
-  EXPECT_TRUE(Convert(cros_healthd::mojom::BatteryInfoPtr()).is_null());
-}
-
 TEST(ProbeServiceConvertors, BatteryInfoPtr) {
   constexpr int64_t kCycleCount = 512;
   constexpr double kVoltageNow = 10.2;
@@ -120,7 +114,7 @@ TEST(ProbeServiceConvertors, BatteryInfoPtr) {
   // Here we intentionaly use health::mojom::BatteryInfo::New not to
   // forget to test new fields.
   EXPECT_EQ(
-      Convert(battery_info.Clone()),
+      ConvertPtr(battery_info.Clone()),
       health::mojom::BatteryInfo::New(
           health::mojom::Int64Value::New(kCycleCount),
           health::mojom::DoubleValue::New(kVoltageNow), kVendor, kSerialNumber,
@@ -132,20 +126,16 @@ TEST(ProbeServiceConvertors, BatteryInfoPtr) {
           kManufactureDate, health::mojom::UInt64Value::New(kTemperature)));
 }
 
-TEST(ProbeServiceConvertors, BatteryResultPtrNull) {
-  EXPECT_TRUE(Convert(cros_healthd::mojom::BatteryResultPtr()).is_null());
-}
-
 TEST(ProbeServiceConvertors, BatteryResultPtrInfo) {
   const health::mojom::BatteryResultPtr ptr =
-      Convert(cros_healthd::mojom::BatteryResult::NewBatteryInfo(nullptr));
+      ConvertPtr(cros_healthd::mojom::BatteryResult::NewBatteryInfo(nullptr));
   ASSERT_TRUE(ptr);
   EXPECT_TRUE(ptr->is_battery_info());
 }
 
 TEST(ProbeServiceConvertors, BatteryResultPtrError) {
   const health::mojom::BatteryResultPtr ptr =
-      Convert(cros_healthd::mojom::BatteryResult::NewError(nullptr));
+      ConvertPtr(cros_healthd::mojom::BatteryResult::NewError(nullptr));
   ASSERT_TRUE(ptr);
   EXPECT_TRUE(ptr->is_error());
 }
@@ -186,7 +176,7 @@ TEST(ProbeServiceConvertors, NonRemovableBlockDeviceInfoPtr) {
   // Here we intentionaly use health::mojom::NonRemovableBlockDeviceInfo::New
   // not to forget to test new fields.
   EXPECT_EQ(
-      Convert(info.Clone()),
+      ConvertPtr(info.Clone()),
       health::mojom::NonRemovableBlockDeviceInfo::New(
           kPath, health::mojom::UInt64Value::New(kSize), kType,
           health::mojom::UInt32Value::New(kManufacturerId), kName,
@@ -199,11 +189,6 @@ TEST(ProbeServiceConvertors, NonRemovableBlockDeviceInfoPtr) {
           health::mojom::UInt64Value::New(kDiscardTimeSecondsSinceLastBoot)));
 }
 
-TEST(ProbeServiceConvertors, NonRemovableBlockDeviceResultPtrNull) {
-  EXPECT_TRUE(Convert(cros_healthd::mojom::NonRemovableBlockDeviceResultPtr())
-                  .is_null());
-}
-
 TEST(ProbeServiceConvertors, NonRemovableBlockDeviceResultPtrInfo) {
   constexpr char kPath1[] = "Path1";
   constexpr char kPath2[] = "Path2";
@@ -218,7 +203,7 @@ TEST(ProbeServiceConvertors, NonRemovableBlockDeviceResultPtrInfo) {
   infos.push_back(std::move(info1));
   infos.push_back(std::move(info2));
 
-  const health::mojom::NonRemovableBlockDeviceResultPtr ptr = Convert(
+  const health::mojom::NonRemovableBlockDeviceResultPtr ptr = ConvertPtr(
       cros_healthd::mojom::NonRemovableBlockDeviceResult::NewBlockDeviceInfo(
           std::move(infos)));
   ASSERT_TRUE(ptr);
@@ -229,16 +214,12 @@ TEST(ProbeServiceConvertors, NonRemovableBlockDeviceResultPtrInfo) {
 }
 
 TEST(ProbeServiceConvertors, NonRemovableBlockDeviceResultPtrError) {
-  const health::mojom::NonRemovableBlockDeviceResultPtr ptr = Convert(
+  const health::mojom::NonRemovableBlockDeviceResultPtr ptr = ConvertPtr(
       cros_healthd::mojom::NonRemovableBlockDeviceResult::NewError(nullptr));
   ASSERT_TRUE(ptr);
   EXPECT_TRUE(ptr->is_error());
 }
 
-TEST(ProbeServiceConvertors, CachedVpdInfoPtrNull) {
-  EXPECT_TRUE(Convert(cros_healthd::mojom::CachedVpdInfoPtr()).is_null());
-}
-
 TEST(ProbeServiceConvertors, CachedVpdInfoPtr) {
   constexpr char kSkuNumber[] = "sku-1";
 
@@ -251,24 +232,20 @@ TEST(ProbeServiceConvertors, CachedVpdInfoPtr) {
 
   // Here we intentionaly use health::mojom::CachedVpdInfo::New
   // not to forget to test new fields.
-  EXPECT_EQ(Convert(info.Clone()),
+  EXPECT_EQ(ConvertPtr(info.Clone()),
             health::mojom::CachedVpdInfo::New(kSkuNumber));
 }
 
-TEST(ProbeServiceConvertors, CachedVpdResultPtrNull) {
-  EXPECT_TRUE(Convert(cros_healthd::mojom::CachedVpdResultPtr()).is_null());
-}
-
 TEST(ProbeServiceConvertors, CachedVpdResultPtrInfo) {
   const health::mojom::CachedVpdResultPtr ptr =
-      Convert(cros_healthd::mojom::CachedVpdResult::NewVpdInfo(nullptr));
+      ConvertPtr(cros_healthd::mojom::CachedVpdResult::NewVpdInfo(nullptr));
   ASSERT_TRUE(ptr);
   EXPECT_TRUE(ptr->is_vpd_info());
 }
 
 TEST(ProbeServiceConvertors, CachedVpdResultPtrError) {
   const health::mojom::CachedVpdResultPtr ptr =
-      Convert(cros_healthd::mojom::CachedVpdResult::NewError(nullptr));
+      ConvertPtr(cros_healthd::mojom::CachedVpdResult::NewError(nullptr));
   ASSERT_TRUE(ptr);
   EXPECT_TRUE(ptr->is_error());
 }
@@ -286,7 +263,7 @@ TEST(ProbeServiceConvertors, TelemetryInfoPtrHasBatteryResult) {
           std::move(battery_info_input));
 
   const health::mojom::TelemetryInfoPtr telemetry_info_output =
-      Convert(std::move(telemetry_info_input));
+      ConvertPtr(std::move(telemetry_info_input));
   ASSERT_TRUE(telemetry_info_output);
   ASSERT_TRUE(telemetry_info_output->battery_result);
   ASSERT_TRUE(telemetry_info_output->battery_result->is_battery_info());
@@ -312,7 +289,7 @@ TEST(ProbeServiceConvertors, TelemetryInfoPtrHasBlockDeviceResult) {
       cros_healthd::mojom::NonRemovableBlockDeviceResult::NewBlockDeviceInfo(
           std::move(device_infos));
 
-  const health::mojom::TelemetryInfoPtr output = Convert(std::move(input));
+  const health::mojom::TelemetryInfoPtr output = ConvertPtr(std::move(input));
   ASSERT_TRUE(output);
   ASSERT_TRUE(output->block_device_result);
   ASSERT_TRUE(output->block_device_result->is_block_device_info());
@@ -338,7 +315,7 @@ TEST(ProbeServiceConvertors, TelemetryInfoPtrHasCachedVpdResult) {
           std::move(vpd_info_input));
 
   const health::mojom::TelemetryInfoPtr telemetry_info_output =
-      Convert(std::move(telemetry_info_input));
+      ConvertPtr(std::move(telemetry_info_input));
   ASSERT_TRUE(telemetry_info_output);
   ASSERT_TRUE(telemetry_info_output->vpd_result);
   ASSERT_TRUE(telemetry_info_output->vpd_result->is_vpd_info());
@@ -352,16 +329,12 @@ TEST(ProbeServiceConvertors, TelemetryInfoPtrHasCachedVpdResult) {
 
 TEST(ProbeServiceConvertors, TelemetryInfoPtrWithNullFields) {
   const health::mojom::TelemetryInfoPtr telemetry_info_output =
-      Convert(cros_healthd::mojom::TelemetryInfo::New());
+      ConvertPtr(cros_healthd::mojom::TelemetryInfo::New());
   ASSERT_TRUE(telemetry_info_output);
   EXPECT_FALSE(telemetry_info_output->battery_result);
   EXPECT_FALSE(telemetry_info_output->block_device_result);
   EXPECT_FALSE(telemetry_info_output->vpd_result);
 }
 
-TEST(ProbeServiceConvertors, TelemetryInfoPtrNull) {
-  EXPECT_TRUE(Convert(cros_healthd::mojom::TelemetryInfoPtr()).is_null());
-}
-
 }  // namespace probe_service_converters
 }  // namespace chromeos