Fix FaviconCache not updating visits times from sync

The previous API, OnReceivedSyncFavicon(), received a parameter
|icon_bytes| that was actually only used in tests. This prevented the
visit time from being updated (in-memory).

While fixing this, we move some code (the loop that iterates all
navigations of a sync_pb::SessionTab) to favicon_cache.cc, to make
SessionsSyncManager smaller and couple FaviconCache's API more with
how it is actually used (single caller).

Bug: 817348
Change-Id: I8fb7457177e298759f40e0bcf8d576ffc9fb7f9a
Reviewed-on: https://chromium-review.googlesource.com/940136Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Nicolas Zea (slow) <zea@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540304}

components/sync_sessions/BUILD.gn

@@ -94,6 +94,7 @@ source_set("unit_tests") {
     ":test_support",
     "//base/test:test_support",
     "//components/bookmarks/browser",
+    "//components/favicon/core/test:test_support",
     "//components/history/core/browser",
     "//components/prefs:test_support",
     "//components/sessions:test_support",

components/sync_sessions/favicon_cache.cc

@@ -8,9 +8,7 @@
 
 #include "base/location.h"
 #include "base/metrics/histogram_macros.h"
-#include "base/single_thread_task_runner.h"
 #include "base/stl_util.h"
-#include "base/threading/thread_task_runner_handle.h"
 #include "components/favicon/core/favicon_service.h"
 #include "components/history/core/browser/history_service.h"
 #include "components/sync/model/time.h"
@@ -69,9 +67,6 @@ struct LocalFaviconUpdateInfo {
 
 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;
 
@@ -404,7 +399,8 @@ syncer::SyncError FaviconCache::ProcessSyncChanges(
   return error;
 }
 
-void FaviconCache::OnPageFaviconUpdated(const GURL& page_url) {
+void FaviconCache::OnPageFaviconUpdated(const GURL& page_url,
+                                        base::Time mtime) {
   DCHECK(page_url.is_valid());
 
   // If a favicon load is already happening for this url, let it finish.
@@ -421,7 +417,7 @@ void FaviconCache::OnPageFaviconUpdated(const GURL& page_url) {
         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());
+      UpdateFaviconVisitTime(icon_iter->second->favicon_url, mtime);
       UpdateSyncState(icon_iter->second->favicon_url,
                       syncer::SyncChange::ACTION_INVALID,
                       syncer::SyncChange::ACTION_UPDATE);
@@ -443,7 +439,7 @@ void FaviconCache::OnPageFaviconUpdated(const GURL& page_url) {
       favicon_service_->GetFaviconForPageURL(
           page_url, SupportedFaviconTypes(), kMaxFaviconResolution,
           base::Bind(&FaviconCache::OnFaviconDataAvailable,
-                     weak_ptr_factory_.GetWeakPtr(), page_url),
+                     weak_ptr_factory_.GetWeakPtr(), page_url, mtime),
           &cancelable_task_tracker_);
   page_task_map_[page_url] = id;
 }
@@ -455,7 +451,7 @@ void FaviconCache::OnFaviconVisited(const GURL& page_url,
       synced_favicons_.find(favicon_url) == synced_favicons_.end()) {
     // TODO(zea): consider triggering a favicon load if we have some but not
     // all desired resolutions?
-    OnPageFaviconUpdated(page_url);
+    OnPageFaviconUpdated(page_url, base::Time::Now());
     return;
   }
 
@@ -506,60 +502,38 @@ bool FaviconCache::GetSyncedFaviconForPageURL(
   return GetSyncedFaviconForFaviconURL(iter->second, favicon_png);
 }
 
-void FaviconCache::OnReceivedSyncFavicon(const GURL& page_url,
-                                         const GURL& icon_url,
-                                         const std::string& icon_bytes,
-                                         int64_t visit_time_ms) {
-  if (!icon_url.is_valid() || !page_url.is_valid() || icon_url.SchemeIs("data"))
-    return;
-  DVLOG(1) << "Associating " << page_url.spec() << " with favicon at "
-           << icon_url.spec();
-  page_favicon_map_[page_url] = icon_url;
+void FaviconCache::UpdateMappingsFromForeignTab(const sync_pb::SessionTab& tab,
+                                                base::Time visit_time) {
+  for (const sync_pb::TabNavigation& navigation : tab.navigation()) {
+    const GURL page_url(navigation.virtual_url());
+    const GURL icon_url(navigation.favicon_url());
 
-  // If there is no actual image, it means there either is no synced
-  // favicon, or it's on its way (race condition).
-  // TODO(zea): potentially trigger a favicon web download here (delayed?).
-  if (icon_bytes.size() == 0)
-    return;
+    if (!icon_url.is_valid() || !page_url.is_valid() ||
+        icon_url.SchemeIs("data")) {
+      continue;
+    }
+
+    DVLOG(1) << "Associating " << page_url << " with favicon at " << icon_url;
+    page_favicon_map_[page_url] = icon_url;
 
-  // Post a task to do the actual association because this method may have been
-  // called while in a transaction.
-  base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::Bind(&FaviconCache::OnReceivedSyncFaviconImpl,
-                            weak_ptr_factory_.GetWeakPtr(), icon_url,
-                            icon_bytes, visit_time_ms));
+    if (synced_favicons_.count(icon_url) != 0)
+      UpdateFaviconVisitTime(icon_url, visit_time);
+  }
 }
 
-void FaviconCache::OnReceivedSyncFaviconImpl(const GURL& icon_url,
-                                             const std::string& icon_bytes,
-                                             int64_t visit_time_ms) {
-  // If this favicon is already synced, do nothing else.
-  if (synced_favicons_.find(icon_url) != synced_favicons_.end())
-    return;
+size_t FaviconCache::NumFaviconsForTest() const {
+  return synced_favicons_.size();
+}
 
-  // Don't add any more favicons once we hit our in memory limit.
-  // TODO(zea): UMA this.
-  if (kMaxFaviconsInMem != 0 && synced_favicons_.size() > kMaxFaviconsInMem)
-    return;
+size_t FaviconCache::NumTasksForTest() const {
+  return page_task_map_.size();
+}
 
-  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);
-  bool added_tracking = !FaviconInfoHasTracking(*favicon_info);
-  UpdateFaviconVisitTime(icon_url,
-                         syncer::ProtoTimeToTime(visit_time_ms));
-
-  UpdateSyncState(icon_url,
-                  syncer::SyncChange::ACTION_ADD,
-                  (added_tracking ?
-                   syncer::SyncChange::ACTION_ADD :
-                   syncer::SyncChange::ACTION_UPDATE));
+base::Time FaviconCache::GetLastVisitTimeForTest(
+    const GURL& favicon_url) const {
+  FaviconMap::const_iterator iter = synced_favicons_.find(favicon_url);
+  DCHECK(iter != synced_favicons_.end());
+  return iter->second->last_visit_time;
 }
 
 bool FaviconCache::FaviconRecencyFunctor::operator()(
@@ -575,6 +549,7 @@ bool FaviconCache::FaviconRecencyFunctor::operator()(
 
 void FaviconCache::OnFaviconDataAvailable(
     const GURL& page_url,
+    base::Time mtime,
     const std::vector<favicon_base::FaviconRawBitmapResult>& bitmap_results) {
   PageTaskMap::iterator page_iter = page_task_map_.find(page_url);
   if (page_iter == page_task_map_.end())
@@ -588,7 +563,6 @@ void FaviconCache::OnFaviconDataAvailable(
     return;
   }
 
-  base::Time now = base::Time::Now();
   std::map<GURL, LocalFaviconUpdateInfo> favicon_updates;
   for (size_t i = 0; i < bitmap_results.size(); ++i) {
     const favicon_base::FaviconRawBitmapResult& bitmap_result =
@@ -620,7 +594,7 @@ void FaviconCache::OnFaviconDataAvailable(
     page_favicon_map_[page_url] = favicon_url;
 
     favicon_info->received_local_update = true;
-    UpdateFaviconVisitTime(favicon_url, now);
+    UpdateFaviconVisitTime(favicon_url, mtime);
 
     syncer::SyncChange::SyncChangeType image_change =
         syncer::SyncChange::ACTION_INVALID;
@@ -1012,14 +986,6 @@ void FaviconCache::DropPartialFavicon(FaviconMap::iterator favicon_iter,
   }
 }
 
-size_t FaviconCache::NumFaviconsForTest() const {
-  return synced_favicons_.size();
-}
-
-size_t FaviconCache::NumTasksForTest() const {
-  return page_task_map_.size();
-}
-
 void FaviconCache::OnURLsDeleted(history::HistoryService* history_service,
                                  bool all_history,
                                  bool expired,

components/sync_sessions/favicon_cache.h

@@ -29,6 +29,7 @@
 #include "components/sync/model/sync_change.h"
 #include "components/sync/model/sync_error_factory.h"
 #include "components/sync/model/syncable_service.h"
+#include "components/sync/protocol/session_specifics.pb.h"
 #include "url/gurl.h"
 
 namespace chrome {
@@ -92,26 +93,26 @@ class FaviconCache : public syncer::SyncableService,
       scoped_refptr<base::RefCountedMemory>* favicon_png) const;
 
   // Load the favicon for |page_url|. Will create a new sync node or update
-  // an existing one as necessary, and set the last visit time to the current
-  // time. Only those favicon types defined in SupportedFaviconTypes will be
-  // synced.
-  void OnPageFaviconUpdated(const GURL& page_url);
+  // an existing one as necessary, and update the last visit time with |mtime|,
+  // Only those favicon types defined in SupportedFaviconTypes will be synced.
+  void OnPageFaviconUpdated(const GURL& page_url, base::Time mtime);
 
   // Update the visit count for the favicon associated with |favicon_url|.
   // If no favicon exists associated with |favicon_url|, triggers a load
   // for the favicon associated with |page_url|.
   void OnFaviconVisited(const GURL& page_url, const GURL& favicon_url);
 
-  // Consume Session sync favicon data. Will not overwrite existing favicons.
-  // If |icon_bytes| is empty, only updates the page->favicon url mapping.
-  // Safe to call within a transaction.
-  void OnReceivedSyncFavicon(const GURL& page_url,
-                             const GURL& icon_url,
-                             const std::string& icon_bytes,
-                             int64_t visit_time_ms);
+  // Consume Session sync favicon data to update the in-memory page->favicon url
+  // mappings and visit times.
+  void UpdateMappingsFromForeignTab(const sync_pb::SessionTab& tab,
+                                    base::Time visit_time);
+
+  // For testing only.
+  size_t NumFaviconsForTest() const;
+  size_t NumTasksForTest() const;
+  base::Time GetLastVisitTimeForTest(const GURL& favicon_url) const;
 
  private:
-  friend class SyncFaviconCacheTest;
   FRIEND_TEST_ALL_PREFIXES(SyncFaviconCacheTest, HistoryFullClear);
   FRIEND_TEST_ALL_PREFIXES(SyncFaviconCacheTest, HistorySubsetClear);
 
@@ -131,16 +132,11 @@ class FaviconCache : public syncer::SyncableService,
   // Map of page url to favicon url.
   using PageFaviconMap = std::map<GURL, GURL>;
 
-  // Helper method to perform OnReceivedSyncFavicon work without worrying about
-  // whether caller holds a sync transaction.
-  void OnReceivedSyncFaviconImpl(const GURL& icon_url,
-                                 const std::string& icon_bytes,
-                                 int64_t visit_time_ms);
-
   // Callback method to store a tab's favicon into its sync node once it becomes
   // available. Does nothing if no favicon data was available.
   void OnFaviconDataAvailable(
       const GURL& page_url,
+      base::Time mtime,
       const std::vector<favicon_base::FaviconRawBitmapResult>& bitmap_result);
 
   // Helper method to update the sync state of the favicon at |icon_url|. If
@@ -203,10 +199,6 @@ class FaviconCache : public syncer::SyncableService,
   void DropPartialFavicon(FaviconMap::iterator favicon_iter,
                           syncer::ModelType type);
 
-  // For testing only.
-  size_t NumFaviconsForTest() const;
-  size_t NumTasksForTest() const;
-
   // history::HistoryServiceObserver:
   void OnURLsDeleted(history::HistoryService* history_service,
                      bool all_history,

components/sync_sessions/sessions_sync_manager.cc

@@ -595,7 +595,7 @@ void SessionsSyncManager::OnFaviconsChanged(const std::set<GURL>& page_urls,
                                             const GURL& /* icon_url */) {
   for (const GURL& page_url : page_urls) {
     if (page_url.is_valid())
-      favicon_cache_.OnPageFaviconUpdated(page_url);
+      favicon_cache_.OnPageFaviconUpdated(page_url, base::Time::Now());
   }
 }
 
@@ -709,7 +709,7 @@ syncer::SyncError SessionsSyncManager::ProcessSyncChanges(
         // If a favicon or favicon urls are present, load the URLs and visit
         // times into the in-memory favicon cache.
         if (session.has_tab()) {
-          RefreshFaviconVisitTimesFromForeignTab(session.tab(), mtime);
+          favicon_cache_.UpdateMappingsFromForeignTab(session.tab(), mtime);
         }
         break;
       default:
@@ -766,8 +766,8 @@ bool SessionsSyncManager::InitFromSyncModel(
         // If a favicon or favicon urls are present, load the URLs and visit
         // times into the in-memory favicon cache.
         if (specifics.has_tab()) {
-          RefreshFaviconVisitTimesFromForeignTab(specifics.tab(),
-                                                 remote.GetModifiedTime());
+          favicon_cache_.UpdateMappingsFromForeignTab(specifics.tab(),
+                                                      remote.GetModifiedTime());
         }
       } else {
         // In the past, like years ago, we believe that some session data was
@@ -893,22 +893,6 @@ void SessionsSyncManager::InitializeCurrentMachineTag(
   }
 }
 
-void SessionsSyncManager::RefreshFaviconVisitTimesFromForeignTab(
-    const sync_pb::SessionTab& tab,
-    const base::Time& modification_time) {
-  // First go through and iterate over all the navigations, checking if any
-  // have valid favicon urls.
-  for (int i = 0; i < tab.navigation_size(); ++i) {
-    if (!tab.navigation(i).favicon_url().empty()) {
-      const std::string& page_url = tab.navigation(i).virtual_url();
-      const std::string& favicon_url = tab.navigation(i).favicon_url();
-      favicon_cache_.OnReceivedSyncFavicon(
-          GURL(page_url), GURL(favicon_url), std::string(),
-          syncer::TimeToProtoTime(modification_time));
-    }
-  }
-}
-
 void SessionsSyncManager::DeleteForeignSessionInternal(
     const std::string& tag,
     syncer::SyncChangeList* change_output) {

components/sync_sessions/sessions_sync_manager.h

@@ -140,12 +140,6 @@ class SessionsSyncManager : public syncer::SyncableService,
   // this node could not be deleted.
   syncer::SyncChange TombstoneTab(const sync_pb::SessionSpecifics& tab);
 
-  // Helper method to load the favicon data from the tab specifics. If the
-  // favicon is valid, stores the favicon data into the favicon cache.
-  void RefreshFaviconVisitTimesFromForeignTab(
-      const sync_pb::SessionTab& tab,
-      const base::Time& modification_time);
-
   // Removes a foreign session from our internal bookkeeping.
   // Returns true if the session was found and deleted, false if no data was
   // found for that session.  This will *NOT* trigger sync deletions. See