AssetResolver updating in AssetManager for Dynamic features #23130
Conversation
GaryQian
changed the title
GaryQian
force-pushed
the
splitaotassets
branch
from
f593d8d to
77fdc1e
Compare
shell/common/shell.cc
Outdated
| if (old_resolver->IsUpdatable()) { | ||
| if (index < asset_resolvers.size()) { | ||
| // Push the replacement updated resolver in place of the old_resolver. | ||
| asset_manager->PushBack(std::move(asset_resolvers[index++])); |
There was a problem hiding this comment.
Will the asset_resolvers argument always contains exactly the same number of updatable resolvers in exactly the same order as the AssetManager's current resolver list?
I'm wondering if it would be simpler to add an API to AssetManager that would replace an AssetResolver of a given type with a new instance. There would need to be some way to identify which resolver is the APKAssetProvider - perhaps each AssetResolver subclass could return an enum indicating its type.
There was a problem hiding this comment.
I was hesitant to add too heavy of a solution, but I agree it would be less confusing to explicitly define which resolver type to update.
In most expected circumstances, the number of replacement resolvers would equal the number of updatable resolvers, but i'm not restricting it to be equal here since there is a reasonable course of action for extra resolvers or fewer resolvers. Restricting would necessitate API to reveal the number of updatable resolvers, which isn't necessary for what is trying to be accomplished here.
There was a problem hiding this comment.
I think it would be sufficient to have UpdateResolversByType accept a single AssetResolver instead of a vector. The only current use case for UpdateResolversByType is replacing the APK resolver, and the caller expects there to be exactly one of that type in the AssetManager's resolver list.
|
The unit tests are failing on CI due to weak_ptr thread checks. |
| bool updated = false; | ||
| std::deque<std::unique_ptr<AssetResolver>> new_resolvers; | ||
| for (auto& old_resolver : resolvers_) { | ||
| if (!updated && old_resolver->GetType() == type) { |
There was a problem hiding this comment.
This behavior is somewhat counterintuitive - if updated_asset_resolver is null, then all existing resolvers of the specified type will be removed. But if updated_asset_resolver is non-null, then only the first resolver of that type will be replaced.
Is it necessary to allow updated_asset_resolver to be null? If there is no use case for that, then I'd prefer to limit this API to just replacing the first resolver of the given type.
There was a problem hiding this comment.
Made nullptr input into a no-op.
assets/asset_manager.cc
Outdated
| } | ||
| } | ||
| // Append resolver to the end if not used as a replacement. | ||
| if (!updated && updated_asset_resolver != nullptr) { |
There was a problem hiding this comment.
The updated_asset_resolver != nullptr check is no longer necessary
assets/asset_manager.h
Outdated
| /// `updated_asset_resolver`. The matching AssetResolver is | ||
| /// removed and replaced with `updated_asset_resolvers`. | ||
| /// | ||
| /// AssetResolvers should be updated when the exisitng resolver |
There was a problem hiding this comment.
typo: "existing"
(This also appears in the comments for UpdateAssetResolverByType in other classes)
GaryQian
added
the
waiting for tree to go green
2 participants
Loosely based off of #21611
Adds
IsUpdatableto AssetResolvers. Updatable resolvers may be replaced with newer versions withUpdateAssetResolvers.