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

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

BUG=702576
Review-Url: https://codereview.chromium.org/2786883008
Cr-Commit-Position: refs/heads/master@{#462636}
Committed: https://chromium.googlesource.com/chromium/src/+/cab3dfe03ca689d5eb085e7a06418e22cbf31a42

[tmartino, 2017-03-31 20:49:09]

The CQ bit was checked by tmartino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 20:49:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 21:35:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-31 21:35:28]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[tmartino, 2017-03-31 22:13:53]

The CQ bit was checked by tmartino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 22:14:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tmartino, 2017-03-31 22:47:35]

The CQ bit was checked by tmartino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 22:48:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tmartino, 2017-03-31 22:48:13]

The CQ bit was unchecked by tmartino@chromium.org

[tmartino, 2017-03-31 22:48:16]

The CQ bit was checked by tmartino@chromium.org to run a CQ dry run

[tmartino, 2017-03-31 22:49:06]

The CQ bit was checked by tmartino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 22:49:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tmartino, 2017-03-31 22:49:11]

The CQ bit was unchecked by tmartino@chromium.org

[tmartino, 2017-03-31 22:49:55]

The CQ bit was checked by tmartino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 22:50:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 23:40:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-31 23:40:44]

Dry run: This issue passed the CQ dry run.

[tmartino, 2017-04-04 19:47:39]

The CQ bit was checked by tmartino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 19:48:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-04 20:38:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-04 20:38:31]

Dry run: This issue passed the CQ dry run.

[tmartino, 2017-04-04 20:38:57]

tmartino@chromium.org changed reviewers:
+ pkasting@chromium.org

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

LGTM with one substantive comment

https://codereview.chromium.org/2786883008/diff/60001/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_tab_provider.cc (right):

https://codereview.chromium.org/2786883008/diff/60001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_tab_provider.cc:59: bool is_signin_allowed =
profile->IsSyncAllowed();
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.

https://codereview.chromium.org/2786883008/diff/60001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_tab_provider.cc:174: } else if
(!has_seen_welcome_page && is_signin_allowed && !is_signed_in) {
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]

https://codereview.chromium.org/2786883008/diff/60001/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_tab_provider.cc (right):

https://codereview.chromium.org/2786883008/diff/60001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_tab_provider.cc:59: bool is_signin_allowed =
profile->IsSyncAllowed();
On 2017/04/05 at 23:04:11, Peter Kasting wrote:
> 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.

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.

https://codereview.chromium.org/2786883008/diff/60001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_tab_provider.cc:174: } else if
(!has_seen_welcome_page && is_signin_allowed && !is_signed_in) {
On 2017/04/05 at 23:04:11, Peter Kasting wrote:
> 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".

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.

[tmartino, 2017-04-06 20:43:35]

The CQ bit was checked by tmartino@chromium.org

[commit-bot: I haz the power, 2017-04-06 20:44:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-06 21:52:37]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491511415926580,
"parent_rev": "6f96245dd91132ea7b140d2f632fcf6edffbc582", "commit_rev":
"cab3dfe03ca689d5eb085e7a06418e22cbf31a42"}

[commit-bot: I haz the power, 2017-04-06 21:53:53]

Description was changed from

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

BUG=702576
==========

to

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

BUG=702576

Review-Url: https://codereview.chromium.org/2786883008
Cr-Commit-Position: refs/heads/master@{#462636}
Committed:
https://chromium.googlesource.com/chromium/src/+/cab3dfe03ca689d5eb085e7a0641...
==========

[commit-bot: I haz the power, 2017-04-06 21:53:54]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/cab3dfe03ca689d5eb085e7a0641...