Skip to content

Commit

Permalink
Revert "[fyfre] Clean up FirstRunService eligibility checks"
Browse files Browse the repository at this point in the history 
This reverts commit 77c78fd.

Reason for revert: Linux ASan failures. Example: https://ci.chromium.org/ui/p/chromium/builders/ci/Linux%20ASan%20LSan%20Tests%20(1)/112172/overview

Original change's description:
> [fyfre] Clean up FirstRunService eligibility checks
>
> Consolidates some state checks that determine whether to instantiate
> a service and whether to open the FRE.
>
> This is preparing some metrics related checks, to simplify the
> determination of whether the FRE could run before checking the state of
> the feature itself.
>
> Bug: 1402712
> Change-Id: Ida783c43b28d9a36240dd766f310cd51c8dad256
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4280190
> Reviewed-by: Alex Ilin <alexilin@chromium.org>
> Commit-Queue: Nicolas Dossou-Gbété <dgn@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1109077}

Bug: 1402712
Change-Id: I3116db59832850107a9d198cc16efe1346c636bd
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4284568
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Owners-Override: Igor Ruvinov <igorruvinov@chromium.org>
Auto-Submit: Igor Ruvinov <igorruvinov@chromium.org>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#1109192}
  • Loading branch information
Igor Ruvinov authored and Chromium LUCI CQ committed Feb 23, 2023
1 parent a59be6f commit fee3344
Show file tree
Hide file tree
Showing 3 changed files with 104 additions and 131 deletions.
111 changes: 56 additions & 55 deletions chrome/browser/ui/startup/first_run_service.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,55 +47,30 @@
namespace {

bool IsFirstRunEligibleProfile(Profile* profile) {
if (profile->IsOffTheRecord()) {
return false;
}

// The parent guest and the profiles in a ChromeOS Guest session get through
// the OTR check above.
if (profile->IsGuestSession()) {
return false;
}
// Profile selections should exclude these already.
DCHECK(!profile->IsOffTheRecord());

#if BUILDFLAG(IS_CHROMEOS_LACROS)
// Skip for users without Gaia account (e.g. Active Directory, Kiosk, Guest…)
if (!profiles::SessionHasGaiaAccount()) {
if (!profiles::SessionHasGaiaAccount())
return false;

// The profile in Guest user sessions is considered "regular" but should
// also be excluded here.
if (profile->IsGuestSession())
return false;
}

// Having secondary profiles implies that the user already used Chrome and so
// should not have to see the FRE. So we never want to run it for these.
if (!profile->IsMainProfile()) {
if (!profile->IsMainProfile())
return false;
}
#else
DCHECK(!profile->IsGuestSession());
#endif

return true;
}

bool IsFirstRunEligibleProcess() {
#if !BUILDFLAG(IS_CHROMEOS_LACROS)
// On Lacros we want to run the FRE beyond the strict first run as defined by
// `IsChromeFirstRun()` for a few reasons:
// - Migrated profiles will have their first run sentinel imported from the
// ash data dir, but we need to run the FRE in silent mode to re-enable sync
// on the Lacros primary profile.
// - If the user exits the FRE without advancing beyond the first step, we
// need to show the FRE again next time they open Chrome, this is definitely
// not the "first run" anymore.
if (!first_run::IsChromeFirstRun()) {
return false;
}
#endif

// TODO(crbug.com/1347504): `IsChromeFirstRun()` should be a sufficient check
// for Dice platforms. We currently keep this because some tests add
// `--force-first-run` while keeping `--no-first-run`. We should updated the
// affected tests to handle correctly the FRE opening instead of a tab.
return !base::CommandLine::ForCurrentProcess()->HasSwitch(
switches::kNoFirstRun);
}

enum class PolicyEffect {
// The First Run experience can proceed unaffected.
kNone,
Expand Down Expand Up @@ -188,11 +163,7 @@ void SetFirstRunFinished(FinishedReason reason) {
#endif
}

// Returns whether `prefs::kFirstRunFinished` is true. This implies that the FRE
// should not be opened again and would set if the user saw the FRE and is done
// with it, or if for some other reason (e.g. policy or some other browser
// state) we determine that we should not show it.
bool IsFirstRunMarkedFinishedInPrefs() {
bool IsFirstRunFinished() {
// Can be null in unit tests.
const PrefService* const local_state = g_browser_process->local_state();
return local_state && local_state->GetBoolean(prefs::kFirstRunFinished);
Expand Down Expand Up @@ -301,7 +272,29 @@ FirstRunService::FirstRunService(Profile* profile) : profile_(profile) {}
FirstRunService::~FirstRunService() = default;

bool FirstRunService::ShouldOpenFirstRun() const {
return ::ShouldOpenFirstRun(profile_);
DCHECK(IsFirstRunEligibleProfile(profile_));

#if !BUILDFLAG(IS_CHROMEOS_LACROS)
// On Lacros we want to run the FRE beyond the strict first run as defined by
// `IsChromeFirstRun()` for a few reasons:
// - Migrated profiles will have their first run sentinel imported from the
// ash data dir, but we need to run the FRE in silent mode to re-enable sync
// on the Lacros primary profile.
// - If the user exits the FRE without advancing beyond the first step, we
// need to show the FRE again next time they open Chrome, this is definitely
// not the "first run" anymore.
if (!first_run::IsChromeFirstRun()) {
return false;
}
#endif

const base::CommandLine* command_line =
base::CommandLine::ForCurrentProcess();
if (command_line->HasSwitch(switches::kNoFirstRun)) {
return false;
}

return !IsFirstRunFinished();
}

void FirstRunService::TryMarkFirstRunAlreadyFinished(
Expand Down Expand Up @@ -394,12 +387,9 @@ void FirstRunService::OpenFirstRunIfNeeded(EntryPoint entry_point,

void FirstRunService::OpenFirstRunInternal(EntryPoint entry_point,
ResumeTaskCallback callback) {
if (IsFirstRunMarkedFinishedInPrefs()) {
// Opening the First Run is not needed. For example it might have been
// marked finished silently, or is suppressed by policy.
//
// Note that this assumes that the prefs state is the the only part of
// `ShouldOpenFirstRun()` that can change during the service's lifetime.
if (!ShouldOpenFirstRun()) {
// Opening the First Run is not needed, it might have been marked finished
// silently for example.
std::move(callback).Run(/*proceed=*/true);
return;
}
Expand Down Expand Up @@ -447,8 +437,15 @@ FirstRunService* FirstRunServiceFactory::GetForBrowserContext(

KeyedService* FirstRunServiceFactory::BuildServiceInstanceFor(
content::BrowserContext* context) const {
if (IsFirstRunFinished()) {
return nullptr;
}

Profile* profile = Profile::FromBrowserContext(context);
if (!ShouldOpenFirstRun(profile)) {
// `ProfileSelections` exclude some profiles already, but they do not check
// for some more specific conditions where we don't want to instantiate the
// service.
if (!IsFirstRunEligibleProfile(profile)) {
return nullptr;
}

Expand All @@ -473,10 +470,14 @@ KeyedService* FirstRunServiceFactory::BuildServiceInstanceFor(

#if BUILDFLAG(IS_CHROMEOS_LACROS)
// Check if we should turn Sync on from the background and skip the FRE.
// If we don't manage to set it, we will just have to defer silent or visual
// handling of the FRE to when the user attempts to open a browser UI. So
// we don't need to do anything when the attempt finishes.
instance->TryMarkFirstRunAlreadyFinished(base::OnceClosure());
// TODO(dgn): maybe post task? For example see
// //chrome/browser/permissions/permission_auditing_service_factory.cc
if (instance->ShouldOpenFirstRun()) {
// If we don't manage to set it, we will just have to defer silent or visual
// handling of the FRE to when the user attempts to open a browser UI. So
// we don't need to do anything when the attempt finishes.
instance->TryMarkFirstRunAlreadyFinished(base::OnceClosure());
}
#endif

return instance;
Expand All @@ -495,6 +496,6 @@ bool FirstRunServiceFactory::ServiceIsCreatedWithBrowserContext() const {
// Helpers ---------------------------------------------------------------------

bool ShouldOpenFirstRun(Profile* profile) {
return IsFirstRunEligibleProcess() && IsFirstRunEligibleProfile(profile) &&
!IsFirstRunMarkedFinishedInPrefs();
auto* instance = FirstRunServiceFactory::GetForBrowserContext(profile);
return instance && instance->ShouldOpenFirstRun();
}
13 changes: 4 additions & 9 deletions chrome/browser/ui/startup/first_run_service.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,8 @@ class FirstRunService : public KeyedService {
explicit FirstRunService(Profile* profile);
~FirstRunService() override;

// Runs `::ShouldOpenFirstRun(Profile*)` with the profile associated with this
// service instance.
// Returns whether first run experience (including sync promo) should be
// opened on startup.
bool ShouldOpenFirstRun() const;

// This function takes the user through the browser FRE.
Expand Down Expand Up @@ -138,7 +138,6 @@ class FirstRunServiceFactory : public ProfileKeyedServiceFactory {

private:
friend class base::NoDestructor<FirstRunServiceFactory>;
friend class FirstRunServiceBrowserTest;

FirstRunServiceFactory();
~FirstRunServiceFactory() override;
Expand All @@ -148,12 +147,8 @@ class FirstRunServiceFactory : public ProfileKeyedServiceFactory {
bool ServiceIsCreatedWithBrowserContext() const override;
};

// Returns whether the first run experience (including sync promo) might be
// opened for `profile`. It should be checked before
// `FirstRunService::OpenFirstRunIfNeeded()` is called.
//
// Even if this method returns `true`, the FRE can still be skipped if for
// example the feature is disabled, a policy suppresses it, etc.
// Helper to call `FirstRunService::ShouldOpenFirstRun()` without having
// to first obtain the service instance.
bool ShouldOpenFirstRun(Profile* profile);

#endif // CHROME_BROWSER_UI_STARTUP_FIRST_RUN_SERVICE_H_

0 comments on commit fee3344

Please sign in to comment.