Skip to content

Extract codecs from plugin API #110

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Aug 7, 2018

Conversation

stuartmorgan-g
Copy link
Collaborator

The splitting of channel.* into separate files for each class, and the fact that plugin_handler was changed enough that it wasn't recognized as a move, makes the Linux change look larger than it actually is (as does the define guard and include path churn). If that gets in the way of reviewing let me know and I can retroactively create a prequel that does all the source rearranging and include path cleanup without any of the code changes.

@stuartmorgan-g stuartmorgan-g requested a review from awdavies July 17, 2018 04:26
@stuartmorgan-g stuartmorgan-g changed the title Plugin refactor codecs Extract codecs from plugin API Jul 17, 2018
Extracts method codecs into stand-alone objects, and allows plugins to
indicate that they use a codec other than JSON. This is an incremental
step toward aligning the plugin API with Flutter's MethodChannel API.

For now no other codecs are actually implemented in the framework, and
the default is still JSON (unlike in existing Flutter code) so that this
is not a breaking change. A future change will full move to MethodChannel,
which will be a breaking change.

Partially addresses issue google#67 in that custom codecs are now supported,
even though the Standard (binary) codec is not yet implemented.
Corresponding Linux change.

Unlike the macOS change, this is a minor breaking change; that was
unavoidable due to the use of Json::Value in key interfaces. The
JsonPlugin class is designed to minimize impact on existing plugins.
@stuartmorgan-g stuartmorgan-g force-pushed the plugin-refactor-codecs branch from cacb510 to 1450b21 Compare August 6, 2018 22:10
@stuartmorgan-g
Copy link
Collaborator Author

Rebased onto the Linux cleanup commits; the Linux change should be less painful to review now.

Copy link
Contributor

@awdavies awdavies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor nit for the Linux, otherwise that portion looks good. Takin a look at the Obj-C side.

~EngineMethodResult();

protected:
void SuccessInternal(const void *result) override;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment for the class which is being overridden.

Copy link
Contributor

@awdavies awdavies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with minor nits

- (nullable NSData*)encodeSuccessEnvelope:(nullable id)result;

/**
* Returns a binary encoding of |error|. Returns nil if the |details| parameter of |erorr| is not
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/erorr/error/

@stuartmorgan-g stuartmorgan-g merged commit d736085 into google:master Aug 7, 2018
@stuartmorgan-g stuartmorgan-g deleted the plugin-refactor-codecs branch August 7, 2018 17:01
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants