[android][downloader] review fixes

9 files changed, 95 insertions, 57 deletions

diff --git a/android/jni/com/mapswithme/maps/DownloaderAdapter.cpp b/android/jni/com/mapswithme/maps/DownloaderAdapter.cpp
index d4a5cb436d..f9046a27ed 100644
--- a/android/jni/com/mapswithme/maps/DownloaderAdapter.cpp
+++ b/android/jni/com/mapswithme/maps/DownloaderAdapter.cpp
@@ -35,14 +35,14 @@ BackgroundDownloaderAdapter::BackgroundDownloaderAdapter()
 
 BackgroundDownloaderAdapter::~BackgroundDownloaderAdapter()
 {
-  CHECK_THREAD_CHECKER(m_threadChecker, (""));
+  CHECK_THREAD_CHECKER(m_threadChecker, ());
 
   g_completionHandlers.clear();
 }
 
 void BackgroundDownloaderAdapter::Remove(CountryId const & countryId)
 {
-  CHECK_THREAD_CHECKER(m_threadChecker, (""));
+  CHECK_THREAD_CHECKER(m_threadChecker, ());
 
   MapFilesDownloader::Remove(countryId);
 
@@ -69,7 +69,7 @@ void BackgroundDownloaderAdapter::Remove(CountryId const & countryId)
 
 void BackgroundDownloaderAdapter::Clear()
 {
-  CHECK_THREAD_CHECKER(m_threadChecker, (""));
+  CHECK_THREAD_CHECKER(m_threadChecker, ());
 
   MapFilesDownloader::Clear();
 
@@ -92,7 +92,7 @@ void BackgroundDownloaderAdapter::Clear()
 
 QueueInterface const & BackgroundDownloaderAdapter::GetQueue() const
 {
-  CHECK_THREAD_CHECKER(m_threadChecker, (""));
+  CHECK_THREAD_CHECKER(m_threadChecker, ());
 
   if (m_queue.IsEmpty())
     return MapFilesDownloader::GetQueue();
@@ -102,7 +102,7 @@ QueueInterface const & BackgroundDownloaderAdapter::GetQueue() const
 
 void BackgroundDownloaderAdapter::Download(QueuedCountry & queuedCountry)
 {
-  CHECK_THREAD_CHECKER(m_threadChecker, (""));
+  CHECK_THREAD_CHECKER(m_threadChecker, ());
 
   if (!IsDownloadingAllowed())
   {
@@ -134,18 +134,21 @@ void BackgroundDownloaderAdapter::DownloadFromLastUrl(CountryId const & countryI
                                                      std::string const & downloadPath,
                                                      std::vector<std::string> && urls)
 {
-  CHECK_THREAD_CHECKER(m_threadChecker, (""));
+  CHECK_THREAD_CHECKER(m_threadChecker, ());
+  ASSERT(!urls.empty(), ());
 
   auto env = jni::GetEnv();
   jni::TScopedLocalRef url(env, jni::ToJavaString(env, urls.back()));
   auto id = static_cast<int64_t>(env->CallLongMethod(*m_downloadManager, m_downloadManagerEnqueue, url.get()));
 
+  jni::HandleJavaException(env);
+
   m_queue.SetTaskInfoForCountryId(countryId, id);
 
   urls.pop_back();
   auto onFinish = [this, countryId, downloadPath, urls = std::move(urls)](bool status) mutable
   {
-    CHECK_THREAD_CHECKER(m_threadChecker, (""));
+    CHECK_THREAD_CHECKER(m_threadChecker, ());
 
     if (!m_queue.Contains(countryId))
       return;
@@ -159,7 +162,9 @@ void BackgroundDownloaderAdapter::DownloadFromLastUrl(CountryId const & countryI
     {
       auto const country = m_queue.GetCountryById(countryId);
       m_queue.Remove(countryId);
-      country.OnDownloadFinished(downloader::DownloadStatus::Completed);
+      country.OnDownloadFinished(status
+                                 ? downloader::DownloadStatus::Completed
+                                 : downloader::DownloadStatus::Failed);
       if (m_queue.IsEmpty())
       {
         for (auto const subscriber : m_subscribers)
@@ -170,7 +175,7 @@ void BackgroundDownloaderAdapter::DownloadFromLastUrl(CountryId const & countryI
 
   auto onProgress = [this, countryId](int64_t bytesDownloaded, int64_t bytesTotal)
   {
-    CHECK_THREAD_CHECKER(m_threadChecker, (""));
+    CHECK_THREAD_CHECKER(m_threadChecker, ());
 
     if (!m_queue.Contains(countryId))
       return;
@@ -186,6 +191,8 @@ void BackgroundDownloaderAdapter::RemoveByRequestId(int64_t id)
 {
   auto env = jni::GetEnv();
   env->CallVoidMethod(*m_downloadManager, m_downloadManagerRemove, static_cast<jlong>(id));
+
+  jni::HandleJavaException(env);
 }
 
 std::unique_ptr<MapFilesDownloader> GetDownloader()
diff --git a/android/src/com/mapswithme/maps/background/AbstractLogBroadcastReceiver.java b/android/src/com/mapswithme/maps/background/AbstractLogBroadcastReceiver.java
index 84fa4842b4..06ac0b7c2a 100644
--- a/android/src/com/mapswithme/maps/background/AbstractLogBroadcastReceiver.java
+++ b/android/src/com/mapswithme/maps/background/AbstractLogBroadcastReceiver.java
@@ -23,7 +23,7 @@ public abstract class AbstractLogBroadcastReceiver extends BroadcastReceiver
   {
     if (intent == null)
     {
-      LOGGER.w(getTag(), "An intent with null intent detected");
+      LOGGER.w(getTag(), "A null intent detected");
       return;
     }
 
diff --git a/android/src/com/mapswithme/maps/background/SystemDownloadCompletedService.java b/android/src/com/mapswithme/maps/background/SystemDownloadCompletedService.java
index de61c5d2b5..2d09daf323 100644
--- a/android/src/com/mapswithme/maps/background/SystemDownloadCompletedService.java
+++ b/android/src/com/mapswithme/maps/background/SystemDownloadCompletedService.java
@@ -1,6 +1,7 @@
 package com.mapswithme.maps.background;
 
 import android.app.DownloadManager;
+import android.content.Context;
 import android.content.Intent;
 import android.database.Cursor;
 
@@ -13,6 +14,11 @@ import com.mapswithme.util.Utils;
 
 public class SystemDownloadCompletedService extends JobIntentService
 {
+  private interface DownloadProcessor
+  {
+    boolean process(@NonNull Context context, long id, @NonNull Cursor cursor);
+  }
+
   @Override
   public void onCreate()
   {
@@ -44,10 +50,16 @@ public class SystemDownloadCompletedService extends JobIntentService
       if (!cursor.moveToFirst())
         return;
 
-      if (MapDownloadCompletedProcessor.isSupported(cursor))
-        MapDownloadCompletedProcessor.process(getApplicationContext(), id, cursor);
-      else
-        BookmarksDownloadCompletedProcessor.process(getApplicationContext(), cursor);
+      final DownloadProcessor[] processors = {
+          MapDownloadCompletedProcessor::process,
+          BookmarksDownloadCompletedProcessor::process
+      };
+
+      for (DownloadProcessor processor : processors)
+      {
+        if (processor.process(getApplicationContext(), id, cursor))
+          break;
+      }
     }
     finally
     {
diff --git a/android/src/com/mapswithme/maps/bookmarks/BookmarksDownloadCompletedProcessor.java b/android/src/com/mapswithme/maps/bookmarks/BookmarksDownloadCompletedProcessor.java
index 5ce7dabfb4..b35fa25a82 100644
--- a/android/src/com/mapswithme/maps/bookmarks/BookmarksDownloadCompletedProcessor.java
+++ b/android/src/com/mapswithme/maps/bookmarks/BookmarksDownloadCompletedProcessor.java
@@ -10,6 +10,7 @@ import android.text.TextUtils;
 
 import androidx.annotation.NonNull;
 import androidx.annotation.Nullable;
+import androidx.annotation.WorkerThread;
 import androidx.localbroadcastmanager.content.LocalBroadcastManager;
 import com.mapswithme.maps.MwmApplication;
 import com.mapswithme.maps.bookmarks.data.Error;
@@ -26,39 +27,44 @@ import java.net.URLDecoder;
 
 public class BookmarksDownloadCompletedProcessor
 {
+  private static final Logger LOGGER = LoggerFactory.INSTANCE.getLogger(LoggerFactory.Type.DOWNLOADER);
+  private static final String TAG = BookmarksDownloadCompletedProcessor.class.getSimpleName();
+
   public final static String ACTION_DOWNLOAD_COMPLETED = "action_download_completed";
   public final static String EXTRA_DOWNLOAD_STATUS = "extra_download_status";
 
-  public static void process(@NonNull Context context, @NonNull Cursor cursor)
+  @WorkerThread
+  public static boolean process(@NonNull Context context, long id, @NonNull Cursor cursor)
   {
-    final OperationStatus status = processInternal(context, cursor);
-    Logger logger = LoggerFactory.INSTANCE.getLogger(LoggerFactory.Type.BILLING);
-    String tag = BookmarksDownloadCompletedProcessor.class.getSimpleName();
-    logger.i(tag, "Download status: " + status);
+    try
+    {
+      final OperationStatus status = processInternal(context, cursor);
+      LOGGER.i(TAG, "Download status: " + status);
+
+      UiThread.run(new BookmarkDownloadCompletedTask(context, status));
+    }
+    catch (Exception e)
+    {
+      LOGGER.e(TAG, "Failed to process bookmark download for request id " + id + ". Exception ", e);
+      return false;
+    }
 
-    UiThread.run(new BookmarkDownloadCompletedTask(context, status));
+    return true;
   }
 
   @NonNull
   private static OperationStatus processInternal(@NonNull Context context, @NonNull Cursor cursor)
   {
-    try
+    if (isDownloadFailed(cursor))
     {
-      if (isDownloadFailed(cursor))
-      {
-        Error error = new Error(getHttpStatus(cursor), getErrorMessage(cursor));
-        return new OperationStatus(null, error);
-      }
+      Error error = new Error(getHttpStatus(cursor), getErrorMessage(cursor));
+      return new OperationStatus(null, error);
+    }
 
-      logToPushWoosh((Application) context, cursor);
+    logToPushWoosh((Application) context, cursor);
 
-      Result result = new Result(getFilePath(cursor), getArchiveId(cursor));
-      return new OperationStatus(result, null);
-    }
-    catch (Exception e)
-    {
-      return new OperationStatus(null, new Error(e.getMessage()));
-    }
+    Result result = new Result(getFilePath(cursor), getArchiveId(cursor));
+    return new OperationStatus(result, null);
   }
 
   private static boolean isDownloadFailed(@NonNull Cursor cursor)
diff --git a/android/src/com/mapswithme/maps/downloader/MapDownloadCompletedProcessor.java b/android/src/com/mapswithme/maps/downloader/MapDownloadCompletedProcessor.java
index b19f6a5bd9..bd346cc855 100644
--- a/android/src/com/mapswithme/maps/downloader/MapDownloadCompletedProcessor.java
+++ b/android/src/com/mapswithme/maps/downloader/MapDownloadCompletedProcessor.java
@@ -10,6 +10,7 @@ import android.text.TextUtils;
 import androidx.annotation.MainThread;
 import androidx.annotation.NonNull;
 import androidx.annotation.Nullable;
+import androidx.annotation.WorkerThread;
 import com.mapswithme.maps.MwmApplication;
 import com.mapswithme.util.concurrency.UiThread;
 import com.mapswithme.util.log.Logger;
@@ -25,29 +26,31 @@ public class MapDownloadCompletedProcessor
   private static final Logger LOGGER = LoggerFactory.INSTANCE.getLogger(LoggerFactory.Type.DOWNLOADER);
   private static final String TAG = MapDownloadCompletedProcessor.class.getSimpleName();
 
-  public static boolean isSupported(@NonNull Cursor cursor)
-  {
-    String targetUri = cursor.getString(cursor.getColumnIndex(DownloadManager.COLUMN_URI));
-    String targetUriPath = Uri.parse(targetUri).getPath();
-
-    return !TextUtils.isEmpty(targetUriPath) && MapManager.nativeIsUrlSupported(targetUriPath);
-  }
-
-  public static void process(@NonNull Context context, long id, @NonNull Cursor cursor)
+  @WorkerThread
+  public static boolean process(@NonNull Context context, long id, @NonNull Cursor cursor)
   {
     try
     {
       String targetUri = cursor.getString(cursor.getColumnIndex(DownloadManager.COLUMN_URI));
+      String targetUriPath = Uri.parse(targetUri).getPath();
+
+      if (TextUtils.isEmpty(targetUriPath) || !MapManager.nativeIsUrlSupported(targetUriPath))
+        return false;
+
       int status = cursor.getInt(cursor.getColumnIndex(DownloadManager.COLUMN_STATUS));
       String downloadedFileUri = cursor.getString(cursor.getColumnIndex(DownloadManager.COLUMN_LOCAL_URI));
 
       boolean result = processColumns(context, status, targetUri, downloadedFileUri);
       UiThread.run(new MapDownloadCompletedTask(context, result, id));
       LOGGER.d(TAG, "Processing for map downloading for request id " + id + " is finished.");
+
+      return true;
     }
     catch (Exception e)
     {
       LOGGER.e(TAG, "Failed to process map download for request id " + id + ". Exception ", e);
+
+      return false;
     }
   }
 
@@ -108,9 +111,6 @@ public class MapDownloadCompletedProcessor
     public void run()
     {
       MwmApplication application = MwmApplication.from(mAppContext);
-      if (!application.arePlatformAndCoreInitialized())
-        return;
-
       MapDownloadManager manager = MapDownloadManager.from(application);
       manager.onDownloadFinished(mStatus, mId);
     }
diff --git a/android/src/com/mapswithme/maps/downloader/MapDownloadManager.java b/android/src/com/mapswithme/maps/downloader/MapDownloadManager.java
index 74980de5ec..0539f3b408 100644
--- a/android/src/com/mapswithme/maps/downloader/MapDownloadManager.java
+++ b/android/src/com/mapswithme/maps/downloader/MapDownloadManager.java
@@ -11,6 +11,8 @@ import androidx.annotation.NonNull;
 import androidx.annotation.Nullable;
 import com.mapswithme.maps.MwmApplication;
 import com.mapswithme.util.Utils;
+import com.mapswithme.util.log.Logger;
+import com.mapswithme.util.log.LoggerFactory;
 
 import java.io.UnsupportedEncodingException;
 import java.net.URLDecoder;
@@ -19,9 +21,12 @@ import java.util.Map;
 
 public class MapDownloadManager
 {
+  private static final Logger LOGGER = LoggerFactory.INSTANCE.getLogger(LoggerFactory.Type.DOWNLOADER);
+  private static final String TAG = MapDownloadManager.class.getSimpleName();
+
   @NonNull
   private DownloadManager mDownloadManager;
-  @NonNull
+  @Nullable
   private Map<String, Long> mRestoredRequests;
   @NonNull
   private MapDownloadProgressTracker mProgressTracker;
@@ -35,15 +40,16 @@ public class MapDownloadManager
       throw new NullPointerException("Download manager is null, failed to create MapDownloadManager");
 
     mDownloadManager = downloadManager;
-    mRestoredRequests = loadEnqueued();
     mProgressTracker = new MapDownloadProgressTracker(context);
   }
 
   @NonNull
   private Map<String, Long> loadEnqueued()
   {
+    Map<String, Long> result = new HashMap<>();
     DownloadManager.Query query = new DownloadManager.Query();
-    query.setFilterByStatus(DownloadManager.STATUS_PENDING | DownloadManager.STATUS_RUNNING);
+    query.setFilterByStatus(DownloadManager.STATUS_PENDING | DownloadManager.STATUS_RUNNING |
+                            DownloadManager.STATUS_PAUSED | DownloadManager.STATUS_SUCCESSFUL);
     Cursor cursor = null;
     try
     {
@@ -51,11 +57,10 @@ public class MapDownloadManager
 
       cursor.moveToFirst();
       int count = cursor.getCount();
-      Map<String, Long> result = new HashMap<>();
 
       for (int i = 0; i < count; ++i)
       {
-        long id = cursor.getInt(cursor.getColumnIndex(DownloadManager.COLUMN_ID));
+        long id = cursor.getLong(cursor.getColumnIndex(DownloadManager.COLUMN_ID));
         String url = cursor.getString(cursor.getColumnIndex(DownloadManager.COLUMN_URI));
         String urlPath = getUrlPath(url);
 
@@ -64,13 +69,17 @@ public class MapDownloadManager
 
         cursor.moveToNext();
       }
-
-      return result;
+    }
+    catch (Exception e)
+    {
+      LOGGER.e(TAG, "Failed to load enqueued requests. Exception ", e);
     }
     finally
     {
       Utils.closeSafely(cursor);
     }
+
+    return result;
   }
 
   @Nullable
@@ -101,6 +110,9 @@ public class MapDownloadManager
     if (uriPath == null)
       throw new AssertionError("The path must be not null");
 
+    if (mRestoredRequests == null)
+      mRestoredRequests = loadEnqueued();
+
     Long id = mRestoredRequests.get(uriPath);
     long requestId = 0;
 
diff --git a/android/src/com/mapswithme/maps/downloader/MapDownloadProgressTracker.java b/android/src/com/mapswithme/maps/downloader/MapDownloadProgressTracker.java
index 6e719b26da..e78e24638c 100644
--- a/android/src/com/mapswithme/maps/downloader/MapDownloadProgressTracker.java
+++ b/android/src/com/mapswithme/maps/downloader/MapDownloadProgressTracker.java
@@ -21,9 +21,9 @@ public class MapDownloadProgressTracker
   private static final long PROGRESS_TRACKING_INTERVAL_MILLISECONDS = 1000;
 
   @NonNull
-  DownloadManager mDownloadManager;
+  private final DownloadManager mDownloadManager;
   @NonNull
-  private Set<Long> mTrackingIds = new HashSet<>();
+  private final Set<Long> mTrackingIds = new HashSet<>();
   private boolean mTrackingEnabled = false;
   private final Runnable mTrackingMethod = this::trackProgress;
 
diff --git a/android/src/com/mapswithme/maps/downloader/MapManager.java b/android/src/com/mapswithme/maps/downloader/MapManager.java
index 23a748bb32..953c99f3fd 100644
--- a/android/src/com/mapswithme/maps/downloader/MapManager.java
+++ b/android/src/com/mapswithme/maps/downloader/MapManager.java
@@ -436,7 +436,8 @@ public final class MapManager
   public static native @Nullable String nativeGetSelectedCountry();
 
   public static native boolean nativeIsUrlSupported(@NonNull String url);
-  public static native @NonNull String nativeGetFilePathByUrl(@NonNull String url);
+  @NonNull
+  public static native String nativeGetFilePathByUrl(@NonNull String url);
 
   public static native void nativeOnDownloadFinished(boolean status, long id);
   public static native void nativeOnDownloadProgress(long id, long bytesDownloaded, long bytesTotal);
diff --git a/storage/CMakeLists.txt b/storage/CMakeLists.txt
index bb367c1882..9450f32a4d 100644
--- a/storage/CMakeLists.txt
+++ b/storage/CMakeLists.txt
@@ -51,7 +51,7 @@ set(
   storage_helpers.hpp
 )
 
-if (${PLATFORM_ANDROID})
+if(${PLATFORM_ANDROID})
   append(
     SRC
     background_downloading/downloader_queue.hpp