[core] Rationalize error handling for resource loading

* Standardize on std::exception_ptr as the error representation
  (fixes #2854).
* Don't format textual strings at the error source; pass on the
  constituent data via observer method parameters instead.
* Use the null object pattern to simplify observer notification code.
* Further refactoring for ResourceLoading tests.

4 files changed, 33 insertions, 82 deletions

diff --git a/src/mbgl/text/glyph_pbf.cpp b/src/mbgl/text/glyph_pbf.cpp
index c14f52de7a..0c0626c0de 100644
--- a/src/mbgl/text/glyph_pbf.cpp
+++ b/src/mbgl/text/glyph_pbf.cpp
@@ -12,8 +12,6 @@
 #include <mbgl/util/token.hpp>
 #include <mbgl/util/url.hpp>
 
-#include <sstream>
-
 namespace {
 
 void parseGlyphPBF(mbgl::FontStack& stack, const std::string& data) {
@@ -65,8 +63,10 @@ namespace mbgl {
 
 GlyphPBF::GlyphPBF(GlyphStore* store,
                    const std::string& fontStack,
-                   const GlyphRange& glyphRange)
-    : parsed(false) {
+                   const GlyphRange& glyphRange,
+                   GlyphStore::Observer* observer_)
+    : parsed(false),
+      observer(observer_) {
     // 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);
@@ -74,7 +74,7 @@ GlyphPBF::GlyphPBF(GlyphStore* store,
         return "";
     });
 
-    auto requestCallback = [this, store, fontStack, url](Response res) {
+    auto requestCallback = [this, store, fontStack, glyphRange](Response res) {
         if (res.stale) {
             // Only handle fresh responses.
             return;
@@ -82,12 +82,10 @@ GlyphPBF::GlyphPBF(GlyphStore* store,
         req = nullptr;
 
         if (res.error) {
-            std::stringstream message;
-            message <<  "Failed to load [" << url << "]: " << res.error->message;
-            emitGlyphPBFLoadingFailed(message.str());
+            observer->onGlyphsError(fontStack, glyphRange, std::make_exception_ptr(std::runtime_error(res.error->message)));
         } else {
             data = res.data;
-            parse(store, fontStack, url);
+            parse(store, fontStack, glyphRange);
         }
     };
 
@@ -97,7 +95,7 @@ GlyphPBF::GlyphPBF(GlyphStore* store,
 
 GlyphPBF::~GlyphPBF() = default;
 
-void GlyphPBF::parse(GlyphStore* store, const std::string& fontStack, const std::string& url) {
+void GlyphPBF::parse(GlyphStore* store, const std::string& fontStack, const GlyphRange& glyphRange) {
     assert(data);
     if (data->empty()) {
         // If there is no data, this means we either haven't
@@ -107,35 +105,13 @@ void GlyphPBF::parse(GlyphStore* store, const std::string& fontStack, const std:
 
     try {
         parseGlyphPBF(**store->getFontStack(fontStack), *data);
-    } catch (const std::exception& ex) {
-        std::stringstream message;
-        message <<  "Failed to parse [" << url << "]: " << ex.what();
-        emitGlyphPBFLoadingFailed(message.str());
+    } catch (...) {
+        observer->onGlyphsError(fontStack, glyphRange, std::current_exception());
         return;
     }
 
     parsed = true;
-
-    emitGlyphPBFLoaded();
-}
-
-void GlyphPBF::setObserver(Observer* observer_) {
-    observer = observer_;
-}
-
-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);
+    observer->onGlyphsLoaded(fontStack, glyphRange);
 }
 
 } // namespace mbgl
diff --git a/src/mbgl/text/glyph_pbf.hpp b/src/mbgl/text/glyph_pbf.hpp
index d595298516..e0f65e7789 100644
--- a/src/mbgl/text/glyph_pbf.hpp
+++ b/src/mbgl/text/glyph_pbf.hpp
@@ -2,6 +2,7 @@
 #define MBGL_TEXT_GLYPH_PBF
 
 #include <mbgl/text/glyph.hpp>
+#include <mbgl/text/glyph_store.hpp>
 #include <mbgl/util/noncopyable.hpp>
 
 #include <atomic>
@@ -11,43 +12,30 @@
 
 namespace mbgl {
 
-class GlyphStore;
 class FontStack;
 class FileRequest;
 
 class GlyphPBF : private util::noncopyable {
 public:
-    class Observer {
-    public:
-        virtual ~Observer() = default;
-
-        virtual void onGlyphPBFLoaded() = 0;
-        virtual void onGlyphPBFLoadingFailed(std::exception_ptr error) = 0;
-    };
-
     GlyphPBF(GlyphStore* store,
              const std::string& fontStack,
-             const GlyphRange& glyphRange);
-    virtual ~GlyphPBF();
+             const GlyphRange&,
+             GlyphStore::Observer*);
+    ~GlyphPBF();
 
     bool isParsed() const {
         return parsed;
-    };
-
-    void setObserver(Observer* observer);
+    }
 
 private:
-    void emitGlyphPBFLoaded();
-    void emitGlyphPBFLoadingFailed(const std::string& message);
-
-    void parse(GlyphStore* store, const std::string& fontStack, const std::string& url);
+    void parse(GlyphStore*, const std::string& fontStack, const GlyphRange&);
 
     std::shared_ptr<const std::string> data;
     std::atomic<bool> parsed;
 
     std::unique_ptr<FileRequest> req;
 
-    Observer* observer = nullptr;
+    GlyphStore::Observer* observer = nullptr;
 };
 
 } // namespace mbgl
diff --git a/src/mbgl/text/glyph_store.cpp b/src/mbgl/text/glyph_store.cpp
index 047db17b7d..a3d1530e3d 100644
--- a/src/mbgl/text/glyph_store.cpp
+++ b/src/mbgl/text/glyph_store.cpp
@@ -7,6 +7,9 @@
 
 namespace mbgl {
 
+GlyphStore::GlyphStore() = default;
+GlyphStore::~GlyphStore() = default;
+
 void GlyphStore::requestGlyphRange(const std::string& fontStackName, const GlyphRange& range) {
     assert(util::ThreadContext::currentlyOn(util::ThreadType::Map));
 
@@ -18,10 +21,8 @@ void GlyphStore::requestGlyphRange(const std::string& fontStackName, const Glyph
         return;
     }
 
-    auto glyphPBF = std::make_unique<GlyphPBF>(this, fontStackName, range);
-    glyphPBF->setObserver(this);
-
-    rangeSets.emplace(range, std::move(glyphPBF));
+    rangeSets.emplace(range,
+        std::make_unique<GlyphPBF>(this, fontStackName, range, observer));
 }
 
 
@@ -66,18 +67,6 @@ util::exclusive<FontStack> GlyphStore::getFontStack(const std::string& fontStack
     return { it->second.get(), std::move(lock) };
 }
 
-void GlyphStore::onGlyphPBFLoaded() {
-    if (observer) {
-        observer->onGlyphRangeLoaded();
-    }
-}
-
-void GlyphStore::onGlyphPBFLoadingFailed(std::exception_ptr error) {
-    if (observer) {
-        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 4e665087ea..2236bce38c 100644
--- a/src/mbgl/text/glyph_store.hpp
+++ b/src/mbgl/text/glyph_store.hpp
@@ -3,7 +3,6 @@
 
 #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/work_queue.hpp>
@@ -15,21 +14,23 @@
 
 namespace mbgl {
 
+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 : public GlyphPBF::Observer, private util::noncopyable {
+class GlyphStore : private util::noncopyable {
 public:
     class Observer {
     public:
         virtual ~Observer() = default;
 
-        virtual void onGlyphRangeLoaded() = 0;
-        virtual void onGlyphRangeLoadingFailed(std::exception_ptr error) = 0;
+        virtual void onGlyphsLoaded(const std::string& /* fontStack */, const GlyphRange&) {};
+        virtual void onGlyphsError(const std::string& /* fontStack */, const GlyphRange&, std::exception_ptr) {};
     };
 
-    GlyphStore() = default;
-    virtual ~GlyphStore() = default;
+    GlyphStore();
+    ~GlyphStore();
 
     util::exclusive<FontStack> getFontStack(const std::string& fontStack);
 
@@ -38,7 +39,7 @@ public:
     // made and when the glyph if finally parsed, it gets added to the respective
     // FontStack and a signal is emitted to notify the observers. This method
     // can be called from any thread.
-    bool hasGlyphRanges(const std::string& fontStackName, const std::set<GlyphRange>& glyphRanges);
+    bool hasGlyphRanges(const std::string& fontStack, const std::set<GlyphRange>& glyphRanges);
 
     void setURL(const std::string &url) {
         glyphURL = url;
@@ -48,10 +49,6 @@ public:
         return glyphURL;
     }
 
-    // GlyphPBF::Observer implementation.
-    void onGlyphPBFLoaded() override;
-    void onGlyphPBFLoadingFailed(std::exception_ptr error) override;
-
     void setObserver(Observer* observer);
 
 private:
@@ -67,7 +64,8 @@ private:
 
     util::WorkQueue workQueue;
 
-    Observer* observer = nullptr;
+    Observer nullObserver;
+    Observer* observer = &nullObserver;
 };
 
 } // namespace mbgl