Replace the usage of SkBitmap with gfx::Image in the suggestion service.

components/suggestions/image_manager.h

 // Copyright 2014 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 COMPONENTS_SUGGESTIONS_IMAGE_MANAGER_H_
 #define COMPONENTS_SUGGESTIONS_IMAGE_MANAGER_H_
 
 #include <map>
 #include <memory>
 #include <string>
 #include <vector>
 
 #include "base/callback.h"
 #include "base/containers/hash_tables.h"
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted_memory.h"
 #include "base/memory/weak_ptr.h"
 #include "base/task_runner.h"
 #include "base/threading/thread_checker.h"
 #include "components/image_fetcher/image_fetcher_delegate.h"
 #include "components/leveldb_proto/proto_database.h"
 #include "components/suggestions/proto/suggestions.pb.h"
 #include "ui/gfx/image/image_skia.h"

[Marc Treib, 2016/05/20 14:40:04]

Not your doing, but is this include needed?

[markusheintz_, 2016/05/23 13:20:47]

Replaced with an include for SkBitmap

This requires adding a new DEP. SkBitmap was pulled in indirectly by
ui/gfx/image/image_skia.h 

[Marc Treib, 2016/05/23 13:44:03]

Ah, okay. If this requires extra approvals for the new DEP, then I'm also fine
with leaving it as-is, since it'll soon go away anyway.

[markusheintz_, 2016/05/24 08:32:31]

Done.

 #include "url/gurl.h"
 
+namespace gfx {
+class Image;
+}
+
 namespace image_fetcher {
 class ImageFetcher;
 }
 
 namespace net {
 class URLRequestContextGetter;
 }
 
 namespace suggestions {
 
 class ImageData;
 class SuggestionsProfile;
 
 // A class used to fetch server images asynchronously and manage the caching
 // layer (both in memory and on disk).
 class ImageManager : public image_fetcher::ImageFetcherDelegate {

[noyau (Ping after 24h), 2016/05/20 15:15:59]

I'm lost in this code, but from what I read, this class caches the image on disk
by converting it to jpeg to save in the cache, and then converted back when
loaded. That's a lot of conversions, and lossy conversions to boot. I suggest
that this code should be revised to provide native encoding/decoding, with no
conversion from and to a bitmap…

[markusheintz_, 2016/05/23 13:20:46]

While I'm cleaning this up I'll try to identify such things. I'll put this on my
list. Marc and me synced about these things too. So expect more CLs.

Once the SkBitmap is gone from the API I'll start working on splitting t the
code for fetching and decoding. 

  public:
   typedef std::vector<ImageData> ImageDataVector;
 
   ImageManager(
       std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher,
       std::unique_ptr<leveldb_proto::ProtoDatabase<ImageData>> database,
       const base::FilePath& database_dir,
       scoped_refptr<base::TaskRunner> background_task_runner);
   ~ImageManager() override;
 
   virtual void Initialize(const SuggestionsProfile& suggestions);
 
   // Should be called from the UI thread.
   virtual void AddImageURL(const GURL& url, const GURL& image_url);
 
   // Should be called from the UI thread.
   virtual void GetImageForURL(
       const GURL& url,
-      base::Callback<void(const GURL&, const SkBitmap*)> callback);
+      base::Callback<void(const GURL&, const gfx::Image&)> callback);
 
  protected:
   // Methods inherited from image_fetcher::ImageFetcherDelegate
 
   // Perform additional tasks when an image has been fetched.
   void OnImageFetched(const GURL& url, const gfx::Image& image) override;
 
  private:
   friend class MockImageManager;
   friend class ImageManagerTest;
   FRIEND_TEST_ALL_PREFIXES(ImageManagerTest, InitializeTest);
   FRIEND_TEST_ALL_PREFIXES(ImageManagerTest, AddImageURL);
   FRIEND_TEST_ALL_PREFIXES(ImageManagerTest, GetImageForURLNetworkCacheHit);
   FRIEND_TEST_ALL_PREFIXES(ImageManagerTest,
                            GetImageForURLNetworkCacheNotInitialized);
 
   // Used for testing.
   ImageManager();
 
-  typedef std::vector<base::Callback<void(const GURL&, const SkBitmap*)> >
+  typedef std::vector<base::Callback<void(const GURL&, const gfx::Image&)> >
       CallbackVector;
   typedef base::hash_map<std::string, scoped_refptr<base::RefCountedMemory>>
       ImageMap;
 
   // State related to an image fetch (associated website url, image_url,
   // pending callbacks).
   struct ImageCacheRequest {
     ImageCacheRequest();
     ImageCacheRequest(const ImageCacheRequest& other);
     ~ImageCacheRequest();
 
     GURL url;
     GURL image_url;
     // Queue for pending callbacks, which may accumulate while the request is in
     // flight.
     CallbackVector callbacks;
   };
 
   typedef std::map<const GURL, ImageCacheRequest> ImageCacheRequestMap;
 
   // Looks up image URL for |url|. If found, writes the result to |image_url|
   // and returns true. Otherwise just returns false.
   bool GetImageURL(const GURL& url, GURL* image_url);
 
   void QueueCacheRequest(
       const GURL& url, const GURL& image_url,
-      base::Callback<void(const GURL&, const SkBitmap*)> callback);
+      base::Callback<void(const GURL&, const gfx::Image&)> callback);

[noyau (Ping after 24h), 2016/05/20 15:15:59]

You are using this callback type in 4 places just in this header. Maybe worth
giving it a name?

[markusheintz_, 2016/05/23 13:20:47]

Done

 
   void ServeFromCacheOrNetwork(
       const GURL& url, const GURL& image_url,
-      base::Callback<void(const GURL&, const SkBitmap*)> callback);
+      base::Callback<void(const GURL&, const gfx::Image&)> callback);
 
   void OnCacheImageDecoded(
       const GURL& url,
       const GURL& image_url,
-      base::Callback<void(const GURL&, const SkBitmap*)> callback,
+      const base::Callback<void(const GURL&, const gfx::Image&)>& callback,
       std::unique_ptr<SkBitmap> bitmap);
 
   // Returns null if the |url| had no entry in the cache.
   scoped_refptr<base::RefCountedMemory> GetEncodedImageFromCache(
       const GURL& url);
 
   // Save the image bitmap in the cache and in the database.
   void SaveImage(const GURL& url, const SkBitmap& bitmap);
 
   // Database callback methods.
@@ skipping 30 matching lines @@
   base::ThreadChecker thread_checker_;
 
   base::WeakPtrFactory<ImageManager> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ImageManager);
 };
 
 }  // namespace suggestions
 
 #endif  // COMPONENTS_SUGGESTIONS_IMAGE_MANAGER_H_