Review fixes.

3party/bsdiff-courgette/bsdiff/bsdiff.h

@@ -439,7 +439,7 @@ BSDiffStatus ApplyBinaryPatch(OldReader & old_reader, NewSink & new_sink,
 
   // We will check whether the application process has been cancelled
   // upon copying every |kCheckCancelledPeriod| bytes from the old file.
-  const size_t kCheckCancelledPeriod = 100 * 1024;
+  constexpr size_t kCheckCancelledPeriod = 100 * 1024;
 
   while (control_stream_copy_counts.Size() > 0) {
     if (cancellable.IsCancelled())

generator/mwm_diff/diff.cpp

@@ -78,20 +78,19 @@ generator::mwm_diff::DiffApplicationResult ApplyDiffVersion0(
   // its checksum is compared to the one stored in the diff file.
   MemReaderWithExceptions diffMemReader(diffBuf.data(), diffBuf.size());
 
-  auto status = bsdiff::ApplyBinaryPatch(oldReader, newWriter, diffMemReader, cancellable);
+  auto const status = bsdiff::ApplyBinaryPatch(oldReader, newWriter, diffMemReader, cancellable);
 
   if (status == bsdiff::BSDiffStatus::CANCELLED)
   {
     LOG(LDEBUG, ("Diff application has been cancelled"));
     return DiffApplicationResult::Cancelled;
   }
-  if (status != bsdiff::BSDiffStatus::OK)
-  {
-    LOG(LERROR, ("Could not apply patch with bsdiff:", status));
-    return DiffApplicationResult::Failed;
-  }
 
-  return DiffApplicationResult::Ok;
+  if (status == bsdiff::BSDiffStatus::OK)
+    return DiffApplicationResult::Ok;
+
+  LOG(LERROR, ("Could not apply patch with bsdiff:", status));
+  return DiffApplicationResult::Failed;
 }
 }  // namespace
 

generator/mwm_diff/pymwm_diff/bindings.cpp

@@ -32,7 +32,7 @@ namespace
 bool ApplyDiff(string const & oldMwmPath, string const & newMwmPath, string const & diffPath)
 {
   base::Cancellable cancellable;
-  auto result = generator::mwm_diff::ApplyDiff(oldMwmPath, newMwmPath, diffPath, cancellable);
+  auto const result = generator::mwm_diff::ApplyDiff(oldMwmPath, newMwmPath, diffPath, cancellable);
 
   switch (result)
   {
@@ -44,6 +44,7 @@ bool ApplyDiff(string const & oldMwmPath, string const & newMwmPath, string cons
     UNREACHABLE();
     break;
   }
+
   UNREACHABLE();
 }
 }  // namespace

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;

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); }

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>;

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));