This repository was archived by the owner on Feb 25, 2025. It is now read-only.
[Linux] Expose channel buffers 'resize' and 'overflow' control commands #44636
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 tasks
shell/platform/linux/public/flutter_linux/fl_binary_messenger.h
Outdated
Show resolved
Hide resolved
shell/platform/linux/public/flutter_linux/fl_binary_messenger.h
Outdated
Show resolved
Hide resolved
shell/platform/linux/public/flutter_linux/fl_binary_messenger.h
Outdated
Show resolved
Hide resolved
robert-ancell
suggested changes
Aug 14, 2023
There was a problem hiding this comment.
Looks generally good, thanks!
This doesn't seem to be implemented for the other platforms that I can see, so I'm not 100% sure if these methods should be part of FlBinaryMessenger or another class. Also unsure if dev.fluter/channel-buffers should be a class of its own, or it's fine to hide it inside FlBinaryMessenger. @cbracken @gspencergoog @stuartmorgan you might have thoughts on this.
bleroux
force-pushed
the
linux_add_channel_buffer_utilities_resize_and_overflow
branch
2 times, most recently
from
003f7c9 to
839432c
Compare
cbracken
reviewed
Aug 16, 2023
|
|
||
| void (*resize_channel)(FlBinaryMessenger* messenger, | ||
| const gchar* channel, | ||
| int64_t new_size); |
There was a problem hiding this comment.
Is int64_t the right type here? Should this be size_t or at least uint64_t, or is there a need to support a negative size?
I notice that the Dart side of this doesn't seem to guard against negative sizes either, though the documentation seems to imply that capacity should always be >= 0.
There was a problem hiding this comment.
If we use size_t here the method will need to validate if it's greater than INT64_MAX. If we allow int64_t, then we let the server Dart side do the validation and return an error. I think in practice the user is unlikely to send a negative number and relying on the second method is probably less likely to cause an issue.
There was a problem hiding this comment.
Perhaps worth adding a debug-mode assertion in that case via FML_DCHECK
cbracken
reviewed
Aug 16, 2023
bleroux
force-pushed
the
linux_add_channel_buffer_utilities_resize_and_overflow
branch
4 times, most recently
from
f950b30 to
2d165d9
Compare
Putting it on the binary messenger seems reasonable to me. If they were likely to be frequently used we'd probably want to expose them on the channel classes themselves, but there's no reason to think that would be the case. |
cbracken
approved these changes
Aug 18, 2023
bleroux
force-pushed
the
linux_add_channel_buffer_utilities_resize_and_overflow
branch
3 times, most recently
from
d1352ba to
6015a74
Compare
@cbracken I added it, can you check if I did it right (I had to update a build configuration file)? |
cbracken
approved these changes
Aug 18, 2023
bleroux
force-pushed
the
linux_add_channel_buffer_utilities_resize_and_overflow
branch
from
4e43857 to
97dac71
Compare
Co-authored-by: Chris Bracken <[email protected]>
bleroux
force-pushed
the
linux_add_channel_buffer_utilities_resize_and_overflow
branch
from
97dac71 to
ccbad9e
Compare
bleroux
added
the
autosubmit
bleroux
deleted the
linux_add_channel_buffer_utilities_resize_and_overflow
branch
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Aug 22, 2023
fluttermirroringbot
pushed a commit
to flutter/flutter
that referenced
this pull request
Aug 22, 2023
…133031) flutter/engine@5e84d73...447981a 2023-08-22 [email protected] Roll Skia from 399e90397a07 to f13ee6ee5433 (1 revision) (flutter/engine#44944) 2023-08-22 [email protected] [Linux] Expose channel buffers 'resize' and 'overflow' control commands (flutter/engine#44636) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
gaaclarke
pushed a commit
to gaaclarke/engine
that referenced
this pull request
Aug 30, 2023
…ds (flutter#44636) ## Description This PR adds two helper functions to invoke the 'resize' and 'overflow' commands exposed by the control channel. See: https://github.com/flutter/engine/blob/93e8901490e78c7ba7e319cce4470d9c6478c6dc/lib/ui/channel_buffers.dart#L302-L309 ## Related Issue Linux implementation for flutter/flutter#132386 ## Tests Adds two tests.
auto-submit bot
pushed a commit
that referenced
this pull request
Sep 1, 2023
## Description This PR fixes a mistake I made on #44636 where the error handling code wrongly relied on `fl_method_channel_invoke_method_finish` instead of `fl_binary_messenger_send_on_channel_finish`. The error handling code was not called when running the tests added in #44636 so this mistake did not pop up. @robert-ancell I added a test that simulates an error response and I had to rely on `g_idle_add` to make it works. Is this approach ok? ## Related Issue Linux implementation for flutter/flutter#132386 ## Tests Adds one test.
Closed
2 tasks
auto-submit bot
pushed a commit
that referenced
this pull request
Sep 19, 2023
#44848) ## Description This PR update the helper function that invokes the control channel 'resize' command (previous implementation relied on a deprecated format). It also adds a similar helper function for the 'overflow' commands exposed by the control channel. See: https://github.com/flutter/engine/blob/93e8901490e78c7ba7e319cce4470d9c6478c6dc/lib/ui/channel_buffers.dart#L302-L309 ## Related Issue iOS and macOS implementation for flutter/flutter#132386 Similar implementations: - Android: #44434 - Linux: #44636 ## Tests Adds two tests.
auto-submit bot
pushed a commit
that referenced
this pull request
Oct 2, 2023
#46360) ## Description This PR is a follow-up to #44636 which introduces the `set_allow_channel_overflow` function. It renames this function to `set_warns_on_channel_overflow`. The previous naming was inspired by the framework side implementation. https://github.com/flutter/engine/blob/93e8901490e78c7ba7e319cce4470d9c6478c6dc/lib/ui/channel_buffers.dart#L574 During the review of the iOS/macOS implementation, #44848 (comment), we agreed that the existing naming is confusing because it implies that overflow will be allowed, but this not the case this function is used to enable/disable error messages when a channel overflows. ## Related Issue Follow-up for flutter/flutter#132386. ## Tests Updates 2 tests.
harryterkelsen
pushed a commit
that referenced
this pull request
Oct 23, 2023
#44848) ## Description This PR update the helper function that invokes the control channel 'resize' command (previous implementation relied on a deprecated format). It also adds a similar helper function for the 'overflow' commands exposed by the control channel. See: https://github.com/flutter/engine/blob/93e8901490e78c7ba7e319cce4470d9c6478c6dc/lib/ui/channel_buffers.dart#L302-L309 ## Related Issue iOS and macOS implementation for flutter/flutter#132386 Similar implementations: - Android: #44434 - Linux: #44636 ## Tests Adds two tests.
harryterkelsen
pushed a commit
that referenced
this pull request
Oct 23, 2023
#46360) ## Description This PR is a follow-up to #44636 which introduces the `set_allow_channel_overflow` function. It renames this function to `set_warns_on_channel_overflow`. The previous naming was inspired by the framework side implementation. https://github.com/flutter/engine/blob/93e8901490e78c7ba7e319cce4470d9c6478c6dc/lib/ui/channel_buffers.dart#L574 During the review of the iOS/macOS implementation, #44848 (comment), we agreed that the existing naming is confusing because it implies that overflow will be allowed, but this not the case this function is used to enable/disable error messages when a channel overflows. ## Related Issue Follow-up for flutter/flutter#132386. ## Tests Updates 2 tests.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR adds two helper functions to invoke the 'resize' and 'overflow' commands exposed by the control channel.
See:
engine/lib/ui/channel_buffers.dart
Lines 302 to 309 in 93e8901
Related Issue
Linux implementation for flutter/flutter#132386
Tests
Adds two tests.