Skip to content

Commit

Permalink
Remove JSON codec from C++ client wrapper (flutter#17312)
Browse files Browse the repository at this point in the history
The JSON codec is awkward to use in the wrapper (since the client has to build and link one of the JSON libraries to do so). Since it would be very cumbersome to wrap in a C API, and there's essentially no reason to use it instead of the standard codec, this removes it from the wrapper entirely.

Since some system channels (internal to the engine) still use it, it's moved into common/cpp instead of being eliminated entirely. Internally we always use RapidJSON though, so the jsoncpp implementation is removed. Also adds some unit test coverage, since there wasn't any.

Fixes flutter#30669
  • Loading branch information
stuartmorgan authored Apr 2, 2020
1 parent ff62dec commit 08ae3bb
Show file tree
Hide file tree
Showing 21 changed files with 239 additions and 147 deletions.
1 change: 1 addition & 0 deletions BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ group("flutter") {
"//flutter/runtime:runtime_unittests",
"//flutter/shell/common:shell_unittests",
"//flutter/shell/platform/common/cpp:common_cpp_core_unittests",
"//flutter/shell/platform/common/cpp:common_cpp_unittests",
"//flutter/shell/platform/common/cpp/client_wrapper:client_wrapper_unittests",
"//flutter/shell/platform/embedder:embedder_unittests",
"//flutter/shell/platform/glfw/client_wrapper:client_wrapper_glfw_unittests",
Expand Down
11 changes: 6 additions & 5 deletions ci/licenses_golden/licenses_flutter
Original file line number Diff line number Diff line change
Expand Up @@ -778,9 +778,6 @@ FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/include/flutter/
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/include/flutter/binary_messenger.h
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/include/flutter/encodable_value.h
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/include/flutter/engine_method_result.h
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/include/flutter/json_message_codec.h
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/include/flutter/json_method_codec.h
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/include/flutter/json_type.h
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/include/flutter/message_codec.h
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/include/flutter/method_call.h
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/include/flutter/method_channel.h
Expand All @@ -790,8 +787,6 @@ FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/include/flutter/
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/include/flutter/plugin_registry.h
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/include/flutter/standard_message_codec.h
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/include/flutter/standard_method_codec.h
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/json_message_codec.cc
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/json_method_codec.cc
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/method_call_unittests.cc
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/method_channel_unittests.cc
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/plugin_registrar.cc
Expand All @@ -802,6 +797,12 @@ FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/standard_message
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/standard_method_codec_unittests.cc
FILE: ../../../flutter/shell/platform/common/cpp/incoming_message_dispatcher.cc
FILE: ../../../flutter/shell/platform/common/cpp/incoming_message_dispatcher.h
FILE: ../../../flutter/shell/platform/common/cpp/json_message_codec.cc
FILE: ../../../flutter/shell/platform/common/cpp/json_message_codec.h
FILE: ../../../flutter/shell/platform/common/cpp/json_message_codec_unittests.cc
FILE: ../../../flutter/shell/platform/common/cpp/json_method_codec.cc
FILE: ../../../flutter/shell/platform/common/cpp/json_method_codec.h
FILE: ../../../flutter/shell/platform/common/cpp/json_method_codec_unittests.cc
FILE: ../../../flutter/shell/platform/common/cpp/path_utils.cc
FILE: ../../../flutter/shell/platform/common/cpp/path_utils.h
FILE: ../../../flutter/shell/platform/common/cpp/path_utils_unittests.cc
Expand Down
34 changes: 28 additions & 6 deletions shell/platform/common/cpp/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,17 @@ source_set("common_cpp_library_headers") {
source_set("common_cpp") {
public = [
"incoming_message_dispatcher.h",
"json_message_codec.h",
"json_method_codec.h",
"text_input_model.h",
]

# TODO: Refactor flutter_glfw.cc to move the implementations corresponding
# to the _public_headers above into this target.
sources = [
"incoming_message_dispatcher.cc",
"json_message_codec.cc",
"json_method_codec.cc",
"text_input_model.cc",
]

Expand All @@ -53,16 +57,12 @@ source_set("common_cpp") {

public_deps = [
":common_cpp_core",
"//third_party/rapidjson",
]

# TODO: Remove once text input model refactor lands, at which point this code
# won't have a JSON dependency.
defines = [ "USE_RAPID_JSON" ]
deps += [ "//third_party/rapidjson" ]

if (is_win) {
# For wstring_conversion. See issue #50053.
defines += [ "_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING" ]
defines = [ "_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING" ]
}
}

Expand Down Expand Up @@ -101,6 +101,28 @@ executable("common_cpp_core_unittests") {
public_configs = [ "//flutter:config" ]
}

test_fixtures("common_cpp_fixtures") {
fixtures = []
}

executable("common_cpp_unittests") {
testonly = true

sources = [
"json_message_codec_unittests.cc",
"json_method_codec_unittests.cc",
]

deps = [
":common_cpp",
":common_cpp_fixtures",
"//flutter/shell/platform/common/cpp/client_wrapper:client_wrapper_library_stubs",
"//flutter/testing",
]

public_configs = [ "//flutter:config" ]
}

copy("publish_headers") {
sources = _public_headers
outputs = [
Expand Down
3 changes: 0 additions & 3 deletions shell/platform/common/cpp/client_wrapper/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,8 @@ source_set("client_wrapper") {

deps = [
"//flutter/shell/platform/common/cpp:common_cpp_library_headers",
"//third_party/rapidjson",
]

defines = [ "USE_RAPID_JSON" ]

configs +=
[ "//flutter/shell/platform/common/cpp:desktop_library_implementation" ]

Expand Down
23 changes: 8 additions & 15 deletions shell/platform/common/cpp/client_wrapper/core_wrapper_files.gni
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,6 @@ core_cpp_client_wrapper_includes =
"include/flutter/binary_messenger.h",
"include/flutter/encodable_value.h",
"include/flutter/engine_method_result.h",
"include/flutter/json_message_codec.h",
"include/flutter/json_method_codec.h",
"include/flutter/json_type.h",
"include/flutter/message_codec.h",
"include/flutter/method_call.h",
"include/flutter/method_channel.h",
Expand All @@ -26,15 +23,11 @@ core_cpp_client_wrapper_includes =
# TODO: Once the wrapper API is more stable, consolidate to as few files as is
# reasonable (without forcing different kinds of clients to take unnecessary
# code) to simplify use.
core_cpp_client_wrapper_sources =
get_path_info(
[
"byte_stream_wrappers.h",
"engine_method_result.cc",
"json_message_codec.cc", # TODO combine into a single json_codec.cc.
"json_method_codec.cc", # TODO combine into a single json_codec.cc.
"plugin_registrar.cc",
"standard_codec_serializer.h",
"standard_codec.cc",
],
"abspath")
core_cpp_client_wrapper_sources = get_path_info([
"byte_stream_wrappers.h",
"engine_method_result.cc",
"plugin_registrar.cc",
"standard_codec_serializer.h",
"standard_codec.cc",
],
"abspath")

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,14 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "include/flutter/json_message_codec.h"
#include "flutter/shell/platform/common/cpp/json_message_codec.h"

#include <iostream>
#include <string>

#ifdef USE_RAPID_JSON
#include "rapidjson/error/en.h"
#include "rapidjson/stringbuffer.h"
#include "rapidjson/writer.h"
#endif

namespace flutter {

Expand All @@ -22,46 +20,28 @@ const JsonMessageCodec& JsonMessageCodec::GetInstance() {
}

std::unique_ptr<std::vector<uint8_t>> JsonMessageCodec::EncodeMessageInternal(
const JsonValueType& message) const {
#ifdef USE_RAPID_JSON
const rapidjson::Document& message) const {
// TODO: Look into alternate writers that would avoid the buffer copy.
rapidjson::StringBuffer buffer;
rapidjson::Writer<rapidjson::StringBuffer> writer(buffer);
message.Accept(writer);
const char* buffer_start = buffer.GetString();
return std::make_unique<std::vector<uint8_t>>(
buffer_start, buffer_start + buffer.GetSize());
#else
Json::StreamWriterBuilder writer_builder;
std::string serialization = Json::writeString(writer_builder, message);
return std::make_unique<std::vector<uint8_t>>(serialization.begin(),
serialization.end());
#endif
}

std::unique_ptr<JsonValueType> JsonMessageCodec::DecodeMessageInternal(
std::unique_ptr<rapidjson::Document> JsonMessageCodec::DecodeMessageInternal(
const uint8_t* binary_message,
const size_t message_size) const {
auto raw_message = reinterpret_cast<const char*>(binary_message);
auto json_message = std::make_unique<JsonValueType>();
std::string parse_errors;
bool parsing_successful = false;
#ifdef USE_RAPID_JSON
auto json_message = std::make_unique<rapidjson::Document>();
rapidjson::ParseResult result =
json_message->Parse(raw_message, message_size);
parsing_successful = result == rapidjson::ParseErrorCode::kParseErrorNone;
if (!parsing_successful) {
parse_errors = rapidjson::GetParseError_En(result.Code());
}
#else
Json::CharReaderBuilder reader_builder;
std::unique_ptr<Json::CharReader> parser(reader_builder.newCharReader());
parsing_successful = parser->parse(raw_message, raw_message + message_size,
json_message.get(), &parse_errors);
#endif
bool parsing_successful =
result == rapidjson::ParseErrorCode::kParseErrorNone;
if (!parsing_successful) {
std::cerr << "Unable to parse JSON message:" << std::endl
<< parse_errors << std::endl;
<< rapidjson::GetParseError_En(result.Code()) << std::endl;
return nullptr;
}
return json_message;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,18 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#ifndef FLUTTER_SHELL_PLATFORM_COMMON_CPP_CLIENT_WRAPPER_INCLUDE_FLUTTER_JSON_MESSAGE_CODEC_H_
#define FLUTTER_SHELL_PLATFORM_COMMON_CPP_CLIENT_WRAPPER_INCLUDE_FLUTTER_JSON_MESSAGE_CODEC_H_
#ifndef FLUTTER_SHELL_PLATFORM_COMMON_CPP_JSON_MESSAGE_CODEC_H_
#define FLUTTER_SHELL_PLATFORM_COMMON_CPP_JSON_MESSAGE_CODEC_H_

#include "json_type.h"
#include "message_codec.h"
#include <rapidjson/document.h>

#include "flutter/shell/platform/common/cpp/client_wrapper/include/flutter/message_codec.h"

namespace flutter {

// A message encoding/decoding mechanism for communications to/from the
// Flutter engine via JSON channels.
class JsonMessageCodec : public MessageCodec<JsonValueType> {
class JsonMessageCodec : public MessageCodec<rapidjson::Document> {
public:
// Returns the shared instance of the codec.
static const JsonMessageCodec& GetInstance();
Expand All @@ -28,15 +29,15 @@ class JsonMessageCodec : public MessageCodec<JsonValueType> {
JsonMessageCodec() = default;

// |flutter::MessageCodec|
std::unique_ptr<JsonValueType> DecodeMessageInternal(
std::unique_ptr<rapidjson::Document> DecodeMessageInternal(
const uint8_t* binary_message,
const size_t message_size) const override;

// |flutter::MessageCodec|
std::unique_ptr<std::vector<uint8_t>> EncodeMessageInternal(
const JsonValueType& message) const override;
const rapidjson::Document& message) const override;
};

} // namespace flutter

#endif // FLUTTER_SHELL_PLATFORM_COMMON_CPP_CLIENT_WRAPPER_INCLUDE_FLUTTER_JSON_MESSAGE_CODEC_H_
#endif // FLUTTER_SHELL_PLATFORM_COMMON_CPP_JSON_MESSAGE_CODEC_H_
46 changes: 46 additions & 0 deletions shell/platform/common/cpp/json_message_codec_unittests.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
// Copyright 2013 The Flutter 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 "flutter/shell/platform/common/cpp/json_message_codec.h"

#include <limits>
#include <map>
#include <vector>

#include "gtest/gtest.h"

namespace flutter {

namespace {

// Validates round-trip encoding and decoding of |value|.
static void CheckEncodeDecode(const rapidjson::Document& value) {
const JsonMessageCodec& codec = JsonMessageCodec::GetInstance();
auto encoded = codec.EncodeMessage(value);
ASSERT_TRUE(encoded);
auto decoded = codec.DecodeMessage(*encoded);
EXPECT_EQ(value, *decoded);
}

} // namespace

// Tests that a JSON document with various data types round-trips correctly.
TEST(JsonMessageCodec, EncodeDecode) {
rapidjson::Document array(rapidjson::kArrayType);
auto& allocator = array.GetAllocator();

array.PushBack("string", allocator);

rapidjson::Value map(rapidjson::kObjectType);
map.AddMember("a", -7, allocator);
map.AddMember("b", std::numeric_limits<int>::max(), allocator);
map.AddMember("c", 3.14159, allocator);
map.AddMember("d", true, allocator);
map.AddMember("e", rapidjson::Value(), allocator);
array.PushBack(map, allocator);

CheckEncodeDecode(array);
}

} // namespace flutter
Loading

0 comments on commit 08ae3bb

Please sign in to comment.