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

This reverts commit 9311d412.

Reason for revert:

Findit (https://goo.gl/kROfz5) identified CL at revision 577500 as the
culprit for failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyRAsSDVdmU3VzcGVjdGVkQ0wiMWNocm9taXVtLzkzMTFkNDEyNTNmYzc0ZDQ5NTI3ZmIyZWU2OWUwMzYzMzM0Yjk2OWQM

Sample Failed Build: https://ci.chromium.org/buildbot/chromium.win/Win%207%20Tests%20x64%20%281%29/41118

Sample Failed Step: viz_content_unittests

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}

Change-Id: I60aa46070806c053cc60d77b32456d192de378c8
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 865063
Reviewed-on: https://chromium-review.googlesource.com/1148500
Cr-Commit-Position: refs/heads/master@{#577559}

content/browser/BUILD.gn

@@ -446,8 +446,6 @@ 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,8 +30,6 @@ 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
@@ -68,7 +66,7 @@ message BackgroundFetchOptions {
 // in memory. This information should be everything needed to reconstruct
 // the state of an interrupted background fetch.
 //
-// Next Tag: 6
+// Next Tag: 7
 message BackgroundFetchMetadata {
   optional int64 creation_microseconds_since_unix_epoch = 1;
   optional string origin = 2;
@@ -78,17 +76,10 @@ 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 = 2;
+  optional bytes icon = 6;
 }
 
 // A background fetch request that is still in a pending state.

content/browser/background_fetch/background_fetch_data_manager_unittest.cc

@@ -16,7 +16,6 @@
 #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"
@@ -408,21 +407,6 @@ 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;
-
-    DCHECK(ui_options.ParseFromString(results[0]));
-    return ui_options;
-  }
-
   // Gets information about the number of background fetch requests by state.
   ResponseStateStats GetRequestStats(int64_t service_worker_registration_id) {
     ResponseStateStats stats;
@@ -779,7 +763,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(metadata->icon().empty());
 }
 
 TEST_F(BackgroundFetchDataManagerTest, UpdateRegistrationUI) {
@@ -795,14 +779,19 @@ TEST_F(BackgroundFetchDataManagerTest, UpdateRegistrationUI) {
   blink::mojom::BackgroundFetchError error;
 
   // There should be no title before the registration.
-  EXPECT_TRUE(GetUIOptions(sw_id).title().empty());
+  std::vector<std::string> title = GetRegistrationUserDataByKeyPrefix(
+      sw_id, background_fetch::kTitleKeyPrefix);
+  EXPECT_TRUE(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.
-  ASSERT_EQ(GetUIOptions(sw_id).title(), kInitialTitle);
+  title = GetRegistrationUserDataByKeyPrefix(sw_id,
+                                             background_fetch::kTitleKeyPrefix);
+  EXPECT_EQ(title.size(), 1u);
+  ASSERT_EQ(title.front(), kInitialTitle);
 
   // Update the title.
   {
@@ -815,7 +804,10 @@ TEST_F(BackgroundFetchDataManagerTest, UpdateRegistrationUI) {
   RestartDataManagerFromPersistentStorage();
 
   // After a restart, GetMetadata should find the new title.
-  ASSERT_EQ(GetUIOptions(sw_id).title(), kUpdatedTitle);
+  title = GetRegistrationUserDataByKeyPrefix(sw_id,
+                                             background_fetch::kTitleKeyPrefix);
+  EXPECT_EQ(title.size(), 1u);
+  ASSERT_EQ(title.front(), kUpdatedTitle);
 }
 
 TEST_F(BackgroundFetchDataManagerTest, CreateAndDeleteRegistration) {
@@ -1365,6 +1357,8 @@ 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();
@@ -1376,7 +1370,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, kInitialTitle);
+    EXPECT_EQ(init.ui_title, kUpdatedTitle);
     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_ui_options_<unique_id>"
-value: "<serialized content::proto::BackgroundFetchUIOptions>"
+key: "bgfetch_title_<unique_id>"
+value: "<ui_title>"
 ```
 
 ```

content/browser/background_fetch/storage/create_metadata_task.cc

@@ -6,17 +6,37 @@
 
 #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,
@@ -46,7 +66,8 @@ void CreateMetadataTask::DidGetUniqueId(const std::vector<std::string>& data,
                                         blink::ServiceWorkerStatusCode status) {
   switch (ToDatabaseStatus(status)) {
     case DatabaseStatus::kNotFound:
-      break;
+      InitializeMetadataProto();
+      return;
     case DatabaseStatus::kOk:
       // Can't create a registration since there is already an active
       // registration with the same |developer_id|. It must be deactivated
@@ -58,17 +79,6 @@ 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() {
@@ -116,21 +126,35 @@ 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::DidSerializeIcon(std::string serialized_icon) {
-  serialized_icon_ = std::move(serialized_icon);
+void CreateMetadataTask::StoreIcon(std::string serialized_icon) {
+  DCHECK(metadata_proto_);
+  metadata_proto_->set_icon(std::move(serialized_icon));
   StoreMetadata();
 }
 
 void CreateMetadataTask::StoreMetadata() {
   DCHECK(metadata_proto_);
   std::vector<std::pair<std::string, std::string>> entries;
-  // - One BackgroundFetchPendingRequest per request
-  // - DeveloperId -> UniqueID
-  // - BackgroundFetchMetadata
-  // - BackgroundFetchUIOptions
-  entries.reserve(requests_.size() + 3);
+  entries.reserve(requests_.size() * 2 + 1);
 
   std::string serialized_metadata_proto;
 
@@ -139,24 +163,12 @@ 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(UIOptionsKey(registration_id_.unique_id()),
-                       serialized_ui_options_proto);
+  entries.emplace_back(TitleKey(registration_id_.unique_id()), options_.title);
 
   // 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 DidSerializeIcon(std::string serialized_icon);
+  void StoreIcon(std::string serialized_icon);
 
   void StoreMetadata();
 
@@ -60,8 +60,6 @@ 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 UIOptionsKey(const std::string& unique_id) {
-  return kUIOptionsKeyPrefix + unique_id;
+std::string TitleKey(const std::string& unique_id) {
+  return kTitleKeyPrefix + unique_id;
 }
 
 std::string PendingRequestKeyPrefix(const std::string& unique_id) {

content/browser/background_fetch/storage/database_helpers.h

@@ -24,17 +24,16 @@ const char kSeparator[] = "_";
 const char kActiveRegistrationUniqueIdKeyPrefix[] =
     "bgfetch_active_registration_unique_id_";
 const char kRegistrationKeyPrefix[] = "bgfetch_registration_";
-const char kUIOptionsKeyPrefix[] = "bgfetch_ui_options_";
+const char kTitleKeyPrefix[] = "bgfetch_title_";
 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 UIOptionsKey(const std::string& unique_id);
+std::string TitleKey(const std::string& unique_id);
 
 std::string PendingRequestKeyPrefix(const std::string& unique_id);
 
@@ -49,7 +48,6 @@ 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_), UIOptionsKey(unique_id_),
+      RegistrationKey(unique_id_), TitleKey(unique_id_),
       PendingRequestKeyPrefix(unique_id_), ActiveRequestKeyPrefix(unique_id_),
       CompletedRequestKeyPrefix(unique_id_)};
 

content/browser/background_fetch/storage/get_initialization_data_task.cc

@@ -11,7 +11,6 @@
 #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"
@@ -70,26 +69,25 @@ class InitializationSubTask : public DatabaseTask {
 };
 
 // Fills the BackgroundFetchInitializationData with the most recent UI title.
-class GetUIOptionsTask : public InitializationSubTask {
+class GetTitleTask : public InitializationSubTask {
  public:
-  GetUIOptionsTask(DatabaseTaskHost* host,
+  GetTitleTask(DatabaseTaskHost* host,
                const SubTaskInit& sub_task_init,
                base::OnceClosure done_closure)
       : InitializationSubTask(host, sub_task_init, std::move(done_closure)),
         weak_factory_(this) {}
 
-  ~GetUIOptionsTask() override = default;
+  ~GetTitleTask() override = default;
 
   void Start() override {
     service_worker_context()->GetRegistrationUserData(
         sub_task_init().service_worker_registration_id,
-        {UIOptionsKey(sub_task_init().unique_id)},
-        base::BindOnce(&GetUIOptionsTask::DidGetUIOptions,
-                       weak_factory_.GetWeakPtr()));
+        {TitleKey(sub_task_init().unique_id)},
+        base::BindOnce(&GetTitleTask::DidGetTitle, weak_factory_.GetWeakPtr()));
   }
 
  private:
-  void DidGetUIOptions(const std::vector<std::string>& data,
+  void DidGetTitle(const std::vector<std::string>& data,
                    blink::ServiceWorkerStatusCode status) {
     switch (ToDatabaseStatus(status)) {
       case DatabaseStatus::kFailed:
@@ -100,36 +98,12 @@ class GetUIOptionsTask : public InitializationSubTask {
         break;
     }
 
-    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);
+    if (!data.empty())
+      sub_task_init().initialization_data->ui_title = data.front();
     FinishWithError(blink::mojom::BackgroundFetchError::NONE);
   }
 
-  base::WeakPtrFactory<GetUIOptionsTask> weak_factory_;  // Keep as last.
+  base::WeakPtrFactory<GetTitleTask> weak_factory_;  // Keep as last.
 };
 
 // Gets the number of completed fetches, the number of active fetches,
@@ -269,6 +243,48 @@ 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 {
@@ -366,8 +382,19 @@ 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);
     }
+  }
 
   base::WeakPtrFactory<FillFromMetadataTask> weak_factory_;  // Keep as last.
 
@@ -389,9 +416,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
+    // 1. Metadata (+ icon deserialization)
     // 2. Request statuses and state sanitization
-    // 3. UI Options (+ icon deserialization)
+    // 3. UI Title
     base::RepeatingClosure barrier_closure = base::BarrierClosure(
         3u,
         base::BindOnce(
@@ -405,8 +432,8 @@ class FillBackgroundFetchInitializationDataTask : public InitializationSubTask {
                                                       barrier_closure));
     AddSubTask(std::make_unique<GetRequestsTask>(this, sub_task_init(),
                                                  barrier_closure));
-    AddSubTask(std::make_unique<GetUIOptionsTask>(this, sub_task_init(),
-                                                  barrier_closure));
+    AddSubTask(
+        std::make_unique<GetTitleTask>(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,21 +28,15 @@ 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(),
-      {{UIOptionsKey(registration_id_.unique_id()),
-        ui_options.SerializeAsString()}},
-      base::BindOnce(&UpdateRegistrationUITask::DidUpdateUIOptions,
+      {{TitleKey(registration_id_.unique_id()), updated_title_}},
+      base::BindOnce(&UpdateRegistrationUITask::DidUpdateTitle,
                      weak_factory_.GetWeakPtr()));
 }
 
-void UpdateRegistrationUITask::DidUpdateUIOptions(
+void UpdateRegistrationUITask::DidUpdateTitle(
     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 DidUpdateUIOptions(blink::ServiceWorkerStatusCode status);
+  void DidUpdateTitle(blink::ServiceWorkerStatusCode status);
 
   void FinishWithError(blink::mojom::BackgroundFetchError error) override;
 

content/test/BUILD.gn

@@ -1266,7 +1266,6 @@ 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",
@@ -1793,7 +1792,6 @@ 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",