Review fixes.

4 files changed, 40 insertions, 27 deletions

diff --git a/storage/diff_scheme/diff_manager.cpp b/storage/diff_scheme/diff_manager.cpp
index 4609c839c0..9f4e78cf46 100644
--- a/storage/diff_scheme/diff_manager.cpp
+++ b/storage/diff_scheme/diff_manager.cpp
@@ -45,9 +45,8 @@ void Manager::Load(LocalMapsInfo && info)
   });
 }
 
-void Manager::ApplyDiff(
-    ApplyDiffParams && p, base::Cancellable const & cancellable,
-    std::function<void(generator::mwm_diff::DiffApplicationResult result)> const & task)
+void Manager::ApplyDiff(ApplyDiffParams && p, base::Cancellable const & cancellable,
+                        Manager::OnDiffApplicationFinished const & task)
 {
   using namespace generator::mwm_diff;
 
@@ -58,7 +57,7 @@ void Manager::ApplyDiff(
     auto & diffReadyPath = p.m_diffReadyPath;
     auto & diffFile = p.m_diffFile;
     auto const diffPath = diffFile->GetPath(MapOptions::Diff);
-    DiffApplicationResult result = DiffApplicationResult::Failed;
+    auto result = DiffApplicationResult::Failed;
 
     diffFile->SyncWithDisk();
 
@@ -75,7 +74,7 @@ void Manager::ApplyDiff(
         std::lock_guard<std::mutex> lock(m_mutex);
         auto it = m_diffs.find(diffFile->GetCountryName());
         CHECK(it != m_diffs.end(), ());
-        it->second.m_downloaded = true;
+        it->second.m_status = SingleDiffStatus::Downloaded;
       }
 
       string const oldMwmPath = p.m_oldMwmFile->GetPath(MapOptions::Map);
@@ -84,8 +83,11 @@ void Manager::ApplyDiff(
 
       result = generator::mwm_diff::ApplyDiff(oldMwmPath, diffApplyingInProgressPath, diffPath,
                                               cancellable);
-      if (!base::RenameFileX(diffApplyingInProgressPath, newMwmPath))
+      if (result == DiffApplicationResult::Ok &&
+          !base::RenameFileX(diffApplyingInProgressPath, newMwmPath))
+      {
         result = DiffApplicationResult::Failed;
+      }
 
       base::DeleteFileX(diffApplyingInProgressPath);
 
@@ -101,10 +103,9 @@ void Manager::ApplyDiff(
       std::lock_guard<std::mutex> lock(m_mutex);
       auto it = m_diffs.find(diffFile->GetCountryName());
       CHECK(it != m_diffs.end(), ());
-      it->second.m_applied = true;
-      it->second.m_downloaded = false;
+      it->second.m_status = SingleDiffStatus::Applied;
+      break;
     }
-    break;
     case DiffApplicationResult::Cancelled: break;
     case DiffApplicationResult::Failed:
     {
@@ -120,9 +121,9 @@ void Manager::ApplyDiff(
 
       auto it = m_diffs.find(diffFile->GetCountryName());
       CHECK(it != m_diffs.end(), ());
-      it->second.m_downloaded = false;
+      it->second.m_status = SingleDiffStatus::NotDownloaded;
+      break;
     }
-    break;
     }
 
     task(result);
@@ -142,8 +143,9 @@ bool Manager::SizeFor(storage::CountryId const & countryId, uint64_t & size) con
 
 bool Manager::SizeToDownloadFor(storage::CountryId const & countryId, uint64_t & size) const
 {
-  return WithDiff(countryId,
-                  [&size](DiffInfo const & info) { size = (info.m_downloaded ? 0 : info.m_size); });
+  return WithDiff(countryId, [&size](DiffInfo const & info) {
+    size = (info.m_status == SingleDiffStatus::Downloaded ? 0 : info.m_size);
+  });
 }
 
 bool Manager::VersionFor(storage::CountryId const & countryId, uint64_t & version) const
@@ -161,7 +163,7 @@ void Manager::RemoveAppliedDiffs()
   std::lock_guard<std::mutex> lock(m_mutex);
   for (auto it = m_diffs.begin(); it != m_diffs.end();)
   {
-    if (it->second.m_applied)
+    if (it->second.m_status == SingleDiffStatus::Applied)
       it = m_diffs.erase(it);
     else
       ++it;
diff --git a/storage/diff_scheme/diff_manager.hpp b/storage/diff_scheme/diff_manager.hpp
index 4a4e1950a6..a993498c6a 100644
--- a/storage/diff_scheme/diff_manager.hpp
+++ b/storage/diff_scheme/diff_manager.hpp
@@ -40,14 +40,19 @@ public:
     virtual void OnDiffStatusReceived(Status const status) = 0;
   };
 
-  // Returns the size of the diff file for |countryId| or 0 if there is no diff.
-  bool SizeFor(storage::CountryId const & countryId, uint64_t & size) const;
+  using OnDiffApplicationFinished =
+      std::function<void(generator::mwm_diff::DiffApplicationResult result)>;
 
-  // Returns how many bytes are left for the diff to be downloaded for |countryId|
+  // If the diff is available, sets |size| to its size and returns true.
+  // Otherwise, returns false.
+  bool SizeFor(storage::TCountryId const & countryId, uint64_t & size) const;
+
+  // Sets |size| to how many bytes are left for the diff to be downloaded for |countryId|
   // or 0 if there is no diff available or it has already been downloaded.
   // This method may overestimate because it does not account for the possibility
   // of resuming an old download, i.e. the return value is either 0 or the diff size.
-  bool SizeToDownloadFor(storage::CountryId const & countryId, uint64_t & size) const;
+  // Returns true iff the diff is available.
+  bool SizeToDownloadFor(storage::TCountryId const & countryId, uint64_t & size) const;
 
   bool VersionFor(storage::CountryId const & countryId, uint64_t & version) const;
   bool IsPossibleToAutoupdate() const;
@@ -62,9 +67,8 @@ public:
   Status GetStatus() const;
 
   void Load(LocalMapsInfo && info);
-  void ApplyDiff(
-      ApplyDiffParams && p, base::Cancellable const & cancellable,
-      std::function<void(generator::mwm_diff::DiffApplicationResult result)> const & task);
+  void ApplyDiff(ApplyDiffParams && p, base::Cancellable const & cancellable,
+                 OnDiffApplicationFinished const & task);
 
   bool AddObserver(Observer & observer) { return m_observers.Add(observer); }
   bool RemoveObserver(Observer const & observer) { return m_observers.Remove(observer); }
diff --git a/storage/diff_scheme/diff_types.hpp b/storage/diff_scheme/diff_types.hpp
index 2f44e80b57..823895d7bc 100644
--- a/storage/diff_scheme/diff_types.hpp
+++ b/storage/diff_scheme/diff_types.hpp
@@ -10,6 +10,7 @@ namespace storage
 {
 namespace diffs
 {
+// Status of the diff manager as a whole.
 enum class Status
 {
   Undefined,
@@ -17,13 +18,21 @@ enum class Status
   Available
 };
 
+// Status of a single diff.
+enum class SingleDiffStatus
+{
+  NotDownloaded,
+  Downloaded,
+  Applied
+};
+
 struct DiffInfo final
 {
   DiffInfo(uint64_t size, uint64_t version) : m_size(size), m_version(version) {}
+
   uint64_t m_size;
   uint64_t m_version;
-  bool m_applied = false;
-  bool m_downloaded = false;
+  SingleDiffStatus m_status = SingleDiffStatus::NotDownloaded;
 };
 
 using NameDiffInfoMap = std::unordered_map<storage::CountryId, DiffInfo>;
diff --git a/storage/storage.cpp b/storage/storage.cpp
index d99c75fd68..d3dc57c1c4 100644
--- a/storage/storage.cpp
+++ b/storage/storage.cpp
@@ -951,12 +951,8 @@ void Storage::RegisterDownloadedFiles(CountryId const & countryId, MapOptions op
 
   if (options == MapOptions::Diff)
   {
-    /// At this point a diff applying process is going to start
-    /// and we can't stop the process.
-    /// TODO: Make the applying process cancellable.
     m_queue.begin()->SetFrozen(true);
     NotifyStatusChangedForHierarchy(countryId);
-    m_diffsCancellable.Reset();
     ApplyDiff(countryId, fn);
     return;
   }
@@ -1561,6 +1557,8 @@ void Storage::LoadDiffScheme()
 
 void Storage::ApplyDiff(CountryId const & countryId, function<void(bool isSuccess)> const & fn)
 {
+  m_diffsCancellable.Reset();
+
   diffs::Manager::ApplyDiffParams params;
   params.m_diffFile =
       PreparePlaceForCountryFiles(GetCurrentDataVersion(), m_dataDir, GetCountryFile(countryId));