Cleanup history favicon code

This removes the currently unused IconURLSizesMap.
It also uses the 'expired' parameter in the 'favicon_bitmaps' table instead of the 'sizes' attribute in the 'favicons' table.

Given that we are unlikely going to move to storing unresized bitmaps to the database due to perf problems when resizing the favicon each time it is requested, using the IconURLSizesMap is probably not the right solution in the future anyways. This CL should make the code in HistoryBackend less scary.

BUG=None
Test=No functionality change
     HistoryBackendTest.*

Review URL: https://chromiumcodereview.appspot.com/11746010

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@178432 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 45 insertions, 113 deletions

diff --git a/chrome/browser/favicon/favicon_handler.cc b/chrome/browser/favicon/favicon_handler.cc
index 539ae64..a25f536 100644
--- a/chrome/browser/favicon/favicon_handler.cc
+++ b/chrome/browser/favicon/favicon_handler.cc
@@ -94,59 +94,41 @@ bool IsValid(const history::FaviconBitmapResult& bitmap_result) {
   return bitmap_result.is_valid();
 }
 
-// Return list with the current value and historical values of
-// history::GetDefaultFaviconSizes().
-const std::vector<history::FaviconSizes>& GetHistoricalDefaultFaviconSizes() {
-  CR_DEFINE_STATIC_LOCAL(
-      std::vector<history::FaviconSizes>, kHistoricalFaviconSizes, ());
-  if (kHistoricalFaviconSizes.empty()) {
-    kHistoricalFaviconSizes.push_back(history::GetDefaultFaviconSizes());
-    // The default favicon sizes used to be a single empty size.
-    history::FaviconSizes old_favicon_sizes;
-    old_favicon_sizes.push_back(gfx::Size());
-    kHistoricalFaviconSizes.push_back(old_favicon_sizes);
-  }
-  return kHistoricalFaviconSizes;
-}
-
 // Returns true if at least one of the bitmaps in |bitmap_results| is expired or
-// if |bitmap_results| is known to be incomplete.
+// if |bitmap_results| is missing favicons for |desired_size_in_dip| and one of
+// the scale factors in FaviconUtil::GetFaviconScaleFactors().
 bool HasExpiredOrIncompleteResult(
-    const std::vector<history::FaviconBitmapResult>& bitmap_results,
-    const history::IconURLSizesMap& icon_url_sizes) {
+    int desired_size_in_dip,
+    const std::vector<history::FaviconBitmapResult>& bitmap_results) {
   // Check if at least one of the bitmaps is expired.
   std::vector<history::FaviconBitmapResult>::const_iterator it =
       std::find_if(bitmap_results.begin(), bitmap_results.end(), IsExpired);
   if (it != bitmap_results.end())
     return true;
 
-  // |bitmap_results| is known to be incomplete if the favicon sizes in
-  // |icon_url_sizes| for any of the icon URLs in |bitmap_results| are unknown.
-  // The favicon sizes for an icon URL are unknown if MergeFavicon() has set
-  // them to the default favicon sizes.
-
-  std::set<GURL> icon_urls;
+   // Any favicon size is good if the desired size is 0.
+   if (desired_size_in_dip == 0)
+     return false;
+
+  // Check if the favicon for at least one of the scale factors is missing.
+  // |bitmap_results| should always be complete for data inserted by
+  // FaviconHandler as the FaviconHandler stores favicons resized to all
+  // of FaviconUtil::GetFaviconScaleFactors() into the history backend.
+  // Examples of when |bitmap_results| can be incomplete:
+  // - Favicons inserted into the history backend by sync.
+  // - Favicons for imported bookmarks.
+  std::vector<gfx::Size> favicon_sizes;
   for (size_t i = 0; i < bitmap_results.size(); ++i)
-    icon_urls.insert(bitmap_results[i].icon_url);
-
-  for (std::set<GURL>::iterator it = icon_urls.begin(); it != icon_urls.end();
-       ++it) {
-    const GURL& icon_url = *it;
-    history::IconURLSizesMap::const_iterator icon_url_sizes_it =
-        icon_url_sizes.find(icon_url);
-    if (icon_url_sizes_it == icon_url_sizes.end()) {
-      // |icon_url_sizes| should have an entry for each icon URL in
-      // |bitmap_results|.
-      NOTREACHED();
-      return true;
-    }
-
-    const history::FaviconSizes& sizes = icon_url_sizes_it->second;
-    const std::vector<history::FaviconSizes>& historical_sizes =
-        GetHistoricalDefaultFaviconSizes();
-    std::vector<history::FaviconSizes>::const_iterator historical_it =
-        std::find(historical_sizes.begin(), historical_sizes.end(), sizes);
-    if (historical_it != historical_sizes.end())
+    favicon_sizes.push_back(bitmap_results[i].pixel_size);
+
+  std::vector<ui::ScaleFactor> scale_factors =
+      FaviconUtil::GetFaviconScaleFactors();
+  for (size_t i = 0; i < scale_factors.size(); ++i) {
+    int edge_size_in_pixel = floor(
+        desired_size_in_dip * ui::GetScaleFactorScale(scale_factors[i]));
+    std::vector<gfx::Size>::iterator it = std::find(favicon_sizes.begin(),
+        favicon_sizes.end(), gfx::Size(edge_size_in_pixel, edge_size_in_pixel));
+    if (it == favicon_sizes.end())
       return true;
   }
   return false;
@@ -483,8 +465,7 @@ bool FaviconHandler::ShouldSaveFavicon(const GURL& url) {
 }
 
 void FaviconHandler::OnFaviconDataForInitialURL(
-    const std::vector<history::FaviconBitmapResult>& favicon_bitmap_results,
-    const history::IconURLSizesMap& icon_url_sizes) {
+    const std::vector<history::FaviconBitmapResult>& favicon_bitmap_results) {
   NavigationEntry* entry = GetEntry();
   if (!entry)
     return;
@@ -494,7 +475,7 @@ void FaviconHandler::OnFaviconDataForInitialURL(
 
   bool has_results = !favicon_bitmap_results.empty();
   favicon_expired_or_incomplete_ = has_results && HasExpiredOrIncompleteResult(
-      favicon_bitmap_results, icon_url_sizes);
+      preferred_icon_size(), favicon_bitmap_results);
 
   if (has_results && icon_types_ == history::FAVICON &&
       !entry->GetFavicon().valid &&
@@ -566,15 +547,14 @@ void FaviconHandler::DownloadFaviconOrAskHistory(
 }
 
 void FaviconHandler::OnFaviconData(
-    const std::vector<history::FaviconBitmapResult>& favicon_bitmap_results,
-    const history::IconURLSizesMap& icon_url_sizes) {
+    const std::vector<history::FaviconBitmapResult>& favicon_bitmap_results) {
   NavigationEntry* entry = GetEntry();
   if (!entry)
     return;
 
   bool has_results = !favicon_bitmap_results.empty();
   bool has_expired_or_incomplete_result = HasExpiredOrIncompleteResult(
-      favicon_bitmap_results, icon_url_sizes);
+      preferred_icon_size(), favicon_bitmap_results);
 
   // No need to update the favicon url. By the time we get here
   // UpdateFaviconURL will have set the favicon url.
diff --git a/chrome/browser/favicon/favicon_handler.h b/chrome/browser/favicon/favicon_handler.h
index 135858f..1c39431 100644
--- a/chrome/browser/favicon/favicon_handler.h
+++ b/chrome/browser/favicon/favicon_handler.h
@@ -190,8 +190,7 @@ class FaviconHandler {
 
   // See description above class for details.
   void OnFaviconDataForInitialURL(
-      const std::vector<history::FaviconBitmapResult>& favicon_bitmap_results,
-      const history::IconURLSizesMap& icon_url_sizes);
+      const std::vector<history::FaviconBitmapResult>& favicon_bitmap_results);
 
   // If the favicon has expired, asks the renderer to download the favicon.
   // Otherwise asks history to update the mapping between page url and icon
@@ -202,8 +201,7 @@ class FaviconHandler {
 
   // See description above class for details.
   void OnFaviconData(
-      const std::vector<history::FaviconBitmapResult>& favicon_bitmap_results,
-      const history::IconURLSizesMap& icon_url_sizes);
+      const std::vector<history::FaviconBitmapResult>& favicon_bitmap_results);
 
   // Schedules a download for the specified entry. This adds the request to
   // download_requests_.
diff --git a/chrome/browser/favicon/favicon_handler_unittest.cc b/chrome/browser/favicon/favicon_handler_unittest.cc
index 3f824a7..445c168 100644
--- a/chrome/browser/favicon/favicon_handler_unittest.cc
+++ b/chrome/browser/favicon/favicon_handler_unittest.cc
@@ -314,18 +314,7 @@ namespace {
 
 void HistoryRequestHandler::InvokeCallback() {
   if (!callback_.is_null()) {
-    history::IconURLSizesMap icon_url_sizes;
-    // Build IconURLSizesMap such that the requirement that all the icon URLs
-    // in |history_results_| be present in |icon_url_sizes| holds.
-    // Add the pixel size for each of |history_results_| to |icon_url_sizes|
-    // as empty favicon sizes has a special meaning.
-    for (size_t i = 0; i < history_results_.size(); ++i) {
-      const history::FaviconBitmapResult& bitmap_result = history_results_[i];
-      const GURL& icon_url = bitmap_result.icon_url;
-      icon_url_sizes[icon_url].push_back(bitmap_result.pixel_size);
-    }
-
-    callback_.Run(history_results_, icon_url_sizes);
+    callback_.Run(history_results_);
   }
 }
 
diff --git a/chrome/browser/favicon/favicon_service.cc b/chrome/browser/favicon/favicon_service.cc
index e4d6350..2f8f31c 100644
--- a/chrome/browser/favicon/favicon_service.cc
+++ b/chrome/browser/favicon/favicon_service.cc
@@ -25,11 +25,10 @@ namespace {
 void CancelOrRunFaviconResultsCallback(
     const CancelableTaskTracker::IsCanceledCallback& is_canceled,
     const FaviconService::FaviconResultsCallback& callback,
-    const std::vector<history::FaviconBitmapResult>& results,
-    const history::IconURLSizesMap& size_map) {
+    const std::vector<history::FaviconBitmapResult>& results) {
   if (is_canceled.Run())
     return;
-  callback.Run(results, size_map);
+  callback.Run(results);
 }
 
 // Helper to run callback with empty results if we cannot get the history
@@ -40,9 +39,7 @@ CancelableTaskTracker::TaskId RunWithEmptyResultAsync(
   return tracker->PostTask(
       base::MessageLoopProxy::current(),
       FROM_HERE,
-      Bind(callback,
-           std::vector<history::FaviconBitmapResult>(),
-           history::IconURLSizesMap()));
+      Bind(callback, std::vector<history::FaviconBitmapResult>()));
 }
 
 }  // namespace
@@ -54,9 +51,8 @@ FaviconService::FaviconService(HistoryService* history_service)
 // static
 void FaviconService::FaviconResultsCallbackRunner(
     const FaviconResultsCallback& callback,
-    const std::vector<history::FaviconBitmapResult>* results,
-    const history::IconURLSizesMap* size_map) {
-  callback.Run(*results, *size_map);
+    const std::vector<history::FaviconBitmapResult>* results) {
+  callback.Run(*results);
 }
 
 CancelableTaskTracker::TaskId FaviconService::GetFaviconImage(
@@ -243,7 +239,6 @@ void FaviconService::SetFavicons(
   image_skia.EnsureRepsForSupportedScaleFactors();
   const std::vector<gfx::ImageSkiaRep>& image_reps = image_skia.image_reps();
   std::vector<history::FaviconBitmapData> favicon_bitmap_data;
-  history::FaviconSizes favicon_sizes;
   for (size_t i = 0; i < image_reps.size(); ++i) {
     scoped_refptr<base::RefCountedBytes> bitmap_data(
         new base::RefCountedBytes());
@@ -258,21 +253,10 @@ void FaviconService::SetFavicons(
       bitmap_data_element.icon_url = icon_url;
 
       favicon_bitmap_data.push_back(bitmap_data_element);
-
-      // Construct favicon sizes from a guess at what the HTML 5 'sizes'
-      // attribute in the link tag is.
-      // TODO(pkotwicz): Plumb the HTML 5 sizes attribute to FaviconHandler.
-      favicon_sizes.push_back(pixel_size);
     }
   }
 
-  // TODO(pkotwicz): Tell the database about all the icon URLs associated
-  // with |page_url|.
-  history::IconURLSizesMap icon_url_sizes;
-  icon_url_sizes[icon_url] = favicon_sizes;
-
-  history_service_->SetFavicons(page_url, icon_type, favicon_bitmap_data,
-      icon_url_sizes);
+  history_service_->SetFavicons(page_url, icon_type, favicon_bitmap_data);
 }
 
 FaviconService::~FaviconService() {}
@@ -308,8 +292,7 @@ CancelableTaskTracker::TaskId FaviconService::GetFaviconForURLImpl(
 void FaviconService::RunFaviconImageCallbackWithBitmapResults(
     const FaviconImageCallback& callback,
     int desired_size_in_dip,
-    const std::vector<history::FaviconBitmapResult>& favicon_bitmap_results,
-    const history::IconURLSizesMap& icon_url_sizes_map) {
+    const std::vector<history::FaviconBitmapResult>& favicon_bitmap_results) {
   history::FaviconImageResult image_result;
   image_result.image = FaviconUtil::SelectFaviconFramesFromPNGs(
       favicon_bitmap_results,
@@ -324,8 +307,7 @@ void FaviconService::RunFaviconRawCallbackWithBitmapResults(
     const FaviconRawCallback& callback,
     int desired_size_in_dip,
     ui::ScaleFactor desired_scale_factor,
-    const std::vector<history::FaviconBitmapResult>& favicon_bitmap_results,
-    const history::IconURLSizesMap& icon_url_sizes_map) {
+    const std::vector<history::FaviconBitmapResult>& favicon_bitmap_results) {
   if (favicon_bitmap_results.empty() || !favicon_bitmap_results[0].is_valid()) {
     callback.Run(history::FaviconBitmapResult());
     return;
diff --git a/chrome/browser/favicon/favicon_service.h b/chrome/browser/favicon/favicon_service.h
index d68bf74..12ad688 100644
--- a/chrome/browser/favicon/favicon_service.h
+++ b/chrome/browser/favicon/favicon_service.h
@@ -77,29 +77,14 @@ class FaviconService : public CancelableRequestProvider,
   // platform (eg MacOS) in addition to 1x. The vector has at most one result
   // for each of the scale factors. There are less entries if a single result
   // is the best bitmap to use for several scale factors.
-  //
-  // Second argument:
-  // a) If the callback is called as a result of GetFaviconForURL():
-  //    The second argument is a map of the icon URLs mapped to |page_url| to
-  //    the sizes at which the favicon is available from the web.
-  // b) If the callback is called as a result of GetFavicon() or
-  //    UpdateFaviconMappingsAndFetch():
-  //    The second argument is a map of the subset of |icon_urls| known to the
-  //    history backend to a vector of sizes of the favicon bitmaps at each
-  //    URL. If none of |icon_urls| are known to the history backend, an empty
-  //    map is returned.
-  // See history_types.h for more information about IconURLSizesMap.
-  typedef base::Callback<
-      void(const std::vector<history::FaviconBitmapResult>&,
-           const history::IconURLSizesMap&)>
+  typedef base::Callback<void(const std::vector<history::FaviconBitmapResult>&)>
       FaviconResultsCallback;
 
   // We usually pass parameters with pointer to avoid copy. This function is a
   // helper to run FaviconResultsCallback with pointer parameters.
   static void FaviconResultsCallbackRunner(
       const FaviconResultsCallback& callback,
-      const std::vector<history::FaviconBitmapResult>* results,
-      const history::IconURLSizesMap* size_map);
+      const std::vector<history::FaviconBitmapResult>* results);
 
   // Requests the favicon at |icon_url| of |icon_type| whose size most closely
   // matches |desired_size_in_dip|. If |desired_size_in_dip| is 0, the largest
@@ -256,8 +241,7 @@ class FaviconService : public CancelableRequestProvider,
   void RunFaviconImageCallbackWithBitmapResults(
       const FaviconImageCallback& callback,
       int desired_size_in_dip,
-      const std::vector<history::FaviconBitmapResult>& favicon_bitmap_results,
-      const history::IconURLSizesMap& icon_url_sizes_map);
+      const std::vector<history::FaviconBitmapResult>& favicon_bitmap_results);
 
   // Intermediate callback for GetRawFavicon() and GetRawFaviconForURL()
   // so that history service can deal solely with FaviconResultsCallback.
@@ -266,8 +250,7 @@ class FaviconService : public CancelableRequestProvider,
       const FaviconRawCallback& callback,
       int desired_size_in_dip,
       ui::ScaleFactor desired_scale_factor,
-      const std::vector<history::FaviconBitmapResult>& favicon_bitmap_results,
-      const history::IconURLSizesMap& icon_url_sizes_map);
+      const std::vector<history::FaviconBitmapResult>& favicon_bitmap_results);
 
   DISALLOW_COPY_AND_ASSIGN(FaviconService);
 };