[sync] WebContentsObserver based sessions notifications

chrome/browser/sync/sessions/sync_sessions_router_tab_helper.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_ROUTER_TAB_HELPER_H_
+#define CHROME_BROWSER_SYNC_SESSIONS_SYNC_SESSIONS_ROUTER_TAB_HELPER_H_
+
+#include "components/sessions/core/session_id.h"
+#include "content/public/browser/web_contents_observer.h"
+#include "content/public/browser/web_contents_user_data.h"
+
+namespace sync_sessions {
+
+class SyncSessionsWebContentsRouter;
+
+// TabHelper class that forwards tab-level WebContentsObserver events to a
+// global sessions router. The global router is responsible for forwarding these

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

it's not really global though right? It's per profile

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

Right.

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

Done.

+// events to sessions sync. This class also tracks the source tab id of its
+// corresponding tab, if available.
+// A TabHelper is a WebContentsObserver tied to the top level WebContents for a
+// browser tab.
+// https://chromium.googlesource.com/chromium/src/+/master/docs/tab_helpers.md
+

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

nit: remove newline?

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

Done.

+class SyncSessionsRouterTabHelper
+    : public content::WebContentsUserData<SyncSessionsRouterTabHelper>,
+      public content::WebContentsObserver {
+ public:
+  ~SyncSessionsRouterTabHelper() override;
+
+  static void CreateForWebContents(
+      content::WebContents* web_contents,
+      SyncSessionsWebContentsRouter* session_router);
+
+  // Get the tab id of the tab responsible for creating the tab this helper
+  // corresponds to. Returns -1 if there is no such tab.

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

maybe define -1 as an enum? E.g. SyncSessionRouterTabHelper::InvalidTabId?

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

Done, but in SyncedTabDelegate. This new definition replaces the definition in
TabNodePool.

+  SessionID::id_type source_tab_id() const { return source_tab_id_; }
+
+ private:
+  friend class content::WebContentsUserData<SyncSessionsRouterTabHelper>;
+  // router_ is a KeyedService and is guaranteed to outlive |this|.

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

newline above

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

Done.

+  SyncSessionsWebContentsRouter* router_;

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

Member variables like this should be defined below the methods. See
https://google.github.io/styleguide/cppguide.html#Class_Format

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

Done.

+  SessionID::id_type source_tab_id_;
+
+  // Set the tab id of the tab reponsible for creating the tab this helper
+  // corresponds to.
+  void SetSourceTabID(const SessionID::id_type id);

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

if you're using source_tab_id() above, for consistency it might be better to use
set_source_tab_id here.

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

Done.

+
+  // WebContentsObserver implementation.

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

Any reason these are private?

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

Just caution about future use. There's not currently a reason they should be
called by anyone except existing call sites for WebContentsObserver, so making
them private insures against that. 

+  void DidFinishNavigation(
+      content::NavigationHandle* navigation_handle) override;
+  void TitleWasSet(content::NavigationEntry* entry, bool explicit_set) override;
+  void WebContentsDestroyed() override;
+  void DidFinishLoad(content::RenderFrameHost* render_frame_host,
+                     const GURL& validated_url) override;
+  void DidOpenRequestedURL(content::WebContents* new_contents,
+                           content::RenderFrameHost* source_render_frame_host,
+                           const GURL& url,
+                           const content::Referrer& referrer,
+                           WindowOpenDisposition disposition,
+                           ui::PageTransition transition,
+                           bool started_from_context_menu,
+                           bool renderer_initiated) override;
+
+  void NotifyRouter();
+
+  explicit SyncSessionsRouterTabHelper(content::WebContents* web_contents,

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

Constructors should be defined before other methods.

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

Done.

+                                       SyncSessionsWebContentsRouter* router);
+
+  DISALLOW_COPY_AND_ASSIGN(SyncSessionsRouterTabHelper);
+};
+
+}  // namespace sync_sessions
+
+#endif  // CHROME_BROWSER_SYNC_SESSIONS_SYNC_SESSIONS_ROUTER_TAB_HELPER_H_