From 89505d6f76d3a8d9122c53d0c4dd1ec1409bb368 Mon Sep 17 00:00:00 2001
From: Alex Zolotarev <alex@maps.me>
Date: Wed, 24 Feb 2016 22:22:45 +0300
Subject: [PATCH] [ios] Removed unnecessary run on main thread wrappers for
 some callbacks. Core guarantees it for UI code.

---
 .../Classes/Framework/MWMFrameworkListener.mm   | 17 +++++++++--------
 map/framework.hpp                               |  9 +++++++++
 storage/storage.hpp                             |  1 +
 3 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/iphone/Maps/Classes/Framework/MWMFrameworkListener.mm b/iphone/Maps/Classes/Framework/MWMFrameworkListener.mm
index d1d0bd2806..4f900d0593 100644
--- a/iphone/Maps/Classes/Framework/MWMFrameworkListener.mm
+++ b/iphone/Maps/Classes/Framework/MWMFrameworkListener.mm
@@ -122,6 +122,11 @@ void loopWrappers(TObservers * observers, TLoopBlock block)
   using namespace storage;
   TObservers * observers = self.routeBuildingObservers;
   auto & f = GetFramework();
+  // TODO(ldragunov,rokuz): Thise two routing callbacks are the only framework callbacks which does not guarantee
+  // that they are called on a main UI thread context. Discuss it with Lev.
+  // Simplest solution is to insert RunOnGuiThread() call in the core where callbacks are called.
+  // This will help to avoid unnecessary parameters copying and will make all our framework callbacks
+  // consistent: every notification to UI will run on a main UI thread.
   f.SetRouteBuildingListener([observers](IRouter::ResultCode code, TCountriesVec const & absentCountries)
   {
     loopWrappers(observers, [code, absentCountries](TRouteBuildingObserver observer)
@@ -184,18 +189,16 @@ void loopWrappers(TObservers * observers, TLoopBlock block)
   auto & s = GetFramework().Storage();
   s.Subscribe([observers](TCountryId const & countryId)
   {
-    loopWrappers(observers, [countryId](TStorageObserver observer)
-    {
+    for (TStorageObserver observer in observers)
       [observer processCountryEvent:countryId];
-    });
   },
   [observers](TCountryId const & countryId, TLocalAndRemoteSize const & progress)
   {
-    loopWrappers(observers, [countryId, progress](TStorageObserver observer)
+    for (TStorageObserver observer in observers)
     {
       if ([observer respondsToSelector:@selector(processCountry:progress:)])
         [observer processCountry:countryId progress:progress];
-    });
+    }
   });
 }
 
@@ -207,10 +210,8 @@ void loopWrappers(TObservers * observers, TLoopBlock block)
   auto & f = GetFramework();
   f.SetCurrentCountryChangedListener([observers](TCountryId const & countryId)
   {
-    loopWrappers(observers, [countryId](TDrapeObserver observer)
-    {
+    for (TDrapeObserver observer in observers)
       [observer processViewportCountryEvent:countryId];
-    });
   });
 }
 
diff --git a/map/framework.hpp b/map/framework.hpp
index 782e65af6a..f1e2d5d8cf 100644
--- a/map/framework.hpp
+++ b/map/framework.hpp
@@ -259,6 +259,7 @@ public:
 
   using TCurrentCountryChanged = function<void(storage::TCountryId const &)>;
   storage::TCountryId const & GetLastReportedCountry() { return m_lastReportedCountry; }
+  /// Guarantees that listener is called in the main thread context.
   void SetCurrentCountryChangedListener(TCurrentCountryChanged const & listener);
 
 private:
@@ -284,6 +285,7 @@ public:
   void OnCompassUpdate(location::CompassInfo const & info);
   void SwitchMyPositionNextMode();
   void InvalidateMyPosition();
+  /// Should be set before Drape initialization. Guarantees that fn is called in main thread context.
   void SetMyPositionModeListener(location::TMyPositionModeChanged && fn);
 
 private:
@@ -534,7 +536,14 @@ public:
   // FollowRoute has a bug where the router follows the route even if the method hads't been called.
   // This method was added because we do not want to break the behaviour that is familiar to our users.
   bool DisableFollowMode();
+  /// @TODO(AlexZ): Warning! These two routing callbacks are the only callbacks which are not called in the main thread context.
+  /// UI code should take it into an account. This is a result of current implementation, that can be improved:
+  /// Drape core calls some RunOnGuiThread with "this" pointers, and it causes crashes on Android, when Drape engine is destroyed
+  /// while switching between activities. Current workaround cleans all callbacks when destroying Drape engine
+  /// (@see MwmApplication.clearFunctorsOnUiThread on Android). Better soulution can be fair copying of all needed information into
+  /// lambdas/functors before calling RunOnGuiThread.
   void SetRouteBuildingListener(TRouteBuildingCallback const & buildingCallback) { m_routingCallback = buildingCallback; }
+  /// See warning above.
   void SetRouteProgressListener(TRouteProgressCallback const & progressCallback) { m_progressCallback = progressCallback; }
   void FollowRoute();
   void CloseRouting();
diff --git a/storage/storage.hpp b/storage/storage.hpp
index d969e9f834..95468ac3a5 100644
--- a/storage/storage.hpp
+++ b/storage/storage.hpp
@@ -363,6 +363,7 @@ public:
   // Returns number of downloaded maps (files), excluding fake countries (World*.mwm).
   size_t GetDownloadedFilesCount() const;
 
+  /// Guarantees that change and progress are called in the main thread context.
   /// @return unique identifier that should be used with Unsubscribe function
   int Subscribe(TChangeCountryFunction const & change, TProgressFunction const & progress);
   void Unsubscribe(int slotId);