Skip to content

Commit

Permalink
Only register top level window message listener upon registering serv…
Browse files Browse the repository at this point in the history
…ice binding (#41733)

Move registering the `WM_CLOSE` message listener in the engine from
initialization to in response to a message sent from `ServiceBinding`
upon its initialization so that we do not intercept window messages when
there is no binding registered.

Addresses flutter/flutter#126033.

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide] and the [C++,
Objective-C, Java style guides].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [ ] I added new tests to check the change I am making or feature I am
adding, or Hixie said the PR is test-exempt. See [testing the engine]
for instructions on writing and running engine tests.
- [x] I updated/added relevant documentation (doc comments with `///`).
- [ ] I signed the [CLA].
- [x] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel
on [Discord].

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#overview
[Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene
[Flutter Style Guide]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo
[C++, Objective-C, Java style guides]:
https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
[testing the engine]:
https://github.com/flutter/flutter/wiki/Testing-the-engine
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat

---------

Co-authored-by: Loïc Sharma <737941+loic-sharma@users.noreply.github.com>
  • Loading branch information
yaakovschectman and loic-sharma committed May 10, 2023
1 parent 4db1fa4 commit fea7d0a
Show file tree
Hide file tree
Showing 8 changed files with 82 additions and 1 deletion.
4 changes: 4 additions & 0 deletions shell/platform/darwin/macos/framework/Source/FlutterEngine.mm
Original file line number Diff line number Diff line change
Expand Up @@ -1031,6 +1031,10 @@ - (void)handleMethodCall:(FlutterMethodCall*)call result:(FlutterResult)result {
result(@{@"value" : @([self clipboardHasStrings])});
} else if ([call.method isEqualToString:@"System.exitApplication"]) {
[[self terminationHandler] handleRequestAppExitMethodCall:call.arguments result:result];
} else if ([call.method isEqualToString:@"System.initializationComplete"]) {
// TODO(gspencergoog): Handle this message to enable exit message listening.
// https://github.com/flutter/flutter/issues/126033
result(nil);
} else {
result(FlutterMethodNotImplemented);
}
Expand Down
6 changes: 6 additions & 0 deletions shell/platform/linux/fl_platform_plugin.cc
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ static constexpr char kSetClipboardDataMethod[] = "Clipboard.setData";
static constexpr char kClipboardHasStringsMethod[] = "Clipboard.hasStrings";
static constexpr char kExitApplicationMethod[] = "System.exitApplication";
static constexpr char kRequestAppExitMethod[] = "System.requestAppExit";
static constexpr char kInitializationCompleteMethod[] =
"System.initializationComplete";
static constexpr char kPlaySoundMethod[] = "SystemSound.play";
static constexpr char kSystemNavigatorPopMethod[] = "SystemNavigator.pop";
static constexpr char kTextKey[] = "text";
Expand Down Expand Up @@ -335,6 +337,10 @@ static void method_call_cb(FlMethodChannel* channel,
response = system_sound_play(self, args);
} else if (strcmp(method, kSystemNavigatorPopMethod) == 0) {
response = system_navigator_pop(self);
} else if (strcmp(method, kInitializationCompleteMethod) == 0) {
// TODO(gspencergoog): Handle this message to enable exit message listening.
// https://github.com/flutter/flutter/issues/126033
response = FL_METHOD_RESPONSE(fl_method_success_response_new(nullptr));
} else {
response = FL_METHOD_RESPONSE(fl_method_not_implemented_response_new());
}
Expand Down
4 changes: 4 additions & 0 deletions shell/platform/windows/flutter_windows_engine.cc
Original file line number Diff line number Diff line change
Expand Up @@ -778,4 +778,8 @@ void FlutterWindowsEngine::OnDwmCompositionChanged() {
view_->OnDwmCompositionChanged();
}

void FlutterWindowsEngine::OnApplicationLifecycleEnabled() {
lifecycle_manager_->BeginProcessingClose();
}

} // namespace flutter
4 changes: 4 additions & 0 deletions shell/platform/windows/flutter_windows_engine.h
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,10 @@ class FlutterWindowsEngine {
// Called when a WM_DWMCOMPOSITIONCHANGED message is received.
void OnDwmCompositionChanged();

// Called in response to the framework registering a ServiceBindings.
// Registers the top level handler for the WM_CLOSE window message.
void OnApplicationLifecycleEnabled();

protected:
// Creates the keyboard key handler.
//
Expand Down
46 changes: 46 additions & 0 deletions shell/platform/windows/flutter_windows_engine_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -657,6 +657,7 @@ TEST_F(FlutterWindowsEngineTest, TestExit) {
ON_CALL(*handler, IsLastWindowOfProcess).WillByDefault([]() { return true; });
EXPECT_CALL(*handler, Quit).Times(1);
modifier.SetLifecycleManager(std::move(handler));
engine->OnApplicationLifecycleEnabled();

engine->Run();

Expand Down Expand Up @@ -693,6 +694,7 @@ TEST_F(FlutterWindowsEngineTest, TestExitCancel) {
ON_CALL(*handler, IsLastWindowOfProcess).WillByDefault([]() { return true; });
EXPECT_CALL(*handler, Quit).Times(0);
modifier.SetLifecycleManager(std::move(handler));
engine->OnApplicationLifecycleEnabled();

auto binary_messenger =
std::make_unique<BinaryMessengerImpl>(engine->messenger());
Expand Down Expand Up @@ -753,6 +755,7 @@ TEST_F(FlutterWindowsEngineTest, TestExitSecondCloseMessage) {
hwnd, msg, wparam, lparam);
});
modifier.SetLifecycleManager(std::move(handler));
engine->OnApplicationLifecycleEnabled();

engine->Run();

Expand Down Expand Up @@ -803,6 +806,7 @@ TEST_F(FlutterWindowsEngineTest, TestExitCloseMultiWindow) {
// Quit should not be called when there is more than one window.
EXPECT_CALL(*handler, Quit).Times(0);
modifier.SetLifecycleManager(std::move(handler));
engine->OnApplicationLifecycleEnabled();

engine->Run();

Expand All @@ -814,5 +818,47 @@ TEST_F(FlutterWindowsEngineTest, TestExitCloseMultiWindow) {
}
}

TEST_F(FlutterWindowsEngineTest, LifecycleManagerDisabledByDefault) {
FlutterWindowsEngineBuilder builder{GetContext()};

auto window_binding_handler =
std::make_unique<::testing::NiceMock<MockWindowBindingHandler>>();
MockFlutterWindowsView view(std::move(window_binding_handler));
view.SetEngine(builder.Build());
FlutterWindowsEngine* engine = view.GetEngine();

EngineModifier modifier(engine);
modifier.embedder_api().RunsAOTCompiledDartCode = []() { return false; };
auto handler = std::make_unique<MockWindowsLifecycleManager>(engine);
EXPECT_CALL(*handler, IsLastWindowOfProcess).Times(0);
modifier.SetLifecycleManager(std::move(handler));

engine->window_proc_delegate_manager()->OnTopLevelWindowProc(0, WM_CLOSE, 0,
0);
}

TEST_F(FlutterWindowsEngineTest, EnableApplicationLifecycle) {
FlutterWindowsEngineBuilder builder{GetContext()};

auto window_binding_handler =
std::make_unique<::testing::NiceMock<MockWindowBindingHandler>>();
MockFlutterWindowsView view(std::move(window_binding_handler));
view.SetEngine(builder.Build());
FlutterWindowsEngine* engine = view.GetEngine();

EngineModifier modifier(engine);
modifier.embedder_api().RunsAOTCompiledDartCode = []() { return false; };
auto handler = std::make_unique<MockWindowsLifecycleManager>(engine);
ON_CALL(*handler, IsLastWindowOfProcess).WillByDefault([]() {
return false;
});
EXPECT_CALL(*handler, IsLastWindowOfProcess).Times(1);
modifier.SetLifecycleManager(std::move(handler));
engine->OnApplicationLifecycleEnabled();

engine->window_proc_delegate_manager()->OnTopLevelWindowProc(0, WM_CLOSE, 0,
0);
}

} // namespace testing
} // namespace flutter
5 changes: 5 additions & 0 deletions shell/platform/windows/platform_handler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ static constexpr char kHasStringsClipboardMethod[] = "Clipboard.hasStrings";
static constexpr char kSetClipboardDataMethod[] = "Clipboard.setData";
static constexpr char kExitApplicationMethod[] = "System.exitApplication";
static constexpr char kRequestAppExitMethod[] = "System.requestAppExit";
static constexpr char kInitializationCompleteMethod[] =
"System.initializationComplete";
static constexpr char kPlaySoundMethod[] = "SystemSound.play";

static constexpr char kExitCodeKey[] = "exitCode";
Expand Down Expand Up @@ -495,6 +497,9 @@ void PlatformHandler::HandleMethodCall(
const rapidjson::Value& sound_type = method_call.arguments()[0];

SystemSoundPlay(sound_type.GetString(), std::move(result));
} else if (method.compare(kInitializationCompleteMethod) == 0) {
engine_->OnApplicationLifecycleEnabled();
result->Success();
} else {
result->NotImplemented();
}
Expand Down
9 changes: 8 additions & 1 deletion shell/platform/windows/windows_lifecycle_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
namespace flutter {

WindowsLifecycleManager::WindowsLifecycleManager(FlutterWindowsEngine* engine)
: engine_(engine) {}
: engine_(engine), process_close_(false) {}

WindowsLifecycleManager::~WindowsLifecycleManager() {}

Expand Down Expand Up @@ -49,6 +49,9 @@ bool WindowsLifecycleManager::WindowProc(HWND hwnd,
// is, we re-dispatch a new WM_CLOSE message. In order to allow the new
// message to reach other delegates, we ignore it here.
case WM_CLOSE: {
if (!process_close_) {
return false;
}
auto key = std::make_tuple(hwnd, wpar, lpar);
auto itr = sent_close_messages_.find(key);
if (itr != sent_close_messages_.end()) {
Expand Down Expand Up @@ -156,4 +159,8 @@ bool WindowsLifecycleManager::IsLastWindowOfProcess() {
return num_windows <= 1;
}

void WindowsLifecycleManager::BeginProcessingClose() {
process_close_ = true;
}

} // namespace flutter
5 changes: 5 additions & 0 deletions shell/platform/windows/windows_lifecycle_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ class WindowsLifecycleManager {
// Intercept top level window messages, only paying attention to WM_CLOSE.
bool WindowProc(HWND hwnd, UINT msg, WPARAM w, LPARAM l, LRESULT* result);

// Signal to start consuming WM_CLOSE messages.
void BeginProcessingClose();

protected:
// Check the number of top-level windows associated with this process, and
// return true only if there are 1 or fewer.
Expand All @@ -51,6 +54,8 @@ class WindowsLifecycleManager {
FlutterWindowsEngine* engine_;

std::map<std::tuple<HWND, WPARAM, LPARAM>, int> sent_close_messages_;

bool process_close_;
};

} // namespace flutter
Expand Down

0 comments on commit fea7d0a

Please sign in to comment.