Adds support for saving favicon size into history database.

chrome/browser/history/history_backend.cc

Index: chrome/browser/history/history_backend.cc
diff --git a/chrome/browser/history/history_backend.cc b/chrome/browser/history/history_backend.cc
index 6c6087e1960d07e76ea165c91a48d6484bfcc8be..4457388e2525a275063dd2ea9e9a8fb9476c5fed 100644
--- a/chrome/browser/history/history_backend.cc
+++ b/chrome/browser/history/history_backend.cc
@@ -94,6 +94,14 @@ static const int kMaxRedirectCount = 32;
 // and is archived.
 static const int kArchiveDaysThreshold = 90;
 
+// The maximum number of icons URLs per page which can be stored in the
+// thumbnail database.
+const size_t kMaxFaviconsPerPage = 8;
+
+// The maximum number of bitmaps for a single icon URL which can be stored in
+// the thumbnail database.
+const size_t kMaxFaviconBitmapsPerIconURL = 8;
+
 // Converts from PageUsageData to MostVisitedURL. |redirects| is a
 // list of redirects for this URL. Empty list means no redirects.
 MostVisitedURL MakeMostVisitedURL(const PageUsageData& page_data,
@@ -1771,21 +1779,86 @@ bool HistoryBackend::GetThumbnailFromOlderRedirect(
   return success;
 }
 
-void HistoryBackend::GetFavicon(scoped_refptr<GetFaviconRequest> request,
-                                const GURL& icon_url,
-                                int icon_types) {
-  UpdateFaviconMappingAndFetchImpl(NULL, icon_url, request, icon_types);
+void HistoryBackend::GetFaviconClosestToSize(
+    scoped_refptr<GetFaviconRequest> request,
+    const GURL& icon_url,
+    int icon_types,
+    const gfx::Size& pixel_size) {
+  std::vector<GURL> icon_urls;
+  icon_urls.push_back(icon_url);
+  UpdateFaviconMappingsAndFetchImpl(NULL, icon_urls, request, icon_types,
+                                    &pixel_size);
+}
+
+void HistoryBackend::GetFavicons(scoped_refptr<GetFaviconRequest> request,
+                                 const std::vector<GURL>& icon_urls,
+                                 int icon_types) {
+  UpdateFaviconMappingsAndFetchImpl(NULL, icon_urls, request, icon_types, NULL);
 }
 
-void HistoryBackend::UpdateFaviconMappingAndFetch(
+void HistoryBackend::GetFaviconForURLClosestToSize(
     scoped_refptr<GetFaviconRequest> request,
     const GURL& page_url,
-    const GURL& icon_url,
+    int icon_types,
+    const gfx::Size& pixel_size) {
+  if (request->canceled())
+    return;
+
+  FaviconData favicon_data;
+
+  // Get the favicon from DB.
+  GetFaviconsFromDB(page_url, icon_types, &pixel_size, &favicon_data);
+
+  request->ForwardResult(request->handle(), favicon_data);
+}
+
+void HistoryBackend::GetFaviconsForURL(
+    scoped_refptr<GetFaviconRequest> request,
+    const GURL& page_url,
+    int icon_types) {
+  if (request->canceled())
+    return;
+
+  FaviconData favicon_data;
+
+  // Get the favicons from DB.
+  GetFaviconsFromDB(page_url, icon_types, NULL, &favicon_data);
+
+  request->ForwardResult(request->handle(), favicon_data);
+}
+
+void HistoryBackend::UpdateFaviconMappingsAndFetch(
+    scoped_refptr<GetFaviconRequest> request,
+    const GURL& page_url,
+    const std::vector<GURL>& icon_urls,
     IconType icon_type) {
-  UpdateFaviconMappingAndFetchImpl(&page_url, icon_url, request, icon_type);
+  UpdateFaviconMappingsAndFetchImpl(&page_url, icon_urls, request, icon_type,
+                                    NULL);
 }
 
-void HistoryBackend::SetFaviconOutOfDateForPage(const GURL& page_url) {
+void HistoryBackend::AddFavicons(
+    const GURL& page_url,
+    IconType icon_type,
+    const std::vector<FaviconDataElement>& elements) {
+  FaviconData favicon_data;
+  GetFaviconsFromDB(page_url, icon_type, NULL, &favicon_data);
+
+  IconURLSizesMap icon_url_sizes;
+  if (favicon_data.known_icon) {
+    icon_url_sizes = favicon_data.icon_url_sizes;
+  } else {
+    // The thumbnail database doesn't know about |page_url|. Guess at what
+    // |icon_url_sizes| should be.
+    for (size_t i = 0; i < elements.size(); ++i) {
+      GURL icon_url = elements[i].icon_url;
+      icon_url_sizes[icon_url].InsertSize(elements[i].pixel_size);
+    }
+  }
+
+  SetFavicons(page_url, icon_type, elements, icon_url_sizes);
+}
+
+void HistoryBackend::SetFaviconsOutOfDateForPage(const GURL& page_url) {
   std::vector<IconMapping> icon_mappings;
 
   if (!thumbnail_db_.get() ||
@@ -1825,7 +1898,7 @@ void HistoryBackend::SetImportedFavicons(
 
   for (size_t i = 0; i < favicon_usage.size(); i++) {
     FaviconID favicon_id = thumbnail_db_->GetFaviconIDForFaviconURL(
-        favicon_usage[i].favicon_url, history::FAVICON, NULL);
+        favicon_usage[i].favicon_url, history::FAVICON);
     if (!favicon_id) {
       // This favicon doesn't exist yet, so we create it using the given data.
       // TODO(pkotwicz): Pass in real pixel size.
@@ -1860,7 +1933,7 @@ void HistoryBackend::SetImportedFavicons(
           favicons_changed.insert(*url);
         }
       } else {
-        if (!thumbnail_db_->GetIconMappingForPageURL(*url, FAVICON, NULL)) {
+        if (!thumbnail_db_->GetIconMappingsForPageURL(*url, FAVICON, NULL)) {
           // URL is present in history, update the favicon *only* if it is not
           // set already.
           thumbnail_db_->AddIconMapping(*url, favicon_id);
@@ -1879,11 +1952,12 @@ void HistoryBackend::SetImportedFavicons(
   }
 }
 
-void HistoryBackend::UpdateFaviconMappingAndFetchImpl(
+void HistoryBackend::UpdateFaviconMappingsAndFetchImpl(
     const GURL* page_url,
-    const GURL& icon_url,
+    const std::vector<GURL>& icon_urls,
     scoped_refptr<GetFaviconRequest> request,
-    int icon_types) {
+    int icon_types,
+    const gfx::Size* desired_pixel_size) {
   // Check only a single type was given when the page_url was specified.
   DCHECK(!page_url || (page_url && (icon_types == FAVICON ||
       icon_types == TOUCH_ICON || icon_types == TOUCH_PRECOMPOSED_ICON)));
@@ -1891,85 +1965,253 @@ void HistoryBackend::UpdateFaviconMappingAndFetchImpl(
   if (request->canceled())
     return;
 
-  FaviconData favicon;
+  FaviconData favicon_data;
 
+  IconURLFaviconIDMap icon_url_id_map;
   if (thumbnail_db_.get()) {
-    const FaviconID favicon_id =
-        thumbnail_db_->GetFaviconIDForFaviconURL(
-            icon_url, icon_types, &favicon.icon_type);
-    if (favicon_id) {
-      scoped_refptr<base::RefCountedMemory> data;
-      favicon.known_icon = true;
-      Time last_updated;
-      if (thumbnail_db_->GetFavicon(favicon_id, &last_updated, &data,
-                                    NULL, NULL)) {
-        favicon.expired = (Time::Now() - last_updated) >
-            TimeDelta::FromDays(kFaviconRefetchDays);
-        favicon.image_data = data;
+    for (size_t i = 0; i < icon_urls.size(); ++i) {
+      GURL icon_url = icon_urls[i];
+      const FaviconID favicon_id =
+          thumbnail_db_->GetFaviconIDForFaviconURL(icon_url, icon_types);
+      if (favicon_id) {
+        AddToFaviconData(favicon_id, desired_pixel_size, &favicon_data);
+        icon_url_id_map[icon_url] = favicon_id;
       }
-
-      if (page_url)
-        SetFaviconMapping(*page_url, favicon_id, favicon.icon_type);
+      // else case, haven't cached entry yet. Caller is responsible for
+      // downloading the favicon and invoking SetFavicons.
     }
-    // else case, haven't cached entry yet. Caller is responsible for
-    // downloading the favicon and invoking SetFavicon.
   }
-  request->ForwardResult(request->handle(), favicon);
+
+  if (page_url) {
+    SetFaviconMappingsForPageAndRedirects(
+        *page_url, favicon_data.icon_type, icon_url_id_map);
+  }
+
+  request->ForwardResult(request->handle(), favicon_data);
 }
 
-void HistoryBackend::GetFaviconForURL(
-    scoped_refptr<GetFaviconRequest> request,
+void HistoryBackend::SetFavicons(
     const GURL& page_url,
-    int icon_types) {
-  if (request->canceled())
+    IconType icon_type,
+    const std::vector<FaviconDataElement>& elements,
+    const IconURLSizesMap& icon_url_sizes) {
+  if (!thumbnail_db_.get() || !db_.get())
     return;
 
-  FaviconData favicon;
+  DCHECK(ValidateSetFaviconsParams(elements, icon_url_sizes));
+
+  // Build map of |elements| for icon url.
+  typedef std::map<GURL, std::vector<FaviconDataElement> > ElementsByIconURL;
+  ElementsByIconURL grouped_by_icon_url;
+  for (size_t i = 0; i < elements.size(); ++i) {
+    GURL icon_url = elements[i].icon_url;

[stevenjb, 2012/08/15 22:59:18]

You could use a GURL& or skip local to avoid an extra copy here.

+    grouped_by_icon_url[icon_url].push_back(elements[i]);
+  }
+
+  IconURLFaviconIDMap icon_url_id_map;
+  for (ElementsByIconURL::iterator it = grouped_by_icon_url.begin();
+       it != grouped_by_icon_url.end(); ++it) {
+    GURL icon_url = it->first;
+    FaviconID icon_id =
+        thumbnail_db_->GetFaviconIDForFaviconURL(icon_url, icon_type);
+    if (!icon_id)
+      icon_id = thumbnail_db_->AddFavicon(icon_url, icon_type);
+    icon_url_id_map[icon_url] = icon_id;
+
+    // Update sizes first because it may result in deleting favicon bitmaps.
+    SetFaviconSizes(icon_id, icon_url_sizes.find(icon_url)->second);
+    SetFaviconBitmaps(icon_id, it->second);
+  }
+
+  // Update sizes for favicons which have entries in |icon_url_sizes| but not
+  // in |elements|.
+  for (IconURLSizesMap::const_iterator it = icon_url_sizes.begin();
+       it != icon_url_sizes.end(); ++it) {
+    GURL icon_url = it->first;
+    if (grouped_by_icon_url.find(icon_url) == grouped_by_icon_url.end()) {
+      FaviconID icon_id =
+          thumbnail_db_->GetFaviconIDForFaviconURL(icon_url, icon_type);
+      if (!icon_id)
+        icon_id = thumbnail_db_->AddFavicon(icon_url, icon_type);
+      icon_url_id_map[icon_url] = icon_id;
+      SetFaviconSizes(icon_id, it->second);
+    }
+  }
+
+  SetFaviconMappingsForPageAndRedirects(page_url, icon_type, icon_url_id_map);
+}
 
-  // Get the favicon from DB.
-  GetFaviconFromDB(page_url, icon_types, &favicon);
+void HistoryBackend::SetFaviconBitmaps(
+    FaviconID icon_id,
+    const std::vector<FaviconDataElement>& elements) {
+  std::vector<FaviconBitmapIDSize> bitmap_id_size_listing;
+  thumbnail_db_->GetFaviconBitmapIDSizeListing(icon_id,
+                                               &bitmap_id_size_listing);
 
-  request->ForwardResult(request->handle(), favicon);
+  // A nested loop is ok because in practice neither |elements| nor
+  // |bitmap_id_size_listing| will have many elements.
+  for (size_t i = 0; i < elements.size(); ++i) {
+    FaviconDataElement element = elements[i];
+    FaviconBitmapID bitmap_id = 0;
+    for (size_t j = 0; j < bitmap_id_size_listing.size(); ++j) {
+      if (bitmap_id_size_listing[j].pixel_size == element.pixel_size) {
+        bitmap_id = bitmap_id_size_listing[j].bitmap_id;
+        break;
+      }
+    }
+    if (bitmap_id) {
+      thumbnail_db_->SetFaviconBitmap(bitmap_id, element.bitmap_data,
+                                      base::Time::Now());
+    } else {
+      thumbnail_db_->AddFaviconBitmap(icon_id, element.bitmap_data,
+                                      base::Time::Now(), element.pixel_size);
+    }
+  }
 }
 
-void HistoryBackend::GetFaviconForID(scoped_refptr<GetFaviconRequest> request,
-                                     FaviconID id) {
-  if (request->canceled())
-    return;
+bool HistoryBackend::ValidateSetFaviconsParams(
+    const std::vector<FaviconDataElement>& elements,
+    const IconURLSizesMap& icon_url_sizes) const {
+  if (icon_url_sizes.size() > kMaxFaviconsPerPage)
+    return false;
+  for (IconURLSizesMap::const_iterator it = icon_url_sizes.begin();
+       it != icon_url_sizes.end(); ++it) {
+    if (it->second.num_sizes() > kMaxFaviconBitmapsPerIconURL)
+      return false;
+  }
 
-  FaviconData favicon;
-  GetFaviconFromDB(id, &favicon);
-  request->ForwardResult(request->handle(), favicon);
+  for (size_t i = 0; i < elements.size(); ++i) {
+    IconURLSizesMap::const_iterator it =
+        icon_url_sizes.find(elements[i].icon_url);
+    if (it == icon_url_sizes.end() ||
+        !it->second.has_size(elements[i].pixel_size) ||
+        !elements[i].bitmap_data.get())
+      return false;
+  }
+  return true;
 }
 
-void HistoryBackend::SetFavicon(
+void HistoryBackend::SetFaviconSizes(
+    FaviconID icon_id,
+    const FaviconSizes& sizes) {
+  std::vector<FaviconBitmapIDSize> bitmap_id_size_listing;
+  thumbnail_db_->GetFaviconBitmapIDSizeListing(icon_id,
+                                               &bitmap_id_size_listing);
+
+  // Remove bitmaps whose pixel size is not contained in |sizes|.
+  for (size_t i = 0; i < bitmap_id_size_listing.size(); ++i) {
+    gfx::Size pixel_size = bitmap_id_size_listing[i].pixel_size;
+    if (!sizes.has_size(pixel_size))
+      thumbnail_db_->DeleteFaviconBitmap(bitmap_id_size_listing[i].bitmap_id);
+  }
+
+  thumbnail_db_->SetFaviconSizes(icon_id, sizes.ToString());
+}
+
+bool HistoryBackend::GetFaviconsFromDB(
     const GURL& page_url,
-    const GURL& icon_url,
-    scoped_refptr<base::RefCountedMemory> data,
-    IconType icon_type) {
-  DCHECK(data.get());
-  if (!thumbnail_db_.get() || !db_.get())
-    return;
+    int icon_types,
+    const gfx::Size* desired_pixel_size,
+    FaviconData* favicon_data) {
+  DCHECK(favicon_data);
+
+  if (!db_.get() || !thumbnail_db_.get())
+    return false;
+
+  // Time the query.
+  TimeTicks beginning_time = TimeTicks::Now();
+
+  bool no_errors = true;
+  std::vector<IconMapping> icon_mappings;
+  if (thumbnail_db_->GetIconMappingsForPageURL(page_url, icon_types,
+                                               &icon_mappings)) {
+    for (std::vector<IconMapping>::iterator i = icon_mappings.begin();
+         i != icon_mappings.end(); ++i) {
+      if (!AddToFaviconData(i->icon_id, desired_pixel_size, favicon_data)) {
+        no_errors = false;
+        break;
+      }
+    }
+  }
+
+  UMA_HISTOGRAM_TIMES("History.GetFavIconFromDB",  // historical name
+                      TimeTicks::Now() - beginning_time);
+  return no_errors && !favicon_data->icon_url_sizes.empty();
+}
+
+bool HistoryBackend::AddToFaviconData(FaviconID favicon_id,
+                                      const gfx::Size* desired_pixel_size,
+                                      FaviconData* favicon_data) {
+  GURL icon_url;
+  IconType icon_type;
+  std::string sizes_as_string;
+
+  if (!thumbnail_db_->GetFaviconHeader(favicon_id, &icon_url, &icon_type,
+                                       &sizes_as_string)) {
+    return false;
+  }
 
-  FaviconID id = thumbnail_db_->GetFaviconIDForFaviconURL(
-      icon_url, icon_type, NULL);
-  if (id) {
-    thumbnail_db_->SetFaviconBitmap(id, data, Time::Now());
+  favicon_data->known_icon = true;
+  if (favicon_data->icon_url_sizes.size() == 0) {
+    favicon_data->icon_type = icon_type;
+    favicon_data->expired = false;
   } else {
-    id = thumbnail_db_->AddFavicon(icon_url,
-                                   icon_type,
-                                   "0 0",
-                                   data,
-                                   Time::Now(),
-                                   gfx::Size());
+    DCHECK_EQ(favicon_data->icon_type, icon_type);
   }
 
-  SetFaviconMapping(page_url, id, icon_type);
+  favicon_data->icon_url_sizes[icon_url] = FaviconSizes(sizes_as_string);
+
+  std::vector<FaviconBitmap> favicon_bitmaps;
+  thumbnail_db_->GetFaviconBitmaps(favicon_id, &favicon_bitmaps);
+  for (size_t i = 0; i < favicon_bitmaps.size(); ++i) {
+    FaviconBitmap favicon_bitmap = favicon_bitmaps[i];
+    FaviconDataElement favicon_element;
+    favicon_element.bitmap_data = favicon_bitmap.bitmap_data;
+    favicon_element.pixel_size = favicon_bitmap.pixel_size;
+    favicon_element.icon_url = icon_url;
+    favicon_data->elements.push_back(favicon_element);
+
+    favicon_data->expired |= (Time::Now() - favicon_bitmap.last_updated) >
+        TimeDelta::FromDays(kFaviconRefetchDays);
+  }
+
+  if (desired_pixel_size) {

[stevenjb, 2012/08/15 22:59:18]

Couldn't this just check on desired_pixel_size.IsEmpty()?

[pkotwicz, 2012/08/16 02:58:37]

I think that it is clearer when desired_pixel_size == NULL. But if you think
that it is clearer to use desired_pixel_size.IsEmpty(), I can change this.

[stevenjb, 2012/08/20 17:20:27]

I guess it's a tradeoff between clarity here and consistency in the API - mostly
we use const gfx::Size& desired_pixel_size, so I think we should use it here
also. Additionally, if &gfx::Size() was passed as an argument here (and we use
that for 'default' elsewhere in this CL so it would be an easy enough mistake),
this would do the wrong thing, so I think we should do an IsEmpty() check
regardless.

All that said, I'm OK with this as-is also.

+    int closest_index = GetBestElementForSizeIndex(favicon_data->elements,
+                                                   *desired_pixel_size);
+    if (closest_index >= 0) {
+      FaviconDataElement element = favicon_data->elements[closest_index];
+      favicon_data->elements.clear();
+      favicon_data->elements.push_back(element);
+    }
+  }
+
+  return true;
 }
 
-void HistoryBackend::SetFaviconMapping(const GURL& page_url,
-                                       FaviconID id,
-                                       IconType icon_type) {
+int HistoryBackend::GetBestElementForSizeIndex(
+    const std::vector<FaviconDataElement>& elements,
+    const gfx::Size& desired_pixel_size) const {
+  // TODO(pkotwicz): Use smarter selection algorithm.
+  int closest_element_index = -1;
+  int smallest_diff = std::numeric_limits<int>::max();
+  for (size_t i = 0; i < elements.size(); ++i) {
+    gfx::Size pixel_size = elements[i].pixel_size;
+    int diff = std::abs(pixel_size.width() - desired_pixel_size.width()) +
+        std::abs(pixel_size.height() - desired_pixel_size.height());
+    if (diff < smallest_diff) {
+      closest_element_index = i;
+      smallest_diff = diff;
+    }
+    if (smallest_diff == 0)
+      break;
+  }
+  return closest_element_index;
+}
+
+void HistoryBackend::SetFaviconMappingsForPageAndRedirects(
+    const GURL& page_url,
+    IconType icon_type,
+    const IconURLFaviconIDMap& icon_url_id_map) {
   if (!thumbnail_db_.get())
     return;
 
@@ -1993,19 +2235,10 @@ void HistoryBackend::SetFaviconMapping(const GURL& page_url,
 
   std::set<GURL> favicons_changed;
 
-  // Save page <-> favicon association.
+  // Save page <-> favicon associations.
   for (history::RedirectList::const_iterator i(redirects->begin());
        i != redirects->end(); ++i) {
-    FaviconID replaced_id;
-    if (AddOrUpdateIconMapping(*i, id, icon_type, &replaced_id)) {
-      // The page's favicon ID changed. This means that the one we just
-      // changed from could have been orphaned, and we need to re-check it.
-      // This is not super fast, but this case will get triggered rarely,
-      // since normally a page will always map to the same favicon ID. It
-      // will mostly happen for favicons we import.
-      if (replaced_id && !thumbnail_db_->HasMappingFor(replaced_id))
-        thumbnail_db_->DeleteFavicon(replaced_id);
-
+    if (SetFaviconMappingsForPage(*i, icon_type, icon_url_id_map)) {
       favicons_changed.insert(*i);
     }
   }
@@ -2019,41 +2252,68 @@ void HistoryBackend::SetFaviconMapping(const GURL& page_url,
   ScheduleCommit();
 }
 
-bool HistoryBackend::AddOrUpdateIconMapping(const GURL& page_url,
-                                            FaviconID id,
-                                            IconType icon_type,
-                                            FaviconID* replaced_icon) {
-  *replaced_icon = 0;
+bool HistoryBackend::SetFaviconMappingsForPage(
+    const GURL& page_url,
+    IconType icon_type,
+    const IconURLFaviconIDMap& icon_url_id_map) {
+  DCHECK_LE(icon_url_id_map.size(), kMaxFaviconsPerPage);
+  bool mappings_changed = false;
+
+  IconURLFaviconIDMap icon_mappings_to_add = icon_url_id_map;
+
+  // Two icon types are considered 'equivalent' if
+  // a. The two icon types are equal.
+  // b. If one of the icon types is ICON_TOUCH and the other is
+  //    ICON_PRECOMPOSED_TOUCH.
+  //
+  // For each of the urls in |icon_url_id_map|.
+  // a. If one of the icon mappings has the same icon url as the url from
+  //    |icon_url_id_map|.
+  //    i.  If the matched icon mapping has the same type as the given
+  //        |icon_type|, the icon mapping was previously created.
+  //    ii. If the matched icon mapping has an 'equivalent' icon type to the
+  //        given |icon_type|, update the icon mapping.
+  // b. Otherwise, add the icon mapping represented by the url.
+  // c. If there are any icon mappings with an icon_url not in
+  //    |icon_url_id_map| and the mapping type is the same or equivalent to the
+  //    given |icon_type|, remove the mapping.
+
   std::vector<IconMapping> icon_mappings;
-  if (!thumbnail_db_->GetIconMappingsForPageURL(page_url, &icon_mappings)) {
-    // There is no mapping add it directly.
-    thumbnail_db_->AddIconMapping(page_url, id);
-    return true;
-  }
-  // Iterate all matched icon mappings,
-  // a. If the given icon id and matched icon id are same, return.
-  // b. If the given icon type and matched icon type are same, but icon id
-  //    are not, update the IconMapping.
-  // c. If the given icon_type and matched icon type are not same, but
-  //    either of them is ICON_TOUCH or ICON_PRECOMPOSED_TOUCH, update the
-  //    IconMapping.
-  // d. Otherwise add a icon mapping.
+  thumbnail_db_->GetIconMappingsForPageURL(page_url, &icon_mappings);
   for (std::vector<IconMapping>::iterator m = icon_mappings.begin();
        m != icon_mappings.end(); ++m) {
-    if (m->icon_id == id)
-      // The mapping is already there.
-      return false;
-
     if ((icon_type == TOUCH_ICON && m->icon_type == TOUCH_PRECOMPOSED_ICON) ||
         (icon_type == TOUCH_PRECOMPOSED_ICON && m->icon_type == TOUCH_ICON) ||
         (icon_type == m->icon_type)) {
-      thumbnail_db_->UpdateIconMapping(m->mapping_id, id);
-      *replaced_icon = m->icon_id;
-      return true;
+      IconURLFaviconIDMap::iterator it =
+          icon_mappings_to_add.find(m->icon_url);
+      if (it == icon_mappings_to_add.end()) {
+        thumbnail_db_->DeleteIconMapping(m->mapping_id);
+      } else {
+        icon_mappings_to_add.erase(it);
+        if (icon_type == m->icon_type) {
+          // The mapping is already there.
+          continue;
+        }
+        thumbnail_db_->UpdateIconMapping(m->mapping_id, it->second);
+      }
+      // Removing / updating the icon mapping may have orphaned the associated
+      // favicon so we must recheck it. This is not super fast, but this case
+      // will get triggered rarely, since normally a page will always map to
+      // the same favicon IDs. It will mostly happen for favicons we import.
+      if (!thumbnail_db_->HasMappingFor(m->icon_id))
+        thumbnail_db_->DeleteFavicon(m->icon_id);
+
+      mappings_changed = true;
     }
   }
-  thumbnail_db_->AddIconMapping(page_url, id);
-  return true;
+
+  for (IconURLFaviconIDMap::iterator it = icon_mappings_to_add.begin();
+       it != icon_mappings_to_add.end(); ++it) {
+    thumbnail_db_->AddIconMapping(page_url, it->second);
+    mappings_changed = true;
+  }
+  return mappings_changed;
 }
 
 void HistoryBackend::Commit() {
@@ -2437,53 +2697,6 @@ BookmarkService* HistoryBackend::GetBookmarkService() {
   return bookmark_service_;
 }
 
-bool HistoryBackend::GetFaviconFromDB(
-    const GURL& page_url,
-    int icon_types,
-    FaviconData* favicon) {
-  DCHECK(favicon);
-
-  if (!db_.get() || !thumbnail_db_.get())
-    return false;
-
-  bool success = false;
-  // Time the query.
-  TimeTicks beginning_time = TimeTicks::Now();
-
-  std::vector<IconMapping> icon_mappings;
-  // Iterate over the known icons looking for one that includes one of the
-  // requested types.
-  if (thumbnail_db_->GetIconMappingsForPageURL(page_url, &icon_mappings)) {
-    for (std::vector<IconMapping>::iterator i = icon_mappings.begin();
-         i != icon_mappings.end(); ++i) {
-      if ((i->icon_type & icon_types) &&
-          GetFaviconFromDB(i->icon_id, favicon)) {
-        success = true;
-        break;
-      }
-    }
-  }
-  UMA_HISTOGRAM_TIMES("History.GetFavIconFromDB",  // historical name
-                      TimeTicks::Now() - beginning_time);
-  return success;
-}
-
-bool HistoryBackend::GetFaviconFromDB(FaviconID favicon_id,
-                                      FaviconData* favicon) {
-  Time last_updated;
-  scoped_refptr<base::RefCountedMemory> data;
-
-  if (!thumbnail_db_->GetFavicon(favicon_id, &last_updated, &data,
-                                 &favicon->icon_url, &favicon->icon_type))
-    return false;
-
-  favicon->expired = (Time::Now() - last_updated) >
-      TimeDelta::FromDays(kFaviconRefetchDays);
-  favicon->known_icon = true;
-  favicon->image_data = data;
-  return true;
-}
-
 void HistoryBackend::NotifyVisitObservers(const VisitRow& visit) {
   BriefVisitInfo info;
   info.url_id = visit.url_id;