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

chrome/browser/ui/startup/startup_tab_provider.cc

 // Copyright 2016 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/startup/startup_tab_provider.h"
 
 #include "base/metrics/histogram_macros.h"
 #include "build/build_config.h"
 #include "chrome/browser/first_run/first_run.h"
 #include "chrome/browser/profile_resetter/triggered_profile_resetter.h"
@@ skipping 38 matching lines @@
 #if defined(OS_CHROMEOS)
   return StartupTabs();
 #else
   if (!profile)
     return StartupTabs();
 
   bool is_first_run = first_run::IsChromeFirstRun();
   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();
   bool is_supervised_user = profile->IsSupervised();
 
 #if defined(OS_WIN)
   // Windows 10 has unique onboarding policies and content.
   if (base::win::GetVersion() >= base::win::VERSION_WIN10) {
     PrefService* local_state = g_browser_process->local_state();
     bool has_seen_win10_promo =
         local_state && local_state->GetBoolean(prefs::kHasSeenWin10PromoPage);
     // The set default browser operation can be disabled by the browser
     // distribution (e.g. SxS Canary), or by enterprise policy. In these cases,
     // the Win 10 promo page should not be displayed.
     bool disabled_by_enterprise_policy =
         local_state &&
         local_state->IsManagedPreference(
             prefs::kDefaultBrowserSettingEnabled) &&
         !local_state->GetBoolean(prefs::kDefaultBrowserSettingEnabled);
     bool set_default_browser_allowed =
         !disabled_by_enterprise_policy &&
         shell_integration::CanSetAsDefaultBrowser();
     bool is_default_browser =
         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)
 }
 
 StartupTabs StartupTabProviderImpl::GetDistributionFirstRunTabs(
     StartupBrowserCreator* browser_creator) const {
   if (!browser_creator)
     return StartupTabs();
   StartupTabs tabs = GetMasterPrefsTabsForState(
       first_run::IsChromeFirstRun(), browser_creator->first_run_tabs_);
   browser_creator->first_run_tabs_.clear();
@@ skipping 30 matching lines @@
     const base::CommandLine& command_line,
     Profile* profile) const {
   return GetNewTabPageTabsForState(
       StartupBrowserCreator::GetSessionStartupPref(command_line, profile));
 }
 
 // static
 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;
 }
 
 #if defined(OS_WIN)
 // static
 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,
     bool is_supervised_user) {
   StartupTabs tabs;
 
   if (is_supervised_user)
     return tabs;
 
   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;
 }
 #endif
 
 // static
 StartupTabs StartupTabProviderImpl::GetMasterPrefsTabsForState(
     bool is_first_run,
     const std::vector<GURL>& first_run_tabs) {
@@ skipping 77 matching lines @@
              ? net::AppendQueryParameter(url, "text", "faster")
              : url;
 }
 #endif
 
 // static
 GURL StartupTabProviderImpl::GetTriggeredResetSettingsUrl() {
   return GURL(
       chrome::GetSettingsUrl(chrome::kTriggeredResetProfileSettingsSubPage));
 }