[Notification scheduler]: Wrap result for IconConverter.

- Support IconConverter to return a bundle of
success result and data.

Bug: 963290
Change-Id: I50d2908047aa1554d7d3c5a3ba2f1bf913c03d01
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1762819
Commit-Queue: Hesen Zhang <hesen@chromium.org>
Reviewed-by: Xing Liu <xingliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#690046}

chrome/browser/notifications/scheduler/internal/BUILD.gn

@@ -20,8 +20,8 @@ source_set("lib") {
     "distribution_policy.cc",
     "distribution_policy.h",
     "icon_converter.h",
-    "icon_converter_impl.cc",
-    "icon_converter_impl.h",
+    "icon_converter_result.cc",
+    "icon_converter_result.h",
     "icon_entry.cc",
     "icon_entry.h",
     "icon_store.cc",
@@ -46,6 +46,8 @@ source_set("lib") {
     "notification_scheduler_context.h",
     "notification_store.cc",
     "notification_store.h",
+    "png_icon_converter_impl.cc",
+    "png_icon_converter_impl.h",
     "proto_conversion.cc",
     "proto_conversion.h",
     "scheduled_notification_manager.cc",

chrome/browser/notifications/scheduler/internal/icon_converter.h

@@ -5,11 +5,13 @@
 #ifndef CHROME_BROWSER_NOTIFICATIONS_SCHEDULER_INTERNAL_ICON_CONVERTER_H_
 #define CHROME_BROWSER_NOTIFICATIONS_SCHEDULER_INTERNAL_ICON_CONVERTER_H_
 
+#include <memory>
 #include <string>
 #include <vector>
 
 #include "base/callback.h"
 #include "base/macros.h"
+#include "chrome/browser/notifications/scheduler/internal/icon_converter_result.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 
 namespace notifications {
@@ -18,8 +20,10 @@ namespace notifications {
 // and to decode strings to icons.
 class IconConverter {
  public:
-  using EncodeCallback = base::OnceCallback<void(std::vector<std::string>)>;
-  using DecodeCallback = base::OnceCallback<void(std::vector<SkBitmap>)>;
+  using EncodeCallback =
+      base::OnceCallback<void(std::unique_ptr<EncodeResult>)>;
+  using DecodeCallback =
+      base::OnceCallback<void(std::unique_ptr<DecodeResult>)>;
 
   // Converts SkBitmap icons to strings.
   virtual void ConvertIconToString(std::vector<SkBitmap> images,

chrome/browser/notifications/scheduler/internal/icon_converter_result.cc

+// Copyright 2019 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 "chrome/browser/notifications/scheduler/internal/icon_converter_result.h"
+
+#include <utility>
+#include <vector>
+
+namespace notifications {
+
+EncodeResult::EncodeResult(bool success, std::vector<std::string> data)
+    : success(success), encoded_data(std::move(data)) {}
+
+bool EncodeResult::operator==(const EncodeResult& other) const {
+  return success == other.success &&
+         encoded_data.size() == other.encoded_data.size();
+}
+
+EncodeResult::~EncodeResult() = default;
+
+DecodeResult::DecodeResult(bool success, std::vector<SkBitmap> icons)
+    : success(success), decoded_icons(std::move(icons)) {}
+
+bool DecodeResult::operator==(const DecodeResult& other) const {
+  return success == other.success &&
+         decoded_icons.size() == other.decoded_icons.size();
+}
+
+DecodeResult::~DecodeResult() = default;
+
+}  // namespace notifications

chrome/browser/notifications/scheduler/internal/icon_converter_result.h

+// Copyright 2019 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 CHROME_BROWSER_NOTIFICATIONS_SCHEDULER_INTERNAL_ICON_CONVERTER_RESULT_H_
+#define CHROME_BROWSER_NOTIFICATIONS_SCHEDULER_INTERNAL_ICON_CONVERTER_RESULT_H_
+
+#include <string>
+#include <vector>
+
+#include "base/optional.h"
+#include "third_party/skia/include/core/SkBitmap.h"
+
+namespace notifications {
+
+// Result from IconConverter's encoding process.
+struct EncodeResult {
+  EncodeResult(bool success, std::vector<std::string> data);
+  bool operator==(const EncodeResult& other) const;
+  ~EncodeResult();
+
+  bool success;
+  std::vector<std::string> encoded_data;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(EncodeResult);
+};
+
+// Result from IconConverter's decoding process.
+struct DecodeResult {
+  DecodeResult(bool success, std::vector<SkBitmap> icons);
+  bool operator==(const DecodeResult& other) const;
+  ~DecodeResult();
+
+  bool success;
+  std::vector<SkBitmap> decoded_icons;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(DecodeResult);
+};
+
+}  // namespace notifications
+
+#endif  // CHROME_BROWSER_NOTIFICATIONS_SCHEDULER_INTERNAL_ICON_CONVERTER_RESULT_H_

chrome/browser/notifications/scheduler/internal/icon_converter_unittest.cc

@@ -5,7 +5,7 @@
 #include "base/bind.h"
 #include "base/run_loop.h"
 #include "base/test/task_environment.h"
-#include "chrome/browser/notifications/scheduler/internal/icon_converter_impl.h"
+#include "chrome/browser/notifications/scheduler/internal/png_icon_converter_impl.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace notifications {
@@ -13,46 +13,49 @@ namespace {
 
 class IconConverterTest : public testing::Test {
  public:
-  IconConverterTest() {}
+  IconConverterTest() : encoded_result_(), decoded_result_() {}
   ~IconConverterTest() override = default;
 
   void SetUp() override {
-    auto icon_converter = std::make_unique<IconConverterImpl>();
+    auto icon_converter = std::make_unique<PngIconConverterImpl>();
     icon_converter_ = std::move(icon_converter);
-    encoded_data_.clear();
-    decoded_icons_.clear();
   }
 
   void OnIconsEncoded(base::OnceClosure quit_closure,
-                      std::vector<std::string> encoded_data) {
-    encoded_data_ = std::move(encoded_data);
+                      std::unique_ptr<EncodeResult> encoded_result) {
+    encoded_result_ = std::move(encoded_result);
     std::move(quit_closure).Run();
   }
 
   void OnIconsDecoded(base::OnceClosure quit_closure,
-                      std::vector<SkBitmap> decoded_icons) {
-    decoded_icons_ = std::move(decoded_icons);
+                      std::unique_ptr<DecodeResult> decoded_result) {
+    decoded_result_ = std::move(decoded_result);
     std::move(quit_closure).Run();
   }
 
   void VerifyEncodeRoundTrip(std::vector<SkBitmap> input) {
-    EXPECT_EQ(decoded_icons_.size(), input.size());
+    EXPECT_EQ(decoded_icons()->size(), input.size());
     for (size_t i = 0; i < input.size(); i++) {
-      EXPECT_EQ(input[i].height(), decoded_icons_[i].height());
-      EXPECT_EQ(input[i].width(), decoded_icons_[i].width());
+      EXPECT_EQ(input[i].height(), decoded_icons()->at(i).height());
+      EXPECT_EQ(input[i].width(), decoded_icons()->at(i).width());
     }
   }
 
  protected:
   IconConverter* icon_converter() { return icon_converter_.get(); }
-  std::vector<SkBitmap>* decoded_icons() { return &decoded_icons_; }
-  std::vector<std::string>* encoded_data() { return &encoded_data_; }
+  std::vector<SkBitmap>* decoded_icons() {
+    return &decoded_result_->decoded_icons;
+  }
+  std::vector<std::string>* encoded_data() {
+    return &encoded_result_->encoded_data;
+  }
 
  private:
   base::test::TaskEnvironment task_environment_;
   std::unique_ptr<IconConverter> icon_converter_;
-  std::vector<std::string> encoded_data_;
-  std::vector<SkBitmap> decoded_icons_;
+  std::unique_ptr<EncodeResult> encoded_result_;
+  std::unique_ptr<DecodeResult> decoded_result_;
+
   DISALLOW_COPY_AND_ASSIGN(IconConverterTest);
 };
 

chrome/browser/notifications/scheduler/internal/icon_store.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/notifications/scheduler/internal/icon_store.h"
 
+#include <map>
 #include <utility>
 
 #include "base/guid.h"
@@ -115,13 +116,17 @@ void IconProtoDbStore::OnIconEntriesLoaded(
 void IconProtoDbStore::OnIconsEncoded(
     AddCallback callback,
     std::vector<std::string> icons_uuid,
-    std::vector<std::string> encoded_icons_data) {
-  // TODO(hesen): Handle result return from PNGCodec.
-  DCHECK_EQ(icons_uuid.size(), encoded_icons_data.size());
+    std::unique_ptr<EncodeResult> encode_result) {
+  if (!encode_result->success) {
+    std::move(callback).Run({} /*icons_uuid*/, false);
+    return;
+  }
+
   auto entries_to_save = std::make_unique<KeyEntryVector>();
-  for (size_t i = 0; i < encoded_icons_data.size(); i++) {
+  auto encoded_data = std::move(encode_result->encoded_data);
+  for (size_t i = 0; i < encoded_data.size(); i++) {
     IconEntry icon_entry;
-    icon_entry.data = std::move(encoded_icons_data[i]);
+    icon_entry.data = std::move(encoded_data[i]);
     entries_to_save->emplace_back(icons_uuid[i], std::move(icon_entry));
   }
   auto add_callback =
@@ -131,15 +136,21 @@ void IconProtoDbStore::OnIconsEncoded(
                      std::move(add_callback));
 }
 
-void IconProtoDbStore::OnIconsDecoded(LoadIconsCallback callback,
-                                      std::vector<std::string> icons_uuid,
-                                      std::vector<SkBitmap> decoded_icons) {
-  // TODO(hesen): Handle result return from PNGCodec.
-  IconsMap icons;
+void IconProtoDbStore::OnIconsDecoded(
+    LoadIconsCallback callback,
+    std::vector<std::string> icons_uuid,
+    std::unique_ptr<DecodeResult> decoded_result) {
+  if (!decoded_result->success) {
+    std::move(callback).Run(false, {} /*IconsMap*/);
+    return;
+  }
+
+  IconsMap icons_map;
+  auto icons = std::move(decoded_result->decoded_icons);
   for (size_t i = 0; i < icons_uuid.size(); i++) {
-    icons.emplace(std::move(icons_uuid[i]), decoded_icons[i]);
+    icons_map.emplace(std::move(icons_uuid[i]), std::move(icons[i]));
   }
-  std::move(callback).Run(true, std::move(icons));
+  std::move(callback).Run(true, std::move(icons_map));
 }
 
 }  // namespace notifications

chrome/browser/notifications/scheduler/internal/icon_store.h

@@ -5,6 +5,7 @@
 #ifndef CHROME_BROWSER_NOTIFICATIONS_SCHEDULER_INTERNAL_ICON_STORE_H_
 #define CHROME_BROWSER_NOTIFICATIONS_SCHEDULER_INTERNAL_ICON_STORE_H_
 
+#include <map>
 #include <memory>
 #include <string>
 #include <vector>
@@ -15,6 +16,7 @@
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/notifications/proto/icon.pb.h"
 #include "chrome/browser/notifications/scheduler/internal/icon_converter.h"
+#include "chrome/browser/notifications/scheduler/internal/icon_converter_result.h"
 #include "chrome/browser/notifications/scheduler/internal/icon_entry.h"
 #include "chrome/browser/notifications/scheduler/public/notification_data.h"
 #include "components/leveldb_proto/public/proto_database.h"
@@ -92,12 +94,12 @@ class IconProtoDbStore : public IconStore {
   // Called when the icons are encoded.
   void OnIconsEncoded(AddCallback callback,
                       std::vector<std::string> icons_uuid,
-                      std::vector<std::string> encoded_icons_data);
+                      std::unique_ptr<EncodeResult> encode_result);
 
   // Called when the encoded data are decoded.
   void OnIconsDecoded(LoadIconsCallback callback,
                       std::vector<std::string> icons_uuid,
-                      std::vector<SkBitmap> decoded_icons);
+                      std::unique_ptr<DecodeResult> decode_result);
 
   // The proto database instance that persists data.
   std::unique_ptr<leveldb_proto::ProtoDatabase<proto::Icon, IconEntry>> db_;

chrome/browser/notifications/scheduler/internal/icon_store_unittest.cc

@@ -75,8 +75,9 @@ class IconStoreTest : public testing::Test {
     EXPECT_CALL(*icon_converter(), ConvertStringToIcon(_, _))
         .WillOnce(Invoke([&](std::vector<std::string> encoded_icons,
                              IconConverter::DecodeCallback callback) {
-          std::vector<SkBitmap> results(encoded_icons.size());
-          std::move(callback).Run(std::move(results));
+          std::vector<SkBitmap> icons(encoded_icons.size());
+          std::move(callback).Run(
+              std::make_unique<DecodeResult>(true, std::move(icons)));
         }));
     store()->LoadIcons(
         std::move(keys),
@@ -89,8 +90,9 @@ class IconStoreTest : public testing::Test {
     EXPECT_CALL(*icon_converter(), ConvertIconToString(_, _))
         .WillOnce(Invoke([&](std::vector<SkBitmap> icons,
                              IconConverter::EncodeCallback callback) {
-          std::vector<std::string> results(icons.size());
-          std::move(callback).Run(std::move(results));
+          std::vector<std::string> encoded_icons(icons.size());
+          std::move(callback).Run(
+              std::make_unique<EncodeResult>(true, std::move(encoded_icons)));
         }));
     store()->AddIcons(std::move(input), std::move(add_icons_callback));
   }

chrome/browser/notifications/scheduler/internal/icon_converter_impl.cc → chrome/browser/notifications/scheduler/internal/png_icon_converter_impl.cc

@@ -2,8 +2,9 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chrome/browser/notifications/scheduler/internal/icon_converter_impl.h"
+#include "chrome/browser/notifications/scheduler/internal/png_icon_converter_impl.h"
 
+#include <memory>
 #include <utility>
 
 #include "base/base64.h"
@@ -15,42 +16,49 @@
 namespace notifications {
 namespace {
 
-std::vector<IconEntry::IconData> ConvertIconToStringInternal(
+std::unique_ptr<EncodeResult> ConvertIconToStringInternal(
     std::vector<SkBitmap> images) {
   base::AssertLongCPUWorkAllowed();
-  std::vector<std::string> results;
+  std::vector<std::string> encoded_data;
   for (size_t i = 0; i < images.size(); i++) {
     std::vector<unsigned char> image_data;
-    gfx::PNGCodec::EncodeBGRASkBitmap(
+    bool success = gfx::PNGCodec::EncodeBGRASkBitmap(
         std::move(images[i]), false /*discard_transparency*/, &image_data);
+    if (!success)
+      return std::make_unique<EncodeResult>(false, std::vector<std::string>());
     std::string encoded(image_data.begin(), image_data.end());
-    results.emplace_back(std::move(encoded));
+    encoded_data.emplace_back(std::move(encoded));
   }
-  return results;
+  return std::make_unique<EncodeResult>(
+      true, std::vector<std::string>(std::move(encoded_data)));
 }
 
-std::vector<SkBitmap> ConvertStringToIconInternal(
+std::unique_ptr<DecodeResult> ConvertStringToIconInternal(
     std::vector<std::string> encoded_data) {
   base::AssertLongCPUWorkAllowed();
-  std::vector<SkBitmap> images;
+  std::vector<SkBitmap> icons;
   for (size_t i = 0; i < encoded_data.size(); i++) {
     SkBitmap image;
-    gfx::PNGCodec::Decode(reinterpret_cast<const unsigned char*>(
-                              std::move(encoded_data[i]).data()),
-                          encoded_data[i].length(), &image);
-    images.emplace_back(std::move(image));
+    bool success = gfx::PNGCodec::Decode(reinterpret_cast<const unsigned char*>(
+                                             std::move(encoded_data[i]).data()),
+                                         encoded_data[i].length(), &image);
+    if (!success)
+      return std::make_unique<DecodeResult>(false, std::vector<SkBitmap>());
+
+    icons.emplace_back(std::move(image));
   }
-  return images;
+  return std::make_unique<DecodeResult>(
+      true, std::vector<SkBitmap>(std::move(icons)));
 }
 
 }  // namespace
 
-IconConverterImpl::IconConverterImpl() = default;
+PngIconConverterImpl::PngIconConverterImpl() = default;
 
-IconConverterImpl::~IconConverterImpl() = default;
+PngIconConverterImpl::~PngIconConverterImpl() = default;
 
-void IconConverterImpl::ConvertIconToString(std::vector<SkBitmap> images,
-                                            EncodeCallback callback) {
+void PngIconConverterImpl::ConvertIconToString(std::vector<SkBitmap> images,
+                                               EncodeCallback callback) {
   DCHECK(callback);
   base::PostTaskAndReplyWithResult(
       FROM_HERE,
@@ -59,7 +67,7 @@ void IconConverterImpl::ConvertIconToString(std::vector<SkBitmap> images,
       std::move(callback));
 }
 
-void IconConverterImpl::ConvertStringToIcon(
+void PngIconConverterImpl::ConvertStringToIcon(
     std::vector<std::string> encoded_data,
     DecodeCallback callback) {
   DCHECK(callback);

chrome/browser/notifications/scheduler/internal/icon_converter_impl.h → chrome/browser/notifications/scheduler/internal/png_icon_converter_impl.h

@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef CHROME_BROWSER_NOTIFICATIONS_SCHEDULER_INTERNAL_ICON_CONVERTER_IMPL_H_
-#define CHROME_BROWSER_NOTIFICATIONS_SCHEDULER_INTERNAL_ICON_CONVERTER_IMPL_H_
+#ifndef CHROME_BROWSER_NOTIFICATIONS_SCHEDULER_INTERNAL_PNG_ICON_CONVERTER_IMPL_H_
+#define CHROME_BROWSER_NOTIFICATIONS_SCHEDULER_INTERNAL_PNG_ICON_CONVERTER_IMPL_H_
 
 #include <string>
 #include <vector>
@@ -14,10 +14,10 @@
 
 namespace notifications {
 
-class IconConverterImpl : public IconConverter {
+class PngIconConverterImpl : public IconConverter {
  public:
-  IconConverterImpl();
-  ~IconConverterImpl() override;
+  PngIconConverterImpl();
+  ~PngIconConverterImpl() override;
 
  private:
   // IconConverter implementation.
@@ -26,9 +26,9 @@ class IconConverterImpl : public IconConverter {
   void ConvertStringToIcon(std::vector<std::string> encoded_data,
                            DecodeCallback callback) override;
 
-  DISALLOW_COPY_AND_ASSIGN(IconConverterImpl);
+  DISALLOW_COPY_AND_ASSIGN(PngIconConverterImpl);
 };
 
 }  // namespace notifications
 
-#endif  // CHROME_BROWSER_NOTIFICATIONS_SCHEDULER_INTERNAL_ICON_CONVERTER_IMPL_H_
+#endif  // CHROME_BROWSER_NOTIFICATIONS_SCHEDULER_INTERNAL_PNG_ICON_CONVERTER_IMPL_H_

chrome/browser/notifications/scheduler/schedule_service_factory_helper.cc

@@ -12,7 +12,6 @@
 #include "base/time/default_clock.h"
 #include "chrome/browser/notifications/scheduler/internal/background_task_coordinator.h"
 #include "chrome/browser/notifications/scheduler/internal/display_decider.h"
-#include "chrome/browser/notifications/scheduler/internal/icon_converter_impl.h"
 #include "chrome/browser/notifications/scheduler/internal/icon_store.h"
 #include "chrome/browser/notifications/scheduler/internal/impression_history_tracker.h"
 #include "chrome/browser/notifications/scheduler/internal/impression_store.h"
@@ -21,6 +20,7 @@
 #include "chrome/browser/notifications/scheduler/internal/notification_schedule_service_impl.h"
 #include "chrome/browser/notifications/scheduler/internal/notification_scheduler_context.h"
 #include "chrome/browser/notifications/scheduler/internal/notification_store.h"
+#include "chrome/browser/notifications/scheduler/internal/png_icon_converter_impl.h"
 #include "chrome/browser/notifications/scheduler/internal/scheduled_notification_manager.h"
 #include "chrome/browser/notifications/scheduler/internal/scheduler_config.h"
 #include "chrome/browser/notifications/scheduler/internal/scheduler_utils.h"
@@ -65,7 +65,7 @@ KeyedService* CreateNotificationScheduleService(
       leveldb_proto::ProtoDbType::NOTIFICATION_SCHEDULER_ICON_STORE,
       icon_store_dir, task_runner);
   auto icon_store = std::make_unique<IconProtoDbStore>(
-      std::move(icon_db), std::make_unique<IconConverterImpl>());
+      std::move(icon_db), std::make_unique<PngIconConverterImpl>());
 
   // Build impression tracker.
   base::FilePath impression_store_dir = storage_dir.Append(kImpressionDBName);