Fix the legacy EncodableValue codepaths (flutter#20501)

A recent refactoring broke the USE_LEGACY_ENCODABLE_VALUE codepath in standard_codec.cc, which went unnoticed since it wasn't being compiled. This fixes the breakage, and also adds a temporary minimal unit test target that ensures that all the USE_LEGACY_ENCODABLE_VALUE paths are being compiled.
engine-flutter-autoroll · Aug 14, 2020 · 84aa365 · 84aa365
1 parent d4f6100
commit 84aa365
Show file tree

Hide file tree

Showing 4 changed files with 83 additions and 5 deletions.
diff --git a/shell/platform/common/cpp/client_wrapper/BUILD.gn b/shell/platform/common/cpp/client_wrapper/BUILD.gn
@@ -19,6 +19,23 @@ source_set("client_wrapper") {
       [ "//flutter/shell/platform/common/cpp:relative_flutter_library_headers" ]
 }
 
+# Temporary test for the legacy EncodableValue implementation. Remove once the
+# legacy version is removed.
+source_set("client_wrapper_legacy_encodable_value") {
+  sources = core_cpp_client_wrapper_sources
+  public = core_cpp_client_wrapper_includes
+
+  deps = [ "//flutter/shell/platform/common/cpp:common_cpp_library_headers" ]
+
+  configs +=
+      [ "//flutter/shell/platform/common/cpp:desktop_library_implementation" ]
+
+  defines = [ "USE_LEGACY_ENCODABLE_VALUE" ]
+
+  public_configs =
+      [ "//flutter/shell/platform/common/cpp:relative_flutter_library_headers" ]
+}
+
 source_set("client_wrapper_library_stubs") {
   sources = [
     "testing/stub_flutter_api.cc",
@@ -56,6 +73,9 @@ executable("client_wrapper_unittests") {
     ":client_wrapper",
     ":client_wrapper_fixtures",
     ":client_wrapper_library_stubs",
+
+    # Build the legacy version as well as a sanity check.
+    ":client_wrapper_unittests_legacy_encodable_value",
     "//flutter/testing",
 
     # TODO(chunhtai): Consider refactoring flutter_root/testing so that there's a testing
@@ -66,3 +86,31 @@ executable("client_wrapper_unittests") {
 
   defines = [ "FLUTTER_DESKTOP_LIBRARY" ]
 }
+
+# Ensures that the legacy EncodableValue codepath still compiles.
+executable("client_wrapper_unittests_legacy_encodable_value") {
+  testonly = true
+
+  sources = [
+    "encodable_value_unittests.cc",
+    "standard_message_codec_unittests.cc",
+    "testing/test_codec_extensions.h",
+  ]
+
+  deps = [
+    ":client_wrapper_fixtures",
+    ":client_wrapper_legacy_encodable_value",
+    ":client_wrapper_library_stubs",
+    "//flutter/testing",
+
+    # TODO(chunhtai): Consider refactoring flutter_root/testing so that there's a testing
+    # target that doesn't require a Dart runtime to be linked in.
+    # https://github.com/flutter/flutter/issues/41414.
+    "//third_party/dart/runtime:libdart_jit",
+  ]
+
+  defines = [
+    "FLUTTER_DESKTOP_LIBRARY",
+    "USE_LEGACY_ENCODABLE_VALUE",
+  ]
+}
diff --git a/shell/platform/common/cpp/client_wrapper/encodable_value_unittests.cc b/shell/platform/common/cpp/client_wrapper/encodable_value_unittests.cc
@@ -15,6 +15,8 @@ TEST(EncodableValueTest, Null) {
   value.IsNull();
 }
 
+#ifndef USE_LEGACY_ENCODABLE_VALUE
+
 TEST(EncodableValueTest, Bool) {
   EncodableValue value(false);
 
@@ -280,4 +282,6 @@ TEST(EncodableValueTest, DeepCopy) {
   EXPECT_EQ(std::get<std::string>(innermost_map[EncodableValue("a")]), "b");
 }
 
+#endif  // !LEGACY_ENCODABLE_VALUE
+
 }  // namespace flutter
diff --git a/shell/platform/common/cpp/client_wrapper/standard_codec.cc b/shell/platform/common/cpp/client_wrapper/standard_codec.cc
@@ -132,14 +132,14 @@ void StandardCodecSerializer::WriteValue(const EncodableValue& value,
       // Null and bool are encoded directly in the type.
       break;
     case EncodableValue::Type::kInt:
-      stream->WriteInt32(std::get<int32_t>(value));
+      stream->WriteInt32(value.IntValue());
       break;
-    case case EncodableValue::Type::kLong:
-      stream->WriteInt64(std::get<int64_t>(value));
+    case EncodableValue::Type::kLong:
+      stream->WriteInt64(value.LongValue());
       break;
     case EncodableValue::Type::kDouble:
       stream->WriteAlignment(8);
-      stream->WriteDouble(std::get<double>(value));
+      stream->WriteDouble(value.DoubleValue());
       break;
     case EncodableValue::Type::kString: {
       const auto& string_value = value.StringValue();

diff --git a/shell/platform/common/cpp/client_wrapper/standard_message_codec_unittests.cc b/shell/platform/common/cpp/client_wrapper/standard_message_codec_unittests.cc
@@ -6,9 +6,12 @@
 #include <vector>
 
 #include "flutter/shell/platform/common/cpp/client_wrapper/include/flutter/standard_message_codec.h"
-#include "flutter/shell/platform/common/cpp/client_wrapper/testing/test_codec_extensions.h"
 #include "gtest/gtest.h"
 
+#ifndef USE_LEGACY_ENCODABLE_VALUE
+#include "flutter/shell/platform/common/cpp/client_wrapper/testing/test_codec_extensions.h"
+#endif
+
 namespace flutter {
 
 // Validates round-trip encoding and decoding of |value|, and checks that the
@@ -30,11 +33,21 @@ static void CheckEncodeDecode(
   EXPECT_EQ(*encoded, expected_encoding);
 
   auto decoded = codec.DecodeMessage(*encoded);
+#ifdef USE_LEGACY_ENCODABLE_VALUE
+  // Full equality isn't implemented for the legacy path; just do a sanity test
+  // of basic types.
+  if (value.IsNull() || value.IsBool() || value.IsInt() || value.IsLong() ||
+      value.IsDouble() || value.IsString()) {
+    EXPECT_FALSE(value < *decoded);
+    EXPECT_FALSE(*decoded < value);
+  }
+#else
   if (custom_comparator) {
     EXPECT_TRUE(custom_comparator(value, *decoded));
   } else {
     EXPECT_EQ(value, *decoded);
   }
+#endif
 }
 
 // Validates round-trip encoding and decoding of |value|, and checks that the
@@ -46,7 +59,11 @@ static void CheckEncodeDecodeWithEncodePrefix(
     const EncodableValue& value,
     const std::vector<uint8_t>& expected_encoding_prefix,
     size_t expected_encoding_length) {
+#ifdef USE_LEGACY_ENCODABLE_VALUE
+  EXPECT_TRUE(value.IsMap());
+#else
   EXPECT_TRUE(std::holds_alternative<EncodableMap>(value));
+#endif
   const StandardMessageCodec& codec = StandardMessageCodec::GetInstance();
   auto encoded = codec.EncodeMessage(value);
   ASSERT_TRUE(encoded);
@@ -58,7 +75,12 @@ static void CheckEncodeDecodeWithEncodePrefix(
       expected_encoding_prefix.begin(), expected_encoding_prefix.end()));
 
   auto decoded = codec.DecodeMessage(*encoded);
+
+#ifdef USE_LEGACY_ENCODABLE_VALUE
+  EXPECT_NE(decoded, nullptr);
+#else
   EXPECT_EQ(value, *decoded);
+#endif
 }
 
 TEST(StandardMessageCodec, CanEncodeAndDecodeNull) {
@@ -182,6 +204,8 @@ TEST(StandardMessageCodec, CanEncodeAndDecodeFloat64Array) {
   CheckEncodeDecode(value, bytes);
 }
 
+#ifndef USE_LEGACY_ENCODABLE_VALUE
+
 TEST(StandardMessageCodec, CanEncodeAndDecodeSimpleCustomType) {
   std::vector<uint8_t> bytes = {0x80, 0x09, 0x00, 0x00, 0x00,
                                 0x10, 0x00, 0x00, 0x00};
@@ -217,4 +241,6 @@ TEST(StandardMessageCodec, CanEncodeAndDecodeVariableLengthCustomType) {
                     some_data_comparator);
 }
 
+#endif  // !USE_LEGACY_ENCODABLE_VALUE
+
 }  // namespace flutter