Skip to content

Conversation

@Hixie
Copy link
Contributor

@Hixie Hixie commented Jan 20, 2021

I accidentally broke this in an earlier patch.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is message always null when argType is void?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean in pigeon-generated code? Or?

Copy link
Contributor

Choose a reason for hiding this comment

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

yup. pigeon-generated code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it looks like _writeHostApi always sends null if the argType is void, but of course people can always bypass pigeon and send whatever.

Copy link
Contributor

Choose a reason for hiding this comment

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

does it make sense to also verify the message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

minor nit: some comment would be helpful. It took me a while to figure out what it was checking :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fair, me too...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@blasten blasten left a comment

Choose a reason for hiding this comment

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

LGTM!

@Hixie
Copy link
Contributor Author

Hixie commented Jan 21, 2021

Blocked on flutter/flutter#74446 which fixes the test failure in local_tests here.

@Hixie Hixie force-pushed the arg-less branch 2 times, most recently from a95a041 to 2671601 Compare January 21, 2021 22:58
@Hixie Hixie marked this pull request as draft January 24, 2021 09:21
@Hixie Hixie force-pushed the arg-less branch 6 times, most recently from 8d1a0f6 to 9db165e Compare January 30, 2021 01:11
I accidentally broke this in an earlier patch.
@Hixie Hixie marked this pull request as ready for review February 3, 2021 09:09
@Hixie
Copy link
Contributor Author

Hixie commented Feb 3, 2021

Horrah the tests pass now.

@Hixie
Copy link
Contributor Author

Hixie commented Feb 3, 2021

Landing!

@Hixie Hixie merged commit 93ae089 into flutter:master Feb 3, 2021
@Hixie Hixie deleted the arg-less branch February 3, 2021 18:44
stuartmorgan-g pushed a commit to stuartmorgan-g/packages that referenced this pull request Apr 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants