Address comments

12 files changed, 54 insertions, 62 deletions

diff --git a/src/components/application_manager/include/application_manager/application_manager_impl.h b/src/components/application_manager/include/application_manager/application_manager_impl.h
index cda02e62fb..ae00108591 100644
--- a/src/components/application_manager/include/application_manager/application_manager_impl.h
+++ b/src/components/application_manager/include/application_manager/application_manager_impl.h
@@ -119,11 +119,10 @@ typedef std::map<std::string, hmi_apis::Common_TransportType::eType>
 
 struct AppIconInfo {
   std::string endpoint;
-  bool icon_exists;
   bool pending_request;
   AppIconInfo();
-  AppIconInfo(std::string url, bool exists, bool pending)
-      : endpoint(url), icon_exists(exists), pending_request(pending) {}
+  AppIconInfo(std::string ws_endpoint, bool pending)
+      : endpoint(ws_endpoint), pending_request(pending) {}
 };
 
 CREATE_LOGGERPTR_GLOBAL(logger_, "ApplicationManager")
@@ -390,7 +389,7 @@ class ApplicationManagerImpl
 
   std::string PolicyIDByIconUrl(const std::string url) OVERRIDE;
 
-  void SetIconExists(const std::string policy_id) OVERRIDE;
+  void SetIconFileFromSystemRequest(const std::string policy_id) OVERRIDE;
 
   /**
    * @brief Notifies the applicaiton manager that a cloud connection status has
@@ -523,9 +522,6 @@ class ApplicationManagerImpl
   // typedef for Applications list
   typedef std::set<ApplicationSharedPtr, ApplicationsAppIdSorter> ApplictionSet;
 
-  // typedef std::set<ApplicationSharedPtr, ApplicationsPolicyAppIdSorter>
-  //    AppsWaitRegistrationSet;
-
   // typedef for Applications list iterator
   typedef ApplictionSet::iterator ApplictionSetIt;
 
diff --git a/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/mobile/system_request.cc b/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/mobile/system_request.cc
index 8cec7b548b..da39aefd6a 100644
--- a/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/mobile/system_request.cc
+++ b/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/mobile/system_request.cc
@@ -540,6 +540,10 @@ void SystemRequest::Run() {
       // Use the URL file name to identify the policy id.
       // Save the icon file with the policy id as the name.
       file_name = application_manager_.PolicyIDByIconUrl(file_name);
+      if (file_name.empty()) {
+        const std::string err_msg = "Invalid file name";
+        SendResponse(false, mobile_apis::Result::INVALID_DATA, err_msg.c_str());
+      }
       LOG4CXX_DEBUG(logger_, "Got ICON_URL Request. File name: " << file_name);
     } else {
       binary_data_folder =
@@ -592,7 +596,7 @@ void SystemRequest::Run() {
   LOG4CXX_DEBUG(logger_, "Binary data ok.");
 
   if (mobile_apis::RequestType::ICON_URL == request_type) {
-    application_manager_.SetIconExists(file_name);
+    application_manager_.SetIconFileFromSystemRequest(file_name);
     SendResponse(true, mobile_apis::Result::SUCCESS);
     return;
   }
diff --git a/src/components/application_manager/src/application_manager_impl.cc b/src/components/application_manager/src/application_manager_impl.cc
index dd35b9ff92..bcd75ec349 100644
--- a/src/components/application_manager/src/application_manager_impl.cc
+++ b/src/components/application_manager/src/application_manager_impl.cc
@@ -73,6 +73,7 @@
 #include "policy/usage_statistics/counter.h"
 #include "utils/custom_string.h"
 #include <time.h>
+#include <boost/filesystem.hpp>
 
 namespace {
 int get_rand_from_range(uint32_t from = 0, int to = RAND_MAX) {
@@ -823,27 +824,25 @@ void ApplicationManagerImpl::OnHMIStartedCooperation() {
 }
 
 std::string ApplicationManagerImpl::PolicyIDByIconUrl(const std::string url) {
-  app_icon_map_lock_ptr_.Acquire();
+  sync_primitives::AutoLock lock(app_icon_map_lock_ptr_);
   for (auto& x : app_icon_map_) {
     auto policy_id = x.first;
     std::string icon_url = GetPolicyHandler().GetIconUrl(policy_id);
     if (icon_url == url) {
       LOG4CXX_DEBUG(logger_, "Matched icon url: " << url);
       x.second.pending_request = false;
-      app_icon_map_lock_ptr_.Release();
       return policy_id;
     }
   }
-  app_icon_map_lock_ptr_.Release();
   return std::string("");
 }
 
-void ApplicationManagerImpl::SetIconExists(const std::string policy_id) {
+void ApplicationManagerImpl::SetIconFileFromSystemRequest(
+    const std::string policy_id) {
   app_icon_map_lock_ptr_.Acquire();
   auto app_icon_it = app_icon_map_.find(policy_id);
   if (app_icon_it != app_icon_map_.end()) {
-    LOG4CXX_DEBUG(logger_, "Updaing icon exists status");
-    app_icon_it->second.icon_exists = true;
+    app_icon_map_.erase(app_icon_it);
   }
   app_icon_map_lock_ptr_.Release();
 
@@ -860,7 +859,17 @@ void ApplicationManagerImpl::SetIconExists(const std::string policy_id) {
     file.is_persistent = true;
     file.is_download_complete = true;
     file.file_name = full_icon_path;
-    file.file_type = mobile_apis::FileType::GRAPHIC_PNG;
+
+    std::string icon_url = GetPolicyHandler().GetIconUrl(policy_id);
+    std::string extension = boost::filesystem::extension(icon_url);
+    if (extension == "bmp" || extension == "BMP") {
+      file.file_type = mobile_apis::FileType::GRAPHIC_BMP;
+    } else if (extension == "JPEG" || extension == "jpeg") {
+      file.file_type = mobile_apis::FileType::GRAPHIC_JPEG;
+    } else {
+      file.file_type = mobile_apis::FileType::GRAPHIC_PNG;
+    }
+
     app->AddFile(file);
     app->set_app_icon_path(full_icon_path);
   }
@@ -928,9 +937,9 @@ void ApplicationManagerImpl::RefreshCloudAppInformation() {
                                              auth_token,
                                              cloud_transport_type,
                                              hybrid_app_preference);
-
+    auto policy_id = *enabled_it;
     pending_device_map_.insert(
-        std::pair<std::string, std::string>(endpoint, *enabled_it));
+        std::pair<std::string, std::string>(endpoint, policy_id));
     // Determine which endpoints were disabled by erasing all enabled apps from
     // the old device list
     auto old_device_it = old_device_map.find(endpoint);
@@ -940,42 +949,32 @@ void ApplicationManagerImpl::RefreshCloudAppInformation() {
 
     // If the device was disconnected, this will reinitialize the device
     connection_handler().AddCloudAppDevice(
-        *enabled_it, endpoint, cloud_transport_type);
+        policy_id, endpoint, cloud_transport_type);
 
     // Look for app icon url data and add to app_icon_url_map
-
-    std::string url = GetPolicyHandler().GetIconUrl(*enabled_it);
+    std::string url = GetPolicyHandler().GetIconUrl(policy_id);
 
     if (url.empty()) {
       LOG4CXX_DEBUG(logger_, "No Icon Url for cloud app");
       continue;
     }
 
-    auto app_icon_it = app_icon_map_.find(*enabled_it);
+    auto app_icon_it = app_icon_map_.find(policy_id);
     if (app_icon_it != app_icon_map_.end()) {
       LOG4CXX_DEBUG(logger_, "Cloud App Already Exists in Icon Map");
       continue;
     }
 
-    bool icon_exists = false;
-
     const std::string app_icon_dir(settings_.app_icons_folder());
-    const std::string full_icon_path(app_icon_dir + "/" + *enabled_it);
-    if (file_system::FileExists(full_icon_path)) {
-      LOG4CXX_DEBUG(logger_, "Cloud app icon already exists");
-      icon_exists = true;
+    const std::string full_icon_path(app_icon_dir + "/" + policy_id);
+    if (!file_system::FileExists(full_icon_path)) {
+      int icon_map_size = app_icon_map_.size();
+      AppIconInfo icon_info(endpoint, false);
+      LOG4CXX_DEBUG(logger_,
+                    "Inserting cloud app into icon map: " << icon_map_size);
+      app_icon_map_.insert(
+          std::pair<std::string, AppIconInfo>(policy_id, icon_info));
     }
-
-    // icon_map_size is used as the idex for identifying the request_sub_type to
-    // the
-    // cloud app that the GetIconUrl request is later made for.
-    int icon_map_size = app_icon_map_.size();
-
-    AppIconInfo icon_info(endpoint, icon_exists, false);
-    LOG4CXX_DEBUG(logger_,
-                  "Inserting cloud app into icon map: " << icon_map_size);
-    app_icon_map_.insert(
-        std::pair<std::string, AppIconInfo>(*enabled_it, icon_info));
   }
   app_icon_map_lock_ptr_.Release();
   pending_device_map_lock_ptr_->Release();
@@ -3763,7 +3762,7 @@ void ApplicationManagerImpl::OnPTUFinished(const bool ptu_result) {
   auto app = application(app_id);
   if (app) {
     SendGetIconUrlNotifications(app->app_id(), app);
-  } 
+  }
 
   auto on_app_policy_updated = [](plugin_manager::RPCPlugin& plugin) {
     plugin.OnPolicyEvent(plugin_manager::kApplicationPolicyUpdated);
@@ -3814,7 +3813,7 @@ void ApplicationManagerImpl::SendGetIconUrlNotifications(
   GetPolicyHandler().GetEnabledCloudApps(enabled_apps);
   std::vector<std::string>::iterator enabled_it = enabled_apps.begin();
   std::vector<std::string>::iterator enabled_end = enabled_apps.end();
-  app_icon_map_lock_ptr_.Acquire();
+  sync_primitives::AutoLock lock(app_icon_map_lock_ptr_);
   for (; enabled_it != enabled_end; ++enabled_it) {
     auto app_icon_it = app_icon_map_.find(*enabled_it);
     if (app_icon_it == app_icon_map_.end()) {
@@ -3823,7 +3822,6 @@ void ApplicationManagerImpl::SendGetIconUrlNotifications(
     }
 
     std::string endpoint = app_icon_it->second.endpoint;
-    bool icon_exists = app_icon_it->second.icon_exists;
     bool pending_request = app_icon_it->second.pending_request;
 
     if (pending_request) {
@@ -3831,11 +3829,6 @@ void ApplicationManagerImpl::SendGetIconUrlNotifications(
       continue;
     }
 
-    if (icon_exists) {
-      LOG4CXX_DEBUG(logger_, "Cloud app has already registered");
-      continue;
-    }
-
     std::string url = GetPolicyHandler().GetIconUrl(*enabled_it);
 
     if (url.empty()) {
@@ -3859,12 +3852,10 @@ void ApplicationManagerImpl::SendGetIconUrlNotifications(
         mobile_apis::RequestType::ICON_URL;
     (*message)[strings::msg_params][strings::url] = url;
 
-    // todo update to false when this request comes back
     app_icon_it->second.pending_request = true;
 
     rpc_service_->ManageMobileCommand(message, commands::Command::SOURCE_SDL);
   }
-  app_icon_map_lock_ptr_.Release();
 }
 
 protocol_handler::MajorProtocolVersion
diff --git a/src/components/application_manager/src/message_helper/message_helper.cc b/src/components/application_manager/src/message_helper/message_helper.cc
index 4de612107d..cc162b03ea 100644
--- a/src/components/application_manager/src/message_helper/message_helper.cc
+++ b/src/components/application_manager/src/message_helper/message_helper.cc
@@ -1548,7 +1548,6 @@ bool MessageHelper::CreateHMIApplicationStruct(
 
   const std::string icon_path = app->app_icon_path();
 
-  LOG4CXX_DEBUG(logger_, "Get ICON PATH" << icon_path);
   if (file_system::FileExists(app->app_icon_path())) {
     message[strings::icon] = icon_path;
   }
diff --git a/src/components/include/application_manager/application_manager.h b/src/components/include/application_manager/application_manager.h
index 94b1dae1f6..cfd6cbe61c 100644
--- a/src/components/include/application_manager/application_manager.h
+++ b/src/components/include/application_manager/application_manager.h
@@ -441,7 +441,7 @@ class ApplicationManager {
 
   virtual std::string PolicyIDByIconUrl(const std::string url) = 0;
 
-  virtual void SetIconExists(const std::string policy_id) = 0;
+  virtual void SetIconFileFromSystemRequest(const std::string policy_id) = 0;
 
   /**
    * @brief Retrieve the current connection status of a cloud app
diff --git a/src/components/include/policy/policy_external/policy/policy_manager.h b/src/components/include/policy/policy_external/policy/policy_manager.h
index 29c3ac762f..f342c2a07a 100644
--- a/src/components/include/policy/policy_external/policy/policy_manager.h
+++ b/src/components/include/policy/policy_external/policy/policy_manager.h
@@ -109,16 +109,16 @@ class PolicyManager : public usage_statistics::StatisticsManager {
 
   /**
    * @brief GetLockScreenIcon allows to obtain lock screen icon url;
-   * @return url which point to the resourse where lock screen icon could be
+   * @return url which point to the resource where lock screen icon could be
    *obtained.
    */
   virtual std::string GetLockScreenIconUrl() const = 0;
 
   /**
-   * @brief Get Icon Url used for showing a cloud apps icon before the intial
+   * @brief Get Icon Url used for showing a cloud apps icon before the initial
    *registration
    *
-   * @return url which point to the resourse where icon could be
+   * @return url which point to the resource where icon could be
    *obtained.
    */
   virtual std::string GetIconUrl(const std::string& policy_app_id) const = 0;
diff --git a/src/components/include/policy/policy_regular/policy/policy_manager.h b/src/components/include/policy/policy_regular/policy/policy_manager.h
index adb6d99d1d..6876611b3f 100644
--- a/src/components/include/policy/policy_regular/policy/policy_manager.h
+++ b/src/components/include/policy/policy_regular/policy/policy_manager.h
@@ -87,16 +87,16 @@ class PolicyManager : public usage_statistics::StatisticsManager {
 
   /**
    * @brief GetLockScreenIcon allows to obtain lock screen icon url;
-   * @return url which point to the resourse where lock screen icon could be
+   * @return url which point to the resource where lock screen icon could be
    *obtained.
    */
   virtual std::string GetLockScreenIconUrl() const = 0;
 
   /**
-   * @brief Get Icon Url used for showing a cloud apps icon before the intial
+   * @brief Get Icon Url used for showing a cloud apps icon before the initial
    *registration
    *
-   * @return url which point to the resourse where icon could be
+   * @return url which point to the resource where icon could be
    *obtained.
    */
   virtual std::string GetIconUrl(const std::string& policy_app_id) const = 0;
diff --git a/src/components/include/test/application_manager/mock_application_manager.h b/src/components/include/test/application_manager/mock_application_manager.h
index f368fd96c2..7cae217371 100644
--- a/src/components/include/test/application_manager/mock_application_manager.h
+++ b/src/components/include/test/application_manager/mock_application_manager.h
@@ -182,7 +182,7 @@ class MockApplicationManager : public application_manager::ApplicationManager {
                      hmi_apis::Common_CloudConnectionStatus::eType(
                          application_manager::ApplicationConstSharedPtr app));
   MOCK_METHOD1(PolicyIDByIconUrl, std::string(const std::string url));
-  MOCK_METHOD1(SetIconExists, void(const std::string policy_id));
+  MOCK_METHOD1(SetIconFileFromSystemRequest, void(const std::string policy_id));
   MOCK_CONST_METHOD0(IsHMICooperating, bool());
   MOCK_METHOD2(IviInfoUpdated,
                void(mobile_apis::VehicleDataType::eType vehicle_info,
diff --git a/src/components/interfaces/MOBILE_API.xml b/src/components/interfaces/MOBILE_API.xml
index 3bc7888e30..66f5a19208 100644
--- a/src/components/interfaces/MOBILE_API.xml
+++ b/src/components/interfaces/MOBILE_API.xml
@@ -2419,7 +2419,7 @@
         <element name="MEDIA" />
         <element name="FOTA" />
         <element name="OEM_SPECIFIC" since="5.0" />
-        <element name ="ICON_URL" />
+        <element name="ICON_URL" since="5.1" />
     </enum>         
  
     <enum name="AppHMIType" since="2.0">
diff --git a/src/components/policy/policy_external/src/cache_manager.cc b/src/components/policy/policy_external/src/cache_manager.cc
index 09d23eef71..0220d38b07 100644
--- a/src/components/policy/policy_external/src/cache_manager.cc
+++ b/src/components/policy/policy_external/src/cache_manager.cc
@@ -1604,7 +1604,8 @@ std::string CacheManager::GetLockScreenIconUrl() const {
 }
 
 std::string CacheManager::GetIconUrl(const std::string& policy_app_id) const {
-  std::string url = "";
+  CACHE_MANAGER_CHECK(std::string());
+  std::string url;
   const policy_table::ApplicationPolicies& policies =
       pt_->policy_table.app_policies_section.apps;
   policy_table::ApplicationPolicies::const_iterator policy_iter =
diff --git a/src/components/policy/policy_external/src/sql_pt_representation.cc b/src/components/policy/policy_external/src/sql_pt_representation.cc
index f0b9985536..885a59ba36 100644
--- a/src/components/policy/policy_external/src/sql_pt_representation.cc
+++ b/src/components/policy/policy_external/src/sql_pt_representation.cc
@@ -1072,8 +1072,8 @@ bool SQLPTRepresentation::SaveSpecificAppPolicy(
       ? app_query.Bind(10, *app.second.cloud_transport_type)
       : app_query.Bind(10);
   app.second.icon_url.is_initialized()
-      ? app_query.Bind(10, *app.second.icon_url)
-      : app_query.Bind(10);
+      ? app_query.Bind(11, *app.second.icon_url)
+      : app_query.Bind(11);
 
   if (!app_query.Exec() || !app_query.Reset()) {
     LOG4CXX_WARN(logger_, "Incorrect insert into application.");
diff --git a/src/components/policy/policy_regular/src/cache_manager.cc b/src/components/policy/policy_regular/src/cache_manager.cc
index 1983f367d8..677ea66454 100644
--- a/src/components/policy/policy_regular/src/cache_manager.cc
+++ b/src/components/policy/policy_regular/src/cache_manager.cc
@@ -893,7 +893,8 @@ std::string CacheManager::GetLockScreenIconUrl() const {
 }
 
 std::string CacheManager::GetIconUrl(const std::string& policy_app_id) const {
-  std::string url = "";
+  CACHE_MANAGER_CHECK(std::string());
+  std::string url;
   const policy_table::ApplicationPolicies& policies =
       pt_->policy_table.app_policies_section.apps;
   policy_table::ApplicationPolicies::const_iterator policy_iter =