Prevent bookmarks from going out of sync part 2

chrome/browser/history/history_backend.cc

 // Copyright (c) 2012 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.
 
 #include "chrome/browser/history/history_backend.h"
 
 #include <algorithm>
 #include <list>
 #include <map>
 #include <set>
@@ skipping 1835 matching lines @@
     thumbnail_db_->GetFaviconBitmapIDSizes(icon_mappings[i].icon_id,
                                            &bitmap_id_sizes);
 
     for (size_t j = 0; j < bitmap_id_sizes.size(); ++j) {
       if (bitmap_id_sizes[j].pixel_size == pixel_size) {
         // There is a favicon bitmap of |pixel_size| already mapped to
         // |page_url|, replace it.
         thumbnail_db_->SetFaviconBitmap(bitmap_id_sizes[j].bitmap_id,
             bitmap_data, base::Time::Now());
 
-        // TODO(pkotwicz): Investigate as to whether the UI should be notified.
+        // Send notification to the UI that the favicon bitmap was updated.
+        SendFaviconChangedNotificationForPageAndRedirects(page_url);
         ScheduleCommit();
         return;
       }
     }
   }
 
   // There is no exact match for |pixel_size|. Create a new favicon with a fake
   // icon URL. Use |page_url| as the fake icon URL as it is guaranteed to be
   // unique.
   const GURL& fake_icon_url = page_url;
@@ skipping 59 matching lines @@
 
   // Build map of FaviconBitmapData for each icon url.
   typedef std::map<GURL, std::vector<FaviconBitmapData> >
       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]);
   }
 
-  bool favicon_bitmap_added_or_removed = false;
-
   std::vector<FaviconID> icon_ids;
   for (IconURLSizesMap::const_iterator it = icon_url_sizes.begin();
        it != icon_url_sizes.end(); ++it) {
     const GURL& icon_url = it->first;
     FaviconID icon_id =
         thumbnail_db_->GetFaviconIDForFaviconURL(icon_url, icon_type, NULL);
-    if (icon_id) {
-      bool favicon_bitmap_removed = false;
-      SetFaviconSizes(icon_id, it->second, &favicon_bitmap_removed);
-      favicon_bitmap_added_or_removed |= favicon_bitmap_removed;
-    } else {
+    if (icon_id)
+      SetFaviconSizes(icon_id, it->second);
+    else
       icon_id = thumbnail_db_->AddFavicon(icon_url, icon_type, it->second);
-    }
     icon_ids.push_back(icon_id);
 
     BitmapDataByIconURL::iterator grouped_by_icon_url_it =
         grouped_by_icon_url.find(icon_url);
-    if (grouped_by_icon_url_it != grouped_by_icon_url.end()) {
-      bool favicon_bitmap_added = false;
-      SetFaviconBitmaps(icon_id, grouped_by_icon_url_it->second,
-                        &favicon_bitmap_added);
-      favicon_bitmap_added_or_removed |= favicon_bitmap_added;
-    }
+    if (grouped_by_icon_url_it != grouped_by_icon_url.end())
+      SetFaviconBitmaps(icon_id, grouped_by_icon_url_it->second);
   }
 
-  bool mappings_changed =
-      SetFaviconMappingsForPageAndRedirects(page_url, icon_type, icon_ids);
+  SetFaviconMappingsForPageAndRedirects(page_url, icon_type, icon_ids);
 
-  // Send notification to the UI if icon mappings, favicons, or favicon bitmaps
-  // were added or removed. Favicon addition and removal is not tracked as
-  // adding or removing a favicon will always be accompanied by an update in
-  // icon mappings.
-  // TODO(pkotwicz): Investigate if notifications should be sent when a favicon
-  // bitmap has been updated but not added or removed.
-  if (mappings_changed || favicon_bitmap_added_or_removed)
-    SendFaviconChangedNotificationForPageAndRedirects(page_url);
+  // Send notification to the UI as an icon mapping, favicon, or favicon bitmap
+  // almost certainly was changed by this function. The situations where no
+  // data was changed, notably when |favicon_bitmap_data| is empty do not occur
+  // in practice.
+  SendFaviconChangedNotificationForPageAndRedirects(page_url);
   ScheduleCommit();
 }
 
 void HistoryBackend::SetFaviconsOutOfDateForPage(const GURL& page_url) {
   std::vector<IconMapping> icon_mappings;
 
   if (!thumbnail_db_.get() ||
       !thumbnail_db_->GetIconMappingsForPageURL(page_url,
                                                 &icon_mappings))
     return;
@@ skipping 151 matching lines @@
       desired_scale_factors, &favicon_bitmap_results);
   IconURLSizesMap icon_url_sizes;
   BuildIconURLSizesMap(favicon_ids, &icon_url_sizes);
 
   request->ForwardResult(request->handle(), favicon_bitmap_results,
                          icon_url_sizes);
 }
 
 void HistoryBackend::SetFaviconBitmaps(
     FaviconID icon_id,
-    const std::vector<FaviconBitmapData>& favicon_bitmap_data,
-    bool* favicon_bitmap_added) {
-  *favicon_bitmap_added = false;
-
+    const std::vector<FaviconBitmapData>& favicon_bitmap_data) {
   std::vector<FaviconBitmapIDSize> bitmap_id_sizes;
   thumbnail_db_->GetFaviconBitmapIDSizes(icon_id, &bitmap_id_sizes);
 
   // A nested loop is ok because in practice neither |favicon_bitmap_data| nor
   // |bitmap_id_sizes| will have many elements.
   for (size_t i = 0; i < favicon_bitmap_data.size(); ++i) {
     const FaviconBitmapData& bitmap_data_element = favicon_bitmap_data[i];
     FaviconBitmapID bitmap_id = 0;
     for (size_t j = 0; j < bitmap_id_sizes.size(); ++j) {
       if (bitmap_id_sizes[j].pixel_size == bitmap_data_element.pixel_size) {
         bitmap_id = bitmap_id_sizes[j].bitmap_id;
         break;
       }
     }
     if (bitmap_id) {
       thumbnail_db_->SetFaviconBitmap(bitmap_id,
           bitmap_data_element.bitmap_data, base::Time::Now());
     } else {
       thumbnail_db_->AddFaviconBitmap(icon_id, bitmap_data_element.bitmap_data,
           base::Time::Now(), bitmap_data_element.pixel_size);
-      *favicon_bitmap_added = true;
     }
   }
 }
 
 bool HistoryBackend::ValidateSetFaviconsParams(
     const std::vector<FaviconBitmapData>& favicon_bitmap_data,
     const IconURLSizesMap& icon_url_sizes) const {
   if (icon_url_sizes.size() > kMaxFaviconsPerPage)
     return false;
 
@@ skipping 14 matching lines @@
 
     const FaviconSizes& favicon_sizes = it->second;
     FaviconSizes::const_iterator it2 = std::find(favicon_sizes.begin(),
         favicon_sizes.end(), favicon_bitmap_data[i].pixel_size);
     if (it2 == favicon_sizes.end())
       return false;
   }
   return true;
 }
 
-void HistoryBackend::SetFaviconSizes(
-    FaviconID icon_id,
-    const FaviconSizes& favicon_sizes,
-    bool* favicon_bitmap_removed) {
-  *favicon_bitmap_removed = false;
-
+void HistoryBackend::SetFaviconSizes(FaviconID icon_id,
+                                     const FaviconSizes& favicon_sizes) {
   std::vector<FaviconBitmapIDSize> bitmap_id_sizes;
   thumbnail_db_->GetFaviconBitmapIDSizes(icon_id, &bitmap_id_sizes);
 
   // Remove bitmaps whose pixel size is not contained in |favicon_sizes|.
   for (size_t i = 0; i < bitmap_id_sizes.size(); ++i) {
     const gfx::Size& pixel_size = bitmap_id_sizes[i].pixel_size;
     FaviconSizes::const_iterator sizes_it = std::find(favicon_sizes.begin(),
         favicon_sizes.end(), pixel_size);
-    if (sizes_it == favicon_sizes.end()) {
+    if (sizes_it == favicon_sizes.end())
       thumbnail_db_->DeleteFaviconBitmap(bitmap_id_sizes[i].bitmap_id);
-      *favicon_bitmap_removed = true;
-    }
   }
 
   thumbnail_db_->SetFaviconSizes(icon_id, favicon_sizes);
 }
 
 bool HistoryBackend::GetFaviconsFromDB(
     const GURL& page_url,
     int icon_types,
     int desired_size_in_dip,
     const std::vector<ui::ScaleFactor>& desired_scale_factors,
@@ skipping 609 matching lines @@
   info.url_id = visit.url_id;
   info.time = visit.visit_time;
   info.transition = visit.transition;
   // If we don't have a delegate yet during setup or shutdown, we will drop
   // these notifications.
   if (delegate_.get())
     delegate_->NotifyVisitDBObserversOnAddVisit(info);
 }
 
 }  // namespace history