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

Conversation

@GaryQian
Copy link
Contributor

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.

@flutter-dashboard
Copy link

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.

@GaryQian GaryQian requested a review from chingjun January 22, 2021 19:59
Copy link
Contributor

@chingjun chingjun 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!

private @NonNull SparseArray<String> sessionIdToState;
private @NonNull Map<String, Integer> nameToSessionId;

protected @NonNull Map<Integer, List<String>> loadingUnitIdToModuleNames;
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

@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 Jan 24, 2021
@fluttergithubbot fluttergithubbot merged commit 4e87f60 into flutter:master Jan 24, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 25, 2021
zanderso pushed a commit to flutter/flutter that referenced this pull request Jan 25, 2021
* 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)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes platform-android waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants