This PR doesn't have the tests completed, as I haven't worked out how to build and run them yet. Please let me know what I should do here! Everything else should be mostly there, so I figured it's worth reviewing now.

Example code of using this:

FlMethodChannel *method_channel;

static void
method_cb (FlMethodChannel *channel, const gchar *method, FlValue *args, FlBinaryMessengerResponseHandle *response_handle, gpointer user_data)
{
    g_autoptr(GError) error = NULL;
    g_autoptr(FlValue) result = fl_value_string_new ("test-response-from-gtk");
    if (!fl_method_channel_respond (channel, response_handle, result, &error))
        g_printerr ("Failed to send method response: %s\n", error->message);
}

static void
method_response_cb (GObject *object, GAsyncResult *result, gpointer user_data)
{
    g_autofree gchar *error_code = NULL;
    g_autofree gchar *error_message = NULL;
    g_autoptr(FlValue) response = NULL;
    g_autoptr(GError) error = NULL;
    if (!fl_method_channel_invoke_finish (FL_METHOD_CHANNEL (object),
                                          result,
                                          &error_code,
                                          &error_message,
                                          &response,
                                          &error)) {
        g_printerr ("Failed to send platform message: %s\n", error->message);
        return;
    }

    if (error_code == NULL)
       handle_success (response);
    else
       handle_failure (error_code, error_message, response);
}

void
some_external_event (gpointer user_data)
{
    fl_method_channel_invoke (method_channel, "Test.test", NULL, NULL, method_response_cb, NULL);
}

int
main (int argc, char **argv)
{
    ...

    g_autoptr(FlDartProject) project = fl_dart_project_new (".");
    view = fl_view_new (project);

    method_channel = fl_method_channel_new (fl_engine_get_binary_messenger (fl_view_get_engine (view)),
                                            "example.com/gtk-test-method",
                                            FL_METHOD_CODEC (fl_standard_method_codec_new ()));
    fl_method_channel_set_callback (method_channel, method_cb, NULL);

    ...
}

Notes:

• I started with the JSON implementation as a placeholder to just get something to work but ended up just finishing it. Obviously we need to consider if this is an appropriate solution. Things in favour of keeping the current implementation:
  • Using the existing rapidjson has issues with header files colliding with GLib ones and so has to be treated carefully to integrate with the existing code. Using something more native like [JSON-GLib|https://wiki.gnome.org/Projects/JsonGlib] requires vetting of that as a dependency.
  • While normally writing your own decoder would be a big security risk, in this case the other side of the channel is trusted (written by Dart) so this is not an issue here (please confirm/deny).
  • Using another JSON parser would require converting to/from their data types and FlValue. The current implementation gets to avoid this.
  • The implementation is quite small and JSON is not a protocol that is likely to change in the future (thanks to the great design of JSON :))
  • We can replace the JSON code later if we decide, as it's not exposed in any way in the API.
• Reasons to replace the JSON code:
  • Obviously there is some maintenance / complexity cost of this.
  • Bugs
• I noticed the darwin code uses "JSON" and the C++ code uses "Json". I've used the latter, but couldn't decide which was better.
• I went with the FlValue objects instead of using GValue or GVariant (similar boxed types). This was because we'd have to handle all the cases where the existing values don't map what we want. FlValue is a pile of boilerplate but I think it will be less errors for the user.
• Decoding method responses (i.e. handling errors) is a bit yucky. I've gone with the best of a bad selection by having the function return both error and success fields and requiring the user to check that code == NULL. Other options considered:
  • Using GError for errors. GError only contains a predefined enum code and a string, so we would lose some of the information from Dart (had similar issues in snapd-glib with JSON errors there).
  • Returning a "FlMethodResult" object. It just adds a bunch of overhead that is a pain in C.
  • Making a special FlValue that is an error. It felt like it was abusing what FlValue was and any FlValue handling code would have to special case this "non-value".
  • Making a GError that indicates an error occurred and requiring a user to get it with a function call. This is generally an issue if you have multiple calls - errors can get mixed up.
  • Using a double function call - fl_method_channel_call_finish () -> { success, error }, fl_method_channel_get_success () / fl_method_channel_get_error (). Has similar issues to the "FlMethodResult" object and the get_error() solution.
• For custom types I figure we add fl_value_custom_new (int id, gpointer data, GDestroyNotify destroy), then the FlStandardCodec has a virtual method to encode/decode those. We can add those later without breaking API.
• I haven't implemented largeint support, but I think we can just add a fl_value_largeint_new for that later.
• FlValueBool/FlValueInt or FlValueBoolean/FlValueInteger. I went with the former for saving typing time, but couldn't decide which one was the correct one (different existing code uses different names). Also similar, Double,Float or Float64?
• I know there are other codecs to implement. I'm haven't investigated in depth what these require, but I think we should be able to add these in later PRs without breaking API. I'll try and get to those sooner rather than later in case we need to make a change.

Neat!

This PR doesn't have the tests completed, as I haven't worked out how to build and run them yet.

Here is an example test runner you can look at:

You can build it with autoninja -C out/$PLATFORM fml_unittests then it is available to run at ./out/$PLATFORM/fml_unittests

I read through your example usage and I have a nit, the signature for the method channel callback has a FlBinaryMessengerResponseHandle as a parameter, and it is used by fl_method_channel_respond. I recommend that that datatype has a name related to method channels, for example: FlMethodChannelResponseHandle.

I noticed the darwin code uses "JSON" and the C++ code uses "Json". I've used the latter, but couldn't decide which was better.

'Json' is the proper naming according to the Google C++ style guide.

Why did you chose to use GAsyncReadyCallback in fl_method_channel_invoke instead of having an apropos callback signature? There is a bit of casting going on there that may be avoided.

This probably deserved a design doc to help you avoid shifting around a big PR. It should have an issue associated with it, too.

Thanks @gaaclarke for the review.

I didn't use FlMethodChannelResponseHandle at the time because it caused a bunch of boilerplate code and I was in two minds if it was worth it. However, as you also noticed the issue I decided to put it in.

GAsyncReadyCallback is the GLib convention for asynchronous callbacks and will be familiar to existing GLib users. It also allows language bindings should we use them in the future to easily wrap these functions in nicer ways.

The issue for this feature is flutter/flutter#54859.

I haven't had time to review yet, but some initial comments on some of the high-level points before I do:

• Using the existing rapidjson has issues with header files colliding with GLib ones and so has to be treated carefully to integrate with the existing code. Using something more native like [JSON-GLib|https://wiki.gnome.org/Projects/JsonGlib] requires vetting of that as a dependency.

Another option would be to see if we could use the library suggested in flutter/flutter#54693. If so, this could be the first stage in a migration away from rapidjson.

• While normally writing your own decoder would be a big security risk, in this case the other side of the channel is trusted (written by Dart) so this is not an issue here (please confirm/deny).

The other side of the system channels is under our control, but the data isn't always. For instance, one of the things we need to implement for alpha using this is the copy/paste channel, which will contain arbitrary strings, so if a custom parser has a bug in string handling that's a serious issue.

• I noticed the darwin code uses "JSON" and the C++ code uses "Json". I've used the latter, but couldn't decide which was better.

Obj-C naming conventions use all caps for acronyms, C++ Google style doesn't. Since these APIs are mostly following GLib conventions, we should use theirs (if there is one; if not we should default to the C++ guide).

• I went with the FlValue objects instead of using GValue or GVariant (similar boxed types). This was because we'd have to handle all the cases where the existing values don't map what we want. FlValue is a pile of boilerplate but I think it will be less errors for the user.

We should use whatever the standard generic boxed type is. Codecs are designed to allow arbitrary implementations, which means I should be able to make a codec that handles arbitrary types of my own creation. That does mean that unhandled types are a runtime error rather than a compile-time error, but that's how the ObjC and Java implementations work (see here for instance).

The only reason the C++ implementation doesn't do that is that there's no generic root type in C++, so it's template-based instead (and there's still an open bug about the fact that the standard codec, which is supposed to allow extensions, currently doesn't in the C++ implementation precisely because it uses a custom type like FlValue.)

• FlValueBool/FlValueInt or FlValueBoolean/FlValueInteger. I went with the former for saving typing time, but couldn't decide which one was the correct one (different existing code uses different names). Also similar, Double,Float or Float64?

I had some of those issues with the C++ code as well; IIRC I just ended up picking the names used on the Dart side when there was doubt, but I wasn't entirely convinced that was the best option. If there are conventions in GLib, I would say to use those preferentially (e.g., since there's a gboolean, I would probably use Boolean in names in this API).

• I know there are other codecs to implement. I'm haven't investigated in depth what these require, but I think we should be able to add these in later PRs without breaking API. I'll try and get to those sooner rather than later in case we need to make a change.

Standard is the only other codec we need for the foreseeable future. I have not yet seen a case in my desktop work where we needed the string codec.

I split the JSON codecs into #18158 which should make this PR more reviewable.

This is how I'd expect an API user would be able to read/write custom data types using the Standard codec:

G_DEFINE_TYPE(CustomCodec, custom_codec, FlStandardCodec)

enum {
    DATE_VALUE,
    FOO_VALUE,
};

static gboolean
custom_codec_write_value (FlStandardCodec *codec, GByteArray *buffer, FlValue *value, GError **error) {
    if (fl_value_get_type (value) == FL_VALUE_TYPE_CUSTOM) {
        if (fl_value_get_custom_id (value) == DATE_VALUE) {
            GDateTime *date = fl_value_get_custom (value);
            fl_standard_codec_write_code (codec, FL_STANDARD_CODEC_EXTENSION_CODE + DATE_VALUE);
            ...
            return TRUE;
        }
        else if (fl_value_get_custom_id (value) == FOO_VALUE) {
            Foo *foo = fl_value_get_custom (value);
            fl_standard_codec_write_code (codec, FL_STANDARD_CODEC_EXTENSION_CODE + FOO_VALUE);
            ...
            return TRUE;
        }
    }

    return FL_STANDARD_CODEC_CLASS(custom_codec_parent_class)->write_value(codec, buffer, value, error);
}

static FlValue *
custom_codec_read_value_data (FlStandardCodec *codec, GBytes *buffer, size_t *offset, uint8_t code, GError **error) {
    if (code == FL_STANDARD_CODEC_EXTENSION_CODE + DATE_VALUE) {
        ...
        return fl_value_new_custom (g_date_time_new (...), g_date_time_unref);
    }
    if (code == FL_STANDARD_CODEC_EXTENSION_CODE + FOO_VALUE) {
        ...
        return fl_value_new_custom (foo_new (...), g_object_unref);
    }

    return FL_CODEC_CLASS(custom_codec_parent_class)->read_value_data(codec, buffer, offset, code, error);
}

void 
custom_codec_class_init(CustomCodecKlass *klass) {
    FL_CODEC_CLASS(klass)->write_value = custom_codec_write_custom_value;
    FL_STANDARD_CODEC_CLASS(klass)->read_value_data = custom_codec_read_value_data;
}

void 
custom_codec_init(CustomCodec *codec) {}

static void
method_response_cb (GObject *object, GAsyncResult *result, gpointer user_data)
{
    g_autofree gchar *error_code = NULL;
    g_autofree gchar *error_message = NULL;
    g_autoptr(FlValue) response = NULL;
    g_autoptr(GError) error = NULL;
    if (!fl_method_channel_invoke_finish (FL_METHOD_CHANNEL (object),
                                          result,
                                          &error_code,
                                          &error_message,
                                          &response,
                                          &error)) {
        g_printerr ("Failed to send platform message: %s\n", error->message);
        return;
    }

    if (error_code == NULL)
       handle_success (response);
    else
       handle_failure (error_code, error_message, response);
}

int
main (int argc, char **argv)
{
    ...

    method_channel = fl_method_channel_new (fl_engine_get_binary_messenger (fl_view_get_engine (view)),
                                            "example.com/gtk-test-method",
                                            FL_METHOD_CODEC (custom_codec_new ()));

    g_autoptr(FlValue) args = NULL;
    args = fl_value_list_new ();
    fl_value_list_add_take (args, fl_value_list_string_new ("Hello World"));
    fl_value_list_add_take (args, fl_value_custom_new (DATE_VALUE, &event.date, NULL)); // Not copied
    fl_value_list_add_take (args, fl_value_custom_new (FOO_VALUE, g_object_ref (event.foo), g_object_unref)); // Copied with GDestroyNotify
    fl_method_channel_invoke (method_channel, "Test.test", args, NULL, method_response_cb, NULL);

    ...
}

This is how I'd expect an API user would be able to read/write custom data types using the Standard codec:

That definitely looks good. What would the "making a custom FlValue" part look like? (Maybe something to discuss in the split-out PR)

The wall of code effect is definitely real and also affecting me :)

I think this PR has achieved it's value, in that I've got some good initial feedback on the changes and I think I have got my head around the whole architecture/API. What I'll do is make a series of PRs that bring these changes across in more manageable chunks:

1. FlValue
2. FlMessageCodec, FlBinaryCodec and FlStringCodec
3. FlBasicMessageChannel
4. FlStandardCodec
5. FlMethodChannel, FlMethodCodec and FlStandardMethodCodec
6. FlJsonCodec, FlJsonMethodCodec
7. plugins...

I'll respond to the changes requested here, then let's consider this PR obsolete and future work will go into the PRs mentioned above.