Remove LogoDelegate

LogoDelegate was used as a mechanism for decoding untrusted logo images.
Now, all platforms use the same LogoDelegateImpl, based on
image_fetcher::ImageDecoder. LogoDelegateImpl should be removed,
in favor of using an image_fetcher::ImageDecoder directly.

Bug: 761828
Change-Id: If5734629a0c16a0a8867421c783b6d49fad2545c
Reviewed-on: https://chromium-review.googlesource.com/c/1290583
Commit-Queue: Zhuoyu Qian <zhuoyu.qian@samsung.com>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Reviewed-by: Chris Pickel <sfiera@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604121}

components/search_provider_logos/DEPS

@@ -10,5 +10,6 @@ include_rules = [
   "+services/network/public/mojom",
   "+services/network/test",
   "+third_party/skia/include/core/SkBitmap.h",
+  "+ui/gfx/geometry",
   "+ui/gfx/image",
 ]

components/search_provider_logos/logo_service_impl.cc

@@ -23,80 +23,12 @@
 #include "components/search_provider_logos/switches.h"
 #include "components/signin/core/browser/gaia_cookie_manager_service.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
-#include "ui/gfx/image/image.h"
 
-using search_provider_logos::LogoDelegate;
 using search_provider_logos::LogoTracker;
 
 namespace search_provider_logos {
 namespace {
 
-const int kDecodeLogoTimeoutSeconds = 30;
-
-// Implements a callback for image_fetcher::ImageDecoder. If Run() is called on
-// a callback returned by GetCallback() within 30 seconds, forwards the decoded
-// image to the wrapped callback. If not, sends an empty image to the wrapped
-// callback instead. Either way, deletes the object and prevents further calls.
-//
-// TODO(sfiera): find a more idiomatic way of setting a deadline on the
-// callback. This is implemented as a self-deleting object in part because it
-// needed to when it used to be a delegate and in part because I couldn't figure
-// out a better way, now that it isn't.
-class ImageDecodedHandlerWithTimeout {
- public:
-  static base::Callback<void(const gfx::Image&)> Wrap(
-      const base::Callback<void(const SkBitmap&)>& image_decoded_callback) {
-    auto* handler = new ImageDecodedHandlerWithTimeout(image_decoded_callback);
-    base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-        FROM_HERE,
-        base::BindOnce(&ImageDecodedHandlerWithTimeout::OnImageDecoded,
-                       handler->weak_ptr_factory_.GetWeakPtr(), gfx::Image()),
-        base::TimeDelta::FromSeconds(kDecodeLogoTimeoutSeconds));
-    return base::Bind(&ImageDecodedHandlerWithTimeout::OnImageDecoded,
-                      handler->weak_ptr_factory_.GetWeakPtr());
-  }
-
- private:
-  explicit ImageDecodedHandlerWithTimeout(
-      const base::Callback<void(const SkBitmap&)>& image_decoded_callback)
-      : image_decoded_callback_(image_decoded_callback),
-        weak_ptr_factory_(this) {}
-
-  void OnImageDecoded(const gfx::Image& decoded_image) {
-    image_decoded_callback_.Run(decoded_image.AsBitmap());
-    delete this;
-  }
-
-  base::Callback<void(const SkBitmap&)> image_decoded_callback_;
-  base::WeakPtrFactory<ImageDecodedHandlerWithTimeout> weak_ptr_factory_;
-
-  DISALLOW_COPY_AND_ASSIGN(ImageDecodedHandlerWithTimeout);
-};
-
-class LogoDelegateImpl : public search_provider_logos::LogoDelegate {
- public:
-  explicit LogoDelegateImpl(
-      std::unique_ptr<image_fetcher::ImageDecoder> image_decoder)
-      : image_decoder_(std::move(image_decoder)) {}
-
-  ~LogoDelegateImpl() override = default;
-
-  // search_provider_logos::LogoDelegate:
-  void DecodeUntrustedImage(
-      const scoped_refptr<base::RefCountedString>& encoded_image,
-      base::Callback<void(const SkBitmap&)> image_decoded_callback) override {
-    image_decoder_->DecodeImage(
-        encoded_image->data(),
-        gfx::Size(),  // No particular size desired.
-        ImageDecodedHandlerWithTimeout::Wrap(image_decoded_callback));
-  }
-
- private:
-  const std::unique_ptr<image_fetcher::ImageDecoder> image_decoder_;
-
-  DISALLOW_COPY_AND_ASSIGN(LogoDelegateImpl);
-};
-
 void ObserverOnLogoAvailable(LogoObserver* observer,
                              bool from_cache,
                              LogoCallbackReason type,
@@ -303,10 +235,9 @@ void LogoServiceImpl::InitializeLogoTrackerIfNecessary() {
     clock = base::DefaultClock::GetInstance();
   }
 
-  logo_tracker_ = std::make_unique<LogoTracker>(
-      url_loader_factory_,
-      std::make_unique<LogoDelegateImpl>(std::move(image_decoder_)),
-      std::move(logo_cache), clock);
+  logo_tracker_ = std::make_unique<LogoTracker>(url_loader_factory_,
+                                                std::move(image_decoder_),
+                                                std::move(logo_cache), clock);
 }
 
 void LogoServiceImpl::SigninStatusChanged() {

components/search_provider_logos/logo_tracker.cc

@@ -14,18 +14,62 @@
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/clock.h"
 #include "components/data_use_measurement/core/data_use_user_data.h"
+#include "components/image_fetcher/core/image_decoder.h"
 #include "net/http/http_response_headers.h"
 #include "net/http/http_status_code.h"
 #include "net/traffic_annotation/network_traffic_annotation.h"
 #include "services/network/public/cpp/resource_request.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "services/network/public/cpp/simple_url_loader.h"
+#include "ui/gfx/geometry/size.h"
+#include "ui/gfx/image/image.h"
 
 namespace search_provider_logos {
 
 namespace {
 
 const int64_t kMaxDownloadBytes = 1024 * 1024;
+const int kDecodeLogoTimeoutSeconds = 30;
+
+// Implements a callback for image_fetcher::ImageDecoder. If Run() is called on
+// a callback returned by GetCallback() within 30 seconds, forwards the decoded
+// image to the wrapped callback. If not, sends an empty image to the wrapped
+// callback instead. Either way, deletes the object and prevents further calls.
+//
+// TODO(sfiera): find a more idiomatic way of setting a deadline on the
+// callback. This is implemented as a self-deleting object in part because it
+// needed to when it used to be a delegate and in part because I couldn't figure
+// out a better way, now that it isn't.
+class ImageDecodedHandlerWithTimeout {
+ public:
+  static base::Callback<void(const gfx::Image&)> Wrap(
+      const base::Callback<void(const SkBitmap&)>& image_decoded_callback) {
+    auto* handler = new ImageDecodedHandlerWithTimeout(image_decoded_callback);
+    base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+        FROM_HERE,
+        base::BindOnce(&ImageDecodedHandlerWithTimeout::OnImageDecoded,
+                       handler->weak_ptr_factory_.GetWeakPtr(), gfx::Image()),
+        base::TimeDelta::FromSeconds(kDecodeLogoTimeoutSeconds));
+    return base::Bind(&ImageDecodedHandlerWithTimeout::OnImageDecoded,
+                      handler->weak_ptr_factory_.GetWeakPtr());
+  }
+
+ private:
+  explicit ImageDecodedHandlerWithTimeout(
+      const base::Callback<void(const SkBitmap&)>& image_decoded_callback)
+      : image_decoded_callback_(image_decoded_callback),
+        weak_ptr_factory_(this) {}
+
+  void OnImageDecoded(const gfx::Image& decoded_image) {
+    image_decoded_callback_.Run(decoded_image.AsBitmap());
+    delete this;
+  }
+
+  base::Callback<void(const SkBitmap&)> image_decoded_callback_;
+  base::WeakPtrFactory<ImageDecodedHandlerWithTimeout> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(ImageDecodedHandlerWithTimeout);
+};
 
 // Returns whether the metadata for the cached logo indicates that the logo is
 // OK to show, i.e. it's not expired or it's allowed to be shown temporarily
@@ -82,12 +126,12 @@ void NotifyAndClear(std::vector<EncodedLogoCallback>* encoded_callbacks,
 
 LogoTracker::LogoTracker(
     scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
-    std::unique_ptr<LogoDelegate> delegate,
+    std::unique_ptr<image_fetcher::ImageDecoder> image_decoder,
     std::unique_ptr<LogoCache> logo_cache,
     base::Clock* clock)
     : is_idle_(true),
       is_cached_logo_valid_(false),
-      logo_delegate_(std::move(delegate)),
+      image_decoder_(std::move(image_decoder)),
       cache_task_runner_(base::CreateSequencedTaskRunnerWithTraits(
           {base::MayBlock(), base::TaskPriority::USER_VISIBLE,
            base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN})),
@@ -194,10 +238,11 @@ void LogoTracker::OnCachedLogoRead(std::unique_ptr<EncodedLogo> cached_logo) {
     // logo to NULL.
     scoped_refptr<base::RefCountedString> encoded_image =
         cached_logo->encoded_image;
-    logo_delegate_->DecodeUntrustedImage(
-        encoded_image,
-        base::Bind(&LogoTracker::OnCachedLogoAvailable,
-                   weak_ptr_factory_.GetWeakPtr(), base::Passed(&cached_logo)));
+    image_decoder_->DecodeImage(
+        encoded_image->data(), gfx::Size(),  // No particular size desired.
+        ImageDecodedHandlerWithTimeout::Wrap(base::Bind(
+            &LogoTracker::OnCachedLogoAvailable, weak_ptr_factory_.GetWeakPtr(),
+            base::Passed(&cached_logo))));
   } else {
     OnCachedLogoAvailable({}, SkBitmap());
   }
@@ -299,12 +344,12 @@ void LogoTracker::OnFreshLogoParsed(bool* parsing_failed,
     // logo->encoded_image is evaulated before base::Passed(&logo), which sets
     // logo to NULL.
     scoped_refptr<base::RefCountedString> encoded_image = logo->encoded_image;
-    logo_delegate_->DecodeUntrustedImage(
-        encoded_image,
-        base::Bind(&LogoTracker::OnFreshLogoAvailable,
-                   weak_ptr_factory_.GetWeakPtr(), base::Passed(&logo),
-                   /*download_failed=*/false, *parsing_failed,
-                   from_http_cache));
+    image_decoder_->DecodeImage(
+        encoded_image->data(), gfx::Size(),  // No particular size desired.
+        ImageDecodedHandlerWithTimeout::Wrap(base::Bind(
+            &LogoTracker::OnFreshLogoAvailable, weak_ptr_factory_.GetWeakPtr(),
+            base::Passed(&logo), /*download_failed=*/false, *parsing_failed,
+            from_http_cache)));
   }
 }
 

components/search_provider_logos/logo_tracker.h

@@ -23,6 +23,10 @@
 #include "components/search_provider_logos/logo_common.h"
 #include "url/gurl.h"
 
+namespace image_fetcher {
+class ImageDecoder;
+}
+
 namespace network {
 class SimpleURLLoader;
 class SharedURLLoaderFactory;
@@ -51,21 +55,6 @@ class LogoObserver {
   virtual void OnObserverRemoved() = 0;
 };
 
-// Provides a LogoTracker with methods it needs to download and cache logos.
-class LogoDelegate {
- public:
-  virtual ~LogoDelegate() {}
-
-  // Decodes an untrusted image safely and returns it as an SkBitmap via
-  // |image_decoded_callback|. If image decoding fails, |image_decoded_callback|
-  // should be called with NULL. This will be called on the thread that
-  // LogoTracker lives on and |image_decoded_callback| must be called on the
-  // same thread.
-  virtual void DecodeUntrustedImage(
-      const scoped_refptr<base::RefCountedString>& encoded_image,
-      base::Callback<void(const SkBitmap&)> image_decoded_callback) = 0;
-};
-
 // This class provides the logo for a search provider. Logos are downloaded from
 // the search provider's logo URL and cached on disk.
 //
@@ -87,7 +76,7 @@ class LogoTracker {
   // the logo.
   explicit LogoTracker(
       scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
-      std::unique_ptr<LogoDelegate> delegate,
+      std::unique_ptr<image_fetcher::ImageDecoder> image_decoder,
       std::unique_ptr<LogoCache> logo_cache,
       base::Clock* clock);
 
@@ -213,7 +202,7 @@ class LogoTracker {
   std::vector<LogoCallback> on_fresh_decoded_logo_;
   std::vector<EncodedLogoCallback> on_fresh_encoded_logo_;
 
-  std::unique_ptr<LogoDelegate> logo_delegate_;
+  const std::unique_ptr<image_fetcher::ImageDecoder> image_decoder_;
 
   // The SequencedTaskRunner on which the cache lives.
   scoped_refptr<base::SequencedTaskRunner> cache_task_runner_;