[Sync] Fix favicon updates to handle orphan nodes

Image and tracking updates are now tracked separately, so that we properly
distinguish between adds/updates.

BUG=226539


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

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

chrome/browser/sync/glue/favicon_cache.cc

@@ -52,6 +52,21 @@ struct SyncedFaviconInfo {
   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).
@@ -187,6 +202,16 @@ bool UpdateFaviconFromBitmapResult(
   }
 }
 
+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() {}
@@ -325,11 +350,6 @@ syncer::SyncError FaviconCache::ProcessSyncChanges(
       } 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) {
@@ -387,7 +407,7 @@ void FaviconCache::OnPageFaviconUpdated(const GURL& page_url) {
                << ": " << 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;
     }
@@ -431,8 +451,11 @@ void FaviconCache::OnFaviconVisited(const GURL& page_url,
   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(
@@ -517,11 +540,15 @@ void FaviconCache::OnReceivedSyncFaviconImpl(
   // 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) {
@@ -594,7 +621,7 @@ void FaviconCache::OnFaviconDataAvailable(
   }
 
   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;
@@ -605,15 +632,19 @@ void FaviconCache::OnFaviconDataAvailable(
     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(
@@ -621,13 +652,19 @@ void FaviconCache::OnFaviconDataAvailable(
           (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,
-                    SYNC_BOTH,
-                    (added_favicon ?
-                     syncer::SyncChange::ACTION_ADD :
-                     syncer::SyncChange::ACTION_UPDATE));
+
+    syncer::SyncChange::SyncChangeType image_change =
+        syncer::SyncChange::ACTION_INVALID;
+    if (iter->second.new_image)
+      image_change = syncer::SyncChange::ACTION_ADD;
+    else if (iter->second.image_needs_rewrite)
+      image_change = syncer::SyncChange::ACTION_UPDATE;
+    syncer::SyncChange::SyncChangeType tracking_change =
+        syncer::SyncChange::ACTION_UPDATE;
+    if (iter->second.new_tracking)
+      tracking_change = syncer::SyncChange::ACTION_ADD;
+    UpdateSyncState(favicon_url, image_change, tracking_change);
     if (legacy_delegate_)
       legacy_delegate_->OnFaviconUpdated(page_url, favicon_url);
 
@@ -638,8 +675,8 @@ void FaviconCache::OnFaviconDataAvailable(
 
 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.
@@ -654,7 +691,7 @@ void FaviconCache::UpdateSyncState(
 
   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();
@@ -662,13 +699,13 @@ void FaviconCache::UpdateSyncState(
 
     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();
@@ -676,7 +713,7 @@ void FaviconCache::UpdateSyncState(
 
     tracking_changes.push_back(
         syncer::SyncChange(FROM_HERE,
-                           change_type,
+                           tracking_change_type,
                            syncer::SyncData::CreateLocalData(
                                icon_url.spec(),
                                icon_url.spec(),

chrome/browser/sync/glue/favicon_cache.h

@@ -134,13 +134,6 @@ class FaviconCache : public syncer::SyncableService,
   // Map of page url to favicon url.
   typedef std::map<GURL, GURL> PageFaviconMap;
 
-  // Enum used to decide which sync datatypes to update on a favicon change.
-  enum SyncState {
-    SYNC_IMAGE,
-    SYNC_TRACKING,
-    SYNC_BOTH
-  };
-
   // Helper method to perform OnReceivedSyncFavicon work without worrying about
   // whether caller holds a sync transaction.
   void OnReceivedSyncFaviconImpl(const GURL& icon_url,
@@ -153,12 +146,14 @@ class FaviconCache : public syncer::SyncableService,
       const GURL& page_url,
       const std::vector<history::FaviconBitmapResult>& bitmap_result);
 
-  // Helper method to update the sync state of the favicon at |icon_url|.
+  // Helper method to update the sync state of the favicon at |icon_url|. If
+  // either |image_change_type| or |tracking_change_type| is ACTION_INVALID,
+  // the corresponding datatype won't be updated.
   // Note: should only be called after both FAVICON_IMAGES and FAVICON_TRACKING
   // have been successfully set up.
   void 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);
 
   // Helper method to get favicon info from |synced_favicons_|. If no info
   // exists for |icon_url|, creates a new SyncedFaviconInfo in both

chrome/browser/sync/glue/favicon_cache_unittest.cc

@@ -1482,4 +1482,84 @@ TEST_F(SyncFaviconCacheTest, ReuseCachedIconUrl) {
   EXPECT_EQ(0U, GetTaskCount());
 }
 
+// If we wind up with orphan image/tracking nodes, then receive an update
+// for those favicons, we should lazily create the missing nodes.
+TEST_F(SyncFaviconCacheTest, UpdatedOrphans) {
+  EXPECT_EQ(0U, GetFaviconCount());
+  SetUpInitialSync(syncer::SyncDataList(), syncer::SyncDataList());
+
+  syncer::SyncChangeList initial_image_changes;
+  syncer::SyncChangeList initial_tracking_changes;
+  for (int i = 0; i < kFaviconBatchSize; ++i) {
+    TestFaviconData test_data = BuildFaviconData(i);
+    // Even favicons have image data but no tracking data. Odd favicons have
+    // tracking data but no image data.
+    if (i % 2 == 0) {
+      sync_pb::EntitySpecifics image_specifics;
+      FillImageSpecifics(BuildFaviconData(i),
+                         image_specifics.mutable_favicon_image());
+      initial_image_changes.push_back(
+          syncer::SyncChange(FROM_HERE,
+                             syncer::SyncChange::ACTION_ADD,
+                             syncer::SyncData::CreateRemoteData(
+                                 1, image_specifics)));
+    } else {
+      sync_pb::EntitySpecifics tracking_specifics;
+      FillTrackingSpecifics(BuildFaviconData(i),
+                            tracking_specifics.mutable_favicon_tracking());
+      initial_tracking_changes.push_back(
+          syncer::SyncChange(FROM_HERE,
+                             syncer::SyncChange::ACTION_ADD,
+                             syncer::SyncData::CreateRemoteData(
+                                 1, tracking_specifics)));
+    }
+  }
+
+  cache()->ProcessSyncChanges(FROM_HERE, initial_image_changes);
+  cache()->ProcessSyncChanges(FROM_HERE, initial_tracking_changes);
+  EXPECT_EQ(0U, processor()->GetAndResetChangeList().size());
+  EXPECT_EQ((unsigned long)kFaviconBatchSize, GetFaviconCount());
+
+  for (int i = 0; i < kFaviconBatchSize/2; ++i) {
+    TestFaviconData test_data = BuildFaviconData(i);
+    cache()->OnFaviconVisited(test_data.page_url, GURL());
+    EXPECT_EQ(1U, GetTaskCount());
+    OnCustomFaviconDataAvailable(test_data);
+    syncer::SyncChangeList changes = processor()->GetAndResetChangeList();
+
+    // Even favicons had image data, so should now receive new tracking data
+    // and updated image data (we allow one update after the initial add).
+    // Odd favicons had tracking so should now receive new image data and
+    // updated tracking data.
+    if (i % 2 == 0) {
+      ASSERT_EQ(2U, changes.size());
+      EXPECT_EQ(syncer::SyncChange::ACTION_UPDATE, changes[0].change_type());
+      EXPECT_TRUE(
+          CompareFaviconDataToSpecifics(test_data,
+                                        changes[0].sync_data().GetSpecifics()));
+      EXPECT_EQ(syncer::SyncChange::ACTION_ADD, changes[1].change_type());
+      EXPECT_EQ(test_data.icon_url.spec(),
+                changes[1].sync_data().GetSpecifics().favicon_tracking().
+                    favicon_url());
+      EXPECT_NE(changes[1].sync_data().GetSpecifics().favicon_tracking().
+                    last_visit_time_ms(), 0);
+    } else {
+      ASSERT_EQ(2U, changes.size());
+      EXPECT_EQ(syncer::SyncChange::ACTION_ADD, changes[0].change_type());
+      EXPECT_TRUE(
+          CompareFaviconDataToSpecifics(test_data,
+                                        changes[0].sync_data().GetSpecifics()));
+      EXPECT_EQ(syncer::SyncChange::ACTION_UPDATE, changes[1].change_type());
+      EXPECT_EQ(test_data.icon_url.spec(),
+                changes[1].sync_data().GetSpecifics().favicon_tracking().
+                    favicon_url());
+      EXPECT_NE(changes[1].sync_data().GetSpecifics().favicon_tracking().
+                    last_visit_time_ms(), 0);
+    }
+  }
+
+  EXPECT_EQ(0U, GetTaskCount());
+  EXPECT_EQ((unsigned long)kFaviconBatchSize, GetFaviconCount());
+}
+
 }  // namespace browser_sync

sync/internal_api/sync_manager_impl.cc

@@ -122,6 +122,8 @@ class NudgeStrategy {
        return ACCOMPANY_ONLY;
      case PREFERENCES:
      case SESSIONS:
+     case FAVICON_IMAGES:
+     case FAVICON_TRACKING:
        return CUSTOM;
      default:
        return IMMEDIATE;
@@ -149,6 +151,8 @@ class NudgeStrategy {
                kPreferencesNudgeDelayMilliseconds);
            break;
          case SESSIONS:
+         case FAVICON_IMAGES:
+         case FAVICON_TRACKING:
            delay = core->scheduler()->GetSessionsCommitDelay();
            break;
          default: