Reland "Reland "[Background Fetch] Move all updatable UI options to their own proto.""

This is a reland of 9311d412

TBR=avi@chromium.org

Original change's description:
> Reland "[Background Fetch] Move all updatable UI options to their own proto."
>
> This is a reland of d3981964
>
> TBR=avi@chromium.org
>
> Original change's description:
> > [Background Fetch] Move all updatable UI options to their own proto.
> >
> > Bundle up the icon and the title since they will be accessed/modified
> > together in the updateUI call.
> >
> > It also makes sense to move the icon from the metadata proto since that
> > will speed up getting the registration info.
> >
> > TBR=avi@chromium.org
> >
> > Bug: 865063
> > Change-Id: I661062f19fddfa6b9c3bf11b5e146d5289a02593
> > Reviewed-on: https://chromium-review.googlesource.com/1146650
> > Commit-Queue: Rayan Kanso <rayankans@chromium.org>
> > Reviewed-by: Avi Drissman <avi@chromium.org>
> > Reviewed-by: Peter Beverloo <peter@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#577272}
>
> Bug: 865063
> Change-Id: I77567e8e2e0ea59ede514c77b40326e7ece48a25
> Reviewed-on: https://chromium-review.googlesource.com/1148261
> Commit-Queue: Rayan Kanso <rayankans@chromium.org>
> Reviewed-by: Peter Beverloo <peter@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#577500}

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

content/browser/BUILD.gn

@@ -446,6 +446,8 @@ jumbo_source_set("browser") {
     "background_fetch/storage/get_metadata_task.h",
     "background_fetch/storage/get_settled_fetches_task.cc",
     "background_fetch/storage/get_settled_fetches_task.h",
+    "background_fetch/storage/image_helpers.cc",
+    "background_fetch/storage/image_helpers.h",
     "background_fetch/storage/mark_registration_for_deletion_task.cc",
     "background_fetch/storage/mark_registration_for_deletion_task.h",
     "background_fetch/storage/mark_request_complete_task.cc",

content/browser/background_fetch/background_fetch.proto

@@ -30,6 +30,8 @@ message BackgroundFetchRegistration {
 //
 // Next Tag: 3
 message BackgroundFetchOptions {
+  // The initial title provided by the developer. This can be updated,
+  // and the most recent value is stored in BackgroundFetchUIOptions.
   optional string title = 1;
 
   // https://w3c.github.io/manifest/#dom-imageresource
@@ -66,7 +68,7 @@ message BackgroundFetchOptions {
 // in memory. This information should be everything needed to reconstruct
 // the state of an interrupted background fetch.
 //
-// Next Tag: 7
+// Next Tag: 6
 message BackgroundFetchMetadata {
   optional int64 creation_microseconds_since_unix_epoch = 1;
   optional string origin = 2;
@@ -76,10 +78,17 @@ message BackgroundFetchMetadata {
 
   // Number of fetches initiated by the developer.
   optional int32 num_fetches = 5;
+}
+
+// All the updateable options that show up in the UI (e.g. notification).
+//
+// Next Tag: 3
+message BackgroundFetchUIOptions {
+  optional string title = 1;
 
   // Raw bytes needed to deserialize into a PNG icon. Only used if the icon
   // has a max resolution of 256x256. This means this is at most ~256KB.
-  optional bytes icon = 6;
+  optional bytes icon = 2;
 }
 
 // A background fetch request that is still in a pending state.

content/browser/background_fetch/background_fetch_data_manager_unittest.cc

@@ -16,6 +16,7 @@
 #include "base/guid.h"
 #include "base/run_loop.h"
 #include "base/strings/string_number_conversions.h"
+#include "content/browser/background_fetch/background_fetch.pb.h"
 #include "content/browser/background_fetch/background_fetch_data_manager_observer.h"
 #include "content/browser/background_fetch/background_fetch_request_info.h"
 #include "content/browser/background_fetch/background_fetch_request_match_params.h"
@@ -407,6 +408,22 @@ class BackgroundFetchDataManagerTest
     return result;
   }
 
+  proto::BackgroundFetchUIOptions GetUIOptions(
+      int64_t service_worker_registration_id) {
+    auto results = GetRegistrationUserDataByKeyPrefix(
+        service_worker_registration_id, background_fetch::kUIOptionsKeyPrefix);
+    DCHECK_LT(results.size(), 2u)
+        << "Using GetUIOptions with multiple registrations is unimplemented";
+
+    proto::BackgroundFetchUIOptions ui_options;
+    if (results.empty())
+      return ui_options;
+
+    bool did_parse = ui_options.ParseFromString(results[0]);
+    DCHECK(did_parse);
+    return ui_options;
+  }
+
   // Gets information about the number of background fetch requests by state.
   ResponseStateStats GetRequestStats(int64_t service_worker_registration_id) {
     ResponseStateStats stats;
@@ -763,7 +780,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(metadata->icon().empty());
+  EXPECT_TRUE(GetUIOptions(sw_id).icon().empty());
 }
 
 TEST_F(BackgroundFetchDataManagerTest, UpdateRegistrationUI) {
@@ -779,19 +796,14 @@ TEST_F(BackgroundFetchDataManagerTest, UpdateRegistrationUI) {
   blink::mojom::BackgroundFetchError error;
 
   // There should be no title before the registration.
-  std::vector<std::string> title = GetRegistrationUserDataByKeyPrefix(
-      sw_id, background_fetch::kTitleKeyPrefix);
-  EXPECT_TRUE(title.empty());
+  EXPECT_TRUE(GetUIOptions(sw_id).title().empty());
 
   // Create a single registration.
   CreateRegistration(registration_id, requests, options, SkBitmap(), &error);
   EXPECT_EQ(error, blink::mojom::BackgroundFetchError::NONE);
 
   // Verify that the title can be retrieved.
-  title = GetRegistrationUserDataByKeyPrefix(sw_id,
-                                             background_fetch::kTitleKeyPrefix);
-  EXPECT_EQ(title.size(), 1u);
-  ASSERT_EQ(title.front(), kInitialTitle);
+  ASSERT_EQ(GetUIOptions(sw_id).title(), kInitialTitle);
 
   // Update the title.
   {
@@ -804,10 +816,7 @@ TEST_F(BackgroundFetchDataManagerTest, UpdateRegistrationUI) {
   RestartDataManagerFromPersistentStorage();
 
   // After a restart, GetMetadata should find the new title.
-  title = GetRegistrationUserDataByKeyPrefix(sw_id,
-                                             background_fetch::kTitleKeyPrefix);
-  EXPECT_EQ(title.size(), 1u);
-  ASSERT_EQ(title.front(), kUpdatedTitle);
+  ASSERT_EQ(GetUIOptions(sw_id).title(), kUpdatedTitle);
 }
 
 TEST_F(BackgroundFetchDataManagerTest, CreateAndDeleteRegistration) {
@@ -1357,8 +1366,6 @@ TEST_F(BackgroundFetchDataManagerTest, GetInitializationData) {
   icon.eraseColor(SK_ColorGREEN);
   CreateRegistration(registration_id, requests, options, icon, &error);
   ASSERT_EQ(error, blink::mojom::BackgroundFetchError::NONE);
-  UpdateRegistrationUI(registration_id, kUpdatedTitle, &error);
-  ASSERT_EQ(error, blink::mojom::BackgroundFetchError::NONE);
   {
     std::vector<BackgroundFetchInitializationData> data =
         GetInitializationData();
@@ -1370,7 +1377,7 @@ TEST_F(BackgroundFetchDataManagerTest, GetInitializationData) {
     EXPECT_EQ(init.registration.developer_id, kExampleDeveloperId);
     EXPECT_EQ(init.options.title, kInitialTitle);
     EXPECT_EQ(init.options.download_total, 42u);
-    EXPECT_EQ(init.ui_title, kUpdatedTitle);
+    EXPECT_EQ(init.ui_title, kInitialTitle);
     EXPECT_EQ(init.num_requests, requests.size());
     EXPECT_EQ(init.num_completed_requests, 0u);
     EXPECT_TRUE(init.active_fetch_guids.empty());

content/browser/background_fetch/storage/README.md

@@ -26,8 +26,8 @@ value: "<serialized content::proto::BackgroundFetchMetadata>"
 ```
 
 ```
-key: "bgfetch_title_<unique_id>"
-value: "<ui_title>"
+key: "bgfetch_ui_options_<unique_id>"
+value: "<serialized content::proto::BackgroundFetchUIOptions>"
 ```
 
 ```

content/browser/background_fetch/storage/create_metadata_task.cc

@@ -6,37 +6,17 @@
 
 #include <utility>
 
-#include "base/sequenced_task_runner.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
-#include "base/task_scheduler/post_task.h"
-#include "base/task_scheduler/task_traits.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/common/service_worker/service_worker_status_code.h"
-#include "ui/gfx/image/image.h"
 
 namespace content {
 
 namespace background_fetch {
 
-namespace {
-
-std::string ConvertAndSerializeIcon(const SkBitmap& icon) {
-  // Serialize the icon and copy the bytes over.
-  std::string serialized_icon;
-  auto icon_bytes = gfx::Image::CreateFrom1xBitmap(icon).As1xPNGBytes();
-  serialized_icon.assign(icon_bytes->front_as<char>(),
-                         icon_bytes->front_as<char>() + icon_bytes->size());
-  return serialized_icon;
-}
-
-}  // namespace
-
-// The max icon resolution, this is used as a threshold to decide
-// whether the icon should be persisted.
-constexpr int kMaxIconResolution = 256 * 256;
-
 CreateMetadataTask::CreateMetadataTask(
     DatabaseTaskHost* host,
     const BackgroundFetchRegistrationId& registration_id,
@@ -66,8 +46,7 @@ void CreateMetadataTask::DidGetUniqueId(const std::vector<std::string>& data,
                                         blink::ServiceWorkerStatusCode status) {
   switch (ToDatabaseStatus(status)) {
     case DatabaseStatus::kNotFound:
-      InitializeMetadataProto();
-      return;
+      break;
     case DatabaseStatus::kOk:
       // Can't create a registration since there is already an active
       // registration with the same |developer_id|. It must be deactivated
@@ -79,6 +58,17 @@ void CreateMetadataTask::DidGetUniqueId(const std::vector<std::string>& data,
       FinishWithError(blink::mojom::BackgroundFetchError::STORAGE_ERROR);
       return;
   }
+
+  InitializeMetadataProto();
+
+  if (ShouldPersistIcon(icon_)) {
+    // Serialize the icon, then store all the metadata.
+    SerializeIcon(icon_, base::BindOnce(&CreateMetadataTask::DidSerializeIcon,
+                                        weak_factory_.GetWeakPtr()));
+  } else {
+    // Directly store the metadata.
+    StoreMetadata();
+  }
 }
 
 void CreateMetadataTask::InitializeMetadataProto() {
@@ -126,35 +116,21 @@ void CreateMetadataTask::InitializeMetadataProto() {
   metadata_proto_->set_creation_microseconds_since_unix_epoch(
       (base::Time::Now() - base::Time::UnixEpoch()).InMicroseconds());
   metadata_proto_->set_num_fetches(requests_.size());
-
-  // Check if |icon_| should be persisted.
-  if (icon_.height() * icon_.width() > kMaxIconResolution) {
-    StoreMetadata();
-    return;
-  }
-
-  // Do the initialization on a seperate thread to avoid blocking on
-  // expensive operations (image conversions), then post back to IO thread
-  // and continue normally.
-  base::PostTaskWithTraitsAndReplyWithResult(
-      FROM_HERE,
-      {base::MayBlock(), base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN,
-       base::TaskPriority::BACKGROUND},
-      base::BindOnce(&ConvertAndSerializeIcon, icon_),
-      base::BindOnce(&CreateMetadataTask::StoreIcon,
-                     weak_factory_.GetWeakPtr()));
 }
 
-void CreateMetadataTask::StoreIcon(std::string serialized_icon) {
-  DCHECK(metadata_proto_);
-  metadata_proto_->set_icon(std::move(serialized_icon));
+void CreateMetadataTask::DidSerializeIcon(std::string serialized_icon) {
+  serialized_icon_ = std::move(serialized_icon);
   StoreMetadata();
 }
 
 void CreateMetadataTask::StoreMetadata() {
   DCHECK(metadata_proto_);
   std::vector<std::pair<std::string, std::string>> entries;
-  entries.reserve(requests_.size() * 2 + 1);
+  // - One BackgroundFetchPendingRequest per request
+  // - DeveloperId -> UniqueID
+  // - BackgroundFetchMetadata
+  // - BackgroundFetchUIOptions
+  entries.reserve(requests_.size() + 3);
 
   std::string serialized_metadata_proto;
 
@@ -163,12 +139,24 @@ void CreateMetadataTask::StoreMetadata() {
     return;
   }
 
+  std::string serialized_ui_options_proto;
+  proto::BackgroundFetchUIOptions ui_options;
+  ui_options.set_title(options_.title);
+  if (!serialized_icon_.empty())
+    ui_options.set_icon(std::move(serialized_icon_));
+
+  if (!ui_options.SerializeToString(&serialized_ui_options_proto)) {
+    FinishWithError(blink::mojom::BackgroundFetchError::STORAGE_ERROR);
+    return;
+  }
+
   entries.emplace_back(
       ActiveRegistrationUniqueIdKey(registration_id_.developer_id()),
       registration_id_.unique_id());
   entries.emplace_back(RegistrationKey(registration_id_.unique_id()),
                        std::move(serialized_metadata_proto));
-  entries.emplace_back(TitleKey(registration_id_.unique_id()), options_.title);
+  entries.emplace_back(UIOptionsKey(registration_id_.unique_id()),
+                       serialized_ui_options_proto);
 
   // Signed integers are used for request indexes to avoid unsigned gotchas.
   for (int i = 0; i < base::checked_cast<int>(requests_.size()); i++) {

content/browser/background_fetch/storage/create_metadata_task.h

@@ -41,7 +41,7 @@ class CreateMetadataTask : public DatabaseTask {
   void DidGetUniqueId(const std::vector<std::string>& data,
                       blink::ServiceWorkerStatusCode status);
 
-  void StoreIcon(std::string serialized_icon);
+  void DidSerializeIcon(std::string serialized_icon);
 
   void StoreMetadata();
 
@@ -60,6 +60,8 @@ class CreateMetadataTask : public DatabaseTask {
 
   std::unique_ptr<proto::BackgroundFetchMetadata> metadata_proto_;
 
+  std::string serialized_icon_;
+
   base::WeakPtrFactory<CreateMetadataTask> weak_factory_;  // Keep as last.
 
   DISALLOW_COPY_AND_ASSIGN(CreateMetadataTask);

content/browser/background_fetch/storage/database_helpers.cc

@@ -23,8 +23,8 @@ std::string RegistrationKey(const std::string& unique_id) {
   return kRegistrationKeyPrefix + unique_id;
 }
 
-std::string TitleKey(const std::string& unique_id) {
-  return kTitleKeyPrefix + unique_id;
+std::string UIOptionsKey(const std::string& unique_id) {
+  return kUIOptionsKeyPrefix + unique_id;
 }
 
 std::string PendingRequestKeyPrefix(const std::string& unique_id) {

content/browser/background_fetch/storage/database_helpers.h

@@ -24,16 +24,17 @@ const char kSeparator[] = "_";
 const char kActiveRegistrationUniqueIdKeyPrefix[] =
     "bgfetch_active_registration_unique_id_";
 const char kRegistrationKeyPrefix[] = "bgfetch_registration_";
-const char kTitleKeyPrefix[] = "bgfetch_title_";
+const char kUIOptionsKeyPrefix[] = "bgfetch_ui_options_";
 const char kPendingRequestKeyPrefix[] = "bgfetch_pending_request_";
 const char kActiveRequestKeyPrefix[] = "bgfetch_active_request_";
 const char kCompletedRequestKeyPrefix[] = "bgfetch_completed_request_";
 
+// Database Keys.
 std::string ActiveRegistrationUniqueIdKey(const std::string& developer_id);
 
 std::string RegistrationKey(const std::string& unique_id);
 
-std::string TitleKey(const std::string& unique_id);
+std::string UIOptionsKey(const std::string& unique_id);
 
 std::string PendingRequestKeyPrefix(const std::string& unique_id);
 
@@ -48,6 +49,7 @@ std::string CompletedRequestKeyPrefix(const std::string& unique_id);
 std::string CompletedRequestKey(const std::string& unique_id,
                                 int request_index);
 
+// Database status.
 enum class DatabaseStatus { kOk, kFailed, kNotFound };
 
 DatabaseStatus ToDatabaseStatus(blink::ServiceWorkerStatusCode status);

content/browser/background_fetch/storage/delete_registration_task.cc

@@ -107,7 +107,7 @@ void DeleteRegistrationTask::DidGetRegistration(
 #endif  // DCHECK_IS_ON()
 
   std::vector<std::string> deletion_key_prefixes{
-      RegistrationKey(unique_id_), TitleKey(unique_id_),
+      RegistrationKey(unique_id_), UIOptionsKey(unique_id_),
       PendingRequestKeyPrefix(unique_id_), ActiveRequestKeyPrefix(unique_id_),
       CompletedRequestKeyPrefix(unique_id_)};
 

content/browser/background_fetch/storage/get_initialization_data_task.cc

@@ -11,6 +11,7 @@
 #include "content/browser/background_fetch/background_fetch.pb.h"
 #include "content/browser/background_fetch/background_fetch_data_manager.h"
 #include "content/browser/background_fetch/storage/database_helpers.h"
+#include "content/browser/background_fetch/storage/image_helpers.h"
 #include "content/browser/background_fetch/storage/mark_registration_for_deletion_task.h"
 #include "content/browser/service_worker/service_worker_context_wrapper.h"
 #include "third_party/blink/public/common/manifest/manifest.h"
@@ -69,26 +70,27 @@ class InitializationSubTask : public DatabaseTask {
 };
 
 // Fills the BackgroundFetchInitializationData with the most recent UI title.
-class GetTitleTask : public InitializationSubTask {
+class GetUIOptionsTask : public InitializationSubTask {
  public:
-  GetTitleTask(DatabaseTaskHost* host,
-               const SubTaskInit& sub_task_init,
-               base::OnceClosure done_closure)
+  GetUIOptionsTask(DatabaseTaskHost* host,
+                   const SubTaskInit& sub_task_init,
+                   base::OnceClosure done_closure)
       : InitializationSubTask(host, sub_task_init, std::move(done_closure)),
         weak_factory_(this) {}
 
-  ~GetTitleTask() override = default;
+  ~GetUIOptionsTask() override = default;
 
   void Start() override {
     service_worker_context()->GetRegistrationUserData(
         sub_task_init().service_worker_registration_id,
-        {TitleKey(sub_task_init().unique_id)},
-        base::BindOnce(&GetTitleTask::DidGetTitle, weak_factory_.GetWeakPtr()));
+        {UIOptionsKey(sub_task_init().unique_id)},
+        base::BindOnce(&GetUIOptionsTask::DidGetUIOptions,
+                       weak_factory_.GetWeakPtr()));
   }
 
  private:
-  void DidGetTitle(const std::vector<std::string>& data,
-                   blink::ServiceWorkerStatusCode status) {
+  void DidGetUIOptions(const std::vector<std::string>& data,
+                       blink::ServiceWorkerStatusCode status) {
     switch (ToDatabaseStatus(status)) {
       case DatabaseStatus::kFailed:
         FinishWithError(blink::mojom::BackgroundFetchError::STORAGE_ERROR);
@@ -98,12 +100,36 @@ class GetTitleTask : public InitializationSubTask {
         break;
     }
 
-    if (!data.empty())
-      sub_task_init().initialization_data->ui_title = data.front();
+    if (data.size() != 1u) {
+      FinishWithError(blink::mojom::BackgroundFetchError::STORAGE_ERROR);
+      return;
+    }
+
+    proto::BackgroundFetchUIOptions ui_options;
+    if (!ui_options.ParseFromString(data[0])) {
+      FinishWithError(blink::mojom::BackgroundFetchError::STORAGE_ERROR);
+      return;
+    }
+
+    if (!ui_options.title().empty())
+      sub_task_init().initialization_data->ui_title = ui_options.title();
+
+    if (!ui_options.icon().empty()) {
+      // Start an icon deserialization SubTask on another thread, then finish.
+      DeserializeIcon(std::unique_ptr<std::string>(ui_options.release_icon()),
+                      base::BindOnce(&GetUIOptionsTask::DidDeserializeIcon,
+                                     weak_factory_.GetWeakPtr()));
+    } else {
+      FinishWithError(blink::mojom::BackgroundFetchError::NONE);
+    }
+  }
+
+  void DidDeserializeIcon(SkBitmap icon) {
+    sub_task_init().initialization_data->icon = std::move(icon);
     FinishWithError(blink::mojom::BackgroundFetchError::NONE);
   }
 
-  base::WeakPtrFactory<GetTitleTask> weak_factory_;  // Keep as last.
+  base::WeakPtrFactory<GetUIOptionsTask> weak_factory_;  // Keep as last.
 };
 
 // Gets the number of completed fetches, the number of active fetches,
@@ -243,48 +269,6 @@ class GetRequestsTask : public InitializationSubTask {
   DISALLOW_COPY_AND_ASSIGN(GetRequestsTask);
 };
 
-// Deserializes the icon and creates an SkBitmap from it.
-class DeserializeIconTask : public InitializationSubTask {
- public:
-  DeserializeIconTask(DatabaseTaskHost* host,
-                      const SubTaskInit& sub_task_init,
-                      base::OnceClosure done_closure,
-                      std::string* serialized_icon)
-      : InitializationSubTask(host, sub_task_init, std::move(done_closure)),
-        serialized_icon_(serialized_icon),
-        weak_factory_(this) {}
-
-  ~DeserializeIconTask() override = default;
-
-  void Start() override {
-    base::PostTaskWithTraitsAndReplyWithResult(
-        FROM_HERE,
-        {base::MayBlock(), base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN,
-         base::TaskPriority::BACKGROUND},
-        base::BindOnce(&DeserializeIcon, std::move(serialized_icon_)),
-        base::BindOnce(&DeserializeIconTask::StoreIcon,
-                       weak_factory_.GetWeakPtr()));
-  }
-
- private:
-  static SkBitmap DeserializeIcon(
-      std::unique_ptr<std::string> serialized_icon) {
-    return gfx::Image::CreateFrom1xPNGBytes(
-               reinterpret_cast<const unsigned char*>(serialized_icon->c_str()),
-               serialized_icon->size())
-        .AsBitmap();
-  }
-
-  void StoreIcon(SkBitmap icon) {
-    sub_task_init().initialization_data->icon = std::move(icon);
-    FinishWithError(blink::mojom::BackgroundFetchError::NONE);
-  }
-
-  std::unique_ptr<std::string> serialized_icon_;
-
-  base::WeakPtrFactory<DeserializeIconTask> weak_factory_;  // Keep as last.
-};
-
 // Fills the BackgroundFetchInitializationData with all the relevant information
 // stored in the BackgroundFetchMetadata proto.
 class FillFromMetadataTask : public InitializationSubTask {
@@ -382,18 +366,7 @@ class FillFromMetadataTask : public InitializationSubTask {
       }
     }
 
-    if (!metadata.icon().empty()) {
-      // Start an icon deserialization SubTask on another thread, then finish.
-      AddSubTask(std::make_unique<DeserializeIconTask>(
-          this, sub_task_init(),
-          base::BindOnce(&FillFromMetadataTask::FinishWithError,
-                         weak_factory_.GetWeakPtr(),
-                         blink::mojom::BackgroundFetchError::NONE),
-          metadata.release_icon()));
-    } else {
-      // Immediately finish.
-      FinishWithError(blink::mojom::BackgroundFetchError::NONE);
-    }
+    FinishWithError(blink::mojom::BackgroundFetchError::NONE);
   }
 
   base::WeakPtrFactory<FillFromMetadataTask> weak_factory_;  // Keep as last.
@@ -416,9 +389,9 @@ class FillBackgroundFetchInitializationDataTask : public InitializationSubTask {
   void Start() override {
     // We need 3 queries to get the initialization data. These are wrapped
     // in a BarrierClosure to avoid querying them serially.
-    // 1. Metadata (+ icon deserialization)
+    // 1. Metadata
     // 2. Request statuses and state sanitization
-    // 3. UI Title
+    // 3. UI Options (+ icon deserialization)
     base::RepeatingClosure barrier_closure = base::BarrierClosure(
         3u,
         base::BindOnce(
@@ -432,8 +405,8 @@ class FillBackgroundFetchInitializationDataTask : public InitializationSubTask {
                                                       barrier_closure));
     AddSubTask(std::make_unique<GetRequestsTask>(this, sub_task_init(),
                                                  barrier_closure));
-    AddSubTask(
-        std::make_unique<GetTitleTask>(this, sub_task_init(), barrier_closure));
+    AddSubTask(std::make_unique<GetUIOptionsTask>(this, sub_task_init(),
+                                                  barrier_closure));
   }
 
  private:

content/browser/background_fetch/storage/image_helpers.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/browser/background_fetch/storage/image_helpers.h"
+
+#include "base/sequenced_task_runner.h"
+#include "base/task_scheduler/post_task.h"
+#include "base/task_scheduler/task_traits.h"
+#include "ui/gfx/image/image.h"
+
+namespace content {
+
+namespace background_fetch {
+
+namespace {
+
+// The max icon resolution, this is used as a threshold to decide
+// whether the icon should be persisted.
+constexpr int kMaxIconResolution = 256 * 256;
+
+std::string ConvertAndSerializeIcon(const SkBitmap& icon) {
+  std::string serialized_icon;
+  auto icon_bytes = gfx::Image::CreateFrom1xBitmap(icon).As1xPNGBytes();
+  serialized_icon.assign(icon_bytes->front_as<char>(),
+                         icon_bytes->front_as<char>() + icon_bytes->size());
+  return serialized_icon;
+}
+
+SkBitmap DeserializeAndConvertIcon(
+    std::unique_ptr<std::string> serialized_icon) {
+  return gfx::Image::CreateFrom1xPNGBytes(
+             reinterpret_cast<const unsigned char*>(serialized_icon->c_str()),
+             serialized_icon->size())
+      .AsBitmap();
+}
+
+}  // namespace
+
+bool ShouldPersistIcon(const SkBitmap& icon) {
+  return !icon.isNull() && (icon.height() * icon.width() <= kMaxIconResolution);
+}
+
+void SerializeIcon(const SkBitmap& icon, SerializeIconCallback callback) {
+  DCHECK(!icon.isNull());
+  // Do the serialization on a seperate thread to avoid blocking on
+  // expensive operations (image conversions), then post back to current
+  // thread and continue normally.
+  base::PostTaskWithTraitsAndReplyWithResult(
+      FROM_HERE,
+      {base::MayBlock(), base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN,
+       base::TaskPriority::BACKGROUND},
+      base::BindOnce(&ConvertAndSerializeIcon, icon), std::move(callback));
+}
+
+void DeserializeIcon(std::unique_ptr<std::string> serialized_icon,
+                     DeserializeIconCallback callback) {
+  DCHECK(serialized_icon);
+  // Do the deserialization on a seperate thread to avoid blocking on
+  // expensive operations (image conversions), then post back to current
+  // thread and continue normally.
+  base::PostTaskWithTraitsAndReplyWithResult(
+      FROM_HERE,
+      {base::MayBlock(), base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN,
+       base::TaskPriority::BACKGROUND},
+      base::BindOnce(&DeserializeAndConvertIcon, std::move(serialized_icon)),
+      base::BindOnce(std::move(callback)));
+}
+
+}  // namespace background_fetch
+
+}  // namespace content

content/browser/background_fetch/storage/image_helpers.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CONTENT_BROWSER_BACKGROUND_FETCH_STORAGE_IMAGE_HELPERS_H_
+#define CONTENT_BROWSER_BACKGROUND_FETCH_STORAGE_IMAGE_HELPERS_H_
+
+#include <memory>
+#include <string>
+
+#include "base/callback_forward.h"
+#include "content/common/content_export.h"
+#include "third_party/skia/include/core/SkBitmap.h"
+
+namespace content {
+
+namespace background_fetch {
+
+using SerializeIconCallback = base::OnceCallback<void(std::string)>;
+using DeserializeIconCallback = base::OnceCallback<void(SkBitmap)>;
+
+// Checks whether the icon should be stored on disk. This is only the case for
+// non-null |icon|s with a resolution of at most 256x256 pixels.
+CONTENT_EXPORT bool ShouldPersistIcon(const SkBitmap& icon);
+
+// Serializes the icon on a separate Task Runner. The |icon| will be serialized
+// as a 1x bitmap to raw PNG-encoded data
+CONTENT_EXPORT void SerializeIcon(const SkBitmap& icon,
+                                  SerializeIconCallback callback);
+
+// Deserializes the icon on a separate Task Runner. he |serialized_icon| must
+// contain raw PNG-encoded data, which will be decoded to a 1x bitmap.
+CONTENT_EXPORT void DeserializeIcon(
+    std::unique_ptr<std::string> serialized_icon,
+    DeserializeIconCallback callback);
+
+}  // namespace background_fetch
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_BACKGROUND_FETCH_STORAGE_IMAGE_HELPERS_H_

content/browser/background_fetch/storage/image_helpers_unittest.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/browser/background_fetch/storage/image_helpers.h"
+
+#include <memory>
+#include <string>
+
+#include "base/bind.h"
+#include "base/logging.h"
+#include "base/run_loop.h"
+#include "base/test/scoped_task_environment.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/skia/include/core/SkBitmap.h"
+
+namespace content {
+
+namespace background_fetch {
+
+namespace {
+
+void DidSerializeIcon(base::OnceClosure quit_closure,
+                      std::string* out_icon,
+                      std::string icon) {
+  DCHECK(out_icon);
+  *out_icon = std::move(icon);
+  std::move(quit_closure).Run();
+}
+
+void DidDeserializeIcon(base::OnceClosure quit_closure,
+                        SkBitmap* out_icon,
+                        SkBitmap icon) {
+  DCHECK(out_icon);
+  *out_icon = std::move(icon);
+  std::move(quit_closure).Run();
+}
+
+TEST(BackgroundFetchImageHelpers, ShouldPersistIcon) {
+  SkBitmap null_icon;
+  EXPECT_FALSE(ShouldPersistIcon(null_icon));
+
+  SkBitmap large_icon;
+  large_icon.allocN32Pixels(512, 512);
+  EXPECT_FALSE(ShouldPersistIcon(large_icon));
+
+  SkBitmap valid_icon;
+  valid_icon.allocN32Pixels(42, 42);
+  EXPECT_TRUE(ShouldPersistIcon(valid_icon));
+}
+
+TEST(BackgroundFetchImageHelpers, SerializeRoundTrip) {
+  base::test::ScopedTaskEnvironment scoped_task_environment;
+
+  SkBitmap icon;
+  icon.allocN32Pixels(42, 42);
+  icon.eraseColor(SK_ColorGREEN);
+
+  // Serialize.
+  std::string serialized_icon;
+  {
+    base::RunLoop run_loop;
+    SerializeIcon(icon,
+                  base::BindOnce(&DidSerializeIcon, run_loop.QuitClosure(),
+                                 &serialized_icon));
+    run_loop.Run();
+  }
+
+  // Deserialize.
+  SkBitmap result_icon;
+  {
+    base::RunLoop run_loop;
+    DeserializeIcon(std::make_unique<std::string>(serialized_icon),
+                    base::BindOnce(&DidDeserializeIcon, run_loop.QuitClosure(),
+                                   &result_icon));
+    run_loop.Run();
+  }
+
+  ASSERT_FALSE(result_icon.isNull());
+  EXPECT_EQ(icon.height(), result_icon.height());
+  EXPECT_EQ(icon.width(), result_icon.width());
+  for (int i = 0; i < result_icon.width(); i++) {
+    for (int j = 0; j < result_icon.height(); j++)
+      EXPECT_EQ(result_icon.getColor(i, j), SK_ColorGREEN);
+  }
+}
+
+}  // namespace
+
+}  // namespace background_fetch
+
+}  // namespace content

content/browser/background_fetch/storage/update_registration_ui_task.cc

@@ -28,15 +28,21 @@ UpdateRegistrationUITask::UpdateRegistrationUITask(
 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_);
+
   service_worker_context()->StoreRegistrationUserData(
       registration_id_.service_worker_registration_id(),
       registration_id_.origin().GetURL(),
-      {{TitleKey(registration_id_.unique_id()), updated_title_}},
-      base::BindOnce(&UpdateRegistrationUITask::DidUpdateTitle,
+      {{UIOptionsKey(registration_id_.unique_id()),
+        ui_options.SerializeAsString()}},
+      base::BindOnce(&UpdateRegistrationUITask::DidUpdateUIOptions,
                      weak_factory_.GetWeakPtr()));
 }
 
-void UpdateRegistrationUITask::DidUpdateTitle(
+void UpdateRegistrationUITask::DidUpdateUIOptions(
     blink::ServiceWorkerStatusCode status) {
   switch (ToDatabaseStatus(status)) {
     case DatabaseStatus::kOk:

content/browser/background_fetch/storage/update_registration_ui_task.h

@@ -33,7 +33,7 @@ class UpdateRegistrationUITask : public DatabaseTask {
   void Start() override;
 
  private:
-  void DidUpdateTitle(blink::ServiceWorkerStatusCode status);
+  void DidUpdateUIOptions(blink::ServiceWorkerStatusCode status);
 
   void FinishWithError(blink::mojom::BackgroundFetchError error) override;
 

content/test/BUILD.gn

@@ -1266,6 +1266,7 @@ test("content_unittests") {
     "../browser/background_fetch/background_fetch_registration_notifier_unittest.cc",
     "../browser/background_fetch/background_fetch_scheduler_unittest.cc",
     "../browser/background_fetch/background_fetch_service_unittest.cc",
+    "../browser/background_fetch/storage/image_helpers_unittest.cc",
     "../browser/background_sync/background_sync_manager_unittest.cc",
     "../browser/background_sync/background_sync_network_observer_unittest.cc",
     "../browser/background_sync/background_sync_service_impl_unittest.cc",
@@ -1792,6 +1793,7 @@ test("content_unittests") {
     "//content:resources",
     "//content/app:both_for_content_tests",
     "//content/browser:for_content_tests",
+    "//content/browser/background_fetch:background_fetch_proto",
     "//content/browser/cache_storage:cache_storage_proto",
     "//content/browser/dom_storage:local_storage_proto",
     "//content/browser/notifications:notification_proto",