[Sync] Fix favicon updates to handle orphan nodes

chrome/browser/sync/glue/favicon_cache.cc

 // Copyright 2013 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/sync/glue/favicon_cache.h"
 
 #include "base/message_loop.h"
 #include "base/metrics/histogram.h"
 #include "chrome/browser/favicon/favicon_service.h"
 #include "chrome/browser/favicon/favicon_service_factory.h"
@@ skipping 34 matching lines @@
   // Note: Do not modify this directly! It should only be modified via
   // UpdateFaviconVisitTime(..).
   base::Time last_visit_time;
   // Whether we've received a local update for this favicon since starting up.
   bool received_local_update;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(SyncedFaviconInfo);
 };
 
+// Information for handling local favicon updates. Used in
+// OnFaviconDataAvailable.
+struct LocalFaviconUpdateInfo {
+  LocalFaviconUpdateInfo()
+      : new_image(false),
+        new_tracking(false),
+        image_needs_rewrite(false),
+        favicon_info(NULL) {}
+
+  bool new_image;
+  bool new_tracking;
+  bool image_needs_rewrite;
+  SyncedFaviconInfo* favicon_info;
+};
+
 namespace {
 
 // Maximum number of favicons to keep in memory (0 means no limit).
 const size_t kMaxFaviconsInMem = 0;
 
 // Maximum width/height resolution supported.
 const int kMaxFaviconResolution = 16;
 
 // Returns a mask of the supported favicon types.
 // TODO(zea): Supporting other favicons types will involve some work in the
@@ skipping 115 matching lines @@
     favicon_info->bitmap_data[icon_size] = bitmap_result;
     favicon_info->received_local_update = true;
     return true;
   } else {
     // We only allow updating the image data once per restart.
     DVLOG(2) << "Ignoring local update for " << bitmap_result.icon_url.spec();
     return false;
   }
 }
 
+bool FaviconInfoHasImages(const SyncedFaviconInfo& favicon_info) {
+  return favicon_info.bitmap_data[SIZE_16].bitmap_data.get() ||
+         favicon_info.bitmap_data[SIZE_32].bitmap_data.get() ||
+         favicon_info.bitmap_data[SIZE_64].bitmap_data.get();
+}
+
+bool FaviconInfoHasTracking(const SyncedFaviconInfo& favicon_info) {
+  return !favicon_info.last_visit_time.is_null();
+}
+
 }  // namespace
 
 FaviconCacheObserver::~FaviconCacheObserver() {}
 
 FaviconCache::FaviconCache(Profile* profile, int max_sync_favicon_limit)
     : profile_(profile),
       weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
       legacy_delegate_(NULL),
       max_sync_favicon_limit_(max_sync_favicon_limit) {
   notification_registrar_.Add(this,
@@ skipping 118 matching lines @@
     }
     FaviconMap::iterator favicon_iter = synced_favicons_.find(favicon_url);
     if (iter->change_type() == syncer::SyncChange::ACTION_DELETE) {
       if (favicon_iter == synced_favicons_.end()) {
         // Two clients might wind up deleting different parts of the same
         // favicon, so ignore this.
         continue;
       } else {
         DVLOG(1) << "Deleting favicon at " << favicon_url.spec();
         DropSyncedFavicon(favicon_iter);
-        // TODO(zea): it's possible that we'll receive a deletion for an image,
-        // but not a tracking data, or vice versa, resulting in an orphan
-        // favicon node in one of the types. We should track how often this
-        // happens, and if it becomes a problem delete each part individually
-        // from the local model.
       }
     } else if (iter->change_type() == syncer::SyncChange::ACTION_UPDATE ||
                iter->change_type() == syncer::SyncChange::ACTION_ADD) {
       // Adds and updates are treated the same due to the lack of strong
       // consistency (it's possible we'll receive an update for a tracking info
       // before we've received the add for the image, and should handle both
       // gracefully).
       if (favicon_iter == synced_favicons_.end()) {
         DVLOG(1) << "Adding favicon at " << favicon_url.spec();
         AddLocalFaviconFromSyncedData(iter->sync_data());
@@ skipping 37 matching lines @@
     FaviconMap::const_iterator icon_iter =
         synced_favicons_.find(url_iter->second);
     // TODO(zea): consider what to do when only a subset of supported
     // resolutions are available.
     if (icon_iter != synced_favicons_.end() &&
         icon_iter->second->bitmap_data[SIZE_16].bitmap_data.get()) {
       DVLOG(2) << "Using cached favicon url for " << page_url.spec()
                << ": " << icon_iter->second->favicon_url.spec();
       UpdateFaviconVisitTime(icon_iter->second->favicon_url, base::Time::Now());
       UpdateSyncState(icon_iter->second->favicon_url,
-                      SYNC_TRACKING,
+                      syncer::SyncChange::ACTION_INVALID,
                       syncer::SyncChange::ACTION_UPDATE);
       return;
     }
   }
 
   DVLOG(1) << "Triggering favicon load for url " << page_url.spec();
 
   if (!profile_) {
     page_task_map_[page_url] = 0;  // For testing only.
     return;
@@ skipping 23 matching lines @@
     // all desired resolutions?
     OnPageFaviconUpdated(page_url);
     return;
   }
 
   DVLOG(1) << "Associating " << page_url.spec() << " with favicon at "
            << favicon_url.spec() << " and marking visited.";
   page_favicon_map_[page_url] = favicon_url;
   UpdateFaviconVisitTime(favicon_url, base::Time::Now());
   UpdateSyncState(favicon_url,
-                  SYNC_TRACKING,
-                  syncer::SyncChange::ACTION_UPDATE);
+                  syncer::SyncChange::ACTION_INVALID,
+                  (FaviconInfoHasTracking(
+                       *synced_favicons_.find(favicon_url)->second) ?
+                   syncer::SyncChange::ACTION_UPDATE :
+                   syncer::SyncChange::ACTION_ADD));
 }
 
 bool FaviconCache::GetSyncedFaviconForFaviconURL(
     const GURL& favicon_url,
     scoped_refptr<base::RefCountedMemory>* favicon_png) const {
   if (!favicon_url.is_valid())
     return false;
   FaviconMap::const_iterator iter = synced_favicons_.find(favicon_url);
 
   UMA_HISTOGRAM_BOOLEAN("Sync.FaviconCacheLookupSucceeded",
@@ skipping 64 matching lines @@
 
   SyncedFaviconInfo* favicon_info = GetFaviconInfo(icon_url);
   if (!favicon_info)
     return;  // We reached the in-memory limit.
   base::RefCountedString* temp_string = new base::RefCountedString();
   temp_string->data() = icon_bytes;
   favicon_info->bitmap_data[SIZE_16].bitmap_data = temp_string;
   // We assume legacy favicons are 16x16.
   favicon_info->bitmap_data[SIZE_16].pixel_size.set_width(16);
   favicon_info->bitmap_data[SIZE_16].pixel_size.set_height(16);
-  UpdateFaviconVisitTime(icon_url, syncer::ProtoTimeToTime(visit_time_ms));
+  bool added_tracking = !FaviconInfoHasTracking(*favicon_info);
+  UpdateFaviconVisitTime(icon_url,
+                         syncer::ProtoTimeToTime(visit_time_ms));
 
   UpdateSyncState(icon_url,
-                  SYNC_BOTH,
-                  syncer::SyncChange::ACTION_ADD);
+                  syncer::SyncChange::ACTION_ADD,
+                  (added_tracking ?
+                   syncer::SyncChange::ACTION_ADD :
+                   syncer::SyncChange::ACTION_UPDATE));
 }
 
 void FaviconCache::SetLegacyDelegate(FaviconCacheObserver* observer) {
   legacy_delegate_ = observer;
 }
 
 void FaviconCache::RemoveLegacyDelegate() {
   legacy_delegate_ = NULL;
 }
 
@@ skipping 52 matching lines @@
   page_task_map_.erase(page_iter);
 
   if (bitmap_results.size() == 0) {
     // Either the favicon isn't loaded yet or there is no valid favicon.
     // We already cleared the task id, so just return.
     DVLOG(1) << "Favicon load failed for page " << page_url.spec();
     return;
   }
 
   base::Time now = base::Time::Now();
-  std::set<SyncedFaviconInfo*> favicon_updates;
+  std::map<GURL, LocalFaviconUpdateInfo> favicon_updates;
   for (size_t i = 0; i < bitmap_results.size(); ++i) {
     const history::FaviconBitmapResult& bitmap_result = bitmap_results[i];
     GURL favicon_url = bitmap_result.icon_url;
     if (!favicon_url.is_valid() || favicon_url.SchemeIs("data"))
       continue;  // Can happen if the page is still loading.
 
     SyncedFaviconInfo* favicon_info = GetFaviconInfo(favicon_url);
     if (!favicon_info)
       return;  // We reached the in-memory limit.
 
-    if (!UpdateFaviconFromBitmapResult(bitmap_result, favicon_info))
-      continue; // Invalid favicon or no change.
-
-    favicon_updates.insert(favicon_info);
+    favicon_updates[favicon_url].new_image |=
+        !FaviconInfoHasImages(*favicon_info);
+    favicon_updates[favicon_url].new_tracking |=
+        !FaviconInfoHasTracking(*favicon_info);
+    favicon_updates[favicon_url].image_needs_rewrite |=
+        UpdateFaviconFromBitmapResult(bitmap_result, favicon_info);
+    favicon_updates[favicon_url].favicon_info = favicon_info;
   }
 
-  for (std::set<SyncedFaviconInfo*>::iterator iter = favicon_updates.begin();
-       iter != favicon_updates.end(); ++iter) {
-    SyncedFaviconInfo* favicon_info = *iter;
+  for (std::map<GURL, LocalFaviconUpdateInfo>::const_iterator
+           iter = favicon_updates.begin(); iter != favicon_updates.end();
+       ++iter) {
+    SyncedFaviconInfo* favicon_info = iter->second.favicon_info;
     const GURL& favicon_url = favicon_info->favicon_url;
     if (!favicon_info->last_visit_time.is_null()) {
       UMA_HISTOGRAM_COUNTS_10000(
           "Sync.FaviconVisitPeriod",
           (now - favicon_info->last_visit_time).InHours());
     }
     favicon_info->received_local_update = true;
-    bool added_favicon = favicon_info->last_visit_time.is_null();
     UpdateFaviconVisitTime(favicon_url, now);
     UpdateSyncState(favicon_url,

[rlarocque, 2013/04/04 23:29:07]

I dislike ternary operators, and wouldn't use them myself, but I'm willing to
let it slide in code reviews.  I draw the line at nested ternary operators. 
Please rewrite this logic as an if statement.  

[Nicolas Zea, 2013/04/04 23:42:29]

Haha, ok, done.

-                    SYNC_BOTH,
-                    (added_favicon ?
+                    (iter->second.new_image ?
+                     syncer::SyncChange::ACTION_ADD :
+                     (iter->second.image_needs_rewrite ?
+                      syncer::SyncChange::ACTION_UPDATE :
+                      syncer::SyncChange::ACTION_INVALID)),
+                    (iter->second.new_tracking ?
                      syncer::SyncChange::ACTION_ADD :
                      syncer::SyncChange::ACTION_UPDATE));
     if (legacy_delegate_)
       legacy_delegate_->OnFaviconUpdated(page_url, favicon_url);
 
     // TODO(zea): support multiple favicon urls per page.
     page_favicon_map_[page_url] = favicon_url;
   }
 }
 
 void FaviconCache::UpdateSyncState(
     const GURL& icon_url,
-    SyncState state_to_update,
-    syncer::SyncChange::SyncChangeType change_type) {
+    syncer::SyncChange::SyncChangeType image_change_type,
+    syncer::SyncChange::SyncChangeType tracking_change_type) {
   DCHECK(icon_url.is_valid());
   // It's possible that we'll receive a favicon update before both types
   // have finished setting up. In that case ignore the update.
   // TODO(zea): consider tracking these skipped updates somehow?
   if (!favicon_images_sync_processor_.get() ||
       !favicon_tracking_sync_processor_.get())
     return;
 
   FaviconMap::const_iterator iter = synced_favicons_.find(icon_url);
   DCHECK(iter != synced_favicons_.end());
   const SyncedFaviconInfo* favicon_info = iter->second.get();
 
   syncer::SyncChangeList image_changes;
   syncer::SyncChangeList tracking_changes;
-  if (state_to_update == SYNC_IMAGE || state_to_update == SYNC_BOTH) {
+  if (image_change_type != syncer::SyncChange::ACTION_INVALID) {
     sync_pb::EntitySpecifics new_specifics;
     sync_pb::FaviconImageSpecifics* image_specifics =
         new_specifics.mutable_favicon_image();
     BuildImageSpecifics(favicon_info, image_specifics);
 
     image_changes.push_back(
         syncer::SyncChange(FROM_HERE,
-                           change_type,
+                           image_change_type,
                            syncer::SyncData::CreateLocalData(
                                icon_url.spec(),
                                icon_url.spec(),
                                new_specifics)));
   }
-  if (state_to_update == SYNC_TRACKING || state_to_update == SYNC_BOTH) {
+  if (tracking_change_type != syncer::SyncChange::ACTION_INVALID) {
     sync_pb::EntitySpecifics new_specifics;
     sync_pb::FaviconTrackingSpecifics* tracking_specifics =
         new_specifics.mutable_favicon_tracking();
     BuildTrackingSpecifics(favicon_info, tracking_specifics);
 
     tracking_changes.push_back(
         syncer::SyncChange(FROM_HERE,
-                           change_type,
+                           tracking_change_type,
                            syncer::SyncData::CreateLocalData(
                                icon_url.spec(),
                                icon_url.spec(),
                                new_specifics)));
   }
   ExpireFaviconsIfNecessary(&image_changes, &tracking_changes);
   if (!image_changes.empty()) {
     favicon_images_sync_processor_->ProcessSyncChanges(FROM_HERE,
                                                        image_changes);
   }
@@ skipping 260 matching lines @@
 
 size_t FaviconCache::NumFaviconsForTest() const {
   return synced_favicons_.size();
 }
 
 size_t FaviconCache::NumTasksForTest() const {
   return page_task_map_.size();
 }
 
 }  // namespace browser_sync