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

Move initialization into FlutterEngine #12806

Merged
matthew-carroll merged 3 commits into from
Oct 11, 2019
Merged

Move initialization into FlutterEngine #12806

matthew-carroll merged 3 commits into from
Oct 11, 2019

Conversation

@matthew-carroll
Copy link
Contributor

@matthew-carroll matthew-carroll commented Oct 4, 2019

Move initialization into FlutterEngine.

This PR is based on @xster's PR here:
#12069

I made 2 changes.

I removed FlutterInjector. We want to ensure that we always support constructor injection, even if we introduce some kind of injection tooling/framework. Since this particular change was not intended to establish an injection system, once I implemented constructor injection for FlutterEngine with a FlutterLoader, the FlutterInjector class became superfluous.

I made all methods in FlutterLoader instance methods instead of static methods. This looks like it was probably just an oversight in the original work.

With this PR, if a real instance of FlutterEngine needs to be instantiated in a test, the test should pass in a mock FlutterLoader and the test will successfully bypass the native library linking.

@matthew-carroll matthew-carroll requested review from mklim and xster October 4, 2019 20:55
@googlebot
Copy link

googlebot commented Oct 4, 2019

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

1 similar comment
@googlebot
Copy link

googlebot commented Oct 4, 2019

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@matthew-carroll matthew-carroll mentioned this pull request Oct 4, 2019
Closed
mklim
mklim approved these changes Oct 4, 2019
View reviewed changes
Copy link
Contributor

@mklim mklim left a comment

Choose a reason for hiding this comment

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

Overall LGTM. I have one question on run_android_tests.sh.

testing/scenario_app/run_android_tests.sh
@@ -0,0 +1,24 @@
#!/bin/sh
Copy link
Contributor

@mklim mklim Oct 4, 2019

Choose a reason for hiding this comment

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

I'd prefer that we put all the test harnesses into run_tests.py instead of having extra bash scripts lying around. Is there a reason to keep this separate, or is it clear to move there?

Copy link
Contributor Author

@matthew-carroll matthew-carroll Oct 4, 2019

Choose a reason for hiding this comment

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

Is this the one question that you mentioned in your LGTM?

Copy link
Contributor

@mklim mklim Oct 4, 2019

Choose a reason for hiding this comment

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

Yeah.

Copy link
Contributor Author

@matthew-carroll matthew-carroll Oct 4, 2019

Choose a reason for hiding this comment

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

Ok. I actually don't know why this is here, or what it's accomplishing. If you can guide me to moving some/all of this to another place, I can do that.

Copy link
Contributor

@mklim mklim Oct 4, 2019

Choose a reason for hiding this comment

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

It looks like it builds libapp.so with compile_android_aot.sh, and then runs ./gradlew assembleAndroidTest && ./gradlew connectedAndroidTest.

I think you could just basically convert the bash commands into subprocess.check_call ones in run_tests.py, with a new AndroidIntegration test type to run these under.

The reason I'm suggesting putting it together is just so that all the tests are in one place, but maybe there's some semantic reason to keep the android e2e tests separate? /cc @dnfield and @xster.

Copy link
Contributor Author

@matthew-carroll matthew-carroll Oct 4, 2019

Choose a reason for hiding this comment

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

Let's assume I'm an idiot when it comes to bash scripts and interfacing with Python (not really an assumption). @mklim can you outline the approach? There are a lot of Python methods in run_tests.py that my IDE is saying have no usages. Is there some kind of convention name matching going on?

Copy link
Contributor Author

@matthew-carroll matthew-carroll Oct 4, 2019

Choose a reason for hiding this comment

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

BTW, would there be any input configurations? For example, java tests let you choose the test, but does it make sense to have any control over which tests runs in the integration case?

Copy link
Contributor

@mklim mklim Oct 4, 2019

Choose a reason for hiding this comment

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

Hmm. I've never tried to edit python in a full IDE before, not sure how it would tell what isn't used. I'm not exactly a python expert either.

I think if the ideal is that we want to delete this eventually, we should just leave it here for now instead of moving it. There's not really a lot of point of refactoring it into python if we don't want it around for long. I can sync up with you on how to translate this if you'd like Matt, but with Dan's input I think it's better to just leave it alone after all.

Copy link
Contributor Author

@matthew-carroll matthew-carroll Oct 4, 2019

Choose a reason for hiding this comment

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

Ok. I'll leave it as-is. Any other comments on the PR at this point?

Copy link
Contributor

@mklim mklim Oct 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

No, everything LGTM. :)

@matthew-carroll
Copy link
Contributor Author

matthew-carroll commented Oct 4, 2019

I'm going to try to rebase this to collapse @xster's commits into a single commit of my own to get rid of the CLA problem.

@matthew-carroll matthew-carroll force-pushed the move_initialization_into_flutterengine branch from a719b99 to 3b35394 Compare October 4, 2019 23:31
@googlebot
Copy link

googlebot commented Oct 4, 2019

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

1 similar comment
@googlebot
Copy link

googlebot commented Oct 4, 2019

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Oct 4, 2019
@matthew-carroll
Copy link
Contributor Author

matthew-carroll commented Oct 7, 2019

Waiting on luci-engine to go green to land this.

@matthew-carroll matthew-carroll mentioned this pull request Oct 11, 2019
Merged
@redbrogdon
Copy link

redbrogdon commented Oct 11, 2019

CC @redbrogdon

@matthew-carroll matthew-carroll force-pushed the move_initialization_into_flutterengine branch from 3b35394 to 7c2fec1 Compare October 11, 2019 22:50
@matthew-carroll matthew-carroll merged commit 9acec41 into flutter:master Oct 11, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 14, 2019
@engine-flutter-autoroll
9acec41 Move initialization into FlutterEngine (flutter/engine#12806)
4e8bcd3
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 14, 2019
Merged
engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Oct 15, 2019
@engine-flutter-autoroll
Roll engine eed171f..66bf00b (44 commits) (#42695)
1d4d63a 
git@github.com:flutter/engine.git/compare/eed171ff3538...66bf00b

git log eed171f..66bf00b --no-merges --oneline
2019-10-14 50856934+nturgut@users.noreply.github.com refactoring chrome_installer (flutter/engine#13122)
2019-10-14 mklim@google.com Fire PlatformViewController FlutterView callbacks (flutter/engine#13015)
2019-10-14 30870216+gaaclarke@users.noreply.github.com iOS Platform View: Fixed overrelease of the observer. (flutter/engine#13093)
2019-10-14 ferhat@gmail.com [web] Add basic color per vertex drawVertices API support (flutter/engine#13066)
2019-10-14 mouad.debbar@gmail.com Support keyboard types on mobile browsers (flutter/engine#13044)
2019-10-14 liyuqian@google.com Change IO thread shader cache strategy (flutter/engine#13121)
2019-10-14 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from _GTls... to xRgq0... (flutter/engine#13115)
2019-10-14 skia-flutter-autoroll@skia.org Roll src/third_party/skia 3838fe3c82b4..a7e1b45d9c28 (1 commits) (flutter/engine#13117)
2019-10-14 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from E3s7G... to Lk7iT... (flutter/engine#13116)
2019-10-14 bkonyi@google.com Roll src/third_party/dart 5fad012d02..892fcf2c45 (15 commits)
2019-10-14 jason-simmons@users.noreply.github.com Integrate more SkParagraph builder patches (flutter/engine#13094)
2019-10-13 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from T3Xkz... to E3s7G... (flutter/engine#13114)
2019-10-12 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from lJPDX... to _GTls... (flutter/engine#13113)
2019-10-12 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from W9PBe... to T3Xkz... (flutter/engine#13112)
2019-10-12 skia-flutter-autoroll@skia.org Roll fuchsia/clang/linux-amd64 from q4DVY... to 2EAvs... (flutter/engine#13111)
2019-10-12 skia-flutter-autoroll@skia.org Roll fuchsia/clang/mac-amd64 from zpVtV... to xTJWs... (flutter/engine#13110)
2019-10-12 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from LsxeL... to lJPDX... (flutter/engine#13109)
2019-10-12 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from ShVbh... to W9PBe... (flutter/engine#13108)
2019-10-12 bkonyi@google.com Roll src/third_party/dart 90ff37e011..5fad012d02 (6 commits)
2019-10-12 bkonyi@google.com Roll src/third_party/dart 42dcdf903c..90ff37e011 (7 commits)
2019-10-12 skia-flutter-autoroll@skia.org Roll src/third_party/skia 8c6c8b0c42e2..3838fe3c82b4 (3 commits) (flutter/engine#13105)
2019-10-12 liyuqian@google.com Analyze framework Dart code in presubmit tests (flutter/engine#13037)
2019-10-12 iska.kaushik@gmail.com [dart_aot_runner] Complete the port of dart_aot_runner (flutter/engine#13103)
2019-10-11 matthew-carroll@users.noreply.github.com Move initialization into FlutterEngine (flutter/engine#12806)
2019-10-11 ferhat@gmail.com Update felt README (flutter/engine#13097)
2019-10-11 bkonyi@google.com Roll src/third_party/dart 9f33e8da04..42dcdf903c (7 commits)
2019-10-11 iska.kaushik@gmail.com [dart_aot_runner] Generate vmservice aotsnapshots (flutter/engine#13101)
2019-10-11 dnfield@google.com ColorFilter matrix docs (flutter/engine#13100)
2019-10-11 dnfield@google.com cleanup gen_package.py (flutter/engine#13089)
2019-10-11 iska.kaushik@gmail.com [dart_aot_runner] Use the host_toolchain to build kernels (flutter/engine#13096)
2019-10-11 50856934+nturgut@users.noreply.github.com do not wrap font family name (flutter/engine#12801)
2019-10-11 skia-flutter-autoroll@skia.org Roll src/third_party/skia df640e6d14a5..8c6c8b0c42e2 (12 commits) (flutter/engine#13098)
2019-10-11 liyuqian@google.com Remove persistent cache unittest timeout (flutter/engine#13091)
2019-10-11 matthew-carroll@users.noreply.github.com Added FlutterActivity and FlutterFragment hook to cleanUpFlutterEngine() as symmetry for configureFlutterEngine(). (#41943) (flutter/engine#12987)
2019-10-11 yjbanov@google.com use rest args for specifying test targets (flutter/engine#13088)
2019-10-11 yjbanov@google.com Snapshot the felt tool for faster start-up (flutter/engine#13090)
2019-10-11 bkonyi@google.com Roll src/third_party/dart 965b8cb1d8..9f33e8da04 (15 commits)
2019-10-11 dnfield@google.com java imports/style (flutter/engine#13082)
2019-10-11 30870216+gaaclarke@users.noreply.github.com Removed retain cycle from notification center. (flutter/engine#13073)
2019-10-11 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from U4IBp... to ShVbh... (flutter/engine#13092)
2019-10-11 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from fk6ou... to LsxeL... (flutter/engine#13083)
2019-10-11 skia-flutter-autoroll@skia.org Roll src/third_party/skia 6c2b2bb02402..df640e6d14a5 (1 commits) (flutter/engine#13080)
2019-10-11 dnfield@google.com Gen package output corrected (flutter/engine#13086)
2019-10-11 dnfield@google.com Print more output when gen_package fails (flutter/engine#13085)

...
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Nov 26, 2019
@engine-flutter-autoroll @Inconnu08
Roll engine eed171f..66bf00b (44 commits) (flutter#42695)
f7381cc 
git@github.com:flutter/engine.git/compare/eed171ff3538...66bf00b

git log eed171f..66bf00b --no-merges --oneline
2019-10-14 50856934+nturgut@users.noreply.github.com refactoring chrome_installer (flutter/engine#13122)
2019-10-14 mklim@google.com Fire PlatformViewController FlutterView callbacks (flutter/engine#13015)
2019-10-14 30870216+gaaclarke@users.noreply.github.com iOS Platform View: Fixed overrelease of the observer. (flutter/engine#13093)
2019-10-14 ferhat@gmail.com [web] Add basic color per vertex drawVertices API support (flutter/engine#13066)
2019-10-14 mouad.debbar@gmail.com Support keyboard types on mobile browsers (flutter/engine#13044)
2019-10-14 liyuqian@google.com Change IO thread shader cache strategy (flutter/engine#13121)
2019-10-14 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from _GTls... to xRgq0... (flutter/engine#13115)
2019-10-14 skia-flutter-autoroll@skia.org Roll src/third_party/skia 3838fe3c82b4..a7e1b45d9c28 (1 commits) (flutter/engine#13117)
2019-10-14 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from E3s7G... to Lk7iT... (flutter/engine#13116)
2019-10-14 bkonyi@google.com Roll src/third_party/dart 5fad012d02..892fcf2c45 (15 commits)
2019-10-14 jason-simmons@users.noreply.github.com Integrate more SkParagraph builder patches (flutter/engine#13094)
2019-10-13 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from T3Xkz... to E3s7G... (flutter/engine#13114)
2019-10-12 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from lJPDX... to _GTls... (flutter/engine#13113)
2019-10-12 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from W9PBe... to T3Xkz... (flutter/engine#13112)
2019-10-12 skia-flutter-autoroll@skia.org Roll fuchsia/clang/linux-amd64 from q4DVY... to 2EAvs... (flutter/engine#13111)
2019-10-12 skia-flutter-autoroll@skia.org Roll fuchsia/clang/mac-amd64 from zpVtV... to xTJWs... (flutter/engine#13110)
2019-10-12 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from LsxeL... to lJPDX... (flutter/engine#13109)
2019-10-12 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from ShVbh... to W9PBe... (flutter/engine#13108)
2019-10-12 bkonyi@google.com Roll src/third_party/dart 90ff37e011..5fad012d02 (6 commits)
2019-10-12 bkonyi@google.com Roll src/third_party/dart 42dcdf903c..90ff37e011 (7 commits)
2019-10-12 skia-flutter-autoroll@skia.org Roll src/third_party/skia 8c6c8b0c42e2..3838fe3c82b4 (3 commits) (flutter/engine#13105)
2019-10-12 liyuqian@google.com Analyze framework Dart code in presubmit tests (flutter/engine#13037)
2019-10-12 iska.kaushik@gmail.com [dart_aot_runner] Complete the port of dart_aot_runner (flutter/engine#13103)
2019-10-11 matthew-carroll@users.noreply.github.com Move initialization into FlutterEngine (flutter/engine#12806)
2019-10-11 ferhat@gmail.com Update felt README (flutter/engine#13097)
2019-10-11 bkonyi@google.com Roll src/third_party/dart 9f33e8da04..42dcdf903c (7 commits)
2019-10-11 iska.kaushik@gmail.com [dart_aot_runner] Generate vmservice aotsnapshots (flutter/engine#13101)
2019-10-11 dnfield@google.com ColorFilter matrix docs (flutter/engine#13100)
2019-10-11 dnfield@google.com cleanup gen_package.py (flutter/engine#13089)
2019-10-11 iska.kaushik@gmail.com [dart_aot_runner] Use the host_toolchain to build kernels (flutter/engine#13096)
2019-10-11 50856934+nturgut@users.noreply.github.com do not wrap font family name (flutter/engine#12801)
2019-10-11 skia-flutter-autoroll@skia.org Roll src/third_party/skia df640e6d14a5..8c6c8b0c42e2 (12 commits) (flutter/engine#13098)
2019-10-11 liyuqian@google.com Remove persistent cache unittest timeout (flutter/engine#13091)
2019-10-11 matthew-carroll@users.noreply.github.com Added FlutterActivity and FlutterFragment hook to cleanUpFlutterEngine() as symmetry for configureFlutterEngine(). (flutter#41943) (flutter/engine#12987)
2019-10-11 yjbanov@google.com use rest args for specifying test targets (flutter/engine#13088)
2019-10-11 yjbanov@google.com Snapshot the felt tool for faster start-up (flutter/engine#13090)
2019-10-11 bkonyi@google.com Roll src/third_party/dart 965b8cb1d8..9f33e8da04 (15 commits)
2019-10-11 dnfield@google.com java imports/style (flutter/engine#13082)
2019-10-11 30870216+gaaclarke@users.noreply.github.com Removed retain cycle from notification center. (flutter/engine#13073)
2019-10-11 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from U4IBp... to ShVbh... (flutter/engine#13092)
2019-10-11 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from fk6ou... to LsxeL... (flutter/engine#13083)
2019-10-11 skia-flutter-autoroll@skia.org Roll src/third_party/skia 6c2b2bb02402..df640e6d14a5 (1 commits) (flutter/engine#13080)
2019-10-11 dnfield@google.com Gen package output corrected (flutter/engine#13086)
2019-10-11 dnfield@google.com Print more output when gen_package fails (flutter/engine#13085)

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

Reviewers

@xster xster Awaiting requested review from xster

2 more reviewers

@mklim mklim mklim approved these changes

@dnfield dnfield dnfield left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@matthew-carroll @googlebot @redbrogdon @mklim @dnfield