[core] FileSource API improvements

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<FileSource>` (previously was `std::shared_ptr<FileSource>`) 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<FileSource> 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<OnlineFileSource> onlineSource = std::static_pointer_cast<OnlineFileSource>(FileSourceManager::get()->getFileSource(..));`
+
+  Possible fix:
+  `std::shared_ptr<FileSource> onlineSource = FileSourceManager::get()->getFileSource(..);`
+
 ## maps-v1.2.0 (2020.02-release-vanillashake)
 
 ### ✨ New features

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

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<mbgl::FileSource> dbfs = mbgl::FileSourceManager::get()->getFileSource(
         mbgl::FileSourceType::Database, mbgl::ResourceOptions().withCachePath(args::get(cacheValue)));
     dbfs->forward(resource, response, [&loop] { loop.stop(); });
     loop.run();

bin/offline.cpp

@@ -163,10 +163,10 @@ int main(int argc, char *argv[]) {
 
 
     util::RunLoop loop;
-    std::shared_ptr<DatabaseFileSource> fileSource =
-        std::static_pointer_cast<DatabaseFileSource>(FileSourceManager::get()->getFileSource(
+    std::shared_ptr<DatabaseFileSource> fileSource = std::static_pointer_cast<DatabaseFileSource>(
+        std::shared_ptr<FileSource>(FileSourceManager::get()->getFileSource(
             FileSourceType::Database,
-            ResourceOptions().withAccessToken(token).withBaseURL(apiBaseURL).withCachePath(output)));
+            ResourceOptions().withAccessToken(token).withBaseURL(apiBaseURL).withCachePath(output))));
 
     std::unique_ptr<OfflineRegion> region;
 

include/mbgl/actor/scheduler.hpp

@@ -1,5 +1,7 @@
 #pragma once
 
+#include <mbgl/util/pass_types.hpp>
+
 #include <mapbox/weak.hpp>
 
 #include <functional>
@@ -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 <typename T>
-class Pass {
-public:
-    Pass(T&& obj_) : obj(std::forward<T>(obj_)) {}
-    Pass(Pass&&) = default;
-    Pass(const Pass&) = delete;
-    operator T() && { return std::move(obj); }
-
-private:
-    T obj;
-};
-
-template <typename T>
-using PassRefPtr = Pass<std::shared_ptr<T>>;
-
 /*
     A `Scheduler` is responsible for coordinating the processing of messages by
     one or more actors via their mailboxes. It's an abstract interface. Currently,

include/mbgl/storage/file_source.hpp

@@ -1,5 +1,6 @@
 #pragma once
 
+#include <mbgl/storage/resource_transform.hpp>
 #include <mbgl/storage/response.hpp>
 
 #include <mapbox/value.hpp>
@@ -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;
 };

include/mbgl/storage/file_source_manager.hpp

@@ -1,6 +1,7 @@
 #pragma once
 
 #include <mbgl/storage/file_source.hpp>
+#include <mbgl/util/pass_types.hpp>
 
 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<FileSource> getFileSource(FileSourceType, const ResourceOptions&) noexcept;
+    PassRefPtr<FileSource> 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.

include/mbgl/storage/online_file_source.hpp

@@ -5,26 +5,21 @@
 
 namespace mbgl {
 
-class ResourceTransform;
-
 class OnlineFileSource : public FileSource {
 public:
     OnlineFileSource();
     ~OnlineFileSource() override;
 
+private:
     // FileSource overrides
     std::unique_ptr<AsyncRequest> request(const Resource&, Callback) override;
     bool canRequest(const Resource&) const override;
     void pause() override;
     void resume() override;
     void setProperty(const std::string&, const mapbox::base::Value&) override;
     mapbox::base::Value getProperty(const std::string&) const override;
+    void setResourceTransform(ResourceTransform) override;
 
-    // OnlineFileSource interface.
-    // TODO: Would be nice to drop it to get uniform interface.
-    virtual void setResourceTransform(ResourceTransform);
-
-private:
     class Impl;
     const std::unique_ptr<Impl> impl;
 };

include/mbgl/util/pass_types.hpp

@@ -0,0 +1,24 @@
+#pragma once
+
+#include <memory>
+
+namespace mbgl {
+
+// Using this type as a return type enforces the client to retain the returned object.
+template <typename T>
+class Pass {
+public:
+    Pass(T obj_) : obj(std::move(obj_)) {}
+    Pass(Pass&&) noexcept = default;
+    Pass(const Pass&) = delete;
+    Pass& operator=(const Pass&) = delete;
+    operator T() && { return std::move(obj); }
+
+private:
+    T obj;
+};
+
+template <typename T>
+using PassRefPtr = Pass<std::shared_ptr<T>>;
+
+} // namespace mbgl

platform/android/src/file_source.cpp

@@ -41,10 +41,9 @@ 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::DatabaseFileSource>(
-        mbgl::FileSourceManager::get()->getFileSource(mbgl::FileSourceType::Database, resourceOptions));
-    onlineSource = std::static_pointer_cast<mbgl::OnlineFileSource>(
-        mbgl::FileSourceManager::get()->getFileSource(mbgl::FileSourceType::Network, resourceOptions));
+    databaseSource = std::static_pointer_cast<mbgl::DatabaseFileSource>(std::shared_ptr<mbgl::FileSource>(
+        mbgl::FileSourceManager::get()->getFileSource(mbgl::FileSourceType::Database, resourceOptions)));
+    onlineSource = mbgl::FileSourceManager::get()->getFileSource(mbgl::FileSourceType::Network, resourceOptions);
 }
 
 FileSource::~FileSource() {

platform/android/src/file_source.hpp

@@ -75,7 +75,7 @@ class FileSource {
     std::unique_ptr<Actor<ResourceTransform::TransformCallback>> resourceTransform;
     std::function<void()> pathChangeCallback;
     std::shared_ptr<mbgl::DatabaseFileSource> databaseSource;
-    std::shared_ptr<mbgl::OnlineFileSource> onlineSource;
+    std::shared_ptr<mbgl::FileSource> onlineSource;
     std::shared_ptr<mbgl::FileSource> resourceLoader;
 };
 

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<FileSource>& jFileSource)
-    : fileSource(std::static_pointer_cast<mbgl::DatabaseFileSource>(mbgl::FileSourceManager::get()->getFileSource(
-          mbgl::FileSourceType::Database, FileSource::getSharedResourceOptions(env, jFileSource)))) {
+    : fileSource(std::static_pointer_cast<mbgl::DatabaseFileSource>(
+          std::shared_ptr<mbgl::FileSource>(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.");
     }

platform/android/src/offline/offline_region.cpp

@@ -16,8 +16,9 @@ namespace android {
 
 OfflineRegion::OfflineRegion(jni::JNIEnv& env, jni::jlong offlineRegionPtr, const jni::Object<FileSource>& jFileSource)
     : region(reinterpret_cast<mbgl::OfflineRegion*>(offlineRegionPtr)),
-      fileSource(std::static_pointer_cast<mbgl::DatabaseFileSource>(mbgl::FileSourceManager::get()->getFileSource(
-          mbgl::FileSourceType::Database, FileSource::getSharedResourceOptions(env, jFileSource)))) {
+      fileSource(std::static_pointer_cast<mbgl::DatabaseFileSource>(
+          std::shared_ptr<mbgl::FileSource>(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.");
     }

platform/darwin/src/MGLOfflineStorage.mm

@@ -46,7 +46,7 @@ @interface MGLOfflineStorage ()
 
 @property (nonatomic, strong, readwrite) NSMutableArray<MGLOfflinePack *> *packs;
 @property (nonatomic) std::shared_ptr<mbgl::DatabaseFileSource> mbglDatabaseFileSource;
-@property (nonatomic) std::shared_ptr<mbgl::OnlineFileSource> mbglOnlineFileSource;
+@property (nonatomic) std::shared_ptr<mbgl::FileSource> mbglOnlineFileSource;
 @property (nonatomic) std::shared_ptr<mbgl::FileSource> mbglFileSource;
 @property (nonatomic) std::string mbglCachePath;
 @property (nonatomic, getter=isPaused) BOOL paused;
@@ -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::OnlineFileSource>(mbgl::FileSourceManager::get()->getFileSource(mbgl::FileSourceType::Network, options));
-        _mbglDatabaseFileSource = std::static_pointer_cast<mbgl::DatabaseFileSource>(mbgl::FileSourceManager::get()->getFileSource(mbgl::FileSourceType::Database, options));
+        _mbglOnlineFileSource = mbgl::FileSourceManager::get()->getFileSource(mbgl::FileSourceType::Network, options);
+        _mbglDatabaseFileSource = std::static_pointer_cast<mbgl::DatabaseFileSource>(std::shared_ptr<mbgl::FileSource>(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

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<mbgl::OnlineFileSource> mbglOnlineFileSource;
+@property (nonatomic) std::shared_ptr<mbgl::FileSource> mbglOnlineFileSource;
 
 /**
  The shared resource loader file source object owned by the shared offline storage object.

platform/default/src/mbgl/storage/file_source_manager.cpp

@@ -27,7 +27,7 @@ class DefaultFileSourceManagerImpl final : public FileSourceManager {
                                   [](const ResourceOptions&) { return std::make_unique<LocalFileSource>(); });
 
         registerFileSourceFactory(FileSourceType::Network, [](const ResourceOptions& options) {
-            auto networkSource = std::make_unique<OnlineFileSource>();
+            std::unique_ptr<FileSource> networkSource = std::make_unique<OnlineFileSource>();
             networkSource->setProperty(ACCESS_TOKEN_KEY, options.accessToken());
             networkSource->setProperty(API_BASE_URL_KEY, options.baseURL());
             return networkSource;

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<mbgl::FileSource> 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<mbgl::FileSource> 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::DatabaseFileSource>(
-        mbgl::FileSourceManager::get()->getFileSource(mbgl::FileSourceType::Database, resourceOptions));
+    auto databaseFileSource = std::static_pointer_cast<mbgl::DatabaseFileSource>(std::shared_ptr<mbgl::FileSource>(
+        mbgl::FileSourceManager::get()->getFileSource(mbgl::FileSourceType::Database, resourceOptions)));
     view->setResetCacheCallback([databaseFileSource]() {
         databaseFileSource->resetDatabase([](std::exception_ptr ex) {
             if (ex) {

platform/qt/src/qmapboxgl.cpp

@@ -1751,8 +1751,9 @@ 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::static_pointer_cast<mbgl::OnlineFileSource>(fs)->setResourceTransform(std::move(transform));
+         std::shared_ptr<mbgl::FileSource> fs =
+             mbgl::FileSourceManager::get()->getFileSource(mbgl::FileSourceType::Network, resourceOptions);
+         fs->setResourceTransform(std::move(transform));
      }
 
     // Needs to be Queued to give time to discard redundant draw calls via the `renderQueued` flag.

render-test/file_source.cpp

@@ -19,7 +19,7 @@ ProxyFileSource::ProxyFileSource(std::shared_ptr<FileSource> defaultResourceLoad
     : defaultResourceLoader(std::move(defaultResourceLoader_)) {
     assert(defaultResourceLoader);
     if (offline) {
-        auto dbfs = FileSourceManager::get()->getFileSource(FileSourceType::Database, options);
+        std::shared_ptr<FileSource> dbfs = FileSourceManager::get()->getFileSource(FileSourceType::Database, options);
         dbfs->setProperty(READ_ONLY_MODE_KEY, true);
     }
 }

src/mbgl/map/map.cpp

@@ -33,7 +33,9 @@ Map::Map(RendererFrontend& frontend,
     : impl(std::make_unique<Impl>(
           frontend,
           observer,
-          FileSourceManager::get() ? FileSourceManager::get()->getFileSource(ResourceLoader, resourceOptions) : nullptr,
+          FileSourceManager::get()
+              ? std::shared_ptr<FileSource>(FileSourceManager::get()->getFileSource(ResourceLoader, resourceOptions))
+              : nullptr,
           mapOptions)) {}
 
 Map::Map(std::unique_ptr<Impl> impl_) : impl(std::move(impl_)) {}

src/mbgl/storage/file_source_manager.cpp

@@ -30,8 +30,7 @@ FileSourceManager::FileSourceManager() : impl(std::make_unique<Impl>()) {}
 
 FileSourceManager::~FileSourceManager() = default;
 
-std::shared_ptr<FileSource> FileSourceManager::getFileSource(FileSourceType type,
-                                                             const ResourceOptions& options) noexcept {
+PassRefPtr<FileSource> FileSourceManager::getFileSource(FileSourceType type, const ResourceOptions& options) noexcept {
     std::lock_guard<std::recursive_mutex> lock(impl->mutex);
 
     // Remove released file sources.

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

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<FileSource> 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<FileSource> 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<FileSource> 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));

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<FileSource> onlineFs = std::make_unique<OnlineFileSource>();
 };
 
 } // namespace mbgl