From d414609b10429f2a3d1d05bc0e1184fdfac82f50 Mon Sep 17 00:00:00 2001 From: Mikhail Pozdnyakov Date: Tue, 25 Feb 2020 19:24:16 +0200 Subject: [PATCH 1/7] [core] Move mbgl::Pass and mbgl::PassRefPtr to a separate header --- CMakeLists.txt | 1 + include/mbgl/actor/scheduler.hpp | 19 ++----------------- include/mbgl/util/pass_types.hpp | 24 ++++++++++++++++++++++++ 3 files changed, 27 insertions(+), 17 deletions(-) create mode 100644 include/mbgl/util/pass_types.hpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 8d1490aea43..0a09f09661c 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -266,6 +266,7 @@ add_library( ${PROJECT_SOURCE_DIR}/include/mbgl/util/logging.hpp ${PROJECT_SOURCE_DIR}/include/mbgl/util/noncopyable.hpp ${PROJECT_SOURCE_DIR}/include/mbgl/util/optional.hpp + ${PROJECT_SOURCE_DIR}/include/mbgl/util/pass_types.hpp ${PROJECT_SOURCE_DIR}/include/mbgl/util/platform.hpp ${PROJECT_SOURCE_DIR}/include/mbgl/util/premultiply.hpp ${PROJECT_SOURCE_DIR}/include/mbgl/util/projection.hpp diff --git a/include/mbgl/actor/scheduler.hpp b/include/mbgl/actor/scheduler.hpp index d7fec41ea67..dca6b132f41 100644 --- a/include/mbgl/actor/scheduler.hpp +++ b/include/mbgl/actor/scheduler.hpp @@ -1,5 +1,7 @@ #pragma once +#include + #include #include @@ -9,23 +11,6 @@ namespace mbgl { class Mailbox; -// Using this type as a return type enforces the client to retain the returned object. -// TODO: Move to a separate file if/when other clients for this aux API turn up. -template -class Pass { -public: - Pass(T&& obj_) : obj(std::forward(obj_)) {} - Pass(Pass&&) = default; - Pass(const Pass&) = delete; - operator T() && { return std::move(obj); } - -private: - T obj; -}; - -template -using PassRefPtr = Pass>; - /* A `Scheduler` is responsible for coordinating the processing of messages by one or more actors via their mailboxes. It's an abstract interface. Currently, diff --git a/include/mbgl/util/pass_types.hpp b/include/mbgl/util/pass_types.hpp new file mode 100644 index 00000000000..5415a3177f8 --- /dev/null +++ b/include/mbgl/util/pass_types.hpp @@ -0,0 +1,24 @@ +#pragma once + +#include + +namespace mbgl { + +// Using this type as a return type enforces the client to retain the returned object. +template +class Pass { +public: + Pass(T&& obj_) : obj(std::forward(obj_)) {} + Pass(Pass&&) noexcept = default; + Pass(const Pass&) = delete; + Pass& operator=(const Pass&) = delete; + operator T() && { return std::move(obj); } + +private: + T obj; +}; + +template +using PassRefPtr = Pass>; + +} // namespace mbgl \ No newline at end of file From 080ad3cc2e9921b4c518390183aeb02a0fca50aa Mon Sep 17 00:00:00 2001 From: Mikhail Pozdnyakov Date: Tue, 25 Feb 2020 22:25:10 +0200 Subject: [PATCH 2/7] [core] FileSourceManager::getFileSource() returns PassRefPtr --- bin/cache.cpp | 2 +- bin/offline.cpp | 6 +-- include/mbgl/storage/file_source_manager.hpp | 3 +- platform/android/src/file_source.cpp | 8 +-- .../android/src/offline/offline_manager.cpp | 5 +- .../android/src/offline/offline_region.cpp | 5 +- platform/darwin/src/MGLOfflineStorage.mm | 4 +- platform/glfw/main.cpp | 8 +-- platform/qt/src/qmapboxgl.cpp | 3 +- render-test/file_source.cpp | 2 +- src/mbgl/map/map.cpp | 4 +- src/mbgl/storage/file_source_manager.cpp | 3 +- test/map/map.test.cpp | 9 ++-- test/storage/database_file_source.test.cpp | 3 +- test/storage/main_resource_loader.test.cpp | 52 ++++++++++++------- 15 files changed, 69 insertions(+), 48 deletions(-) diff --git a/bin/cache.cpp b/bin/cache.cpp index 1df782f7523..69d96c1d424 100644 --- a/bin/cache.cpp +++ b/bin/cache.cpp @@ -90,7 +90,7 @@ int main(int argc, char* argv[]) { } mbgl::util::RunLoop loop; - auto dbfs = mbgl::FileSourceManager::get()->getFileSource( + std::shared_ptr dbfs = mbgl::FileSourceManager::get()->getFileSource( mbgl::FileSourceType::Database, mbgl::ResourceOptions().withCachePath(args::get(cacheValue))); dbfs->forward(resource, response, [&loop] { loop.stop(); }); loop.run(); diff --git a/bin/offline.cpp b/bin/offline.cpp index c18fc31810f..97b60c223fc 100644 --- a/bin/offline.cpp +++ b/bin/offline.cpp @@ -163,10 +163,10 @@ int main(int argc, char *argv[]) { util::RunLoop loop; - std::shared_ptr fileSource = - std::static_pointer_cast(FileSourceManager::get()->getFileSource( + std::shared_ptr fileSource = std::static_pointer_cast( + std::shared_ptr(FileSourceManager::get()->getFileSource( FileSourceType::Database, - ResourceOptions().withAccessToken(token).withBaseURL(apiBaseURL).withCachePath(output))); + ResourceOptions().withAccessToken(token).withBaseURL(apiBaseURL).withCachePath(output)))); std::unique_ptr region; diff --git a/include/mbgl/storage/file_source_manager.hpp b/include/mbgl/storage/file_source_manager.hpp index 2b2a43cbeca..e62d321af7c 100644 --- a/include/mbgl/storage/file_source_manager.hpp +++ b/include/mbgl/storage/file_source_manager.hpp @@ -1,6 +1,7 @@ #pragma once #include +#include namespace mbgl { @@ -29,7 +30,7 @@ class FileSourceManager { // Returns shared instance of a file source for (type, options) tuple. // Creates new instance via registered factory if needed. If new instance cannot be // created, nullptr would be returned. - std::shared_ptr getFileSource(FileSourceType, const ResourceOptions&) noexcept; + PassRefPtr getFileSource(FileSourceType, const ResourceOptions&) noexcept; // Registers file source factory for a provided FileSourceType type. If factory for the // same type was already registered, will unregister previously registered factory. diff --git a/platform/android/src/file_source.cpp b/platform/android/src/file_source.cpp index 8bbe9acbd24..5a46bffd3e6 100644 --- a/platform/android/src/file_source.cpp +++ b/platform/android/src/file_source.cpp @@ -41,10 +41,10 @@ FileSource::FileSource(jni::JNIEnv& _env, const jni::String& accessToken, const // TODO: Split Android FileSource API to smaller interfaces resourceLoader = mbgl::FileSourceManager::get()->getFileSource(mbgl::FileSourceType::ResourceLoader, resourceOptions); - databaseSource = std::static_pointer_cast( - mbgl::FileSourceManager::get()->getFileSource(mbgl::FileSourceType::Database, resourceOptions)); - onlineSource = std::static_pointer_cast( - mbgl::FileSourceManager::get()->getFileSource(mbgl::FileSourceType::Network, resourceOptions)); + databaseSource = std::static_pointer_cast(std::shared_ptr( + mbgl::FileSourceManager::get()->getFileSource(mbgl::FileSourceType::Database, resourceOptions))); + onlineSource = std::static_pointer_cast(std::shared_ptr( + mbgl::FileSourceManager::get()->getFileSource(mbgl::FileSourceType::Network, resourceOptions))); } FileSource::~FileSource() { diff --git a/platform/android/src/offline/offline_manager.cpp b/platform/android/src/offline/offline_manager.cpp index 98f2b88a7f9..51d2f70fe03 100644 --- a/platform/android/src/offline/offline_manager.cpp +++ b/platform/android/src/offline/offline_manager.cpp @@ -25,8 +25,9 @@ void handleException(std::exception_ptr exception, // OfflineManager // OfflineManager::OfflineManager(jni::JNIEnv& env, const jni::Object& jFileSource) - : fileSource(std::static_pointer_cast(mbgl::FileSourceManager::get()->getFileSource( - mbgl::FileSourceType::Database, FileSource::getSharedResourceOptions(env, jFileSource)))) { + : fileSource(std::static_pointer_cast( + std::shared_ptr(mbgl::FileSourceManager::get()->getFileSource( + mbgl::FileSourceType::Database, FileSource::getSharedResourceOptions(env, jFileSource))))) { if (!fileSource) { ThrowNew(env, jni::FindClass(env, "java/lang/IllegalStateException"), "Offline functionality is disabled."); } diff --git a/platform/android/src/offline/offline_region.cpp b/platform/android/src/offline/offline_region.cpp index 4ee835bab33..de7abef624c 100644 --- a/platform/android/src/offline/offline_region.cpp +++ b/platform/android/src/offline/offline_region.cpp @@ -16,8 +16,9 @@ namespace android { OfflineRegion::OfflineRegion(jni::JNIEnv& env, jni::jlong offlineRegionPtr, const jni::Object& jFileSource) : region(reinterpret_cast(offlineRegionPtr)), - fileSource(std::static_pointer_cast(mbgl::FileSourceManager::get()->getFileSource( - mbgl::FileSourceType::Database, FileSource::getSharedResourceOptions(env, jFileSource)))) { + fileSource(std::static_pointer_cast( + std::shared_ptr(mbgl::FileSourceManager::get()->getFileSource( + mbgl::FileSourceType::Database, FileSource::getSharedResourceOptions(env, jFileSource))))) { if (!fileSource) { ThrowNew(env, jni::FindClass(env, "java/lang/IllegalStateException"), "Offline functionality is disabled."); } diff --git a/platform/darwin/src/MGLOfflineStorage.mm b/platform/darwin/src/MGLOfflineStorage.mm index abf67d330e3..275a58f418c 100644 --- a/platform/darwin/src/MGLOfflineStorage.mm +++ b/platform/darwin/src/MGLOfflineStorage.mm @@ -233,8 +233,8 @@ - (instancetype)init { options.withCachePath(_mbglCachePath) .withAssetPath([NSBundle mainBundle].resourceURL.path.UTF8String); _mbglFileSource = mbgl::FileSourceManager::get()->getFileSource(mbgl::FileSourceType::ResourceLoader, options); - _mbglOnlineFileSource = std::static_pointer_cast(mbgl::FileSourceManager::get()->getFileSource(mbgl::FileSourceType::Network, options)); - _mbglDatabaseFileSource = std::static_pointer_cast(mbgl::FileSourceManager::get()->getFileSource(mbgl::FileSourceType::Database, options)); + _mbglOnlineFileSource = std::static_pointer_cast(std::shared_ptr(mbgl::FileSourceManager::get()->getFileSource(mbgl::FileSourceType::Network, options))); + _mbglDatabaseFileSource = std::static_pointer_cast(std::shared_ptr(mbgl::FileSourceManager::get()->getFileSource(mbgl::FileSourceType::Database, options))); // Observe for changes to the API base URL (and find out the current one). [[MGLAccountManager sharedManager] addObserver:self diff --git a/platform/glfw/main.cpp b/platform/glfw/main.cpp index ded8ee3e1fb..784e002a656 100644 --- a/platform/glfw/main.cpp +++ b/platform/glfw/main.cpp @@ -107,7 +107,7 @@ int main(int argc, char *argv[]) { mbgl::ResourceOptions resourceOptions; resourceOptions.withCachePath(cacheDB).withAccessToken(token); - auto onlineFileSource = + std::shared_ptr onlineFileSource = mbgl::FileSourceManager::get()->getFileSource(mbgl::FileSourceType::Network, resourceOptions); if (!settings.online) { if (onlineFileSource) { @@ -165,7 +165,7 @@ int main(int argc, char *argv[]) { }); // Resource loader controls top-level request processing and can resume / pause all managed sources simultaneously. - auto resourceLoader = + std::shared_ptr resourceLoader = mbgl::FileSourceManager::get()->getFileSource(mbgl::FileSourceType::ResourceLoader, resourceOptions); view->setPauseResumeCallback([resourceLoader]() { static bool isPaused = false; @@ -180,8 +180,8 @@ int main(int argc, char *argv[]) { }); // Database file source. - auto databaseFileSource = std::static_pointer_cast( - mbgl::FileSourceManager::get()->getFileSource(mbgl::FileSourceType::Database, resourceOptions)); + auto databaseFileSource = std::static_pointer_cast(std::shared_ptr( + mbgl::FileSourceManager::get()->getFileSource(mbgl::FileSourceType::Database, resourceOptions))); view->setResetCacheCallback([databaseFileSource]() { databaseFileSource->resetDatabase([](std::exception_ptr ex) { if (ex) { diff --git a/platform/qt/src/qmapboxgl.cpp b/platform/qt/src/qmapboxgl.cpp index 65ecf1ce617..5b47174b773 100644 --- a/platform/qt/src/qmapboxgl.cpp +++ b/platform/qt/src/qmapboxgl.cpp @@ -1751,7 +1751,8 @@ QMapboxGLPrivate::QMapboxGLPrivate(QMapboxGL *q, const QMapboxGLSettings &settin mbgl::ResourceTransform::FinishedCallback onFinished) { actorRef.invoke(&mbgl::ResourceTransform::TransformCallback::operator(), kind, url, std::move(onFinished)); }}; - auto fs = mbgl::FileSourceManager::get()->getFileSource(mbgl::FileSourceType::Network, resourceOptions); + std::shared_ptr fs = + mbgl::FileSourceManager::get()->getFileSource(mbgl::FileSourceType::Network, resourceOptions); std::static_pointer_cast(fs)->setResourceTransform(std::move(transform)); } diff --git a/render-test/file_source.cpp b/render-test/file_source.cpp index b93325f9014..e81bfb3f17c 100644 --- a/render-test/file_source.cpp +++ b/render-test/file_source.cpp @@ -19,7 +19,7 @@ ProxyFileSource::ProxyFileSource(std::shared_ptr defaultResourceLoad : defaultResourceLoader(std::move(defaultResourceLoader_)) { assert(defaultResourceLoader); if (offline) { - auto dbfs = FileSourceManager::get()->getFileSource(FileSourceType::Database, options); + std::shared_ptr dbfs = FileSourceManager::get()->getFileSource(FileSourceType::Database, options); dbfs->setProperty(READ_ONLY_MODE_KEY, true); } } diff --git a/src/mbgl/map/map.cpp b/src/mbgl/map/map.cpp index 0d9574a2993..7e725951493 100644 --- a/src/mbgl/map/map.cpp +++ b/src/mbgl/map/map.cpp @@ -33,7 +33,9 @@ Map::Map(RendererFrontend& frontend, : impl(std::make_unique( frontend, observer, - FileSourceManager::get() ? FileSourceManager::get()->getFileSource(ResourceLoader, resourceOptions) : nullptr, + FileSourceManager::get() + ? std::shared_ptr(FileSourceManager::get()->getFileSource(ResourceLoader, resourceOptions)) + : nullptr, mapOptions)) {} Map::Map(std::unique_ptr impl_) : impl(std::move(impl_)) {} diff --git a/src/mbgl/storage/file_source_manager.cpp b/src/mbgl/storage/file_source_manager.cpp index 62fd5e8f98c..6689d5314a2 100644 --- a/src/mbgl/storage/file_source_manager.cpp +++ b/src/mbgl/storage/file_source_manager.cpp @@ -30,8 +30,7 @@ FileSourceManager::FileSourceManager() : impl(std::make_unique()) {} FileSourceManager::~FileSourceManager() = default; -std::shared_ptr FileSourceManager::getFileSource(FileSourceType type, - const ResourceOptions& options) noexcept { +PassRefPtr FileSourceManager::getFileSource(FileSourceType type, const ResourceOptions& options) noexcept { std::lock_guard lock(impl->mutex); // Remove released file sources. diff --git a/test/map/map.test.cpp b/test/map/map.test.cpp index 8f7270529f2..b3c73be3d01 100644 --- a/test/map/map.test.cpp +++ b/test/map/map.test.cpp @@ -342,7 +342,8 @@ TEST(Map, Offline) { NetworkStatus::Set(NetworkStatus::Status::Offline); const std::string prefix = "http://127.0.0.1:3000/"; - auto dbfs = FileSourceManager::get()->getFileSource(FileSourceType::Database, ResourceOptions{}); + std::shared_ptr dbfs = + FileSourceManager::get()->getFileSource(FileSourceType::Database, ResourceOptions{}); dbfs->forward(Resource::style(prefix + "style.json"), expiredItem("style.json")); dbfs->forward(Resource::source(prefix + "streets.json"), expiredItem("streets.json")); dbfs->forward(Resource::spriteJSON(prefix + "sprite", 1.0), expiredItem("sprite.json")); @@ -882,7 +883,8 @@ TEST(Map, NoContentTiles) { Response response; response.noContent = true; response.expires = util::now() + 1h; - auto dbfs = FileSourceManager::get()->getFileSource(FileSourceType::Database, ResourceOptions{}); + std::shared_ptr dbfs = + FileSourceManager::get()->getFileSource(FileSourceType::Database, ResourceOptions{}); dbfs->forward( Resource::tile("http://example.com/{z}-{x}-{y}.vector.pbf", 1.0, 0, 0, 0, Tileset::Scheme::XYZ), response, [&] { test.map.getStyle().loadJSON(R"STYLE({ @@ -1327,7 +1329,8 @@ TEST(Map, TEST_REQUIRES_SERVER(ExpiredSpriteSheet)) { NetworkStatus::Set(NetworkStatus::Status::Offline); const std::string prefix = "http://127.0.0.1:3000/online/"; - auto dbfs = FileSourceManager::get()->getFileSource(FileSourceType::Database, ResourceOptions{}); + std::shared_ptr dbfs = + FileSourceManager::get()->getFileSource(FileSourceType::Database, ResourceOptions{}); dbfs->forward(Resource::style(prefix + "style.json"), makeResponse("style.json")); dbfs->forward(Resource::source(prefix + "streets.json"), makeResponse("streets.json")); dbfs->forward(Resource::spriteJSON(prefix + "sprite", 1.0), makeResponse("sprite.json", true)); diff --git a/test/storage/database_file_source.test.cpp b/test/storage/database_file_source.test.cpp index 579e9a8c57d..62e59546054 100644 --- a/test/storage/database_file_source.test.cpp +++ b/test/storage/database_file_source.test.cpp @@ -12,7 +12,8 @@ using namespace mbgl; TEST(DatabaseFileSource, PauseResume) { util::RunLoop loop; - auto dbfs = FileSourceManager::get()->getFileSource(FileSourceType::Database, ResourceOptions{}); + std::shared_ptr dbfs = + FileSourceManager::get()->getFileSource(FileSourceType::Database, ResourceOptions{}); dbfs->pause(); const Resource res{Resource::Unknown, "http://127.0.0.1:3000/test", {}, Resource::LoadingMethod::CacheOnly}; diff --git a/test/storage/main_resource_loader.test.cpp b/test/storage/main_resource_loader.test.cpp index b5245dbad89..307f92463cc 100644 --- a/test/storage/main_resource_loader.test.cpp +++ b/test/storage/main_resource_loader.test.cpp @@ -263,7 +263,8 @@ TEST(MainResourceLoader, OptionalNonExpired) { response.expires = util::now() + 1h; std::unique_ptr req; - auto dbfs = FileSourceManager::get()->getFileSource(FileSourceType::Database, ResourceOptions{}); + std::shared_ptr dbfs = + FileSourceManager::get()->getFileSource(FileSourceType::Database, ResourceOptions{}); dbfs->forward(optionalResource, response, [&] { req = fs.request(optionalResource, [&](Response res) { req.reset(); @@ -294,7 +295,8 @@ TEST(MainResourceLoader, OptionalExpired) { Response response; response.data = std::make_shared("Cached value"); response.expires = util::now() - 1h; - auto dbfs = FileSourceManager::get()->getFileSource(FileSourceType::Database, ResourceOptions{}); + std::shared_ptr dbfs = + FileSourceManager::get()->getFileSource(FileSourceType::Database, ResourceOptions{}); std::unique_ptr req; dbfs->forward(optionalResource, response, [&] { req = fs.request(optionalResource, [&](Response res) { @@ -356,7 +358,8 @@ TEST(MainResourceLoader, TEST_REQUIRES_SERVER(NoCacheRefreshEtagNotModified)) { response.data = std::make_shared("Cached value"); response.expires = util::now() + 1h; std::unique_ptr req; - auto dbfs = FileSourceManager::get()->getFileSource(FileSourceType::Database, ResourceOptions{}); + std::shared_ptr dbfs = + FileSourceManager::get()->getFileSource(FileSourceType::Database, ResourceOptions{}); dbfs->forward(resource, response, [&] { req = fs.request(resource, [&](Response res) { req.reset(); @@ -392,7 +395,8 @@ TEST(MainResourceLoader, TEST_REQUIRES_SERVER(NoCacheRefreshEtagModified)) { response.data = std::make_shared("Cached value"); response.expires = util::now() + 1h; std::unique_ptr req; - auto dbfs = FileSourceManager::get()->getFileSource(FileSourceType::Database, ResourceOptions{}); + std::shared_ptr dbfs = + FileSourceManager::get()->getFileSource(FileSourceType::Database, ResourceOptions{}); dbfs->forward(resource, response, [&] { req = fs.request(resource, [&](Response res) { req.reset(); @@ -427,7 +431,8 @@ TEST(MainResourceLoader, TEST_REQUIRES_SERVER(NoCacheFull)) { response.data = std::make_shared("Cached value"); response.expires = util::now() + 1h; std::unique_ptr req; - auto dbfs = FileSourceManager::get()->getFileSource(FileSourceType::Database, ResourceOptions{}); + std::shared_ptr dbfs = + FileSourceManager::get()->getFileSource(FileSourceType::Database, ResourceOptions{}); dbfs->forward(resource, response, [&] { req = fs.request(resource, [&](Response res) { req.reset(); @@ -464,7 +469,8 @@ TEST(MainResourceLoader, TEST_REQUIRES_SERVER(NoCacheRefreshModifiedNotModified) response.data = std::make_shared("Cached value"); response.expires = util::now() + 1h; std::unique_ptr req; - auto dbfs = FileSourceManager::get()->getFileSource(FileSourceType::Database, ResourceOptions{}); + std::shared_ptr dbfs = + FileSourceManager::get()->getFileSource(FileSourceType::Database, ResourceOptions{}); dbfs->forward(resource, response, [&] { req = fs.request(resource, [&](Response res) { req.reset(); @@ -501,7 +507,8 @@ TEST(MainResourceLoader, TEST_REQUIRES_SERVER(NoCacheRefreshModifiedModified)) { response.data = std::make_shared("Cached value"); response.expires = util::now() + 1h; std::unique_ptr req; - auto dbfs = FileSourceManager::get()->getFileSource(FileSourceType::Database, ResourceOptions{}); + std::shared_ptr dbfs = + FileSourceManager::get()->getFileSource(FileSourceType::Database, ResourceOptions{}); dbfs->forward(resource, response, [&] { req = fs.request(resource, [&](Response res) { req.reset(); @@ -522,10 +529,10 @@ TEST(MainResourceLoader, TEST_REQUIRES_SERVER(NoCacheRefreshModifiedModified)) { TEST(MainResourceLoader, TEST_REQUIRES_SERVER(SetResourceTransform)) { util::RunLoop loop; - MainResourceLoader fs(ResourceOptions{}); - - auto onlinefs = std::static_pointer_cast( - FileSourceManager::get()->getFileSource(FileSourceType::Network, ResourceOptions{})); + MainResourceLoader resourceLoader(ResourceOptions{}); + std::shared_ptr fs = + FileSourceManager::get()->getFileSource(FileSourceType::Network, ResourceOptions{}); + auto onlinefs = std::static_pointer_cast(fs); // Translates the URL "localhost://test to http://127.0.0.1:3000/test Actor transform( @@ -545,7 +552,7 @@ TEST(MainResourceLoader, TEST_REQUIRES_SERVER(SetResourceTransform)) { const Resource resource1{Resource::Unknown, "localhost://test"}; std::unique_ptr req; - req = fs.request(resource1, [&](Response res) { + req = resourceLoader.request(resource1, [&](Response res) { req.reset(); EXPECT_EQ(nullptr, res.error); ASSERT_TRUE(res.data.get()); @@ -562,7 +569,7 @@ TEST(MainResourceLoader, TEST_REQUIRES_SERVER(SetResourceTransform)) { onlinefs->setResourceTransform({}); const Resource resource2{Resource::Unknown, "http://127.0.0.1:3000/test"}; - req = fs.request(resource2, [&](Response res) { + req = resourceLoader.request(resource2, [&](Response res) { req.reset(); EXPECT_EQ(nullptr, res.error); ASSERT_TRUE(res.data.get()); @@ -579,9 +586,10 @@ TEST(MainResourceLoader, TEST_REQUIRES_SERVER(SetResourceTransform)) { TEST(MainResourceLoader, SetResourceCachePath) { util::RunLoop loop; - MainResourceLoader fs(ResourceOptions{}); - auto dbfs = std::static_pointer_cast( - FileSourceManager::get()->getFileSource(FileSourceType::Database, ResourceOptions{})); + MainResourceLoader resourceLoader(ResourceOptions{}); + std::shared_ptr fs = + FileSourceManager::get()->getFileSource(FileSourceType::Database, ResourceOptions{}); + auto dbfs = std::static_pointer_cast(fs); dbfs->setDatabasePath("./new_offline.db", [&loop] { loop.stop(); }); loop.run(); } @@ -604,7 +612,8 @@ TEST(MainResourceLoader, TEST_REQUIRES_SERVER(RespondToStaleMustRevalidate)) { response.mustRevalidate = true; response.etag.emplace("snowfall"); std::unique_ptr req; - auto dbfs = FileSourceManager::get()->getFileSource(FileSourceType::Database, ResourceOptions{}); + std::shared_ptr dbfs = + FileSourceManager::get()->getFileSource(FileSourceType::Database, ResourceOptions{}); dbfs->forward(resource, response, [&] { req = fs.request(resource, [&](Response res) { req.reset(); @@ -676,8 +685,10 @@ TEST(MainResourceLoader, TEST_REQUIRES_SERVER(CachedResourceLowPriority)) { using namespace std::chrono_literals; response.expires = util::now() - 1h; - auto dbfs = FileSourceManager::get()->getFileSource(FileSourceType::Database, ResourceOptions{}); - auto onlineFs = FileSourceManager::get()->getFileSource(FileSourceType::Network, ResourceOptions{}); + std::shared_ptr dbfs = + FileSourceManager::get()->getFileSource(FileSourceType::Database, ResourceOptions{}); + std::shared_ptr onlineFs = + FileSourceManager::get()->getFileSource(FileSourceType::Network, ResourceOptions{}); // Put existing values into the cache. Resource resource1{Resource::Unknown, "http://127.0.0.1:3000/load/3", {}, Resource::LoadingMethod::All}; @@ -762,7 +773,8 @@ TEST(MainResourceLoader, TEST_REQUIRES_SERVER(NoDoubleDispatch)) { std::unique_ptr req; unsigned responseCount = 0u; - auto dbfs = FileSourceManager::get()->getFileSource(FileSourceType::Database, ResourceOptions{}); + std::shared_ptr dbfs = + FileSourceManager::get()->getFileSource(FileSourceType::Database, ResourceOptions{}); dbfs->forward(resource, response, [&] { req = fs.request(resource, [&](Response res) { EXPECT_EQ(nullptr, res.error); From b4d1bc5ace08f4255ca32def4eeb04f1f87477ba Mon Sep 17 00:00:00 2001 From: Mikhail Pozdnyakov Date: Wed, 26 Feb 2020 17:19:36 +0200 Subject: [PATCH 3/7] [core] Move setResourceTransform() to FileSource interface --- include/mbgl/storage/file_source.hpp | 4 ++++ include/mbgl/storage/online_file_source.hpp | 7 +------ 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/include/mbgl/storage/file_source.hpp b/include/mbgl/storage/file_source.hpp index baf3fabcfa5..43143c640f1 100644 --- a/include/mbgl/storage/file_source.hpp +++ b/include/mbgl/storage/file_source.hpp @@ -1,5 +1,6 @@ #pragma once +#include #include #include @@ -80,6 +81,9 @@ class FileSource { virtual void setProperty(const std::string&, const mapbox::base::Value&){}; virtual mapbox::base::Value getProperty(const std::string&) const { return {}; }; + // When supported, sets the modifier of the requested resources. + virtual void setResourceTransform(ResourceTransform) {} + protected: FileSource() = default; }; diff --git a/include/mbgl/storage/online_file_source.hpp b/include/mbgl/storage/online_file_source.hpp index 3c61f20d2ef..bb7adbb5d50 100644 --- a/include/mbgl/storage/online_file_source.hpp +++ b/include/mbgl/storage/online_file_source.hpp @@ -5,8 +5,6 @@ namespace mbgl { -class ResourceTransform; - class OnlineFileSource : public FileSource { public: OnlineFileSource(); @@ -19,10 +17,7 @@ class OnlineFileSource : public FileSource { void resume() override; void setProperty(const std::string&, const mapbox::base::Value&) override; mapbox::base::Value getProperty(const std::string&) const override; - - // OnlineFileSource interface. - // TODO: Would be nice to drop it to get uniform interface. - virtual void setResourceTransform(ResourceTransform); + void setResourceTransform(ResourceTransform) override; private: class Impl; From d179730b2c44b48d293be5fbacba74ced4ede5a9 Mon Sep 17 00:00:00 2001 From: Mikhail Pozdnyakov Date: Wed, 26 Feb 2020 17:31:02 +0200 Subject: [PATCH 4/7] [core] Fix mbgl::Pass constructor --- include/mbgl/util/pass_types.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/mbgl/util/pass_types.hpp b/include/mbgl/util/pass_types.hpp index 5415a3177f8..227a81778d0 100644 --- a/include/mbgl/util/pass_types.hpp +++ b/include/mbgl/util/pass_types.hpp @@ -8,7 +8,7 @@ namespace mbgl { template class Pass { public: - Pass(T&& obj_) : obj(std::forward(obj_)) {} + Pass(T obj_) : obj(std::move(obj_)) {} Pass(Pass&&) noexcept = default; Pass(const Pass&) = delete; Pass& operator=(const Pass&) = delete; From 83a10932b326f572b2852904de8f29b212d4cd49 Mon Sep 17 00:00:00 2001 From: Mikhail Pozdnyakov Date: Wed, 26 Feb 2020 17:47:03 +0200 Subject: [PATCH 5/7] [core] OnlineFileSource is never accessed directly --- include/mbgl/storage/online_file_source.hpp | 2 +- platform/android/src/file_source.cpp | 3 +- platform/android/src/file_source.hpp | 2 +- platform/darwin/src/MGLOfflineStorage.mm | 4 +- .../darwin/src/MGLOfflineStorage_Private.h | 2 +- .../src/mbgl/storage/file_source_manager.cpp | 2 +- platform/qt/src/qmapboxgl.cpp | 2 +- test/src/mbgl/test/fake_file_source.hpp | 4 +- test/storage/main_resource_loader.test.cpp | 3 +- test/storage/online_file_source.test.cpp | 274 +++++++++--------- 10 files changed, 144 insertions(+), 154 deletions(-) diff --git a/include/mbgl/storage/online_file_source.hpp b/include/mbgl/storage/online_file_source.hpp index bb7adbb5d50..d830450f967 100644 --- a/include/mbgl/storage/online_file_source.hpp +++ b/include/mbgl/storage/online_file_source.hpp @@ -10,6 +10,7 @@ class OnlineFileSource : public FileSource { OnlineFileSource(); ~OnlineFileSource() override; +private: // FileSource overrides std::unique_ptr request(const Resource&, Callback) override; bool canRequest(const Resource&) const override; @@ -19,7 +20,6 @@ class OnlineFileSource : public FileSource { mapbox::base::Value getProperty(const std::string&) const override; void setResourceTransform(ResourceTransform) override; -private: class Impl; const std::unique_ptr impl; }; diff --git a/platform/android/src/file_source.cpp b/platform/android/src/file_source.cpp index 5a46bffd3e6..f95066546a7 100644 --- a/platform/android/src/file_source.cpp +++ b/platform/android/src/file_source.cpp @@ -43,8 +43,7 @@ FileSource::FileSource(jni::JNIEnv& _env, const jni::String& accessToken, const mbgl::FileSourceManager::get()->getFileSource(mbgl::FileSourceType::ResourceLoader, resourceOptions); databaseSource = std::static_pointer_cast(std::shared_ptr( mbgl::FileSourceManager::get()->getFileSource(mbgl::FileSourceType::Database, resourceOptions))); - onlineSource = std::static_pointer_cast(std::shared_ptr( - mbgl::FileSourceManager::get()->getFileSource(mbgl::FileSourceType::Network, resourceOptions))); + onlineSource = mbgl::FileSourceManager::get()->getFileSource(mbgl::FileSourceType::Network, resourceOptions); } FileSource::~FileSource() { diff --git a/platform/android/src/file_source.hpp b/platform/android/src/file_source.hpp index 591bfbc93e3..3bf0fed2eb0 100644 --- a/platform/android/src/file_source.hpp +++ b/platform/android/src/file_source.hpp @@ -75,7 +75,7 @@ class FileSource { std::unique_ptr> resourceTransform; std::function pathChangeCallback; std::shared_ptr databaseSource; - std::shared_ptr onlineSource; + std::shared_ptr onlineSource; std::shared_ptr resourceLoader; }; diff --git a/platform/darwin/src/MGLOfflineStorage.mm b/platform/darwin/src/MGLOfflineStorage.mm index 275a58f418c..4c71286b798 100644 --- a/platform/darwin/src/MGLOfflineStorage.mm +++ b/platform/darwin/src/MGLOfflineStorage.mm @@ -46,7 +46,7 @@ @interface MGLOfflineStorage () @property (nonatomic, strong, readwrite) NSMutableArray *packs; @property (nonatomic) std::shared_ptr mbglDatabaseFileSource; -@property (nonatomic) std::shared_ptr mbglOnlineFileSource; +@property (nonatomic) std::shared_ptr mbglOnlineFileSource; @property (nonatomic) std::shared_ptr mbglFileSource; @property (nonatomic) std::string mbglCachePath; @property (nonatomic, getter=isPaused) BOOL paused; @@ -233,7 +233,7 @@ - (instancetype)init { options.withCachePath(_mbglCachePath) .withAssetPath([NSBundle mainBundle].resourceURL.path.UTF8String); _mbglFileSource = mbgl::FileSourceManager::get()->getFileSource(mbgl::FileSourceType::ResourceLoader, options); - _mbglOnlineFileSource = std::static_pointer_cast(std::shared_ptr(mbgl::FileSourceManager::get()->getFileSource(mbgl::FileSourceType::Network, options))); + _mbglOnlineFileSource = mbgl::FileSourceManager::get()->getFileSource(mbgl::FileSourceType::Network, options); _mbglDatabaseFileSource = std::static_pointer_cast(std::shared_ptr(mbgl::FileSourceManager::get()->getFileSource(mbgl::FileSourceType::Database, options))); // Observe for changes to the API base URL (and find out the current one). diff --git a/platform/darwin/src/MGLOfflineStorage_Private.h b/platform/darwin/src/MGLOfflineStorage_Private.h index 13bddd79992..c01163e766b 100644 --- a/platform/darwin/src/MGLOfflineStorage_Private.h +++ b/platform/darwin/src/MGLOfflineStorage_Private.h @@ -19,7 +19,7 @@ NS_ASSUME_NONNULL_BEGIN /** The shared online file source object owned by the shared offline storage object. */ -@property (nonatomic) std::shared_ptr mbglOnlineFileSource; +@property (nonatomic) std::shared_ptr mbglOnlineFileSource; /** The shared resource loader file source object owned by the shared offline storage object. diff --git a/platform/default/src/mbgl/storage/file_source_manager.cpp b/platform/default/src/mbgl/storage/file_source_manager.cpp index 2981096dace..1a13f055681 100644 --- a/platform/default/src/mbgl/storage/file_source_manager.cpp +++ b/platform/default/src/mbgl/storage/file_source_manager.cpp @@ -27,7 +27,7 @@ class DefaultFileSourceManagerImpl final : public FileSourceManager { [](const ResourceOptions&) { return std::make_unique(); }); registerFileSourceFactory(FileSourceType::Network, [](const ResourceOptions& options) { - auto networkSource = std::make_unique(); + std::unique_ptr networkSource = std::make_unique(); networkSource->setProperty(ACCESS_TOKEN_KEY, options.accessToken()); networkSource->setProperty(API_BASE_URL_KEY, options.baseURL()); return networkSource; diff --git a/platform/qt/src/qmapboxgl.cpp b/platform/qt/src/qmapboxgl.cpp index 5b47174b773..14e9d575582 100644 --- a/platform/qt/src/qmapboxgl.cpp +++ b/platform/qt/src/qmapboxgl.cpp @@ -1753,7 +1753,7 @@ QMapboxGLPrivate::QMapboxGLPrivate(QMapboxGL *q, const QMapboxGLSettings &settin }}; std::shared_ptr fs = mbgl::FileSourceManager::get()->getFileSource(mbgl::FileSourceType::Network, resourceOptions); - std::static_pointer_cast(fs)->setResourceTransform(std::move(transform)); + fs->setResourceTransform(std::move(transform)); } // Needs to be Queued to give time to discard redundant draw calls via the `renderQueued` flag. diff --git a/test/src/mbgl/test/fake_file_source.hpp b/test/src/mbgl/test/fake_file_source.hpp index 1faf4b7a18e..7c5adbfff4f 100644 --- a/test/src/mbgl/test/fake_file_source.hpp +++ b/test/src/mbgl/test/fake_file_source.hpp @@ -75,10 +75,10 @@ class FakeOnlineFileSource : public FakeFileSource { } mapbox::base::Value getProperty(const std::string& property) const override { - return onlineFs.getProperty(property); + return onlineFs->getProperty(property); } - OnlineFileSource onlineFs; + std::unique_ptr onlineFs = std::make_unique(); }; } // namespace mbgl diff --git a/test/storage/main_resource_loader.test.cpp b/test/storage/main_resource_loader.test.cpp index 307f92463cc..2622d91183f 100644 --- a/test/storage/main_resource_loader.test.cpp +++ b/test/storage/main_resource_loader.test.cpp @@ -530,9 +530,8 @@ TEST(MainResourceLoader, TEST_REQUIRES_SERVER(NoCacheRefreshModifiedModified)) { TEST(MainResourceLoader, TEST_REQUIRES_SERVER(SetResourceTransform)) { util::RunLoop loop; MainResourceLoader resourceLoader(ResourceOptions{}); - std::shared_ptr fs = + std::shared_ptr onlinefs = FileSourceManager::get()->getFileSource(FileSourceType::Network, ResourceOptions{}); - auto onlinefs = std::static_pointer_cast(fs); // Translates the URL "localhost://test to http://127.0.0.1:3000/test Actor transform( diff --git a/test/storage/online_file_source.test.cpp b/test/storage/online_file_source.test.cpp index 9d8fbfab64e..1bed5f9618d 100644 --- a/test/storage/online_file_source.test.cpp +++ b/test/storage/online_file_source.test.cpp @@ -14,25 +14,23 @@ using namespace mbgl; TEST(OnlineFileSource, Cancel) { util::RunLoop loop; - OnlineFileSource fs; + std::unique_ptr fs = std::make_unique(); - fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, [&](Response) { - ADD_FAILURE() << "Callback should not be called"; - }); + fs->request({Resource::Unknown, "http://127.0.0.1:3000/test"}, + [&](Response) { ADD_FAILURE() << "Callback should not be called"; }); loop.runOnce(); } TEST(OnlineFileSource, TEST_REQUIRES_SERVER(CancelMultiple)) { util::RunLoop loop; - OnlineFileSource fs; + std::unique_ptr fs = std::make_unique(); const Resource resource { Resource::Unknown, "http://127.0.0.1:3000/test" }; - std::unique_ptr req2 = fs.request(resource, [&](Response) { - ADD_FAILURE() << "Callback should not be called"; - }); - std::unique_ptr req = fs.request(resource, [&](Response res) { + std::unique_ptr req2 = + fs->request(resource, [&](Response) { ADD_FAILURE() << "Callback should not be called"; }); + std::unique_ptr req = fs->request(resource, [&](Response res) { req.reset(); EXPECT_EQ(nullptr, res.error); ASSERT_TRUE(res.data.get()); @@ -50,38 +48,38 @@ TEST(OnlineFileSource, TEST_REQUIRES_SERVER(CancelMultiple)) { TEST(OnlineFileSource, TEST_REQUIRES_SERVER(TemporaryError)) { util::RunLoop loop; - OnlineFileSource fs; + std::unique_ptr fs = std::make_unique(); const auto start = Clock::now(); int counter = 0; - auto req = fs.request({ Resource::Unknown, "http://127.0.0.1:3000/temporary-error" }, [&](Response res) { + auto req = fs->request({Resource::Unknown, "http://127.0.0.1:3000/temporary-error"}, [&](Response res) { switch (counter++) { - case 0: { - const auto duration = std::chrono::duration(Clock::now() - start).count(); - EXPECT_GT(0.2, duration) << "Initial error request took too long"; - ASSERT_NE(nullptr, res.error); - EXPECT_EQ(Response::Error::Reason::Server, res.error->reason); - EXPECT_EQ("HTTP status code 500", res.error->message); - ASSERT_FALSE(bool(res.data)); - EXPECT_FALSE(bool(res.expires)); - EXPECT_FALSE(res.mustRevalidate); - EXPECT_FALSE(bool(res.modified)); - EXPECT_FALSE(bool(res.etag)); - } break; - case 1: { - const auto duration = std::chrono::duration(Clock::now() - start).count(); - EXPECT_LT(0.99, duration) << "Backoff timer didn't wait 1 second"; - EXPECT_GT(1.2, duration) << "Backoff timer fired too late"; - EXPECT_EQ(nullptr, res.error); - ASSERT_TRUE(res.data.get()); - EXPECT_EQ("Hello World!", *res.data); - EXPECT_FALSE(bool(res.expires)); - EXPECT_FALSE(res.mustRevalidate); - EXPECT_FALSE(bool(res.modified)); - EXPECT_FALSE(bool(res.etag)); - loop.stop(); - } break; + case 0: { + const auto duration = std::chrono::duration(Clock::now() - start).count(); + EXPECT_GT(0.2, duration) << "Initial error request took too long"; + ASSERT_NE(nullptr, res.error); + EXPECT_EQ(Response::Error::Reason::Server, res.error->reason); + EXPECT_EQ("HTTP status code 500", res.error->message); + ASSERT_FALSE(bool(res.data)); + EXPECT_FALSE(bool(res.expires)); + EXPECT_FALSE(res.mustRevalidate); + EXPECT_FALSE(bool(res.modified)); + EXPECT_FALSE(bool(res.etag)); + } break; + case 1: { + const auto duration = std::chrono::duration(Clock::now() - start).count(); + EXPECT_LT(0.99, duration) << "Backoff timer didn't wait 1 second"; + EXPECT_GT(1.2, duration) << "Backoff timer fired too late"; + EXPECT_EQ(nullptr, res.error); + ASSERT_TRUE(res.data.get()); + EXPECT_EQ("Hello World!", *res.data); + EXPECT_FALSE(bool(res.expires)); + EXPECT_FALSE(res.mustRevalidate); + EXPECT_FALSE(bool(res.modified)); + EXPECT_FALSE(bool(res.etag)); + loop.stop(); + } break; } }); @@ -90,13 +88,13 @@ TEST(OnlineFileSource, TEST_REQUIRES_SERVER(TemporaryError)) { TEST(OnlineFileSource, TEST_REQUIRES_SERVER(ConnectionError)) { util::RunLoop loop; - OnlineFileSource fs; + std::unique_ptr fs = std::make_unique(); const auto start = Clock::now(); int counter = 0; int wait = 0; - std::unique_ptr req = fs.request({ Resource::Unknown, "http://127.0.0.1:3001/" }, [&](Response res) { + std::unique_ptr req = fs->request({Resource::Unknown, "http://127.0.0.1:3001/"}, [&](Response res) { const auto duration = std::chrono::duration(Clock::now() - start).count(); EXPECT_LT(wait - 0.01, duration) << "Backoff timer didn't wait 1 second"; EXPECT_GT(wait + 0.3, duration) << "Backoff timer fired too late"; @@ -121,12 +119,12 @@ TEST(OnlineFileSource, TEST_REQUIRES_SERVER(ConnectionError)) { TEST(OnlineFileSource, TEST_REQUIRES_SERVER(Timeout)) { util::RunLoop loop; - OnlineFileSource fs; + std::unique_ptr fs = std::make_unique(); int counter = 0; const Resource resource { Resource::Unknown, "http://127.0.0.1:3000/test?cachecontrol=max-age=1" }; - std::unique_ptr req = fs.request(resource, [&](Response res) { + std::unique_ptr req = fs->request(resource, [&](Response res) { counter++; EXPECT_EQ(nullptr, res.error); ASSERT_TRUE(res.data.get()); @@ -148,12 +146,12 @@ TEST(OnlineFileSource, TEST_REQUIRES_SERVER(Timeout)) { TEST(OnlineFileSource, TEST_REQUIRES_SERVER(RetryDelayOnExpiredTile)) { util::RunLoop loop; - OnlineFileSource fs; + std::unique_ptr fs = std::make_unique(); int counter = 0; const Resource resource { Resource::Unknown, "http://127.0.0.1:3000/test?expires=10000" }; - std::unique_ptr req = fs.request(resource, [&](Response res) { + std::unique_ptr req = fs->request(resource, [&](Response res) { counter++; EXPECT_EQ(nullptr, res.error); EXPECT_GT(util::now(), *res.expires); @@ -168,27 +166,27 @@ TEST(OnlineFileSource, TEST_REQUIRES_SERVER(RetryDelayOnExpiredTile)) { TEST(OnlineFileSource, TEST_REQUIRES_SERVER(RetryOnClockSkew)) { util::RunLoop loop; - OnlineFileSource fs; + std::unique_ptr fs = std::make_unique(); int counter = 0; const Resource resource { Resource::Unknown, "http://127.0.0.1:3000/clockskew" }; - std::unique_ptr req1 = fs.request(resource, [&](Response res) { + std::unique_ptr req1 = fs->request(resource, [&](Response res) { EXPECT_FALSE(res.mustRevalidate); switch (counter++) { - case 0: { - EXPECT_EQ(nullptr, res.error); - EXPECT_GT(util::now(), *res.expires); - } break; - case 1: { - EXPECT_EQ(nullptr, res.error); + case 0: { + EXPECT_EQ(nullptr, res.error); + EXPECT_GT(util::now(), *res.expires); + } break; + case 1: { + EXPECT_EQ(nullptr, res.error); - auto now = util::now(); - EXPECT_LT(now + Seconds(40), *res.expires) << "Expiration not interpolated to 60s"; - EXPECT_GT(now + Seconds(80), *res.expires) << "Expiration not interpolated to 60s"; + auto now = util::now(); + EXPECT_LT(now + Seconds(40), *res.expires) << "Expiration not interpolated to 60s"; + EXPECT_GT(now + Seconds(80), *res.expires) << "Expiration not interpolated to 60s"; - loop.stop(); - } break; + loop.stop(); + } break; } }); @@ -197,37 +195,31 @@ TEST(OnlineFileSource, TEST_REQUIRES_SERVER(RetryOnClockSkew)) { TEST(OnlineFileSource, TEST_REQUIRES_SERVER(RespectPriorExpires)) { util::RunLoop loop; - OnlineFileSource fs; + std::unique_ptr fs = std::make_unique(); // Very long expiration time, should never arrive. Resource resource1{ Resource::Unknown, "http://127.0.0.1:3000/test" }; resource1.priorExpires = util::now() + Seconds(100000); - std::unique_ptr req1 = fs.request(resource1, [&](Response) { - FAIL() << "Should never be called"; - }); + std::unique_ptr req1 = fs->request(resource1, [&](Response) { FAIL() << "Should never be called"; }); // No expiration time, should be requested immediately. Resource resource2{ Resource::Unknown, "http://127.0.0.1:3000/test" }; - std::unique_ptr req2 = fs.request(resource2, [&](Response) { - loop.stop(); - }); + std::unique_ptr req2 = fs->request(resource2, [&](Response) { loop.stop(); }); // Very long expiration time, should never arrive. Resource resource3{ Resource::Unknown, "http://127.0.0.1:3000/test" }; resource3.priorExpires = util::now() + Seconds(100000); - std::unique_ptr req3 = fs.request(resource3, [&](Response) { - FAIL() << "Should never be called"; - }); + std::unique_ptr req3 = fs->request(resource3, [&](Response) { FAIL() << "Should never be called"; }); loop.run(); } TEST(OnlineFileSource, TEST_REQUIRES_SERVER(Load)) { util::RunLoop loop; - OnlineFileSource fs; + std::unique_ptr fs = std::make_unique(); const int concurrency = 50; const int max = 10000; @@ -237,24 +229,23 @@ TEST(OnlineFileSource, TEST_REQUIRES_SERVER(Load)) { std::function req = [&](int i) { const auto current = number++; - reqs[i] = fs.request({ Resource::Unknown, - std::string("http://127.0.0.1:3000/load/") + util::toString(current) }, - [&, i, current](Response res) { - reqs[i].reset(); - EXPECT_EQ(nullptr, res.error); - ASSERT_TRUE(res.data.get()); - EXPECT_EQ(std::string("Request ") + util::toString(current), *res.data); - EXPECT_FALSE(bool(res.expires)); - EXPECT_FALSE(res.mustRevalidate); - EXPECT_FALSE(bool(res.modified)); - EXPECT_FALSE(bool(res.etag)); - - if (number <= max) { - req(i); - } else if (current == max) { - loop.stop(); - } - }); + reqs[i] = fs->request({Resource::Unknown, std::string("http://127.0.0.1:3000/load/") + util::toString(current)}, + [&, i, current](Response res) { + reqs[i].reset(); + EXPECT_EQ(nullptr, res.error); + ASSERT_TRUE(res.data.get()); + EXPECT_EQ(std::string("Request ") + util::toString(current), *res.data); + EXPECT_FALSE(bool(res.expires)); + EXPECT_FALSE(res.mustRevalidate); + EXPECT_FALSE(bool(res.modified)); + EXPECT_FALSE(bool(res.etag)); + + if (number <= max) { + req(i); + } else if (current == max) { + loop.stop(); + } + }); }; for (int i = 0; i < concurrency; i++) { @@ -272,21 +263,21 @@ TEST(OnlineFileSource, TEST_REQUIRES_SERVER(Load)) { TEST(OnlineFileSource, TEST_REQUIRES_SERVER(NetworkStatusChange)) { util::RunLoop loop; - OnlineFileSource fs; + std::unique_ptr fs = std::make_unique(); const Resource resource { Resource::Unknown, "http://127.0.0.1:3000/delayed" }; // This request takes 200 milliseconds to answer. - std::unique_ptr req = fs.request(resource, [&](Response res) { - req.reset(); - EXPECT_EQ(nullptr, res.error); - ASSERT_TRUE(res.data.get()); - EXPECT_EQ("Response", *res.data); - EXPECT_FALSE(bool(res.expires)); - EXPECT_FALSE(res.mustRevalidate); - EXPECT_FALSE(bool(res.modified)); - EXPECT_FALSE(bool(res.etag)); - loop.stop(); + std::unique_ptr req = fs->request(resource, [&](Response res) { + req.reset(); + EXPECT_EQ(nullptr, res.error); + ASSERT_TRUE(res.data.get()); + EXPECT_EQ("Response", *res.data); + EXPECT_FALSE(bool(res.expires)); + EXPECT_FALSE(res.mustRevalidate); + EXPECT_FALSE(bool(res.modified)); + EXPECT_FALSE(bool(res.etag)); + loop.stop(); }); // After 50 milliseconds, we're going to trigger a NetworkStatus change. @@ -302,14 +293,14 @@ TEST(OnlineFileSource, TEST_REQUIRES_SERVER(NetworkStatusChange)) { // reachability issues. TEST(OnlineFileSource, TEST_REQUIRES_SERVER(NetworkStatusChangePreempt)) { util::RunLoop loop; - OnlineFileSource fs; - fs.pause(); + std::unique_ptr fs = std::make_unique(); + fs->pause(); const auto start = Clock::now(); int counter = 0; const Resource resource{ Resource::Unknown, "http://127.0.0.1:3001/test" }; - std::unique_ptr req = fs.request(resource, [&](Response res) { + std::unique_ptr req = fs->request(resource, [&](Response res) { const auto duration = std::chrono::duration(Clock::now() - start).count(); if (counter == 0) { EXPECT_GT(0.2, duration) << "Response came in too late"; @@ -339,13 +330,13 @@ TEST(OnlineFileSource, TEST_REQUIRES_SERVER(NetworkStatusChangePreempt)) { mbgl::NetworkStatus::Reachable(); }); - fs.resume(); + fs->resume(); loop.run(); } TEST(OnlineFileSource, TEST_REQUIRES_SERVER(NetworkStatusOnlineOffline)) { util::RunLoop loop; - OnlineFileSource fs; + std::unique_ptr fs = std::make_unique(); const Resource resource { Resource::Unknown, "http://127.0.0.1:3000/test" }; @@ -357,7 +348,7 @@ TEST(OnlineFileSource, TEST_REQUIRES_SERVER(NetworkStatusOnlineOffline)) { NetworkStatus::Set(NetworkStatus::Status::Online); }); - std::unique_ptr req = fs.request(resource, [&](Response res) { + std::unique_ptr req = fs->request(resource, [&](Response res) { req.reset(); EXPECT_EQ(nullptr, res.error); @@ -373,9 +364,9 @@ TEST(OnlineFileSource, TEST_REQUIRES_SERVER(NetworkStatusOnlineOffline)) { TEST(OnlineFileSource, TEST_REQUIRES_SERVER(RateLimitStandard)) { util::RunLoop loop; - OnlineFileSource fs; + std::unique_ptr fs = std::make_unique(); - auto req = fs.request({ Resource::Unknown, "http://127.0.0.1:3000/rate-limit?std=true" }, [&](Response res) { + auto req = fs->request({Resource::Unknown, "http://127.0.0.1:3000/rate-limit?std=true"}, [&](Response res) { ASSERT_NE(nullptr, res.error); EXPECT_EQ(Response::Error::Reason::RateLimit, res.error->reason); ASSERT_EQ(true, bool(res.error->retryAfter)); @@ -388,9 +379,9 @@ TEST(OnlineFileSource, TEST_REQUIRES_SERVER(RateLimitStandard)) { TEST(OnlineFileSource, TEST_REQUIRES_SERVER(RateLimitMBX)) { util::RunLoop loop; - OnlineFileSource fs; + std::unique_ptr fs = std::make_unique(); - auto req = fs.request({ Resource::Unknown, "http://127.0.0.1:3000/rate-limit?mbx=true" }, [&](Response res) { + auto req = fs->request({Resource::Unknown, "http://127.0.0.1:3000/rate-limit?mbx=true"}, [&](Response res) { ASSERT_NE(nullptr, res.error); EXPECT_EQ(Response::Error::Reason::RateLimit, res.error->reason); ASSERT_EQ(true, bool(res.error->retryAfter)); @@ -403,9 +394,9 @@ TEST(OnlineFileSource, TEST_REQUIRES_SERVER(RateLimitMBX)) { TEST(OnlineFileSource, TEST_REQUIRES_SERVER(RateLimitDefault)) { util::RunLoop loop; - OnlineFileSource fs; + std::unique_ptr fs = std::make_unique(); - auto req = fs.request({ Resource::Unknown, "http://127.0.0.1:3000/rate-limit" }, [&](Response res) { + auto req = fs->request({Resource::Unknown, "http://127.0.0.1:3000/rate-limit"}, [&](Response res) { ASSERT_NE(nullptr, res.error); EXPECT_EQ(Response::Error::Reason::RateLimit, res.error->reason); ASSERT_FALSE(res.error->retryAfter); @@ -417,46 +408,46 @@ TEST(OnlineFileSource, TEST_REQUIRES_SERVER(RateLimitDefault)) { TEST(OnlineFileSource, GetBaseURLAndAccessTokenWhilePaused) { util::RunLoop loop; - OnlineFileSource fs; + std::unique_ptr fs = std::make_unique(); - fs.pause(); + fs->pause(); auto baseURL = "http://url"; auto accessToken = "access_token"; - fs.setProperty(API_BASE_URL_KEY, baseURL); - fs.setProperty(ACCESS_TOKEN_KEY, accessToken); + fs->setProperty(API_BASE_URL_KEY, baseURL); + fs->setProperty(ACCESS_TOKEN_KEY, accessToken); - EXPECT_EQ(*fs.getProperty(API_BASE_URL_KEY).getString(), baseURL); - EXPECT_EQ(*fs.getProperty(ACCESS_TOKEN_KEY).getString(), accessToken); + EXPECT_EQ(*fs->getProperty(API_BASE_URL_KEY).getString(), baseURL); + EXPECT_EQ(*fs->getProperty(ACCESS_TOKEN_KEY).getString(), accessToken); } TEST(OnlineFileSource, ChangeAPIBaseURL){ util::RunLoop loop; - OnlineFileSource fs; + std::unique_ptr fs = std::make_unique(); - EXPECT_EQ(mbgl::util::API_BASE_URL, *fs.getProperty(API_BASE_URL_KEY).getString()); + EXPECT_EQ(mbgl::util::API_BASE_URL, *fs->getProperty(API_BASE_URL_KEY).getString()); const std::string customURL = "test.domain"; - fs.setProperty(API_BASE_URL_KEY, customURL); - EXPECT_EQ(customURL, *fs.getProperty(API_BASE_URL_KEY).getString()); + fs->setProperty(API_BASE_URL_KEY, customURL); + EXPECT_EQ(customURL, *fs->getProperty(API_BASE_URL_KEY).getString()); } TEST(OnlineFileSource, TEST_REQUIRES_SERVER(LowHighPriorityRequests)) { util::RunLoop loop; - OnlineFileSource fs; + std::unique_ptr fs = std::make_unique(); std::size_t response_counter = 0; const std::size_t NUM_REQUESTS = 3; NetworkStatus::Set(NetworkStatus::Status::Offline); - fs.setProperty(MAX_CONCURRENT_REQUESTS_KEY, 1u); + fs->setProperty(MAX_CONCURRENT_REQUESTS_KEY, 1u); // After DefaultFileSource was split, OnlineFileSource lives on a separate // thread. Pause OnlineFileSource, so that messages are queued for processing. - fs.pause(); + fs->pause(); // First regular request. Resource regular1{Resource::Unknown, "http://127.0.0.1:3000/load/1"}; - std::unique_ptr req_0 = fs.request(regular1, [&](Response) { + std::unique_ptr req_0 = fs->request(regular1, [&](Response) { response_counter++; req_0.reset(); }); @@ -464,7 +455,7 @@ TEST(OnlineFileSource, TEST_REQUIRES_SERVER(LowHighPriorityRequests)) { // Low priority request that will be queued and should be requested last. Resource low_prio{Resource::Unknown, "http://127.0.0.1:3000/load/2"}; low_prio.setPriority(Resource::Priority::Low); - std::unique_ptr req_1 = fs.request(low_prio, [&](Response) { + std::unique_ptr req_1 = fs->request(low_prio, [&](Response) { response_counter++; req_1.reset(); EXPECT_EQ(response_counter, NUM_REQUESTS); // make sure this is responded last @@ -473,12 +464,12 @@ TEST(OnlineFileSource, TEST_REQUIRES_SERVER(LowHighPriorityRequests)) { // Second regular priority request that should de-preoritize low priority request. Resource regular2{ Resource::Unknown, "http://127.0.0.1:3000/load/3" }; - std::unique_ptr req_2 = fs.request(regular2, [&](Response) { + std::unique_ptr req_2 = fs->request(regular2, [&](Response) { response_counter++; req_2.reset(); }); - fs.resume(); + fs->resume(); NetworkStatus::Set(NetworkStatus::Status::Online); loop.run(); } @@ -486,33 +477,34 @@ TEST(OnlineFileSource, TEST_REQUIRES_SERVER(LowHighPriorityRequests)) { TEST(OnlineFileSource, TEST_REQUIRES_SERVER(LowHighPriorityRequestsMany)) { util::RunLoop loop; - OnlineFileSource fs; + std::unique_ptr fs = std::make_unique(); int response_counter = 0; int correct_low = 0; int correct_regular = 0; NetworkStatus::Set(NetworkStatus::Status::Offline); - fs.setProperty(MAX_CONCURRENT_REQUESTS_KEY, 1u); - fs.pause(); + fs->setProperty(MAX_CONCURRENT_REQUESTS_KEY, 1u); + fs->pause(); std::vector> collector; for (int num_reqs = 0; num_reqs < 20; num_reqs++) { if (num_reqs % 2 == 0) { - std::unique_ptr req = fs.request({ Resource::Unknown, "http://127.0.0.1:3000/load/" + std::to_string(num_reqs) }, [&](Response) { - response_counter++; + std::unique_ptr req = fs->request( + {Resource::Unknown, "http://127.0.0.1:3000/load/" + std::to_string(num_reqs)}, [&](Response) { + response_counter++; - if (response_counter <= 10) { // count the high priority requests that arrive late correctly - correct_regular++; - } - }); + if (response_counter <= 10) { // count the high priority requests that arrive late correctly + correct_regular++; + } + }); collector.push_back(std::move(req)); } else { Resource resource = { Resource::Unknown, "http://127.0.0.1:3000/load/" + std::to_string(num_reqs) }; resource.setPriority(Resource::Priority::Low); - std::unique_ptr req = fs.request(std::move(resource), [&](Response) { + std::unique_ptr req = fs->request(std::move(resource), [&](Response) { response_counter++; if (response_counter > 10) { // count the low priority requests that arrive late correctly @@ -533,30 +525,30 @@ TEST(OnlineFileSource, TEST_REQUIRES_SERVER(LowHighPriorityRequestsMany)) { } } - fs.resume(); + fs->resume(); NetworkStatus::Set(NetworkStatus::Status::Online); loop.run(); } TEST(OnlineFileSource, TEST_REQUIRES_SERVER(MaximumConcurrentRequests)) { util::RunLoop loop; - OnlineFileSource fs; + std::unique_ptr fs = std::make_unique(); - ASSERT_EQ(*fs.getProperty(MAX_CONCURRENT_REQUESTS_KEY).getUint(), 20u); + ASSERT_EQ(*fs->getProperty(MAX_CONCURRENT_REQUESTS_KEY).getUint(), 20u); - fs.setProperty(MAX_CONCURRENT_REQUESTS_KEY, 10u); - ASSERT_EQ(*fs.getProperty(MAX_CONCURRENT_REQUESTS_KEY).getUint(), 10u); + fs->setProperty(MAX_CONCURRENT_REQUESTS_KEY, 10u); + ASSERT_EQ(*fs->getProperty(MAX_CONCURRENT_REQUESTS_KEY).getUint(), 10u); } TEST(OnlineFileSource, TEST_REQUIRES_SERVER(RequestSameUrlMultipleTimes)) { util::RunLoop loop; - OnlineFileSource fs; + std::unique_ptr fs = std::make_unique(); int count = 0; std::vector> requests; for (int i = 0; i < 100; ++i) { - requests.emplace_back(fs.request({ Resource::Unknown, "http://127.0.0.1:3000/load" }, [&](Response) { + requests.emplace_back(fs->request({Resource::Unknown, "http://127.0.0.1:3000/load"}, [&](Response) { if (++count == 100) { loop.stop(); } From ac5eda8f3dcf192fc9ae319927ffc5de46292281 Mon Sep 17 00:00:00 2001 From: Mikhail Pozdnyakov Date: Wed, 26 Feb 2020 18:19:45 +0200 Subject: [PATCH 6/7] Add change log entry --- CHANGELOG.md | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d65b89eb38c..51779018ed0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,10 +16,34 @@ This is a back porting from GL JS [#9333](https://github.com/mapbox/mapbox-gl-js/pull/9333) +### ✨ New features + - [core] Add Layer::serialize() method ([#16231](https://github.com/mapbox/mapbox-gl-native/pull/16231)) New method allows serialization of a layer into a Value type, including all modifications done via runtime style API. New method is also an enabler for Style object serialization (sources, layers, etc). +##### ⚠️ Breaking changes + +- Changes to `mbgl::FileSourceManager::getFileSource()` ([#16238](https://github.com/mapbox/mapbox-gl-native/pull/16238)) + + It returns now `mbgl::PassRefPtr` (previously was `std::shared_ptr`) in order to enforce keeping the strong reference to the returned object. + + Breaking code example: + `auto fs = FileSourceManager::getFileSource(); fs->..` + + Posible fix: + `std::shared_ptr fs = `; + +- The `mbgl::OnlineFileSource` class cannot be used directly ([#16238](https://github.com/mapbox/mapbox-gl-native/pull/16238)) + + Clients must use the parent `mbgl::FileSource` interface instead. + + Breaking code example: + `std::shared_ptr onlineSource = std::static_pointer_cast(FileSourceManager::get()->getFileSource(..));` + + Possible fix: + `std::shared_ptr onlineSource = FileSourceManager::get()->getFileSource(..);` + ## maps-v1.2.0 (2020.02-release-vanillashake) ### ✨ New features From 22a1a846c6142c52d19f151319d75ebe94efb5b9 Mon Sep 17 00:00:00 2001 From: Mikhail Pozdnyakov Date: Wed, 26 Feb 2020 21:50:41 +0200 Subject: [PATCH 7/7] Add Pass.NoCopy unit test --- test/CMakeLists.txt | 1 + test/util/pass.test.cpp | 16 ++++++++++++++++ 2 files changed, 17 insertions(+) create mode 100644 test/util/pass.test.cpp diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index a9ae0ac8748..75f91762a72 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -97,6 +97,7 @@ add_library( ${PROJECT_SOURCE_DIR}/test/util/merge_lines.test.cpp ${PROJECT_SOURCE_DIR}/test/util/number_conversions.test.cpp ${PROJECT_SOURCE_DIR}/test/util/offscreen_texture.test.cpp + ${PROJECT_SOURCE_DIR}/test/util/pass.test.cpp ${PROJECT_SOURCE_DIR}/test/util/position.test.cpp ${PROJECT_SOURCE_DIR}/test/util/projection.test.cpp ${PROJECT_SOURCE_DIR}/test/util/run_loop.test.cpp diff --git a/test/util/pass.test.cpp b/test/util/pass.test.cpp new file mode 100644 index 00000000000..10651f854b4 --- /dev/null +++ b/test/util/pass.test.cpp @@ -0,0 +1,16 @@ +#include + +#include + +TEST(Pass, NoCopy) { + static int copyCount = 0; + struct A { + A() = default; + A(A&&) = default; + A(const A&) { ++copyCount; } + static mbgl::Pass create() { return A(); } + }; + A a = A::create(); + (void)a; + EXPECT_EQ(0, copyCount); +} \ No newline at end of file