Remove RequestBase::start

Requests are always started immediately upon construction.

While here, use typed pointers, not void*.

platform/darwin/http_request_nsurl.mm

@@ -200,7 +200,7 @@ void HTTPRequestImpl::handleResponse() {
         }
 
         context->removeRequest(request);
-        request->ptr = nullptr;
+        request->impl = nullptr;
         delete request;
         request = nullptr;
     }
@@ -243,7 +243,7 @@ HTTPRequestImpl::~HTTPRequestImpl() {
 
     if (request) {
         context->removeRequest(request);
-        request->ptr = nullptr;
+        request->impl = nullptr;
     }
 }
 
@@ -398,24 +398,21 @@ void HTTPRequestImpl::restart(uv_timer_t *timer, int) {
 
 // -------------------------------------------------------------------------------------------------
 
-HTTPRequest::HTTPRequest(const Resource& resource, Callback callback)
-    : RequestBase(resource, callback) {
+HTTPRequest::HTTPRequest(const Resource& resource, Callback callback,
+                         uv_loop_t* loop, std::shared_ptr<const Response> response)
+    : RequestBase(resource, callback)
+    , impl(new HTTPRequestImpl(this, loop, response)) {
 }
 
 HTTPRequest::~HTTPRequest() {
-    if (ptr) {
-        reinterpret_cast<HTTPRequestImpl *>(ptr)->cancel();
+    if (impl) {
+        impl->cancel();
     }
 }
 
-void HTTPRequest::start(uv_loop_t *loop, std::shared_ptr<const Response> response) {
-    assert(!ptr);
-    ptr = new HTTPRequestImpl(this, loop, response);
-}
-
 void HTTPRequest::retryImmediately() {
-    if (ptr) {
-        reinterpret_cast<HTTPRequestImpl *>(ptr)->retryImmediately();
+    if (impl) {
+        impl->retryImmediately();
     }
 }
 

platform/default/asset_request_fs.cpp

@@ -26,7 +26,7 @@ class AssetRequestImpl {
     MBGL_STORE_THREAD(tid)
 
 public:
-    AssetRequestImpl(AssetRequest *request, uv_loop_t *loop);
+    AssetRequestImpl(AssetRequest*, uv_loop_t*, const std::string& assetRoot);
     ~AssetRequestImpl();
 
     static void fileOpened(uv_fs_t *req);
@@ -36,8 +36,8 @@ public:
     static void notifyError(uv_fs_t *req);
     static void cleanup(uv_fs_t *req);
 
-
     AssetRequest *request = nullptr;
+    std::string assetRoot;
     bool canceled = false;
     uv_fs_t req;
     uv_file fd = -1;
@@ -49,11 +49,13 @@ AssetRequestImpl::~AssetRequestImpl() {
     MBGL_VERIFY_THREAD(tid);
 
     if (request) {
-        request->ptr = nullptr;
+        request->impl = nullptr;
     }
 }
 
-AssetRequestImpl::AssetRequestImpl(AssetRequest *request_, uv_loop_t *loop) : request(request_) {
+AssetRequestImpl::AssetRequestImpl(AssetRequest* request_, uv_loop_t* loop, const std::string& assetRoot_)
+    : request(request_)
+    , assetRoot(assetRoot_) {
     req.data = this;
 
     const auto &url = request->resource.url;
@@ -63,7 +65,7 @@ AssetRequestImpl::AssetRequestImpl(AssetRequest *request_, uv_loop_t *loop) : re
         path = url.substr(8);
     } else {
         // This is a relative path. Prefix with the application root.
-        path = request->assetRoot + "/" + url.substr(8);
+        path = assetRoot + "/" + url.substr(8);
     }
 
     uv_fs_open(loop, &req, path.c_str(), O_RDONLY, S_IRUSR, fileOpened);
@@ -208,40 +210,26 @@ void AssetRequestImpl::cleanup(uv_fs_t *req) {
 
 // -------------------------------------------------------------------------------------------------
 
-AssetRequest::AssetRequest(const Resource& resource_, Callback callback_, const std::string& assetRoot_)
+AssetRequest::AssetRequest(const Resource& resource_, Callback callback_,
+                           uv_loop_t* loop, const std::string& assetRoot)
     : RequestBase(resource_, callback_)
-    , assetRoot(assetRoot_) {
+    , impl(new AssetRequestImpl(this, loop, assetRoot)) {
     assert(algo::starts_with(resource.url, "asset://"));
-}
-
-AssetRequest::~AssetRequest() {
-    if (ptr) {
-        reinterpret_cast<AssetRequestImpl *>(ptr)->request = nullptr;
-    }
-}
-
-void AssetRequest::start(uv_loop_t *loop, std::shared_ptr<const Response> response) {
-    // We're ignoring the existing response if any.
-    (void(response));
-
-    assert(!ptr);
-    ptr = new AssetRequestImpl(this, loop);
     // Note: the AssetRequestImpl deletes itself.
 }
 
-void AssetRequest::cancel() {
-    if (ptr) {
-        reinterpret_cast<AssetRequestImpl *>(ptr)->canceled = true;
-
-        // uv_cancel fails frequently when the request has already been started.
-        // In that case, we have to let it complete and check the canceled bool
-        // instead. The cancelation callback will delete the AssetRequest object.
-        uv_cancel((uv_req_t *)&reinterpret_cast<AssetRequestImpl *>(ptr)->req);
-    } else {
-        // This request is canceled before we called start. We're safe to delete
-        // ourselves now.
-        delete this;
+AssetRequest::~AssetRequest() {
+    if (impl) {
+        impl->request = nullptr;
     }
 }
 
+void AssetRequest::cancel() {
+    impl->canceled = true;
+    // uv_cancel fails frequently when the request has already been started.
+    // In that case, we have to let it complete and check the canceled bool
+    // instead. The cancelation callback will delete the AssetRequest object.
+    uv_cancel((uv_req_t *)&impl->req);
+}
+
 }

platform/default/asset_request_zip.cpp

@@ -77,7 +77,7 @@ class AssetRequestImpl {
     MBGL_STORE_THREAD(tid)
 
 public:
-    AssetRequestImpl(AssetRequest *request, uv_loop_t *loop);
+    AssetRequestImpl(AssetRequest*, uv_loop_t*, const std::string& assetRoot);
     ~AssetRequestImpl();
 
     void cancel();
@@ -112,10 +112,10 @@ AssetRequestImpl::~AssetRequestImpl() {
     }
 }
 
-AssetRequestImpl::AssetRequestImpl(AssetRequest *request_, uv_loop_t *loop)
+AssetRequestImpl::AssetRequestImpl(AssetRequest* request_, uv_loop_t* loop, const std::string& assetRoot_)
     : context(*AssetZipContext::Get(loop)),
       request(request_),
-      root(request->assetRoot),
+      root(assetRoot_),
       path(std::string { "assets/" } + request->resource.url.substr(8)) {
     auto zip = context.getHandle(root);
     if (zip) {
@@ -275,30 +275,22 @@ void AssetRequestImpl::cancel() {
 
 // -------------------------------------------------------------------------------------------------
 
-AssetRequest::AssetRequest(const Resource& resource_, Callback callback_, const std::string& assetRoot_)
+AssetRequest::AssetRequest(const Resource& resource_, Callback callback_,
+                           uv_loop_t* loop, const std::string& assetRoot)
     : RequestBase(resource_, callback_)
-    , assetRoot(assetRoot_) {
+    , impl(new AssetRequestImpl(this, loop, assetRoot)) {
     assert(algo::starts_with(resource.url, "asset://"));
 }
 
 AssetRequest::~AssetRequest() {
-    if (ptr) {
-        reinterpret_cast<AssetRequestImpl *>(ptr)->cancel();
+    if (impl) {
+        impl->cancel();
     }
 }
 
-void AssetRequest::start(uv_loop_t *loop, std::shared_ptr<const Response> response) {
-    // We're ignoring the existing response if any.
-    (void(response));
-
-    assert(!ptr);
-    ptr = new AssetRequestImpl(this, loop);
-    // Note: the AssetRequestImpl deletes itself.
-}
-
 void AssetRequest::cancel() {
-    if (ptr) {
-        reinterpret_cast<AssetRequestImpl *>(ptr)->cancel();
+    if (impl) {
+        impl->cancel();
     }
 
     delete this;

platform/default/http_request_curl.cpp

@@ -495,7 +495,7 @@ HTTPRequestImpl::~HTTPRequestImpl() {
 
     if (request) {
         context->removeRequest(request);
-        request->ptr = nullptr;
+        request->impl = nullptr;
     }
 
     handleError(curl_multi_remove_handle(context->multi, handle));
@@ -723,33 +723,27 @@ void HTTPRequestImpl::handleResult(CURLcode code) {
 
 // -------------------------------------------------------------------------------------------------
 
-HTTPRequest::HTTPRequest(const Resource& resource_, Callback callback_)
-    : RequestBase(resource_, callback_) {
+HTTPRequest::HTTPRequest(const Resource& resource_, Callback callback_,
+                         uv_loop_t* loop, std::shared_ptr<const Response> response)
+    : RequestBase(resource_, callback_)
+    , impl(new HTTPRequestImpl(this, loop, response)) {
 }
 
 HTTPRequest::~HTTPRequest() {
-    if (ptr) {
-        reinterpret_cast<HTTPRequestImpl *>(ptr)->abandon();
+    if (impl) {
+        impl->abandon();
     }
 }
 
-void HTTPRequest::start(uv_loop_t *loop, std::shared_ptr<const Response> response) {
-    assert(!ptr);
-    ptr = new HTTPRequestImpl(this, loop, response);
-}
-
 void HTTPRequest::retryImmediately() {
-    if (ptr) {
-        reinterpret_cast<HTTPRequestImpl *>(ptr)->retryImmediately();
+    if (impl) {
+        impl->retryImmediately();
     }
 }
 
 void HTTPRequest::cancel() {
-    if (ptr) {
-        delete reinterpret_cast<HTTPRequestImpl *>(ptr);
-        ptr = nullptr;
-    }
-
+    delete impl;
+    impl = nullptr;
     delete this;
 }
 

src/mbgl/storage/asset_request.hpp

@@ -3,20 +3,22 @@
 
 #include "request_base.hpp"
 
+typedef struct uv_loop_s uv_loop_t;
+
 namespace mbgl {
 
+class AssetRequestImpl;
+
 class AssetRequest : public RequestBase {
 public:
-    AssetRequest(const Resource&, Callback, const std::string& assetRoot);
+    AssetRequest(const Resource&, Callback, uv_loop_t*, const std::string& assetRoot);
 
-    void start(uv_loop_t *loop, std::shared_ptr<const Response> response = nullptr);
-    void cancel();
+    void cancel() override;
 
 private:
     ~AssetRequest();
-    void *ptr = nullptr;
-    const std::string& assetRoot;
 
+    AssetRequestImpl* impl;
     friend class AssetRequestImpl;
 };
 

src/mbgl/storage/default_file_source.cpp

@@ -134,12 +134,10 @@ void DefaultFileSource::Impl::startRealRequest(const Resource& resource, std::sh
     };
 
     if (algo::starts_with(resource.url, "asset://")) {
-        request->request = new AssetRequest(resource, callback, assetRoot);
+        request->request = new AssetRequest(resource, callback, request->loop, assetRoot);
     } else {
-        request->request = new HTTPRequest(resource, callback);
+        request->request = new HTTPRequest(resource, callback, request->loop, response);
     }
-
-    request->request->start(request->loop, response);
 }
 
 void DefaultFileSource::Impl::cancel(Request* req) {

src/mbgl/storage/http_request.hpp

@@ -3,23 +3,23 @@
 
 #include "request_base.hpp"
 
+typedef struct uv_loop_s uv_loop_t;
+
 namespace mbgl {
 
-struct Resource;
+class HTTPRequestImpl;
 
 class HTTPRequest : public RequestBase {
 public:
-    HTTPRequest(const Resource&, Callback);
-
-    void start(uv_loop_t *loop, std::shared_ptr<const Response> response = nullptr);
-    void cancel();
+    HTTPRequest(const Resource&, Callback, uv_loop_t*, std::shared_ptr<const Response> = nullptr);
 
+    void cancel() override;
     void retryImmediately();
 
 private:
     ~HTTPRequest();
-    void *ptr = nullptr;
 
+    HTTPRequestImpl* impl;
     friend class HTTPRequestImpl;
 };
 

src/mbgl/storage/request_base.hpp

@@ -7,8 +7,6 @@
 #include <memory>
 #include <functional>
 
-typedef struct uv_loop_s uv_loop_t;
-
 namespace mbgl {
 
 struct Resource;
@@ -19,19 +17,16 @@ public:
     using Callback = std::function<void (std::shared_ptr<const Response> response, FileCache::Hint hint)>;
 
     RequestBase(const Resource& resource_, Callback notify_)
-        : notify(notify_)
-        , resource(resource_) {
+        : resource(resource_)
+        , notify(notify_) {
     }
 
     virtual ~RequestBase() = default;
-
-    virtual void start(uv_loop_t *loop, std::shared_ptr<const Response> response = nullptr) = 0;
     virtual void cancel() = 0;
 
-    Callback notify;
-
 protected:
     const Resource& resource;
+    Callback notify;
 };
 
 }