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

Make FlutterEngineGroup support more params (#107394) #34663

Merged
auto-submit merged 15 commits into from
Aug 4, 2022
Merged

Make FlutterEngineGroup support more params (#107394) #34663

auto-submit merged 15 commits into from
Aug 4, 2022

Conversation

@Nayuta403
Copy link
Contributor

@Nayuta403 Nayuta403 commented Jul 14, 2022 •
edited
Loading

flutter/flutter#107394

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.

@Nayuta403 Nayuta403 changed the title Make FlutterEngineGroup support more params (#107394) [WIP]Make FlutterEngineGroup support more params (#107394) Jul 14, 2022
@Nayuta403 Nayuta403 changed the title [WIP]Make FlutterEngineGroup support more params (#107394) Make FlutterEngineGroup support more params (#107394) Jul 17, 2022
@Nayuta403
Copy link
Contributor Author

Nayuta403 commented Jul 17, 2022

cc ❤️ @dnfield @ColdPaleLight @chinmaygarde

@ColdPaleLight
Copy link
Member

ColdPaleLight commented Jul 17, 2022 •
edited
Loading

It looks reasonable to me. Can we add a new test to verify that custom new params are passed to FlutterEngine?

@Nayuta403
Copy link
Contributor Author

Nayuta403 commented Jul 18, 2022

Here's what I think.
platformViewsController is a controller, and FlutterEngine#getPlatformViewsController is used to determine success.
automaticallyRegisterPlugins this affects whether plugin registration is performed in the constructor of FluttterEngine.
waitForRestorationData is used fo RestorationChannel.
I'm not sure how to test automaticallyRegisterPlugins and waitForRestorationData, any suggestions?

@Nayuta403
Copy link
Contributor Author

Nayuta403 commented Jul 18, 2022

I'll fill in the test cases later

@chinmaygarde
Copy link
Member

chinmaygarde commented Jul 21, 2022

cc @gaaclarke

@Nayuta403
Copy link
Contributor Author

Nayuta403 commented Jul 22, 2022

After offline discussions with @ColdPaleLight , I should add test cases for createEngine, which I will fill in later

@Nayuta403 Nayuta403 requested a review from ColdPaleLight July 25, 2022 02:47
@ColdPaleLight ColdPaleLight requested review from gaaclarke and removed request for GaryQian and dnfield July 25, 2022 03:54
@ColdPaleLight ColdPaleLight removed the request for review from jason-simmons July 25, 2022 03:54
@Nayuta403 Nayuta403 mentioned this pull request Jul 26, 2022
Closed
ColdPaleLight
ColdPaleLight reviewed Jul 28, 2022
View reviewed changes
Copy link
Member

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

I've done the first round of reviews on this PR.

@gaaclarke Would you mind taking a look when you have a change? :)

gaaclarke
gaaclarke approved these changes Aug 3, 2022
View reviewed changes
Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

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

LGTM, sorry I was out on vacation and missed this.

@gaaclarke gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 3, 2022
@auto-submit
Copy link
Contributor

auto-submit bot commented Aug 3, 2022

  • Please get at least one approved review if you are already a member or two member reviews if you are not a member before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

@auto-submit
Copy link
Contributor

auto-submit bot commented Aug 3, 2022

Validations Fail.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 3, 2022
ColdPaleLight
ColdPaleLight approved these changes Aug 4, 2022
View reviewed changes
Copy link
Member

@ColdPaleLight ColdPaleLight left a comment

Choose a reason for hiding this comment

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

LGTM

@ColdPaleLight ColdPaleLight added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 4, 2022
@auto-submit auto-submit bot merged commit 7b6911a into flutter:main Aug 4, 2022
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 4, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 4, 2022
@engine-flutter-autoroll
7b6911a Make FlutterEngineGroup support more params (flutter#107394) (f…
b9b0799 
…lutter/engine#34663)
@ColdPaleLight ColdPaleLight mentioned this pull request Aug 4, 2022
Closed
betrevisan pushed a commit to betrevisan/engine that referenced this pull request Aug 5, 2022
@Nayuta403 @betrevisan
Make FlutterEngineGroup support more params (#107394) (flutter#34663)
e5f8168
@Nayuta403 Nayuta403 deleted the support_params branch August 11, 2022 10:41
emilyabest pushed a commit to emilyabest/engine that referenced this pull request Aug 12, 2022
@Nayuta403 @emilyabest
Make FlutterEngineGroup support more params (#107394) (flutter#34663)
84c695e
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@gaaclarke gaaclarke gaaclarke approved these changes

@ColdPaleLight ColdPaleLight ColdPaleLight 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-android

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Nayuta403 @ColdPaleLight @chinmaygarde @gaaclarke @linxuebin1990