Create FlutterActivity/FlutterFragment using light weight engine with FlutterEngineGroup #36963

Nayuta403 · 2022-10-24T13:59:30Z

fix:flutter/flutter#72742

The problems mentioned in #29293 have been completed

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.

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

GaryQian · 2022-10-25T17:13:30Z

@Nayuta403 Is this ready for review?

Nayuta403 · 2022-10-26T02:28:34Z

@GaryQian Yes. Also cc @xster @ColdPaleLight

chinmaygarde · 2022-10-27T20:28:59Z

cc @gaaclarke

gaaclarke

This looks awesome and timely, lgtm for the most part. I just had a few questions before approval.

shell/platform/android/io/flutter/embedding/android/FlutterActivity.java

shell/platform/android/io/flutter/embedding/android/FlutterFragment.java

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

...atform/android/test/io/flutter/embedding/android/FlutterActivityAndFragmentDelegateTest.java

Co-authored-by: gaaclarke <[email protected]>

skia-gold · 2022-11-11T06:45:29Z

Gold has detected about 72 new digest(s) on patchset 13.
View them at https://flutter-engine-gold.skia.org/cl/github/36963

chinmaygarde · 2022-11-17T21:27:07Z

Ping @gaaclarke. Looks like this is good for a re-review.

gaaclarke

@chinmaygarde So, the code looks good to me. Here is my concern: I don't understand the need for the code. This is dealing with an aspect of Android development I'm not familiar with. I would like someone from the Android team to verify that this code is needed and appropriate. If this code is just providing a convenience, I'd error on the side of caution and avoid adding more logic, unless of course the alternative is super inconvenient. cc @reidbaker who is ramping up but has some context on the FlutterEngineGroup and would have a good idea of how to make this decision.

I think the thesis of this PR is that this doesn't provide any new functionality, but provides a java wrapper around metadata which is more convenient, right? Is that expected for Android developers, does that scale across all uses of the metadata? I'm ill equipped to make that decision.

Nayuta403 · 2022-11-18T03:39:39Z

@gaaclarke Hi, thanks for reply. I would like to provide some context about this PR. If anything is wrong, please let me know

EngineGroup has provided api spawn allowing us to use a lightweight engine. But currently, the default creation in FlutterActivity is heavyweight engines. So developer have to manage the EngineGroup, extends FlutterActivity and overwrite provideEngine functions to use of lightweight engine. This PR provides a way for users to directly create FlutterActivity using a lightweight engine without having to worry about managing the engine. Like you said this provide an easier way for developers to use it, but I think developers don't have to care about this, what do you think?

but provides a java wrapper around metadata which is more convenient, right? Is that expected for Android developers, does that scale across all uses of the metadata? I'm ill equipped to make that decision.

IIUC, Referring to EXTRA_DART_ENTRYPOINT modification? Because we may start multiple engines, they may have different entrypoints, so we can't determine which entrypoint each engine should go to during the coding phase, So using MetaData in this case doesn't seem to make sense.

gaaclarke

Thanks, that helps. I have an outstanding PR that starts making activities and fragments use an engine group by default too. So there is probably going to be an engine group that is available through that. Since we are going to start using engine groups by default, accepting the maintenance of code that makes their usage easier makes sense to me.

gaaclarke · 2022-11-30T17:25:01Z

@GaryQian the code looks good here and the tests. The justification makes sense to me. I want to make sure that it matches Android expectations, since you'd have a better idea.

Nayuta403 · 2022-12-04T12:29:28Z

@xster is an expert on the subject and please help review the code :-D

skia-gold · 2022-12-06T16:15:29Z

Gold has detected about 72 new digest(s) on patchset 16.
View them at https://flutter-engine-gold.skia.org/cl/github/36963

ColdPaleLight

LGTM

…ine with FlutterEngineGroup (flutter/engine#36963)

* bd8bcf9 Roll Fuchsia Mac SDK from crEcyXdyZ686cAqMV... to pMV6A0ykZQ8aA3NG2... (flutter/engine#38120) * dec8b52 Preliminary implementation of UIA for A11y on Windows (flutter/engine#37754) * 5545ccf Roll Fuchsia Linux SDK from NlJGkMbtZqQ6_BCpu... to xn8ztWtp-zww-jObz... (flutter/engine#38122) * 80a15a4 Create FlutterActivity/FlutterFragment using light weight engine with FlutterEngineGroup (flutter/engine#36963) * 5caef85 Full implementation of text-input-test (flutter/engine#37986) * 8f6036e Reland fix wrong VSYNC event (flutter/engine#37865) * 4101c36 [iOS] Change locale format for spell check (flutter/engine#38080) * 2f5b67e [embedder] Ensure destruction called on present (flutter/engine#38078) * 0bddc60 [Impeller Scene] Depth attachment; baked lighting example (flutter/engine#38118) * 6aa4ccd Remove dlCanvasRecorder from flutter::PictureRecorder (flutter/engine#38127) * dbb5284 [Windows] Add more cursor plugin tests (flutter/engine#38112) * 6e91204 Roll Fuchsia Mac SDK from pMV6A0ykZQ8aA3NG2... to 9SnrQ0vbR8IC7UIoP... (flutter/engine#38135) * 3140ad9 [Impeller] order metal samplers according to declared order and not usage order (flutter/engine#38115) * 84abf21 Remove autoninja. (flutter/engine#38136) * 8a113d3 [embedder] Expose metal surface from test context (flutter/engine#38133) * 1ef25b6 Roll Fuchsia Mac SDK from 9SnrQ0vbR8IC7UIoP... to aMW0DjntzFJj4RoR3... (flutter/engine#38139) * 748b3bc Revert "Remove dlCanvasRecorder from flutter::PictureRecorder (#38127)" (flutter/engine#38137) * b6daf3d [embedder] Consistent naming for GL/Metal tests (flutter/engine#38141) * 339d04b [web] Trivial fix for non-static interop JS interop class. (flutter/engine#38126) * 1fcbb9c [tools] Eliminate version on Obj-C docs (flutter/engine#38145) * 71928b6 [Impeller] Use DrawPath instead of Rect geometry when the paint style is stroke (flutter/engine#38146) * 23ce8fd Roll Skia from dd3285a80b23 to f84dc9303045 (4 revisions) (flutter/engine#38123) * 366f866 Roll Skia from f84dc9303045 to 2691cd7b4110 (40 revisions) (flutter/engine#38151) * 447e701 Roll Skia from 2691cd7b4110 to 711396b81248 (1 revision) (flutter/engine#38152) * cd5d91b Pylint testing/run_tests.py (flutter/engine#38016) * aafac08 Roll Skia from 711396b81248 to b253b10374e7 (7 revisions) (flutter/engine#38157) * 799dc78 Roll Fuchsia Linux SDK from xn8ztWtp-zww-jObz... to rRJIjuO-dPNCpCTd9... (flutter/engine#38134) * 3aa3d2a Massage the JS interop around `didCreateEngineInitializer` (flutter/engine#38147) * 030950f Roll Skia from b253b10374e7 to ec407902999b (3 revisions) (flutter/engine#38158)

…16802) * bd8bcf9 Roll Fuchsia Mac SDK from crEcyXdyZ686cAqMV... to pMV6A0ykZQ8aA3NG2... (flutter/engine#38120) * dec8b52 Preliminary implementation of UIA for A11y on Windows (flutter/engine#37754) * 5545ccf Roll Fuchsia Linux SDK from NlJGkMbtZqQ6_BCpu... to xn8ztWtp-zww-jObz... (flutter/engine#38122) * 80a15a4 Create FlutterActivity/FlutterFragment using light weight engine with FlutterEngineGroup (flutter/engine#36963) * 5caef85 Full implementation of text-input-test (flutter/engine#37986) * 8f6036e Reland fix wrong VSYNC event (flutter/engine#37865) * 4101c36 [iOS] Change locale format for spell check (flutter/engine#38080) * 2f5b67e [embedder] Ensure destruction called on present (flutter/engine#38078) * 0bddc60 [Impeller Scene] Depth attachment; baked lighting example (flutter/engine#38118) * 6aa4ccd Remove dlCanvasRecorder from flutter::PictureRecorder (flutter/engine#38127) * dbb5284 [Windows] Add more cursor plugin tests (flutter/engine#38112) * 6e91204 Roll Fuchsia Mac SDK from pMV6A0ykZQ8aA3NG2... to 9SnrQ0vbR8IC7UIoP... (flutter/engine#38135) * 3140ad9 [Impeller] order metal samplers according to declared order and not usage order (flutter/engine#38115) * 84abf21 Remove autoninja. (flutter/engine#38136) * 8a113d3 [embedder] Expose metal surface from test context (flutter/engine#38133) * 1ef25b6 Roll Fuchsia Mac SDK from 9SnrQ0vbR8IC7UIoP... to aMW0DjntzFJj4RoR3... (flutter/engine#38139) * 748b3bc Revert "Remove dlCanvasRecorder from flutter::PictureRecorder (flutter#38127)" (flutter/engine#38137) * b6daf3d [embedder] Consistent naming for GL/Metal tests (flutter/engine#38141) * 339d04b [web] Trivial fix for non-static interop JS interop class. (flutter/engine#38126) * 1fcbb9c [tools] Eliminate version on Obj-C docs (flutter/engine#38145) * 71928b6 [Impeller] Use DrawPath instead of Rect geometry when the paint style is stroke (flutter/engine#38146) * 23ce8fd Roll Skia from dd3285a80b23 to f84dc9303045 (4 revisions) (flutter/engine#38123) * 366f866 Roll Skia from f84dc9303045 to 2691cd7b4110 (40 revisions) (flutter/engine#38151) * 447e701 Roll Skia from 2691cd7b4110 to 711396b81248 (1 revision) (flutter/engine#38152) * cd5d91b Pylint testing/run_tests.py (flutter/engine#38016) * aafac08 Roll Skia from 711396b81248 to b253b10374e7 (7 revisions) (flutter/engine#38157) * 799dc78 Roll Fuchsia Linux SDK from xn8ztWtp-zww-jObz... to rRJIjuO-dPNCpCTd9... (flutter/engine#38134) * 3aa3d2a Massage the JS interop around `didCreateEngineInitializer` (flutter/engine#38147) * 030950f Roll Skia from b253b10374e7 to ec407902999b (3 revisions) (flutter/engine#38158)

Nayuta403 added 3 commits October 24, 2022 19:50

init commit

f549d97

test fix

80d43f4

doc fix

3dd4086

flutter-dashboard bot added the platform-android label Oct 24, 2022

Nayuta403 changed the title ~~Engine group~~ Create FlutterActivity/FlutterFragment using light weight engine with FlutterEngineGroup Oct 24, 2022

Nayuta403 added 2 commits October 24, 2022 22:01

Merge branch 'flutter:main' into engine_group

453e521

Update DEPS

e53b6f1

Merge branch 'flutter:main' into engine_group

241e359

GaryQian requested review from gaaclarke and xster November 1, 2022 17:17

gaaclarke reviewed Nov 1, 2022

View reviewed changes

Nayuta403 and others added 5 commits November 2, 2022 15:33

Apply suggestions from code review

89a738a

Co-authored-by: gaaclarke <[email protected]>

synchronized

dd5b9cf

add volatile

309b136

format

b27c3bf

Merge branch 'flutter:main' into engine_group

7ccb4ad

Nayuta403 mentioned this pull request Nov 3, 2022

Abstract Builder class to share some parameters flutter/flutter#114567

Open

Merge branch 'flutter:main' into engine_group

acd8b63

GaryQian requested a review from gaaclarke November 10, 2022 19:49

Merge branch 'flutter:main' into engine_group

7ea6a00

Merge branch 'flutter:main' into engine_group

0052962

gaaclarke reviewed Nov 17, 2022

View reviewed changes

Merge branch 'flutter:main' into engine_group

d57c066

gaaclarke approved these changes Nov 30, 2022

View reviewed changes

gaaclarke requested a review from GaryQian November 30, 2022 17:24

Merge branch 'flutter:main' into engine_group

e61d1bf

ColdPaleLight approved these changes Dec 7, 2022

View reviewed changes

gaaclarke merged commit 80a15a4 into flutter:main Dec 7, 2022

engine-flutter-autoroll mentioned this pull request Dec 9, 2022

Roll Flutter Engine from 8d83b98c55b3 to 030950f3070c (29 revisions) flutter/flutter#116802

Merged

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

80a15a4 Create FlutterActivity/FlutterFragment using light weight eng…

3f725d9

…ine with FlutterEngineGroup (flutter/engine#36963)

jmagman mentioned this pull request Aug 27, 2024

FlutterEngineGroupCache on iOS flutter/flutter#154095

Open

Create FlutterActivity/FlutterFragment using light weight engine with FlutterEngineGroup #36963

Create FlutterActivity/FlutterFragment using light weight engine with FlutterEngineGroup #36963

Uh oh!

Conversation

Nayuta403 commented Oct 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

GaryQian commented Oct 25, 2022

Uh oh!

Nayuta403 commented Oct 26, 2022

Uh oh!

chinmaygarde commented Oct 27, 2022

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

skia-gold commented Nov 11, 2022

Uh oh!

chinmaygarde commented Nov 17, 2022

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Nayuta403 commented Nov 18, 2022

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

gaaclarke commented Nov 30, 2022

Uh oh!

Nayuta403 commented Dec 4, 2022

Uh oh!

skia-gold commented Dec 6, 2022

Uh oh!

ColdPaleLight left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Nayuta403 commented Oct 24, 2022 •

edited

Loading