Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
500e60c
Define structs, class
yaakovschectman Feb 8, 2024
8dd4543
Define PV type registration runner API function
yaakovschectman Feb 8, 2024
ae0d049
Bare minimum PlatformViewHandler
yaakovschectman Feb 8, 2024
5f14af0
Compositor owns platform view manager
yaakovschectman Feb 8, 2024
200f13a
Set up for mocking
yaakovschectman Feb 8, 2024
ed90810
Add unit test
yaakovschectman Feb 13, 2024
5d410ff
Formatting
yaakovschectman Feb 13, 2024
484c8c1
License fix, await future
yaakovschectman Feb 13, 2024
b8fadd7
PR Feedback
yaakovschectman Feb 13, 2024
1d203f4
_internal header file
yaakovschectman Feb 14, 2024
b9acc1a
Wait to add manager to compositors
yaakovschectman Feb 14, 2024
35eb688
Rename fixture function
yaakovschectman Feb 14, 2024
659ebd6
Reorder parameters
yaakovschectman Feb 14, 2024
db2851b
Use constants
yaakovschectman Feb 14, 2024
db48ddd
Reorder params in test
yaakovschectman Feb 14, 2024
f87ed96
Format
yaakovschectman Feb 14, 2024
42f976e
License
yaakovschectman Feb 14, 2024
284b309
Break up PlatformViewManager
yaakovschectman Feb 16, 2024
e000930
PR Feedback
yaakovschectman Feb 16, 2024
676868f
Formatting
yaakovschectman Feb 16, 2024
6e6ef7e
PR Feedback
yaakovschectman Feb 16, 2024
22a9e46
Update shell/platform/windows/flutter_windows_engine_unittests.cc
yaakovschectman Feb 21, 2024
95a8498
Update shell/platform/windows/platform_view_manager.h
yaakovschectman Feb 21, 2024
1fa97f3
Update shell/platform/windows/public/flutter_windows.h
yaakovschectman Feb 21, 2024
5453b29
Update shell/platform/windows/flutter_windows_engine_unittests.cc
yaakovschectman Feb 21, 2024
dfa6b73
PR Feedback
yaakovschectman Feb 21, 2024
cde468e
Merge branch 'main' into win_pv_i
yaakovschectman Feb 21, 2024
585ca11
Include internal header
yaakovschectman Feb 22, 2024
54d91a3
Merge branch 'main' into win_pv_i
yaakovschectman Feb 22, 2024
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
3 changes: 3 additions & 0 deletions shell/platform/windows/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,8 @@ source_set("flutter_windows_source") {
"keyboard_utils.h",
"platform_handler.cc",
"platform_handler.h",
"platform_view_manager.cc",
"platform_view_manager.h",
"sequential_id_generator.cc",
"sequential_id_generator.h",
"settings_plugin.cc",
Expand Down Expand Up @@ -220,6 +222,7 @@ executable("flutter_windows_unittests") {
"testing/flutter_windows_engine_builder.cc",
"testing/flutter_windows_engine_builder.h",
"testing/mock_direct_manipulation.h",
"testing/mock_platform_view_manager.h",
"testing/mock_text_input_manager.cc",
"testing/mock_text_input_manager.h",
"testing/mock_window.cc",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,16 @@ bool FlutterDesktopEngineProcessExternalWindowMessage(
return false;
}

void FlutterDesktopEngineRegisterPlatformViewType(
FlutterDesktopEngineRef engine,
const char* view_type_name,
FlutterPlatformViewTypeEntry view_type) {
if (s_stub_implementation) {
s_stub_implementation->EngineRegisterPlatformViewType(view_type_name,
view_type);
}
}

FlutterDesktopViewRef FlutterDesktopPluginRegistrarGetView(
FlutterDesktopPluginRegistrarRef controller) {
// The stub ignores this, so just return an arbitrary non-zero value.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,11 @@ class StubFlutterWindowsApi {
// Called for FlutterDesktopEngineReloadSystemFonts.
virtual void EngineReloadSystemFonts() {}

// Called for FlutterDesktopEngineRegisterPlatformViewType.
virtual void EngineRegisterPlatformViewType(
const char* view_type_name,
FlutterPlatformViewTypeEntry view_type) {}

// Called for FlutterDesktopViewGetHWND.
virtual HWND ViewGetHWND() { return reinterpret_cast<HWND>(1); }

Expand Down
6 changes: 6 additions & 0 deletions shell/platform/windows/compositor.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@

#include "flutter/shell/platform/embedder/embedder.h"

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

namespace flutter {

// Enables the Flutter engine to render content on Windows.
Expand All @@ -19,6 +21,7 @@ namespace flutter {
// Platform views are not yet supported.
class Compositor {
public:
Compositor(PlatformViewManager* manager) : platform_view_manager_(manager) {}
Copy link
Member

@loic-sharma loic-sharma Feb 13, 2024

Choose a reason for hiding this comment

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

Compositor is just an interface that knows nothing about the implementation. I'd remove the platform_view_manager_ field from this type and move it to the software & opengl compositors.

... but since these compositors don't use this platform view manager yet, I wouldn't update them to accept this platform view manager yet.

virtual ~Compositor() = default;

// Creates a backing store used for rendering Flutter content.
Expand All @@ -32,6 +35,9 @@ class Compositor {

// Present Flutter content and platform views onto the view.
virtual bool Present(const FlutterLayer** layers, size_t layers_count) = 0;

protected:
PlatformViewManager* platform_view_manager_;
Copy link
Member

Choose a reason for hiding this comment

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

Consider documenting that this is a weak pointer, and that the PlatformViewManager is owned by FlutterEngine.

Copy link
Member

@loic-sharma loic-sharma Feb 13, 2024

Choose a reason for hiding this comment

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

Should we make this be a std::shared_ptr to avoid raw pointers? We're not creating tons of these objects so the performance impact should be acceptable.

};

} // namespace flutter
Expand Down
5 changes: 3 additions & 2 deletions shell/platform/windows/compositor_opengl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,10 @@ int GetSupportedTextureFormat(const impeller::DescriptionGLES* description) {

} // namespace

CompositorOpenGL::CompositorOpenGL(FlutterWindowsEngine* engine,
CompositorOpenGL::CompositorOpenGL(PlatformViewManager* manager,
FlutterWindowsEngine* engine,
impeller::ProcTableGLES::Resolver resolver)
: engine_(engine), resolver_(resolver) {}
: Compositor(manager), engine_(engine), resolver_(resolver) {}

bool CompositorOpenGL::CreateBackingStore(
const FlutterBackingStoreConfig& config,
Expand Down
3 changes: 2 additions & 1 deletion shell/platform/windows/compositor_opengl.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ namespace flutter {
// Enables the Flutter engine to render content on Windows using OpenGL.
class CompositorOpenGL : public Compositor {
public:
CompositorOpenGL(FlutterWindowsEngine* engine,
CompositorOpenGL(PlatformViewManager* manager,
FlutterWindowsEngine* engine,
impeller::ProcTableGLES::Resolver resolver);

/// |Compositor|
Expand Down
12 changes: 6 additions & 6 deletions shell/platform/windows/compositor_opengl_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ class CompositorOpenGLTest : public WindowsTest {
TEST_F(CompositorOpenGLTest, CreateBackingStore) {
UseHeadlessEngine();

auto compositor = CompositorOpenGL{engine(), kMockResolver};
auto compositor = CompositorOpenGL{nullptr, engine(), kMockResolver};

FlutterBackingStoreConfig config = {};
FlutterBackingStore backing_store = {};
Expand All @@ -127,7 +127,7 @@ TEST_F(CompositorOpenGLTest, CreateBackingStore) {
TEST_F(CompositorOpenGLTest, InitializationFailure) {
UseHeadlessEngine();

auto compositor = CompositorOpenGL{engine(), kMockResolver};
auto compositor = CompositorOpenGL{nullptr, engine(), kMockResolver};

FlutterBackingStoreConfig config = {};
FlutterBackingStore backing_store = {};
Expand All @@ -140,7 +140,7 @@ TEST_F(CompositorOpenGLTest, InitializationFailure) {
TEST_F(CompositorOpenGLTest, Present) {
UseEngineWithView();

auto compositor = CompositorOpenGL{engine(), kMockResolver};
auto compositor = CompositorOpenGL{nullptr, engine(), kMockResolver};

FlutterBackingStoreConfig config = {};
FlutterBackingStore backing_store = {};
Expand All @@ -164,7 +164,7 @@ TEST_F(CompositorOpenGLTest, Present) {
TEST_F(CompositorOpenGLTest, PresentEmpty) {
UseEngineWithView();

auto compositor = CompositorOpenGL{engine(), kMockResolver};
auto compositor = CompositorOpenGL{nullptr, engine(), kMockResolver};

// The context will be bound twice: first to initialize the compositor, second
// to clear the surface.
Expand All @@ -177,7 +177,7 @@ TEST_F(CompositorOpenGLTest, PresentEmpty) {
TEST_F(CompositorOpenGLTest, HeadlessPresentIgnored) {
UseHeadlessEngine();

auto compositor = CompositorOpenGL{engine(), kMockResolver};
auto compositor = CompositorOpenGL{nullptr, engine(), kMockResolver};

FlutterBackingStoreConfig config = {};
FlutterBackingStore backing_store = {};
Expand All @@ -199,7 +199,7 @@ TEST_F(CompositorOpenGLTest, HeadlessPresentIgnored) {
TEST_F(CompositorOpenGLTest, NoSurfaceIgnored) {
UseEngineWithView(/*add_surface = */ false);

auto compositor = CompositorOpenGL{engine(), kMockResolver};
auto compositor = CompositorOpenGL{nullptr, engine(), kMockResolver};

FlutterBackingStoreConfig config = {};
FlutterBackingStore backing_store = {};
Expand Down
5 changes: 3 additions & 2 deletions shell/platform/windows/compositor_software.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,9 @@

namespace flutter {

CompositorSoftware::CompositorSoftware(FlutterWindowsEngine* engine)
: engine_(engine) {}
CompositorSoftware::CompositorSoftware(PlatformViewManager* manager,
FlutterWindowsEngine* engine)
: Compositor(manager), engine_(engine) {}

bool CompositorSoftware::CreateBackingStore(
const FlutterBackingStoreConfig& config,
Expand Down
3 changes: 2 additions & 1 deletion shell/platform/windows/compositor_software.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ namespace flutter {
// rasterization and bitmaps.
class CompositorSoftware : public Compositor {
public:
CompositorSoftware(FlutterWindowsEngine* engine);
CompositorSoftware(PlatformViewManager* manager,
FlutterWindowsEngine* engine);

/// |Compositor|
bool CreateBackingStore(const FlutterBackingStoreConfig& config,
Expand Down
8 changes: 4 additions & 4 deletions shell/platform/windows/compositor_software_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ class CompositorSoftwareTest : public WindowsTest {
TEST_F(CompositorSoftwareTest, CreateBackingStore) {
UseHeadlessEngine();

auto compositor = CompositorSoftware{engine()};
auto compositor = CompositorSoftware{nullptr, engine()};

FlutterBackingStoreConfig config = {};
FlutterBackingStore backing_store = {};
Expand All @@ -88,7 +88,7 @@ TEST_F(CompositorSoftwareTest, CreateBackingStore) {
TEST_F(CompositorSoftwareTest, Present) {
UseEngineWithView();

auto compositor = CompositorSoftware{engine()};
auto compositor = CompositorSoftware{nullptr, engine()};

FlutterBackingStoreConfig config = {};
FlutterBackingStore backing_store = {};
Expand All @@ -109,7 +109,7 @@ TEST_F(CompositorSoftwareTest, Present) {
TEST_F(CompositorSoftwareTest, PresentEmpty) {
UseEngineWithView();

auto compositor = CompositorSoftware{engine()};
auto compositor = CompositorSoftware{nullptr, engine()};

EXPECT_CALL(*view(), ClearSoftwareBitmap).WillOnce(Return(true));
EXPECT_TRUE(compositor.Present(nullptr, 0));
Expand All @@ -118,7 +118,7 @@ TEST_F(CompositorSoftwareTest, PresentEmpty) {
TEST_F(CompositorSoftwareTest, HeadlessPresentIgnored) {
UseHeadlessEngine();

auto compositor = CompositorSoftware{engine()};
auto compositor = CompositorSoftware{nullptr, engine()};

FlutterBackingStoreConfig config = {};
FlutterBackingStore backing_store = {};
Expand Down
22 changes: 21 additions & 1 deletion shell/platform/windows/fixtures/main.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

import 'dart:async';
import 'dart:io' as io;
import 'dart:typed_data' show ByteData, Uint8List;
import 'dart:typed_data' show ByteData, Endian, Uint8List;
import 'dart:ui' as ui;
import 'dart:convert';

Expand Down Expand Up @@ -159,6 +159,26 @@ void enableLifecycleToFrom() async {
});
}

@pragma('vm:entry-point')
void sendCreationMethod() async {
final List<int> data = <int>[
// Method name
7, 'create'.length, ...utf8.encode('create'),
Copy link
Member

Choose a reason for hiding this comment

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

For readability, could we create constants for these? Something like:

const int valueNull = 0;
const int valueInt32 = 3;
const int valueString = 7;
const int valueMap = 13;

Copy link
Member

@cbracken cbracken Feb 14, 2024

Choose a reason for hiding this comment

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

I like it. Even better:

enum class ValueType {
  kNull = 0,
  kInt32 = 3,
  ...
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cbracken I think this appears applicable only for C++ code. For the dart file, the const ints may be the best we can do.

Copy link
Member

Choose a reason for hiding this comment

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

Agh yes, not sure where my mind was at there. Dart enhanced enums do provide a mechanism for this, but it's entirely up to you.

// Method arguments: {'type': 'type':, 'id': 0}
13, 2,
7, 'type'.length, ...utf8.encode('type'),
7, 'type'.length, ...utf8.encode('type'),
7, 'id'.length, ...utf8.encode('id'),
3, 0, 0, 0, 0,
];

final Completer<ByteData?> completed = Completer<ByteData?>();
final ByteData bytes = ByteData.sublistView(Uint8List.fromList(data));
ui.PlatformDispatcher.instance.sendPlatformMessage('flutter/platform_views', bytes, (ByteData? response) {
completed.complete(response);
});
}

@pragma('vm:entry-point')
void customEntrypoint() {}

Expand Down
7 changes: 7 additions & 0 deletions shell/platform/windows/flutter_windows.cc
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,13 @@ bool FlutterDesktopEngineProcessExternalWindowMessage(
return lresult.has_value();
}

void FlutterDesktopEngineRegisterPlatformViewType(
FlutterDesktopEngineRef engine,
const char* view_type_name,
FlutterPlatformViewTypeEntry view_type) {
// TODO(schectman): forward to platform view manager
}

FlutterDesktopViewRef FlutterDesktopPluginRegistrarGetView(
FlutterDesktopPluginRegistrarRef registrar) {
return HandleForView(registrar->engine->view());
Expand Down
10 changes: 8 additions & 2 deletions shell/platform/windows/flutter_windows_engine.cc
Original file line number Diff line number Diff line change
Expand Up @@ -380,14 +380,20 @@ bool FlutterWindowsEngine::Run(std::string_view entrypoint) {

args.custom_task_runners = &custom_task_runners;

if (!platform_view_manager_) {
platform_view_manager_ = std::make_unique<PlatformViewManager>(
task_runner_.get(), messenger_wrapper_.get());
}
if (egl_manager_) {
auto resolver = [](const char* name) -> void* {
return reinterpret_cast<void*>(::eglGetProcAddress(name));
};

compositor_ = std::make_unique<CompositorOpenGL>(this, resolver);
compositor_ = std::make_unique<CompositorOpenGL>(
platform_view_manager_.get(), this, resolver);
} else {
compositor_ = std::make_unique<CompositorSoftware>(this);
compositor_ = std::make_unique<CompositorSoftware>(
platform_view_manager_.get(), this);
}

FlutterCompositor compositor = {};
Expand Down
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 @@ -137,6 +137,8 @@ class FlutterWindowsEngine {

TaskRunner* task_runner() { return task_runner_.get(); }

BinaryMessenger* messenger_wrapper() { return messenger_wrapper_.get(); }

FlutterWindowsTextureRegistrar* texture_registrar() {
return texture_registrar_.get();
}
Expand Down Expand Up @@ -427,6 +429,8 @@ class FlutterWindowsEngine {

std::shared_ptr<egl::ProcTable> gl_;

std::unique_ptr<PlatformViewManager> platform_view_manager_;

FML_DISALLOW_COPY_AND_ASSIGN(FlutterWindowsEngine);
};

Expand Down
25 changes: 25 additions & 0 deletions shell/platform/windows/flutter_windows_engine_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "flutter/shell/platform/windows/testing/egl/mock_manager.h"
#include "flutter/shell/platform/windows/testing/engine_modifier.h"
#include "flutter/shell/platform/windows/testing/flutter_windows_engine_builder.h"
#include "flutter/shell/platform/windows/testing/mock_platform_view_manager.h"
#include "flutter/shell/platform/windows/testing/mock_window_binding_handler.h"
#include "flutter/shell/platform/windows/testing/mock_windows_proc_table.h"
#include "flutter/shell/platform/windows/testing/test_keyboard.h"
Expand Down Expand Up @@ -1169,5 +1170,29 @@ TEST_F(FlutterWindowsEngineTest, ChannelListenedTo) {
}
}

TEST_F(FlutterWindowsEngineTest, ReceivePlatformViewMessage) {
Copy link
Member

Choose a reason for hiding this comment

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

From my understanding, this file is more for engine unit tests. Since this launches & runs a full app, should we move this to the integration tests in flutter_windows_unittests.cc?

I don't feel strongly about this, feel free to keep as is.

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 seems to me that this test is quite similar in most regards to the handful of unit tests immediately preceding it in this file. Would you disagree?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's probably fine to land as-is, but I do think we should probably do a followup evaluation of which tests belong where. We're definitely not super consistent and to be honest, I'm not sure we've ever drawn a really clear line (not just in the Windows embedder but also on macOS).

FlutterWindowsEngineBuilder builder{GetContext()};
builder.SetDartEntrypoint("sendCreationMethod");
auto engine = builder.Build();

EngineModifier modifier(engine.get());
modifier.embedder_api().RunsAOTCompiledDartCode = []() { return false; };

bool received_call = false;

auto manager = std::make_unique<MockPlatformViewManager>(engine.get());
EXPECT_CALL(*manager, QueuePlatformViewCreation)
.WillRepeatedly([&](std::string_view type_name, int64_t id) {
received_call = true;
});
modifier.SetPlatformViewManager(std::move(manager));

engine->Run();

while (!received_call) {
engine->task_runner()->ProcessTasks();
}
}

} // namespace testing
} // namespace flutter
57 changes: 57 additions & 0 deletions shell/platform/windows/platform_view_manager.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
// 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/platform_view_manager.h"

#include "flutter/shell/platform/common/client_wrapper/include/flutter/standard_method_codec.h"

namespace flutter {

namespace {
constexpr char kChannelName[] = "flutter/platform_views";
}

PlatformViewManager::PlatformViewManager(TaskRunner* task_runner,
BinaryMessenger* binary_messenger)
: task_runner_(task_runner),
channel_(std::make_unique<MethodChannel<EncodableValue>>(
binary_messenger,
kChannelName,
&StandardMethodCodec::GetInstance())) {
channel_->SetMethodCallHandler(
[this](const MethodCall<EncodableValue>& call,
std::unique_ptr<MethodResult<EncodableValue>> result) {
const auto& args = std::get<EncodableMap>(*call.arguments());
if (call.method_name() == "create") {
const auto& type =
std::get<std::string>(args.find(EncodableValue("type"))->second);
const auto& id =
std::get<std::int32_t>(args.find(EncodableValue("id"))->second);
QueuePlatformViewCreation(type, id);
}
result->Success();
});
}

PlatformViewManager::~PlatformViewManager() {}

void PlatformViewManager::QueuePlatformViewCreation(std::string_view type_name,
int64_t id) {}

void PlatformViewManager::InstantiatePlatformView(int64_t id) {}

void PlatformViewManager::RegisterPlatformViewType(
std::string_view type_name,
const FlutterPlatformViewTypeEntry& type) {}

void PlatformViewManager::FocusPlatformView(int64_t id,
FocusChangeDirection direction,
bool focus) {}

std::optional<HWND> PlatformViewManager::GetNativeHandleForId(
int64_t id) const {
return std::nullopt;
}

} // namespace flutter
Loading