[Desktop FRE] Do not show Welcome if sign-in disabled

chrome/browser/ui/startup/startup_tab_provider.cc

Index: chrome/browser/ui/startup/startup_tab_provider.cc
diff --git a/chrome/browser/ui/startup/startup_tab_provider.cc b/chrome/browser/ui/startup/startup_tab_provider.cc
index a68ea46ac86a64903ab80f214016b52b143d2aaa..f8a25f3b2105ad542341855e720e45a2386b0ad8 100644
--- a/chrome/browser/ui/startup/startup_tab_provider.cc
+++ b/chrome/browser/ui/startup/startup_tab_provider.cc
@@ -56,6 +56,7 @@ StartupTabs StartupTabProviderImpl::GetOnboardingTabs(Profile* profile) const {
   PrefService* prefs = profile->GetPrefs();
   bool has_seen_welcome_page =
       prefs && prefs->GetBoolean(prefs::kHasSeenWelcomePage);
+  bool is_signin_allowed = profile->IsSyncAllowed();

[Peter Kasting, 2017/04/05 23:04:11]

Naively it seems like this should be checking signin_manager->IsSigninAllowed().
 How does the behavior of these two differ?  If this code is correct and that
code would not be, this definitely deserves a comment as to why.

[tmartino, 2017/04/06 20:43:05]

In fact, the policy backing IsSigninAllowed is deprecated (see:
https://www.chromium.org/administrators/policy-list-3#SigninAllowed ), and
SyncDisabled is the suggested replacement. I don't quite feel right adding a
comment to the effect of "use this thing because this other thing is
deprecated," because the comment is likely to be outlive the code it's
describing.

   SigninManagerBase* signin_manager =
       SigninManagerFactory::GetForProfile(profile);
   bool is_signed_in = signin_manager && signin_manager->IsAuthenticated();
@@ -82,13 +83,15 @@ StartupTabs StartupTabProviderImpl::GetOnboardingTabs(Profile* profile) const {
         g_browser_process->CachedDefaultWebClientState() ==
         shell_integration::IS_DEFAULT;
     return GetWin10OnboardingTabsForState(
-        is_first_run, has_seen_welcome_page, has_seen_win10_promo, is_signed_in,
-        set_default_browser_allowed, is_default_browser, is_supervised_user);
+        is_first_run, has_seen_welcome_page, has_seen_win10_promo,
+        is_signin_allowed, is_signed_in, set_default_browser_allowed,
+        is_default_browser, is_supervised_user);
   }
 #endif  // defined(OS_WIN)
 
   return GetStandardOnboardingTabsForState(is_first_run, has_seen_welcome_page,
-                                           is_signed_in, is_supervised_user);
+                                           is_signin_allowed, is_signed_in,
+                                           is_supervised_user);
 #endif  // defined(OS_CHROMEOS)
 }
 
@@ -139,10 +142,12 @@ StartupTabs StartupTabProviderImpl::GetNewTabPageTabs(
 StartupTabs StartupTabProviderImpl::GetStandardOnboardingTabsForState(
     bool is_first_run,
     bool has_seen_welcome_page,
+    bool is_signin_allowed,
     bool is_signed_in,
     bool is_supervised_user) {
   StartupTabs tabs;
-  if (!has_seen_welcome_page && !is_signed_in && !is_supervised_user)
+  if (!has_seen_welcome_page && is_signin_allowed && !is_signed_in &&
+      !is_supervised_user)
     tabs.emplace_back(GetWelcomePageUrl(!is_first_run), false);
   return tabs;
 }
@@ -153,6 +158,7 @@ StartupTabs StartupTabProviderImpl::GetWin10OnboardingTabsForState(
     bool is_first_run,
     bool has_seen_welcome_page,
     bool has_seen_win10_promo,
+    bool is_signin_allowed,
     bool is_signed_in,
     bool set_default_browser_allowed,
     bool is_default_browser,
@@ -165,7 +171,7 @@ StartupTabs StartupTabProviderImpl::GetWin10OnboardingTabsForState(
   if (set_default_browser_allowed && !has_seen_win10_promo &&
       !is_default_browser) {
     tabs.emplace_back(GetWin10WelcomePageUrl(!is_first_run), false);
-  } else if (!has_seen_welcome_page && !is_signed_in) {
+  } else if (!has_seen_welcome_page && is_signin_allowed && !is_signed_in) {

[Peter Kasting, 2017/04/05 23:04:11]

Nit: This function could be simplified to something like:

  if (set_default_browser_allowed && !has_seen_win10_promo && 
      !is_default_browser && !is_supervised_user)
    return {StartupTab(GetWin10WelcomePageUrl(!is_first_run), false);

  return GetStandardOnboardingTabsForState(...);

Obviously, there are other ways to refactor, my overall point is basically "try
to make use of the cross-platform variant instead of duplicating code".

[tmartino, 2017/04/06 20:43:05]

Ack. Will follow up with a CL that reorganizes this code, but I want to land
this as it's a Stable Block for M58.

     tabs.emplace_back(GetWelcomePageUrl(!is_first_run), false);
   }
   return tabs;