Cleanup FaviconSQLHandler::Update

chrome/browser/history/android/favicon_sql_handler.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/android/favicon_sql_handler.h"
 
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/ref_counted_memory.h"
 #include "chrome/browser/history/thumbnail_database.h"
@@ skipping 33 matching lines @@
         Time::Now(),
         gfx::Size());
 
     if (!favicon_id)
       return false;
   }
 
   std::vector<FaviconID> favicon_ids;
   for (TableIDRows::const_iterator i = ids_set.begin();
        i != ids_set.end(); ++i) {
+    // Remove existing icon mappings.
     std::vector<IconMapping> icon_mappings;
-    if (thumbnail_db_->GetIconMappingsForPageURL(i->url, FAVICON,
-                                                 &icon_mappings)) {
-      if (favicon_id) {
-        if (!thumbnail_db_->UpdateIconMapping(icon_mappings[0].mapping_id,
-                                              favicon_id))
-          return false;
-      } else {
-        // Require to delete the icon mapping.
-        if (!thumbnail_db_->DeleteIconMappings(i->url))
-          return false;
-      }
+    thumbnail_db_->GetIconMappingsForPageURL(i->url, FAVICON,
+                                             &icon_mappings);
+    for (std::vector<IconMapping>::const_iterator m = icon_mappings.begin();
+         m != icon_mappings.end(); ++m) {
       // Keep the old icon for deleting it later if possible.
-      favicon_ids.push_back(icon_mappings[0].icon_id);
-    } else if (favicon_id) {
-      // The URL doesn't have icon before, add the icon mapping.
+      favicon_ids.push_back(m->icon_id);
+    }
+    if (!thumbnail_db_->DeleteIconMappings(i->url))

[michaelbai, 2012/09/07 00:29:59]

This not right, you will delete all the mappings onto this url. Not the one
specified.

[pkotwicz, 2012/09/07 01:07:04]

Is it the case, that in the case of favicon_id != 0, only the favicon mappings
to favicons of type FAVICON should be deleted?

+      return false;
+    if (favicon_id) {
+      // Add the icon mapping.
       if (!thumbnail_db_->AddIconMapping(i->url, favicon_id))
         return false;
     }
   }
   // As we update the favicon, Let's remove unused favicons if any.
   if (!favicon_ids.empty() && !DeleteUnusedFavicon(favicon_ids))
     return false;
 
   return true;
 }
@@ skipping 46 matching lines @@
 bool FaviconSQLHandler::DeleteUnusedFavicon(const std::vector<FaviconID>& ids) {
   for (std::vector<FaviconID>::const_iterator i = ids.begin(); i != ids.end();
        ++i) {
     if (!thumbnail_db_->HasMappingFor(*i) && !thumbnail_db_->DeleteFavicon(*i))
       return false;
   }
   return true;
 }
 
 }  // namespace history.