Update conditions to show Assistant onboarding.

Conditions are:
- Onboarding feature should be targeted to "new" users
- "New" users are users who haven't interacted w/ Assistant in 28+ days
- Once shown in a user session, we should continue to show onboarding
  in that user session until an Assistant interaction takes place
- Once shown in any user session, we should continue to show onboarding
  in each subsequent user session until we reach a maximum of 3

This CL also deprecates Warmer Welcome which is being replaced by the
new onboarding feature.

Bug: b:157510970
Change-Id: If60ac0e9f7d87aaf2240e76a5ba4e5fe06e8dd5b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2324801
Commit-Queue: David Black <dmblack@google.com>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#798729}

ash/app_list/views/assistant/assistant_page_view_unittest.cc

@@ -492,47 +492,129 @@ TEST_F(AssistantPageViewTest, ShouldShowGreetingLabelAgainAfterReopening) {
   EXPECT_EQ(nullptr, onboarding_view());
 }
 
-TEST_F(AssistantPageViewTest, ShouldShowOnboardingAgainAfterReopening) {
+TEST_F(AssistantPageViewTest,
+       ShouldNotShowGreetingLabelWhenOpeningFromSearchResult) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndDisableFeature(
+      chromeos::assistant::features::kAssistantBetterOnboarding);
+
+  ShowAssistantUi(AssistantEntryPoint::kLauncherSearchResult);
+
+  EXPECT_FALSE(greeting_label()->IsDrawn());
+  EXPECT_EQ(nullptr, onboarding_view());
+}
+
+TEST_F(AssistantPageViewTest,
+       ShouldNotShowOnboardingWhenOpeningFromSearchResult) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(
+      chromeos::assistant::features::kAssistantBetterOnboarding);
+
+  ShowAssistantUi(AssistantEntryPoint::kLauncherSearchResult);
+
+  EXPECT_FALSE(onboarding_view()->IsDrawn());
+  EXPECT_FALSE(greeting_label()->IsDrawn());
+}
+
+TEST_F(AssistantPageViewTest, ShouldShowOnboardingForNewUsers) {
   base::test::ScopedFeatureList scoped_feature_list;
   scoped_feature_list.InitAndEnableFeature(
       chromeos::assistant::features::kAssistantBetterOnboarding);
 
+  // A user is considered new if they haven't had an Assistant interaction in
+  // the past 28 days.
+  const base::Time new_user_cutoff =
+      base::Time::Now() - base::TimeDelta::FromDays(28);
+
+  SetTimeOfLastInteraction(new_user_cutoff + base::TimeDelta::FromMinutes(1));
   ShowAssistantUi();
 
-  // Cause the label to be hidden.
-  MockTextInteraction().WithTextResponse("The response");
-  ASSERT_FALSE(onboarding_view()->IsDrawn());
+  // This user *has* interacted with Assistant more recently than 28 days ago so
+  // they are *not* considered new. Therefore, onboarding should *not* be shown.
+  EXPECT_FALSE(onboarding_view()->IsDrawn());
+
+  SetTimeOfLastInteraction(new_user_cutoff);
 
-  // Close and reopen the Assistant UI.
   CloseAssistantUi();
   ShowAssistantUi();
 
+  // This user has *not* interacted with Assistant more recently than 28 days
+  // ago so they *are* considered new. Therefore, onboarding *should* be shown.
   EXPECT_TRUE(onboarding_view()->IsDrawn());
-  EXPECT_FALSE(greeting_label()->IsDrawn());
+}
+
+TEST_F(AssistantPageViewTest, ShouldShowOnboardingUntilInteractionOccurs) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(
+      chromeos::assistant::features::kAssistantBetterOnboarding);
+
+  SetTimeOfLastInteraction(base::Time::Now() - base::TimeDelta::FromDays(28));
+  ShowAssistantUi();
+
+  // This user has *not* interacted with Assistant more recently than 28 days
+  // ago so they *are* considered new. Therefore, onboarding *should* be shown.
+  EXPECT_TRUE(onboarding_view()->IsDrawn());
+
+  CloseAssistantUi();
+  ShowAssistantUi();
+
+  // The user has *not* yet interacted with Assistant in this user session, so
+  // we should continue to show onboarding.
+  EXPECT_TRUE(onboarding_view()->IsDrawn());
+
+  MockTextInteraction().WithQuery("Any Query").WithTextResponse("Any Response");
+
+  CloseAssistantUi();
+  ShowAssistantUi();
+
+  // The user *has* had an interaction with Assistant in this user session, so
+  // we should *not* show onboarding anymore.
+  EXPECT_FALSE(onboarding_view()->IsDrawn());
 }
 
 TEST_F(AssistantPageViewTest,
-       ShouldNotShowGreetingLabelWhenOpeningFromSearchResult) {
+       ShouldShowOnboardingToExistingUsersIfShownPreviouslyInDifferentSession) {
   base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitAndDisableFeature(
+  scoped_feature_list.InitAndEnableFeature(
       chromeos::assistant::features::kAssistantBetterOnboarding);
 
-  ShowAssistantUi(AssistantEntryPoint::kLauncherSearchResult);
+  SetTimeOfLastInteraction(base::Time::Now());
+  SetNumberOfSessionsWhereOnboardingShown(1);
 
-  EXPECT_FALSE(greeting_label()->IsDrawn());
-  EXPECT_EQ(nullptr, onboarding_view());
+  ShowAssistantUi();
+
+  // This user *has* interacted with Assistant more recently than 28 days ago so
+  // so they are *not* considered new. Onboarding would not normally be shown
+  // but, since it *was* shown in a previous user session, we *do* show it.
+  EXPECT_TRUE(onboarding_view()->IsDrawn());
+
+  MockTextInteraction().WithQuery("Any Query").WithTextResponse("Any Response");
+
+  CloseAssistantUi();
+  ShowAssistantUi();
+
+  // But once the user has had an interaction with Assistant in this user
+  // session, we still expect onboarding to no longer show.
+  EXPECT_FALSE(onboarding_view()->IsDrawn());
 }
 
 TEST_F(AssistantPageViewTest,
-       ShouldNotShowOnboardingWhenOpeningFromSearchResult) {
+       ShouldNotShowOnboardingToExistingUsersIfShownPreviouslyInMaxSessions) {
   base::test::ScopedFeatureList scoped_feature_list;
   scoped_feature_list.InitAndEnableFeature(
       chromeos::assistant::features::kAssistantBetterOnboarding);
 
-  ShowAssistantUi(AssistantEntryPoint::kLauncherSearchResult);
+  SetTimeOfLastInteraction(base::Time::Now());
+  SetNumberOfSessionsWhereOnboardingShown(
+      assistant::ui::kOnboardingMaxSessionsShown);
+
+  ShowAssistantUi();
 
+  // This user has *not* interacted with Assistant more recently than 28 days
+  // ago so they *are* considered new. Onboarding would normally be shown but,
+  // since it was shown already in the max number of previous user sessions, we
+  // do *not* show it.
   EXPECT_FALSE(onboarding_view()->IsDrawn());
-  EXPECT_FALSE(greeting_label()->IsDrawn());
 }
 
 TEST_F(AssistantPageViewTest, ShouldFocusMicViewWhenPressingVoiceInputToggle) {

ash/app_list/views/assistant/assistant_test_api_impl.cc

@@ -10,6 +10,7 @@
 #include "ash/app_list/views/app_list_view.h"
 #include "ash/app_list/views/contents_view.h"
 #include "ash/assistant/ui/assistant_view_ids.h"
+#include "ash/public/cpp/ash_pref_names.h"
 #include "ash/public/cpp/assistant/assistant_state.h"
 #include "ash/public/cpp/tablet_mode.h"
 #include "ash/session/session_controller_impl.h"
@@ -158,6 +159,12 @@ void AssistantTestApiImpl::SetConsentStatus(
          "a waiter here to wait for the new state to take effect.";
 }
 
+void AssistantTestApiImpl::SetNumberOfSessionsWhereOnboardingShown(
+    int number_of_sessions) {
+  Shell::Get()->session_controller()->GetPrimaryUserPrefService()->SetInteger(
+      prefs::kAssistantNumSessionsWhereOnboardingShown, number_of_sessions);
+}
+
 void AssistantTestApiImpl::SetOnboardingMode(
     chromeos::assistant::prefs::AssistantOnboardingMode onboarding_mode) {
   Shell::Get()->session_controller()->GetPrimaryUserPrefService()->SetString(
@@ -182,6 +189,11 @@ void AssistantTestApiImpl::SetPreferVoice(bool value) {
          "a waiter here to wait for the new state to take effect.";
 }
 
+void AssistantTestApiImpl::SetTimeOfLastInteraction(base::Time time) {
+  Shell::Get()->session_controller()->GetPrimaryUserPrefService()->SetTime(
+      prefs::kAssistantTimeOfLastInteraction, time);
+}
+
 AssistantState* AssistantTestApiImpl::GetAssistantState() {
   return AssistantState::Get();
 }

ash/app_list/views/assistant/assistant_test_api_impl.h

@@ -33,9 +33,11 @@ class AssistantTestApiImpl : public AssistantTestApi {
   void SetScreenContextEnabled(bool enabled) override;
   void SetTabletMode(bool enable) override;
   void SetConsentStatus(chromeos::assistant::prefs::ConsentStatus) override;
+  void SetNumberOfSessionsWhereOnboardingShown(int number_of_sessions) override;
   void SetOnboardingMode(
       chromeos::assistant::prefs::AssistantOnboardingMode) override;
   void SetPreferVoice(bool value) override;
+  void SetTimeOfLastInteraction(base::Time time) override;
   void StartOverview() override;
   AssistantState* GetAssistantState() override;
   void WaitUntilIdle() override;

ash/assistant/assistant_controller_impl.cc

@@ -52,8 +52,8 @@ AssistantControllerImpl::~AssistantControllerImpl() {
 // static
 void AssistantControllerImpl::RegisterProfilePrefs(
     PrefRegistrySimple* registry) {
-  registry->RegisterIntegerPref(prefs::kAssistantNumWarmerWelcomeTriggered, 0);
   AssistantInteractionControllerImpl::RegisterProfilePrefs(registry);
+  AssistantUiControllerImpl::RegisterProfilePrefs(registry);
 }
 
 void AssistantControllerImpl::BindReceiver(

ash/assistant/assistant_controller_impl.h

@@ -157,7 +157,7 @@ class ASH_EXPORT AssistantControllerImpl
       this};
   AssistantSetupController assistant_setup_controller_{this};
   AssistantSuggestionsControllerImpl assistant_suggestions_controller_;
-  AssistantUiControllerImpl assistant_ui_controller_;
+  AssistantUiControllerImpl assistant_ui_controller_{this};
   AssistantWebUiController assistant_web_ui_controller_;
 
   AssistantViewDelegateImpl view_delegate_{this};

ash/assistant/assistant_interaction_controller_impl.cc

@@ -45,7 +45,6 @@ namespace ash {
 
 namespace {
 
-using assistant::ui::kWarmerWelcomesMaxTimesTriggered;
 using chromeos::assistant::features::IsResponseProcessingV2Enabled;
 using chromeos::assistant::features::IsTimersV2Enabled;
 using chromeos::assistant::features::IsWaitSchedulingEnabled;
@@ -78,20 +77,11 @@ bool IsPreferVoice() {
 
 PrefService* pref_service() {
   auto* result =
-      Shell::Get()->session_controller()->GetLastActiveUserPrefService();
+      Shell::Get()->session_controller()->GetPrimaryUserPrefService();
   DCHECK(result);
   return result;
 }
 
-int num_warmer_welcome_triggered() {
-  return pref_service()->GetInteger(prefs::kAssistantNumWarmerWelcomeTriggered);
-}
-
-void IncrementNumWarmerWelcomeTriggered() {
-  pref_service()->SetInteger(prefs::kAssistantNumWarmerWelcomeTriggered,
-                             num_warmer_welcome_triggered() + 1);
-}
-
 }  // namespace
 
 // AssistantInteractionController ----------------------------------------------
@@ -141,6 +131,10 @@ AssistantInteractionControllerImpl::GetTimeDeltaSinceLastInteraction() const {
          pref_service()->GetTime(prefs::kAssistantTimeOfLastInteraction);
 }
 
+bool AssistantInteractionControllerImpl::HasHadInteraction() const {
+  return has_had_interaction_;
+}
+
 void AssistantInteractionControllerImpl::StartTextInteraction(
     const std::string& text,
     bool allow_tts,
@@ -340,6 +334,10 @@ void AssistantInteractionControllerImpl::OnCommittedQueryChanged(
   pref_service()->SetTime(prefs::kAssistantTimeOfLastInteraction,
                           base::Time::Now());
 
+  // Cache the fact that the user has now had an interaction with the Assistant
+  // during this user session.
+  has_had_interaction_ = true;
+
   std::string query;
   switch (assistant_query.type()) {
     case AssistantQueryType::kText: {
@@ -885,8 +883,6 @@ void AssistantInteractionControllerImpl::OnUiVisible(
     AssistantEntryPoint entry_point) {
   DCHECK(IsVisible());
 
-  ++number_of_times_shown_;
-
   // We don't explicitly start a new voice interaction if the entry point
   // is hotword since in such cases a voice interaction will already be in
   // progress.
@@ -895,46 +891,6 @@ void AssistantInteractionControllerImpl::OnUiVisible(
     StartVoiceInteraction();
     return;
   }
-
-  if (ShouldAttemptWarmerWelcome(entry_point))
-    AttemptWarmerWelcome();
-}
-
-bool AssistantInteractionControllerImpl::ShouldAttemptWarmerWelcome(
-    AssistantEntryPoint entry_point) const {
-  if (number_of_times_shown_ > 1)
-    return false;
-
-  if (!assistant::util::ShouldAttemptWarmerWelcome(entry_point))
-    return false;
-
-  // Explicitly check the interaction state to ensure warmer welcome will not
-  // interrupt any ongoing active interactions. This happens, for example, when
-  // the first Assistant launch of the current user session is trigger by
-  // Assistant notification, or directly sending query without showing Ui during
-  // integration test.
-  if (HasActiveInteraction())
-    return false;
-
-  if (num_warmer_welcome_triggered() >= kWarmerWelcomesMaxTimesTriggered)
-    return false;
-
-  return true;
-}
-
-void AssistantInteractionControllerImpl::AttemptWarmerWelcome() {
-  // TODO(yileili): Currently WW is only triggered when the first Assistant
-  // launch of the user session does not automatically start an interaction that
-  // would otherwise cause us to interrupt the user.  Need further UX design to
-  // attempt WW after the first interaction.
-
-  // If the user has opted to launch Assistant with the mic open, we
-  // can reasonably assume there is an expectation of TTS.
-  bool allow_tts = launch_with_mic_open();
-
-  assistant_->StartWarmerWelcomeInteraction(num_warmer_welcome_triggered(),
-                                            allow_tts);
-  IncrementNumWarmerWelcomeTriggered();
 }
 
 void AssistantInteractionControllerImpl::StartScreenContextInteraction(

ash/assistant/assistant_interaction_controller_impl.h

@@ -65,6 +65,7 @@ class AssistantInteractionControllerImpl
   // AssistantInteractionController:
   const AssistantInteractionModel* GetModel() const override;
   base::TimeDelta GetTimeDeltaSinceLastInteraction() const override;
+  bool HasHadInteraction() const override;
   void StartTextInteraction(const std::string& text,
                             bool allow_tts,
                             AssistantQuerySource query_source) override;
@@ -135,8 +136,6 @@ class AssistantInteractionControllerImpl
   void OnPendingResponseProcessed(bool is_completed);
 
   void OnUiVisible(AssistantEntryPoint entry_point);
-  bool ShouldAttemptWarmerWelcome(AssistantEntryPoint entry_point) const;
-  void AttemptWarmerWelcome();
 
   void StartScreenContextInteraction(bool include_assistant_structure,
                                      const gfx::Rect& region,
@@ -150,17 +149,12 @@ class AssistantInteractionControllerImpl
   bool IsVisible() const;
 
   AssistantControllerImpl* const assistant_controller_;  // Owned by Shell.
+  AssistantInteractionModel model_;
+  bool has_had_interaction_ = false;
 
   // Owned by AssistantService.
   chromeos::assistant::Assistant* assistant_ = nullptr;
 
-  AssistantInteractionModel model_;
-
-  // The number of times the Assistant UI has been shown (since the device
-  // booted).
-  // Might overflow so do not use for super critical things.
-  int number_of_times_shown_ = 0;
-
   ScopedObserver<AssistantController, AssistantControllerObserver>
       assistant_controller_observer_{this};