Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Windows] Don't always stop engine on view destruction #51681

Merged
merged 1 commit into from
Mar 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions ci/licenses_golden/licenses_flutter
Original file line number Diff line number Diff line change
Expand Up @@ -41819,6 +41819,7 @@ ORIGIN: ../../../flutter/shell/platform/windows/flutter_windows_texture_registra
ORIGIN: ../../../flutter/shell/platform/windows/flutter_windows_texture_registrar.h + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/shell/platform/windows/flutter_windows_view.cc + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/shell/platform/windows/flutter_windows_view.h + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/shell/platform/windows/flutter_windows_view_controller.cc + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/shell/platform/windows/flutter_windows_view_controller.h + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/shell/platform/windows/keyboard_handler_base.h + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/shell/platform/windows/keyboard_key_channel_handler.cc + ../../../flutter/LICENSE
Expand Down Expand Up @@ -44723,6 +44724,7 @@ FILE: ../../../flutter/shell/platform/windows/flutter_windows_texture_registrar.
FILE: ../../../flutter/shell/platform/windows/flutter_windows_texture_registrar.h
FILE: ../../../flutter/shell/platform/windows/flutter_windows_view.cc
FILE: ../../../flutter/shell/platform/windows/flutter_windows_view.h
FILE: ../../../flutter/shell/platform/windows/flutter_windows_view_controller.cc
FILE: ../../../flutter/shell/platform/windows/flutter_windows_view_controller.h
FILE: ../../../flutter/shell/platform/windows/keyboard_handler_base.h
FILE: ../../../flutter/shell/platform/windows/keyboard_key_channel_handler.cc
Expand Down
1 change: 1 addition & 0 deletions shell/platform/windows/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ source_set("flutter_windows_source") {
"flutter_windows_texture_registrar.h",
"flutter_windows_view.cc",
"flutter_windows_view.h",
"flutter_windows_view_controller.cc",
"flutter_windows_view_controller.h",
"keyboard_handler_base.h",
"keyboard_key_channel_handler.cc",
Expand Down
1 change: 1 addition & 0 deletions shell/platform/windows/flutter_windows.cc
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ FlutterDesktopViewControllerRef FlutterDesktopEngineCreateViewController(

void FlutterDesktopViewControllerDestroy(FlutterDesktopViewControllerRef ref) {
auto controller = ViewControllerFromHandle(ref);
controller->Destroy();
delete controller;
}

Expand Down
18 changes: 18 additions & 0 deletions shell/platform/windows/flutter_windows_engine.cc
Original file line number Diff line number Diff line change
Expand Up @@ -502,6 +502,24 @@ std::unique_ptr<FlutterWindowsView> FlutterWindowsEngine::CreateView(
return std::move(view);
}

void FlutterWindowsEngine::RemoveView(FlutterViewId view_id) {
FML_DCHECK(running());
FML_DCHECK(views_.find(view_id) != views_.end());

if (view_id == kImplicitViewId) {
// The engine and framework assume the implicit view always exists.
// Attempts to render to the implicit view will be ignored.
views_.erase(view_id);
return;
}

// TODO(loicsharma): Remove the view from the engine using the
// `FlutterEngineRemoveView` embedder API. Windows does not
// support views other than the implicit view yet.
// https://github.com/flutter/flutter/issues/144810
FML_UNREACHABLE();
}

void FlutterWindowsEngine::OnVsync(intptr_t baton) {
std::chrono::nanoseconds current_time =
std::chrono::nanoseconds(embedder_api_.GetCurrentTime());
Expand Down
3 changes: 3 additions & 0 deletions shell/platform/windows/flutter_windows_engine.h
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,9 @@ class FlutterWindowsEngine {
std::unique_ptr<FlutterWindowsView> CreateView(
std::unique_ptr<WindowBindingHandler> window);

// Remove a view. The engine will no longer render into it.
virtual void RemoveView(FlutterViewId view_id);

// Get a view that displays this engine's content.
//
// Returns null if the view does not exist.
Expand Down
3 changes: 3 additions & 0 deletions shell/platform/windows/flutter_windows_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,9 @@ TEST_F(WindowsTest, EngineCanTransitionToHeadless) {

// The engine is back in headless mode now.
ASSERT_NE(engine, nullptr);

auto engine_ptr = reinterpret_cast<FlutterWindowsEngine*>(engine.get());
ASSERT_TRUE(engine_ptr->running());
}

// Verify that accessibility features are initialized when a view is created.
Expand Down
4 changes: 0 additions & 4 deletions shell/platform/windows/flutter_windows_view.cc
Original file line number Diff line number Diff line change
Expand Up @@ -105,10 +105,6 @@ FlutterWindowsView::~FlutterWindowsView() {
// Notify the engine the view's child window will no longer be visible.
engine_->OnWindowStateEvent(GetWindowHandle(), WindowStateEvent::kHide);

// The engine renders into the view's surface. The engine must be
// shutdown before the view's resources can be destroyed.
engine_->Stop();

DestroyRenderSurface();
}

Expand Down
30 changes: 30 additions & 0 deletions shell/platform/windows/flutter_windows_view_controller.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "flutter/shell/platform/windows/flutter_windows_view_controller.h"

namespace flutter {

FlutterWindowsViewController::~FlutterWindowsViewController() {
Destroy();
}

void FlutterWindowsViewController::Destroy() {
if (!view_) {
return;
}

// Prevent the engine from rendering into this view.
if (view_->GetEngine()->running()) {
auto view_id = view_->view_id();

view_->GetEngine()->RemoveView(view_id);
}

// Destroy the view, followed by the engine if it is owned by this controller.
view_.reset();
engine_.reset();
}

} // namespace flutter
14 changes: 12 additions & 2 deletions shell/platform/windows/flutter_windows_view_controller.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,9 @@

#include <memory>

#include "flutter_windows_engine.h"
#include "flutter_windows_view.h"
#include "flutter/fml/macros.h"
#include "flutter/shell/platform/windows/flutter_windows_engine.h"
#include "flutter/shell/platform/windows/flutter_windows_view.h"

namespace flutter {

Expand All @@ -19,6 +20,13 @@ class FlutterWindowsViewController {
std::unique_ptr<FlutterWindowsView> view)
: engine_(std::move(engine)), view_(std::move(view)) {}

~FlutterWindowsViewController();

// Destroy this view controller and its view.
//
// If this view controller owns the engine, the engine is also destroyed.
void Destroy();

FlutterWindowsEngine* engine() { return view_->GetEngine(); }
FlutterWindowsView* view() { return view_.get(); }

Expand All @@ -36,6 +44,8 @@ class FlutterWindowsViewController {
std::unique_ptr<FlutterWindowsEngine> engine_;

std::unique_ptr<FlutterWindowsView> view_;

FML_DISALLOW_COPY_AND_ASSIGN(FlutterWindowsViewController);
};

} // namespace flutter
Expand Down
17 changes: 10 additions & 7 deletions shell/platform/windows/flutter_windows_view_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include "flutter/shell/platform/windows/flutter_window.h"
#include "flutter/shell/platform/windows/flutter_windows_engine.h"
#include "flutter/shell/platform/windows/flutter_windows_texture_registrar.h"
#include "flutter/shell/platform/windows/flutter_windows_view_controller.h"
#include "flutter/shell/platform/windows/testing/egl/mock_context.h"
#include "flutter/shell/platform/windows/testing/egl/mock_manager.h"
#include "flutter/shell/platform/windows/testing/egl/mock_window_surface.h"
Expand Down Expand Up @@ -121,6 +122,7 @@ class MockFlutterWindowsEngine : public FlutterWindowsEngine {

MOCK_METHOD(bool, running, (), (const));
MOCK_METHOD(bool, Stop, (), ());
MOCK_METHOD(void, RemoveView, (FlutterViewId view_id), ());
MOCK_METHOD(bool, PostRasterThreadTask, (fml::closure), (const));

private:
Expand Down Expand Up @@ -241,6 +243,8 @@ TEST(FlutterWindowsViewTest, Shutdown) {
std::make_unique<NiceMock<MockWindowBindingHandler>>();
auto egl_manager = std::make_unique<egl::MockManager>();
auto surface = std::make_unique<egl::MockWindowSurface>();

auto engine_ptr = engine.get();
auto surface_ptr = surface.get();

EngineModifier modifier{engine.get()};
Expand All @@ -250,12 +254,16 @@ TEST(FlutterWindowsViewTest, Shutdown) {
std::unique_ptr<FlutterWindowsView> view =
engine->CreateView(std::move(window_binding_handler));

auto view_id = view->view_id();
ViewModifier view_modifier{view.get()};
view_modifier.SetSurface(std::move(surface));

// The engine must be stopped before the surface can be destroyed.
FlutterWindowsViewController controller{std::move(engine), std::move(view)};

// The view must be removed before the surface can be destroyed.
InSequence s;
EXPECT_CALL(*engine.get(), Stop).Times(1);
EXPECT_CALL(*engine_ptr, running).WillOnce(Return(true));
EXPECT_CALL(*engine_ptr, RemoveView(view_id)).Times(1);
EXPECT_CALL(*surface_ptr, Destroy).Times(1);
}
}
Expand Down Expand Up @@ -1392,7 +1400,6 @@ TEST(FlutterWindowsViewTest, DisablesVSyncAtStartup) {
EXPECT_CALL(*surface_ptr, SetVSyncEnabled(false)).WillOnce(Return(true));
EXPECT_CALL(render_context, ClearCurrent).WillOnce(Return(true));

EXPECT_CALL(*engine.get(), Stop).Times(1);
EXPECT_CALL(*surface_ptr, Destroy).Times(1);

EngineModifier modifier{engine.get()};
Expand Down Expand Up @@ -1430,7 +1437,6 @@ TEST(FlutterWindowsViewTest, EnablesVSyncAtStartup) {
EXPECT_CALL(*surface_ptr, SetVSyncEnabled(true)).WillOnce(Return(true));
EXPECT_CALL(render_context, ClearCurrent).WillOnce(Return(true));

EXPECT_CALL(*engine.get(), Stop).Times(1);
EXPECT_CALL(*surface_ptr, Destroy).Times(1);

EngineModifier modifier{engine.get()};
Expand Down Expand Up @@ -1471,7 +1477,6 @@ TEST(FlutterWindowsViewTest, DisablesVSyncAfterStartup) {
EXPECT_CALL(*surface_ptr, MakeCurrent).WillOnce(Return(true));
EXPECT_CALL(*surface_ptr, SetVSyncEnabled(false)).WillOnce(Return(true));
EXPECT_CALL(render_context, ClearCurrent).WillOnce(Return(true));
EXPECT_CALL(*engine.get(), Stop).Times(1);
EXPECT_CALL(*surface_ptr, Destroy).Times(1);

EngineModifier modifier{engine.get()};
Expand Down Expand Up @@ -1514,7 +1519,6 @@ TEST(FlutterWindowsViewTest, EnablesVSyncAfterStartup) {
EXPECT_CALL(*surface_ptr, MakeCurrent).WillOnce(Return(true));
EXPECT_CALL(*surface_ptr, SetVSyncEnabled(true)).WillOnce(Return(true));
EXPECT_CALL(render_context, ClearCurrent).WillOnce(Return(true));
EXPECT_CALL(*engine.get(), Stop).Times(1);
EXPECT_CALL(*surface_ptr, Destroy).Times(1);

EngineModifier modifier{engine.get()};
Expand Down Expand Up @@ -1563,7 +1567,6 @@ TEST(FlutterWindowsViewTest, UpdatesVSyncOnDwmUpdates) {
EXPECT_CALL(*surface_ptr, SetVSyncEnabled(false)).WillOnce(Return(true));
EXPECT_CALL(render_context, ClearCurrent).WillOnce(Return(true));

EXPECT_CALL(*engine.get(), Stop).Times(1);
EXPECT_CALL(*surface_ptr, Destroy).Times(1);

EngineModifier engine_modifier{engine.get()};
Expand Down