From 475d0bcbcc7514e770818b8d17f4156ec5842c15 Mon Sep 17 00:00:00 2001
From: ExMix <rahuba.youri@mapswithme.com>
Date: Tue, 31 Mar 2015 11:40:18 +0300
Subject: [PATCH] review fixes

---
 base/string_utils.hpp                     |  1 +
 drape_frontend/apply_feature_functors.hpp |  3 ++-
 drape_frontend/drape_engine.hpp           |  2 +-
 drape_frontend/engine_context.cpp         | 11 +++++------
 drape_frontend/engine_context.hpp         |  2 +-
 drape_frontend/map_data_provider.cpp      |  4 ++--
 drape_frontend/map_data_provider.hpp      | 11 +++++------
 drape_frontend/read_manager.cpp           | 19 +++++++++----------
 drape_frontend/read_manager.hpp           | 14 ++++++--------
 drape_frontend/read_mwm_task.hpp          |  1 +
 drape_frontend/rule_drawer.hpp            |  2 +-
 drape_frontend/tile_info.hpp              |  3 ++-
 drape_gui/country_status_helper.cpp       | 11 ++++++-----
 drape_gui/country_status_helper.hpp       |  7 ++++++-
 drape_gui/drape_gui.hpp                   |  2 +-
 map/framework.cpp                         |  8 +++-----
 map/storage_bridge.hpp                    |  1 +
 17 files changed, 53 insertions(+), 49 deletions(-)

diff --git a/base/string_utils.hpp b/base/string_utils.hpp
index 8e85b8f695..9030b1ef82 100644
--- a/base/string_utils.hpp
+++ b/base/string_utils.hpp
@@ -53,6 +53,7 @@ void AsciiToLower(string & s);
 // TODO(AlexZ): current boost impl uses default std::locale() to trim.
 // In general, it does not work for any unicode whitespace except ASCII U+0020 one.
 void Trim(string & s);
+/// Remove any characters that contain in "anyOf" on left and right side of string s
 void Trim(string & s, char const * anyOf);
 
 void MakeLowerCaseInplace(string & s);
diff --git a/drape_frontend/apply_feature_functors.hpp b/drape_frontend/apply_feature_functors.hpp
index c44fa56f2a..396f867311 100644
--- a/drape_frontend/apply_feature_functors.hpp
+++ b/drape_frontend/apply_feature_functors.hpp
@@ -24,13 +24,14 @@ public:
   BaseApplyFeature(EngineContext & context, FeatureID const & id,
                    CaptionDescription const & captions);
 
+  virtual ~BaseApplyFeature() {}
+
 protected:
   void ExtractCaptionParams(CaptionDefProto const * primaryProto,
                             CaptionDefProto const * secondaryProto,
                             double depth,
                             TextViewParams & params) const;
 
-protected:
   EngineContext & m_context;
   FeatureID m_id;
   CaptionDescription const & m_captions;
diff --git a/drape_frontend/drape_engine.hpp b/drape_frontend/drape_engine.hpp
index 0c2565ef4d..92fa6401c6 100644
--- a/drape_frontend/drape_engine.hpp
+++ b/drape_frontend/drape_engine.hpp
@@ -46,7 +46,7 @@ public:
     dp::RefPointer<gui::StorageAccessor> m_storageAccessor;
     Viewport m_viewport;
     MapDataProvider m_model;
-    double m_vs = 1.0f;
+    double m_vs;
   };
 
   DrapeEngine(Params const & params);
diff --git a/drape_frontend/engine_context.cpp b/drape_frontend/engine_context.cpp
index 0f6fc7a140..1af3246403 100644
--- a/drape_frontend/engine_context.cpp
+++ b/drape_frontend/engine_context.cpp
@@ -22,12 +22,12 @@ EngineContext::EngineContext(TileKey tileKey, dp::RefPointer<ThreadsCommutator>
 
 void EngineContext::BeginReadTile()
 {
-  PostMessage(new TileReadStartMessage(m_tileKey));
+  PostMessage(dp::MovePointer<Message>(new TileReadStartMessage(m_tileKey)));
 }
 
 void EngineContext::InsertShape(dp::TransferPointer<MapShape> shape)
 {
-  PostMessage(new MapShapeReadedMessage(m_tileKey, shape));
+  PostMessage(dp::MovePointer<Message>(new MapShapeReadedMessage(m_tileKey, shape)));
 }
 
 void EngineContext::EndReadTile()
@@ -64,13 +64,12 @@ void EngineContext::EndReadTile()
   InsertShape(key, dp::MovePointer<df::MapShape>(new TextShape(r.Center(), tp)));
 #endif
 
-  PostMessage(new TileReadEndMessage(m_tileKey));
+  PostMessage(dp::MovePointer<Message>(new TileReadEndMessage(m_tileKey)));
 }
 
-void EngineContext::PostMessage(Message * message)
+void EngineContext::PostMessage(dp::TransferPointer<Message> message)
 {
-  m_commutator->PostMessage(ThreadsCommutator::ResourceUploadThread,
-                            dp::MovePointer(message),
+  m_commutator->PostMessage(ThreadsCommutator::ResourceUploadThread, message,
                             MessagePriority::Normal);
 }
 
diff --git a/drape_frontend/engine_context.hpp b/drape_frontend/engine_context.hpp
index 1aa743a0aa..fb6283e123 100644
--- a/drape_frontend/engine_context.hpp
+++ b/drape_frontend/engine_context.hpp
@@ -25,7 +25,7 @@ public:
   void EndReadTile();
 
 private:
-  void PostMessage(Message * message);
+  void PostMessage(dp::TransferPointer<Message> message);
 
 private:
   TileKey m_tileKey;
diff --git a/drape_frontend/map_data_provider.cpp b/drape_frontend/map_data_provider.cpp
index 9adadbddaa..a147690823 100644
--- a/drape_frontend/map_data_provider.cpp
+++ b/drape_frontend/map_data_provider.cpp
@@ -12,12 +12,12 @@ MapDataProvider::MapDataProvider(TReadIDsFn const & idsReader,
 {
 }
 
-void MapDataProvider::ReadFeaturesID(TReadIdCallback const & fn, m2::RectD const & r, int scale) const
+void MapDataProvider::ReadFeaturesID(TReadCallback<FeatureID> const & fn, m2::RectD const & r, int scale) const
 {
   m_idsReader(fn, r, scale);
 }
 
-void MapDataProvider::ReadFeatures(TReadFeatureCallback const & fn, vector<FeatureID> const & ids) const
+void MapDataProvider::ReadFeatures(TReadCallback<FeatureType> const & fn, vector<FeatureID> const & ids) const
 {
   m_featureReader(fn, ids);
 }
diff --git a/drape_frontend/map_data_provider.hpp b/drape_frontend/map_data_provider.hpp
index 59acdc7c11..2588c78190 100644
--- a/drape_frontend/map_data_provider.hpp
+++ b/drape_frontend/map_data_provider.hpp
@@ -14,18 +14,17 @@ namespace df
 class MapDataProvider
 {
 public:
-  using TReadIdCallback = function<void (FeatureID const &)>;
-  using TReadFeatureCallback = function<void (FeatureType const &)>;
-  using TReadFeaturesFn = function<void (TReadFeatureCallback const & , vector<FeatureID> const &)>;
-  using TReadIDsFn = function<void (TReadIdCallback const & , m2::RectD const &, int)>;
+  template <typename T> using TReadCallback = function<void (T const &)>;
+  using TReadFeaturesFn = function<void (TReadCallback<FeatureType> const & , vector<FeatureID> const &)>;
+  using TReadIDsFn = function<void (TReadCallback<FeatureID> const & , m2::RectD const &, int)>;
   using TResolveCountryFn = function<storage::TIndex (m2::PointF const &)>;
 
   MapDataProvider(TReadIDsFn const & idsReader,
                   TReadFeaturesFn const & featureReader,
                   TResolveCountryFn const & countryResolver);
 
-  void ReadFeaturesID(TReadIdCallback const & fn, m2::RectD const & r, int scale) const;
-  void ReadFeatures(TReadFeatureCallback const & fn, vector<FeatureID> const & ids) const;
+  void ReadFeaturesID(TReadCallback<FeatureID> const & fn, m2::RectD const & r, int scale) const;
+  void ReadFeatures(TReadCallback<FeatureType> const & fn, vector<FeatureID> const & ids) const;
 
   storage::TIndex FindCountry(m2::PointF const & pt);
 
diff --git a/drape_frontend/read_manager.cpp b/drape_frontend/read_manager.cpp
index 83447b540e..9713b8de15 100644
--- a/drape_frontend/read_manager.cpp
+++ b/drape_frontend/read_manager.cpp
@@ -61,7 +61,7 @@ void ReadManager::UpdateCoverage(ScreenBase const & screen, set<TileKey> const &
   else
   {
     // Find rects that go out from viewport
-    buffer_vector<TTileInfoPtr, 8> outdatedTiles;
+    buffer_vector<shared_ptr<TileInfo>, 8> outdatedTiles;
 #ifdef _MSC_VER
     vs_bug::
 #endif
@@ -87,13 +87,12 @@ void ReadManager::UpdateCoverage(ScreenBase const & screen, set<TileKey> const &
 
 void ReadManager::Invalidate(set<TileKey> const & keyStorage)
 {
-  TTileSet::iterator it = m_tileInfos.begin();
-  for (; it != m_tileInfos.end(); ++it)
+  for (auto & info : m_tileInfos)
   {
-    if (keyStorage.find((*it)->GetTileKey()) != keyStorage.end())
+    if (keyStorage.find(info->GetTileKey()) != keyStorage.end())
     {
-      CancelTileInfo(*it);
-      PushTaskFront(*it);
+      CancelTileInfo(info);
+      PushTaskFront(info);
     }
   }
 }
@@ -121,26 +120,26 @@ bool ReadManager::MustDropAllTiles(ScreenBase const & screen) const
 
 void ReadManager::PushTaskBackForTileKey(TileKey const & tileKey)
 {
-  TTileInfoPtr tileInfo(new TileInfo(EngineContext(tileKey, m_commutator)));
+  shared_ptr<TileInfo> tileInfo(new TileInfo(EngineContext(tileKey, m_commutator)));
   m_tileInfos.insert(tileInfo);
   ReadMWMTask * task = myPool.Get();
   task->Init(tileInfo);
   m_pool->PushBack(task);
 }
 
-void ReadManager::PushTaskFront(TTileInfoPtr const & tileToReread)
+void ReadManager::PushTaskFront(shared_ptr<TileInfo> const & tileToReread)
 {
   ReadMWMTask * task = myPool.Get();
   task->Init(tileToReread);
   m_pool->PushFront(task);
 }
 
-void ReadManager::CancelTileInfo(TTileInfoPtr const & tileToCancel)
+void ReadManager::CancelTileInfo(shared_ptr<TileInfo> const & tileToCancel)
 {
   tileToCancel->Cancel(m_memIndex);
 }
 
-void ReadManager::ClearTileInfo(TTileInfoPtr const & tileToClear)
+void ReadManager::ClearTileInfo(shared_ptr<TileInfo> const & tileToClear)
 {
   CancelTileInfo(tileToClear);
   m_tileInfos.erase(tileToClear);
diff --git a/drape_frontend/read_manager.hpp b/drape_frontend/read_manager.hpp
index 72b5f8fe5c..12aee94e75 100644
--- a/drape_frontend/read_manager.hpp
+++ b/drape_frontend/read_manager.hpp
@@ -21,8 +21,6 @@ namespace df
 class MapDataProvider;
 class CoverageUpdateDescriptor;
 
-typedef shared_ptr<TileInfo> TTileInfoPtr;
-
 class ReadManager
 {
 public:
@@ -39,7 +37,7 @@ private:
   bool MustDropAllTiles(ScreenBase const & screen) const;
 
   void PushTaskBackForTileKey(TileKey const & tileKey);
-  void PushTaskFront(TTileInfoPtr const & tileToReread);
+  void PushTaskFront(shared_ptr<TileInfo> const & tileToReread);
 
 private:
   MemoryFeatureIndex m_memIndex;
@@ -51,21 +49,21 @@ private:
 
   ScreenBase m_currentViewport;
 
-  struct LessByTileKey
+  struct LessByTileInfo
   {
-    bool operator ()(TTileInfoPtr const & l, TTileInfoPtr const & r) const
+    bool operator ()(shared_ptr<TileInfo> const & l, shared_ptr<TileInfo> const & r) const
     {
       return *l < *r;
     }
   };
 
-  using TTileSet = set<TTileInfoPtr, LessByTileKey>;
+  using TTileSet = set<shared_ptr<TileInfo>, LessByTileInfo>;
   TTileSet m_tileInfos;
 
   ObjectPool<ReadMWMTask, ReadMWMTaskFactory> myPool;
 
-  void CancelTileInfo(TTileInfoPtr const & tileToCancel);
-  void ClearTileInfo(TTileInfoPtr const & tileToClear);
+  void CancelTileInfo(shared_ptr<TileInfo> const & tileToCancel);
+  void ClearTileInfo(shared_ptr<TileInfo> const & tileToClear);
 };
 
 } // namespace df
diff --git a/drape_frontend/read_mwm_task.hpp b/drape_frontend/read_mwm_task.hpp
index 62c952fc71..a6c389a8b8 100644
--- a/drape_frontend/read_mwm_task.hpp
+++ b/drape_frontend/read_mwm_task.hpp
@@ -45,6 +45,7 @@ public:
     : m_memIndex(memIndex)
     , m_model(model) {}
 
+  /// Caller must handle object life cycle
   ReadMWMTask * GetNew() const
   {
     return new ReadMWMTask(m_memIndex, m_model);
diff --git a/drape_frontend/rule_drawer.hpp b/drape_frontend/rule_drawer.hpp
index e1543adb1b..b0600b4765 100644
--- a/drape_frontend/rule_drawer.hpp
+++ b/drape_frontend/rule_drawer.hpp
@@ -16,11 +16,11 @@ namespace df
 
 class EngineContext;
 class Stylist;
-typedef function<void (FeatureType const &, Stylist &)> TDrawerCallback;
 
 class RuleDrawer
 {
 public:
+  using TDrawerCallback = function<void (FeatureType const &, Stylist &)>;
   RuleDrawer(TDrawerCallback const & fn,
              EngineContext & context);
 
diff --git a/drape_frontend/tile_info.hpp b/drape_frontend/tile_info.hpp
index 872be0c1cb..31e948f174 100644
--- a/drape_frontend/tile_info.hpp
+++ b/drape_frontend/tile_info.hpp
@@ -9,6 +9,7 @@
 #include "base/exception.hpp"
 #include "base/mutex.hpp"
 
+#include "std/atomic.hpp"
 #include "std/mutex.hpp"
 #include "std/noncopyable.hpp"
 #include "std/vector.hpp"
@@ -50,7 +51,7 @@ private:
   EngineContext m_context;
   vector<FeatureInfo> m_featureInfo;
 
-  bool m_isCanceled;
+  atomic<bool> m_isCanceled;
   mutex m_mutex;
 };
 
diff --git a/drape_gui/country_status_helper.cpp b/drape_gui/country_status_helper.cpp
index f1d178df79..d71fca749d 100644
--- a/drape_gui/country_status_helper.cpp
+++ b/drape_gui/country_status_helper.cpp
@@ -39,12 +39,12 @@ void FormatMapSize(uint64_t sizeInBytes, string & units, size_t & sizeToDownload
   int const kbInBytes = 1024;
   if (sizeInBytes > mbInBytes)
   {
-    sizeToDownload = (sizeInBytes + (mbInBytes >> 1)) / mbInBytes;
+    sizeToDownload = (sizeInBytes + mbInBytes - 1) / mbInBytes;
     units = "MB";
   }
   else if (sizeInBytes > kbInBytes)
   {
-    sizeToDownload = (sizeInBytes + (kbInBytes >> 1)) / kbInBytes;
+    sizeToDownload = (sizeInBytes + kbInBytes -1) / kbInBytes;
     units = "KB";
   }
   else
@@ -61,7 +61,7 @@ char const * DownloadingLabelID = "country_status_downloading";
 char const * DownloadingFailedID = "country_status_download_failed";
 char const * InQueueID = "country_status_added_to_queue";
 
-}
+} // namespace
 
 ////////////////////////////////////////////////////////////
 
@@ -148,7 +148,8 @@ string CountryStatusHelper::GetProgressValue() const
 void CountryStatusHelper::FillControlsForState()
 {
   m_controls.clear();
-  switch (m_state)
+  ECountryState state = m_state;
+  switch (state)
   {
   case COUNTRY_STATE_EMPTY:
     FillControlsForEmpty();
@@ -198,7 +199,7 @@ void CountryStatusHelper::FillControlsForLoading()
   {
     string secondLabel = text.substr(secondPos + 1);
     strings::Trim(secondLabel , "\n ");
-    m_controls.push_back(MakeLabel(secondLabel ));
+    m_controls.push_back(MakeLabel(secondLabel));
   }
 }
 
diff --git a/drape_gui/country_status_helper.hpp b/drape_gui/country_status_helper.hpp
index 76a30ac2f6..2675f92666 100644
--- a/drape_gui/country_status_helper.hpp
+++ b/drape_gui/country_status_helper.hpp
@@ -4,6 +4,7 @@
 
 #include "../base/buffer_vector.hpp"
 
+#include "../std/atomic.hpp"
 #include "../std/string.hpp"
 #include "../std/unique_ptr.hpp"
 
@@ -55,6 +56,10 @@ public:
 
   void SetState(ECountryState state);
   ECountryState GetState() const;
+  /// CountryStatusHandle work on FrontendRenderer and call this function to check "is visible"
+  /// or state has already changed.
+  /// State changes from BackendRenderer thread, when recache operation started.
+  /// In that moment no need to show old CountryStatus
   bool IsVisibleForState(ECountryState state) const;
 
   size_t GetComponentCount() const;
@@ -78,7 +83,7 @@ private:
   string FormatTryAgain();
 
 private:
-  ECountryState m_state;
+  atomic<ECountryState> m_state;
   buffer_vector<Control, 4> m_controls;
   dp::RefPointer<StorageAccessor> m_accessor;
 };
diff --git a/drape_gui/drape_gui.hpp b/drape_gui/drape_gui.hpp
index 7f699fded0..5a15141d50 100644
--- a/drape_gui/drape_gui.hpp
+++ b/drape_gui/drape_gui.hpp
@@ -21,7 +21,7 @@ class CountryStatusHelper;
 class StorageAccessor
 {
 public:
-  using TSlotFn = function<void (void)>;
+  using TSlotFn = function<void ()>;
 
   virtual ~StorageAccessor() {}
   virtual string GetCurrentCountryName() const = 0;
diff --git a/map/framework.cpp b/map/framework.cpp
index 2b011c7a23..ead2a491cc 100644
--- a/map/framework.cpp
+++ b/map/framework.cpp
@@ -1,7 +1,7 @@
 #include "map/framework.hpp"
 
-#include "map/geourl_process.hpp"
 #include "map/ge0_parser.hpp"
+#include "map/geourl_process.hpp"
 #include "map/storage_bridge.hpp"
 
 #include "defines.hpp"
@@ -1366,16 +1366,14 @@ void Framework::CreateDrapeEngine(dp::RefPointer<dp::OGLContextFactory> contextF
 {
   using TReadIDsFn = df::MapDataProvider::TReadIDsFn;
   using TReadFeaturesFn = df::MapDataProvider::TReadFeaturesFn;
-  using TReadIdCallback = df::MapDataProvider::TReadIdCallback;
-  using TReadFeatureCallback = df::MapDataProvider::TReadFeatureCallback;
   using TResolveCountryFn = df::MapDataProvider::TResolveCountryFn;
 
-  TReadIDsFn idReadFn = [this](TReadIdCallback const & fn, m2::RectD const & r, int scale) -> void
+  TReadIDsFn idReadFn = [this](df::MapDataProvider::TReadCallback<FeatureID> const & fn, m2::RectD const & r, int scale) -> void
   {
     m_model.ForEachFeatureID(r, fn, scale);
   };
 
-  TReadFeaturesFn featureReadFn = [this](TReadFeatureCallback const & fn, vector<FeatureID> const & ids) -> void
+  TReadFeaturesFn featureReadFn = [this](df::MapDataProvider::TReadCallback<FeatureType> const & fn, vector<FeatureID> const & ids) -> void
   {
     m_model.ReadFeatures(fn, ids);
   };
diff --git a/map/storage_bridge.hpp b/map/storage_bridge.hpp
index 47bf4189b8..772f5a3825 100644
--- a/map/storage_bridge.hpp
+++ b/map/storage_bridge.hpp
@@ -7,6 +7,7 @@
 #include "../storage/index.hpp"
 #include "../storage/storage_defines.hpp"
 
+/// Provide access to Storage in DrapeGui subsystem. Need to CountryStatus buttons
 class StorageBridge : public gui::StorageAccessor
                     , public storage::ActiveMapsLayout::ActiveMapsListener
 {