-
Notifications
You must be signed in to change notification settings - Fork 6k
Read loading unit mapping from AndroidManifest instead of strings #23868
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
chingjun
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM!
| private @NonNull SparseArray<String> sessionIdToState; | ||
| private @NonNull Map<String, Integer> nameToSessionId; | ||
|
|
||
| protected @NonNull Map<Integer, List<String>> loadingUnitIdToModuleNames; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be Map<Integer, String>? How can a loading unit to be contained in multiple modules?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is possible to include the same loading unit in more than one module. For example, if you had a credit card feature, and wanted to include it in multiple locales but not china, then you could include the loading unit in multiple modules. This would just pick the first module that includes it, which isn't optimal, but a full system that can analyze which modules to pick with the same loading unit is quite a bit of work. This is an edge case in general, but adding the multiple modules here allows better expansion later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case the "credit card" feature should be its own module, and when user is using it from China we should just avoid using them?
I don't think we should conflate the two things together though, feature modules and locale.
Otherwise what could happen is that when the user is in US, and when the code tries to do credit_card.loadLibrary(), it could then download the loading unit from feature module EU, which contains a lot of other things that the user won't need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restricting a loading unit to be only assigned to one module seems like a reasonable restriction for now. In any case, removing this restriction in the future would be safe and non-breaking, so I can make this just take a single module name for now.
...ndroid/io/flutter/embedding/engine/deferredcomponents/PlayStoreDeferredComponentManager.java
Outdated
Show resolved
Hide resolved
* 63b3440 Roll Fuchsia Linux SDK from mODEe2CNk... to edqShE0QE... (flutter/engine#23873) * f0e25c5 Roll Skia from 3193ff271628 to 2a4c0fbdca1a (3 revisions) (flutter/engine#23875) * 492759e Roll Dart SDK from 82b4c77fb17f to 748993c3997a (1 revision) (flutter/engine#23874) * 8671aef Notify Win32FlutterWindow of cursor updates (flutter/engine#23795) * c8620c3 Implement delayed key event synthesis for Windows (flutter/engine#23524) * ebbf0df Roll Skia from 2a4c0fbdca1a to 8a42b09c162e (9 revisions) (flutter/engine#23878) * bb00cb6 Roll Fuchsia Linux Toolchain from IJxh_9dNS... to 8LaTdqf7w... (flutter/engine#23876) * f77fea2 Roll Dart SDK from 748993c3997a to 2ddf810f71f6 (1 revision) (flutter/engine#23881) * dc22ede Roll Skia from 8a42b09c162e to 9702fc6f3852 (1 revision) (flutter/engine#23882) * 1f30e56 Roll Fuchsia Mac SDK from tuJCioUf3... to 9Lh_vPIXU... (flutter/engine#23883) * 443bf5c Roll Fuchsia Linux SDK from edqShE0QE... to uMOnDLfvl... (flutter/engine#23886) * a152470 Roll Fuchsia Mac SDK from 9Lh_vPIXU... to PsYsfVNbW... (flutter/engine#23888) * 221259b Roll Skia from 9702fc6f3852 to 07c5f52c947d (2 revisions) (flutter/engine#23890) * 381d8bd Roll Skia from 07c5f52c947d to 8d29ab630996 (1 revision) (flutter/engine#23892) * 397274f Roll Skia from 8d29ab630996 to d396cd50ff15 (1 revision) (flutter/engine#23893) * a5c305e push methods return layers with correct class names (flutter/engine#23542) * 4e87f60 Read loading unit mapping from AndroidManifest instead of strings (flutter/engine#23868) * d3a1acb Roll Fuchsia Linux SDK from uMOnDLfvl... to VYUnZ3Tbh... (flutter/engine#23894) * 3d966fa Roll Fuchsia Mac SDK from PsYsfVNbW... to 6swTf93jz... (flutter/engine#23897) * cae9130 Roll Skia from d396cd50ff15 to 5bbf72757349 (2 revisions) (flutter/engine#23898) * f3c5687 Roll Skia from 5bbf72757349 to 069e484cc3b9 (2 revisions) (flutter/engine#23900) * 9365230 Add support for IME-based text input on Windows (flutter/engine#23853) * cad597f Roll Fuchsia Linux SDK from VYUnZ3Tbh... to mrFdelzNr... (flutter/engine#23903) * 4557989 Roll Fuchsia Mac SDK from 6swTf93jz... to 7LGbVIHUD... (flutter/engine#23904)
This PR replaces the string resource based encoding of the loading unit id -> module name mapping. Instead, we now store the mapping in AndroidManifest.xml of a deferred components enabled app's base module.
This helps separate string resources from app configuration.
flutter/flutter#63773 implements the storing of the data inside of the manifest file.