[sync] WebContentsObserver based sessions notifications

chrome/browser/sync/sessions/sync_sessions_web_contents_router.h

+// Copyright 2017 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.
+
+#ifndef CHROME_BROWSER_SYNC_SESSIONS_SYNC_SESSIONS_WEB_CONTENTS_ROUTER_H_
+#define CHROME_BROWSER_SYNC_SESSIONS_SYNC_SESSIONS_WEB_CONTENTS_ROUTER_H_
+
+#include "base/callback_list.h"
+
+// Android has no BrowserList or TabStripModel, so we exclude code that refers
+// to those two things. For non-android platforms, this code is used to
+// ensure we are notified of tabs being added and moved between tab strips.
+#if !defined(OS_ANDROID)

[Nicolas Zea, 2017/03/16 19:57:43]

is this pattern of using platform conditional code used elsewhere? If not, I
think it might be better to just have separate files with the platform specific
code as much as possible, otherwise this might become brittle. Thoughts?

[Patrick Noland, 2017/03/16 20:23:27]

I've seen it done both ways(ifdefs and separate files). My hesitation with using
two separate files is code duplication; any change to the "main" file will have
to be repeated in the android file. This might be avoidable with inheritance
though?

[Patrick Noland, 2017/03/17 00:20:10]

I've added a subclass that isolates the android incompatible code. 

+#include "chrome/browser/ui/browser_list_observer.h"
+#include "chrome/browser/ui/tabs/tab_strip_model_observer.h"
+#endif  // !defined(OS_ANDROID)
+
+#include "components/keyed_service/core/keyed_service.h"
+#include "components/sync/model/syncable_service.h"
+#include "components/sync_sessions/local_session_event_router.h"
+
+namespace content {
+class WebContents;
+}  // namespace content
+
+class Profile;
+
+namespace sync_sessions {
+
+// WebContentsObserver-based implementation of LocalSessionEventRouter. This
+// class is responsible for notifying Sessions Sync when local tabs are
+// modified. It does this by aggregating the events pushed to it by individual

[Nicolas Zea, 2017/03/16 19:57:43]

it's not actually doing any aggregation right, just plumbing the events through?
Perhaps replace "aggregating" with "routing"?

[Patrick Noland, 2017/03/16 20:23:27]

Right, it's really just routing events from multiple sources, not aggregating. 

+// WebContentsObservers, which are scoped to a single WebContents/tab.
+class SyncSessionsWebContentsRouter : public LocalSessionEventRouter,
+                                      public KeyedService
+#if !defined(OS_ANDROID)
+                                      ,
+                                      public chrome::BrowserListObserver,
+                                      public TabStripModelObserver
+#endif  // !defined(OS_ANDROID)
+{
+ public:
+  explicit SyncSessionsWebContentsRouter(Profile* profile);
+
+  // Notify the router that the tab corresponding to |web_contents| has been
+  // modified in some way.
+  void NotifyTabModified(content::WebContents* web_contents);
+  // Inject a flare that can be used to start sync. See the comment for
+  // StartSyncFlare in syncable_service.h for more.
+  void InjectStartSyncFlare(syncer::SyncableService::StartSyncFlare flare);
+
+  // SessionsSyncManager::LocalEventRouter implementation.
+  void StartRoutingTo(LocalSessionEventHandler* handler) override;
+  void Stop() override;
+
+  // KeyedService implementation.
+  void Shutdown() override;
+
+ protected:
+  ~SyncSessionsWebContentsRouter() override;
+
+ private:
+#if !defined(OS_ANDROID)
+  // chrome::BrowserListObserver implementation.
+  void OnBrowserAdded(Browser* browser) override;
+  void OnBrowserRemoved(Browser* browser) override;
+  // TabStripModelObserver implementation.
+  void TabInsertedAt(TabStripModel* model,
+                     content::WebContents* web_contents,
+                     int index,
+                     bool foreground) override;
+#endif  // !defined(OS_ANDROID)
+
+  void OnFaviconsChanged(const std::set<GURL>& page_urls, const GURL& icon_url);
+
+  LocalSessionEventHandler* handler_;

[Nicolas Zea, 2017/03/16 19:57:43]

nit: may as well initialize this to nullptr here.

[Patrick Noland, 2017/03/17 00:20:10]

Done.

+  std::unique_ptr<base::CallbackList<void(const std::set<GURL>&,
+                                          const GURL&)>::Subscription>
+      favicon_changed_subscription_;
+  syncer::SyncableService::StartSyncFlare flare_;
+
+  DISALLOW_COPY_AND_ASSIGN(SyncSessionsWebContentsRouter);
+};
+
+}  // namespace sync_sessions
+
+#endif  // CHROME_BROWSER_SYNC_SESSIONS_SYNC_SESSIONS_WEB_CONTENTS_ROUTER_H_