Review fixes.

indexer/index.cpp

@@ -123,7 +123,6 @@ int Index::RegisterMap(string const & fileName, m2::RectD & rect)
     }
   }
 
-  lock_guard<mutex> lock(m_lock);
   int rv = Register(fileName, rect);
   if (rv >= 0)
     m_observers.ForEach(&Observer::OnMapRegistered, fileName);
@@ -132,12 +131,14 @@ int Index::RegisterMap(string const & fileName, m2::RectD & rect)
 
 bool Index::DeleteMap(string const & fileName)
 {
-  lock_guard<mutex> lock(m_lock);
+  {
+    lock_guard<mutex> lock(m_lock);
 
-  if (!DeregisterImpl(fileName))
-    return false;
+    if (!DeregisterImpl(fileName))
+      return false;
 
-  DeleteMapFiles(GetFullPath(fileName), true);
+    DeleteMapFiles(GetFullPath(fileName), true);
+  }
   m_observers.ForEach(&Observer::OnMapDeleted, fileName);
   return true;
 }
@@ -154,17 +155,21 @@ bool Index::RemoveObserver(Observer & observer)
 
 int Index::UpdateMap(string const & fileName, m2::RectD & rect)
 {
-  lock_guard<mutex> lock(m_lock);
-
-  MwmId const id = GetIdByName(fileName);
-  if (id != INVALID_MWM_ID && m_info[id].m_lockCount > 0)
+  int rv = -1;
   {
-    m_info[id].SetStatus(MwmInfo::STATUS_PENDING_UPDATE);
-    return -2;
-  }
+    lock_guard<mutex> lock(m_lock);
 
-  ReplaceFileWithReady(fileName);
-  int rv = RegisterImpl(fileName, rect);
+    MwmId const id = GetIdByName(fileName);
+    if (id != INVALID_MWM_ID && m_info[id].m_lockCount > 0)
+    {
+      m_info[id].SetStatus(MwmInfo::STATUS_PENDING_UPDATE);
+      return -2;
+    }
+
+    ReplaceFileWithReady(fileName);
+
+    rv = RegisterImpl(fileName, rect);
+  }
   if (rv >= 0)
     m_observers.ForEach(&Observer::OnMapUpdated, fileName);
   return rv;

indexer/index.hpp

@@ -275,6 +275,13 @@ public:
 
   MwmId GetMwmIdByName(string const & name) const
   {
+      // const_cast<> is used here and in some other methods not only
+      // to lock/unlock mutex, which can be circumvented by marking
+      // the mutex as mutable, but also to call MwmSet::GetIdByName()
+      // method which is non-constant by design, because it updates
+      // MWM info for all registered MWMs.
+      //
+      // TODO (y@): probably a better design is possible, investigate it.
       Index * p = const_cast<Index *>(this);
       lock_guard<mutex> lock(p->m_lock);
 

indexer/indexer_tests/index_test.cpp

@@ -23,18 +23,34 @@ void CheckedDeleteFile(string const & file)
 class Observer : public Index::Observer
 {
 public:
-  Observer() : m_map_registered_calls(0), m_map_updated_calls(0), m_map_deleted_calls(0) {}
+  Observer(string const & file)
+      : m_file(file), m_map_registered_calls(0), m_map_updated_calls(0), m_map_deleted_calls(0)
+  {
+  }
 
   // Index::Observer overrides:
-  void OnMapRegistered(string const & file) override { ++m_map_registered_calls; }
-  void OnMapUpdated(string const & file) override { ++m_map_updated_calls; }
-  void OnMapDeleted(string const & file) override { ++m_map_deleted_calls; }
+  void OnMapRegistered(string const & file) override
+  {
+    CHECK_EQUAL(m_file, file, ());
+    ++m_map_registered_calls;
+  }
+  void OnMapUpdated(string const & file) override
+  {
+    CHECK_EQUAL(m_file, file, ());
+    ++m_map_updated_calls;
+  }
+  void OnMapDeleted(string const & file) override
+  {
+    CHECK_EQUAL(m_file, file, ());
+    ++m_map_deleted_calls;
+  }
 
   int map_registered_calls() const { return m_map_registered_calls; }
   int map_updated_calls() const { return m_map_updated_calls; }
   int map_deleted_calls() const { return m_map_deleted_calls; }
 
 private:
+  string const m_file;
   int m_map_registered_calls;
   int m_map_updated_calls;
   int m_map_deleted_calls;
@@ -65,19 +81,19 @@ UNIT_TEST(Index_MwmStatusNotifications)
   MY_SCOPE_GUARD(testMapGuard, bind(&CheckedDeleteFile, testMapPath));
 
   Index index;
-  Observer observer;
+  Observer observer(testMapName);
   index.AddObserver(observer);
 
   TEST_EQUAL(0, observer.map_registered_calls(), ());
   m2::RectD dummyRect;
 
   // Check that observers are triggered after map registration.
-  TEST_LESS_OR_EQUAL(0, index.RegisterMap(testMapPath, dummyRect), ());
+  TEST_LESS_OR_EQUAL(0, index.RegisterMap(testMapName, dummyRect), ());
   TEST_EQUAL(1, observer.map_registered_calls(), ());
 
   // Check that map can't registered twice and observers aren't
   // triggered.
-  TEST_EQUAL(-1, index.RegisterMap(testMapPath, dummyRect), ());
+  TEST_EQUAL(-1, index.RegisterMap(testMapName, dummyRect), ());
   TEST_EQUAL(1, observer.map_registered_calls(), ());
 
   TEST(my::CopyFileX(testMapPath, testMapUpdatePath), ());

indexer/mwm_set.hpp

@@ -31,6 +31,7 @@ public:
   m2::RectD m_limitRect;  ///< Limit rect of mwm.
   uint8_t m_minScale;     ///< Min zoom level of mwm.
   uint8_t m_maxScale;     ///< Max zoom level of mwm.
+  uint8_t m_lockCount;    ///< Number of locks.
 
   inline bool IsRegistered() const
   {
@@ -45,8 +46,6 @@ public:
 
   inline Status GetStatus() const { return m_status; }
 
-  uint8_t m_lockCount;  ///< Number of locks.
-
 private:
   Status m_status;  ///< Current country status.
 };