Reland "Avoid a proxy tab helper on Android for sync delegation"

This is a reland of 42c62276

The underlying issue in SyncedTabDelegateAndroid::GetSessionId()
has been reverted, which introduced an accidental behavioral
difference in the original version, causing crashes on Android
for placeholder tabs (i.e. tabs without WebContents).

TBR=avi@chromium.org,treib@chromium.org

Original change's description:
> Avoid a proxy tab helper on Android for sync delegation
>
> Prior to this patch, TabContentsSyncedTabDelegate was itself a tab
> helper (WebContentsUserData) that was created on all platforms, with the
> whole purpose to reuse a partial implementation of
> sync_sessions::SyncedTabDelegate.
>
> This was a bit weird because
> a) The life cycle diverges across platforms: created lazily on Android,
>    early otherwise.
> b) On Android there was no real need to register a tab helper.
> c) On Android, it's weird to reason about two tab delegates coexisting,
>    one of which is a proxy but also overrides some logic.
>
> Instead, let's make TabContentsSyncedTabDelegate a base class that is
> *NOT* a WebContentsUserData, and let platform-specific subclasses
> decide whether they want to register as WebContentsUserData, as well
> as which logic they need to override.
>
> This also removes code that is dead on some platforms, because the
> base class is abtract and doesn't implements all methods (as opposed
> to a proxy object which cannot be abstract).
>
> Bug: 851905
> Change-Id: I305ad85e2392fdd2ce428c0464b7750ac599babf
> Reviewed-on: https://chromium-review.googlesource.com/1097405
> Commit-Queue: Mikel Astiz <mastiz@chromium.org>
> Reviewed-by: Avi Drissman <avi@chromium.org>
> Reviewed-by: Marc Treib <treib@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#567213}

Bug: 851905
Change-Id: I9e060ecf1272fec1ceebd4353fd390b95e64b669
Reviewed-on: https://chromium-review.googlesource.com/1103717Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567966}

chrome/browser/media/android/remote/remote_media_player_manager.cc

@@ -7,6 +7,7 @@
 #include "chrome/browser/android/tab_android.h"
 #include "chrome/common/chrome_content_client.h"
 #include "content/common/media/media_player_messages_android.h"
+#include "content/public/browser/web_contents.h"
 #include "third_party/blink/public/platform/modules/remoteplayback/web_remote_playback_availability.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/gfx/android/java_bitmap.h"

chrome/browser/media/android/router/media_router_android.cc

@@ -15,6 +15,7 @@
 #include "chrome/browser/media/router/route_message_observer.h"
 #include "chrome/common/media_router/route_request_result.h"
 #include "content/public/browser/browser_context.h"
+#include "content/public/browser/web_contents.h"
 #include "content/public/common/presentation_connection_message.h"
 #include "url/gurl.h"
 

chrome/browser/sync/glue/synced_tab_delegate_android.cc

@@ -18,106 +18,44 @@ using content::NavigationEntry;
 
 namespace browser_sync {
 SyncedTabDelegateAndroid::SyncedTabDelegateAndroid(TabAndroid* tab_android)
-    : web_contents_(nullptr),
-      tab_android_(tab_android),
-      tab_contents_delegate_(nullptr) {
-}
+    : tab_android_(tab_android) {}
 
 SyncedTabDelegateAndroid::~SyncedTabDelegateAndroid() {}
 
 SessionID SyncedTabDelegateAndroid::GetWindowId() const {
-  return tab_contents_delegate_->GetWindowId();
+  return tab_android_->window_id();
 }
 
 SessionID SyncedTabDelegateAndroid::GetSessionId() const {
   return tab_android_->session_id();
 }
 
-bool SyncedTabDelegateAndroid::IsBeingDestroyed() const {
-  return tab_contents_delegate_->IsBeingDestroyed();
-}
-
-SessionID SyncedTabDelegateAndroid::GetSourceTabID() const {
-  return tab_contents_delegate_->GetSourceTabID();
-}
-
-std::string SyncedTabDelegateAndroid::GetExtensionAppId() const {
-  return tab_contents_delegate_->GetExtensionAppId();
-}
-
-bool SyncedTabDelegateAndroid::IsInitialBlankNavigation() const {
-  return tab_contents_delegate_->IsInitialBlankNavigation();
-}
-
-int SyncedTabDelegateAndroid::GetCurrentEntryIndex() const {
-  return tab_contents_delegate_->GetCurrentEntryIndex();
-}
-
-int SyncedTabDelegateAndroid::GetEntryCount() const {
-  return tab_contents_delegate_->GetEntryCount();
-}
-
-GURL SyncedTabDelegateAndroid::GetVirtualURLAtIndex(int i) const {
-  return tab_contents_delegate_->GetVirtualURLAtIndex(i);
-}
-
-GURL SyncedTabDelegateAndroid::GetFaviconURLAtIndex(int i) const {
-  return tab_contents_delegate_->GetFaviconURLAtIndex(i);
-}
-
-ui::PageTransition SyncedTabDelegateAndroid::GetTransitionAtIndex(int i) const {
-  return tab_contents_delegate_->GetTransitionAtIndex(i);
+bool SyncedTabDelegateAndroid::IsPlaceholderTab() const {
+  return web_contents() == nullptr;
 }
 
-void SyncedTabDelegateAndroid::GetSerializedNavigationAtIndex(
-    int i,
-    sessions::SerializedNavigationEntry* serialized_entry) const {
-  tab_contents_delegate_->GetSerializedNavigationAtIndex(i, serialized_entry);
+int SyncedTabDelegateAndroid::GetSyncId() const {
+  return tab_android_->GetSyncId();
 }
 
-bool SyncedTabDelegateAndroid::IsPlaceholderTab() const {
-  return web_contents_ == nullptr;
+void SyncedTabDelegateAndroid::SetSyncId(int sync_id) {
+  tab_android_->SetSyncId(sync_id);
 }
 
 void SyncedTabDelegateAndroid::SetWebContents(
     content::WebContents* web_contents,
     content::WebContents* source_web_contents) {
-  web_contents_ = web_contents;
-  TabContentsSyncedTabDelegate::CreateForWebContents(web_contents_);
-  // Store the TabContentsSyncedTabDelegate object that was created.
-  tab_contents_delegate_ =
-      TabContentsSyncedTabDelegate::FromWebContents(web_contents_);
+  TabContentsSyncedTabDelegate::SetWebContents(web_contents);
+
   if (source_web_contents) {
     sync_sessions::SyncSessionsRouterTabHelper::FromWebContents(
         source_web_contents)
-        ->SetSourceTabIdForChild(web_contents_);
+        ->SetSourceTabIdForChild(web_contents);
   }
 }
 
 void SyncedTabDelegateAndroid::ResetWebContents() {
-  web_contents_ = nullptr;
-}
-
-bool SyncedTabDelegateAndroid::ProfileIsSupervised() const {
-  return tab_contents_delegate_->ProfileIsSupervised();
-}
-
-const std::vector<std::unique_ptr<const sessions::SerializedNavigationEntry>>*
-SyncedTabDelegateAndroid::GetBlockedNavigations() const {
-  return tab_contents_delegate_->GetBlockedNavigations();
-}
-
-int SyncedTabDelegateAndroid::GetSyncId() const {
-  return tab_android_->GetSyncId();
-}
-
-void SyncedTabDelegateAndroid::SetSyncId(int sync_id) {
-  tab_android_->SetSyncId(sync_id);
-}
-
-bool SyncedTabDelegateAndroid::ShouldSync(
-    sync_sessions::SyncSessionsClient* sessions_client) {
-  return tab_contents_delegate_->ShouldSync(sessions_client);
+  TabContentsSyncedTabDelegate::SetWebContents(nullptr);
 }
 
 }  // namespace browser_sync

chrome/browser/sync/glue/synced_tab_delegate_android.h

@@ -9,24 +9,22 @@
 #include <string>
 #include <vector>
 
-#include "base/compiler_specific.h"
 #include "base/macros.h"
-#include "components/sync_sessions/synced_tab_delegate.h"
-#include "content/public/browser/web_contents_user_data.h"
+#include "chrome/browser/ui/sync/tab_contents_synced_tab_delegate.h"
 
 namespace content {
 class WebContents;
 }
 
 class TabAndroid;
-class TabContentsSyncedTabDelegate;
 
 namespace browser_sync {
 // On Android a tab can exist even without web contents.
 
-// SyncedTabDelegateAndroid wraps TabContentsSyncedTabDelegate and provides
-// a method to set web contents later when tab is brought to memory.
-class SyncedTabDelegateAndroid : public sync_sessions::SyncedTabDelegate {
+// SyncedTabDelegateAndroid specializes TabContentsSyncedTabDelegate with
+// support for setting web contents on a late stage (for placeholder tabs),
+// when the tab is brought to memory.
+class SyncedTabDelegateAndroid : public TabContentsSyncedTabDelegate {
  public:
   explicit SyncedTabDelegateAndroid(TabAndroid* owning_tab_);
   ~SyncedTabDelegateAndroid() override;
@@ -34,38 +32,18 @@ class SyncedTabDelegateAndroid : public sync_sessions::SyncedTabDelegate {
   // SyncedTabDelegate:
   SessionID GetWindowId() const override;
   SessionID GetSessionId() const override;
-  bool IsBeingDestroyed() const override;
-  SessionID GetSourceTabID() const override;
-  std::string GetExtensionAppId() const override;
-  bool IsInitialBlankNavigation() const override;
-  int GetCurrentEntryIndex() const override;
-  int GetEntryCount() const override;
-  GURL GetVirtualURLAtIndex(int i) const override;
-  GURL GetFaviconURLAtIndex(int i) const override;
-  ui::PageTransition GetTransitionAtIndex(int i) const override;
-  void GetSerializedNavigationAtIndex(
-      int i,
-      sessions::SerializedNavigationEntry* serialized_entry) const override;
   bool IsPlaceholderTab() const override;
   int GetSyncId() const override;
   void SetSyncId(int sync_id) override;
-  bool ShouldSync(sync_sessions::SyncSessionsClient* sessions_client) override;
-  bool ProfileIsSupervised() const override;
-  const std::vector<std::unique_ptr<const sessions::SerializedNavigationEntry>>*
-  GetBlockedNavigations() const override;
 
-  // Set the web contents for this tab. Also creates
-  // TabContentsSyncedTabDelegate for this tab and handles source tab id
-  // initialization.
-  virtual void SetWebContents(content::WebContents* web_contents,
-                              content::WebContents* source_web_contents);
+  // Set the web contents for this tab and handles source tab ID initialization.
+  void SetWebContents(content::WebContents* web_contents,
+                      content::WebContents* source_web_contents);
   // Set web contents to null.
-  virtual void ResetWebContents();
+  void ResetWebContents();
 
  private:
-  content::WebContents* web_contents_;
   TabAndroid* tab_android_;
-  TabContentsSyncedTabDelegate* tab_contents_delegate_;
 
   DISALLOW_COPY_AND_ASSIGN(SyncedTabDelegateAndroid);
 };

chrome/browser/sync/sessions/sync_sessions_router_tab_helper_unittest.cc

@@ -9,7 +9,7 @@
 
 #include "chrome/browser/sync/sessions/sync_sessions_web_contents_router.h"
 #include "chrome/browser/sync/sessions/sync_sessions_web_contents_router_factory.h"
-#include "chrome/browser/ui/sync/tab_contents_synced_tab_delegate.h"
+#include "chrome/browser/ui/sync/browser_synced_tab_delegate.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "chrome/test/base/testing_profile.h"
 #include "content/public/browser/navigation_handle.h"
@@ -38,7 +38,7 @@ class FakeLocalSessionEventHandler : public LocalSessionEventHandler {
   }
 
  private:
-  bool was_notified_;
+  bool was_notified_ = false;
 };
 
 class SyncSessionsRouterTabHelperTest : public ChromeRenderViewHostTestHarness {
@@ -56,7 +56,7 @@ class SyncSessionsRouterTabHelperTest : public ChromeRenderViewHostTestHarness {
     SyncSessionsRouterTabHelper::CreateForWebContents(web_contents(), router_);
     router_->StartRoutingTo(handler());
 
-    TabContentsSyncedTabDelegate::CreateForWebContents(web_contents());
+    BrowserSyncedTabDelegate::CreateForWebContents(web_contents());
     NavigateAndCommit(GURL("about:blank"));
   }
 
@@ -72,7 +72,7 @@ TEST_F(SyncSessionsRouterTabHelperTest, SubframeNavigationsIgnored) {
   SyncSessionsRouterTabHelper* helper =
       SyncSessionsRouterTabHelper::FromWebContents(web_contents());
 
-  EXPECT_TRUE(handler()->was_notified_since_last_call());
+  ASSERT_TRUE(handler()->was_notified_since_last_call());
 
   content::RenderFrameHost* child_rfh =
       content::RenderFrameHostTester::For(main_rfh())->AppendChild("subframe");

chrome/browser/sync/sessions/sync_sessions_web_contents_router.cc

@@ -8,11 +8,10 @@
 #include "chrome/browser/profiles/profile.h"
 #if !defined(OS_ANDROID)
 #include "chrome/browser/sync/sessions/browser_list_router_helper.h"
+#include "chrome/browser/ui/sync/browser_synced_tab_delegate.h"
 #else
 #include "chrome/browser/android/tab_android.h"
 #endif  // !defined(OS_ANDROID)
-#include "chrome/browser/ui/sync/tab_contents_synced_tab_delegate.h"
-#include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "components/history/core/browser/history_service.h"
 #include "components/sync_sessions/sync_sessions_client.h"
 #include "components/sync_sessions/synced_tab_delegate.h"
@@ -28,7 +27,7 @@ SyncedTabDelegate* GetSyncedTabDelegateFromWebContents(
   return tab ? tab->GetSyncedTabDelegate() : nullptr;
 #else
   SyncedTabDelegate* delegate =
-      TabContentsSyncedTabDelegate::FromWebContents(web_contents);
+      BrowserSyncedTabDelegate::FromWebContents(web_contents);
   return delegate;
 #endif
 }

chrome/browser/sync/sessions/sync_sessions_web_contents_router_unittest.cc

@@ -3,8 +3,10 @@
 // found in the LICENSE file.
 
 #include "chrome/browser/sync/sessions/sync_sessions_web_contents_router.h"
+
+#include "build/build_config.h"
 #include "chrome/browser/sync/sessions/sync_sessions_web_contents_router_factory.h"
-#include "chrome/browser/ui/sync/tab_contents_synced_tab_delegate.h"
+#include "chrome/browser/ui/sync/browser_synced_tab_delegate.h"
 #include "chrome/test/base/testing_profile.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "content/public/test/web_contents_tester.h"
@@ -47,6 +49,10 @@ class SyncSessionsWebContentsRouterTest : public testing::Test {
   std::unique_ptr<content::WebContents> test_contents_;
 };
 
+// Disabled on android due to complexity of creating a full TabAndroid object
+// for a unit test. The logic being tested here isn't directly affected by
+// platform-specific peculiarities.
+#if !defined(OS_ANDROID)
 TEST_F(SyncSessionsWebContentsRouterTest, FlareNotRun) {
   StartSyncFlareMock mock;
   router()->InjectStartSyncFlare(
@@ -56,7 +62,7 @@ TEST_F(SyncSessionsWebContentsRouterTest, FlareNotRun) {
   router()->NotifyTabModified(test_contents(), false);
   EXPECT_FALSE(mock.was_run());
 
-  TabContentsSyncedTabDelegate::CreateForWebContents(test_contents());
+  BrowserSyncedTabDelegate::CreateForWebContents(test_contents());
 
   // There's a delegate for the tab, but it's not a load completed event, so the
   // flare still shouldn't run.
@@ -66,16 +72,12 @@ TEST_F(SyncSessionsWebContentsRouterTest, FlareNotRun) {
 
 // Make sure we don't crash when there's not a flare.
 TEST_F(SyncSessionsWebContentsRouterTest, FlareNotSet) {
-  TabContentsSyncedTabDelegate::CreateForWebContents(test_contents());
+  BrowserSyncedTabDelegate::CreateForWebContents(test_contents());
   router()->NotifyTabModified(test_contents(), false);
 }
 
-// Disabled on android due to complexity of creating a full TabAndroid object
-// for a unit test. The logic being tested here isn't directly affected by
-// platform-specific peculiarities.
-#if !defined(OS_ANDROID)
 TEST_F(SyncSessionsWebContentsRouterTest, FlareRunsForLoadCompleted) {
-  TabContentsSyncedTabDelegate::CreateForWebContents(test_contents());
+  BrowserSyncedTabDelegate::CreateForWebContents(test_contents());
 
   StartSyncFlareMock mock;
   router()->InjectStartSyncFlare(

chrome/browser/ui/BUILD.gn

@@ -1474,6 +1474,8 @@ split_static_library("ui") {
       "startup/startup_tab_provider.cc",
       "startup/startup_tab_provider.h",
       "startup/startup_types.h",
+      "sync/browser_synced_tab_delegate.cc",
+      "sync/browser_synced_tab_delegate.h",
       "sync/browser_synced_window_delegate.cc",
       "sync/browser_synced_window_delegate.h",
       "sync/browser_synced_window_delegates_getter.cc",

chrome/browser/ui/sync/browser_synced_tab_delegate.cc

+// Copyright 2018 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.
+
+#include "chrome/browser/ui/sync/browser_synced_tab_delegate.h"
+
+#include "chrome/browser/sessions/session_tab_helper.h"
+#include "components/sync_sessions/tab_node_pool.h"
+
+DEFINE_WEB_CONTENTS_USER_DATA_KEY(BrowserSyncedTabDelegate);
+
+BrowserSyncedTabDelegate::BrowserSyncedTabDelegate(
+    content::WebContents* web_contents)
+    : sync_id_(sync_sessions::TabNodePool::kInvalidTabNodeID) {
+  SetWebContents(web_contents);
+}
+
+BrowserSyncedTabDelegate::~BrowserSyncedTabDelegate() {}
+
+SessionID BrowserSyncedTabDelegate::GetWindowId() const {
+  return SessionTabHelper::FromWebContents(web_contents())->window_id();
+}
+
+SessionID BrowserSyncedTabDelegate::GetSessionId() const {
+  return SessionTabHelper::FromWebContents(web_contents())->session_id();
+}
+
+bool BrowserSyncedTabDelegate::IsPlaceholderTab() const {
+  return false;
+}
+
+int BrowserSyncedTabDelegate::GetSyncId() const {
+  return sync_id_;
+}
+
+void BrowserSyncedTabDelegate::SetSyncId(int sync_id) {
+  sync_id_ = sync_id;
+}

chrome/browser/ui/sync/browser_synced_tab_delegate.h

+// Copyright 2018 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_UI_SYNC_BROWSER_SYNCED_TAB_DELEGATE_H_
+#define CHROME_BROWSER_UI_SYNC_BROWSER_SYNCED_TAB_DELEGATE_H_
+
+#include "base/macros.h"
+#include "chrome/browser/ui/sync/tab_contents_synced_tab_delegate.h"
+#include "components/sessions/core/session_id.h"
+#include "content/public/browser/web_contents_user_data.h"
+
+namespace content {
+class WebContents;
+}
+
+// A BrowserSyncedTabDelegate is the Browser-based implementation of
+// SyncedTabDelegate.
+class BrowserSyncedTabDelegate
+    : public TabContentsSyncedTabDelegate,
+      public content::WebContentsUserData<BrowserSyncedTabDelegate> {
+ public:
+  ~BrowserSyncedTabDelegate() override;
+
+  // SyncedTabDelegate:
+  SessionID GetWindowId() const override;
+  SessionID GetSessionId() const override;
+  bool IsPlaceholderTab() const override;
+  int GetSyncId() const override;
+  void SetSyncId(int sync_id) override;
+
+ private:
+  explicit BrowserSyncedTabDelegate(content::WebContents* web_contents);
+  friend class content::WebContentsUserData<BrowserSyncedTabDelegate>;
+
+  int sync_id_;
+
+  DISALLOW_COPY_AND_ASSIGN(BrowserSyncedTabDelegate);
+};
+
+#endif  // CHROME_BROWSER_UI_SYNC_BROWSER_SYNCED_TAB_DELEGATE_H_

chrome/browser/ui/sync/browser_synced_window_delegate.cc

@@ -8,7 +8,7 @@
 
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
-#include "chrome/browser/ui/sync/tab_contents_synced_tab_delegate.h"
+#include "chrome/browser/ui/sync/browser_synced_tab_delegate.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 
 BrowserSyncedWindowDelegate::BrowserSyncedWindowDelegate(Browser* browser)
@@ -30,7 +30,7 @@ bool BrowserSyncedWindowDelegate::IsTabPinned(
 
 sync_sessions::SyncedTabDelegate* BrowserSyncedWindowDelegate::GetTabAt(
     int index) const {
-  return TabContentsSyncedTabDelegate::FromWebContents(
+  return BrowserSyncedTabDelegate::FromWebContents(
       browser_->tab_strip_model()->GetWebContentsAt(index));
 }
 

chrome/browser/ui/sync/tab_contents_synced_tab_delegate.cc

@@ -6,14 +6,12 @@
 
 #include "base/memory/ref_counted.h"
 #include "chrome/browser/profiles/profile.h"
-#include "chrome/browser/sessions/session_tab_helper.h"
 #include "chrome/browser/sync/sessions/sync_sessions_router_tab_helper.h"
 #include "chrome/common/buildflags.h"
 #include "components/sessions/content/content_serialized_navigation_builder.h"
 #include "components/sync_sessions/sync_sessions_client.h"
 #include "components/sync_sessions/synced_window_delegate.h"
 #include "components/sync_sessions/synced_window_delegates_getter.h"
-#include "components/sync_sessions/tab_node_pool.h"
 #include "content/public/browser/favicon_status.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_entry.h"
@@ -31,8 +29,6 @@
 
 using content::NavigationEntry;
 
-DEFINE_WEB_CONTENTS_USER_DATA_KEY(TabContentsSyncedTabDelegate);
-
 namespace {
 
 // Helper to access the correct NavigationEntry, accounting for pending entries.
@@ -47,21 +43,11 @@ NavigationEntry* GetPossiblyPendingEntryAtIndex(
 
 }  // namespace
 
-TabContentsSyncedTabDelegate::TabContentsSyncedTabDelegate(
-    content::WebContents* web_contents)
-    : web_contents_(web_contents),
-      sync_session_id_(sync_sessions::TabNodePool::kInvalidTabNodeID) {}
+TabContentsSyncedTabDelegate::TabContentsSyncedTabDelegate()
+    : web_contents_(nullptr) {}
 
 TabContentsSyncedTabDelegate::~TabContentsSyncedTabDelegate() {}
 
-SessionID TabContentsSyncedTabDelegate::GetWindowId() const {
-  return SessionTabHelper::FromWebContents(web_contents_)->window_id();
-}
-
-SessionID TabContentsSyncedTabDelegate::GetSessionId() const {
-  return SessionTabHelper::FromWebContents(web_contents_)->session_id();
-}
-
 bool TabContentsSyncedTabDelegate::IsBeingDestroyed() const {
   return web_contents_->IsBeingDestroyed();
 }
@@ -142,18 +128,6 @@ TabContentsSyncedTabDelegate::GetBlockedNavigations() const {
 #endif
 }
 
-bool TabContentsSyncedTabDelegate::IsPlaceholderTab() const {
-  return false;
-}
-
-int TabContentsSyncedTabDelegate::GetSyncId() const {
-  return sync_session_id_;
-}
-
-void TabContentsSyncedTabDelegate::SetSyncId(int sync_id) {
-  sync_session_id_ = sync_id;
-}
-
 bool TabContentsSyncedTabDelegate::ShouldSync(
     sync_sessions::SyncSessionsClient* sessions_client) {
   if (sessions_client->GetSyncedWindowDelegatesGetter()->FindById(
@@ -185,3 +159,16 @@ SessionID TabContentsSyncedTabDelegate::GetSourceTabID() const {
           web_contents_);
   return helper->source_tab_id();
 }
+
+const content::WebContents* TabContentsSyncedTabDelegate::web_contents() const {
+  return web_contents_;
+}
+
+content::WebContents* TabContentsSyncedTabDelegate::web_contents() {
+  return web_contents_;
+}
+
+void TabContentsSyncedTabDelegate::SetWebContents(
+    content::WebContents* web_contents) {
+  web_contents_ = web_contents;
+}

chrome/browser/ui/sync/tab_contents_synced_tab_delegate.h

@@ -9,25 +9,22 @@
 #include <string>
 #include <vector>
 
-#include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "components/sessions/core/session_id.h"
 #include "components/sync_sessions/synced_tab_delegate.h"
-#include "content/public/browser/web_contents_user_data.h"
 
 namespace content {
 class WebContents;
 }
 
-class TabContentsSyncedTabDelegate
-    : public sync_sessions::SyncedTabDelegate,
-      public content::WebContentsUserData<TabContentsSyncedTabDelegate> {
+// Partial implementation of SyncedTabDelegate for the cases where the tab has
+// (either initially or late) a WebContents.
+class TabContentsSyncedTabDelegate : public sync_sessions::SyncedTabDelegate {
  public:
+  TabContentsSyncedTabDelegate();
   ~TabContentsSyncedTabDelegate() override;
 
   // SyncedTabDelegate:
-  SessionID GetWindowId() const override;
-  SessionID GetSessionId() const override;
   bool IsBeingDestroyed() const override;
   std::string GetExtensionAppId() const override;
   bool IsInitialBlankNavigation() const override;
@@ -42,18 +39,16 @@ class TabContentsSyncedTabDelegate
   bool ProfileIsSupervised() const override;
   const std::vector<std::unique_ptr<const sessions::SerializedNavigationEntry>>*
   GetBlockedNavigations() const override;
-  bool IsPlaceholderTab() const override;
-  int GetSyncId() const override;
-  void SetSyncId(int sync_id) override;
   bool ShouldSync(sync_sessions::SyncSessionsClient* sessions_client) override;
   SessionID GetSourceTabID() const override;
 
- private:
-  explicit TabContentsSyncedTabDelegate(content::WebContents* web_contents);
-  friend class content::WebContentsUserData<TabContentsSyncedTabDelegate>;
+ protected:
+  const content::WebContents* web_contents() const;
+  content::WebContents* web_contents();
+  void SetWebContents(content::WebContents* web_contents);
 
+ private:
   content::WebContents* web_contents_;
-  int sync_session_id_;
 
   DISALLOW_COPY_AND_ASSIGN(TabContentsSyncedTabDelegate);
 };

chrome/browser/ui/sync/tab_contents_synced_tab_delegate_unittest.cc

@@ -5,10 +5,26 @@
 #include "chrome/browser/ui/sync/tab_contents_synced_tab_delegate.h"
 
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
+#include "content/public/browser/web_contents.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace {
 
+class TestSyncedTabDelegate : public TabContentsSyncedTabDelegate {
+ public:
+  explicit TestSyncedTabDelegate(content::WebContents* web_contents) {
+    SetWebContents(web_contents);
+  }
+
+  ~TestSyncedTabDelegate() override {}
+
+  SessionID GetWindowId() const override { return SessionID::InvalidValue(); }
+  SessionID GetSessionId() const override { return SessionID::InvalidValue(); }
+  bool IsPlaceholderTab() const override { return false; }
+  int GetSyncId() const override { return -1; }
+  void SetSyncId(int sync_id) override {}
+};
+
 class TabContentsSyncedTabDelegateTest
     : public ChromeRenderViewHostTestHarness {
  public:
@@ -19,28 +35,23 @@ class TabContentsSyncedTabDelegateTest
     content::RenderViewHostTestHarness::SetUp();
 
     NavigateAndCommit(GURL("about:blank"));
-    TabContentsSyncedTabDelegate::CreateForWebContents(web_contents());
   }
 };
 
 TEST_F(TabContentsSyncedTabDelegateTest, InvalidEntryIndexReturnsDefault) {
   std::unique_ptr<content::WebContents> web_contents(CreateTestWebContents());
-
-  TabContentsSyncedTabDelegate::CreateForWebContents(web_contents.get());
-
-  TabContentsSyncedTabDelegate* delegate =
-      TabContentsSyncedTabDelegate::FromWebContents(web_contents.get());
+  TestSyncedTabDelegate delegate(web_contents.get());
 
   // -1 and 2 are invalid indices because there's only one navigation
   // recorded(the initial one to "about:blank")
-  EXPECT_EQ(delegate->GetFaviconURLAtIndex(-1), GURL());
-  EXPECT_EQ(delegate->GetFaviconURLAtIndex(2), GURL());
+  EXPECT_EQ(delegate.GetFaviconURLAtIndex(-1), GURL());
+  EXPECT_EQ(delegate.GetFaviconURLAtIndex(2), GURL());
 
   EXPECT_TRUE(
-      PageTransitionCoreTypeIs(delegate->GetTransitionAtIndex(-1),
+      PageTransitionCoreTypeIs(delegate.GetTransitionAtIndex(-1),
                                ui::PageTransition::PAGE_TRANSITION_LINK));
   EXPECT_TRUE(
-      PageTransitionCoreTypeIs(delegate->GetTransitionAtIndex(2),
+      PageTransitionCoreTypeIs(delegate.GetTransitionAtIndex(2),
                                ui::PageTransition::PAGE_TRANSITION_LINK));
 }
 

chrome/browser/ui/tab_helpers.cc

@@ -108,7 +108,7 @@
 #include "chrome/browser/ui/hung_plugin_tab_helper.h"
 #include "chrome/browser/ui/sad_tab_helper.h"
 #include "chrome/browser/ui/search/search_tab_helper.h"
-#include "chrome/browser/ui/sync/tab_contents_synced_tab_delegate.h"
+#include "chrome/browser/ui/sync/browser_synced_tab_delegate.h"
 #include "components/pdf/browser/pdf_web_contents_helper.h"
 #include "components/web_modal/web_contents_modal_dialog_manager.h"
 #include "components/zoom/zoom_controller.h"
@@ -276,6 +276,7 @@ void TabHelpers::AttachTabHelpers(WebContents* web_contents) {
   ViewAndroidHelper::CreateForWebContents(web_contents);
 #else
   BookmarkTabHelper::CreateForWebContents(web_contents);
+  BrowserSyncedTabDelegate::CreateForWebContents(web_contents);
   extensions::ChromeExtensionWebContentsObserver::CreateForWebContents(
       web_contents);
   extensions::WebNavigationTabObserver::CreateForWebContents(web_contents);
@@ -292,7 +293,6 @@ void TabHelpers::AttachTabHelpers(WebContents* web_contents) {
   safe_browsing::SafeBrowsingNavigationObserver::MaybeCreateForWebContents(
       web_contents);
   SearchTabHelper::CreateForWebContents(web_contents);
-  TabContentsSyncedTabDelegate::CreateForWebContents(web_contents);
   TabDialogs::CreateForWebContents(web_contents);
   ThumbnailTabHelper::CreateForWebContents(web_contents);
   web_modal::WebContentsModalDialogManager::CreateForWebContents(web_contents);