Use the observer pattern for GlyphPBF loading

This will make the code a lot more clear and it will
also move how parsing is initiated to the GlyphPBF
class, to be initiated after the request, like we do
for other resources.

4 files changed, 135 insertions, 139 deletions

diff --git a/src/mbgl/text/glyph_pbf.cpp b/src/mbgl/text/glyph_pbf.cpp
index 24993ed72d..ba92ce6b4e 100644
--- a/src/mbgl/text/glyph_pbf.cpp
+++ b/src/mbgl/text/glyph_pbf.cpp
@@ -1,77 +1,32 @@
 #include <mbgl/text/glyph_pbf.hpp>
-#include <mbgl/text/font_stack.hpp>
 
 #include <mbgl/storage/file_source.hpp>
 #include <mbgl/storage/resource.hpp>
 #include <mbgl/storage/response.hpp>
-
+#include <mbgl/text/font_stack.hpp>
+#include <mbgl/text/glyph_store.hpp>
+#include <mbgl/util/exception.hpp>
 #include <mbgl/util/pbf.hpp>
 #include <mbgl/util/string.hpp>
-#include <mbgl/util/thread.hpp>
+#include <mbgl/util/thread_context.hpp>
 #include <mbgl/util/token.hpp>
 #include <mbgl/util/url.hpp>
 
 #include <sstream>
 
-namespace mbgl {
-
-GlyphPBF::GlyphPBF(const std::string& glyphURL,
-                   const std::string& fontStack,
-                   GlyphRange glyphRange,
-                   const GlyphLoadedCallback& successCallback,
-                   const GlyphLoadingFailedCallback& failureCallback)
-    : parsed(false) {
-    // Load the glyph set URL
-    url = util::replaceTokens(glyphURL, [&](const std::string &name) -> std::string {
-        if (name == "fontstack") return util::percentEncode(fontStack);
-        if (name == "range") return util::toString(glyphRange.first) + "-" + util::toString(glyphRange.second);
-        return "";
-    });
+namespace {
 
-    // The prepare call jumps back to the main thread.
-    FileSource* fs = util::ThreadContext::getFileSource();
-    req = fs->request({ Resource::Kind::Glyphs, url }, util::RunLoop::getLoop(), [&, successCallback, failureCallback](const Response &res) {
-        req = nullptr;
-
-        if (res.status != Response::Successful) {
-            std::stringstream message;
-            message <<  "Failed to load [" << url << "]: " << res.message;
-            failureCallback(message.str());
-        } else {
-            // Transfer the data to the GlyphSet and signal its availability.
-            // Once it is available, the caller will need to call parse() to actually
-            // parse the data we received. We are not doing this here since this callback is being
-            // called from another (unknown) thread.
-            data = res.data;
-            successCallback(this);
-        }
-    });
-}
-
-GlyphPBF::~GlyphPBF() {
-    if (req) {
-        util::ThreadContext::getFileSource()->cancel(req);
-    }
-}
-
-void GlyphPBF::parse(FontStack &stack) {
-    if (!data.size()) {
-        // If there is no data, this means we either haven't received any data, or
-        // we have already parsed the data.
-        return;
-    }
-
-    // Parse the glyph PBF
-    pbf glyphs_pbf(reinterpret_cast<const uint8_t *>(data.data()), data.size());
+void parseGlyphPBF(mbgl::FontStack& stack, const std::string& data) {
+    mbgl::pbf glyphs_pbf(reinterpret_cast<const uint8_t *>(data.data()), data.size());
 
     while (glyphs_pbf.next()) {
         if (glyphs_pbf.tag == 1) { // stacks
-            pbf fontstack_pbf = glyphs_pbf.message();
+            mbgl::pbf fontstack_pbf = glyphs_pbf.message();
             while (fontstack_pbf.next()) {
                 if (fontstack_pbf.tag == 3) { // glyphs
-                    pbf glyph_pbf = fontstack_pbf.message();
+                    mbgl::pbf glyph_pbf = fontstack_pbf.message();
 
-                    SDFGlyph glyph;
+                    mbgl::SDFGlyph glyph;
 
                     while (glyph_pbf.next()) {
                         if (glyph_pbf.tag == 1) { // id
@@ -102,14 +57,84 @@ void GlyphPBF::parse(FontStack &stack) {
             glyphs_pbf.skip();
         }
     }
+}
+
+}
+
+namespace mbgl {
+
+GlyphPBF::GlyphPBF(GlyphStore* store,
+                   const std::string& fontStack,
+                   const GlyphRange& glyphRange)
+    : parsed(false) {
+    // Load the glyph set URL
+    std::string url = util::replaceTokens(store->getURL(), [&](const std::string &name) -> std::string {
+        if (name == "fontstack") return util::percentEncode(fontStack);
+        if (name == "range") return util::toString(glyphRange.first) + "-" + util::toString(glyphRange.second);
+        return "";
+    });
+
+    auto requestCallback = [this, store, fontStack, url](const Response &res) {
+        req = nullptr;
+
+        if (res.status != Response::Successful) {
+            std::stringstream message;
+            message <<  "Failed to load [" << url << "]: " << res.message;
+            emitGlyphPBFLoadingFailed(message.str());
+        } else {
+            data = res.data;
+            parse(store, fontStack, url);
+        }
+    };
+
+    FileSource* fs = util::ThreadContext::getFileSource();
+    req = fs->request({ Resource::Kind::Glyphs, url }, util::RunLoop::getLoop(), requestCallback);
+}
+
+GlyphPBF::~GlyphPBF() {
+    if (req) {
+        util::ThreadContext::getFileSource()->cancel(req);
+    }
+}
+
+void GlyphPBF::parse(GlyphStore* store, const std::string& fontStack, const std::string& url) {
+    if (data.empty()) {
+        // If there is no data, this means we either haven't
+        // received any data.
+        return;
+    }
 
-    data.clear();
+    try {
+        parseGlyphPBF(**store->getFontStack(fontStack), std::move(data));
+    } catch (const std::exception& ex) {
+        std::stringstream message;
+        message <<  "Failed to parse [" << url << "]: " << ex.what();
+        emitGlyphPBFLoadingFailed(message.str());
+        return;
+    }
 
     parsed = true;
+
+    emitGlyphPBFLoaded();
+}
+
+void GlyphPBF::setObserver(Observer* observer_) {
+    observer = observer_;
 }
 
-bool GlyphPBF::isParsed() const {
-    return parsed;
+void GlyphPBF::emitGlyphPBFLoaded() {
+    if (observer) {
+        observer->onGlyphPBFLoaded();
+    }
+}
+
+void GlyphPBF::emitGlyphPBFLoadingFailed(const std::string& message) {
+    if (!observer) {
+        return;
+    }
+
+    auto error = std::make_exception_ptr(util::GlyphRangeLoadingException(message));
+    observer->onGlyphPBFLoadingFailed(error);
 }
 
 }
diff --git a/src/mbgl/text/glyph_pbf.hpp b/src/mbgl/text/glyph_pbf.hpp
index cc083e7f10..2aa2134d16 100644
--- a/src/mbgl/text/glyph_pbf.hpp
+++ b/src/mbgl/text/glyph_pbf.hpp
@@ -2,48 +2,53 @@
 #define MBGL_TEXT_GLYPH_PBF
 
 #include <mbgl/text/glyph.hpp>
+#include <mbgl/util/noncopyable.hpp>
 
-#include <functional>
 #include <atomic>
+#include <functional>
 #include <string>
 
 namespace mbgl {
 
+class GlyphStore;
 class FontStack;
 class Request;
 
-class GlyphPBF {
+class GlyphPBF : private util::noncopyable {
 public:
-    using GlyphLoadedCallback = std::function<void(GlyphPBF*)>;
-    using GlyphLoadingFailedCallback = std::function<void(const std::string&)>;
+    class Observer {
+    public:
+        virtual ~Observer() = default;
+
+        virtual void onGlyphPBFLoaded() = 0;
+        virtual void onGlyphPBFLoadingFailed(std::exception_ptr error) = 0;
+    };
 
-    GlyphPBF(const std::string &glyphURL,
-             const std::string &fontStack,
-             GlyphRange glyphRange,
-             const GlyphLoadedCallback& successCallback,
-             const GlyphLoadingFailedCallback& failureCallback);
-    ~GlyphPBF();
+    GlyphPBF(GlyphStore* store,
+             const std::string& fontStack,
+             const GlyphRange& glyphRange);
+    virtual ~GlyphPBF();
 
-    void parse(FontStack &stack);
-    bool isParsed() const;
+    bool isParsed() const {
+        return parsed;
+    };
 
-    std::string getURL() const {
-        return url;
-    }
+    void setObserver(Observer* observer);
 
 private:
-    GlyphPBF(const GlyphPBF &) = delete;
-    GlyphPBF(GlyphPBF &&) = delete;
-    GlyphPBF &operator=(const GlyphPBF &) = delete;
-    GlyphPBF &operator=(GlyphPBF &&) = delete;
+    void emitGlyphPBFLoaded();
+    void emitGlyphPBFLoadingFailed(const std::string& message);
+
+    void parse(GlyphStore* store, const std::string& fontStack, const std::string& url);
 
     std::string data;
-    std::string url;
     std::atomic<bool> parsed;
 
     Request* req = nullptr;
+
+    Observer* observer = nullptr;
 };
 
-} // end namespace mbgl
+}
 
 #endif
diff --git a/src/mbgl/text/glyph_store.cpp b/src/mbgl/text/glyph_store.cpp
index f34f8ecd04..eb770fbf6e 100644
--- a/src/mbgl/text/glyph_store.cpp
+++ b/src/mbgl/text/glyph_store.cpp
@@ -1,38 +1,13 @@
 #include <mbgl/text/glyph_store.hpp>
 
-#include <mbgl/text/font_stack.hpp>
 #include <mbgl/text/glyph_pbf.hpp>
-#include <mbgl/util/exception.hpp>
 #include <mbgl/util/thread_context.hpp>
 
 namespace mbgl {
 
-GlyphStore::GlyphStore() {
-}
-
-GlyphStore::~GlyphStore() {
-}
-
 void GlyphStore::requestGlyphRange(const std::string& fontStackName, const GlyphRange& range) {
     assert(util::ThreadContext::currentlyOn(util::ThreadType::Map));
 
-    auto successCallback = [this, fontStackName](GlyphPBF* glyph) {
-        try {
-            {
-                auto fontStack = createFontStack(fontStackName);
-                glyph->parse(**fontStack);
-            }
-            emitGlyphRangeLoaded();
-        } catch (const std::exception&) {
-            std::string message = "Failed to parse [" + glyph->getURL() + "]";
-            emitGlyphRangeLoadingFailed(message);
-        }
-    };
-
-    auto failureCallback = [this](const std::string& message) {
-        emitGlyphRangeLoadingFailed(message);
-    };
-
     std::lock_guard<std::mutex> lock(rangesMutex);
     auto& rangeSets = ranges[fontStackName];
 
@@ -41,8 +16,10 @@ void GlyphStore::requestGlyphRange(const std::string& fontStackName, const Glyph
         return;
     }
 
-    rangeSets.emplace(range, std::make_unique<GlyphPBF>(glyphURL, fontStackName, range,
-        successCallback, failureCallback));
+    auto glyphPBF = std::make_unique<GlyphPBF>(this, fontStackName, range);
+    glyphPBF->setObserver(this);
+
+    rangeSets.emplace(range, std::move(glyphPBF));
 }
 
 
@@ -55,7 +32,6 @@ bool GlyphStore::hasGlyphRanges(const std::string& fontStackName, const std::set
     const auto& rangeSets = ranges[fontStackName];
 
     bool hasRanges = true;
-
     for (const auto& range : glyphRanges) {
         const auto& rangeSetsIt = rangeSets.find(range);
         if (rangeSetsIt == rangeSets.end()) {
@@ -75,7 +51,7 @@ bool GlyphStore::hasGlyphRanges(const std::string& fontStackName, const std::set
     return hasRanges;
 }
 
-util::exclusive<FontStack> GlyphStore::createFontStack(const std::string& fontStack) {
+util::exclusive<FontStack> GlyphStore::getFontStack(const std::string& fontStack) {
     auto lock = std::make_unique<std::lock_guard<std::mutex>>(stacksMutex);
 
     auto it = stacks.find(fontStack);
@@ -88,34 +64,20 @@ util::exclusive<FontStack> GlyphStore::createFontStack(const std::string& fontSt
     return { it->second.get(), std::move(lock) };
 }
 
-util::exclusive<FontStack> GlyphStore::getFontStack(const std::string &fontStack) {
-    auto lock = std::make_unique<std::lock_guard<std::mutex>>(stacksMutex);
-
-    const auto& it = stacks.find(fontStack);
-    if (it == stacks.end()) {
-        return { nullptr, nullptr };
-    }
-
-    return { it->second.get(), std::move(lock) };
-}
-
-void GlyphStore::setObserver(Observer* observer_) {
-    observer = observer_;
-}
-
-void GlyphStore::emitGlyphRangeLoaded() {
+void GlyphStore::onGlyphPBFLoaded() {
     if (observer) {
         observer->onGlyphRangeLoaded();
     }
 }
 
-void GlyphStore::emitGlyphRangeLoadingFailed(const std::string& message) {
-    if (!observer) {
-        return;
+void GlyphStore::onGlyphPBFLoadingFailed(std::exception_ptr error) {
+    if (observer) {
+        observer->onGlyphRangeLoadingFailed(error);
     }
+}
 
-    auto error = std::make_exception_ptr(util::GlyphRangeLoadingException(message));
-    observer->onGlyphRangeLoadingFailed(error);
+void GlyphStore::setObserver(Observer* observer_) {
+    observer = observer_;
 }
 
 }
diff --git a/src/mbgl/text/glyph_store.hpp b/src/mbgl/text/glyph_store.hpp
index 1e31e77bf5..f8de6cbaf5 100644
--- a/src/mbgl/text/glyph_store.hpp
+++ b/src/mbgl/text/glyph_store.hpp
@@ -1,8 +1,11 @@
 #ifndef MBGL_TEXT_GLYPH_STORE
 #define MBGL_TEXT_GLYPH_STORE
 
+#include <mbgl/text/font_stack.hpp>
 #include <mbgl/text/glyph.hpp>
+#include <mbgl/text/glyph_pbf.hpp>
 #include <mbgl/util/exclusive.hpp>
+#include <mbgl/util/noncopyable.hpp>
 #include <mbgl/util/run_loop.hpp>
 #include <mbgl/util/work_queue.hpp>
 
@@ -13,13 +16,10 @@
 
 namespace mbgl {
 
-class FontStack;
-class GlyphPBF;
-
 // The GlyphStore manages the loading and storage of Glyphs
 // and creation of FontStack objects. The GlyphStore lives
 // on the MapThread but can be queried from any thread.
-class GlyphStore {
+class GlyphStore : public GlyphPBF::Observer, private util::noncopyable {
 public:
     class Observer {
     public:
@@ -29,8 +29,8 @@ public:
         virtual void onGlyphRangeLoadingFailed(std::exception_ptr error) = 0;
     };
 
-    GlyphStore();
-    ~GlyphStore();
+    GlyphStore() = default;
+    virtual ~GlyphStore() = default;
 
     util::exclusive<FontStack> getFontStack(const std::string& fontStack);
 
@@ -45,13 +45,17 @@ public:
         glyphURL = url;
     }
 
+    std::string getURL() const {
+        return glyphURL;
+    }
+
+    // GlyphPBF::Observer implementation.
+    void onGlyphPBFLoaded() override;
+    void onGlyphPBFLoadingFailed(std::exception_ptr error) override;
+
     void setObserver(Observer* observer);
 
 private:
-    void emitGlyphRangeLoaded();
-    void emitGlyphRangeLoadingFailed(const std::string& message);
-
-    util::exclusive<FontStack> createFontStack(const std::string &fontStack);
     void requestGlyphRange(const std::string& fontStackName, const GlyphRange& range);
 
     std::string glyphURL;