[sync] Prevent sessions flare on startup

chrome/browser/sync/sessions/sync_sessions_web_contents_router.cc

 // 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.
 
 #include "chrome/browser/sync/sessions/sync_sessions_web_contents_router.h"
 
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #if !defined(OS_ANDROID)
 #include "chrome/browser/sync/sessions/browser_list_router_helper.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"
 
 namespace sync_sessions {
 
 namespace {
 
 SyncedTabDelegate* GetSyncedTabDelegateFromWebContents(
     content::WebContents* web_contents) {
 #if defined(OS_ANDROID)
   TabAndroid* tab = TabAndroid::FromWebContents(web_contents);
@@ skipping 22 matching lines @@
 #endif  // !defined(OS_ANDROID)
 }
 
 SyncSessionsWebContentsRouter::~SyncSessionsWebContentsRouter() {}
 
 void SyncSessionsWebContentsRouter::NotifyTabModified(
     content::WebContents* web_contents) {
   if (handler_ && web_contents) {
     SyncedTabDelegate* delegate =
         GetSyncedTabDelegateFromWebContents(web_contents);
-    if (delegate)
+    if (delegate) {
       handler_->OnLocalTabModified(delegate);
-  }
+      if (sessions_client_ && !delegate->ShouldSync(sessions_client_))
+        return;

[skym, 2017/04/18 15:50:51]

This early return feels really oddly structured.

[Patrick Noland, 2017/04/18 22:37:41]

Done.

+    }
 
-  if (!flare_.is_null()) {
-    flare_.Run(syncer::SESSIONS);
-    flare_.Reset();
+    if (!flare_.is_null()) {

[skym, 2017/04/18 15:50:51]

This looks broken to me. My understanding is that we're given the |handler_| at
the end of SessionsSyncManager::MergeDataAndStartSyncing. If your flare call is
inside if (handler_) it will never execute when it's actually needed.

[Patrick Noland, 2017/04/18 22:37:41]

Done.

+      flare_.Run(syncer::SESSIONS);
+      flare_.Reset();
+    }
   }
 }
 
 void SyncSessionsWebContentsRouter::InjectStartSyncFlare(
     syncer::SyncableService::StartSyncFlare flare) {
   flare_ = flare;
 }
 
 void SyncSessionsWebContentsRouter::StartRoutingTo(
     LocalSessionEventHandler* handler) {
@@ skipping 13 matching lines @@
 
 void SyncSessionsWebContentsRouter::Shutdown() {
   favicon_changed_subscription_.reset();
 
 #if !defined(OS_ANDROID)
   browser_list_helper_.reset();
 #endif  // !defined(OS_ANDROID)
 }
 
 }  // namespace sync_sessions