sync: Route local sessions events to SessionsSyncManager

chrome/browser/sync/sessions2/sessions_sync_manager.cc

Index: chrome/browser/sync/sessions2/sessions_sync_manager.cc
diff --git a/chrome/browser/sync/sessions2/sessions_sync_manager.cc b/chrome/browser/sync/sessions2/sessions_sync_manager.cc
index 66ae4d31bb6ad92e29ed83a1fd462a9bc77e3050..182d4c886e385ee090cb90eeb5c270d9e85cd090 100644
--- a/chrome/browser/sync/sessions2/sessions_sync_manager.cc
+++ b/chrome/browser/sync/sessions2/sessions_sync_manager.cc
@@ -37,14 +37,20 @@ static const int kMaxSyncFavicons = 200;
 // The maximum number of navigations in each direction we care to sync.
 static const int kMaxSyncNavigationCount = 6;
 
+// The URL at which the set of synced tabs is displayed. We treat it differently
+// from all other URL's as accessing it triggers a sync refresh of Sessions.
+static const char kNTPOpenTabSyncURL[] = "chrome://newtab/#open_tabs";
+
 SessionsSyncManager::SessionsSyncManager(
     Profile* profile,
-    SyncInternalApiDelegate* delegate)
+    SyncInternalApiDelegate* delegate,
+    scoped_ptr<LocalEventRouter> router)
     : favicon_cache_(profile, kMaxSyncFavicons),
       sync_prefs_(profile->GetPrefs()),
       profile_(profile),
       delegate_(delegate),
-      local_session_header_node_id_(TabNodePool2::kInvalidTabNodeID) {
+      local_session_header_node_id_(TabNodePool2::kInvalidTabNodeID),
+      local_event_router_(router.Pass()) {
 }
 
 SessionsSyncManager::~SessionsSyncManager() {
@@ -116,6 +122,8 @@ syncer::SyncMergeResult SessionsSyncManager::MergeDataAndStartSyncing(
 
   merge_result.set_error(
       sync_processor_->ProcessSyncChanges(FROM_HERE, new_changes));
+
+  local_event_router_->StartRoutingTo(this);
   return merge_result;
 }
 
@@ -293,13 +301,43 @@ void SessionsSyncManager::AssociateTab(SyncedTabDelegate* const tab,
       base::Time::Now();
 }
 
-void SessionsSyncManager::OnLocalTabModified(
-    const SyncedTabDelegate& modified_tab, syncer::SyncError* error) {
-  NOTIMPLEMENTED() << "TODO(tim): SessionModelAssociator::Observe equivalent.";
+void SessionsSyncManager::OnLocalTabModified(SyncedTabDelegate* modified_tab) {
+  const content::NavigationEntry* entry = modified_tab->GetActiveEntry();
+  if (!modified_tab->IsBeingDestroyed() &&
+      entry &&
+      entry->GetVirtualURL().is_valid() &&
+      entry->GetVirtualURL().spec() == kNTPOpenTabSyncURL) {
+    DVLOG(1) << "Triggering sync refresh for sessions datatype.";
+    const syncer::ModelTypeSet types(syncer::SESSIONS);
+    content::NotificationService::current()->Notify(

[rlarocque, 2013/11/27 22:47:51]

I've always thought that this would be better implemented as a WebUI Javascript
callback.  That way we could:
- Clean up the sessions code.
- Not require that sessions sync be enabled for this to work.
- Allow more complicated fetch logic (eg. Refresh every time the user activates
an "other tabs" UI element, rather than on every NTP open).

No need to change it now, though.

[tim (not reviewing), 2013/12/02 18:29:49]

That does seem nice. I actually wrestled around with this code a fair bit
because as it turns out, it's no longer reachable on desktop (hence the
decorations around my test in the _unittest.cc file).  It is reachable on
Android though.

To your point about not requiring sessions to be enabled, can you explain? It
occurs to me that we could do this filtering entirely within the
LocalEventRouter now, which exists unless sessions sync is explicitly disabled,
and probably makes a lot more sense; That could also be an action that
explicitly sends a flare to sync to start.

For now I'd like to maintain as much parity as I can though.  But interested in
changing this down the road.

[rlarocque, 2013/12/02 18:53:53]

By "not requiring sessions to be enabled", I mean that the JavaScript callback
would work regardless of whether or not sessions sync has been explicitly
disabled by the user.  I'm not sure that we care about that, since (I think)
this was implemented to help update the "other devices" UI, so we'd probably
want to disable it if sessions sync were disabled anyway.  Still, though, a
solution that works even when sessions sync is disabled is more flexible and
could be nice to have in the future.

+        chrome::NOTIFICATION_SYNC_REFRESH_LOCAL,
+        content::Source<Profile>(profile_),
+        content::Details<const syncer::ModelTypeSet>(&types));
+  }
+
+  syncer::SyncChangeList changes;
+  // Associate tabs first so the synced session tracker is aware of them.
+  AssociateTab(modified_tab, &changes);
+  // Note, we always associate windows because it's possible a tab became
+  // "interesting" by going to a valid URL, in which case it needs to be added
+  // to the window's tab information.
+  AssociateWindows(DONT_RELOAD_TABS, &changes);
+  sync_processor_->ProcessSyncChanges(FROM_HERE, changes);
 }
 
-void SessionsSyncManager::OnBrowserOpened() {
-  NOTIMPLEMENTED() << "TODO(tim): SessionModelAssociator::Observe equivalent.";
+void SessionsSyncManager::OnFaviconPageUrlsUpdated(
+    const std::set<GURL>& updated_favicon_page_urls) {
+  // TODO(zea): consider a separate container for tabs with outstanding favicon
+  // loads so we don't have to iterate through all tabs comparing urls.
+  for (std::set<GURL>::const_iterator i = updated_favicon_page_urls.begin();
+       i != updated_favicon_page_urls.end(); ++i) {
+    for (TabLinksMap::iterator tab_iter = local_tab_map_.begin();
+         tab_iter != local_tab_map_.end();
+         ++tab_iter) {
+      if (tab_iter->second->url() == *i)
+        favicon_cache_.OnPageFaviconUpdated(*i);
+    }
+  }
 }
 
 bool SessionsSyncManager::ShouldSyncTab(const SyncedTabDelegate& tab) const {
@@ -345,21 +383,6 @@ bool SessionsSyncManager::ShouldSyncWindow(
   return window->IsTypeTabbed() || window->IsTypePopup();
 }
 
-void SessionsSyncManager::ForwardRelevantFaviconUpdatesToFaviconCache(
-    const std::set<GURL>& updated_favicon_page_urls) {
-  // TODO(zea): consider a separate container for tabs with outstanding favicon
-  // loads so we don't have to iterate through all tabs comparing urls.
-  for (std::set<GURL>::const_iterator i = updated_favicon_page_urls.begin();
-       i != updated_favicon_page_urls.end(); ++i) {
-    for (TabLinksMap::iterator tab_iter = local_tab_map_.begin();
-         tab_iter != local_tab_map_.end();
-         ++tab_iter) {
-      if (tab_iter->second->url() == *i)
-        favicon_cache_.OnPageFaviconUpdated(*i);
-    }
-  }
-}
-
 void SessionsSyncManager::StopSyncing(syncer::ModelType type) {
   NOTIMPLEMENTED();
 }
@@ -396,6 +419,8 @@ syncer::SyncError SessionsSyncManager::ProcessSyncChanges(
           // Another client has attempted to delete our local data (possibly by
           // error or a clock is inaccurate). Just ignore the deletion for now
           // to avoid any possible ping-pong delete/reassociate sequence.
+          // TODO(tim): Bug 98892.  This corrupts TabNodePool. Perform full
+          // re-association.
           LOG(WARNING) << "Local session data deleted. Ignoring until next "
                        << "local navigation event.";
         } else if (session.has_header()) {
@@ -868,7 +893,6 @@ void SessionsSyncManager::SetSessionTabFromDelegate(
   session_tab->session_storage_persistent_id.clear();
 }
 
-
 FaviconCache* SessionsSyncManager::GetFaviconCache() {
   return &favicon_cache_;
 }