[Windows] Expose channel buffers 'resize' and 'overflow' control commands exposed by the control channel. by bleroux · Pull Request #47158 · flutter/engine

bleroux · 2023-10-20T14:16:36Z

Description

This PR adds two helper functions to invoke the 'resize' and 'overflow' commands exposed by the control channel.
See:

Lines 302 to 309 in 93e8901

    
           /// ## The control channel 
        
           /// 
        
           /// A plugin can configure its channel's buffers by sending messages to the 
        
           /// control channel, `dev.flutter/channel-buffers` (see [kControlChannelName]). 
        
           /// 
        
           /// There are two messages that can be sent to this control channel, to adjust 
        
           /// the buffer size and to disable the overflow warnings. See [handleMessage] 
        
           /// for details on these messages.

Implementation based on the discussion from #46998.

Related Issue

Windows implementation for flutter/flutter#132386

Tests

Adds 4 tests.

shell/platform/common/client_wrapper/basic_message_channel_unittests.cc

shell/platform/common/client_wrapper/include/flutter/method_channel.h

shell/platform/common/client_wrapper/include/flutter/basic_message_channel.h

shell/platform/common/client_wrapper/basic_message_channel_unittests.cc

shell/platform/common/client_wrapper/include/flutter/basic_message_channel.h

shell/platform/common/client_wrapper/include/flutter/method_channel.h

jonahwilliams · 2023-11-30T21:09:54Z

Friendly ping @stuartmorgan it looks like this needs another review.

shell/platform/common/client_wrapper/method_channel.cc

shell/platform/common/client_wrapper/include/flutter/method_channel.h

bleroux · 2023-12-01T10:33:37Z

Thanks for the feedback. I will pursue this work in two weeks (I won't have access to my Windows setup until December 11th).

…ands

stuartmorgan-g · 2023-12-13T17:51:11Z

shell/platform/common/client_wrapper/core_implementations.cc

+
+static constexpr char kControlChannelName[] = "dev.flutter/channel-buffers";
+static constexpr char kResizeMethod[] = "resize";
+static constexpr char kOverflowMethod[] = "overflow";


These three lines should be in an anonymous namespace (and thus don't need static to have internal linkage), rather than the internal namespace, since they don't need to be referenced outside of this file.

stuartmorgan-g · 2023-12-13T17:52:48Z

shell/platform/common/client_wrapper/core_implementations.cc

+// Adjusts the number of messages that will get buffered when sending messages
+// to channels that aren't fully set up yet. For example, the engine isn't
+// running yet or the channel's message handler isn't set up on the Dart side
+// yet.


Here and below: per the style guide, declaration comments belong at the declaration point (which is the header), not the definition point. That's true even for things that not intended for general use, like internal-namespaced functions or private class methods.

stuartmorgan-g · 2023-12-13T17:53:12Z

shell/platform/common/client_wrapper/include/flutter/basic_message_channel.h

 namespace flutter {

+namespace internal {
+// Internal helpers functions used by BasicMessageChannel and MethodChannel.


Nit: helper

stuartmorgan-g

LGTM, thanks!

…rol commands exposed by the control channel. (flutter/engine#47158)

flutter/engine@caf3327...a565cea 2023-12-14 [email protected] [Windows] Expose channel buffers 'resize' and 'overflow' control commands exposed by the control channel. (flutter/engine#47158) 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

bleroux requested a review from stuartmorgan-g October 23, 2023 15:38

loic-sharma requested review from cbracken and yaakovschectman October 26, 2023 16:06

loic-sharma reviewed Oct 26, 2023

View reviewed changes

shell/platform/common/client_wrapper/basic_message_channel_unittests.cc Outdated Show resolved Hide resolved

loic-sharma reviewed Oct 26, 2023

View reviewed changes

shell/platform/common/client_wrapper/include/flutter/method_channel.h Outdated Show resolved Hide resolved

loic-sharma reviewed Oct 26, 2023

View reviewed changes

shell/platform/common/client_wrapper/include/flutter/basic_message_channel.h Outdated Show resolved Hide resolved

loic-sharma reviewed Oct 26, 2023

View reviewed changes

shell/platform/common/client_wrapper/basic_message_channel_unittests.cc Show resolved Hide resolved

bleroux force-pushed the windows_add_channel_buffer_helpers_resize_and_overflow branch from 5eb2181 to d7510bf Compare November 2, 2023 13:20

bleroux requested a review from loic-sharma November 2, 2023 14:40

stuartmorgan-g suggested changes Nov 2, 2023

View reviewed changes

bleroux force-pushed the windows_add_channel_buffer_helpers_resize_and_overflow branch 8 times, most recently from 9782c71 to 69e56a1 Compare November 9, 2023 14:46

bleroux commented Nov 9, 2023

View reviewed changes

shell/platform/common/client_wrapper/include/flutter/method_channel.h Outdated Show resolved Hide resolved

chinmaygarde changed the title ~~[Windows] Expose channel buffers 'resize' and 'overflow' control comm…~~ [Windows] Expose channel buffers 'resize' and 'overflow' control commands exposed by the control channel Nov 9, 2023

chinmaygarde changed the title ~~[Windows] Expose channel buffers 'resize' and 'overflow' control commands exposed by the control channel~~ [Windows] Expose channel buffers 'resize' and 'overflow' control commands exposed by the control channel. Nov 9, 2023

stuartmorgan-g suggested changes Nov 10, 2023

View reviewed changes

shell/platform/common/client_wrapper/include/flutter/method_channel.h Outdated Show resolved Hide resolved

shell/platform/common/client_wrapper/include/flutter/method_channel.h Outdated Show resolved Hide resolved

bleroux force-pushed the windows_add_channel_buffer_helpers_resize_and_overflow branch from 26cb681 to c0e81b2 Compare November 13, 2023 15:11

bleroux requested a review from stuartmorgan-g November 13, 2023 19:37

stuartmorgan-g suggested changes Nov 30, 2023

View reviewed changes

bleroux mentioned this pull request Dec 1, 2023

Pressing and releasing a key before the framework has started throws flutter/flutter#125975

Open

2 tasks

bleroux and others added 2 commits December 11, 2023 15:04

[Windows] Expose channel buffers 'resize' and 'overflow' control comm…

c9464ed

…ands

Address review comments

c601940

bleroux added 3 commits December 11, 2023 15:04

Add a comment to explain why the test relies on bytes array comparaison

6df13f8

Update int truncation comment + remove last_message_size in tests

7be1657

Move control channel logic to a private namespace

308cda2

bleroux force-pushed the windows_add_channel_buffer_helpers_resize_and_overflow branch from c0e81b2 to 0c1609b Compare December 12, 2023 08:56

bleroux requested a review from stuartmorgan-g December 12, 2023 09:29

stuartmorgan-g suggested changes Dec 13, 2023

View reviewed changes

Move helper functions to core_implementations.cc

c6e9fe9

bleroux force-pushed the windows_add_channel_buffer_helpers_resize_and_overflow branch 2 times, most recently from 1ed71eb to c6e9fe9 Compare December 14, 2023 09:10

bleroux requested a review from stuartmorgan-g December 14, 2023 13:24

stuartmorgan-g approved these changes Dec 14, 2023

View reviewed changes

bleroux added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 14, 2023

auto-submit bot merged commit a565cea into flutter:main Dec 14, 2023

engine-flutter-autoroll mentioned this pull request Dec 14, 2023

Roll Flutter Engine from caf33276468b to a565cea256c7 (1 revision) flutter/flutter#140163

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 14, 2023

a565cea [Windows] Expose channel buffers 'resize' and 'overflow' cont…

563a433

…rol commands exposed by the control channel. (flutter/engine#47158)

bleroux deleted the windows_add_channel_buffer_helpers_resize_and_overflow branch December 15, 2023 20:35

bleroux mentioned this pull request Dec 15, 2023

Provide helper functions for channel buffer 'resize' and 'overflow' commands flutter/flutter#132386

Closed

2 tasks

	/// ## The control channel
	///
	/// A plugin can configure its channel's buffers by sending messages to the
	/// control channel, `dev.flutter/channel-buffers` (see [kControlChannelName]).
	///
	/// There are two messages that can be sent to this control channel, to adjust
	/// the buffer size and to disable the overflow warnings. See [handleMessage]
	/// for details on these messages.

Conversation

bleroux commented Oct 20, 2023

Description

Related Issue

Tests

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonahwilliams commented Nov 30, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bleroux commented Dec 1, 2023

Uh oh!

stuartmorgan-g Dec 13, 2023

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Dec 13, 2023

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Dec 13, 2023

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants