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

[windows] Separate the engine from the view#19896

Merged
stuartmorgan-g merged 4 commits intoflutter:masterfrom
stuartmorgan-g:windows-engine-api
Aug 5, 2020
Merged

[windows] Separate the engine from the view#19896
stuartmorgan-g merged 4 commits intoflutter:masterfrom
stuartmorgan-g:windows-engine-api

Conversation

@stuartmorgan-g
Copy link
Copy Markdown
Contributor

Description

Refactors the Windows embedding internals to make an engine object that
owns things associated with the engine rather than the view, and updates
the API surface to allow using the engine directly.

This is an incremental step toward both a cleaner, non-struct-based
internal structure and a finalized API surface.

Related Issues

flutter/flutter#52748

Tests

I added the following tests: New engine wrapper class tests

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the contributor guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the C++, Objective-C, Java style guides for the engine.
  • I read the tree hygiene wiki page, which explains my responsibilities.
  • I updated/added relevant documentation.
  • All existing and new tests are passing.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

  • No, no existing tests failed, so this is not a breaking change.
  • Yes, this is a breaking change.

Refactors the Windows embedding internals to make an engine object that
owns things associated with the engine rather than the view, and updates
the API surface to allow using the engine directly.

This is an incremental step toward both a cleaner, non-struct-based
internal structure and a finalized API surface.
Copy link
Copy Markdown

@clarkezone clarkezone left a comment

Choose a reason for hiding this comment

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

Great change, couple of comment related requests but otherwise LGTM


namespace flutter {

// An engine for running a headless Flutter application.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Since flutter is GUI framework I'm a little confused what a headless Flutter application is and why you need a special engine for that scenario. Might be worth expanding this comment a bit

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added a bit more context about the current state to the comment.

Headless engines are a very niche (other than some add-to-app prewarming scenarios on mobile I believe, but we don't support that). The more important thing is that this lays the foundation for actually separating the view and the engine in the wrapper API; in the future things like running the loop will be done via the engine object, not the view controller. However, there are messy create/destroy semantics I need to figure out for that given that this isn't refcounted and needs to be somewhat coherent at both the wrapper and the C layers, so I didn't want to try to do that in this PR.


class FlutterWindowsView;

// Manages state associated with the underlying FlutterEngine.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Since there is "Windowsness" here would be good to have a bit more detail in this comment.. Is this class adding "headedness" to FlutterEngine? Similar to comment on engine, might be good to add a bit more to this comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Expanded slightly. The Windows in the name is to indicate its relationship to flutter_windows and FlutterWindowsView; flutter_windows has two major support classes, the view part (FlutterWindowsView) and the engine part (FlutterWindowsEngine). (And, honestly, because with FlutterEngine already being a name in the public API surface, I can't call it that without changing the namespace used internally. I might have to do that eventually, but for now this seemed like a simple way to differentiate them.)

Copy link
Copy Markdown

@clarkezone clarkezone left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants