From e2cb4eafd38841a0d19993fcc80abbae02afbc8a Mon Sep 17 00:00:00 2001 From: garyqian Date: Wed, 16 Dec 2020 20:04:04 -0800 Subject: [PATCH 01/18] Initial code, add IsUpdatable to AssetResolver --- assets/asset_manager.cc | 5 +++ assets/asset_manager.h | 3 ++ assets/asset_resolver.h | 15 +++++++ assets/directory_asset_bundle.cc | 5 +++ assets/directory_asset_bundle.h | 3 ++ shell/common/platform_view.cc | 4 +- shell/common/platform_view.h | 7 ++-- shell/common/shell.cc | 5 ++- shell/common/shell.h | 3 +- shell/platform/android/apk_asset_provider.cc | 4 ++ shell/platform/android/apk_asset_provider.h | 3 ++ .../flutter/embedding/engine/FlutterJNI.java | 6 +-- .../PlayStoreDynamicFeatureManager.java | 2 +- .../platform/android/platform_view_android.cc | 6 +-- .../platform/android/platform_view_android.h | 3 +- .../android/platform_view_android_jni_impl.cc | 42 +++++++++++-------- 16 files changed, 83 insertions(+), 33 deletions(-) diff --git a/assets/asset_manager.cc b/assets/asset_manager.cc index 31d2131bd05c8..534e6c87bbb88 100644 --- a/assets/asset_manager.cc +++ b/assets/asset_manager.cc @@ -79,4 +79,9 @@ bool AssetManager::IsValidAfterAssetManagerChange() const { return false; } +// |AssetResolver| +bool AssetManager::IsUpdatable() const { + return false; +} + } // namespace flutter diff --git a/assets/asset_manager.h b/assets/asset_manager.h index c6fc28657699d..7e38be2576ad2 100644 --- a/assets/asset_manager.h +++ b/assets/asset_manager.h @@ -33,6 +33,9 @@ class AssetManager final : public AssetResolver { // |AssetResolver| bool IsValidAfterAssetManagerChange() const override; + // |AssetResolver| + bool IsUpdatable() const override; + // |AssetResolver| std::unique_ptr GetAsMapping( const std::string& asset_name) const override; diff --git a/assets/asset_resolver.h b/assets/asset_resolver.h index 470ce3864cfff..cd86e2e505ac8 100644 --- a/assets/asset_resolver.h +++ b/assets/asset_resolver.h @@ -39,6 +39,21 @@ class AssetResolver { /// virtual bool IsValidAfterAssetManagerChange() const = 0; + //---------------------------------------------------------------------------- + /// @brief Some asset resolvers may be replaced by an updated version + /// during runtime. For example, when downloading a dynamic + /// feature, Android provides a new java asset manager that has + /// access to the newly installed assets. This new manager should + /// replace the existing java asset manager resolver. We call + /// this replacement an update as the old resolver is obsolete and + /// the new one should assume responsibility for providing access + /// to android assets. Updatable asset resolvers will be removed + /// in favor of the replacement resolvers at runtime. + /// + /// @return Returns whether this resolver should be updated. + /// + virtual bool IsUpdatable() const = 0; + [[nodiscard]] virtual std::unique_ptr GetAsMapping( const std::string& asset_name) const = 0; diff --git a/assets/directory_asset_bundle.cc b/assets/directory_asset_bundle.cc index 9c09c00bb4533..a94cfeb09baa5 100644 --- a/assets/directory_asset_bundle.cc +++ b/assets/directory_asset_bundle.cc @@ -36,6 +36,11 @@ bool DirectoryAssetBundle::IsValidAfterAssetManagerChange() const { return is_valid_after_asset_manager_change_; } +// |AssetResolver| +bool DirectoryAssetBundle::IsUpdatable() const { + return false; +} + // |AssetResolver| std::unique_ptr DirectoryAssetBundle::GetAsMapping( const std::string& asset_name) const { diff --git a/assets/directory_asset_bundle.h b/assets/directory_asset_bundle.h index b85ff1dbacad0..668d2e8490a06 100644 --- a/assets/directory_asset_bundle.h +++ b/assets/directory_asset_bundle.h @@ -30,6 +30,9 @@ class DirectoryAssetBundle : public AssetResolver { // |AssetResolver| bool IsValidAfterAssetManagerChange() const override; + // |AssetResolver| + bool IsUpdatable() const override; + // |AssetResolver| std::unique_ptr GetAsMapping( const std::string& asset_name) const override; diff --git a/shell/common/platform_view.cc b/shell/common/platform_view.cc index 4205e9853e44f..779d94869541e 100644 --- a/shell/common/platform_view.cc +++ b/shell/common/platform_view.cc @@ -170,7 +170,7 @@ void PlatformView::LoadDartDeferredLibraryError(intptr_t loading_unit_id, const std::string error_message, bool transient) {} -void PlatformView::UpdateAssetManager( - std::shared_ptr asset_manager) {} +void PlatformView::UpdateAssetResolvers( + const std::vector>& asset_resolvers) {} } // namespace flutter diff --git a/shell/common/platform_view.h b/shell/common/platform_view.h index 56e3f6af379f2..cf3f1685e4e0a 100644 --- a/shell/common/platform_view.h +++ b/shell/common/platform_view.h @@ -280,8 +280,8 @@ class PlatformView { /// /// @param[in] asset_manager The asset manager to use. /// - virtual void UpdateAssetManager( - std::shared_ptr asset_manager) = 0; + virtual void UpdateAssetResolvers( + const std::vector>& asset_resolvers) = 0; }; //---------------------------------------------------------------------------- @@ -727,7 +727,8 @@ class PlatformView { /// /// @param[in] asset_manager The asset manager to use. /// - virtual void UpdateAssetManager(std::shared_ptr asset_manager); + virtual void UpdateAssetResolvers( + const std::vector>& asset_resolvers); protected: PlatformView::Delegate& delegate_; diff --git a/shell/common/shell.cc b/shell/common/shell.cc index a58451fd2d5ba..17ca34db7ef95 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -1223,8 +1223,9 @@ void Shell::LoadDartDeferredLibraryError(intptr_t loading_unit_id, transient); } -void Shell::UpdateAssetManager(std::shared_ptr asset_manager) { - engine_->UpdateAssetManager(std::move(asset_manager)); +void Shell::UpdateAssetResolvers( + const std::vector>& asset_resolvers) { + // engine_->UpdateAssetManager(std::move(asset_manager)); } // |Engine::Delegate| diff --git a/shell/common/shell.h b/shell/common/shell.h index 503347c39bbd6..336dfa4ef5840 100644 --- a/shell/common/shell.h +++ b/shell/common/shell.h @@ -536,7 +536,8 @@ class Shell final : public PlatformView::Delegate, bool transient) override; // |PlatformView::Delegate| - void UpdateAssetManager(std::shared_ptr asset_manager) override; + void UpdateAssetResolvers(const std::vector>& + asset_resolvers) override; // |Animator::Delegate| void OnAnimatorBeginFrame(fml::TimePoint frame_target_time) override; diff --git a/shell/platform/android/apk_asset_provider.cc b/shell/platform/android/apk_asset_provider.cc index 73a4a26bd1d2d..8096071e7a641 100644 --- a/shell/platform/android/apk_asset_provider.cc +++ b/shell/platform/android/apk_asset_provider.cc @@ -31,6 +31,10 @@ bool APKAssetProvider::IsValidAfterAssetManagerChange() const { return true; } +bool APKAssetProvider::IsUpdatable() const { + return true; +} + class APKAssetMapping : public fml::Mapping { public: APKAssetMapping(AAsset* asset) : asset_(asset) {} diff --git a/shell/platform/android/apk_asset_provider.h b/shell/platform/android/apk_asset_provider.h index f457b6c4ecc16..5bd0d8a2e5f1f 100644 --- a/shell/platform/android/apk_asset_provider.h +++ b/shell/platform/android/apk_asset_provider.h @@ -32,6 +32,9 @@ class APKAssetProvider final : public AssetResolver { // |flutter::AssetResolver| bool IsValidAfterAssetManagerChange() const override; + // |flutter::AssetResolver| + bool IsUpdatable() const override; + // |flutter::AssetResolver| std::unique_ptr GetAsMapping( const std::string& asset_name) const override; diff --git a/shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java b/shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java index 85d49909cea9d..c5a12252f5a3a 100644 --- a/shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java +++ b/shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java @@ -1056,14 +1056,14 @@ private native void nativeLoadDartDeferredLibrary( * value is `flutter_assets`. */ @UiThread - public void updateAssetManager( + public void updateJavaAssetManager( @NonNull AssetManager assetManager, @NonNull String assetBundlePath) { ensureRunningOnMainThread(); ensureAttachedToNative(); - nativeUpdateAssetManager(nativePlatformViewId, assetManager, assetBundlePath); + nativeUpdateJavaAssetManager(nativePlatformViewId, assetManager, assetBundlePath); } - private native void nativeUpdateAssetManager( + private native void nativeUpdateJavaAssetManager( long nativePlatformViewId, @NonNull AssetManager assetManager, @NonNull String assetBundlePath); diff --git a/shell/platform/android/io/flutter/embedding/engine/dynamicfeatures/PlayStoreDynamicFeatureManager.java b/shell/platform/android/io/flutter/embedding/engine/dynamicfeatures/PlayStoreDynamicFeatureManager.java index 2e1b8592a2dd4..41032e13bbaa2 100644 --- a/shell/platform/android/io/flutter/embedding/engine/dynamicfeatures/PlayStoreDynamicFeatureManager.java +++ b/shell/platform/android/io/flutter/embedding/engine/dynamicfeatures/PlayStoreDynamicFeatureManager.java @@ -317,7 +317,7 @@ public void loadAssets(int loadingUnitId, String moduleName) { context = context.createPackageContext(context.getPackageName(), 0); AssetManager assetManager = context.getAssets(); - flutterJNI.updateAssetManager( + flutterJNI.updateJavaAssetManager( assetManager, // TODO(garyq): Made the "flutter_assets" directory dynamic based off of DartEntryPoint. "flutter_assets"); diff --git a/shell/platform/android/platform_view_android.cc b/shell/platform/android/platform_view_android.cc index 33bf0b4fc3e25..74feec264f0e3 100644 --- a/shell/platform/android/platform_view_android.cc +++ b/shell/platform/android/platform_view_android.cc @@ -363,9 +363,9 @@ void PlatformViewAndroid::LoadDartDeferredLibraryError( } // |PlatformView| -void PlatformViewAndroid::UpdateAssetManager( - std::shared_ptr asset_manager) { - delegate_.UpdateAssetManager(std::move(asset_manager)); +void PlatformViewAndroid::UpdateAssetResolvers( + const std::vector>& asset_resolvers) { + delegate_.UpdateAssetResolvers(asset_resolvers); } void PlatformViewAndroid::InstallFirstFrameCallback() { diff --git a/shell/platform/android/platform_view_android.h b/shell/platform/android/platform_view_android.h index 19631203365f2..8bf823d41bc37 100644 --- a/shell/platform/android/platform_view_android.h +++ b/shell/platform/android/platform_view_android.h @@ -104,7 +104,8 @@ class PlatformViewAndroid final : public PlatformView { bool transient) override; // |PlatformView| - void UpdateAssetManager(std::shared_ptr asset_manager) override; + void UpdateAssetResolvers(const std::vector>& + asset_resolvers) override; private: const std::shared_ptr jni_facade_; diff --git a/shell/platform/android/platform_view_android_jni_impl.cc b/shell/platform/android/platform_view_android_jni_impl.cc index d83c615043332..5ad7cc698b586 100644 --- a/shell/platform/android/platform_view_android_jni_impl.cc +++ b/shell/platform/android/platform_view_android_jni_impl.cc @@ -573,22 +573,30 @@ static void LoadDartDeferredLibrary(JNIEnv* env, // TODO(garyq): persist additional asset resolvers by updating instead of // replacing with newly created asset_manager -static void UpdateAssetManager(JNIEnv* env, - jobject obj, - jlong shell_holder, - jobject jAssetManager, - jstring jAssetBundlePath) { - auto asset_manager = std::make_shared(); - asset_manager->PushBack(std::make_unique( - env, // jni environment - jAssetManager, // asset manager - fml::jni::JavaStringToString(env, jAssetBundlePath)) // apk asset dir - ); +static void UpdateJavaAssetManager(JNIEnv* env, + jobject obj, + jlong shell_holder, + jobject jAssetManager, + jstring jAssetBundlePath) { + // auto asset_manager = std::make_shared(); + // asset_manager->PushBack(std::make_unique( + // env, // jni + // environment jAssetManager, // + // asset manager fml::jni::JavaStringToString(env, jAssetBundlePath)) // + // apk asset dir + // ); // Create config to set persistent cache asset manager - RunConfiguration config(nullptr, std::move(asset_manager)); - - ANDROID_SHELL_HOLDER->GetPlatformView()->UpdateAssetManager( - config.GetAssetManager()); + // RunConfiguration config(nullptr, std::move(asset_manager)); + auto asset_resolver = std::make_unique( + env, // jni environment + jAssetManager, // asset manager + fml::jni::JavaStringToString(env, jAssetBundlePath)); // apk asset dir + std::unique_ptr>> resolver_vector = + std::make_unique>>(); + resolver_vector->push_back(std::move(asset_resolver)); + + ANDROID_SHELL_HOLDER->GetPlatformView()->UpdateAssetResolvers( + *resolver_vector); } bool RegisterApi(JNIEnv* env) { @@ -753,10 +761,10 @@ bool RegisterApi(JNIEnv* env) { .fnPtr = reinterpret_cast(&LoadDartDeferredLibrary), }, { - .name = "nativeUpdateAssetManager", + .name = "nativeUpdateJavaAssetManager", .signature = "(JLandroid/content/res/AssetManager;Ljava/lang/String;)V", - .fnPtr = reinterpret_cast(&UpdateAssetManager), + .fnPtr = reinterpret_cast(&UpdateJavaAssetManager), }, { .name = "nativeDynamicFeatureInstallFailure", From 16633bcac34cf5e8f877570c7a053a0e8d9be152 Mon Sep 17 00:00:00 2001 From: garyqian Date: Thu, 17 Dec 2020 21:06:03 -0800 Subject: [PATCH 02/18] Add update algo --- shell/common/platform_view.cc | 2 +- shell/common/platform_view.h | 22 +++++++++++++------ shell/common/shell.cc | 21 ++++++++++++++++-- shell/common/shell.h | 2 +- .../platform/android/platform_view_android.cc | 2 +- .../platform/android/platform_view_android.h | 2 +- .../android/platform_view_android_jni_impl.cc | 21 +++++------------- 7 files changed, 44 insertions(+), 28 deletions(-) diff --git a/shell/common/platform_view.cc b/shell/common/platform_view.cc index 779d94869541e..48f94a6874d0b 100644 --- a/shell/common/platform_view.cc +++ b/shell/common/platform_view.cc @@ -171,6 +171,6 @@ void PlatformView::LoadDartDeferredLibraryError(intptr_t loading_unit_id, bool transient) {} void PlatformView::UpdateAssetResolvers( - const std::vector>& asset_resolvers) {} + const std::vector>& asset_resolvers) {} } // namespace flutter diff --git a/shell/common/platform_view.h b/shell/common/platform_view.h index cf3f1685e4e0a..6849722837db2 100644 --- a/shell/common/platform_view.h +++ b/shell/common/platform_view.h @@ -281,7 +281,7 @@ class PlatformView { /// @param[in] asset_manager The asset manager to use. /// virtual void UpdateAssetResolvers( - const std::vector>& asset_resolvers) = 0; + const std::vector>& asset_resolvers) = 0; }; //---------------------------------------------------------------------------- @@ -720,15 +720,23 @@ class PlatformView { const std::string error_message, bool transient); - // TODO(garyq): Implement a proper asset_resolver replacement instead of - // overwriting the entire asset manager. //-------------------------------------------------------------------------- - /// @brief Sets the asset manager of the engine to asset_manager - /// - /// @param[in] asset_manager The asset manager to use. + /// @brief Replaces asset resolvers in the current engine's + /// `AssetManager` that are marked as updatable + /// (`IsUpdateable()` returns true). Updatable AssetResolvers + /// are removed and replaced with the next available resolver + /// in `asset_resolvers`. If less resolvers are provided than + /// existing resolvers marked updatable, then the extra + /// existing resolvers will be removed without replacement. If + /// more resolvers are provided than existing resolvers marked + /// updatable, then the extra provided resolvers will be added + /// to the end of the resolver deque. + /// + /// @param[in] asset_resolvers The asset resolvers to replace updatable + /// existing resolvers with. /// virtual void UpdateAssetResolvers( - const std::vector>& asset_resolvers); + const std::vector>& asset_resolvers); protected: PlatformView::Delegate& delegate_; diff --git a/shell/common/shell.cc b/shell/common/shell.cc index 17ca34db7ef95..c6a9171eee38c 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -1224,8 +1224,25 @@ void Shell::LoadDartDeferredLibraryError(intptr_t loading_unit_id, } void Shell::UpdateAssetResolvers( - const std::vector>& asset_resolvers) { - // engine_->UpdateAssetManager(std::move(asset_manager)); + const std::vector>& asset_resolvers) { + size_t index = 0; + auto asset_manager = std::make_shared(); + auto old_asset_manager = engine_->GetAssetManager(); + if (old_asset_manager != nullptr) { + for (auto& old_resolver : old_asset_manager->TakeResolvers()) { + if (old_resolver->IsUpdatable()) { + if (index < asset_resolvers.size()) { + asset_manager->PushBack(std::move(asset_resolvers[index++])); + } + } else { + asset_manager->PushBack(std::move(old_resolver)); + } + } + } + while (index < asset_resolvers.size()) { + asset_manager->PushBack(std::move(asset_resolvers[index++])); + } + engine_->UpdateAssetManager(std::move(asset_manager)); } // |Engine::Delegate| diff --git a/shell/common/shell.h b/shell/common/shell.h index 336dfa4ef5840..0107a68146a6b 100644 --- a/shell/common/shell.h +++ b/shell/common/shell.h @@ -536,7 +536,7 @@ class Shell final : public PlatformView::Delegate, bool transient) override; // |PlatformView::Delegate| - void UpdateAssetResolvers(const std::vector>& + void UpdateAssetResolvers(const std::vector>& asset_resolvers) override; // |Animator::Delegate| diff --git a/shell/platform/android/platform_view_android.cc b/shell/platform/android/platform_view_android.cc index 74feec264f0e3..80a6fafb887e8 100644 --- a/shell/platform/android/platform_view_android.cc +++ b/shell/platform/android/platform_view_android.cc @@ -364,7 +364,7 @@ void PlatformViewAndroid::LoadDartDeferredLibraryError( // |PlatformView| void PlatformViewAndroid::UpdateAssetResolvers( - const std::vector>& asset_resolvers) { + const std::vector>& asset_resolvers) { delegate_.UpdateAssetResolvers(asset_resolvers); } diff --git a/shell/platform/android/platform_view_android.h b/shell/platform/android/platform_view_android.h index 8bf823d41bc37..9121c2a3a3808 100644 --- a/shell/platform/android/platform_view_android.h +++ b/shell/platform/android/platform_view_android.h @@ -104,7 +104,7 @@ class PlatformViewAndroid final : public PlatformView { bool transient) override; // |PlatformView| - void UpdateAssetResolvers(const std::vector>& + void UpdateAssetResolvers(const std::vector>& asset_resolvers) override; private: diff --git a/shell/platform/android/platform_view_android_jni_impl.cc b/shell/platform/android/platform_view_android_jni_impl.cc index 5ad7cc698b586..97477ba3ea2f3 100644 --- a/shell/platform/android/platform_view_android_jni_impl.cc +++ b/shell/platform/android/platform_view_android_jni_impl.cc @@ -571,32 +571,23 @@ static void LoadDartDeferredLibrary(JNIEnv* env, std::move(instructions_mapping)); } -// TODO(garyq): persist additional asset resolvers by updating instead of -// replacing with newly created asset_manager static void UpdateJavaAssetManager(JNIEnv* env, jobject obj, jlong shell_holder, jobject jAssetManager, jstring jAssetBundlePath) { - // auto asset_manager = std::make_shared(); - // asset_manager->PushBack(std::make_unique( - // env, // jni - // environment jAssetManager, // - // asset manager fml::jni::JavaStringToString(env, jAssetBundlePath)) // - // apk asset dir - // ); - // Create config to set persistent cache asset manager - // RunConfiguration config(nullptr, std::move(asset_manager)); auto asset_resolver = std::make_unique( env, // jni environment jAssetManager, // asset manager fml::jni::JavaStringToString(env, jAssetBundlePath)); // apk asset dir - std::unique_ptr>> resolver_vector = - std::make_unique>>(); - resolver_vector->push_back(std::move(asset_resolver)); + std::vector> resolver_vector; + // auto resolver_vector = + // std::make_unique>>(); + // resolver_vector->push_back(std::move(asset_resolver)); + resolver_vector.push_back(std::move(asset_resolver)); ANDROID_SHELL_HOLDER->GetPlatformView()->UpdateAssetResolvers( - *resolver_vector); + resolver_vector); } bool RegisterApi(JNIEnv* env) { From fd6b49ae08fd0f2b29e2c5046b0eec233cce79a1 Mon Sep 17 00:00:00 2001 From: garyqian Date: Thu, 17 Dec 2020 21:57:16 -0800 Subject: [PATCH 03/18] Working system --- shell/common/platform_view.cc | 2 +- shell/common/platform_view.h | 24 ++++++++++++------- shell/common/shell.cc | 2 +- shell/common/shell.h | 4 ++-- .../platform/android/platform_view_android.cc | 2 +- .../platform/android/platform_view_android.h | 4 ++-- 6 files changed, 23 insertions(+), 15 deletions(-) diff --git a/shell/common/platform_view.cc b/shell/common/platform_view.cc index 48f94a6874d0b..699362521f1b5 100644 --- a/shell/common/platform_view.cc +++ b/shell/common/platform_view.cc @@ -171,6 +171,6 @@ void PlatformView::LoadDartDeferredLibraryError(intptr_t loading_unit_id, bool transient) {} void PlatformView::UpdateAssetResolvers( - const std::vector>& asset_resolvers) {} + std::vector>& asset_resolvers) {} } // namespace flutter diff --git a/shell/common/platform_view.h b/shell/common/platform_view.h index 6849722837db2..87450a24b9847 100644 --- a/shell/common/platform_view.h +++ b/shell/common/platform_view.h @@ -281,7 +281,7 @@ class PlatformView { /// @param[in] asset_manager The asset manager to use. /// virtual void UpdateAssetResolvers( - const std::vector>& asset_resolvers) = 0; + std::vector>& asset_resolvers) = 0; }; //---------------------------------------------------------------------------- @@ -725,18 +725,26 @@ class PlatformView { /// `AssetManager` that are marked as updatable /// (`IsUpdateable()` returns true). Updatable AssetResolvers /// are removed and replaced with the next available resolver - /// in `asset_resolvers`. If less resolvers are provided than - /// existing resolvers marked updatable, then the extra - /// existing resolvers will be removed without replacement. If - /// more resolvers are provided than existing resolvers marked - /// updatable, then the extra provided resolvers will be added - /// to the end of the resolver deque. + /// in `asset_resolvers`. + /// + /// AssetResolvers should be updated when the exisitng resolver + /// becomes obsolete and a newer one becomes available that + /// provides updated access to the same type of assets as the + /// existing one. This update process is meant to be performed at + /// runtime. + /// + /// If less resolvers are provided than existing updatable + /// resolvers, the the extra existing updatable resolvers will be + /// removed without replacement. If more resolvers are provided + /// than existing updatable resolvers, then the extra provided + /// resolvers will be added to the end of the AssetManager + /// resolvers list. /// /// @param[in] asset_resolvers The asset resolvers to replace updatable /// existing resolvers with. /// virtual void UpdateAssetResolvers( - const std::vector>& asset_resolvers); + std::vector>& asset_resolvers); protected: PlatformView::Delegate& delegate_; diff --git a/shell/common/shell.cc b/shell/common/shell.cc index c6a9171eee38c..ebfa6f444a588 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -1224,7 +1224,7 @@ void Shell::LoadDartDeferredLibraryError(intptr_t loading_unit_id, } void Shell::UpdateAssetResolvers( - const std::vector>& asset_resolvers) { + std::vector>& asset_resolvers) { size_t index = 0; auto asset_manager = std::make_shared(); auto old_asset_manager = engine_->GetAssetManager(); diff --git a/shell/common/shell.h b/shell/common/shell.h index 0107a68146a6b..6414688f99461 100644 --- a/shell/common/shell.h +++ b/shell/common/shell.h @@ -536,8 +536,8 @@ class Shell final : public PlatformView::Delegate, bool transient) override; // |PlatformView::Delegate| - void UpdateAssetResolvers(const std::vector>& - asset_resolvers) override; + void UpdateAssetResolvers( + std::vector>& asset_resolvers) override; // |Animator::Delegate| void OnAnimatorBeginFrame(fml::TimePoint frame_target_time) override; diff --git a/shell/platform/android/platform_view_android.cc b/shell/platform/android/platform_view_android.cc index 80a6fafb887e8..b7e3241fe0dbe 100644 --- a/shell/platform/android/platform_view_android.cc +++ b/shell/platform/android/platform_view_android.cc @@ -364,7 +364,7 @@ void PlatformViewAndroid::LoadDartDeferredLibraryError( // |PlatformView| void PlatformViewAndroid::UpdateAssetResolvers( - const std::vector>& asset_resolvers) { + std::vector>& asset_resolvers) { delegate_.UpdateAssetResolvers(asset_resolvers); } diff --git a/shell/platform/android/platform_view_android.h b/shell/platform/android/platform_view_android.h index 9121c2a3a3808..87cab1cf2794f 100644 --- a/shell/platform/android/platform_view_android.h +++ b/shell/platform/android/platform_view_android.h @@ -104,8 +104,8 @@ class PlatformViewAndroid final : public PlatformView { bool transient) override; // |PlatformView| - void UpdateAssetResolvers(const std::vector>& - asset_resolvers) override; + void UpdateAssetResolvers( + std::vector>& asset_resolvers) override; private: const std::shared_ptr jni_facade_; From a87031029b8ffba2ea04416ce0a1558f10a04a10 Mon Sep 17 00:00:00 2001 From: garyqian Date: Mon, 21 Dec 2020 08:51:31 -0800 Subject: [PATCH 04/18] Tests compile --- assets/asset_resolver.h | 21 ++++---- shell/common/platform_view.cc | 4 +- shell/common/shell.cc | 4 +- shell/common/shell_unittests.cc | 53 +++++++++++++++++++ shell/platform/android/apk_asset_provider.cc | 3 ++ .../android/platform_view_android_jni_impl.cc | 3 -- 6 files changed, 74 insertions(+), 14 deletions(-) diff --git a/assets/asset_resolver.h b/assets/asset_resolver.h index cd86e2e505ac8..62312ca71c505 100644 --- a/assets/asset_resolver.h +++ b/assets/asset_resolver.h @@ -41,16 +41,19 @@ class AssetResolver { //---------------------------------------------------------------------------- /// @brief Some asset resolvers may be replaced by an updated version - /// during runtime. For example, when downloading a dynamic - /// feature, Android provides a new java asset manager that has - /// access to the newly installed assets. This new manager should - /// replace the existing java asset manager resolver. We call - /// this replacement an update as the old resolver is obsolete and - /// the new one should assume responsibility for providing access - /// to android assets. Updatable asset resolvers will be removed - /// in favor of the replacement resolvers at runtime. + /// during runtime. Resolvers marked `Updatable` are removed and + /// invalidated when an update is processed and is replaced by a + /// new resolver that provides the latest availablity state of + /// assets. This usually adds access to new assets or removes + /// access to old/invalid/deleted assets. For example, when + /// downloading a dynamic feature, Android provides a new java + /// asset manager that has access to the newly installed assets. + /// This new manager should replace the existing java asset + /// manager resolver. We call this replacement an update as the + /// old resolver is obsolete and the new one should assume + /// responsibility for providing access to android assets. /// - /// @return Returns whether this resolver should be updated. + /// @return Returns whether this resolver can be updated. /// virtual bool IsUpdatable() const = 0; diff --git a/shell/common/platform_view.cc b/shell/common/platform_view.cc index 699362521f1b5..e450559ba7dc6 100644 --- a/shell/common/platform_view.cc +++ b/shell/common/platform_view.cc @@ -171,6 +171,8 @@ void PlatformView::LoadDartDeferredLibraryError(intptr_t loading_unit_id, bool transient) {} void PlatformView::UpdateAssetResolvers( - std::vector>& asset_resolvers) {} + std::vector>& asset_resolvers) { + delegate_.UpdateAssetResolvers(asset_resolvers); +} } // namespace flutter diff --git a/shell/common/shell.cc b/shell/common/shell.cc index ebfa6f444a588..7e422c7253298 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -1232,13 +1232,15 @@ void Shell::UpdateAssetResolvers( for (auto& old_resolver : old_asset_manager->TakeResolvers()) { 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++])); - } + } // Drop the resolver if no replacement available. } else { asset_manager->PushBack(std::move(old_resolver)); } } } + // Append all extra resolvers to the end. while (index < asset_resolvers.size()) { asset_manager->PushBack(std::move(asset_resolvers[index++])); } diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index c3b0f13c1be2d..ac200ba8c4c76 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -9,6 +9,7 @@ #include #include #include +#include #include "assets/directory_asset_bundle.h" #include "flutter/common/graphics/persistent_cache.h" @@ -115,6 +116,32 @@ class MockPlatformView : public PlatformView { }; } // namespace +class TestAssetResolver final : public AssetResolver { + public: + TestAssetResolver(bool updatable) : updatable_(updatable) {} + + // ~TestAssetResolver() {} + + bool IsValid() const override { return true; } + + bool IsValidAfterAssetManagerChange() const override { return true; } + + bool IsUpdatable() const override { return updatable_; } + + std::unique_ptr GetAsMapping( + const std::string& asset_name) const override { + return nullptr; + } + + std::vector> GetAsMappings( + const std::string& asset_pattern) const override { + return {}; + }; + + private: + bool updatable_; +}; + static bool ValidateShell(Shell* shell) { if (!shell) { return false; @@ -188,6 +215,32 @@ static void TestDartVmFlags(std::vector& flags) { EXPECT_EQ(settings.dart_flags[i], flags[i]); } } +TEST_F(ShellTest, UpdateAssetResolvers) { + ASSERT_FALSE(DartVMRef::IsInstanceRunning()); + Settings settings = CreateSettingsForFixture(); + ThreadHost thread_host("io.flutter.test." + GetCurrentTestName() + ".", + ThreadHost::Type::Platform); + auto task_runner = thread_host.platform_thread->GetTaskRunner(); + TaskRunners task_runners("test", task_runner, task_runner, task_runner, + task_runner); + auto shell = CreateShell(std::move(settings), task_runners); + ASSERT_TRUE(DartVMRef::IsInstanceRunning()); + ASSERT_TRUE(ValidateShell(shell.get())); + + auto configuration = RunConfiguration::InferFromSettings(settings); + configuration.SetEntrypoint("emptyMain"); + RunEngine(shell.get(), std::move(configuration)); + + auto platform_view = + std::make_unique(*shell.get(), std::move(task_runners)); + + std::vector> resolver_vector; + resolver_vector.push_back(std::make_unique(true)); + platform_view->UpdateAssetResolvers(resolver_vector); + + // DestroyShell(std::move(shell), std::move(task_runners)); + ASSERT_FALSE(DartVMRef::IsInstanceRunning()); +} TEST_F(ShellTest, InitializeWithInvalidThreads) { ASSERT_FALSE(DartVMRef::IsInstanceRunning()); diff --git a/shell/platform/android/apk_asset_provider.cc b/shell/platform/android/apk_asset_provider.cc index 8096071e7a641..717df0557d0f5 100644 --- a/shell/platform/android/apk_asset_provider.cc +++ b/shell/platform/android/apk_asset_provider.cc @@ -32,6 +32,9 @@ bool APKAssetProvider::IsValidAfterAssetManagerChange() const { } bool APKAssetProvider::IsUpdatable() const { + // APKAssetProvider is always updatable to allow dynamic features to + // runtime-update the Java AssetManager instance being used when a new dynamic + // feature is installed. return true; } diff --git a/shell/platform/android/platform_view_android_jni_impl.cc b/shell/platform/android/platform_view_android_jni_impl.cc index 97477ba3ea2f3..053d5ba993060 100644 --- a/shell/platform/android/platform_view_android_jni_impl.cc +++ b/shell/platform/android/platform_view_android_jni_impl.cc @@ -581,9 +581,6 @@ static void UpdateJavaAssetManager(JNIEnv* env, jAssetManager, // asset manager fml::jni::JavaStringToString(env, jAssetBundlePath)); // apk asset dir std::vector> resolver_vector; - // auto resolver_vector = - // std::make_unique>>(); - // resolver_vector->push_back(std::move(asset_resolver)); resolver_vector.push_back(std::move(asset_resolver)); ANDROID_SHELL_HOLDER->GetPlatformView()->UpdateAssetResolvers( From 394e5fd75414ee9d9256766c628b72c9d5b204ae Mon Sep 17 00:00:00 2001 From: garyqian Date: Mon, 21 Dec 2020 12:45:47 -0800 Subject: [PATCH 05/18] Test coverage complete and passing --- assets/asset_manager.h | 2 + shell/common/shell_unittests.cc | 116 +++++++++++++++++++++++++++++--- 2 files changed, 109 insertions(+), 9 deletions(-) diff --git a/assets/asset_manager.h b/assets/asset_manager.h index 7e38be2576ad2..4bca77debd226 100644 --- a/assets/asset_manager.h +++ b/assets/asset_manager.h @@ -44,6 +44,8 @@ class AssetManager final : public AssetResolver { std::vector> GetAsMappings( const std::string& asset_pattern) const override; + size_t Size() { return resolvers_.size(); } + private: std::deque> resolvers_; diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index ac200ba8c4c76..8ac8a9f9b416d 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -116,17 +116,19 @@ class MockPlatformView : public PlatformView { }; } // namespace -class TestAssetResolver final : public AssetResolver { +class TestAssetResolver : public AssetResolver { public: - TestAssetResolver(bool updatable) : updatable_(updatable) {} - - // ~TestAssetResolver() {} + TestAssetResolver(bool updatable, int valid) + : updatable_(updatable), valid_(valid) {} bool IsValid() const override { return true; } - bool IsValidAfterAssetManagerChange() const override { return true; } + bool IsValidAfterAssetManagerChange() const override { return valid_; } - bool IsUpdatable() const override { return updatable_; } + bool IsUpdatable() const override { + FML_LOG(ERROR) << "Returning updatable_ " << updatable_; + return updatable_; + } std::unique_ptr GetAsMapping( const std::string& asset_name) const override { @@ -140,6 +142,7 @@ class TestAssetResolver final : public AssetResolver { private: bool updatable_; + int valid_; }; static bool ValidateShell(Shell* shell) { @@ -215,7 +218,53 @@ static void TestDartVmFlags(std::vector& flags) { EXPECT_EQ(settings.dart_flags[i], flags[i]); } } -TEST_F(ShellTest, UpdateAssetResolvers) { + +TEST_F(ShellTest, UpdateAssetResolversReplaces) { + ASSERT_FALSE(DartVMRef::IsInstanceRunning()); + Settings settings = CreateSettingsForFixture(); + ThreadHost thread_host("io.flutter.test." + GetCurrentTestName() + ".", + ThreadHost::Type::Platform); + auto task_runner = thread_host.platform_thread->GetTaskRunner(); + TaskRunners task_runners("test", task_runner, task_runner, task_runner, + task_runner); + auto shell = CreateShell(std::move(settings), task_runners); + ASSERT_TRUE(DartVMRef::IsInstanceRunning()); + ASSERT_TRUE(ValidateShell(shell.get())); + + auto configuration = RunConfiguration::InferFromSettings(settings); + configuration.SetEntrypoint("emptyMain"); + RunEngine(shell.get(), std::move(configuration)); + + auto platform_view = + std::make_unique(*shell.get(), std::move(task_runners)); + + auto asset_manager = shell->GetEngine()->GetAssetManager(); + + auto old_resolver = std::make_unique(true, true); + ASSERT_TRUE(old_resolver->IsUpdatable()); + ASSERT_TRUE(old_resolver->IsValid()); + asset_manager->PushBack(std::move(old_resolver)); + + std::vector> resolver_vector; + auto updated_resolver = std::make_unique(true, false); + ASSERT_TRUE(updated_resolver->IsUpdatable()); + ASSERT_FALSE(updated_resolver->IsValidAfterAssetManagerChange()); + resolver_vector.push_back(std::move(updated_resolver)); + platform_view->UpdateAssetResolvers(resolver_vector); + + auto resolvers = shell->GetEngine()->GetAssetManager()->TakeResolvers(); + ASSERT_EQ(resolvers.size(), 2ull); + ASSERT_FALSE(resolvers[0]->IsUpdatable()); + ASSERT_TRUE(resolvers[0]->IsValidAfterAssetManagerChange()); + + ASSERT_TRUE(resolvers[1]->IsUpdatable()); + ASSERT_FALSE(resolvers[1]->IsValidAfterAssetManagerChange()); + + DestroyShell(std::move(shell), std::move(task_runners)); + ASSERT_FALSE(DartVMRef::IsInstanceRunning()); +} + +TEST_F(ShellTest, UpdateAssetResolversAppends) { ASSERT_FALSE(DartVMRef::IsInstanceRunning()); Settings settings = CreateSettingsForFixture(); ThreadHost thread_host("io.flutter.test." + GetCurrentTestName() + ".", @@ -235,10 +284,59 @@ TEST_F(ShellTest, UpdateAssetResolvers) { std::make_unique(*shell.get(), std::move(task_runners)); std::vector> resolver_vector; - resolver_vector.push_back(std::make_unique(true)); + auto updated_resolver = std::make_unique(true, false); + ASSERT_TRUE(updated_resolver->IsUpdatable()); + ASSERT_FALSE(updated_resolver->IsValidAfterAssetManagerChange()); + resolver_vector.push_back(std::move(updated_resolver)); platform_view->UpdateAssetResolvers(resolver_vector); - // DestroyShell(std::move(shell), std::move(task_runners)); + auto resolvers = shell->GetEngine()->GetAssetManager()->TakeResolvers(); + ASSERT_EQ(resolvers.size(), 2ull); + ASSERT_FALSE(resolvers[0]->IsUpdatable()); + ASSERT_TRUE(resolvers[0]->IsValidAfterAssetManagerChange()); + + ASSERT_TRUE(resolvers[1]->IsUpdatable()); + ASSERT_FALSE(resolvers[1]->IsValidAfterAssetManagerChange()); + + DestroyShell(std::move(shell), std::move(task_runners)); + ASSERT_FALSE(DartVMRef::IsInstanceRunning()); +} + +TEST_F(ShellTest, UpdateAssetResolversRemoves) { + ASSERT_FALSE(DartVMRef::IsInstanceRunning()); + Settings settings = CreateSettingsForFixture(); + ThreadHost thread_host("io.flutter.test." + GetCurrentTestName() + ".", + ThreadHost::Type::Platform); + auto task_runner = thread_host.platform_thread->GetTaskRunner(); + TaskRunners task_runners("test", task_runner, task_runner, task_runner, + task_runner); + auto shell = CreateShell(std::move(settings), task_runners); + ASSERT_TRUE(DartVMRef::IsInstanceRunning()); + ASSERT_TRUE(ValidateShell(shell.get())); + + auto configuration = RunConfiguration::InferFromSettings(settings); + configuration.SetEntrypoint("emptyMain"); + RunEngine(shell.get(), std::move(configuration)); + + auto platform_view = + std::make_unique(*shell.get(), std::move(task_runners)); + + auto asset_manager = shell->GetEngine()->GetAssetManager(); + + auto old_resolver = std::make_unique(true, true); + ASSERT_TRUE(old_resolver->IsUpdatable()); + ASSERT_TRUE(old_resolver->IsValid()); + asset_manager->PushBack(std::move(old_resolver)); + + std::vector> resolver_vector; + platform_view->UpdateAssetResolvers(resolver_vector); + + auto resolvers = shell->GetEngine()->GetAssetManager()->TakeResolvers(); + ASSERT_EQ(resolvers.size(), 1ull); + ASSERT_FALSE(resolvers[0]->IsUpdatable()); + ASSERT_TRUE(resolvers[0]->IsValidAfterAssetManagerChange()); + + DestroyShell(std::move(shell), std::move(task_runners)); ASSERT_FALSE(DartVMRef::IsInstanceRunning()); } From 77fdc1eb264250485c833f92d386394b0165d919 Mon Sep 17 00:00:00 2001 From: garyqian Date: Mon, 21 Dec 2020 13:09:32 -0800 Subject: [PATCH 06/18] FIx java test --- shell/common/shell_unittests.cc | 248 +++++++++--------- .../PlayStoreDynamicFeatureManagerTest.java | 2 +- 2 files changed, 124 insertions(+), 126 deletions(-) diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index 8ac8a9f9b416d..991e3ac549c91 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -123,12 +123,10 @@ class TestAssetResolver : public AssetResolver { bool IsValid() const override { return true; } + // This is used to identify if replacement was made or not. bool IsValidAfterAssetManagerChange() const override { return valid_; } - bool IsUpdatable() const override { - FML_LOG(ERROR) << "Returning updatable_ " << updatable_; - return updatable_; - } + bool IsUpdatable() const override { return updatable_; } std::unique_ptr GetAsMapping( const std::string& asset_name) const override { @@ -219,127 +217,6 @@ static void TestDartVmFlags(std::vector& flags) { } } -TEST_F(ShellTest, UpdateAssetResolversReplaces) { - ASSERT_FALSE(DartVMRef::IsInstanceRunning()); - Settings settings = CreateSettingsForFixture(); - ThreadHost thread_host("io.flutter.test." + GetCurrentTestName() + ".", - ThreadHost::Type::Platform); - auto task_runner = thread_host.platform_thread->GetTaskRunner(); - TaskRunners task_runners("test", task_runner, task_runner, task_runner, - task_runner); - auto shell = CreateShell(std::move(settings), task_runners); - ASSERT_TRUE(DartVMRef::IsInstanceRunning()); - ASSERT_TRUE(ValidateShell(shell.get())); - - auto configuration = RunConfiguration::InferFromSettings(settings); - configuration.SetEntrypoint("emptyMain"); - RunEngine(shell.get(), std::move(configuration)); - - auto platform_view = - std::make_unique(*shell.get(), std::move(task_runners)); - - auto asset_manager = shell->GetEngine()->GetAssetManager(); - - auto old_resolver = std::make_unique(true, true); - ASSERT_TRUE(old_resolver->IsUpdatable()); - ASSERT_TRUE(old_resolver->IsValid()); - asset_manager->PushBack(std::move(old_resolver)); - - std::vector> resolver_vector; - auto updated_resolver = std::make_unique(true, false); - ASSERT_TRUE(updated_resolver->IsUpdatable()); - ASSERT_FALSE(updated_resolver->IsValidAfterAssetManagerChange()); - resolver_vector.push_back(std::move(updated_resolver)); - platform_view->UpdateAssetResolvers(resolver_vector); - - auto resolvers = shell->GetEngine()->GetAssetManager()->TakeResolvers(); - ASSERT_EQ(resolvers.size(), 2ull); - ASSERT_FALSE(resolvers[0]->IsUpdatable()); - ASSERT_TRUE(resolvers[0]->IsValidAfterAssetManagerChange()); - - ASSERT_TRUE(resolvers[1]->IsUpdatable()); - ASSERT_FALSE(resolvers[1]->IsValidAfterAssetManagerChange()); - - DestroyShell(std::move(shell), std::move(task_runners)); - ASSERT_FALSE(DartVMRef::IsInstanceRunning()); -} - -TEST_F(ShellTest, UpdateAssetResolversAppends) { - ASSERT_FALSE(DartVMRef::IsInstanceRunning()); - Settings settings = CreateSettingsForFixture(); - ThreadHost thread_host("io.flutter.test." + GetCurrentTestName() + ".", - ThreadHost::Type::Platform); - auto task_runner = thread_host.platform_thread->GetTaskRunner(); - TaskRunners task_runners("test", task_runner, task_runner, task_runner, - task_runner); - auto shell = CreateShell(std::move(settings), task_runners); - ASSERT_TRUE(DartVMRef::IsInstanceRunning()); - ASSERT_TRUE(ValidateShell(shell.get())); - - auto configuration = RunConfiguration::InferFromSettings(settings); - configuration.SetEntrypoint("emptyMain"); - RunEngine(shell.get(), std::move(configuration)); - - auto platform_view = - std::make_unique(*shell.get(), std::move(task_runners)); - - std::vector> resolver_vector; - auto updated_resolver = std::make_unique(true, false); - ASSERT_TRUE(updated_resolver->IsUpdatable()); - ASSERT_FALSE(updated_resolver->IsValidAfterAssetManagerChange()); - resolver_vector.push_back(std::move(updated_resolver)); - platform_view->UpdateAssetResolvers(resolver_vector); - - auto resolvers = shell->GetEngine()->GetAssetManager()->TakeResolvers(); - ASSERT_EQ(resolvers.size(), 2ull); - ASSERT_FALSE(resolvers[0]->IsUpdatable()); - ASSERT_TRUE(resolvers[0]->IsValidAfterAssetManagerChange()); - - ASSERT_TRUE(resolvers[1]->IsUpdatable()); - ASSERT_FALSE(resolvers[1]->IsValidAfterAssetManagerChange()); - - DestroyShell(std::move(shell), std::move(task_runners)); - ASSERT_FALSE(DartVMRef::IsInstanceRunning()); -} - -TEST_F(ShellTest, UpdateAssetResolversRemoves) { - ASSERT_FALSE(DartVMRef::IsInstanceRunning()); - Settings settings = CreateSettingsForFixture(); - ThreadHost thread_host("io.flutter.test." + GetCurrentTestName() + ".", - ThreadHost::Type::Platform); - auto task_runner = thread_host.platform_thread->GetTaskRunner(); - TaskRunners task_runners("test", task_runner, task_runner, task_runner, - task_runner); - auto shell = CreateShell(std::move(settings), task_runners); - ASSERT_TRUE(DartVMRef::IsInstanceRunning()); - ASSERT_TRUE(ValidateShell(shell.get())); - - auto configuration = RunConfiguration::InferFromSettings(settings); - configuration.SetEntrypoint("emptyMain"); - RunEngine(shell.get(), std::move(configuration)); - - auto platform_view = - std::make_unique(*shell.get(), std::move(task_runners)); - - auto asset_manager = shell->GetEngine()->GetAssetManager(); - - auto old_resolver = std::make_unique(true, true); - ASSERT_TRUE(old_resolver->IsUpdatable()); - ASSERT_TRUE(old_resolver->IsValid()); - asset_manager->PushBack(std::move(old_resolver)); - - std::vector> resolver_vector; - platform_view->UpdateAssetResolvers(resolver_vector); - - auto resolvers = shell->GetEngine()->GetAssetManager()->TakeResolvers(); - ASSERT_EQ(resolvers.size(), 1ull); - ASSERT_FALSE(resolvers[0]->IsUpdatable()); - ASSERT_TRUE(resolvers[0]->IsValidAfterAssetManagerChange()); - - DestroyShell(std::move(shell), std::move(task_runners)); - ASSERT_FALSE(DartVMRef::IsInstanceRunning()); -} - TEST_F(ShellTest, InitializeWithInvalidThreads) { ASSERT_FALSE(DartVMRef::IsInstanceRunning()); Settings settings = CreateSettingsForFixture(); @@ -2588,5 +2465,126 @@ TEST_F(ShellTest, Spawn) { ASSERT_FALSE(DartVMRef::IsInstanceRunning()); } +TEST_F(ShellTest, UpdateAssetResolversReplaces) { + ASSERT_FALSE(DartVMRef::IsInstanceRunning()); + Settings settings = CreateSettingsForFixture(); + ThreadHost thread_host("io.flutter.test." + GetCurrentTestName() + ".", + ThreadHost::Type::Platform); + auto task_runner = thread_host.platform_thread->GetTaskRunner(); + TaskRunners task_runners("test", task_runner, task_runner, task_runner, + task_runner); + auto shell = CreateShell(std::move(settings), task_runners); + ASSERT_TRUE(DartVMRef::IsInstanceRunning()); + ASSERT_TRUE(ValidateShell(shell.get())); + + auto configuration = RunConfiguration::InferFromSettings(settings); + configuration.SetEntrypoint("emptyMain"); + RunEngine(shell.get(), std::move(configuration)); + + auto platform_view = + std::make_unique(*shell.get(), std::move(task_runners)); + + auto asset_manager = shell->GetEngine()->GetAssetManager(); + + auto old_resolver = std::make_unique(true, true); + ASSERT_TRUE(old_resolver->IsUpdatable()); + ASSERT_TRUE(old_resolver->IsValid()); + asset_manager->PushBack(std::move(old_resolver)); + + std::vector> resolver_vector; + auto updated_resolver = std::make_unique(true, false); + ASSERT_TRUE(updated_resolver->IsUpdatable()); + ASSERT_FALSE(updated_resolver->IsValidAfterAssetManagerChange()); + resolver_vector.push_back(std::move(updated_resolver)); + platform_view->UpdateAssetResolvers(resolver_vector); + + auto resolvers = shell->GetEngine()->GetAssetManager()->TakeResolvers(); + ASSERT_EQ(resolvers.size(), 2ull); + ASSERT_FALSE(resolvers[0]->IsUpdatable()); + ASSERT_TRUE(resolvers[0]->IsValidAfterAssetManagerChange()); + + ASSERT_TRUE(resolvers[1]->IsUpdatable()); + ASSERT_FALSE(resolvers[1]->IsValidAfterAssetManagerChange()); + + DestroyShell(std::move(shell), std::move(task_runners)); + ASSERT_FALSE(DartVMRef::IsInstanceRunning()); +} + +TEST_F(ShellTest, UpdateAssetResolversAppends) { + ASSERT_FALSE(DartVMRef::IsInstanceRunning()); + Settings settings = CreateSettingsForFixture(); + ThreadHost thread_host("io.flutter.test." + GetCurrentTestName() + ".", + ThreadHost::Type::Platform); + auto task_runner = thread_host.platform_thread->GetTaskRunner(); + TaskRunners task_runners("test", task_runner, task_runner, task_runner, + task_runner); + auto shell = CreateShell(std::move(settings), task_runners); + ASSERT_TRUE(DartVMRef::IsInstanceRunning()); + ASSERT_TRUE(ValidateShell(shell.get())); + + auto configuration = RunConfiguration::InferFromSettings(settings); + configuration.SetEntrypoint("emptyMain"); + RunEngine(shell.get(), std::move(configuration)); + + auto platform_view = + std::make_unique(*shell.get(), std::move(task_runners)); + + std::vector> resolver_vector; + auto updated_resolver = std::make_unique(true, false); + ASSERT_TRUE(updated_resolver->IsUpdatable()); + ASSERT_FALSE(updated_resolver->IsValidAfterAssetManagerChange()); + resolver_vector.push_back(std::move(updated_resolver)); + platform_view->UpdateAssetResolvers(resolver_vector); + + auto resolvers = shell->GetEngine()->GetAssetManager()->TakeResolvers(); + ASSERT_EQ(resolvers.size(), 2ull); + ASSERT_FALSE(resolvers[0]->IsUpdatable()); + ASSERT_TRUE(resolvers[0]->IsValidAfterAssetManagerChange()); + + ASSERT_TRUE(resolvers[1]->IsUpdatable()); + ASSERT_FALSE(resolvers[1]->IsValidAfterAssetManagerChange()); + + DestroyShell(std::move(shell), std::move(task_runners)); + ASSERT_FALSE(DartVMRef::IsInstanceRunning()); +} + +TEST_F(ShellTest, UpdateAssetResolversRemoves) { + ASSERT_FALSE(DartVMRef::IsInstanceRunning()); + Settings settings = CreateSettingsForFixture(); + ThreadHost thread_host("io.flutter.test." + GetCurrentTestName() + ".", + ThreadHost::Type::Platform); + auto task_runner = thread_host.platform_thread->GetTaskRunner(); + TaskRunners task_runners("test", task_runner, task_runner, task_runner, + task_runner); + auto shell = CreateShell(std::move(settings), task_runners); + ASSERT_TRUE(DartVMRef::IsInstanceRunning()); + ASSERT_TRUE(ValidateShell(shell.get())); + + auto configuration = RunConfiguration::InferFromSettings(settings); + configuration.SetEntrypoint("emptyMain"); + RunEngine(shell.get(), std::move(configuration)); + + auto platform_view = + std::make_unique(*shell.get(), std::move(task_runners)); + + auto asset_manager = shell->GetEngine()->GetAssetManager(); + + auto old_resolver = std::make_unique(true, true); + ASSERT_TRUE(old_resolver->IsUpdatable()); + ASSERT_TRUE(old_resolver->IsValid()); + asset_manager->PushBack(std::move(old_resolver)); + + std::vector> resolver_vector; + platform_view->UpdateAssetResolvers(resolver_vector); + + auto resolvers = shell->GetEngine()->GetAssetManager()->TakeResolvers(); + ASSERT_EQ(resolvers.size(), 1ull); + ASSERT_FALSE(resolvers[0]->IsUpdatable()); + ASSERT_TRUE(resolvers[0]->IsValidAfterAssetManagerChange()); + + DestroyShell(std::move(shell), std::move(task_runners)); + ASSERT_FALSE(DartVMRef::IsInstanceRunning()); +} + } // namespace testing } // namespace flutter diff --git a/shell/platform/android/test/io/flutter/embedding/engine/dynamicfeatures/PlayStoreDynamicFeatureManagerTest.java b/shell/platform/android/test/io/flutter/embedding/engine/dynamicfeatures/PlayStoreDynamicFeatureManagerTest.java index d6c7bea3c160f..a6d11abfeae44 100644 --- a/shell/platform/android/test/io/flutter/embedding/engine/dynamicfeatures/PlayStoreDynamicFeatureManagerTest.java +++ b/shell/platform/android/test/io/flutter/embedding/engine/dynamicfeatures/PlayStoreDynamicFeatureManagerTest.java @@ -44,7 +44,7 @@ public void loadDartDeferredLibrary(int loadingUnitId, @NonNull String[] searchP } @Override - public void updateAssetManager( + public void updateJavaAssetManager( @NonNull AssetManager assetManager, @NonNull String assetBundlePath) { updateAssetManagerCalled++; this.loadingUnitId = loadingUnitId; From 807f9d504a4b5d9fce25d0924862eed0c0bd7187 Mon Sep 17 00:00:00 2001 From: garyqian Date: Tue, 22 Dec 2020 10:28:47 -0800 Subject: [PATCH 07/18] Switch to enum system --- assets/asset_manager.cc | 27 ++++++ assets/asset_manager.h | 7 ++ assets/asset_resolver.h | 8 ++ assets/directory_asset_bundle.cc | 5 ++ assets/directory_asset_bundle.h | 3 + shell/common/platform_view.cc | 5 +- shell/common/platform_view.h | 6 +- shell/common/shell.cc | 24 +---- shell/common/shell.h | 3 +- shell/common/shell_unittests.cc | 87 +++++++++++++++---- shell/platform/android/apk_asset_provider.cc | 5 ++ shell/platform/android/apk_asset_provider.h | 3 + .../platform/android/platform_view_android.cc | 5 +- .../platform/android/platform_view_android.h | 3 +- .../android/platform_view_android_jni_impl.cc | 2 +- 15 files changed, 147 insertions(+), 46 deletions(-) diff --git a/assets/asset_manager.cc b/assets/asset_manager.cc index 534e6c87bbb88..6cdeb71def7bb 100644 --- a/assets/asset_manager.cc +++ b/assets/asset_manager.cc @@ -29,6 +29,28 @@ void AssetManager::PushBack(std::unique_ptr resolver) { resolvers_.push_back(std::move(resolver)); } +void AssetManager::UpdateResolversByType( + std::vector>& updated_asset_resolvers, + AssetResolver::AssetResolverType type) { + size_t index = 0; + std::deque> new_resolvers; + for (auto& old_resolver : resolvers_) { + if (old_resolver->GetType() == type) { + if (index < updated_asset_resolvers.size()) { + // Push the replacement updated resolver in place of the old_resolver. + new_resolvers.push_back(std::move(updated_asset_resolvers[index++])); + } // Drop the resolver if no replacement available. + } else { + new_resolvers.push_back(std::move(old_resolver)); + } + } + // Append all extra resolvers to the end. + while (index < updated_asset_resolvers.size()) { + new_resolvers.push_back(std::move(updated_asset_resolvers[index++])); + } + resolvers_.swap(new_resolvers); +} + std::deque> AssetManager::TakeResolvers() { return std::move(resolvers_); } @@ -84,4 +106,9 @@ bool AssetManager::IsUpdatable() const { return false; } +// |AssetResolver| +AssetResolver::AssetResolverType AssetManager::GetType() const { + return AssetResolverType::kAssetManager; +} + } // namespace flutter diff --git a/assets/asset_manager.h b/assets/asset_manager.h index 4bca77debd226..63722a48a24c3 100644 --- a/assets/asset_manager.h +++ b/assets/asset_manager.h @@ -25,6 +25,10 @@ class AssetManager final : public AssetResolver { void PushBack(std::unique_ptr resolver); + void UpdateResolversByType( + std::vector>& updated_asset_resolvers, + AssetResolver::AssetResolverType type); + std::deque> TakeResolvers(); // |AssetResolver| @@ -36,6 +40,9 @@ class AssetManager final : public AssetResolver { // |AssetResolver| bool IsUpdatable() const override; + // |AssetResolver| + AssetResolver::AssetResolverType GetType() const override; + // |AssetResolver| std::unique_ptr GetAsMapping( const std::string& asset_name) const override; diff --git a/assets/asset_resolver.h b/assets/asset_resolver.h index 62312ca71c505..5a7105e736a27 100644 --- a/assets/asset_resolver.h +++ b/assets/asset_resolver.h @@ -19,6 +19,12 @@ class AssetResolver { virtual ~AssetResolver() = default; + enum AssetResolverType { + kAssetManager, + kApkAssetProvider, + kDirectoryAssetBundle + }; + virtual bool IsValid() const = 0; //---------------------------------------------------------------------------- @@ -57,6 +63,8 @@ class AssetResolver { /// virtual bool IsUpdatable() const = 0; + virtual AssetResolverType GetType() const = 0; + [[nodiscard]] virtual std::unique_ptr GetAsMapping( const std::string& asset_name) const = 0; diff --git a/assets/directory_asset_bundle.cc b/assets/directory_asset_bundle.cc index a94cfeb09baa5..807ad51a5fe39 100644 --- a/assets/directory_asset_bundle.cc +++ b/assets/directory_asset_bundle.cc @@ -41,6 +41,11 @@ bool DirectoryAssetBundle::IsUpdatable() const { return false; } +// |AssetResolver| +AssetResolver::AssetResolverType DirectoryAssetBundle::GetType() const { + return AssetResolver::AssetResolverType::kDirectoryAssetBundle; +} + // |AssetResolver| std::unique_ptr DirectoryAssetBundle::GetAsMapping( const std::string& asset_name) const { diff --git a/assets/directory_asset_bundle.h b/assets/directory_asset_bundle.h index 668d2e8490a06..59713d8a747cf 100644 --- a/assets/directory_asset_bundle.h +++ b/assets/directory_asset_bundle.h @@ -33,6 +33,9 @@ class DirectoryAssetBundle : public AssetResolver { // |AssetResolver| bool IsUpdatable() const override; + // |AssetResolver| + AssetResolver::AssetResolverType GetType() const override; + // |AssetResolver| std::unique_ptr GetAsMapping( const std::string& asset_name) const override; diff --git a/shell/common/platform_view.cc b/shell/common/platform_view.cc index e450559ba7dc6..6ea3eef27bad8 100644 --- a/shell/common/platform_view.cc +++ b/shell/common/platform_view.cc @@ -171,8 +171,9 @@ void PlatformView::LoadDartDeferredLibraryError(intptr_t loading_unit_id, bool transient) {} void PlatformView::UpdateAssetResolvers( - std::vector>& asset_resolvers) { - delegate_.UpdateAssetResolvers(asset_resolvers); + std::vector>& asset_resolvers, + AssetResolver::AssetResolverType type) { + delegate_.UpdateAssetResolvers(asset_resolvers, type); } } // namespace flutter diff --git a/shell/common/platform_view.h b/shell/common/platform_view.h index 87450a24b9847..e1a9d936f2c7c 100644 --- a/shell/common/platform_view.h +++ b/shell/common/platform_view.h @@ -281,7 +281,8 @@ class PlatformView { /// @param[in] asset_manager The asset manager to use. /// virtual void UpdateAssetResolvers( - std::vector>& asset_resolvers) = 0; + std::vector>& asset_resolvers, + AssetResolver::AssetResolverType type) = 0; }; //---------------------------------------------------------------------------- @@ -744,7 +745,8 @@ class PlatformView { /// existing resolvers with. /// virtual void UpdateAssetResolvers( - std::vector>& asset_resolvers); + std::vector>& asset_resolvers, + AssetResolver::AssetResolverType type); protected: PlatformView::Delegate& delegate_; diff --git a/shell/common/shell.cc b/shell/common/shell.cc index 7e422c7253298..80b9a98b46d36 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -1224,27 +1224,9 @@ void Shell::LoadDartDeferredLibraryError(intptr_t loading_unit_id, } void Shell::UpdateAssetResolvers( - std::vector>& asset_resolvers) { - size_t index = 0; - auto asset_manager = std::make_shared(); - auto old_asset_manager = engine_->GetAssetManager(); - if (old_asset_manager != nullptr) { - for (auto& old_resolver : old_asset_manager->TakeResolvers()) { - 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++])); - } // Drop the resolver if no replacement available. - } else { - asset_manager->PushBack(std::move(old_resolver)); - } - } - } - // Append all extra resolvers to the end. - while (index < asset_resolvers.size()) { - asset_manager->PushBack(std::move(asset_resolvers[index++])); - } - engine_->UpdateAssetManager(std::move(asset_manager)); + std::vector>& asset_resolvers, + AssetResolver::AssetResolverType type) { + engine_->GetAssetManager()->UpdateResolversByType(asset_resolvers, type); } // |Engine::Delegate| diff --git a/shell/common/shell.h b/shell/common/shell.h index 6414688f99461..c056b436b6e12 100644 --- a/shell/common/shell.h +++ b/shell/common/shell.h @@ -537,7 +537,8 @@ class Shell final : public PlatformView::Delegate, // |PlatformView::Delegate| void UpdateAssetResolvers( - std::vector>& asset_resolvers) override; + std::vector>& asset_resolvers, + AssetResolver::AssetResolverType type) override; // |Animator::Delegate| void OnAnimatorBeginFrame(fml::TimePoint frame_target_time) override; diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index 991e3ac549c91..b3fecc7693b15 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -89,8 +89,10 @@ class MockPlatformViewDelegate : public PlatformView::Delegate { const std::string error_message, bool transient)); - MOCK_METHOD1(UpdateAssetManager, - void(std::shared_ptr asset_manager)); + MOCK_METHOD2( + UpdateAssetResolvers, + void(std::vector>& updated_asset_resolvers, + AssetResolver::AssetResolverType type)); }; class MockSurface : public Surface { @@ -118,15 +120,17 @@ class MockPlatformView : public PlatformView { class TestAssetResolver : public AssetResolver { public: - TestAssetResolver(bool updatable, int valid) - : updatable_(updatable), valid_(valid) {} + TestAssetResolver(bool valid, AssetResolver::AssetResolverType type) + : valid_(valid), type_(type) {} bool IsValid() const override { return true; } // This is used to identify if replacement was made or not. bool IsValidAfterAssetManagerChange() const override { return valid_; } - bool IsUpdatable() const override { return updatable_; } + bool IsUpdatable() const override { return true; } + + AssetResolver::AssetResolverType GetType() const override { return type_; } std::unique_ptr GetAsMapping( const std::string& asset_name) const override { @@ -139,8 +143,8 @@ class TestAssetResolver : public AssetResolver { }; private: - bool updatable_; - int valid_; + bool valid_; + AssetResolver::AssetResolverType type_; }; static bool ValidateShell(Shell* shell) { @@ -2486,17 +2490,20 @@ TEST_F(ShellTest, UpdateAssetResolversReplaces) { auto asset_manager = shell->GetEngine()->GetAssetManager(); - auto old_resolver = std::make_unique(true, true); + auto old_resolver = std::make_unique( + true, AssetResolver::AssetResolverType::kApkAssetProvider); ASSERT_TRUE(old_resolver->IsUpdatable()); ASSERT_TRUE(old_resolver->IsValid()); asset_manager->PushBack(std::move(old_resolver)); std::vector> resolver_vector; - auto updated_resolver = std::make_unique(true, false); + auto updated_resolver = std::make_unique( + false, AssetResolver::AssetResolverType::kApkAssetProvider); ASSERT_TRUE(updated_resolver->IsUpdatable()); ASSERT_FALSE(updated_resolver->IsValidAfterAssetManagerChange()); resolver_vector.push_back(std::move(updated_resolver)); - platform_view->UpdateAssetResolvers(resolver_vector); + platform_view->UpdateAssetResolvers( + resolver_vector, AssetResolver::AssetResolverType::kApkAssetProvider); auto resolvers = shell->GetEngine()->GetAssetManager()->TakeResolvers(); ASSERT_EQ(resolvers.size(), 2ull); @@ -2530,11 +2537,13 @@ TEST_F(ShellTest, UpdateAssetResolversAppends) { std::make_unique(*shell.get(), std::move(task_runners)); std::vector> resolver_vector; - auto updated_resolver = std::make_unique(true, false); + auto updated_resolver = std::make_unique( + false, AssetResolver::AssetResolverType::kApkAssetProvider); ASSERT_TRUE(updated_resolver->IsUpdatable()); ASSERT_FALSE(updated_resolver->IsValidAfterAssetManagerChange()); resolver_vector.push_back(std::move(updated_resolver)); - platform_view->UpdateAssetResolvers(resolver_vector); + platform_view->UpdateAssetResolvers( + resolver_vector, AssetResolver::AssetResolverType::kApkAssetProvider); auto resolvers = shell->GetEngine()->GetAssetManager()->TakeResolvers(); ASSERT_EQ(resolvers.size(), 2ull); @@ -2569,22 +2578,68 @@ TEST_F(ShellTest, UpdateAssetResolversRemoves) { auto asset_manager = shell->GetEngine()->GetAssetManager(); - auto old_resolver = std::make_unique(true, true); - ASSERT_TRUE(old_resolver->IsUpdatable()); + auto old_resolver = std::make_unique( + true, AssetResolver::AssetResolverType::kApkAssetProvider); ASSERT_TRUE(old_resolver->IsValid()); asset_manager->PushBack(std::move(old_resolver)); std::vector> resolver_vector; - platform_view->UpdateAssetResolvers(resolver_vector); + platform_view->UpdateAssetResolvers( + resolver_vector, AssetResolver::AssetResolverType::kApkAssetProvider); auto resolvers = shell->GetEngine()->GetAssetManager()->TakeResolvers(); ASSERT_EQ(resolvers.size(), 1ull); - ASSERT_FALSE(resolvers[0]->IsUpdatable()); ASSERT_TRUE(resolvers[0]->IsValidAfterAssetManagerChange()); DestroyShell(std::move(shell), std::move(task_runners)); ASSERT_FALSE(DartVMRef::IsInstanceRunning()); } +TEST_F(ShellTest, UpdateAssetResolversDoesNotReplaceMismatchType) { + ASSERT_FALSE(DartVMRef::IsInstanceRunning()); + Settings settings = CreateSettingsForFixture(); + ThreadHost thread_host("io.flutter.test." + GetCurrentTestName() + ".", + ThreadHost::Type::Platform); + auto task_runner = thread_host.platform_thread->GetTaskRunner(); + TaskRunners task_runners("test", task_runner, task_runner, task_runner, + task_runner); + auto shell = CreateShell(std::move(settings), task_runners); + ASSERT_TRUE(DartVMRef::IsInstanceRunning()); + ASSERT_TRUE(ValidateShell(shell.get())); + + auto configuration = RunConfiguration::InferFromSettings(settings); + configuration.SetEntrypoint("emptyMain"); + RunEngine(shell.get(), std::move(configuration)); + + auto platform_view = + std::make_unique(*shell.get(), std::move(task_runners)); + + auto asset_manager = shell->GetEngine()->GetAssetManager(); + + auto old_resolver = std::make_unique( + true, AssetResolver::AssetResolverType::kAssetManager); + ASSERT_TRUE(old_resolver->IsValid()); + asset_manager->PushBack(std::move(old_resolver)); + + std::vector> resolver_vector; + auto updated_resolver = std::make_unique( + false, AssetResolver::AssetResolverType::kApkAssetProvider); + ASSERT_FALSE(updated_resolver->IsValidAfterAssetManagerChange()); + resolver_vector.push_back(std::move(updated_resolver)); + platform_view->UpdateAssetResolvers( + resolver_vector, AssetResolver::AssetResolverType::kApkAssetProvider); + + auto resolvers = shell->GetEngine()->GetAssetManager()->TakeResolvers(); + ASSERT_EQ(resolvers.size(), 3ull); + ASSERT_TRUE(resolvers[0]->IsValidAfterAssetManagerChange()); + + ASSERT_TRUE(resolvers[1]->IsValidAfterAssetManagerChange()); + + ASSERT_FALSE(resolvers[2]->IsValidAfterAssetManagerChange()); + + DestroyShell(std::move(shell), std::move(task_runners)); + ASSERT_FALSE(DartVMRef::IsInstanceRunning()); +} + } // namespace testing } // namespace flutter diff --git a/shell/platform/android/apk_asset_provider.cc b/shell/platform/android/apk_asset_provider.cc index 717df0557d0f5..c1287d52821ce 100644 --- a/shell/platform/android/apk_asset_provider.cc +++ b/shell/platform/android/apk_asset_provider.cc @@ -38,6 +38,11 @@ bool APKAssetProvider::IsUpdatable() const { return true; } +// |AssetResolver| +AssetResolver::AssetResolverType APKAssetProvider::GetType() const { + return AssetResolver::AssetResolverType::kAssetManager; +} + class APKAssetMapping : public fml::Mapping { public: APKAssetMapping(AAsset* asset) : asset_(asset) {} diff --git a/shell/platform/android/apk_asset_provider.h b/shell/platform/android/apk_asset_provider.h index 5bd0d8a2e5f1f..3d76f399ec749 100644 --- a/shell/platform/android/apk_asset_provider.h +++ b/shell/platform/android/apk_asset_provider.h @@ -35,6 +35,9 @@ class APKAssetProvider final : public AssetResolver { // |flutter::AssetResolver| bool IsUpdatable() const override; + // |AssetResolver| + AssetResolver::AssetResolverType GetType() const override; + // |flutter::AssetResolver| std::unique_ptr GetAsMapping( const std::string& asset_name) const override; diff --git a/shell/platform/android/platform_view_android.cc b/shell/platform/android/platform_view_android.cc index b7e3241fe0dbe..f62b31176b88c 100644 --- a/shell/platform/android/platform_view_android.cc +++ b/shell/platform/android/platform_view_android.cc @@ -364,8 +364,9 @@ void PlatformViewAndroid::LoadDartDeferredLibraryError( // |PlatformView| void PlatformViewAndroid::UpdateAssetResolvers( - std::vector>& asset_resolvers) { - delegate_.UpdateAssetResolvers(asset_resolvers); + std::vector>& asset_resolvers, + AssetResolver::AssetResolverType type) { + delegate_.UpdateAssetResolvers(asset_resolvers, type); } void PlatformViewAndroid::InstallFirstFrameCallback() { diff --git a/shell/platform/android/platform_view_android.h b/shell/platform/android/platform_view_android.h index 87cab1cf2794f..de97c31abb9c3 100644 --- a/shell/platform/android/platform_view_android.h +++ b/shell/platform/android/platform_view_android.h @@ -105,7 +105,8 @@ class PlatformViewAndroid final : public PlatformView { // |PlatformView| void UpdateAssetResolvers( - std::vector>& asset_resolvers) override; + std::vector>& asset_resolvers, + AssetResolver::AssetResolverType type) override; private: const std::shared_ptr jni_facade_; diff --git a/shell/platform/android/platform_view_android_jni_impl.cc b/shell/platform/android/platform_view_android_jni_impl.cc index 053d5ba993060..698d9c404cc85 100644 --- a/shell/platform/android/platform_view_android_jni_impl.cc +++ b/shell/platform/android/platform_view_android_jni_impl.cc @@ -584,7 +584,7 @@ static void UpdateJavaAssetManager(JNIEnv* env, resolver_vector.push_back(std::move(asset_resolver)); ANDROID_SHELL_HOLDER->GetPlatformView()->UpdateAssetResolvers( - resolver_vector); + resolver_vector, AssetResolver::AssetResolverType::kApkAssetProvider); } bool RegisterApi(JNIEnv* env) { From 7495aaebd267f4429940bdb4a81d06d501e99be9 Mon Sep 17 00:00:00 2001 From: garyqian Date: Tue, 22 Dec 2020 10:38:39 -0800 Subject: [PATCH 08/18] Docs and cleanup --- assets/asset_manager.cc | 5 --- assets/asset_manager.h | 30 +++++++++++++++-- assets/asset_resolver.h | 18 ++--------- assets/directory_asset_bundle.cc | 5 --- assets/directory_asset_bundle.h | 3 -- shell/common/platform_view.h | 34 +++++++++++++++++--- shell/platform/android/apk_asset_provider.cc | 9 +----- shell/platform/android/apk_asset_provider.h | 3 -- 8 files changed, 60 insertions(+), 47 deletions(-) diff --git a/assets/asset_manager.cc b/assets/asset_manager.cc index 6cdeb71def7bb..7d655b7ce382f 100644 --- a/assets/asset_manager.cc +++ b/assets/asset_manager.cc @@ -101,11 +101,6 @@ bool AssetManager::IsValidAfterAssetManagerChange() const { return false; } -// |AssetResolver| -bool AssetManager::IsUpdatable() const { - return false; -} - // |AssetResolver| AssetResolver::AssetResolverType AssetManager::GetType() const { return AssetResolverType::kAssetManager; diff --git a/assets/asset_manager.h b/assets/asset_manager.h index 63722a48a24c3..7be2d6d2bc53a 100644 --- a/assets/asset_manager.h +++ b/assets/asset_manager.h @@ -25,6 +25,33 @@ class AssetManager final : public AssetResolver { void PushBack(std::unique_ptr resolver); + //-------------------------------------------------------------------------- + /// @brief Replaces asset resolvers handled by this AssetManager that are + /// of the specified `type` with the resolvers provided in + /// `updated_asset_resolvers`. Updatable AssetResolvers + /// are removed and replaced with the next available resolver + /// in `updated_asset_resolvers`. + /// + /// AssetResolvers should be updated when the exisitng resolver + /// becomes obsolete and a newer one becomes available that + /// provides updated access to the same type of assets as the + /// existing one. This update process is meant to be performed at + /// runtime. + /// + /// If less resolvers are provided than existing resolvers of + /// matching type, the the extra existing resolvers will + /// be removed without replacement. If more resolvers are provided + /// than existing matching resolvers, then the extra provided + /// resolvers will be added to the end of the AssetManager + /// resolvers queue. + /// + /// @param[in] asset_resolvers The asset resolvers to replace updatable + /// existing resolvers with. + /// + /// @param[in] type The type of AssetResolver to update. Only resolvers of + /// the specified type will be replaced by an updated + /// resolver. + /// void UpdateResolversByType( std::vector>& updated_asset_resolvers, AssetResolver::AssetResolverType type); @@ -37,9 +64,6 @@ class AssetManager final : public AssetResolver { // |AssetResolver| bool IsValidAfterAssetManagerChange() const override; - // |AssetResolver| - bool IsUpdatable() const override; - // |AssetResolver| AssetResolver::AssetResolverType GetType() const override; diff --git a/assets/asset_resolver.h b/assets/asset_resolver.h index 5a7105e736a27..611ea22e956f7 100644 --- a/assets/asset_resolver.h +++ b/assets/asset_resolver.h @@ -46,23 +46,11 @@ class AssetResolver { virtual bool IsValidAfterAssetManagerChange() const = 0; //---------------------------------------------------------------------------- - /// @brief Some asset resolvers may be replaced by an updated version - /// during runtime. Resolvers marked `Updatable` are removed and - /// invalidated when an update is processed and is replaced by a - /// new resolver that provides the latest availablity state of - /// assets. This usually adds access to new assets or removes - /// access to old/invalid/deleted assets. For example, when - /// downloading a dynamic feature, Android provides a new java - /// asset manager that has access to the newly installed assets. - /// This new manager should replace the existing java asset - /// manager resolver. We call this replacement an update as the - /// old resolver is obsolete and the new one should assume - /// responsibility for providing access to android assets. + /// @brief Gets the type of AssetResolver this is. Types are defined in + /// AssetResolverType. /// - /// @return Returns whether this resolver can be updated. + /// @return Returns the AssetResolverType that this resolver is. /// - virtual bool IsUpdatable() const = 0; - virtual AssetResolverType GetType() const = 0; [[nodiscard]] virtual std::unique_ptr GetAsMapping( diff --git a/assets/directory_asset_bundle.cc b/assets/directory_asset_bundle.cc index 807ad51a5fe39..45ab571823902 100644 --- a/assets/directory_asset_bundle.cc +++ b/assets/directory_asset_bundle.cc @@ -36,11 +36,6 @@ bool DirectoryAssetBundle::IsValidAfterAssetManagerChange() const { return is_valid_after_asset_manager_change_; } -// |AssetResolver| -bool DirectoryAssetBundle::IsUpdatable() const { - return false; -} - // |AssetResolver| AssetResolver::AssetResolverType DirectoryAssetBundle::GetType() const { return AssetResolver::AssetResolverType::kDirectoryAssetBundle; diff --git a/assets/directory_asset_bundle.h b/assets/directory_asset_bundle.h index 59713d8a747cf..b004201e0ad96 100644 --- a/assets/directory_asset_bundle.h +++ b/assets/directory_asset_bundle.h @@ -30,9 +30,6 @@ class DirectoryAssetBundle : public AssetResolver { // |AssetResolver| bool IsValidAfterAssetManagerChange() const override; - // |AssetResolver| - bool IsUpdatable() const override; - // |AssetResolver| AssetResolver::AssetResolverType GetType() const override; diff --git a/shell/common/platform_view.h b/shell/common/platform_view.h index e1a9d936f2c7c..aff66000854a8 100644 --- a/shell/common/platform_view.h +++ b/shell/common/platform_view.h @@ -273,12 +273,32 @@ class PlatformView { const std::string error_message, bool transient) = 0; - // TODO(garyq): Implement a proper asset_resolver replacement instead of - // overwriting the entire asset manager. //-------------------------------------------------------------------------- - /// @brief Sets the asset manager of the engine to asset_manager - /// - /// @param[in] asset_manager The asset manager to use. + /// @brief Replaces asset resolvers handled by the engine's + /// AssetManager that are of the specified `type` with the + /// resolvers provided in `updated_asset_resolvers`. Updatable + /// AssetResolvers are removed and replaced with the next + /// available resolver in `updated_asset_resolvers`. + /// + /// AssetResolvers should be updated when the exisitng resolver + /// becomes obsolete and a newer one becomes available that + /// provides updated access to the same type of assets as the + /// existing one. This update process is meant to be performed + /// at runtime. + /// + /// If less resolvers are provided than existing resolvers of + /// matching type, the the extra existing resolvers will + /// be removed without replacement. If more resolvers are + /// provided than existing matching resolvers, then the extra + /// provided resolvers will be added to the end of the + /// AssetManager resolvers queue. + /// + /// @param[in] asset_resolvers The asset resolvers to replace updatable + /// existing resolvers with. + /// + /// @param[in] type The type of AssetResolver to update. Only resolvers of + /// the specified type will be replaced by an updated + /// resolver. /// virtual void UpdateAssetResolvers( std::vector>& asset_resolvers, @@ -744,6 +764,10 @@ class PlatformView { /// @param[in] asset_resolvers The asset resolvers to replace updatable /// existing resolvers with. /// + /// @param[in] type The type of AssetResolver to update. Only resolvers of + /// the specified type will be replaced by an updated + /// resolver. + /// virtual void UpdateAssetResolvers( std::vector>& asset_resolvers, AssetResolver::AssetResolverType type); diff --git a/shell/platform/android/apk_asset_provider.cc b/shell/platform/android/apk_asset_provider.cc index c1287d52821ce..178227e9b156d 100644 --- a/shell/platform/android/apk_asset_provider.cc +++ b/shell/platform/android/apk_asset_provider.cc @@ -31,16 +31,9 @@ bool APKAssetProvider::IsValidAfterAssetManagerChange() const { return true; } -bool APKAssetProvider::IsUpdatable() const { - // APKAssetProvider is always updatable to allow dynamic features to - // runtime-update the Java AssetManager instance being used when a new dynamic - // feature is installed. - return true; -} - // |AssetResolver| AssetResolver::AssetResolverType APKAssetProvider::GetType() const { - return AssetResolver::AssetResolverType::kAssetManager; + return AssetResolver::AssetResolverType::kApkAssetProvider; } class APKAssetMapping : public fml::Mapping { diff --git a/shell/platform/android/apk_asset_provider.h b/shell/platform/android/apk_asset_provider.h index 3d76f399ec749..d1e7e2f707aab 100644 --- a/shell/platform/android/apk_asset_provider.h +++ b/shell/platform/android/apk_asset_provider.h @@ -32,9 +32,6 @@ class APKAssetProvider final : public AssetResolver { // |flutter::AssetResolver| bool IsValidAfterAssetManagerChange() const override; - // |flutter::AssetResolver| - bool IsUpdatable() const override; - // |AssetResolver| AssetResolver::AssetResolverType GetType() const override; From 9db7506835bfb2630300c56b63306ab9b1c3088f Mon Sep 17 00:00:00 2001 From: garyqian Date: Tue, 22 Dec 2020 10:40:51 -0800 Subject: [PATCH 09/18] Remove dev method --- assets/asset_manager.h | 2 -- assets/asset_resolver.h | 3 +++ 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/assets/asset_manager.h b/assets/asset_manager.h index 7be2d6d2bc53a..743f337ffb546 100644 --- a/assets/asset_manager.h +++ b/assets/asset_manager.h @@ -75,8 +75,6 @@ class AssetManager final : public AssetResolver { std::vector> GetAsMappings( const std::string& asset_pattern) const override; - size_t Size() { return resolvers_.size(); } - private: std::deque> resolvers_; diff --git a/assets/asset_resolver.h b/assets/asset_resolver.h index 611ea22e956f7..0674e71121b23 100644 --- a/assets/asset_resolver.h +++ b/assets/asset_resolver.h @@ -19,6 +19,9 @@ class AssetResolver { virtual ~AssetResolver() = default; + //---------------------------------------------------------------------------- + /// @brief Identifies the type of AssetResolver an instance is. + /// enum AssetResolverType { kAssetManager, kApkAssetProvider, From 4eab4541c7ca866280ae66bae4c5f653a9b34039 Mon Sep 17 00:00:00 2001 From: garyqian Date: Tue, 22 Dec 2020 10:42:42 -0800 Subject: [PATCH 10/18] Fix docs --- shell/common/platform_view.h | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/shell/common/platform_view.h b/shell/common/platform_view.h index aff66000854a8..9dce20e493438 100644 --- a/shell/common/platform_view.h +++ b/shell/common/platform_view.h @@ -742,24 +742,24 @@ class PlatformView { bool transient); //-------------------------------------------------------------------------- - /// @brief Replaces asset resolvers in the current engine's - /// `AssetManager` that are marked as updatable - /// (`IsUpdateable()` returns true). Updatable AssetResolvers - /// are removed and replaced with the next available resolver - /// in `asset_resolvers`. + /// @brief Replaces asset resolvers handled by the engine's + /// AssetManager that are of the specified `type` with the + /// resolvers provided in `updated_asset_resolvers`. Updatable + /// AssetResolvers are removed and replaced with the next + /// available resolver in `updated_asset_resolvers`. /// /// AssetResolvers should be updated when the exisitng resolver /// becomes obsolete and a newer one becomes available that /// provides updated access to the same type of assets as the - /// existing one. This update process is meant to be performed at - /// runtime. - /// - /// If less resolvers are provided than existing updatable - /// resolvers, the the extra existing updatable resolvers will be - /// removed without replacement. If more resolvers are provided - /// than existing updatable resolvers, then the extra provided - /// resolvers will be added to the end of the AssetManager - /// resolvers list. + /// existing one. This update process is meant to be performed + /// at runtime. + /// + /// If less resolvers are provided than existing resolvers of + /// matching type, the the extra existing resolvers will + /// be removed without replacement. If more resolvers are + /// provided than existing matching resolvers, then the extra + /// provided resolvers will be added to the end of the + /// AssetManager resolvers queue. /// /// @param[in] asset_resolvers The asset resolvers to replace updatable /// existing resolvers with. From d3fc041a9ab21f80f3a1c8d0f8312cf169a21a1b Mon Sep 17 00:00:00 2001 From: garyqian Date: Tue, 22 Dec 2020 11:10:12 -0800 Subject: [PATCH 11/18] Fix tests --- shell/common/shell_unittests.cc | 9 --------- 1 file changed, 9 deletions(-) diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index b3fecc7693b15..d61b527ba34aa 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -128,8 +128,6 @@ class TestAssetResolver : public AssetResolver { // This is used to identify if replacement was made or not. bool IsValidAfterAssetManagerChange() const override { return valid_; } - bool IsUpdatable() const override { return true; } - AssetResolver::AssetResolverType GetType() const override { return type_; } std::unique_ptr GetAsMapping( @@ -2492,14 +2490,12 @@ TEST_F(ShellTest, UpdateAssetResolversReplaces) { auto old_resolver = std::make_unique( true, AssetResolver::AssetResolverType::kApkAssetProvider); - ASSERT_TRUE(old_resolver->IsUpdatable()); ASSERT_TRUE(old_resolver->IsValid()); asset_manager->PushBack(std::move(old_resolver)); std::vector> resolver_vector; auto updated_resolver = std::make_unique( false, AssetResolver::AssetResolverType::kApkAssetProvider); - ASSERT_TRUE(updated_resolver->IsUpdatable()); ASSERT_FALSE(updated_resolver->IsValidAfterAssetManagerChange()); resolver_vector.push_back(std::move(updated_resolver)); platform_view->UpdateAssetResolvers( @@ -2507,10 +2503,8 @@ TEST_F(ShellTest, UpdateAssetResolversReplaces) { auto resolvers = shell->GetEngine()->GetAssetManager()->TakeResolvers(); ASSERT_EQ(resolvers.size(), 2ull); - ASSERT_FALSE(resolvers[0]->IsUpdatable()); ASSERT_TRUE(resolvers[0]->IsValidAfterAssetManagerChange()); - ASSERT_TRUE(resolvers[1]->IsUpdatable()); ASSERT_FALSE(resolvers[1]->IsValidAfterAssetManagerChange()); DestroyShell(std::move(shell), std::move(task_runners)); @@ -2539,7 +2533,6 @@ TEST_F(ShellTest, UpdateAssetResolversAppends) { std::vector> resolver_vector; auto updated_resolver = std::make_unique( false, AssetResolver::AssetResolverType::kApkAssetProvider); - ASSERT_TRUE(updated_resolver->IsUpdatable()); ASSERT_FALSE(updated_resolver->IsValidAfterAssetManagerChange()); resolver_vector.push_back(std::move(updated_resolver)); platform_view->UpdateAssetResolvers( @@ -2547,10 +2540,8 @@ TEST_F(ShellTest, UpdateAssetResolversAppends) { auto resolvers = shell->GetEngine()->GetAssetManager()->TakeResolvers(); ASSERT_EQ(resolvers.size(), 2ull); - ASSERT_FALSE(resolvers[0]->IsUpdatable()); ASSERT_TRUE(resolvers[0]->IsValidAfterAssetManagerChange()); - ASSERT_TRUE(resolvers[1]->IsUpdatable()); ASSERT_FALSE(resolvers[1]->IsValidAfterAssetManagerChange()); DestroyShell(std::move(shell), std::move(task_runners)); From 566f9322888aff5a31d1322fa4024a069cb9c948 Mon Sep 17 00:00:00 2001 From: garyqian Date: Tue, 22 Dec 2020 15:42:16 -0800 Subject: [PATCH 12/18] Fix test threading --- shell/common/shell_unittests.cc | 18 ++++++++---------- .../fuchsia/flutter/platform_view_unittest.cc | 5 +++-- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index d61b527ba34aa..44af7867eaafa 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -2481,13 +2481,12 @@ TEST_F(ShellTest, UpdateAssetResolversReplaces) { auto configuration = RunConfiguration::InferFromSettings(settings); configuration.SetEntrypoint("emptyMain"); + auto asset_manager = configuration.GetAssetManager(); RunEngine(shell.get(), std::move(configuration)); auto platform_view = std::make_unique(*shell.get(), std::move(task_runners)); - auto asset_manager = shell->GetEngine()->GetAssetManager(); - auto old_resolver = std::make_unique( true, AssetResolver::AssetResolverType::kApkAssetProvider); ASSERT_TRUE(old_resolver->IsValid()); @@ -2501,7 +2500,7 @@ TEST_F(ShellTest, UpdateAssetResolversReplaces) { platform_view->UpdateAssetResolvers( resolver_vector, AssetResolver::AssetResolverType::kApkAssetProvider); - auto resolvers = shell->GetEngine()->GetAssetManager()->TakeResolvers(); + auto resolvers = asset_manager->TakeResolvers(); ASSERT_EQ(resolvers.size(), 2ull); ASSERT_TRUE(resolvers[0]->IsValidAfterAssetManagerChange()); @@ -2525,6 +2524,7 @@ TEST_F(ShellTest, UpdateAssetResolversAppends) { auto configuration = RunConfiguration::InferFromSettings(settings); configuration.SetEntrypoint("emptyMain"); + auto asset_manager = configuration.GetAssetManager(); RunEngine(shell.get(), std::move(configuration)); auto platform_view = @@ -2538,7 +2538,7 @@ TEST_F(ShellTest, UpdateAssetResolversAppends) { platform_view->UpdateAssetResolvers( resolver_vector, AssetResolver::AssetResolverType::kApkAssetProvider); - auto resolvers = shell->GetEngine()->GetAssetManager()->TakeResolvers(); + auto resolvers = asset_manager->TakeResolvers(); ASSERT_EQ(resolvers.size(), 2ull); ASSERT_TRUE(resolvers[0]->IsValidAfterAssetManagerChange()); @@ -2562,13 +2562,12 @@ TEST_F(ShellTest, UpdateAssetResolversRemoves) { auto configuration = RunConfiguration::InferFromSettings(settings); configuration.SetEntrypoint("emptyMain"); + auto asset_manager = configuration.GetAssetManager(); RunEngine(shell.get(), std::move(configuration)); auto platform_view = std::make_unique(*shell.get(), std::move(task_runners)); - auto asset_manager = shell->GetEngine()->GetAssetManager(); - auto old_resolver = std::make_unique( true, AssetResolver::AssetResolverType::kApkAssetProvider); ASSERT_TRUE(old_resolver->IsValid()); @@ -2578,7 +2577,7 @@ TEST_F(ShellTest, UpdateAssetResolversRemoves) { platform_view->UpdateAssetResolvers( resolver_vector, AssetResolver::AssetResolverType::kApkAssetProvider); - auto resolvers = shell->GetEngine()->GetAssetManager()->TakeResolvers(); + auto resolvers = asset_manager->TakeResolvers(); ASSERT_EQ(resolvers.size(), 1ull); ASSERT_TRUE(resolvers[0]->IsValidAfterAssetManagerChange()); @@ -2600,13 +2599,12 @@ TEST_F(ShellTest, UpdateAssetResolversDoesNotReplaceMismatchType) { auto configuration = RunConfiguration::InferFromSettings(settings); configuration.SetEntrypoint("emptyMain"); + auto asset_manager = configuration.GetAssetManager(); RunEngine(shell.get(), std::move(configuration)); auto platform_view = std::make_unique(*shell.get(), std::move(task_runners)); - auto asset_manager = shell->GetEngine()->GetAssetManager(); - auto old_resolver = std::make_unique( true, AssetResolver::AssetResolverType::kAssetManager); ASSERT_TRUE(old_resolver->IsValid()); @@ -2620,7 +2618,7 @@ TEST_F(ShellTest, UpdateAssetResolversDoesNotReplaceMismatchType) { platform_view->UpdateAssetResolvers( resolver_vector, AssetResolver::AssetResolverType::kApkAssetProvider); - auto resolvers = shell->GetEngine()->GetAssetManager()->TakeResolvers(); + auto resolvers = asset_manager->TakeResolvers(); ASSERT_EQ(resolvers.size(), 3ull); ASSERT_TRUE(resolvers[0]->IsValidAfterAssetManagerChange()); diff --git a/shell/platform/fuchsia/flutter/platform_view_unittest.cc b/shell/platform/fuchsia/flutter/platform_view_unittest.cc index 3222450c04aa3..1b95fc341ae1a 100644 --- a/shell/platform/fuchsia/flutter/platform_view_unittest.cc +++ b/shell/platform/fuchsia/flutter/platform_view_unittest.cc @@ -114,8 +114,9 @@ class MockPlatformViewDelegate : public flutter::PlatformView::Delegate { const std::string error_message, bool transient) {} // |flutter::PlatformView::Delegate| - void UpdateAssetManager( - std::shared_ptr asset_manager) {} + void UpdateAssetResolvers( + std::vector>& asset_resolvers, + AssetResolver::AssetResolverType type) {} flutter::Surface* surface() const { return surface_.get(); } flutter::PlatformMessage* message() const { return message_.get(); } From 1a64e1e1197479f2195ef777dc79139e11b2b973 Mon Sep 17 00:00:00 2001 From: garyqian Date: Tue, 22 Dec 2020 16:32:58 -0800 Subject: [PATCH 13/18] Single updateed asset resolver --- assets/asset_manager.cc | 19 +++--- assets/asset_manager.h | 34 +++++----- shell/common/platform_view.cc | 6 +- shell/common/platform_view.h | 62 +++++++++---------- shell/common/shell.cc | 7 ++- shell/common/shell.h | 4 +- shell/common/shell_unittests.cc | 41 ++++++------ .../platform/android/platform_view_android.cc | 6 +- .../platform/android/platform_view_android.h | 4 +- .../android/platform_view_android_jni_impl.cc | 7 +-- .../fuchsia/flutter/platform_view_unittest.cc | 4 +- 11 files changed, 93 insertions(+), 101 deletions(-) diff --git a/assets/asset_manager.cc b/assets/asset_manager.cc index 7d655b7ce382f..697f0ff7e1684 100644 --- a/assets/asset_manager.cc +++ b/assets/asset_manager.cc @@ -29,24 +29,25 @@ void AssetManager::PushBack(std::unique_ptr resolver) { resolvers_.push_back(std::move(resolver)); } -void AssetManager::UpdateResolversByType( - std::vector>& updated_asset_resolvers, +void AssetManager::UpdateResolverByType( + std::unique_ptr updated_asset_resolver, AssetResolver::AssetResolverType type) { - size_t index = 0; + bool updated = false; std::deque> new_resolvers; for (auto& old_resolver : resolvers_) { - if (old_resolver->GetType() == type) { - if (index < updated_asset_resolvers.size()) { + if (!updated && old_resolver->GetType() == type) { + if (updated_asset_resolver != nullptr) { // Push the replacement updated resolver in place of the old_resolver. - new_resolvers.push_back(std::move(updated_asset_resolvers[index++])); + new_resolvers.push_back(std::move(updated_asset_resolver)); + updated = true; } // Drop the resolver if no replacement available. } else { new_resolvers.push_back(std::move(old_resolver)); } } - // Append all extra resolvers to the end. - while (index < updated_asset_resolvers.size()) { - new_resolvers.push_back(std::move(updated_asset_resolvers[index++])); + // Append resolver to the end if not used as a replacement. + if (!updated && updated_asset_resolver != nullptr) { + new_resolvers.push_back(std::move(updated_asset_resolver)); } resolvers_.swap(new_resolvers); } diff --git a/assets/asset_manager.h b/assets/asset_manager.h index 743f337ffb546..a3e38e6cd7c65 100644 --- a/assets/asset_manager.h +++ b/assets/asset_manager.h @@ -26,34 +26,32 @@ class AssetManager final : public AssetResolver { void PushBack(std::unique_ptr resolver); //-------------------------------------------------------------------------- - /// @brief Replaces asset resolvers handled by this AssetManager that are - /// of the specified `type` with the resolvers provided in - /// `updated_asset_resolvers`. Updatable AssetResolvers - /// are removed and replaced with the next available resolver - /// in `updated_asset_resolvers`. + /// @brief Replaces an asset resolver of the specified `type` with + /// `updated_asset_resolver`. The matching AssetResolver is + /// removed and replaced with `updated_asset_resolvers`. /// /// AssetResolvers should be updated when the exisitng resolver /// becomes obsolete and a newer one becomes available that /// provides updated access to the same type of assets as the - /// existing one. This update process is meant to be performed at - /// runtime. + /// existing one. This update process is meant to be performed + /// at runtime. /// - /// If less resolvers are provided than existing resolvers of - /// matching type, the the extra existing resolvers will - /// be removed without replacement. If more resolvers are provided - /// than existing matching resolvers, then the extra provided - /// resolvers will be added to the end of the AssetManager - /// resolvers queue. + /// If a null resolver is provided, any existing matching + /// resolvers will be removed without replacement. If no + /// matching resolver is found, the provided resolver will be + /// added to the end of the AssetManager resolvers queue. The + /// replacement only occurs with the first matching resolver. + /// Any additional matching resolvers are untouched. /// - /// @param[in] asset_resolvers The asset resolvers to replace updatable - /// existing resolvers with. + /// @param[in] updated_asset_resolver The asset resolver to replace the + /// resolver of matching type with. /// /// @param[in] type The type of AssetResolver to update. Only resolvers of - /// the specified type will be replaced by an updated + /// the specified type will be replaced by the updated /// resolver. /// - void UpdateResolversByType( - std::vector>& updated_asset_resolvers, + void UpdateResolverByType( + std::unique_ptr updated_asset_resolver, AssetResolver::AssetResolverType type); std::deque> TakeResolvers(); diff --git a/shell/common/platform_view.cc b/shell/common/platform_view.cc index 6ea3eef27bad8..22c2b395ba76e 100644 --- a/shell/common/platform_view.cc +++ b/shell/common/platform_view.cc @@ -170,10 +170,10 @@ void PlatformView::LoadDartDeferredLibraryError(intptr_t loading_unit_id, const std::string error_message, bool transient) {} -void PlatformView::UpdateAssetResolvers( - std::vector>& asset_resolvers, +void PlatformView::UpdateAssetResolverByType( + std::unique_ptr updated_asset_resolver, AssetResolver::AssetResolverType type) { - delegate_.UpdateAssetResolvers(asset_resolvers, type); + delegate_.UpdateAssetResolverByType(std::move(updated_asset_resolver), type); } } // namespace flutter diff --git a/shell/common/platform_view.h b/shell/common/platform_view.h index 9dce20e493438..177c35a6a97b6 100644 --- a/shell/common/platform_view.h +++ b/shell/common/platform_view.h @@ -274,11 +274,10 @@ class PlatformView { bool transient) = 0; //-------------------------------------------------------------------------- - /// @brief Replaces asset resolvers handled by the engine's - /// AssetManager that are of the specified `type` with the - /// resolvers provided in `updated_asset_resolvers`. Updatable - /// AssetResolvers are removed and replaced with the next - /// available resolver in `updated_asset_resolvers`. + /// @brief Replaces the asset resolver handled by the engine's + /// AssetManager of the specified `type` with + /// `updated_asset_resolver`. The matching AssetResolver is + /// removed and replaced with `updated_asset_resolvers`. /// /// AssetResolvers should be updated when the exisitng resolver /// becomes obsolete and a newer one becomes available that @@ -286,22 +285,22 @@ class PlatformView { /// existing one. This update process is meant to be performed /// at runtime. /// - /// If less resolvers are provided than existing resolvers of - /// matching type, the the extra existing resolvers will - /// be removed without replacement. If more resolvers are - /// provided than existing matching resolvers, then the extra - /// provided resolvers will be added to the end of the - /// AssetManager resolvers queue. + /// If a null resolver is provided, any existing matching + /// resolvers will be removed without replacement. If no + /// matching resolver is found, the provided resolver will be + /// added to the end of the AssetManager resolvers queue. The + /// replacement only occurs with the first matching resolver. + /// Any additional matching resolvers are untouched. /// - /// @param[in] asset_resolvers The asset resolvers to replace updatable - /// existing resolvers with. + /// @param[in] updated_asset_resolver The asset resolver to replace the + /// resolver of matching type with. /// /// @param[in] type The type of AssetResolver to update. Only resolvers of - /// the specified type will be replaced by an updated + /// the specified type will be replaced by the updated /// resolver. /// - virtual void UpdateAssetResolvers( - std::vector>& asset_resolvers, + virtual void UpdateAssetResolverByType( + std::unique_ptr updated_asset_resolver, AssetResolver::AssetResolverType type) = 0; }; @@ -742,11 +741,10 @@ class PlatformView { bool transient); //-------------------------------------------------------------------------- - /// @brief Replaces asset resolvers handled by the engine's - /// AssetManager that are of the specified `type` with the - /// resolvers provided in `updated_asset_resolvers`. Updatable - /// AssetResolvers are removed and replaced with the next - /// available resolver in `updated_asset_resolvers`. + /// @brief Replaces the asset resolver handled by the engine's + /// AssetManager of the specified `type` with + /// `updated_asset_resolver`. The matching AssetResolver is + /// removed and replaced with `updated_asset_resolvers`. /// /// AssetResolvers should be updated when the exisitng resolver /// becomes obsolete and a newer one becomes available that @@ -754,22 +752,22 @@ class PlatformView { /// existing one. This update process is meant to be performed /// at runtime. /// - /// If less resolvers are provided than existing resolvers of - /// matching type, the the extra existing resolvers will - /// be removed without replacement. If more resolvers are - /// provided than existing matching resolvers, then the extra - /// provided resolvers will be added to the end of the - /// AssetManager resolvers queue. + /// If a null resolver is provided, any existing matching + /// resolvers will be removed without replacement. If no + /// matching resolver is found, the provided resolver will be + /// added to the end of the AssetManager resolvers queue. The + /// replacement only occurs with the first matching resolver. + /// Any additional matching resolvers are untouched. /// - /// @param[in] asset_resolvers The asset resolvers to replace updatable - /// existing resolvers with. + /// @param[in] updated_asset_resolver The asset resolver to replace the + /// resolver of matching type with. /// /// @param[in] type The type of AssetResolver to update. Only resolvers of - /// the specified type will be replaced by an updated + /// the specified type will be replaced by the updated /// resolver. /// - virtual void UpdateAssetResolvers( - std::vector>& asset_resolvers, + virtual void UpdateAssetResolverByType( + std::unique_ptr updated_asset_resolver, AssetResolver::AssetResolverType type); protected: diff --git a/shell/common/shell.cc b/shell/common/shell.cc index 80b9a98b46d36..dae1ae59b3194 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -1223,10 +1223,11 @@ void Shell::LoadDartDeferredLibraryError(intptr_t loading_unit_id, transient); } -void Shell::UpdateAssetResolvers( - std::vector>& asset_resolvers, +void Shell::UpdateAssetResolverByType( + std::unique_ptr updated_asset_resolver, AssetResolver::AssetResolverType type) { - engine_->GetAssetManager()->UpdateResolversByType(asset_resolvers, type); + engine_->GetAssetManager()->UpdateResolverByType( + std::move(updated_asset_resolver), type); } // |Engine::Delegate| diff --git a/shell/common/shell.h b/shell/common/shell.h index c056b436b6e12..024d434d16f36 100644 --- a/shell/common/shell.h +++ b/shell/common/shell.h @@ -536,8 +536,8 @@ class Shell final : public PlatformView::Delegate, bool transient) override; // |PlatformView::Delegate| - void UpdateAssetResolvers( - std::vector>& asset_resolvers, + void UpdateAssetResolverByType( + std::unique_ptr updated_asset_resolver, AssetResolver::AssetResolverType type) override; // |Animator::Delegate| diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index 44af7867eaafa..8aec380063c6f 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -89,10 +89,9 @@ class MockPlatformViewDelegate : public PlatformView::Delegate { const std::string error_message, bool transient)); - MOCK_METHOD2( - UpdateAssetResolvers, - void(std::vector>& updated_asset_resolvers, - AssetResolver::AssetResolverType type)); + MOCK_METHOD2(UpdateAssetResolverByType, + void(std::unique_ptr updated_asset_resolver, + AssetResolver::AssetResolverType type)); }; class MockSurface : public Surface { @@ -2467,7 +2466,7 @@ TEST_F(ShellTest, Spawn) { ASSERT_FALSE(DartVMRef::IsInstanceRunning()); } -TEST_F(ShellTest, UpdateAssetResolversReplaces) { +TEST_F(ShellTest, UpdateAssetResolverByTypeReplaces) { ASSERT_FALSE(DartVMRef::IsInstanceRunning()); Settings settings = CreateSettingsForFixture(); ThreadHost thread_host("io.flutter.test." + GetCurrentTestName() + ".", @@ -2492,13 +2491,12 @@ TEST_F(ShellTest, UpdateAssetResolversReplaces) { ASSERT_TRUE(old_resolver->IsValid()); asset_manager->PushBack(std::move(old_resolver)); - std::vector> resolver_vector; auto updated_resolver = std::make_unique( false, AssetResolver::AssetResolverType::kApkAssetProvider); ASSERT_FALSE(updated_resolver->IsValidAfterAssetManagerChange()); - resolver_vector.push_back(std::move(updated_resolver)); - platform_view->UpdateAssetResolvers( - resolver_vector, AssetResolver::AssetResolverType::kApkAssetProvider); + platform_view->UpdateAssetResolverByType( + std::move(updated_resolver), + AssetResolver::AssetResolverType::kApkAssetProvider); auto resolvers = asset_manager->TakeResolvers(); ASSERT_EQ(resolvers.size(), 2ull); @@ -2510,7 +2508,7 @@ TEST_F(ShellTest, UpdateAssetResolversReplaces) { ASSERT_FALSE(DartVMRef::IsInstanceRunning()); } -TEST_F(ShellTest, UpdateAssetResolversAppends) { +TEST_F(ShellTest, UpdateAssetResolverByTypeAppends) { ASSERT_FALSE(DartVMRef::IsInstanceRunning()); Settings settings = CreateSettingsForFixture(); ThreadHost thread_host("io.flutter.test." + GetCurrentTestName() + ".", @@ -2530,13 +2528,12 @@ TEST_F(ShellTest, UpdateAssetResolversAppends) { auto platform_view = std::make_unique(*shell.get(), std::move(task_runners)); - std::vector> resolver_vector; auto updated_resolver = std::make_unique( false, AssetResolver::AssetResolverType::kApkAssetProvider); ASSERT_FALSE(updated_resolver->IsValidAfterAssetManagerChange()); - resolver_vector.push_back(std::move(updated_resolver)); - platform_view->UpdateAssetResolvers( - resolver_vector, AssetResolver::AssetResolverType::kApkAssetProvider); + platform_view->UpdateAssetResolverByType( + std::move(updated_resolver), + AssetResolver::AssetResolverType::kApkAssetProvider); auto resolvers = asset_manager->TakeResolvers(); ASSERT_EQ(resolvers.size(), 2ull); @@ -2548,7 +2545,7 @@ TEST_F(ShellTest, UpdateAssetResolversAppends) { ASSERT_FALSE(DartVMRef::IsInstanceRunning()); } -TEST_F(ShellTest, UpdateAssetResolversRemoves) { +TEST_F(ShellTest, UpdateAssetResolverByTypeRemoves) { ASSERT_FALSE(DartVMRef::IsInstanceRunning()); Settings settings = CreateSettingsForFixture(); ThreadHost thread_host("io.flutter.test." + GetCurrentTestName() + ".", @@ -2573,9 +2570,8 @@ TEST_F(ShellTest, UpdateAssetResolversRemoves) { ASSERT_TRUE(old_resolver->IsValid()); asset_manager->PushBack(std::move(old_resolver)); - std::vector> resolver_vector; - platform_view->UpdateAssetResolvers( - resolver_vector, AssetResolver::AssetResolverType::kApkAssetProvider); + platform_view->UpdateAssetResolverByType( + std::move(nullptr), AssetResolver::AssetResolverType::kApkAssetProvider); auto resolvers = asset_manager->TakeResolvers(); ASSERT_EQ(resolvers.size(), 1ull); @@ -2585,7 +2581,7 @@ TEST_F(ShellTest, UpdateAssetResolversRemoves) { ASSERT_FALSE(DartVMRef::IsInstanceRunning()); } -TEST_F(ShellTest, UpdateAssetResolversDoesNotReplaceMismatchType) { +TEST_F(ShellTest, UpdateAssetResolverByTypeDoesNotReplaceMismatchType) { ASSERT_FALSE(DartVMRef::IsInstanceRunning()); Settings settings = CreateSettingsForFixture(); ThreadHost thread_host("io.flutter.test." + GetCurrentTestName() + ".", @@ -2610,13 +2606,12 @@ TEST_F(ShellTest, UpdateAssetResolversDoesNotReplaceMismatchType) { ASSERT_TRUE(old_resolver->IsValid()); asset_manager->PushBack(std::move(old_resolver)); - std::vector> resolver_vector; auto updated_resolver = std::make_unique( false, AssetResolver::AssetResolverType::kApkAssetProvider); ASSERT_FALSE(updated_resolver->IsValidAfterAssetManagerChange()); - resolver_vector.push_back(std::move(updated_resolver)); - platform_view->UpdateAssetResolvers( - resolver_vector, AssetResolver::AssetResolverType::kApkAssetProvider); + platform_view->UpdateAssetResolverByType( + std::move(updated_resolver), + AssetResolver::AssetResolverType::kApkAssetProvider); auto resolvers = asset_manager->TakeResolvers(); ASSERT_EQ(resolvers.size(), 3ull); diff --git a/shell/platform/android/platform_view_android.cc b/shell/platform/android/platform_view_android.cc index f62b31176b88c..68b2b8fdb1a3f 100644 --- a/shell/platform/android/platform_view_android.cc +++ b/shell/platform/android/platform_view_android.cc @@ -363,10 +363,10 @@ void PlatformViewAndroid::LoadDartDeferredLibraryError( } // |PlatformView| -void PlatformViewAndroid::UpdateAssetResolvers( - std::vector>& asset_resolvers, +void PlatformViewAndroid::UpdateAssetResolverByType( + std::unique_ptr updated_asset_resolver, AssetResolver::AssetResolverType type) { - delegate_.UpdateAssetResolvers(asset_resolvers, type); + delegate_.UpdateAssetResolverByType(std::move(updated_asset_resolver), type); } void PlatformViewAndroid::InstallFirstFrameCallback() { diff --git a/shell/platform/android/platform_view_android.h b/shell/platform/android/platform_view_android.h index de97c31abb9c3..49e3c15c13929 100644 --- a/shell/platform/android/platform_view_android.h +++ b/shell/platform/android/platform_view_android.h @@ -104,8 +104,8 @@ class PlatformViewAndroid final : public PlatformView { bool transient) override; // |PlatformView| - void UpdateAssetResolvers( - std::vector>& asset_resolvers, + void UpdateAssetResolverByType( + std::unique_ptr updated_asset_resolver, AssetResolver::AssetResolverType type) override; private: diff --git a/shell/platform/android/platform_view_android_jni_impl.cc b/shell/platform/android/platform_view_android_jni_impl.cc index 698d9c404cc85..efe994f52d94b 100644 --- a/shell/platform/android/platform_view_android_jni_impl.cc +++ b/shell/platform/android/platform_view_android_jni_impl.cc @@ -580,11 +580,10 @@ static void UpdateJavaAssetManager(JNIEnv* env, env, // jni environment jAssetManager, // asset manager fml::jni::JavaStringToString(env, jAssetBundlePath)); // apk asset dir - std::vector> resolver_vector; - resolver_vector.push_back(std::move(asset_resolver)); - ANDROID_SHELL_HOLDER->GetPlatformView()->UpdateAssetResolvers( - resolver_vector, AssetResolver::AssetResolverType::kApkAssetProvider); + ANDROID_SHELL_HOLDER->GetPlatformView()->UpdateAssetResolverByType( + std::move(asset_resolver), + AssetResolver::AssetResolverType::kApkAssetProvider); } bool RegisterApi(JNIEnv* env) { diff --git a/shell/platform/fuchsia/flutter/platform_view_unittest.cc b/shell/platform/fuchsia/flutter/platform_view_unittest.cc index 1b95fc341ae1a..de4ac03ea37f2 100644 --- a/shell/platform/fuchsia/flutter/platform_view_unittest.cc +++ b/shell/platform/fuchsia/flutter/platform_view_unittest.cc @@ -114,8 +114,8 @@ class MockPlatformViewDelegate : public flutter::PlatformView::Delegate { const std::string error_message, bool transient) {} // |flutter::PlatformView::Delegate| - void UpdateAssetResolvers( - std::vector>& asset_resolvers, + void UpdateAssetResolverByType( + std::unique_ptr updated_asset_resolver, AssetResolver::AssetResolverType type) {} flutter::Surface* surface() const { return surface_.get(); } From 2ddbd8130b7df592ccea9fb5c2db36095c9d01d9 Mon Sep 17 00:00:00 2001 From: garyqian Date: Tue, 22 Dec 2020 20:07:47 -0800 Subject: [PATCH 14/18] Fix Fuchsia and Mac tests --- .../darwin/ios/framework/Source/FlutterPlatformViewsTest.mm | 3 ++- .../darwin/ios/framework/Source/accessibility_bridge_test.mm | 3 ++- shell/platform/fuchsia/flutter/platform_view_unittest.cc | 4 ++-- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/FlutterPlatformViewsTest.mm b/shell/platform/darwin/ios/framework/Source/FlutterPlatformViewsTest.mm index f8b5e4ebc3e7b..8f4b3e7a972c4 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterPlatformViewsTest.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterPlatformViewsTest.mm @@ -111,7 +111,8 @@ void LoadDartDeferredLibrary(intptr_t loading_unit_id, void LoadDartDeferredLibraryError(intptr_t loading_unit_id, const std::string error_message, bool transient) override {} - void UpdateAssetManager(std::shared_ptr asset_manager) override {} + void UpdateAssetResolverByType(std::unique_ptr updated_asset_resolver, + flutter::AssetResolver::AssetResolverType type) override {} }; } // namespace diff --git a/shell/platform/darwin/ios/framework/Source/accessibility_bridge_test.mm b/shell/platform/darwin/ios/framework/Source/accessibility_bridge_test.mm index ee56a49a9385d..08f01b7c705ed 100644 --- a/shell/platform/darwin/ios/framework/Source/accessibility_bridge_test.mm +++ b/shell/platform/darwin/ios/framework/Source/accessibility_bridge_test.mm @@ -94,7 +94,8 @@ void LoadDartDeferredLibrary(intptr_t loading_unit_id, void LoadDartDeferredLibraryError(intptr_t loading_unit_id, const std::string error_message, bool transient) override {} - void UpdateAssetManager(std::shared_ptr asset_manager) override {} + void UpdateAssetResolverByType(std::unique_ptr updated_asset_resolver, + flutter::AssetResolver::AssetResolverType type) override {} }; class MockIosDelegate : public AccessibilityBridge::IosDelegate { diff --git a/shell/platform/fuchsia/flutter/platform_view_unittest.cc b/shell/platform/fuchsia/flutter/platform_view_unittest.cc index de4ac03ea37f2..7fabcbd11d1ba 100644 --- a/shell/platform/fuchsia/flutter/platform_view_unittest.cc +++ b/shell/platform/fuchsia/flutter/platform_view_unittest.cc @@ -115,8 +115,8 @@ class MockPlatformViewDelegate : public flutter::PlatformView::Delegate { bool transient) {} // |flutter::PlatformView::Delegate| void UpdateAssetResolverByType( - std::unique_ptr updated_asset_resolver, - AssetResolver::AssetResolverType type) {} + std::unique_ptr updated_asset_resolver, + flutter::AssetResolver::AssetResolverType type) {} flutter::Surface* surface() const { return surface_.get(); } flutter::PlatformMessage* message() const { return message_.get(); } From 0277881302011bd8d55442b21bb22e888af72d35 Mon Sep 17 00:00:00 2001 From: garyqian Date: Wed, 23 Dec 2020 09:40:54 -0800 Subject: [PATCH 15/18] Fix null behavior, fix Mac test --- assets/asset_manager.cc | 11 ++++++----- assets/asset_manager.h | 3 +-- .../framework/Source/FlutterEnginePlatformViewTest.mm | 3 ++- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/assets/asset_manager.cc b/assets/asset_manager.cc index 697f0ff7e1684..c32904c47c8fd 100644 --- a/assets/asset_manager.cc +++ b/assets/asset_manager.cc @@ -32,15 +32,16 @@ void AssetManager::PushBack(std::unique_ptr resolver) { void AssetManager::UpdateResolverByType( std::unique_ptr updated_asset_resolver, AssetResolver::AssetResolverType type) { + if (updated_asset_resolver == nullptr) { + return; + } bool updated = false; std::deque> new_resolvers; for (auto& old_resolver : resolvers_) { if (!updated && old_resolver->GetType() == type) { - if (updated_asset_resolver != nullptr) { - // Push the replacement updated resolver in place of the old_resolver. - new_resolvers.push_back(std::move(updated_asset_resolver)); - updated = true; - } // Drop the resolver if no replacement available. + // Push the replacement updated resolver in place of the old_resolver. + new_resolvers.push_back(std::move(updated_asset_resolver)); + updated = true; } else { new_resolvers.push_back(std::move(old_resolver)); } diff --git a/assets/asset_manager.h b/assets/asset_manager.h index a3e38e6cd7c65..657726904def5 100644 --- a/assets/asset_manager.h +++ b/assets/asset_manager.h @@ -36,8 +36,7 @@ class AssetManager final : public AssetResolver { /// existing one. This update process is meant to be performed /// at runtime. /// - /// If a null resolver is provided, any existing matching - /// resolvers will be removed without replacement. If no + /// If a null resolver is provided, nothing will be done. If no /// matching resolver is found, the provided resolver will be /// added to the end of the AssetManager resolvers queue. The /// replacement only occurs with the first matching resolver. diff --git a/shell/platform/darwin/ios/framework/Source/FlutterEnginePlatformViewTest.mm b/shell/platform/darwin/ios/framework/Source/FlutterEnginePlatformViewTest.mm index 96fb359b96665..dc57e1df40820 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterEnginePlatformViewTest.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterEnginePlatformViewTest.mm @@ -41,7 +41,8 @@ void LoadDartDeferredLibrary(intptr_t loading_unit_id, void LoadDartDeferredLibraryError(intptr_t loading_unit_id, const std::string error_message, bool transient) override {} - void UpdateAssetManager(std::shared_ptr asset_manager) override {} + void UpdateResolverByType(std::unique_ptr updated_asset_resolver, + AssetResolver::AssetResolverType type) override {} }; } // namespace From 246964fc4d5415a1041d807d109a358f6da56158 Mon Sep 17 00:00:00 2001 From: garyqian Date: Wed, 23 Dec 2020 09:44:00 -0800 Subject: [PATCH 16/18] Update tests and docs --- shell/common/platform_view.h | 6 ++---- shell/common/shell_unittests.cc | 5 +++-- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/shell/common/platform_view.h b/shell/common/platform_view.h index 177c35a6a97b6..c3804bbb7481c 100644 --- a/shell/common/platform_view.h +++ b/shell/common/platform_view.h @@ -285,8 +285,7 @@ class PlatformView { /// existing one. This update process is meant to be performed /// at runtime. /// - /// If a null resolver is provided, any existing matching - /// resolvers will be removed without replacement. If no + /// If a null resolver is provided, nothing will be done. If no /// matching resolver is found, the provided resolver will be /// added to the end of the AssetManager resolvers queue. The /// replacement only occurs with the first matching resolver. @@ -752,8 +751,7 @@ class PlatformView { /// existing one. This update process is meant to be performed /// at runtime. /// - /// If a null resolver is provided, any existing matching - /// resolvers will be removed without replacement. If no + /// If a null resolver is provided, nothing will be done. If no /// matching resolver is found, the provided resolver will be /// added to the end of the AssetManager resolvers queue. The /// replacement only occurs with the first matching resolver. diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index 8aec380063c6f..98621f2f0f75e 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -2545,7 +2545,7 @@ TEST_F(ShellTest, UpdateAssetResolverByTypeAppends) { ASSERT_FALSE(DartVMRef::IsInstanceRunning()); } -TEST_F(ShellTest, UpdateAssetResolverByTypeRemoves) { +TEST_F(ShellTest, UpdateAssetResolverByTypeNull) { ASSERT_FALSE(DartVMRef::IsInstanceRunning()); Settings settings = CreateSettingsForFixture(); ThreadHost thread_host("io.flutter.test." + GetCurrentTestName() + ".", @@ -2574,8 +2574,9 @@ TEST_F(ShellTest, UpdateAssetResolverByTypeRemoves) { std::move(nullptr), AssetResolver::AssetResolverType::kApkAssetProvider); auto resolvers = asset_manager->TakeResolvers(); - ASSERT_EQ(resolvers.size(), 1ull); + ASSERT_EQ(resolvers.size(), 2ull); ASSERT_TRUE(resolvers[0]->IsValidAfterAssetManagerChange()); + ASSERT_TRUE(resolvers[1]->IsValidAfterAssetManagerChange()); DestroyShell(std::move(shell), std::move(task_runners)); ASSERT_FALSE(DartVMRef::IsInstanceRunning()); From 9beb9608d839bd0710248b43408e0e3d98449539 Mon Sep 17 00:00:00 2001 From: garyqian Date: Wed, 23 Dec 2020 10:15:36 -0800 Subject: [PATCH 17/18] Fix naming --- .../ios/framework/Source/FlutterEnginePlatformViewTest.mm | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/FlutterEnginePlatformViewTest.mm b/shell/platform/darwin/ios/framework/Source/FlutterEnginePlatformViewTest.mm index dc57e1df40820..c81910f892767 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterEnginePlatformViewTest.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterEnginePlatformViewTest.mm @@ -41,8 +41,8 @@ void LoadDartDeferredLibrary(intptr_t loading_unit_id, void LoadDartDeferredLibraryError(intptr_t loading_unit_id, const std::string error_message, bool transient) override {} - void UpdateResolverByType(std::unique_ptr updated_asset_resolver, - AssetResolver::AssetResolverType type) override {} + void UpdateAssetResolverByType(std::unique_ptr updated_asset_resolver, + AssetResolver::AssetResolverType type) override {} }; } // namespace From 5bb25f2fe706b8d06672aa44429e2e7a3cf1a23b Mon Sep 17 00:00:00 2001 From: garyqian Date: Wed, 23 Dec 2020 16:42:49 -0800 Subject: [PATCH 18/18] Fix nits --- assets/asset_manager.cc | 2 +- assets/asset_manager.h | 2 +- shell/common/platform_view.h | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/assets/asset_manager.cc b/assets/asset_manager.cc index c32904c47c8fd..b1c6e04b897f1 100644 --- a/assets/asset_manager.cc +++ b/assets/asset_manager.cc @@ -47,7 +47,7 @@ void AssetManager::UpdateResolverByType( } } // Append resolver to the end if not used as a replacement. - if (!updated && updated_asset_resolver != nullptr) { + if (!updated) { new_resolvers.push_back(std::move(updated_asset_resolver)); } resolvers_.swap(new_resolvers); diff --git a/assets/asset_manager.h b/assets/asset_manager.h index 657726904def5..a116d6e906466 100644 --- a/assets/asset_manager.h +++ b/assets/asset_manager.h @@ -30,7 +30,7 @@ class AssetManager final : public AssetResolver { /// `updated_asset_resolver`. The matching AssetResolver is /// removed and replaced with `updated_asset_resolvers`. /// - /// AssetResolvers should be updated when the exisitng resolver + /// AssetResolvers should be updated when the existing resolver /// becomes obsolete and a newer one becomes available that /// provides updated access to the same type of assets as the /// existing one. This update process is meant to be performed diff --git a/shell/common/platform_view.h b/shell/common/platform_view.h index c3804bbb7481c..92e670677c25a 100644 --- a/shell/common/platform_view.h +++ b/shell/common/platform_view.h @@ -279,7 +279,7 @@ class PlatformView { /// `updated_asset_resolver`. The matching AssetResolver is /// removed and replaced with `updated_asset_resolvers`. /// - /// AssetResolvers should be updated when the exisitng resolver + /// AssetResolvers should be updated when the existing resolver /// becomes obsolete and a newer one becomes available that /// provides updated access to the same type of assets as the /// existing one. This update process is meant to be performed @@ -745,7 +745,7 @@ class PlatformView { /// `updated_asset_resolver`. The matching AssetResolver is /// removed and replaced with `updated_asset_resolvers`. /// - /// AssetResolvers should be updated when the exisitng resolver + /// AssetResolvers should be updated when the existing resolver /// becomes obsolete and a newer one becomes available that /// provides updated access to the same type of assets as the /// existing one. This update process is meant to be performed