[Background Fetch] Persist icon passed to updateUI.

Bug: 865063
Change-Id: Ic1a85aaaf1874f6b1d4538463a7968a1d9a6be85
Reviewed-on: https://chromium-review.googlesource.com/1145303
Commit-Queue: Rayan Kanso <rayankans@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577988}

content/browser/background_fetch/background_fetch_context.cc

@@ -182,7 +182,8 @@ void BackgroundFetchContext::AddRegistrationObserver(
 
 void BackgroundFetchContext::UpdateUI(
     const BackgroundFetchRegistrationId& registration_id,
-    const std::string& title,
+    const base::Optional<std::string>& title,
+    const base::Optional<SkBitmap>& icon,
     blink::mojom::BackgroundFetchService::UpdateUICallback callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
@@ -193,7 +194,7 @@ void BackgroundFetchContext::UpdateUI(
     return;
   }
 
-  data_manager_->UpdateRegistrationUI(registration_id, title,
+  data_manager_->UpdateRegistrationUI(registration_id, title, icon,
                                       std::move(callback));
 }
 

content/browser/background_fetch/background_fetch_context.h

@@ -13,6 +13,7 @@
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
+#include "base/optional.h"
 #include "content/browser/background_fetch/background_fetch_data_manager_observer.h"
 #include "content/browser/background_fetch/background_fetch_delegate_proxy.h"
 #include "content/browser/background_fetch/background_fetch_event_dispatcher.h"
@@ -105,12 +106,15 @@ class CONTENT_EXPORT BackgroundFetchContext
       const std::string& unique_id,
       blink::mojom::BackgroundFetchRegistrationObserverPtr observer);
 
-  // Updates the title of the Background Fetch identified by |registration_id|.
-  // The |callback| will be invoked when the title has been updated, or an error
-  // occurred that prevents it from doing so.
+  // Updates the title or icon of the Background Fetch identified by
+  // |registration_id|. The |callback| will be invoked when the title has been
+  // updated, or an error occurred that prevents it from doing so.
+  // The icon is wrapped in an optional. If the optional has a value then the
+  // internal |icon| is guarnteed to be not null.
   void UpdateUI(
       const BackgroundFetchRegistrationId& registration_id,
-      const std::string& title,
+      const base::Optional<std::string>& title,
+      const base::Optional<SkBitmap>& icon,
       blink::mojom::BackgroundFetchService::UpdateUICallback callback);
 
   // BackgroundFetchDataManagerObserver implementation.

content/browser/background_fetch/background_fetch_data_manager.cc

@@ -116,12 +116,13 @@ void BackgroundFetchDataManager::GetRegistration(
 
 void BackgroundFetchDataManager::UpdateRegistrationUI(
     const BackgroundFetchRegistrationId& registration_id,
-    const std::string& title,
+    const base::Optional<std::string>& title,
+    const base::Optional<SkBitmap>& icon,
     blink::mojom::BackgroundFetchService::UpdateUICallback callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   AddDatabaseTask(std::make_unique<background_fetch::UpdateRegistrationUITask>(
-      this, registration_id, title, std::move(callback)));
+      this, registration_id, title, icon, std::move(callback)));
 }
 
 void BackgroundFetchDataManager::PopNextRequest(

content/browser/background_fetch/background_fetch_data_manager.h

@@ -18,6 +18,7 @@
 #include "base/macros.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/observer_list.h"
+#include "base/optional.h"
 #include "content/browser/background_fetch/background_fetch.pb.h"
 #include "content/browser/background_fetch/background_fetch_registration_id.h"
 #include "content/browser/background_fetch/background_fetch_scheduler.h"
@@ -111,7 +112,8 @@ class CONTENT_EXPORT BackgroundFetchDataManager
   // Updates the UI values for a Background Fetch registration.
   void UpdateRegistrationUI(
       const BackgroundFetchRegistrationId& registration_id,
-      const std::string& title,
+      const base::Optional<std::string>& title,
+      const base::Optional<SkBitmap>& icon,
       blink::mojom::BackgroundFetchService::UpdateUICallback callback);
 
   // Reads the settled fetches for the given |registration_id| based on

content/browser/background_fetch/background_fetch_data_manager_unittest.cc

@@ -23,6 +23,7 @@
 #include "content/browser/background_fetch/background_fetch_test_base.h"
 #include "content/browser/background_fetch/background_fetch_test_data_manager.h"
 #include "content/browser/background_fetch/storage/database_helpers.h"
+#include "content/browser/background_fetch/storage/image_helpers.h"
 #include "content/browser/cache_storage/cache_storage_manager.h"
 #include "content/browser/service_worker/service_worker_context_wrapper.h"
 #include "content/public/browser/background_fetch_response.h"
@@ -148,6 +149,23 @@ std::vector<ServiceWorkerFetchRequest> CreateValidRequests(
   return requests;
 }
 
+SkBitmap CreateTestIcon(int size = 42, SkColor color = SK_ColorGREEN) {
+  SkBitmap icon;
+  icon.allocN32Pixels(size, size);
+  icon.eraseColor(SK_ColorGREEN);
+  return icon;
+}
+
+void ExpectIconProperties(const SkBitmap& icon, int size, SkColor color) {
+  EXPECT_FALSE(icon.isNull());
+  EXPECT_EQ(icon.width(), size);
+  EXPECT_EQ(icon.height(), size);
+  for (int i = 0; i < icon.width(); i++) {
+    for (int j = 0; j < icon.height(); j++)
+      EXPECT_EQ(icon.getColor(i, j), color);
+  }
+}
+
 }  // namespace
 
 class BackgroundFetchDataManagerTest
@@ -245,13 +263,14 @@ class BackgroundFetchDataManagerTest
 
   void UpdateRegistrationUI(
       const BackgroundFetchRegistrationId& registration_id,
-      const std::string& updated_title,
+      const base::Optional<std::string>& updated_title,
+      const base::Optional<SkBitmap>& updated_icon,
       blink::mojom::BackgroundFetchError* out_error) {
     DCHECK(out_error);
 
     base::RunLoop run_loop;
     background_fetch_data_manager_->UpdateRegistrationUI(
-        registration_id, updated_title,
+        registration_id, updated_title, updated_icon,
         base::BindOnce(&BackgroundFetchDataManagerTest::DidUpdateRegistrationUI,
                        base::Unretained(this), run_loop.QuitClosure(),
                        out_error));
@@ -406,7 +425,8 @@ class BackgroundFetchDataManagerTest
     return result;
   }
 
-  proto::BackgroundFetchUIOptions GetUIOptions(
+  // Returns the title and the icon.
+  std::pair<std::string, SkBitmap> GetUIOptions(
       int64_t service_worker_registration_id) {
     auto results = GetRegistrationUserDataByKeyPrefix(
         service_worker_registration_id, background_fetch::kUIOptionsKeyPrefix);
@@ -415,11 +435,33 @@ class BackgroundFetchDataManagerTest
 
     proto::BackgroundFetchUIOptions ui_options;
     if (results.empty())
-      return ui_options;
+      return {"", SkBitmap()};
 
     bool did_parse = ui_options.ParseFromString(results[0]);
     DCHECK(did_parse);
-    return ui_options;
+
+    std::pair<std::string, SkBitmap> result{ui_options.title(), SkBitmap()};
+
+    if (ui_options.icon().empty())
+      return result;
+
+    // Deserialize icon.
+    {
+      base::RunLoop run_loop;
+      background_fetch::DeserializeIcon(
+          std::unique_ptr<std::string>(ui_options.release_icon()),
+          base::BindOnce(
+              [](base::OnceClosure quit_closure, SkBitmap* out_icon,
+                 SkBitmap icon) {
+                DCHECK(out_icon);
+                *out_icon = std::move(icon);
+                std::move(quit_closure).Run();
+              },
+              run_loop.QuitClosure(), &result.second));
+      run_loop.Run();
+    }
+
+    return result;
   }
 
   // Gets information about the number of background fetch requests by state.
@@ -780,9 +822,8 @@ TEST_F(BackgroundFetchDataManagerTest, LargeIconNotPersisted) {
   BackgroundFetchOptions options;
   blink::mojom::BackgroundFetchError error;
 
-  SkBitmap icon;
-  icon.allocN32Pixels(512, 512);
-  icon.eraseColor(SK_ColorGREEN);
+  SkBitmap icon = CreateTestIcon(512 /* size */);
+  ASSERT_FALSE(background_fetch::ShouldPersistIcon(icon));
 
   // Create a single registration.
   {
@@ -798,7 +839,7 @@ TEST_F(BackgroundFetchDataManagerTest, LargeIconNotPersisted) {
   EXPECT_EQ(metadata->origin(), origin().Serialize());
   EXPECT_NE(metadata->creation_microseconds_since_unix_epoch(), 0);
   EXPECT_EQ(metadata->num_fetches(), static_cast<int>(requests.size()));
-  EXPECT_TRUE(GetUIOptions(sw_id).icon().empty());
+  EXPECT_TRUE(GetUIOptions(sw_id).second.isNull());
 }
 
 TEST_F(BackgroundFetchDataManagerTest, UpdateRegistrationUI) {
@@ -814,31 +855,88 @@ TEST_F(BackgroundFetchDataManagerTest, UpdateRegistrationUI) {
   blink::mojom::BackgroundFetchError error;
 
   // There should be no title before the registration.
-  EXPECT_TRUE(GetUIOptions(sw_id).title().empty());
+  EXPECT_TRUE(GetUIOptions(sw_id).first.empty());
 
   // Create a single registration.
   {
     EXPECT_CALL(*this, OnRegistrationCreated(registration_id, _, _, _, _));
 
-    CreateRegistration(registration_id, requests, options, SkBitmap(), &error);
+    CreateRegistration(registration_id, requests, options, CreateTestIcon(),
+                       &error);
     ASSERT_EQ(error, blink::mojom::BackgroundFetchError::NONE);
   }
 
-  // Verify that the title can be retrieved.
-  ASSERT_EQ(GetUIOptions(sw_id).title(), kInitialTitle);
+  // Verify that the UI Options can be retrieved.
+  {
+    auto ui_options = GetUIOptions(sw_id);
+    EXPECT_EQ(ui_options.first, kInitialTitle);
+    EXPECT_NO_FATAL_FAILURE(
+        ExpectIconProperties(ui_options.second, 42, SK_ColorGREEN));
+  }
 
-  // Update the title.
+  // Update only the title.
   {
     EXPECT_CALL(*this, OnUpdatedUI(registration_id, kUpdatedTitle));
 
-    UpdateRegistrationUI(registration_id, kUpdatedTitle, &error);
+    UpdateRegistrationUI(registration_id, kUpdatedTitle, base::nullopt, &error);
+    ASSERT_EQ(error, blink::mojom::BackgroundFetchError::NONE);
+
+    auto ui_options = GetUIOptions(sw_id);
+    // Expect new title.
+    EXPECT_EQ(ui_options.first, kUpdatedTitle);
+    // Expect same icon as before.
+    EXPECT_NO_FATAL_FAILURE(
+        ExpectIconProperties(ui_options.second, 42, SK_ColorGREEN));
+  }
+
+  // Update only the icon.
+  {
+    // TODO(crbug.com/865063): Icon updates are not supported yet.
+    EXPECT_CALL(*this, OnUpdatedUI(registration_id, kUpdatedTitle)).Times(0);
+
+    UpdateRegistrationUI(registration_id, base::nullopt,
+                         CreateTestIcon(24 /* size */), &error);
     ASSERT_EQ(error, blink::mojom::BackgroundFetchError::NONE);
+
+    auto ui_options = GetUIOptions(sw_id);
+    // Expect the same title as before.
+    EXPECT_EQ(ui_options.first, kUpdatedTitle);
+    // Expect the new icon with the different size.
+    EXPECT_NO_FATAL_FAILURE(
+        ExpectIconProperties(ui_options.second, 24, SK_ColorGREEN));
   }
 
-  RestartDataManagerFromPersistentStorage();
+  // Update both the title and icon.
+  {
+    EXPECT_CALL(*this, OnUpdatedUI(registration_id, kInitialTitle));
+
+    UpdateRegistrationUI(registration_id, kInitialTitle,
+                         CreateTestIcon(66 /* size */), &error);
+    ASSERT_EQ(error, blink::mojom::BackgroundFetchError::NONE);
+
+    auto ui_options = GetUIOptions(sw_id);
+    // Expect the initial title again.
+    EXPECT_EQ(ui_options.first, kInitialTitle);
+    // Expect the new icon with the different size.
+    EXPECT_NO_FATAL_FAILURE(
+        ExpectIconProperties(ui_options.second, 66, SK_ColorGREEN));
+  }
+
+  // New title and an icon that's too large.
+  {
+    EXPECT_CALL(*this, OnUpdatedUI(registration_id, kUpdatedTitle));
+
+    UpdateRegistrationUI(registration_id, kUpdatedTitle,
+                         CreateTestIcon(512 /* size */), &error);
+    ASSERT_EQ(error, blink::mojom::BackgroundFetchError::NONE);
 
-  // After a restart, GetMetadata should find the new title.
-  ASSERT_EQ(GetUIOptions(sw_id).title(), kUpdatedTitle);
+    auto ui_options = GetUIOptions(sw_id);
+    // Expect the new title.
+    EXPECT_EQ(ui_options.first, kUpdatedTitle);
+    // Expect same icon as before.
+    EXPECT_NO_FATAL_FAILURE(
+        ExpectIconProperties(ui_options.second, 66, SK_ColorGREEN));
+  }
 }
 
 TEST_F(BackgroundFetchDataManagerTest, CreateAndDeleteRegistration) {
@@ -1425,14 +1523,12 @@ TEST_F(BackgroundFetchDataManagerTest, GetInitializationData) {
   // Register a Background Fetch.
   BackgroundFetchRegistrationId registration_id(
       sw_id, origin(), kExampleDeveloperId, kExampleUniqueId);
-  SkBitmap icon;
-  icon.allocN32Pixels(42, 42);
-  icon.eraseColor(SK_ColorGREEN);
 
   {
     EXPECT_CALL(*this, OnRegistrationCreated(registration_id, _, _, _, _));
 
-    CreateRegistration(registration_id, requests, options, icon, &error);
+    CreateRegistration(registration_id, requests, options, CreateTestIcon(),
+                       &error);
     ASSERT_EQ(error, blink::mojom::BackgroundFetchError::NONE);
   }
 
@@ -1454,12 +1550,7 @@ TEST_F(BackgroundFetchDataManagerTest, GetInitializationData) {
 
     // Check icon.
     ASSERT_FALSE(init.icon.drawsNothing());
-    EXPECT_EQ(icon.width(), init.icon.width());
-    EXPECT_EQ(icon.height(), init.icon.height());
-    for (int i = 0; i < icon.width(); i++) {
-      for (int j = 0; j < icon.height(); j++)
-        EXPECT_EQ(init.icon.getColor(i, j), SK_ColorGREEN);
-    }
+    EXPECT_NO_FATAL_FAILURE(ExpectIconProperties(init.icon, 42, SK_ColorGREEN));
   }
 
   // Mark one request as complete and start another.

content/browser/background_fetch/background_fetch_service_impl.cc

@@ -111,9 +111,7 @@ void BackgroundFetchServiceImpl::UpdateUI(
     UpdateUICallback callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-  // TODO(crbug.com/865063): Remove the check for |title| when updating both the
-  // title and icons is supported.
-  if (!ValidateUniqueId(unique_id) || !title || !ValidateTitle(*title)) {
+  if (!ValidateUniqueId(unique_id) || (title && !ValidateTitle(*title))) {
     std::move(callback).Run(
         blink::mojom::BackgroundFetchError::INVALID_ARGUMENT);
     return;
@@ -121,7 +119,12 @@ void BackgroundFetchServiceImpl::UpdateUI(
 
   BackgroundFetchRegistrationId registration_id(
       service_worker_registration_id, origin_, developer_id, unique_id);
-  background_fetch_context_->UpdateUI(registration_id, *title,
+
+  // Wrap the icon in an optional for clarity.
+  auto optional_icon =
+      icon.isNull() ? base::nullopt : base::Optional<SkBitmap>(icon);
+
+  background_fetch_context_->UpdateUI(registration_id, title, optional_icon,
                                       std::move(callback));
 }
 

content/browser/background_fetch/storage/update_registration_ui_task.cc

@@ -7,6 +7,7 @@
 #include "content/browser/background_fetch/background_fetch_data_manager.h"
 #include "content/browser/background_fetch/background_fetch_data_manager_observer.h"
 #include "content/browser/background_fetch/storage/database_helpers.h"
+#include "content/browser/background_fetch/storage/image_helpers.h"
 #include "content/browser/service_worker/service_worker_context_wrapper.h"
 #include "third_party/blink/public/platform/modules/background_fetch/background_fetch.mojom.h"
 
@@ -17,27 +18,77 @@ namespace background_fetch {
 UpdateRegistrationUITask::UpdateRegistrationUITask(
     DatabaseTaskHost* host,
     const BackgroundFetchRegistrationId& registration_id,
-    const std::string& updated_title,
+    const base::Optional<std::string>& title,
+    const base::Optional<SkBitmap>& icon,
     UpdateRegistrationUICallback callback)
     : DatabaseTask(host),
       registration_id_(registration_id),
-      updated_title_(updated_title),
+      title_(title),
+      icon_(icon),
       callback_(std::move(callback)),
-      weak_factory_(this) {}
+      weak_factory_(this) {
+  DCHECK(title_ || icon_);
+}
 
 UpdateRegistrationUITask::~UpdateRegistrationUITask() = default;
 
 void UpdateRegistrationUITask::Start() {
-  // TODO(crbug.com/865063): Persist new icon if applicable and don't
-  // overwrite unupdated values.
-  proto::BackgroundFetchUIOptions ui_options;
-  ui_options.set_title(updated_title_);
+  if (title_ && icon_ && ShouldPersistIcon(*icon_)) {
+    // Directly overwrite whatever's stored in the SWDB.
+    SerializeIcon(*icon_,
+                  base::BindOnce(&UpdateRegistrationUITask::DidSerializeIcon,
+                                 weak_factory_.GetWeakPtr()));
+    return;
+  }
+
+  service_worker_context()->GetRegistrationUserData(
+      registration_id_.service_worker_registration_id(),
+      {UIOptionsKey(registration_id_.unique_id())},
+      base::BindOnce(&UpdateRegistrationUITask::DidGetUIOptions,
+                     weak_factory_.GetWeakPtr()));
+}
+
+void UpdateRegistrationUITask::DidGetUIOptions(
+    const std::vector<std::string>& data,
+    blink::ServiceWorkerStatusCode status) {
+  switch (ToDatabaseStatus(status)) {
+    case DatabaseStatus::kFailed:
+      FinishWithError(blink::mojom::BackgroundFetchError::STORAGE_ERROR);
+      return;
+    case DatabaseStatus::kNotFound:
+    case DatabaseStatus::kOk:
+      break;
+  }
+
+  if (data.empty() || !ui_options_.ParseFromString(data[0])) {
+    FinishWithError(blink::mojom::BackgroundFetchError::STORAGE_ERROR);
+    return;
+  }
+
+  if (icon_ && ShouldPersistIcon(*icon_)) {
+    ui_options_.clear_icon();
+    SerializeIcon(*icon_,
+                  base::BindOnce(&UpdateRegistrationUITask::DidSerializeIcon,
+                                 weak_factory_.GetWeakPtr()));
+  } else {
+    StoreUIOptions();
+  }
+}
+
+void UpdateRegistrationUITask::DidSerializeIcon(std::string serialized_icon) {
+  ui_options_.set_icon(std::move(serialized_icon));
+  StoreUIOptions();
+}
+
+void UpdateRegistrationUITask::StoreUIOptions() {
+  if (title_)
+    ui_options_.set_title(*title_);
 
   service_worker_context()->StoreRegistrationUserData(
       registration_id_.service_worker_registration_id(),
       registration_id_.origin().GetURL(),
       {{UIOptionsKey(registration_id_.unique_id()),
-        ui_options.SerializeAsString()}},
+        ui_options_.SerializeAsString()}},
       base::BindOnce(&UpdateRegistrationUITask::DidUpdateUIOptions,
                      weak_factory_.GetWeakPtr()));
 }
@@ -53,14 +104,16 @@ void UpdateRegistrationUITask::DidUpdateUIOptions(
       return;
   }
 
-  for (auto& observer : data_manager()->observers())
-    observer.OnUpdatedUI(registration_id_, updated_title_);
-
   FinishWithError(blink::mojom::BackgroundFetchError::NONE);
 }
 
 void UpdateRegistrationUITask::FinishWithError(
     blink::mojom::BackgroundFetchError error) {
+  for (auto& observer : data_manager()->observers()) {
+    if (title_)
+      observer.OnUpdatedUI(registration_id_, *title_);
+  }
+
   std::move(callback_).Run(error);
   Finished();  // Destroys |this|.
 }

content/browser/background_fetch/storage/update_registration_ui_task.h

@@ -9,6 +9,7 @@
 #include <string>
 #include <vector>
 
+#include "base/optional.h"
 #include "content/browser/background_fetch/background_fetch.pb.h"
 #include "content/browser/background_fetch/storage/database_task.h"
 #include "third_party/blink/public/common/service_worker/service_worker_status_code.h"
@@ -25,7 +26,8 @@ class UpdateRegistrationUITask : public DatabaseTask {
 
   UpdateRegistrationUITask(DatabaseTaskHost* host,
                            const BackgroundFetchRegistrationId& registration_id,
-                           const std::string& updated_title,
+                           const base::Optional<std::string>& title,
+                           const base::Optional<SkBitmap>& icon,
                            UpdateRegistrationUICallback callback);
 
   ~UpdateRegistrationUITask() override;
@@ -33,12 +35,22 @@ class UpdateRegistrationUITask : public DatabaseTask {
   void Start() override;
 
  private:
+  void DidGetUIOptions(const std::vector<std::string>& data,
+                       blink::ServiceWorkerStatusCode status);
+
+  void DidSerializeIcon(std::string serialized_icon);
+
+  void StoreUIOptions();
+
   void DidUpdateUIOptions(blink::ServiceWorkerStatusCode status);
 
   void FinishWithError(blink::mojom::BackgroundFetchError error) override;
 
   BackgroundFetchRegistrationId registration_id_;
-  std::string updated_title_;
+  base::Optional<std::string> title_;
+  base::Optional<SkBitmap> icon_;
+
+  proto::BackgroundFetchUIOptions ui_options_;
 
   UpdateRegistrationUICallback callback_;