Delete engine v1 android embedding #51229

gmackall · 2024-03-06T16:53:14Z

Also fixes a random typo I found

~~TODO to test this~~ (no more todo):
-~~test the framework against this as well, probably with a dummy PR changing the engine commit to my branch if this is possible~~ not possible, made a best effort removal of framework code in flutter/flutter#144726.
-~~figure out if the old embedding is used in g3 at all~~ removed all uses
-~~figure out exactly what the ShimPluginRegistry/ShimRegistrar are doing, and if fully deleting them was right~~ (see #51229 (comment))

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 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.

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

gmackall · 2024-03-06T20:50:19Z

It seems like the ShimPluginRegistry is used when an app w/ embedding 2 is using a plugin w/ embedding 1, therefore making it probably also fair game to delete?
https://github.com/flutter/flutter/blob/f38c5ad44141d3b7711dddd5bbf9c8862a5ed68a/packages/flutter_tools/lib/src/flutter_plugins.dart#L401

There are tests that this will make fail in the framework, so I'll need to address those before landing this.

shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java

gmackall · 2024-03-06T22:04:38Z

This will be blocked until I finish and land flutter/flutter#144726, but should be ready for review now. Please inform me if you believe I've deleted something that shouldn't be deleted.

flutter-dashboard · 2024-03-06T22:09:05Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #51229 at sha 2d3f5d6

johnmccutchan · 2024-03-06T22:24:43Z

This is so exciting!!!

gmackall · 2024-03-06T22:46:18Z

It seems like the ShimPluginRegistry is used when an app w/ embedding 2 is using a plugin w/ embedding 1, therefore making it probably also fair game to delete? https://github.com/flutter/flutter/blob/f38c5ad44141d3b7711dddd5bbf9c8862a5ed68a/packages/flutter_tools/lib/src/flutter_plugins.dart#L401

But it also looks like plugins are always version 2, so I'm not sure this is ever even used regardless

gmackall · 2024-03-06T22:53:43Z

Looks like there was a doc here, and another at go/flutter-delete-android-v1-embedding (I've requested ownership)

Pre work for flutter/engine#51229. Removes a lot of code referencing v1 of the android embedding, though not necessarily all of it (I may have missed some, it is hard to know). Will hopefully make landing that PR less painful (or maybe painless?)

gmackall · 2024-04-03T17:51:58Z

Just waiting on one more internal CL to land 🙏

gmackall · 2024-04-09T17:28:37Z

Last internal CL has landed, so this is unblocked. I've removed framework code referencing the v1 embedding, and removed internal uses, so at this point the path should be cleared for this removal.

That said, there is still a decent chance landing this will uncover more places it is being referenced, so I'll be watching closely to see if a revert->fix->reland is needed.

gmackall · 2024-04-09T19:08:53Z

Reason for revert: blocking engine->framework roll (I missed some framework code referencing the v1 embedding).

This reverts commit b735b76.

Reverts: #51229 Initiated by: gmackall Reason for reverting: blocking engine->framework roll (I missed some framework code referencing the v1 embedding). Original PR Author: gmackall Reviewed By: {matanlurey, reidbaker} This change reverts the following previous change: Fixes flutter/flutter#143531 Also fixes a random typo I found ~TODO to test this~ (no more todo): -~test the framework against this as well, probably with a dummy PR changing the engine commit to my branch if this is possible~ not possible, made a best effort removal of framework code in flutter/flutter#144726. -~figure out if the old embedding is used in g3 at all~ removed all uses -~figure out exactly what the ShimPluginRegistry/ShimRegistrar are doing, and if fully deleting them was right~ (see #51229 (comment)) [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

…146524) flutter/engine@5a824e2...5dca50d 2024-04-09 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Delete engine v1 android embedding (#51229)" (flutter/engine#51996) 2024-04-09 34871572+gmackall@users.noreply.github.com Delete engine v1 android embedding (flutter/engine#51229) 2024-04-09 flar@google.com better output from engine layer unit test failures (flutter/engine#51975) 2024-04-09 zanderso@users.noreply.github.com Revert "Roll Dart SDK from 78174b41ab0f to 7a5e410f982e (1 revision) (#51980)" (flutter/engine#51990) 2024-04-09 jacksongardner@google.com [skwasm] Reify the SkPicture pointer as the right type. (flutter/engine#51991) 2024-04-09 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Roll Dart SDK from 7a5e410f982e to db99af14c4bc (1 revision) (#51986)" (flutter/engine#51989) 2024-04-09 jason-simmons@users.noreply.github.com Move the Dart SDK to //flutter/third_party/dart (flutter/engine#51917) 2024-04-09 matej.knopp@gmail.com [macOS] Implement hit testing and handle platform view cursor changes (flutter/engine#43101) 2024-04-09 skia-flutter-autoroll@skia.org Roll Dart SDK from 7a5e410f982e to db99af14c4bc (1 revision) (flutter/engine#51986) 2024-04-09 skia-flutter-autoroll@skia.org Roll Dart SDK from 78174b41ab0f to 7a5e410f982e (1 revision) (flutter/engine#51980) 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 chinmaygarde@google.com,rmistry@google.com,zra@google.com 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…lutter#51996)" This reverts commit 5dca50d.

flutter/engine#51229 blocked [the roll](#146522) and had to be reverted, which is a shame, but on the bright side it made it possible to point the framework at my removal pr, at the point of its merging the first time This fixes all errors that are fixable in the framework that would have blocked the roll. There are some that aren't fixable here (they need to be fixed in the engine)*, so I'll fix those in the engine but unfortunately I can't pick up another version here to re-test until I try to roll again ð��¢ *This category is: uses of plugins that in turn have a `registerWith`, that references the v1 embedding. The plan to fix these cases is to leave the interface that that method relies on around for now. See flutter/packages#6494 (comment) for details

…lutter#146524) flutter/engine@5a824e2...5dca50d 2024-04-09 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Delete engine v1 android embedding (flutter#51229)" (flutter/engine#51996) 2024-04-09 34871572+gmackall@users.noreply.github.com Delete engine v1 android embedding (flutter/engine#51229) 2024-04-09 flar@google.com better output from engine layer unit test failures (flutter/engine#51975) 2024-04-09 zanderso@users.noreply.github.com Revert "Roll Dart SDK from 78174b41ab0f to 7a5e410f982e (1 revision) (flutter#51980)" (flutter/engine#51990) 2024-04-09 jacksongardner@google.com [skwasm] Reify the SkPicture pointer as the right type. (flutter/engine#51991) 2024-04-09 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Roll Dart SDK from 7a5e410f982e to db99af14c4bc (1 revision) (flutter#51986)" (flutter/engine#51989) 2024-04-09 jason-simmons@users.noreply.github.com Move the Dart SDK to //flutter/third_party/dart (flutter/engine#51917) 2024-04-09 matej.knopp@gmail.com [macOS] Implement hit testing and handle platform view cursor changes (flutter/engine#43101) 2024-04-09 skia-flutter-autoroll@skia.org Roll Dart SDK from 7a5e410f982e to db99af14c4bc (1 revision) (flutter/engine#51986) 2024-04-09 skia-flutter-autoroll@skia.org Roll Dart SDK from 78174b41ab0f to 7a5e410f982e (1 revision) (flutter/engine#51980) 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 chinmaygarde@google.com,rmistry@google.com,zra@google.com 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…146523) flutter/engine#51229 blocked [the roll](flutter#146522) and had to be reverted, which is a shame, but on the bright side it made it possible to point the framework at my removal pr, at the point of its merging the first time This fixes all errors that are fixable in the framework that would have blocked the roll. There are some that aren't fixable here (they need to be fixed in the engine)*, so I'll fix those in the engine but unfortunately I can't pick up another version here to re-test until I try to roll again ð��¢ *This category is: uses of plugins that in turn have a `registerWith`, that references the v1 embedding. The plan to fix these cases is to leave the interface that that method relies on around for now. See flutter/packages#6494 (comment) for details

I may have deleted too much, and also not enough

a257dd1

github-actions bot added the platform-android label Mar 6, 2024

Gray Mackall added 15 commits March 6, 2024 08:58

format

f9d2577

remove deleted files from build.gn

57dc421

actually do what the previous commit said

4ba2e91

remove comment reference to deleted file

cabf156

remove most shimplugin tests

f3d9061

remove all of a test class

f49c76f

change licenses file

d4e5232

experimental delete

5fea64f

remove the removed files

3697b64

javadoc

afa562f

another one

08ecdd5

fix a random typo

934cab8

make gary's change to the manifest

d27de87

remove file from build again (forget every time)

daced97

licenses and manifest changes

2d3f5d6

gmackall mentioned this pull request Mar 6, 2024

Remove embedding v1 code in framework flutter/flutter#144726

Merged

9 tasks

gmackall commented Mar 6, 2024

View reviewed changes

shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java Outdated Show resolved Hide resolved

gmackall marked this pull request as ready for review March 6, 2024 22:05

gmackall requested a review from a team March 6, 2024 22:05

gmackall changed the title ~~Delete engine v1 embedding [wip]~~ Delete engine v1 embedding Mar 6, 2024

flutter-dashboard bot added the will affect goldens label Mar 6, 2024

gmackall requested a review from matanlurey March 6, 2024 22:15

Merge branch 'main' into remove_ghosts

720afb4

gmackall mentioned this pull request Mar 26, 2024

Delete the 2018-era Android v1 embedder flutter/flutter#143531

Open

reidbaker approved these changes Mar 29, 2024

View reviewed changes

merge by accepting both deletes

e1b8ee0

matanlurey approved these changes Apr 3, 2024

View reviewed changes

gmackall and others added 2 commits April 9, 2024 10:24

Merge branch 'main' into remove_ghosts

59839dc

don't find another example

26b6b19

gmackall added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 9, 2024

gmackall changed the title ~~Delete engine v1 embedding~~ Delete engine v1 android embedding Apr 9, 2024

auto-submit bot merged commit b735b76 into flutter:main Apr 9, 2024
31 checks passed

engine-flutter-autoroll mentioned this pull request Apr 9, 2024

Roll Flutter Engine from 5a824e22deb2 to b735b76bf376 (9 revisions) flutter/flutter#146522

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 9, 2024

b735b76bf Delete engine v1 android embedding (flutter/engine#51229)

66f2c60

gmackall added the revert Label used to revert changes in a closed and merged pull request. label Apr 9, 2024

auto-submit bot pushed a commit that referenced this pull request Apr 9, 2024

Revert "Delete engine v1 android embedding (#51229)"

2ef523e

This reverts commit b735b76.

auto-submit bot mentioned this pull request Apr 9, 2024

Reverts "Delete engine v1 android embedding (#51229)" #51996

Merged

8 tasks

auto-submit bot removed the revert Label used to revert changes in a closed and merged pull request. label Apr 9, 2024

gmackall mentioned this pull request Apr 9, 2024

Remove additional references to engine v1 android embedding flutter/flutter#146523

Merged

9 tasks

engine-flutter-autoroll mentioned this pull request Apr 9, 2024

Roll Flutter Engine from 5a824e22deb2 to 5dca50d3e268 (10 revisions) flutter/flutter#146524

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 9, 2024

b735b76bf Delete engine v1 android embedding (flutter/engine#51229)

7a54421

gmackall pushed a commit to gmackall/engine that referenced this pull request Apr 10, 2024

Revert "Reverts "Delete engine v1 android embedding (flutter#51229)" (f…

6156079

…lutter#51996)" This reverts commit 5dca50d.

gmackall mentioned this pull request Apr 10, 2024

Delete v1 android engine embedding #52022

Draft

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete engine v1 android embedding #51229

Delete engine v1 android embedding #51229

gmackall commented Mar 6, 2024 •

edited

gmackall commented Mar 6, 2024 •

edited

gmackall commented Mar 6, 2024 •

edited

flutter-dashboard bot commented Mar 6, 2024

johnmccutchan commented Mar 6, 2024

gmackall commented Mar 6, 2024 •

edited

gmackall commented Mar 6, 2024

gmackall commented Apr 3, 2024

gmackall commented Apr 9, 2024

gmackall commented Apr 9, 2024

Delete engine v1 android embedding #51229

Delete engine v1 android embedding #51229

Conversation

gmackall commented Mar 6, 2024 • edited

Pre-launch Checklist

gmackall commented Mar 6, 2024 • edited

gmackall commented Mar 6, 2024 • edited

flutter-dashboard bot commented Mar 6, 2024

johnmccutchan commented Mar 6, 2024

gmackall commented Mar 6, 2024 • edited

gmackall commented Mar 6, 2024

gmackall commented Apr 3, 2024

gmackall commented Apr 9, 2024

gmackall commented Apr 9, 2024

gmackall commented Mar 6, 2024 •

edited

gmackall commented Mar 6, 2024 •

edited

gmackall commented Mar 6, 2024 •

edited

gmackall commented Mar 6, 2024 •

edited