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

Conversation

@GaryQian
Copy link
Contributor

@GaryQian GaryQian commented Dec 2, 2020

Adds a DynamicFeatureChannel for manual invocation and tracking of the deferred library/asset split process.

@google-cla google-cla bot added the cla: yes label Dec 2, 2020
@GaryQian GaryQian changed the title [WIP] Split AOT MethodChannel [WIP] Split AOT MethodChannel and Install state tracking Dec 3, 2020
import org.json.JSONException;
import org.json.JSONObject;

public class SplitAotChannel {
Copy link
Member

Choose a reason for hiding this comment

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

Just coming in early to comment on the name. Can we s/SplitAot/DeferredFeature or DeferredModule here and elsewhere in docs and instructions etc? Since "split AOT" is a strict subset of what we're really shipping.

FILE: ../../../flutter/shell/platform/android/io/flutter/embedding/engine/systemchannels/PlatformViewsChannel.java
FILE: ../../../flutter/shell/platform/android/io/flutter/embedding/engine/systemchannels/RestorationChannel.java
FILE: ../../../flutter/shell/platform/android/io/flutter/embedding/engine/systemchannels/SettingsChannel.java
FILE: ../../../flutter/shell/platform/android/io/flutter/embedding/engine/systemchannels/SplitAotChannel.java
Copy link
Member

Choose a reason for hiding this comment

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

do you need to change this too?

@GaryQian
Copy link
Contributor Author

GaryQian commented Dec 8, 2020

Framework paired PR: flutter/flutter#71879

@GaryQian GaryQian marked this pull request as ready for review December 11, 2020 05:03
@GaryQian GaryQian changed the title [WIP] Split AOT MethodChannel and Install state tracking DynamicFeatureChannel MethodChannel and Install state tracking Dec 11, 2020
textInputChannel = new TextInputChannel(dartExecutor);

if (dynamicFeatureManager != null) {
dynamicFeatureManager.setDynamicFeatureChannel(dynamicFeatureChannel);
Copy link
Member

Choose a reason for hiding this comment

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

if you create circular connections, best to break them yourself. Especially when there are singletons like JNI involved.

* Gets the current state of the installation session corresponding to the specified loadingUnitId
* and/or moduleName.
*
* <p>Invocations of {@link installDynamicFeature} typically result in asynchronous downloading
Copy link
Member

Choose a reason for hiding this comment

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

though you can check the completion of that call by just waiting for that future itself no? Explain why they need this

Copy link
Contributor Author

@GaryQian GaryQian Dec 11, 2020

Choose a reason for hiding this comment

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

This is for displaying things like loading bars, status messages, etc. The status here is purely informational and is not necessary for function. I'll add that to the docs.

Copy link
Member

Choose a reason for hiding this comment

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

I thought you added somewhere the redundancy with loadLibrary. Best be explicit about the web that exists everywhere. i.e. if you installDynamicFeature, you don't really have to check this or listen or the channel, if you listen on the channel, you don't really have to wait for the loadLibrary future, if you loadLibrary, it's the same as installDynamicFeature with loading unit etc, and fully create the web and cross reference everything to everything.

i.e. we don't want users guessing there's 3-4 APIs that can more or less do the same thing. What am I missing out if I just use 1 and don't use all of them.

sessionIdToLoadingUnitId.get(sessionId), sessionIdToName.get(sessionId));
}
if (channel != null) {
channel.completeInstallSuccess(sessionIdToName.get(sessionId));
Copy link
Member

Choose a reason for hiding this comment

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

to make sure I understand, this is purely optional right? i.e. if a user calls loadLibrary in Dart (which ultimately leads here), they can just await that call instead of listening to this right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the methodChannel lives in parallel with the loadLibrary() code path. Since assets only modules do not have any library to load off of, this manual methodChannel way mirrors the functionality but allows specifying the moduleName directly. The channel accepts a loadingUnitId and a moduleName, but in the dart-side wrapper API I plan on exposing will restrict this to just taking a moduleName for now. The full loadingUnitId+moduleName API can be exposed in the future if/when dart allows initiating a loading unit load directly by loading unit id without a matching dart library loadLibrary() call.

Copy link
Member

@xster xster left a comment

Choose a reason for hiding this comment

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

LGTM

flutterJNI.removeEngineLifecycleListener(engineLifecycleListener);
flutterJNI.setDynamicFeatureManager(null);
flutterJNI.detachFromNativeAndReleaseResources();
dynamicFeatureChannel.destroy();
Copy link
Member

Choose a reason for hiding this comment

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

maybe just do if feature manager is not null, set channel to null in the if below? So we don't have to create a new channel destroy pattern.

* installed dynamic features.
*
* <p>Since this class may be instantiated for injection before the FlutterEngine and System
* Channels are initialized, this method should be called to provide the DynamicFeatureChannel.
Copy link
Member

Choose a reason for hiding this comment

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

We're also telling implementers to now take this and do stuff with it right? Describe what's expected to be sent when on this channel if you subclass.

* Gets the current state of the installation session corresponding to the specified loadingUnitId
* and/or moduleName.
*
* <p>Invocations of {@link installDynamicFeature} typically result in asynchronous downloading
Copy link
Member

Choose a reason for hiding this comment

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

I thought you added somewhere the redundancy with loadLibrary. Best be explicit about the web that exists everywhere. i.e. if you installDynamicFeature, you don't really have to check this or listen or the channel, if you listen on the channel, you don't really have to wait for the loadLibrary future, if you loadLibrary, it's the same as installDynamicFeature with loading unit etc, and fully create the web and cross reference everything to everything.

i.e. we don't want users guessing there's 3-4 APIs that can more or less do the same thing. What am I missing out if I just use 1 and don't use all of them.

String resolvedModuleName =
moduleName != null ? moduleName : loadingUnitIdToModuleName(loadingUnitId);
if (resolvedModuleName == null) {
Log.d(TAG, "Dynamic feature module name was null.");
Copy link
Member

Choose a reason for hiding this comment

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

This is probably a more prominent error right? If they look up something that don't exist, don't silently fail.

@GaryQian GaryQian added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Dec 14, 2020
@fluttergithubbot
Copy link
Contributor

This pull request is not suitable for automatic merging in its current state.

  • Please get at least one approved review before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

@fluttergithubbot fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Dec 14, 2020
@GaryQian GaryQian merged commit 056b8be into flutter:master Dec 14, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 15, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 15, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 15, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 15, 2020
gspencergoog pushed a commit to gspencergoog/engine that referenced this pull request Jan 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants