Give Explore Sites feature its own Data Decoder

Instead of going through Service Manager to get a Data Decoder instance,
this changes the Explore Sites feature to maintain its own lazily
spawned Data Decoder instance using the public Content API.

This should effectively mirror the old behavior, including the 5-second
timeout of the instance used.

Bug: 977637
Change-Id: Ic76a472dabffbbce339cde6c3fab21d1f8b9a195
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1872418
Commit-Queue: Ken Rockot <rockot@google.com>
Reviewed-by: Justin DeWitt <dewittj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#708857}

chrome/browser/BUILD.gn

@@ -1891,6 +1891,7 @@ jumbo_static_library("browser") {
     "//components/sync_device_info",
     "//content/public/browser",
     "//ipc",
+    "//services/data_decoder/public/mojom",
     "//services/image_annotation/public/mojom",
     "//sql",
   ]

chrome/browser/android/explore_sites/image_helper.cc

@@ -7,10 +7,10 @@
 #include "base/bind.h"
 #include "base/memory/weak_ptr.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/time/time.h"
 #include "chrome/browser/android/explore_sites/explore_sites_types.h"
-#include "content/public/browser/system_connector.h"
+#include "content/public/browser/data_decoder_service.h"
 #include "services/data_decoder/public/cpp/decode_image.h"
-#include "services/service_manager/public/cpp/connector.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "third_party/skia/include/core/SkImageInfo.h"
 #include "third_party/skia/include/core/SkPixmap.h"
@@ -19,9 +19,17 @@
 #include "ui/gfx/geometry/size.h"
 
 namespace explore_sites {
+
 namespace {
+
 // Ratio of icon size to the amount of padding between the icons.
 const int kIconPaddingScale = 8;
+
+// How long to let our Data Decoder service instance hang around unused before
+// terminating it.
+constexpr base::TimeDelta kDataDecoderInstanceTimeout{
+    base::TimeDelta::FromSeconds(5)};
+
 }  // namespace
 
 // Class Job is used to manage multiple calls to the ImageHelper. Each request
@@ -31,26 +39,26 @@ class ImageHelper::Job {
  public:
   // WARNING: When ImageJobFinishedCallback is called, |this| may be deleted.
   // So nothing can be called after this callback.
-  Job(ImageJobType job_type,
+  Job(ImageHelper* image_helper,
+      ImageJobType job_type,
       ImageJobFinishedCallback job_finished_callback,
       BitmapCallback bitmap_callback,
       EncodedImageList images,
-      int pixel_size,
-      std::unique_ptr<service_manager::Connector> connector);
+      int pixel_size);
   ~Job();
 
   // Start begins the work that a Job performs (decoding and composition).
-  void Start();
+  void Start(data_decoder::mojom::DataDecoderService* service_override);
 
-  void DecodeImageBytes(std::unique_ptr<EncodedImageBytes> image_bytes);
+  void DecodeImageBytes(
+      std::unique_ptr<EncodedImageBytes> image_bytes,
+      data_decoder::mojom::DataDecoderService* service_override);
   void OnDecodeSiteImageDone(const SkBitmap& decoded_image);
   void OnDecodeCategoryImageDone(const SkBitmap& decoded_image);
   std::unique_ptr<SkBitmap> CombineImages();
 
  private:
-  // Used to inject connector in tests.
-  void SetupConnector();
-
+  ImageHelper* const image_helper_;
   const ImageJobType job_type_;
   ImageJobFinishedCallback job_finished_callback_;
   BitmapCallback bitmap_callback_;
@@ -59,50 +67,42 @@ class ImageHelper::Job {
   int num_icons_, pixel_size_;
   std::vector<SkBitmap> bitmaps_;
 
-  std::unique_ptr<service_manager::Connector> connector_;
-
   base::WeakPtrFactory<Job> weak_ptr_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(Job);
 };
 
-ImageHelper::Job::Job(ImageJobType job_type,
+ImageHelper::Job::Job(ImageHelper* image_helper,
+                      ImageJobType job_type,
                       ImageJobFinishedCallback job_finished_callback,
                       BitmapCallback bitmap_callback,
                       EncodedImageList images,
-                      int pixel_size,
-                      std::unique_ptr<service_manager::Connector> connector)
-    : job_type_(job_type),
+                      int pixel_size)
+    : image_helper_(image_helper),
+      job_type_(job_type),
       job_finished_callback_(std::move(job_finished_callback)),
       bitmap_callback_(std::move(bitmap_callback)),
       images_(std::move(images)),
-      pixel_size_(pixel_size),
-      connector_(std::move(connector)) {
+      pixel_size_(pixel_size) {
   num_icons_ = (images_.size() < kFaviconsPerCategoryImage)
                    ? images_.size()
                    : kFaviconsPerCategoryImage;
 }
 
-ImageHelper::Job::~Job() {}
+ImageHelper::Job::~Job() = default;
 
-void ImageHelper::Job::Start() {
+void ImageHelper::Job::Start(
+    data_decoder::mojom::DataDecoderService* service_override) {
   for (int i = 0; i < num_icons_; i++) {
     // TODO(freedjm): preserve order of images.
     DVLOG(1) << "Decoding image " << i + 1 << " of " << images_.size();
-    DecodeImageBytes(std::move(images_[i]));
+    DecodeImageBytes(std::move(images_[i]), service_override);
   }
 }
 
-void ImageHelper::Job::SetupConnector() {
-  connector_ = content::GetSystemConnector()->Clone();
-}
-
 void ImageHelper::Job::DecodeImageBytes(
-    std::unique_ptr<EncodedImageBytes> image_bytes) {
-  if (!connector_) {
-    SetupConnector();
-  }
-
+    std::unique_ptr<EncodedImageBytes> image_bytes,
+    data_decoder::mojom::DataDecoderService* service_override) {
   data_decoder::mojom::ImageDecoder::DecodeImageCallback callback;
   if (job_type_ == ImageJobType::kSiteIcon) {
     callback = base::BindOnce(&ImageHelper::Job::OnDecodeSiteImageDone,
@@ -112,7 +112,16 @@ void ImageHelper::Job::DecodeImageBytes(
                               weak_ptr_factory_.GetWeakPtr());
   }
 
-  data_decoder::DecodeImage(connector_.get(), *image_bytes,
+  mojo::PendingRemote<data_decoder::mojom::ImageDecoder> decoder;
+  if (service_override) {
+    service_override->BindImageDecoder(
+        decoder.InitWithNewPipeAndPassReceiver());
+  } else {
+    image_helper_->GetDataDecoder()->BindImageDecoder(
+        decoder.InitWithNewPipeAndPassReceiver());
+  }
+
+  data_decoder::DecodeImage(std::move(decoder), *image_bytes,
                             data_decoder::mojom::ImageCodec::DEFAULT, false,
                             data_decoder::kDefaultMaxSizeInBytes, gfx::Size(),
                             std::move(callback));
@@ -285,12 +294,12 @@ void ImageHelper::NewJob(
     BitmapCallback bitmap_callback,
     EncodedImageList images,
     int pixel_size,
-    std::unique_ptr<service_manager::Connector> connector) {
+    data_decoder::mojom::DataDecoderService* service_override) {
   auto job = std::make_unique<Job>(
-      job_type, std::move(job_finished_callback), std::move(bitmap_callback),
-      std::move(images), pixel_size, std::move(connector));
+      this, job_type, std::move(job_finished_callback),
+      std::move(bitmap_callback), std::move(images), pixel_size);
   id_to_job_[last_used_job_id_] = std::move(job);
-  id_to_job_[last_used_job_id_]->Start();
+  id_to_job_[last_used_job_id_]->Start(service_override);
 }
 
 void ImageHelper::OnJobFinished(int job_id) {
@@ -301,7 +310,7 @@ void ImageHelper::OnJobFinished(int job_id) {
 void ImageHelper::ComposeSiteImage(
     BitmapCallback callback,
     EncodedImageList images,
-    std::unique_ptr<service_manager::Connector> connector) {
+    data_decoder::mojom::DataDecoderService* service_override) {
   DVLOG(1) << "Requested decoding for site image";
   if (images.size() == 0) {
     std::move(callback).Run(nullptr);
@@ -311,14 +320,14 @@ void ImageHelper::ComposeSiteImage(
   NewJob(ImageJobType::kSiteIcon,
          base::BindOnce(&ImageHelper::OnJobFinished, weak_factory_.GetWeakPtr(),
                         ++last_used_job_id_),
-         std::move(callback), std::move(images), -1, std::move(connector));
+         std::move(callback), std::move(images), -1, service_override);
 }
 
 void ImageHelper::ComposeCategoryImage(
     BitmapCallback callback,
     int pixel_size,
     EncodedImageList images,
-    std::unique_ptr<service_manager::Connector> connector) {
+    data_decoder::mojom::DataDecoderService* service_override) {
   DVLOG(1) << "Requested decoding " << images.size()
            << " images for category image";
 
@@ -330,7 +339,17 @@ void ImageHelper::ComposeCategoryImage(
   NewJob(ImageJobType::kCategoryImage,
          base::BindOnce(&ImageHelper::OnJobFinished, weak_factory_.GetWeakPtr(),
                         ++last_used_job_id_),
-         std::move(callback), std::move(images), pixel_size,
-         std::move(connector));
+         std::move(callback), std::move(images), pixel_size, service_override);
 }
+
+data_decoder::mojom::DataDecoderService* ImageHelper::GetDataDecoder() {
+  if (!data_decoder_) {
+    data_decoder_ = content::LaunchDataDecoder();
+    data_decoder_.reset_on_disconnect();
+    data_decoder_.reset_on_idle_timeout(kDataDecoderInstanceTimeout);
+  }
+
+  return data_decoder_.get();
+}
+
 }  // namespace explore_sites

chrome/browser/android/explore_sites/image_helper.h

@@ -11,7 +11,8 @@
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/android/explore_sites/explore_sites_types.h"
-#include "services/service_manager/public/cpp/connector.h"
+#include "mojo/public/cpp/bindings/remote.h"
+#include "services/data_decoder/public/mojom/data_decoder_service.mojom.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 
 namespace explore_sites {
@@ -31,7 +32,7 @@ class ImageHelper {
   void ComposeSiteImage(
       BitmapCallback callback,
       EncodedImageList images,
-      std::unique_ptr<service_manager::Connector> connector = nullptr);
+      data_decoder::mojom::DataDecoderService* service_override = nullptr);
 
   // Compose a category icon containing [1 - 4] site icons and return via
   // |callback|.
@@ -39,7 +40,7 @@ class ImageHelper {
       BitmapCallback callback,
       int pixel_size,
       EncodedImageList images,
-      std::unique_ptr<service_manager::Connector> connector = nullptr);
+      data_decoder::mojom::DataDecoderService* service_override = nullptr);
 
  private:
   class Job;
@@ -49,10 +50,13 @@ class ImageHelper {
               BitmapCallback bitmap_callback,
               EncodedImageList images,
               int pixel_size,
-              std::unique_ptr<service_manager::Connector> connector);
+              data_decoder::mojom::DataDecoderService* service_override);
 
   void OnJobFinished(int job_id);
 
+  data_decoder::mojom::DataDecoderService* GetDataDecoder();
+
+  mojo::Remote<data_decoder::mojom::DataDecoderService> data_decoder_;
   std::map<int, std::unique_ptr<Job>> id_to_job_;
   int last_used_job_id_;
 

chrome/browser/android/explore_sites/image_helper_unittest.cc

@@ -13,8 +13,6 @@
 #include "base/test/task_environment.h"
 #include "base/test/test_simple_task_runner.h"
 #include "services/data_decoder/public/cpp/test_data_decoder_service.h"
-#include "services/data_decoder/public/mojom/constants.mojom.h"
-#include "services/service_manager/public/cpp/test/test_connector_factory.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "third_party/skia/include/core/SkColor.h"
@@ -51,11 +49,8 @@ const int kUpperBoundCorner = 26;
 
 class ExploreSitesImageHelperTest : public testing::Test {
  public:
-  ExploreSitesImageHelperTest()
-      : data_decoder_(connector_factory_.RegisterInstance(
-            data_decoder::mojom::kServiceName)) {}
-
-  ~ExploreSitesImageHelperTest() override {}
+  ExploreSitesImageHelperTest() = default;
+  ~ExploreSitesImageHelperTest() override = default;
 
   EncodedImageList GetEncodedImageList(int num_icons);
   BitmapCallback StoreBitmap() {
@@ -68,14 +63,11 @@ class ExploreSitesImageHelperTest : public testing::Test {
 
   std::vector<std::unique_ptr<SkBitmap>> last_bitmap_list;
 
+  data_decoder::DataDecoderService* data_decoder() { return &data_decoder_; }
+
  protected:
   base::test::TaskEnvironment task_environment_;
 
-  std::unique_ptr<service_manager::Connector> GetConnector() {
-    return connector_factory_.CreateConnector();
-  }
-
-  service_manager::TestConnectorFactory connector_factory_;
   data_decoder::DataDecoderService data_decoder_;
   base::HistogramTester histogram_tester_;
 };
@@ -95,7 +87,7 @@ TEST_F(ExploreSitesImageHelperTest, TestImageHelper_SiteIcon) {
   ImageHelper image_helper;
 
   image_helper.ComposeSiteImage(StoreBitmap(), GetEncodedImageList(1),
-                                GetConnector());
+                                data_decoder());
 
   task_environment_.RunUntilIdle();
 
@@ -110,14 +102,14 @@ TEST_F(ExploreSitesImageHelperTest, TestImageHelper_SiteIcon) {
 TEST_F(ExploreSitesImageHelperTest, TestImageHelper_SiteIcon_MultipleCalls) {
   ImageHelper image_helper;
   image_helper.ComposeSiteImage(StoreBitmap(), GetEncodedImageList(1),
-                                GetConnector());
+                                data_decoder());
   task_environment_.RunUntilIdle();
 
   ASSERT_NE(nullptr, last_bitmap_list[0]);
   EXPECT_FALSE(last_bitmap_list[0]->isNull());
 
   image_helper.ComposeSiteImage(StoreBitmap(), GetEncodedImageList(1),
-                                GetConnector());
+                                data_decoder());
   task_environment_.RunUntilIdle();
 
   ASSERT_NE(nullptr, last_bitmap_list[1]);
@@ -128,9 +120,9 @@ TEST_F(ExploreSitesImageHelperTest, TestImageHelper_SiteIcon_MultipleCalls) {
 TEST_F(ExploreSitesImageHelperTest, TestImageHelper_SiteIcon_ConcurrentCalls) {
   ImageHelper image_helper;
   image_helper.ComposeSiteImage(StoreBitmap(), GetEncodedImageList(1),
-                                GetConnector());
+                                data_decoder());
   image_helper.ComposeSiteImage(StoreBitmap(), GetEncodedImageList(1),
-                                GetConnector());
+                                data_decoder());
   task_environment_.RunUntilIdle();
 
   ASSERT_NE(nullptr, last_bitmap_list[0]);
@@ -143,7 +135,7 @@ TEST_F(ExploreSitesImageHelperTest, TestImageHelper_SiteIcon_ConcurrentCalls) {
 TEST_F(ExploreSitesImageHelperTest, TestImageHelper_CategoryImage_One) {
   ImageHelper image_helper;
   image_helper.ComposeCategoryImage(StoreBitmap(), kIconSize,
-                                    GetEncodedImageList(1), GetConnector());
+                                    GetEncodedImageList(1), data_decoder());
   task_environment_.RunUntilIdle();
 
   ASSERT_NE(nullptr, last_bitmap_list[0]);
@@ -170,7 +162,7 @@ TEST_F(ExploreSitesImageHelperTest, TestImageHelper_CategoryImage_One) {
 TEST_F(ExploreSitesImageHelperTest, TestImageHelper_CategoryImage_Two) {
   ImageHelper image_helper;
   image_helper.ComposeCategoryImage(StoreBitmap(), kIconSize,
-                                    GetEncodedImageList(2), GetConnector());
+                                    GetEncodedImageList(2), data_decoder());
   task_environment_.RunUntilIdle();
 
   ASSERT_NE(nullptr, last_bitmap_list[0]);
@@ -196,7 +188,7 @@ TEST_F(ExploreSitesImageHelperTest, TestImageHelper_CategoryImage_Two) {
 TEST_F(ExploreSitesImageHelperTest, TestImageHelper_CategoryImage_Three) {
   ImageHelper image_helper;
   image_helper.ComposeCategoryImage(StoreBitmap(), kIconSize,
-                                    GetEncodedImageList(3), GetConnector());
+                                    GetEncodedImageList(3), data_decoder());
   task_environment_.RunUntilIdle();
 
   ASSERT_NE(nullptr, last_bitmap_list[0]);
@@ -225,7 +217,7 @@ TEST_F(ExploreSitesImageHelperTest, TestImageHelper_CategoryImage_Three) {
 TEST_F(ExploreSitesImageHelperTest, TestImageHelper_CategoryImage_Four) {
   ImageHelper image_helper;
   image_helper.ComposeCategoryImage(StoreBitmap(), kIconSize,
-                                    GetEncodedImageList(4), GetConnector());
+                                    GetEncodedImageList(4), data_decoder());
   task_environment_.RunUntilIdle();
 
   ASSERT_NE(nullptr, last_bitmap_list[0]);
@@ -265,7 +257,7 @@ TEST_F(ExploreSitesImageHelperTest, TestImageHelper_CategoryImage_InvalidWebP) {
       image_list.push_back(std::make_unique<EncodedImageBytes>(kWebpBytes));
     }
     image_helper.ComposeCategoryImage(StoreBitmap(), kIconSize,
-                                      std::move(image_list), GetConnector());
+                                      std::move(image_list), data_decoder());
 
     task_environment_.RunUntilIdle();
 
@@ -284,7 +276,7 @@ TEST_F(ExploreSitesImageHelperTest, TestImageHelper_ImageDecodedUMA) {
 
   // Record one success UMA from CompseSiteImage.
   image_helper.ComposeSiteImage(StoreBitmap(), GetEncodedImageList(1),
-                                GetConnector());
+                                data_decoder());
   task_environment_.RunUntilIdle();
 
   histograms().ExpectTotalCount("ExploreSites.ImageDecoded", 1);
@@ -294,7 +286,7 @@ TEST_F(ExploreSitesImageHelperTest, TestImageHelper_ImageDecodedUMA) {
   EncodedImageList image_list;
   image_list.push_back(std::make_unique<EncodedImageBytes>(kInvalidWebpBytes));
   image_helper.ComposeSiteImage(StoreBitmap(), std::move(image_list),
-                                GetConnector());
+                                data_decoder());
   task_environment_.RunUntilIdle();
 
   histograms().ExpectTotalCount("ExploreSites.ImageDecoded", 2);
@@ -302,7 +294,7 @@ TEST_F(ExploreSitesImageHelperTest, TestImageHelper_ImageDecodedUMA) {
 
   // Record 2 samples from ComposeCategoryImage.
   image_helper.ComposeCategoryImage(StoreBitmap(), kIconSize,
-                                    GetEncodedImageList(2), GetConnector());
+                                    GetEncodedImageList(2), data_decoder());
   task_environment_.RunUntilIdle();
 
   histograms().ExpectTotalCount("ExploreSites.ImageDecoded", 4);