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

Remove GFX branches from Flutter engine #44401

Merged
auto-submit merged 11 commits into from
Aug 8, 2023
Merged

Remove GFX branches from Flutter engine #44401

auto-submit merged 11 commits into from
Aug 8, 2023

Conversation

@caroqliu
Copy link
Contributor

@caroqliu caroqliu commented Aug 4, 2023

This change does the following:

  • Refactors engine.cc/.h and component_v2.cc/.h to remove GFX ctors and callbacks.
  • Removes gfx_external_view_embedder, gfx_platform_view, gfx_session_connection, their tests, and any supporting testing infrastructure (e.g. fake_session) from the build. This change does not delete the source files themselves in order to keep the change size smaller (as all these file deletions together are about another ~8K lines worth of deletions.) A separate follow up change will delete these files.

Related bug: fxbug.dev/64206

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
  • 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.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the [CLA].
  • All existing and new tests are passing.

@arbreng arbreng self-requested a review August 4, 2023 21:55
arbreng
arbreng reviewed Aug 4, 2023
View reviewed changes
Copy link
Contributor

@arbreng arbreng left a comment

Choose a reason for hiding this comment

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

Please rename all of the "flatland_" files by dropping the "flatland_" prefix

Flatland is the only implementation of these interfaces now, and the name will not be meaningful to other contributors in this repository who have to change these files. Names like "platform_view" and "external_view_embedder" will however. Dropping the prefix removes cognitive overhead for Flutter contributors.

For "platform_view", "flatland_platform_view.cc" should be collapsed back into "platform_view.cc" and the various callbacks removed. They add complexity and overhead which is no longer necessary.

I would greatly prefer these changes be made as part of this PR, but an immediate follow-up is acceptable if you don't want to bloat the line count. However, punting on the work is not acceptable to me at this time.

arbreng
arbreng reviewed Aug 4, 2023
View reviewed changes
@caroqliu
Copy link
Contributor Author

caroqliu commented Aug 7, 2023

Please rename all of the "flatland_" files by dropping the "flatland_" prefix

Flatland is the only implementation of these interfaces now, and the name will not be meaningful to other contributors in this repository who have to change these files. Names like "platform_view" and "external_view_embedder" will however. Dropping the prefix removes cognitive overhead for Flutter contributors.

For "platform_view", "flatland_platform_view.cc" should be collapsed back into "platform_view.cc" and the various callbacks removed. They add complexity and overhead which is no longer necessary.

I would greatly prefer these changes be made as part of this PR, but an immediate follow-up is acceptable if you don't want to bloat the line count. However, punting on the work is not acceptable to me at this time.

I've removed the flatland_ prefixes as suggested, with the following exceptions:

  • flatland_ itself, which is referring to the composition API. (Let me know if this should be renamed as composition_ or some such.
  • flatland_connection_/FlatlandConnection, since this is referring to the connection with the composition API. I assume Connection on its own is too vague, but let me know if you prefer CompositionConnection or some such.

I've also added the Callback suffix to the existing callback types related to the PlatformView, since OnCreateView is overloaded with the refactor (both as the function and as the callback type). I added Callback to distinguish these two in particular, and added Callback to the other types so they would have a consistent naming style. LMK if you have any thoughts on this as well.

I'm sending an early unpolished version to CI before I head out for the day; if you have any suggestions for rearranging code blocks (e.g. with FlatlandPlatformView and PlatformView being merged together), or anything like that, please weigh in as these are still flexible.

Thanks for the review!

arbreng
arbreng approved these changes Aug 8, 2023
View reviewed changes
Copy link
Contributor

@arbreng arbreng left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Most excellent!

Just looks like some small license issues to clean up, then we can land it.

@chinmaygarde chinmaygarde added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 8, 2023
@auto-submit auto-submit bot merged commit dd03fae into flutter:main Aug 8, 2023
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 8, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 8, 2023
@engine-flutter-autoroll
dd03fae51 Remove GFX branches from Flutter engine (flutter/engine#44401)
788e904
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Aug 8, 2023
@engine-flutter-autoroll
Roll Flutter Engine from 22bd35a19352 to dd03fae51d38 (3 revisions) (#…
e2badf3 
…132149)

flutter/engine@22bd35a...dd03fae

2023-08-08 [email protected] Remove GFX branches from Flutter engine (flutter/engine#44401)
2023-08-08 [email protected] Use the Clang unreachable code warning flag in the engine tree (flutter/engine#44458)
2023-08-08 [email protected] Roll Skia from 66ba512c613c to 30c0319e7e42 (3 revisions) (flutter/engine#44500)

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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@caroqliu caroqliu mentioned this pull request Aug 10, 2023
Merged
8 tasks
arbreng pushed a commit that referenced this pull request Aug 14, 2023
@caroqliu
[fuchsia] Delete obsolete GFX-specific source files (#44594)
21aed05 
This change removes 
- GFX source files removed from the BUILD in #44401 
- Extraneous unused Scenic includes in the same directory
(`//shell/platform/fuchsia/flutter/`)
- All remaining uses of the Scenic library from the Fuchsia SDK
- All remaining uses of fuchsia.ui.scenic.*
@reidbaker reidbaker mentioned this pull request Aug 15, 2023
Merged
14 tasks
gaaclarke pushed a commit to gaaclarke/engine that referenced this pull request Aug 30, 2023
@caroqliu @gaaclarke
Remove GFX branches from Flutter engine (flutter#44401)
472501c 
This change does the following:
- Refactors `engine.cc`/`.h` and `component_v2.cc`/`.h` to remove GFX ctors and callbacks.
- Removes `gfx_external_view_embedder`, `gfx_platform_view`, `gfx_session_connection`, their tests, and any supporting testing infrastructure (e.g. `fake_session`) from the build. This change does not delete the source files themselves in order to keep the change size smaller (as all these file deletions together are about another ~8K lines worth of deletions.) A separate follow up change will delete these files.

Related bug: fxbug.dev/64206
gaaclarke pushed a commit to gaaclarke/engine that referenced this pull request Aug 30, 2023
@caroqliu @gaaclarke
[fuchsia] Delete obsolete GFX-specific source files (flutter#44594)
97098b5 
This change removes 
- GFX source files removed from the BUILD in flutter#44401 
- Extraneous unused Scenic includes in the same directory
(`//shell/platform/fuchsia/flutter/`)
- All remaining uses of the Scenic library from the Fuchsia SDK
- All remaining uses of fuchsia.ui.scenic.*
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@chinmaygarde chinmaygarde chinmaygarde approved these changes

@arbreng arbreng arbreng approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App platform-fuchsia

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@caroqliu @chinmaygarde @arbreng