[storage][ios] review fixes for background downloading

mapsme · Nov 10, 2020 · 44cb3a6 · 44cb3a6
1 parent a6bce01
commit 44cb3a6
Show file tree

Hide file tree

Showing 11 changed files with 27 additions and 31 deletions.
diff --git a/platform/downloader_utils.cpp b/platform/downloader_utils.cpp
@@ -38,7 +38,7 @@ std::string GetFilePathByUrl(std::string const & url)
   CHECK(strings::to_int64(urlComponents[1], dataVersion), ());
 
   auto const countryComponents = strings::Tokenize(url::UrlDecode(urlComponents.back()), ".");
-  CHECK(!urlComponents.empty(), ());
+  CHECK(!countryComponents.empty(), ());
 
   auto const fileType = urlComponents[0] == kDiffsPath ? MapFileType::Diff : MapFileType::Map;
 

diff --git a/storage/background_downloading/downloader_adapter_ios.h b/storage/background_downloading/downloader_adapter_ios.h
@@ -7,27 +7,27 @@
 
 namespace storage
 {
-class BackgroundDownloaderAdapter : public storage::MapFilesDownloaderWithPing
+class BackgroundDownloaderAdapter : public MapFilesDownloaderWithPing
 {
 public:
   BackgroundDownloaderAdapter();
 
   // MapFilesDownloader overrides:
-  void Remove(storage::CountryId const & countryId) override;
+  void Remove(CountryId const & countryId) override;
 
   void Clear() override;
 
-  storage::QueueInterface const & GetQueue() const override;
+  QueueInterface const & GetQueue() const override;
 
 private:
   // MapFilesDownloaderWithServerList overrides:
-  void Download(storage::QueuedCountry & queuedCountry) override;
+  void Download(QueuedCountry & queuedCountry) override;
 
   // Trying to download mwm from different servers recursively.
   void DownloadFromAnyUrl(CountryId const & countryId, std::string const & downloadPath,
                           std::vector<std::string> const & urls);
 
-  storage::BackgroundDownloaderQueue m_queue;
+  BackgroundDownloaderQueue m_queue;
   SubscriberAdapter * m_subscriberAdapter;
 };
 }  // namespace storage
diff --git a/storage/background_downloading/downloader_adapter_ios.mm b/storage/background_downloading/downloader_adapter_ios.mm
@@ -19,7 +19,7 @@
   [downloader subscribe:m_subscriberAdapter];
 }
 
-void BackgroundDownloaderAdapter::Remove(storage::CountryId const & countryId)
+void BackgroundDownloaderAdapter::Remove(CountryId const & countryId)
 {
   MapFilesDownloader::Remove(countryId);
 
@@ -42,15 +42,15 @@
   m_queue.Clear();
 };
 
-storage::QueueInterface const & BackgroundDownloaderAdapter::GetQueue() const
+QueueInterface const & BackgroundDownloaderAdapter::GetQueue() const
 {
   if (m_queue.IsEmpty())
     return MapFilesDownloader::GetQueue();
 
   return m_queue;
 }
 
-void BackgroundDownloaderAdapter::Download(storage::QueuedCountry & queuedCountry)
+void BackgroundDownloaderAdapter::Download(QueuedCountry & queuedCountry)
 {
   if (!IsDownloadingAllowed())
   {
@@ -79,17 +79,12 @@
     return;
 
   auto onFinish = [this, countryId, downloadPath, urls = urls](NSURL *location, NSError *error) mutable {
-    if ((!location && !error) || (error && error.code != NSURLErrorCancelled))
+    if (!error || error.code == NSURLErrorCancelled)
       return;
 
     downloader::DownloadStatus status = downloader::DownloadStatus::Completed;
-    if (error)
-    {
-     status = error.code == NSURLErrorFileDoesNotExist ? downloader::DownloadStatus::FileNotFound
-                                                       : downloader::DownloadStatus::Failed;
-    }
-
-    ASSERT(location, ());
+    status = error.code == NSURLErrorFileDoesNotExist ? downloader::DownloadStatus::FileNotFound
+                                                      : downloader::DownloadStatus::Failed;
 
     if (!m_queue.Contains(countryId))
       return;

diff --git a/storage/background_downloading/downloader_queue_ios.cpp b/storage/background_downloading/downloader_queue_ios.cpp
@@ -30,7 +30,7 @@ void BackgroundDownloaderQueue::SetTaskIdForCountryId(CountryId const & country,
   it->second.m_taskId = taskId;
 }
 
-boost::optional<uint64_t> BackgroundDownloaderQueue::GetTaskIdByCountryId(CountryId const & country) const
+std::optional<uint64_t> BackgroundDownloaderQueue::GetTaskIdByCountryId(CountryId const & country) const
 {
   auto const it = m_queue.find(country);
   if (it == m_queue.cend())

diff --git a/storage/background_downloading/downloader_queue_ios.hpp b/storage/background_downloading/downloader_queue_ios.hpp
@@ -5,11 +5,10 @@
 #include "storage/storage_defines.hpp"
 
 #include <cstdint>
+#include <optional>
 #include <unordered_map>
 #include <utility>
 
-#include <boost/optional.hpp>
-
 namespace storage
 {
 class BackgroundDownloaderQueue : public QueueInterface
@@ -27,7 +26,7 @@ class BackgroundDownloaderQueue : public QueueInterface
   }
 
   void SetTaskIdForCountryId(CountryId const & countryId, uint64_t taskId);
-  boost::optional<uint64_t> GetTaskIdByCountryId(CountryId const & countryId) const;
+  std::optional<uint64_t> GetTaskIdByCountryId(CountryId const & countryId) const;
 
   QueuedCountry & GetCountryById(CountryId const & countryId);
 
@@ -40,7 +39,7 @@ class BackgroundDownloaderQueue : public QueueInterface
     explicit TaskData(QueuedCountry && country) : m_queuedCountry(std::move(country)) {}
 
     QueuedCountry m_queuedCountry;
-    boost::optional<uint64_t> m_taskId;
+    std::optional<uint64_t> m_taskId;
   };
 
   std::unordered_map<CountryId, TaskData> m_queue;

diff --git a/storage/downloader_queue_universal.cpp b/storage/downloader_queue_universal.cpp
@@ -58,6 +58,12 @@ void Queue::PopFront()
   m_queue.pop_front();
 }
 
+void Queue::Append(QueuedCountry && country)
+{
+  m_queue.emplace_back(std::move(country));
+  m_queue.back().OnCountryInQueue();
+}
+
 void Queue::Clear()
 {
   m_queue.clear();

diff --git a/storage/downloader_queue_universal.hpp b/storage/downloader_queue_universal.hpp
@@ -25,11 +25,7 @@ class Queue : public QueueInterface
   QueuedCountry const & GetFirstCountry() const;
   void PopFront();
 
-  void Append(QueuedCountry && country)
-  {
-    m_queue.emplace_back(std::move(country));
-    m_queue.back().OnCountryInQueue();
-  }
+  void Append(QueuedCountry && country);
 
   void Remove(CountryId const & country);
   void Clear();

diff --git a/storage/http_map_files_downloader.cpp b/storage/http_map_files_downloader.cpp
@@ -87,7 +87,7 @@ void HttpMapFilesDownloader::Remove(CountryId const & id)
   CHECK_THREAD_CHECKER(m_checker, ());
 
   MapFilesDownloader::Remove(id);
-    
+
   if (!m_queue.Contains(id))
     return;
 

diff --git a/storage/queued_country.hpp b/storage/queued_country.hpp
@@ -65,6 +65,6 @@ class QueuedCountry
   std::string m_dataDir;
   diffs::DiffsSourcePtr m_diffsDataSource;
 
-  Subscriber * m_subscriber;
+  Subscriber * m_subscriber = nullptr;
 };
 }  // namespace storage
diff --git a/storage/storage.cpp b/storage/storage.cpp
@@ -705,6 +705,8 @@ void Storage::ReportProgressForHierarchy(CountryId const & countryId, Progress c
 
 void Storage::OnCountryInQueue(QueuedCountry const & queuedCountry)
 {
+  CHECK_THREAD_CHECKER(m_threadChecker, ());
+
   NotifyStatusChangedForHierarchy(queuedCountry.GetCountryId());
   SaveDownloadQueue();
 }

diff --git a/storage/storage.hpp b/storage/storage.hpp
@@ -632,8 +632,6 @@ class Storage : public MapFilesDownloader::Subscriber,
 
   /// Calculates progress of downloading for expandable nodes in country tree.
   /// |descendants| All descendants of the parent node.
-  /// |downloadingMwm| Downloading leaf node country id if any. If not, downloadingMwm == kInvalidCountryId.
-  /// |downloadingMwm| Must be only leaf.
   downloader::Progress CalculateProgress(CountriesVec const & descendants) const;
 
   template <class ToDo>