Move encoding SkBitmaps to PNG out of FaviconService::SetFavicons() and onto

the history thread.

BUG=402104
TEST=None

Review URL: https://codereview.chromium.org/531493002

Cr-Commit-Position: refs/heads/master@{#293240}

chrome/browser/favicon/favicon_service.cc

@@ -311,32 +311,18 @@ void FaviconService::SetFavicons(const GURL& page_url,
   gfx::ImageSkia image_skia = image.AsImageSkia();
   image_skia.EnsureRepsForSupportedScales();
   const std::vector<gfx::ImageSkiaRep>& image_reps = image_skia.image_reps();
-  std::vector<favicon_base::FaviconRawBitmapData> favicon_bitmap_data;
+  std::vector<SkBitmap> bitmaps;
   const std::vector<float> favicon_scales = favicon_base::GetFaviconScales();
   for (size_t i = 0; i < image_reps.size(); ++i) {
-    // Don't save if the scale isn't one of supported favicon scale.
+    // Don't save if the scale isn't one of supported favicon scales.
     if (std::find(favicon_scales.begin(),
                   favicon_scales.end(),
                   image_reps[i].scale()) == favicon_scales.end()) {
       continue;
     }
-
-    scoped_refptr<base::RefCountedBytes> bitmap_data(
-        new base::RefCountedBytes());
-    if (gfx::PNGCodec::EncodeBGRASkBitmap(image_reps[i].sk_bitmap(),
-                                          false,
-                                          &bitmap_data->data())) {
-      gfx::Size pixel_size(image_reps[i].pixel_width(),
-                           image_reps[i].pixel_height());
-      favicon_base::FaviconRawBitmapData bitmap_data_element;
-      bitmap_data_element.bitmap_data = bitmap_data;
-      bitmap_data_element.pixel_size = pixel_size;
-      bitmap_data_element.icon_url = icon_url;
-
-      favicon_bitmap_data.push_back(bitmap_data_element);
-    }
+    bitmaps.push_back(image_reps[i].sk_bitmap());
   }
-  history_service_->SetFavicons(page_url, icon_type, favicon_bitmap_data);
+  history_service_->SetFavicons(page_url, icon_type, icon_url, bitmaps);
 }
 
 void FaviconService::UnableToDownloadFavicon(const GURL& icon_url) {

chrome/browser/history/android/android_provider_backend_unittest.cc

@@ -31,6 +31,7 @@
 #include "content/public/test/test_browser_thread.h"
 #include "content/public/test/test_utils.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/skia/include/core/SkBitmap.h"
 
 using base::Time;
 using base::TimeDelta;
@@ -55,6 +56,14 @@ struct BookmarkCacheRow {
   favicon_base::FaviconID favicon_id_;
 };
 
+// Creates a 16x16 bitmap.
+SkBitmap CreateBitmap() {
+  SkBitmap bitmap;
+  bitmap.allocN32Pixels(16, 16);
+  bitmap.eraseColor(SK_ColorBLUE);
+  return bitmap;
+}
+
 }  // namespace
 
 class AndroidProviderBackendDelegate : public HistoryBackend::Delegate {
@@ -256,17 +265,8 @@ TEST_F(AndroidProviderBackendTest, UpdateTables) {
   url_id2 = url_row.id();
 
   // Set favicon to url2.
-  std::vector<unsigned char> data;
-  data.push_back('1');
-  favicon_base::FaviconRawBitmapData bitmap_data_element;
-  bitmap_data_element.bitmap_data = new base::RefCountedBytes(data);
-  bitmap_data_element.pixel_size = gfx::Size();
-  bitmap_data_element.icon_url = GURL();
-  std::vector<favicon_base::FaviconRawBitmapData> favicon_bitmap_data;
-  favicon_bitmap_data.push_back(bitmap_data_element);
-
-  history_backend->SetFavicons(
-      url2, favicon_base::FAVICON, favicon_bitmap_data);
+  std::vector<SkBitmap> bitmaps(1u, CreateBitmap());
+  history_backend->SetFavicons(url2, favicon_base::FAVICON, GURL(), bitmaps);
   history_backend->Closing();
   }
 
@@ -403,17 +403,8 @@ TEST_F(AndroidProviderBackendTest, QueryHistoryAndBookmarks) {
   ASSERT_EQ(2u, history_backend->UpdateURLs(url_rows));
 
   // Set favicon to url2.
-  std::vector<unsigned char> data;
-  data.push_back('1');
-  favicon_base::FaviconRawBitmapData bitmap_data_element;
-  bitmap_data_element.bitmap_data = new base::RefCountedBytes(data);
-  bitmap_data_element.pixel_size = gfx::Size();
-  bitmap_data_element.icon_url = GURL();
-  std::vector<favicon_base::FaviconRawBitmapData> favicon_bitmap_data;
-  favicon_bitmap_data.push_back(bitmap_data_element);
-
-  history_backend->SetFavicons(
-      url2, favicon_base::FAVICON, favicon_bitmap_data);
+  std::vector<SkBitmap> bitmaps(1u, CreateBitmap());
+  history_backend->SetFavicons(url2, favicon_base::FAVICON, GURL(), bitmaps);
   history_backend->Closing();
   }
 
@@ -1849,17 +1840,8 @@ TEST_F(AndroidProviderBackendTest, QueryWithoutThumbnailDB) {
   ASSERT_EQ(2u, history_backend->UpdateURLs(url_rows));
 
   // Set favicon to url2.
-  std::vector<unsigned char> data;
-  data.push_back('1');
-  favicon_base::FaviconRawBitmapData bitmap_data_element;
-  bitmap_data_element.bitmap_data = new base::RefCountedBytes(data);
-  bitmap_data_element.pixel_size = gfx::Size();
-  bitmap_data_element.icon_url = GURL();
-  std::vector<favicon_base::FaviconRawBitmapData> favicon_bitmap_data;
-  favicon_bitmap_data.push_back(bitmap_data_element);
-
-  history_backend->SetFavicons(
-      url2, favicon_base::FAVICON, favicon_bitmap_data);
+  std::vector<SkBitmap> bitmaps(1u, CreateBitmap());
+  history_backend->SetFavicons(url2, favicon_base::FAVICON, GURL(), bitmaps);
   history_backend->Closing();
   }
 

chrome/browser/history/history_backend.cc

@@ -44,6 +44,8 @@
 #include "components/history/core/browser/page_usage_data.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #include "sql/error_delegate_util.h"
+#include "third_party/skia/include/core/SkBitmap.h"
+#include "ui/gfx/codec/png_codec.h"
 #include "url/gurl.h"
 
 #if defined(OS_ANDROID)
@@ -1774,50 +1776,30 @@ void HistoryBackend::MergeFavicon(
   ScheduleCommit();
 }
 
-void HistoryBackend::SetFavicons(
-    const GURL& page_url,
-    favicon_base::IconType icon_type,
-    const std::vector<favicon_base::FaviconRawBitmapData>&
-        favicon_bitmap_data) {
+void HistoryBackend::SetFavicons(const GURL& page_url,
+                                 favicon_base::IconType icon_type,
+                                 const GURL& icon_url,
+                                 const std::vector<SkBitmap>& bitmaps) {
   if (!thumbnail_db_ || !db_)
     return;
 
-  DCHECK(ValidateSetFaviconsParams(favicon_bitmap_data));
-
-  // Build map of FaviconRawBitmapData for each icon url.
-  typedef std::map<GURL, std::vector<favicon_base::FaviconRawBitmapData> >
-      BitmapDataByIconURL;
-  BitmapDataByIconURL grouped_by_icon_url;
-  for (size_t i = 0; i < favicon_bitmap_data.size(); ++i) {
-    const GURL& icon_url = favicon_bitmap_data[i].icon_url;
-    grouped_by_icon_url[icon_url].push_back(favicon_bitmap_data[i]);
-  }
+  DCHECK_GE(kMaxFaviconBitmapsPerIconURL, bitmaps.size());
 
   // Track whether the method modifies or creates any favicon bitmaps, favicons
   // or icon mappings.
   bool data_modified = false;
 
-  std::vector<favicon_base::FaviconID> icon_ids;
-  for (BitmapDataByIconURL::const_iterator it = grouped_by_icon_url.begin();
-       it != grouped_by_icon_url.end(); ++it) {
-    const GURL& icon_url = it->first;
-    favicon_base::FaviconID icon_id =
-        thumbnail_db_->GetFaviconIDForFaviconURL(icon_url, icon_type, NULL);
-
-    if (!icon_id) {
-      // TODO(pkotwicz): Remove the favicon sizes attribute from
-      // ThumbnailDatabase::AddFavicon().
-      icon_id = thumbnail_db_->AddFavicon(icon_url, icon_type);
-      data_modified = true;
-    }
-    icon_ids.push_back(icon_id);
+  favicon_base::FaviconID icon_id =
+      thumbnail_db_->GetFaviconIDForFaviconURL(icon_url, icon_type, NULL);
 
-    if (!data_modified)
-      SetFaviconBitmaps(icon_id, it->second, &data_modified);
-    else
-      SetFaviconBitmaps(icon_id, it->second, NULL);
+  if (!icon_id) {
+    icon_id = thumbnail_db_->AddFavicon(icon_url, icon_type);
+    data_modified = true;
   }
 
+  data_modified |= SetFaviconBitmaps(icon_id, bitmaps);
+
+  std::vector<favicon_base::FaviconID> icon_ids(1u, icon_id);
   data_modified |=
     SetFaviconMappingsForPageAndRedirects(page_url, icon_type, icon_ids);
 
@@ -1984,27 +1966,33 @@ void HistoryBackend::UpdateFaviconMappingsAndFetchImpl(
       bitmap_results);
 }
 
-void HistoryBackend::SetFaviconBitmaps(
-    favicon_base::FaviconID icon_id,
-    const std::vector<favicon_base::FaviconRawBitmapData>& favicon_bitmap_data,
-    bool* favicon_bitmaps_changed) {
-  if (favicon_bitmaps_changed)
-    *favicon_bitmaps_changed = false;
-
+bool HistoryBackend::SetFaviconBitmaps(favicon_base::FaviconID icon_id,
+                                       const std::vector<SkBitmap>& bitmaps) {
   std::vector<FaviconBitmapIDSize> bitmap_id_sizes;
   thumbnail_db_->GetFaviconBitmapIDSizes(icon_id, &bitmap_id_sizes);
 
-  std::vector<favicon_base::FaviconRawBitmapData> to_add = favicon_bitmap_data;
+  typedef std::pair<scoped_refptr<base::RefCountedBytes>, gfx::Size>
+      PNGEncodedBitmap;
+  std::vector<PNGEncodedBitmap> to_add;
+  for (size_t i = 0; i < bitmaps.size(); ++i) {
+    scoped_refptr<base::RefCountedBytes> bitmap_data(
+        new base::RefCountedBytes);
+    if (!gfx::PNGCodec::EncodeBGRASkBitmap(
+            bitmaps[i], false, &bitmap_data->data())) {
+      continue;
+    }
+    to_add.push_back(std::make_pair(
+        bitmap_data, gfx::Size(bitmaps[i].width(), bitmaps[i].height())));
+  }
 
+  bool favicon_bitmaps_changed = false;
   for (size_t i = 0; i < bitmap_id_sizes.size(); ++i) {
     const gfx::Size& pixel_size = bitmap_id_sizes[i].pixel_size;
-    std::vector<favicon_base::FaviconRawBitmapData>::iterator match_it =
-        to_add.end();
-    for (std::vector<favicon_base::FaviconRawBitmapData>::iterator it =
-             to_add.begin();
+    std::vector<PNGEncodedBitmap>::iterator match_it = to_add.end();
+    for (std::vector<PNGEncodedBitmap>::iterator it = to_add.begin();
          it != to_add.end();
          ++it) {
-      if (it->pixel_size == pixel_size) {
+      if (it->second == pixel_size) {
         match_it = it;
         break;
       }
@@ -2014,58 +2002,28 @@ void HistoryBackend::SetFaviconBitmaps(
     if (match_it == to_add.end()) {
       thumbnail_db_->DeleteFaviconBitmap(bitmap_id);
 
-      if (favicon_bitmaps_changed)
-        *favicon_bitmaps_changed = true;
+      favicon_bitmaps_changed = true;
     } else {
-      if (favicon_bitmaps_changed &&
-          !*favicon_bitmaps_changed &&
-          IsFaviconBitmapDataEqual(bitmap_id, match_it->bitmap_data)) {
+      if (!favicon_bitmaps_changed &&
+          IsFaviconBitmapDataEqual(bitmap_id, match_it->first)) {
         thumbnail_db_->SetFaviconBitmapLastUpdateTime(
             bitmap_id, base::Time::Now());
       } else {
-        thumbnail_db_->SetFaviconBitmap(bitmap_id, match_it->bitmap_data,
+        thumbnail_db_->SetFaviconBitmap(bitmap_id, match_it->first,
             base::Time::Now());
-
-        if (favicon_bitmaps_changed)
-          *favicon_bitmaps_changed = true;
+        favicon_bitmaps_changed = true;
       }
       to_add.erase(match_it);
     }
   }
 
   for (size_t i = 0; i < to_add.size(); ++i) {
-    thumbnail_db_->AddFaviconBitmap(icon_id, to_add[i].bitmap_data,
-        base::Time::Now(), to_add[i].pixel_size);
+    thumbnail_db_->AddFaviconBitmap(icon_id, to_add[i].first,
+        base::Time::Now(), to_add[i].second);
 
-    if (favicon_bitmaps_changed)
-      *favicon_bitmaps_changed = true;
+    favicon_bitmaps_changed = true;
   }
-}
-
-bool HistoryBackend::ValidateSetFaviconsParams(const std::vector<
-    favicon_base::FaviconRawBitmapData>& favicon_bitmap_data) const {
-  typedef std::map<GURL, size_t> BitmapsPerIconURL;
-  BitmapsPerIconURL num_bitmaps_per_icon_url;
-  for (size_t i = 0; i < favicon_bitmap_data.size(); ++i) {
-    if (!favicon_bitmap_data[i].bitmap_data.get())
-      return false;
-
-    const GURL& icon_url = favicon_bitmap_data[i].icon_url;
-    if (!num_bitmaps_per_icon_url.count(icon_url))
-      num_bitmaps_per_icon_url[icon_url] = 1u;
-    else
-      ++num_bitmaps_per_icon_url[icon_url];
-  }
-
-  if (num_bitmaps_per_icon_url.size() > kMaxFaviconsPerPage)
-    return false;
-
-  for (BitmapsPerIconURL::const_iterator it = num_bitmaps_per_icon_url.begin();
-       it != num_bitmaps_per_icon_url.end(); ++it) {
-    if (it->second > kMaxFaviconBitmapsPerIconURL)
-      return false;
-  }
-  return true;
+  return favicon_bitmaps_changed;
 }
 
 bool HistoryBackend::IsFaviconBitmapDataEqual(

chrome/browser/history/history_backend.h

@@ -33,6 +33,7 @@
 class HistoryURLProvider;
 struct HistoryURLProviderParams;
 struct ImportedFaviconUsage;
+class SkBitmap;
 class TestingProfile;
 struct ThumbnailScore;
 
@@ -266,8 +267,8 @@ class HistoryBackend : public base::RefCountedThreadSafe<HistoryBackend>,
 
   void SetFavicons(const GURL& page_url,
                    favicon_base::IconType icon_type,
-                   const std::vector<favicon_base::FaviconRawBitmapData>&
-                       favicon_bitmap_data);
+                   const GURL& icon_url,
+                   const std::vector<SkBitmap>& bitmaps);
 
   void SetFaviconsOutOfDateForPage(const GURL& page_url);
 
@@ -546,7 +547,6 @@ class HistoryBackend : public base::RefCountedThreadSafe<HistoryBackend>,
                            GetFaviconsFromDBNoFaviconBitmaps);
   FRIEND_TEST_ALL_PREFIXES(HistoryBackendTest,
                            GetFaviconsFromDBSelectClosestMatch);
-  FRIEND_TEST_ALL_PREFIXES(HistoryBackendTest, GetFaviconsFromDBSingleIconURL);
   FRIEND_TEST_ALL_PREFIXES(HistoryBackendTest, GetFaviconsFromDBIconType);
   FRIEND_TEST_ALL_PREFIXES(HistoryBackendTest, GetFaviconsFromDBExpired);
   FRIEND_TEST_ALL_PREFIXES(HistoryBackendTest,
@@ -689,28 +689,15 @@ class HistoryBackend : public base::RefCountedThreadSafe<HistoryBackend>,
       std::vector<favicon_base::FaviconRawBitmapResult>* results);
 
   // Set the favicon bitmaps for |icon_id|.
-  // For each entry in |favicon_bitmap_data|, if a favicon bitmap already
-  // exists at the entry's pixel size, replace the favicon bitmap's data with
-  // the entry's bitmap data. Otherwise add a new favicon bitmap.
-  // Any favicon bitmaps already mapped to |icon_id| whose pixel sizes are not
-  // in |favicon_bitmap_data| are deleted.
-  // If not NULL, |favicon_bitmaps_changed| is set to whether any of the bitmap
-  // data at |icon_id| is changed as a result of calling this method.
-  // Computing |favicon_bitmaps_changed| requires additional database queries
-  // so should be avoided if unnecessary.
-  void SetFaviconBitmaps(favicon_base::FaviconID icon_id,
-                         const std::vector<favicon_base::FaviconRawBitmapData>&
-                             favicon_bitmap_data,
-                         bool* favicon_bitmaps_changed);
-
-  // Returns true if |favicon_bitmap_data| passed to SetFavicons() is valid.
-  // Criteria:
-  // 1) |favicon_bitmap_data| contains no more than
-  //      kMaxFaviconsPerPage unique icon URLs.
-  //      kMaxFaviconBitmapsPerIconURL favicon bitmaps for each icon URL.
-  // 2) FaviconRawBitmapData::bitmap_data contains non NULL bitmap data.
-  bool ValidateSetFaviconsParams(const std::vector<
-      favicon_base::FaviconRawBitmapData>& favicon_bitmap_data) const;
+  // For each entry in |bitmaps|, if a favicon bitmap already exists at the
+  // entry's pixel size, replace the favicon bitmap's data with the entry's
+  // bitmap data. Otherwise add a new favicon bitmap.
+  // Any favicon bitmaps already mapped to |icon_id| whose pixel size does not
+  // match the pixel size of one of |bitmaps| is deleted.
+  // Returns true if any of the bitmap data at |icon_id| is changed as a result
+  // of calling this method.
+  bool SetFaviconBitmaps(favicon_base::FaviconID icon_id,
+                         const std::vector<SkBitmap>& bitmaps);
 
   // Returns true if the bitmap data at |bitmap_id| equals |new_bitmap_data|.
   bool IsFaviconBitmapDataEqual(

chrome/browser/history/history_service.cc

@@ -620,15 +620,15 @@ void HistoryService::MergeFavicon(
 void HistoryService::SetFavicons(
     const GURL& page_url,
     favicon_base::IconType icon_type,
-    const std::vector<favicon_base::FaviconRawBitmapData>&
-        favicon_bitmap_data) {
+    const GURL& icon_url,
+    const std::vector<SkBitmap>& bitmaps) {
   DCHECK(thread_) << "History service being called after cleanup";
   DCHECK(thread_checker_.CalledOnValidThread());
   if (!CanAddURL(page_url))
     return;
 
   ScheduleAndForget(PRIORITY_NORMAL, &HistoryBackend::SetFavicons, page_url,
-      icon_type, favicon_bitmap_data);
+      icon_type, icon_url, bitmaps);
 }
 
 void HistoryService::SetFaviconsOutOfDateForPage(const GURL& page_url) {

chrome/browser/history/history_service.h

@@ -45,6 +45,7 @@ class PageUsageData;
 class PageUsageRequest;
 class Profile;
 struct ImportedFaviconUsage;
+class SkBitmap;
 
 namespace base {
 class FilePath;
@@ -696,21 +697,16 @@ class HistoryService : public content::NotificationObserver,
                     scoped_refptr<base::RefCountedMemory> bitmap_data,
                     const gfx::Size& pixel_size);
 
-  // Used by the FaviconService to set the favicons for a page on the history
-  // backend.
-  // |favicon_bitmap_data| replaces all the favicon bitmaps mapped to
-  // |page_url|.
-  // |expired| and |icon_type| fields in FaviconBitmapData are ignored.
-  // Use MergeFavicon() if |favicon_bitmap_data| is incomplete, and favicon
-  // bitmaps in the database should be preserved if possible. For instance,
-  // favicon bitmaps from sync are 1x only. MergeFavicon() is used to avoid
-  // deleting the 2x favicon bitmap if it is present in the history backend.
-  // See HistoryBackend::ValidateSetFaviconsParams() for more details on the
-  // criteria for |favicon_bitmap_data| to be valid.
+  // Used by the FaviconService to replace all of the favicon bitmaps mapped to
+  // |page_url| for |icon_type|.
+  // Use MergeFavicon() if |bitmaps| is incomplete, and favicon bitmaps in the
+  // database should be preserved if possible. For instance, favicon bitmaps
+  // from sync are 1x only. MergeFavicon() is used to avoid deleting the 2x
+  // favicon bitmap if it is present in the history backend.
   void SetFavicons(const GURL& page_url,
                    favicon_base::IconType icon_type,
-                   const std::vector<favicon_base::FaviconRawBitmapData>&
-                       favicon_bitmap_data);
+                   const GURL& icon_url,
+                   const std::vector<SkBitmap>& bitmaps);
 
   // Used by the FaviconService to mark the favicon for the page as being out
   // of date.