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

Android Embedding PR 11: Add FlutterEngine to FlutterFragment.#7972

Merged
matthew-carroll merged 1 commit intoflutter:masterfrom
matthew-carroll:android_embedding_refactor_pr11_add_engine_to_flutterfragment
Feb 28, 2019
Merged

Android Embedding PR 11: Add FlutterEngine to FlutterFragment.#7972
matthew-carroll merged 1 commit intoflutter:masterfrom
matthew-carroll:android_embedding_refactor_pr11_add_engine_to_flutterfragment

Conversation

@matthew-carroll
Copy link
Contributor

No description provided.

* By default, this method returns a standard {@link FlutterEngine} without any modification.
*/
@NonNull
protected FlutterEngine onCreateFlutterEngine(@NonNull Context context) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The naming of this seems awkward to me. I think this should just be called createFlutterEngine. Having a non-void method called onCreate seems unusual.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is really a nit though, feel free to disregard if it fits some other pattern that I'm just not aware of.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The convention I was trying to follow here is similar to the onCreateView() method in Fragments. There's a primary method invoked in the superclass, in this case called createFlutterEngine(), which then internally invokes a subclass hook, in this case called onCreateFlutterEngine(). Does that make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough

Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

LGTM with nit

@matthew-carroll matthew-carroll merged commit 2360b45 into flutter:master Feb 28, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 28, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 28, 2019
engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Feb 28, 2019
flutter/engine@9b92476...5194919

git log 9b92476..5194919 --no-merges --oneline
5194919 Roll src/third_party/skia 1179d5dfaf8d..b7b2da871e95 (1 commits) (flutter/engine#7996)
58ce0ff Roll src/third_party/skia 90043480b587..1179d5dfaf8d (3 commits) (flutter/engine#7995)
72cbe69 Fix two typos in embedder.h (flutter/engine#7993)
2360b45 Android Embedding PR 11: Add FlutterEngine to FlutterFragment. (flutter/engine#7972)
df03ec7 Roll src/third_party/skia 0cedddc1420b..90043480b587 (1 commits) (flutter/engine#7994)
4a148cd Roll src/third_party/skia 67d87128fd00..0cedddc1420b (10 commits) (flutter/engine#7992)

The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff ([email protected]), and stop
the roller if necessary.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants