[core] Refactor to instance methods on OnlineFileRequestImpl

2 files changed, 107 insertions, 106 deletions

diff --git a/include/mbgl/storage/online_file_source.hpp b/include/mbgl/storage/online_file_source.hpp
index 357b1773d7..2164d40032 100644
--- a/include/mbgl/storage/online_file_source.hpp
+++ b/include/mbgl/storage/online_file_source.hpp
@@ -22,6 +22,8 @@ public:
 
 private:
     friend class OnlineFileRequest;
+    friend class OnlineFileRequestImpl;
+
     void cancel(const Resource&, FileRequest*);
 
     class Impl;
diff --git a/platform/default/online_file_source.cpp b/platform/default/online_file_source.cpp
index 755c5e590d..668c466732 100644
--- a/platform/default/online_file_source.cpp
+++ b/platform/default/online_file_source.cpp
@@ -47,27 +47,29 @@ class OnlineFileRequestImpl : public util::noncopyable {
 public:
     using Callback = std::function<void (Response)>;
 
-    const Resource resource;
-    std::unique_ptr<WorkRequest> cacheRequest;
-    RequestBase* realRequest = nullptr;
-    util::Timer realRequestTimer;
-
-    inline OnlineFileRequestImpl(const Resource& resource_)
-        : resource(resource_) {}
-
+    OnlineFileRequestImpl(const Resource&);
     ~OnlineFileRequestImpl();
 
     // Observer accessors.
-    void addObserver(FileRequest*, Callback);
+    void addObserver(FileRequest*, Callback, OnlineFileSource::Impl&);
     void removeObserver(FileRequest*);
     bool hasObservers() const;
 
+    void networkIsReachableAgain(OnlineFileSource::Impl&);
+
+private:
+    void startCacheRequest(OnlineFileSource::Impl&);
+    void startRealRequest(OnlineFileSource::Impl&);
+    void reschedule(OnlineFileSource::Impl&);
+
+    const Resource resource;
+    std::unique_ptr<WorkRequest> cacheRequest;
+    RequestBase* realRequest = nullptr;
+    util::Timer realRequestTimer;
+
     // Set the response of this request object and notify all observers.
     void setResponse(const std::shared_ptr<const Response>&);
 
-    // Get the response of this request object.
-    const std::shared_ptr<const Response>& getResponse() const;
-
     // Returns the seconds we have to wait until we need to redo this request. A value of 0
     // means that we need to redo it immediately, and a negative value means that we're not setting
     // a timeout at all.
@@ -77,7 +79,6 @@ public:
     // stale flag set, if the response is expired.
     void checkResponseFreshness();
 
-private:
     // Stores a set of all observing Request objects.
     std::unordered_map<FileRequest*, Callback> observers;
 
@@ -104,9 +105,7 @@ public:
     void cancel(Resource, FileRequest*);
 
 private:
-    void startCacheRequest(OnlineFileRequestImpl&);
-    void startRealRequest(OnlineFileRequestImpl&);
-    void reschedule(OnlineFileRequestImpl&);
+    friend OnlineFileRequestImpl;
 
     std::unordered_map<Resource, std::unique_ptr<OnlineFileRequestImpl>, Resource::Hash> pending;
     FileCache* const cache;
@@ -184,13 +183,7 @@ OnlineFileSource::Impl::~Impl() {
 
 void OnlineFileSource::Impl::networkIsReachableAgain() {
     for (auto& req : pending) {
-        auto& request = *req.second;
-        auto& response = request.getResponse();
-        if (!request.realRequest && response && response->error && response->error->reason == Response::Error::Reason::Connection) {
-            // We need all requests to fail at least once before we are going to start retrying
-            // them, and we only immediately restart request that failed due to connection issues.
-            startRealRequest(request);
-        }
+        req.second->networkIsReachableAgain(*this);
     }
 }
 
@@ -198,149 +191,159 @@ void OnlineFileSource::Impl::add(Resource resource, FileRequest* req, Callback c
     auto& request = *pending.emplace(resource,
         std::make_unique<OnlineFileRequestImpl>(resource)).first->second;
 
-    if (request.getResponse()) {
+    // Add this request as an observer so that it'll get notified when something about this
+    // request changes.
+    request.addObserver(req, callback, *this);
+}
+
+void OnlineFileSource::Impl::cancel(Resource resource, FileRequest* req) {
+    auto it = pending.find(resource);
+    if (it != pending.end()) {
+        // If the number of dependent requests of the OnlineFileRequest drops to zero,
+        // cancel the request and remove it from the pending list.
+        auto& request = *it->second;
+        request.removeObserver(req);
+        if (!request.hasObservers()) {
+            pending.erase(it);
+        }
+    } else {
+        // There is no request for this URL anymore. Likely, the request already completed
+        // before we got around to process the cancelation request.
+    }
+}
+
+// ----- OnlineFileRequest -----
+
+OnlineFileRequestImpl::OnlineFileRequestImpl(const Resource& resource_)
+    : resource(resource_) {
+}
+
+OnlineFileRequestImpl::~OnlineFileRequestImpl() {
+    if (realRequest) {
+        realRequest->cancel();
+        realRequest = nullptr;
+    }
+    // realRequestTimer and cacheRequest are automatically canceled upon destruction.
+}
+
+void OnlineFileRequestImpl::addObserver(FileRequest* req, Callback callback, OnlineFileSource::Impl& impl) {
+    if (response) {
         // We've at least obtained a cache value, potentially we also got a final response.
         // Before returning the existing response, make sure that it is still fresh, or update the
         // `stale` flag.
-        request.checkResponseFreshness();
-        callback(*request.getResponse());
+        checkResponseFreshness();
+        callback(*response);
 
-        if (request.getResponse()->stale && !request.realRequest) {
+        if (response->stale && !realRequest) {
             // We've returned a stale response; now make sure the requester also gets a fresh
             // response eventually. It's possible that there's already a request in progress.
             // Note that this will also trigger updates to all other existing listeners.
             // Since we already have data, we're going to verify
-            startRealRequest(request);
+            startRealRequest(impl);
         } else {
             // The response is still fresh (or there's already a request for refreshing the resource
             // in progress), so there's nothing we need to do.
         }
-    } else if (!request.cacheRequest && !request.realRequest) {
+    } else if (!cacheRequest && !realRequest) {
         // There is no request in progress, and we don't have a response yet. This means we'll have
         // to start the request ourselves.
-        if (cache && !util::isAssetURL(request.resource.url)) {
-            startCacheRequest(request);
+        if (impl.cache && !util::isAssetURL(resource.url)) {
+            startCacheRequest(impl);
         } else {
-            startRealRequest(request);
+            startRealRequest(impl);
         }
     } else {
         // There is a request in progress. We just have to wait.
     }
 
-    // Add this request as an observer so that it'll get notified when something about this
-    // request changes.
-    request.addObserver(req, callback);
+    observers.emplace(req, callback);
+}
+
+void OnlineFileRequestImpl::removeObserver(FileRequest* req) {
+    observers.erase(req);
 }
 
-void OnlineFileSource::Impl::startCacheRequest(OnlineFileRequestImpl& request) {
+bool OnlineFileRequestImpl::hasObservers() const {
+    return !observers.empty();
+}
+
+void OnlineFileRequestImpl::startCacheRequest(OnlineFileSource::Impl& impl) {
     // Check the cache for existing data so that we can potentially
     // revalidate the information without having to redownload everything.
-    request.cacheRequest =
-        cache->get(request.resource, [this, &request](std::shared_ptr<Response> response) {
-            request.cacheRequest = nullptr;
-            if (response) {
-                response->stale = response->isExpired();
-                request.setResponse(response);
+    cacheRequest =
+        impl.cache->get(resource, [this, &impl](std::shared_ptr<Response> response_) {
+            cacheRequest = nullptr;
+            if (response_) {
+                response_->stale = response_->isExpired();
+                setResponse(response_);
             }
 
-            if (!response || response->stale) {
+            if (!response_ || response_->stale) {
                 // No response or stale cache. Run the real request.
-                startRealRequest(request);
+                startRealRequest(impl);
             }
 
-            reschedule(request);
+            reschedule(impl);
         });
 }
 
-void OnlineFileSource::Impl::startRealRequest(OnlineFileRequestImpl& request) {
-    assert(!request.realRequest);
+void OnlineFileRequestImpl::startRealRequest(OnlineFileSource::Impl& impl) {
+    assert(!realRequest);
 
     // Ensure the timer is stopped.
-    request.realRequestTimer.stop();
+    realRequestTimer.stop();
 
-    auto callback = [this, &request](std::shared_ptr<const Response> response) {
-        request.realRequest = nullptr;
+    auto callback = [this, &impl](std::shared_ptr<const Response> response_) {
+        realRequest = nullptr;
 
         // Only update the cache for successful or 404 responses.
         // In particular, we don't want to write a Canceled request, or one that failed due to
         // connection errors to the cache. Server errors are hopefully also temporary, so we're not
         // caching them either.
-        if (cache && !util::isAssetURL(request.resource.url) &&
-            (!response->error || (response->error->reason == Response::Error::Reason::NotFound))) {
+        if (impl.cache && !util::isAssetURL(resource.url) &&
+            (!response_->error || (response_->error->reason == Response::Error::Reason::NotFound))) {
             // Store response in database. Make sure we only refresh the expires column if the data
             // didn't change.
             FileCache::Hint hint = FileCache::Hint::Full;
-            if (request.getResponse() && response->data == request.getResponse()->data) {
+            if (response && response_->data == response->data) {
                 hint = FileCache::Hint::Refresh;
             }
-            cache->put(request.resource, response, hint);
+            impl.cache->put(resource, response_, hint);
         }
 
-        request.setResponse(response);
-        reschedule(request);
+        setResponse(response_);
+        reschedule(impl);
     };
 
-    if (util::isAssetURL(request.resource.url)) {
-        request.realRequest =
-            assetContext->createRequest(request.resource.url, callback, assetRoot);
+    if (util::isAssetURL(resource.url)) {
+        realRequest = impl.assetContext->createRequest(resource.url, callback, impl.assetRoot);
     } else {
-        request.realRequest =
-            httpContext->createRequest(request.resource.url, callback, request.getResponse());
+        realRequest = impl.httpContext->createRequest(resource.url, callback, response);
     }
 }
 
-void OnlineFileSource::Impl::cancel(Resource resource, FileRequest* req) {
-    auto it = pending.find(resource);
-    if (it != pending.end()) {
-        // If the number of dependent requests of the OnlineFileRequest drops to zero,
-        // cancel the request and remove it from the pending list.
-        auto& request = *it->second;
-        request.removeObserver(req);
-        if (!request.hasObservers()) {
-            pending.erase(it);
-        }
-    } else {
-        // There is no request for this URL anymore. Likely, the request already completed
-        // before we got around to process the cancelation request.
-    }
-}
-
-void OnlineFileSource::Impl::reschedule(OnlineFileRequestImpl& request) {
-    if (request.realRequest) {
+void OnlineFileRequestImpl::reschedule(OnlineFileSource::Impl& impl) {
+    if (realRequest) {
         // There's already a request in progress; don't start another one.
         return;
     }
 
-    const Seconds timeout = request.getRetryTimeout();
+    const Seconds timeout = getRetryTimeout();
 
     if (timeout >= Seconds::zero()) {
-        request.realRequestTimer.start(timeout, Duration::zero(), [this, &request] {
-            assert(!request.realRequest);
-            startRealRequest(request);
+        realRequestTimer.start(timeout, Duration::zero(), [this, &impl] {
+            assert(!realRequest);
+            startRealRequest(impl);
         });
     }
 }
 
-// ----- OnlineFileRequest -----
-
-OnlineFileRequestImpl::~OnlineFileRequestImpl() {
-    if (realRequest) {
-        realRequest->cancel();
-        realRequest = nullptr;
+void OnlineFileRequestImpl::networkIsReachableAgain(OnlineFileSource::Impl& impl) {
+    // We need all requests to fail at least once before we are going to start retrying
+    // them, and we only immediately restart request that failed due to connection issues.
+    if (!realRequest && response && response->error && response->error->reason == Response::Error::Reason::Connection) {
+        startRealRequest(impl);
     }
-    // realRequestTimer and cacheRequest are automatically canceled upon destruction.
-}
-
-void OnlineFileRequestImpl::addObserver(FileRequest* req, Callback callback) {
-    observers.emplace(req, callback);
-}
-
-void OnlineFileRequestImpl::removeObserver(FileRequest* req) {
-    observers.erase(req);
-}
-
-bool OnlineFileRequestImpl::hasObservers() const {
-    return !observers.empty();
 }
 
 void OnlineFileRequestImpl::setResponse(const std::shared_ptr<const Response>& response_) {
@@ -359,10 +362,6 @@ void OnlineFileRequestImpl::setResponse(const std::shared_ptr<const Response>& r
     }
 }
 
-const std::shared_ptr<const Response>& OnlineFileRequestImpl::getResponse() const {
-    return response;
-}
-
 Seconds OnlineFileRequestImpl::getRetryTimeout() const {
     Seconds timeout = Seconds::zero();