Skip to content

Commit

Permalink
Componentize user education.
Browse files Browse the repository at this point in the history
Moves all of the application-independent bits of user education to their
own component.

This is initially intended to support some uses cases where Ash
developers wanted to use HelpBubbleView but could not because it was
under chrome/.

It was always intended to move this code into a component once it was
tested in the browser, as there is some interest from other platforms
(for example, for tutorials on mobile).

The user education code is split across the following subcomponents:
 * common - stuff that doesn't depend on any platform or framework
 * views - implementation of Views- (and Mac-)specific visual elements
 * test - test-only utility classes

All code is placed in namespace user_education. A handful of strings
that are not app-specific have been moved to components_strings.

Some browser and integration testing remains in
  chrome/browser/ui[/views]/user_education
along with Chrome browser-specific implementation.

Almost all of this CL is the movement of existing code. To the extent
any of it has been changed or rewritten it is typically to add proper
namespace scoping or for clarity.

Change-Id: Iea86a6af6ae32a60d8ee8cb457a928783f33a9be
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3630872
Reviewed-by: David Pennington <dpenning@chromium.org>
Reviewed-by: Theresa Sullivan <twellington@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Commit-Queue: Dana Fried <dfried@chromium.org>
Reviewed-by: Cait Phillips <caitkp@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1001860}
  • Loading branch information
Dana Fried authored and Chromium LUCI CQ committed May 11, 2022
1 parent 6d6259f commit 764e635
Show file tree
Hide file tree
Showing 128 changed files with 1,934 additions and 1,033 deletions.
34 changes: 0 additions & 34 deletions chrome/app/generated_resources.grd
Original file line number Diff line number Diff line change
Expand Up @@ -7163,12 +7163,6 @@ Keep your key file in a safe place. You will need it to create new versions of y
<message name="IDS_CHROME_TIP" desc="Accessible name used by a screen reader for the in-product-help or tutorial bubble icon." is_accessibility_with_no_ui="true">
Chrome tip
</message>
<message name="IDS_CLOSE_TUTORIAL" desc="Accessible name used by a screen reader for the tutorial bubble close button." is_accessibility_with_no_ui="true">
Close tutorial
</message>
<message name="IDS_CLOSE_PROMO" desc="Accessible name used by a screen reader for the in-product-help bubble close button." is_accessibility_with_no_ui="true">
Close help bubble
</message>
<message name="IDS_GLOBAL_MEDIA_CONTROLS_PROMO" desc="Text shown on promotional UI appearing next to the global media controls toolbar button.">
New! Control your music, videos, and more.
</message>
Expand Down Expand Up @@ -7226,39 +7220,11 @@ Keep your key file in a safe place. You will need it to create new versions of y
<message name="IDS_TAB_AUDIO_MUTING_PROMO" desc="Text shown on the promotional UI appearing next to the tab alert indicator.">
Click the speaker icon to mute this tab
</message>
<if expr="use_titlecase">
<message name="IDS_PROMO_DISMISS_BUTTON" desc="Text shown on the button for the user to acknowledge that they understand the message">
Got It
</message>
<message name="IDS_PROMO_SNOOZE_BUTTON" desc="Text shown on the button to snooze a promotional UI, which indicates that users want to see this promotion at a later time">
Remind Me Later
</message>
<message name="IDS_PROMO_SHOW_TUTORIAL_BUTTON" desc="Text shown on a promo bubble button for the user to initiate a tutorial after being asked if they want to learn more.">
Show Me How
</message>
</if>
<if expr="not use_titlecase">
<message name="IDS_PROMO_DISMISS_BUTTON" desc="Text shown on the button for the user to acknowledge that they understand the message">
Got it
</message>
<message name="IDS_PROMO_SNOOZE_BUTTON" desc="Text shown on the button to snooze a promotional UI, which indicates that users want to see this promotion at a later time">
Remind me later
</message>
<message name="IDS_PROMO_SHOW_TUTORIAL_BUTTON" desc="Text shown on a promo bubble button for the user to initiate a tutorial after being asked if they want to learn more.">
Show me how
</message>
</if>
<message name="IDS_SHARED_HIGHLIGHTING_PROMO" desc="The title of in-product-help message encouraging users to interact with the highlight">
To create a highlight like this one, select any text and right-click.
</message>

<!-- User Education Tutorial Strings -->
<message name="IDS_TUTORIAL_RESTART_TUTORIAL" desc="The text shown on the button for restarting a tutorial">
Restart tutorial
</message>
<message name="IDS_TUTORIAL_CLOSE_TUTORIAL" desc="The text shown on the button for closing a tutorial">
Close
</message>
<if expr="use_titlecase">
<message name="IDS_TUTORIAL_TAB_GROUP_ADD_TAB_TO_GROUP" desc="The description of the add tab step in the tab group tutorial">
Right-click on a tab and select "Add Tab To New Group"
Expand Down
38 changes: 4 additions & 34 deletions chrome/browser/ui/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -1412,21 +1412,8 @@ static_library("ui") {
"user_education/active_tab_tracker.h",
"user_education/browser_feature_promo_snooze_service.cc",
"user_education/browser_feature_promo_snooze_service.h",
"user_education/feature_promo_controller.cc",
"user_education/feature_promo_controller.h",
"user_education/feature_promo_registry.cc",
"user_education/feature_promo_registry.h",
"user_education/feature_promo_snooze_service.cc",
"user_education/feature_promo_snooze_service.h",
"user_education/feature_promo_specification.cc",
"user_education/feature_promo_specification.h",
"user_education/help_bubble.cc",
"user_education/help_bubble.h",
"user_education/help_bubble_factory.h",
"user_education/help_bubble_factory_registry.cc",
"user_education/help_bubble_factory_registry.h",
"user_education/help_bubble_params.cc",
"user_education/help_bubble_params.h",
"user_education/browser_tutorial_service.cc",
"user_education/browser_tutorial_service.h",
"user_education/reopen_tab_in_product_help.cc",
"user_education/reopen_tab_in_product_help.h",
"user_education/reopen_tab_in_product_help_factory.cc",
Expand All @@ -1435,15 +1422,6 @@ static_library("ui") {
"user_education/reopen_tab_in_product_help_trigger.h",
"user_education/scoped_new_badge_tracker.cc",
"user_education/scoped_new_badge_tracker.h",
"user_education/tutorial/tutorial.cc",
"user_education/tutorial/tutorial.h",
"user_education/tutorial/tutorial_description.cc",
"user_education/tutorial/tutorial_description.h",
"user_education/tutorial/tutorial_identifier.h",
"user_education/tutorial/tutorial_registry.cc",
"user_education/tutorial/tutorial_registry.h",
"user_education/tutorial/tutorial_service.cc",
"user_education/tutorial/tutorial_service.h",
"user_education/user_education_service.cc",
"user_education/user_education_service.h",
"user_education/user_education_service_factory.cc",
Expand Down Expand Up @@ -1806,6 +1784,7 @@ static_library("ui") {
"//components/services/app_service/public/mojom",
"//components/ui_metrics",
"//components/url_formatter",
"//components/user_education/common",
"//components/user_notes:features",
"//components/vector_icons",
"//components/web_modal",
Expand Down Expand Up @@ -4209,8 +4188,6 @@ static_library("ui") {
"views/chrome_typography.h",
"views/chrome_typography_provider.cc",
"views/chrome_typography_provider.h",
"views/chrome_view_class_properties.cc",
"views/chrome_view_class_properties.h",
"views/chrome_views_delegate.cc",
"views/chrome_views_delegate.h",
"views/chrome_web_dialog_view.cc",
Expand Down Expand Up @@ -4929,12 +4906,6 @@ static_library("ui") {
"views/user_education/browser_feature_promo_controller.h",
"views/user_education/browser_user_education_service.cc",
"views/user_education/browser_user_education_service.h",
"views/user_education/help_bubble_factory_views.cc",
"views/user_education/help_bubble_factory_views.h",
"views/user_education/help_bubble_view.cc",
"views/user_education/help_bubble_view.h",
"views/user_education/new_badge_label.cc",
"views/user_education/new_badge_label.h",
"views/user_education/tip_marquee_view.cc",
"views/user_education/tip_marquee_view.h",
"views/web_apps/file_handler_launch_dialog_view.cc",
Expand Down Expand Up @@ -5044,6 +5015,7 @@ static_library("ui") {
"//components/soda:constants",
"//components/tab_count_metrics",
"//components/ui_devtools/views",
"//components/user_education/views",
"//components/user_notes:features",
"//components/user_notes/browser",
"//components/user_notes/interfaces",
Expand Down Expand Up @@ -5122,8 +5094,6 @@ static_library("ui") {
"views/chrome_views_delegate_mac.cc",
"views/policy/enterprise_startup_dialog_mac_util.h",
"views/policy/enterprise_startup_dialog_mac_util.mm",
"views/user_education/help_bubble_factory_mac.h",
"views/user_education/help_bubble_factory_mac.mm",
]
} else {
sources += [
Expand Down
2 changes: 2 additions & 0 deletions chrome/browser/ui/DEPS
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ include_rules = [
"+components/safety_check",
"+components/soda",
"+components/translate/content/android",
"+components/user_education/common",
"+components/user_education/test",
"+services/content/public",
"+services/device/public/mojom",
"+components/user_notes",
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/ui/browser_commands.cc
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,6 @@
#include "chrome/browser/ui/tabs/tab_group_model.h"
#include "chrome/browser/ui/translate/translate_bubble_ui_action_logger.h"
#include "chrome/browser/ui/ui_features.h"
#include "chrome/browser/ui/user_education/feature_promo_controller.h"
#include "chrome/browser/ui/user_education/reopen_tab_in_product_help.h"
#include "chrome/browser/ui/user_education/reopen_tab_in_product_help_factory.h"
#include "chrome/browser/ui/web_applications/app_browser_controller.h"
Expand Down Expand Up @@ -121,6 +120,7 @@
#include "components/tab_groups/tab_group_id.h"
#include "components/tab_groups/tab_group_visual_data.h"
#include "components/translate/core/browser/language_state.h"
#include "components/user_education/common/feature_promo_controller.h"
#include "components/web_modal/web_contents_modal_dialog_manager.h"
#include "components/zoom/page_zoom.h"
#include "components/zoom/zoom_controller.h"
Expand Down
18 changes: 10 additions & 8 deletions chrome/browser/ui/browser_window.h
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,11 @@
#include "chrome/browser/ui/exclusive_access/exclusive_access_bubble_type.h"
#include "chrome/browser/ui/hats/hats_service.h"
#include "chrome/browser/ui/page_action/page_action_icon_type.h"
#include "chrome/browser/ui/user_education/feature_promo_controller.h"
#include "chrome/browser/ui/user_education/feature_promo_specification.h"
#include "chrome/common/buildflags.h"
#include "components/content_settings/core/common/content_settings_types.h"
#include "components/translate/core/common/translate_errors.h"
#include "components/user_education/common/feature_promo_controller.h"
#include "components/user_education/common/feature_promo_specification.h"
#include "third_party/abseil-cpp/absl/types/optional.h"
#include "ui/base/base_window.h"
#include "ui/base/interaction/element_identifier.h"
Expand Down Expand Up @@ -583,7 +583,8 @@ class BrowserWindow : public ui::BaseWindow {

// Gets the windows's FeaturePromoController which manages display of
// in-product help. Will return null in incognito and guest profiles.
virtual FeaturePromoController* GetFeaturePromoController() = 0;
virtual user_education::FeaturePromoController*
GetFeaturePromoController() = 0;

// Returns whether the promo bubble associated with `iph_feature` is visible.
// If `include_continued_promos` is true, will also return true if
Expand All @@ -597,9 +598,10 @@ class BrowserWindow : public ui::BaseWindow {
// In cases where there is no promo controller, immediately returns false.
virtual bool MaybeShowFeaturePromo(
const base::Feature& iph_feature,
FeaturePromoSpecification::StringReplacements body_text_replacements = {},
FeaturePromoController::BubbleCloseCallback close_callback =
base::DoNothing()) = 0;
user_education::FeaturePromoSpecification::StringReplacements
body_text_replacements = {},
user_education::FeaturePromoController::BubbleCloseCallback
close_callback = base::DoNothing()) = 0;

// Closes the in-product help promo for `iph_feature` if it is showing;
// returns true if the promo was closed, false if it was not showing.
Expand All @@ -609,8 +611,8 @@ class BrowserWindow : public ui::BaseWindow {
// handle that can be used to end the promo when it is destructed. The handle
// will be valid (i.e. have a true boolean value) if the promo was showing,
// invalid otherwise.
virtual FeaturePromoController::PromoHandle CloseFeaturePromoAndContinue(
const base::Feature& iph_feature) = 0;
virtual user_education::FeaturePromoController::PromoHandle
CloseFeaturePromoAndContinue(const base::Feature& iph_feature) = 0;

// Records that the user has engaged with a particular feature that has an
// associated promo; this information is used to determine whether to show
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@

#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/browser_window.h"
#include "chrome/browser/ui/user_education/feature_promo_controller.h"
#include "components/feature_engagement/public/feature_constants.h"
#include "components/user_education/common/feature_promo_controller.h"
#include "content/public/browser/render_frame_host.h"
#include "content/public/browser/web_contents.h"
#include "mojo/public/cpp/bindings/pending_remote.h"
Expand All @@ -19,7 +19,8 @@
namespace {

void OnGetExistingSelectorsComplete(
base::WeakPtr<FeaturePromoController> feature_promo_controller,
base::WeakPtr<user_education::FeaturePromoController>
feature_promo_controller,
const std::vector<std::string>& selectors) {
if (feature_promo_controller && selectors.size() > 0) {
feature_promo_controller->MaybeShowPromo(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/signin_view_controller.h"
#include "chrome/browser/ui/tabs/tab_strip_model.h"
#include "chrome/browser/ui/user_education/feature_promo_controller.h"
#include "chrome/browser/ui/views/profiles/avatar_toolbar_button.h"
#include "chrome/browser/ui/webui/signin/login_ui_service.h"
#include "chrome/browser/ui/webui/signin/login_ui_service_factory.h"
Expand All @@ -39,6 +38,7 @@
#include "components/signin/public/base/account_consistency_method.h"
#include "components/signin/public/base/signin_metrics.h"
#include "components/sync/driver/test_sync_service.h"
#include "components/user_education/common/feature_promo_controller.h"
#include "content/public/browser/web_contents.h"
#include "content/public/test/browser_test.h"
#include "content/public/test/test_navigation_observer.h"
Expand Down Expand Up @@ -152,8 +152,8 @@ class SigninInterceptFirstRunExperienceDialogBrowserTest
// Needed for profile switch IPH testing.
AvatarToolbarButton::SetIPHMinDelayAfterCreationForTesting(
base::Seconds(0));
test_lock_ =
FeaturePromoControllerCommon::BlockActiveWindowCheckForTesting();
test_lock_ = user_education::FeaturePromoControllerCommon::
BlockActiveWindowCheckForTesting();
}

// Returns true if the profile switch IPH has been shown.
Expand Down Expand Up @@ -275,7 +275,7 @@ class SigninInterceptFirstRunExperienceDialogBrowserTest
base::UserActionTester user_action_tester_;

CoreAccountId account_id_;
FeaturePromoControllerCommon::TestLock test_lock_;
user_education::FeaturePromoControllerCommon::TestLock test_lock_;
};

// Shows and closes the fre dialog.
Expand Down
1 change: 0 additions & 1 deletion chrome/browser/ui/tabs/tab_strip_model_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
#include "chrome/browser/ui/tabs/tab_group.h"
#include "chrome/browser/ui/tabs/tab_group_model.h"
#include "chrome/browser/ui/tabs/test_tab_strip_model_delegate.h"
#include "chrome/browser/ui/user_education/mock_feature_promo_controller.h"
#include "chrome/test/base/browser_with_test_window_test.h"
#include "chrome/test/base/testing_profile.h"
#include "components/tab_groups/tab_group_color.h"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ void BrowserFeaturePromoSnoozeService::Reset(const base::Feature& iph_feature) {
pref_data->RemovePath(iph_feature.name);
}

absl::optional<FeaturePromoSnoozeService::SnoozeData>
absl::optional<user_education::FeaturePromoSnoozeService::SnoozeData>
BrowserFeaturePromoSnoozeService::ReadSnoozeData(
const base::Feature& iph_feature) {
std::string path_prefix = std::string(iph_feature.name) + ".";
Expand Down Expand Up @@ -112,7 +112,7 @@ BrowserFeaturePromoSnoozeService::ReadSnoozeData(

void BrowserFeaturePromoSnoozeService::SaveSnoozeData(
const base::Feature& iph_feature,
const FeaturePromoSnoozeService::SnoozeData& snooze_data) {
const user_education::FeaturePromoSnoozeService::SnoozeData& snooze_data) {
std::string path_prefix = std::string(iph_feature.name) + ".";

DictionaryPrefUpdate update(profile_->GetPrefs(), kIPHSnoozeDataPath);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,14 @@

#include "base/memory/raw_ptr.h"
#include "base/time/time.h"
#include "chrome/browser/ui/user_education/feature_promo_snooze_service.h"
#include "components/user_education/common/feature_promo_snooze_service.h"
#include "third_party/abseil-cpp/absl/types/optional.h"

class Profile;
class PrefRegistrySimple;

class BrowserFeaturePromoSnoozeService : public FeaturePromoSnoozeService {
class BrowserFeaturePromoSnoozeService
: public user_education::FeaturePromoSnoozeService {
public:
explicit BrowserFeaturePromoSnoozeService(Profile* profile);
~BrowserFeaturePromoSnoozeService() override;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,36 +24,27 @@ base::Feature kTestIPHFeature2{"TestIPHFeature2",

} // namespace

class FeaturePromoSnoozeServiceTest : public testing::Test {
// Repeats some of the tests in FeaturePromoSnoozeServiceTest except that a live
// test profile is used to back the service instead of a dummy data map.
class BrowserFeaturePromoSnoozeServiceTest : public testing::Test {
public:
FeaturePromoSnoozeServiceTest()
BrowserFeaturePromoSnoozeServiceTest()
: task_environment_{base::test::SingleThreadTaskEnvironment::TimeSource::
MOCK_TIME},
service_{&profile_} {}

void SetNonClickerPolicy(base::test::ScopedFeatureList& feature_list,
FeaturePromoSnoozeService::NonClickerPolicy policy) {
std::map<std::string, std::string> parameters = {
{"x_iph_snooze_non_clicker_policy",
policy == FeaturePromoSnoozeService::NonClickerPolicy::kDismiss
? "dismiss"
: "long_snooze"}};
feature_list.InitAndEnableFeatureWithParameters(kTestIPHFeature,
parameters);
}

protected:
content::BrowserTaskEnvironment task_environment_;
TestingProfile profile_;
BrowserFeaturePromoSnoozeService service_;
};

TEST_F(FeaturePromoSnoozeServiceTest, AllowFirstTimeIPH) {
TEST_F(BrowserFeaturePromoSnoozeServiceTest, AllowFirstTimeIPH) {
service_.Reset(kTestIPHFeature);
EXPECT_FALSE(service_.IsBlocked(kTestIPHFeature));
}

TEST_F(FeaturePromoSnoozeServiceTest, BlockDismissedIPH) {
TEST_F(BrowserFeaturePromoSnoozeServiceTest, BlockDismissedIPH) {
service_.Reset(kTestIPHFeature);
service_.OnPromoShown(kTestIPHFeature);
service_.OnUserDismiss(kTestIPHFeature);
Expand All @@ -62,14 +53,14 @@ TEST_F(FeaturePromoSnoozeServiceTest, BlockDismissedIPH) {
EXPECT_FALSE(service_.IsBlocked(kTestIPHFeature));
}

TEST_F(FeaturePromoSnoozeServiceTest, BlockSnoozedIPH) {
TEST_F(BrowserFeaturePromoSnoozeServiceTest, BlockSnoozedIPH) {
service_.Reset(kTestIPHFeature);
service_.OnPromoShown(kTestIPHFeature);
service_.OnUserSnooze(kTestIPHFeature);
EXPECT_TRUE(service_.IsBlocked(kTestIPHFeature));
}

TEST_F(FeaturePromoSnoozeServiceTest, ReleaseSnoozedIPH) {
TEST_F(BrowserFeaturePromoSnoozeServiceTest, ReleaseSnoozedIPH) {
service_.Reset(kTestIPHFeature);
service_.OnPromoShown(kTestIPHFeature);
service_.OnUserSnooze(kTestIPHFeature, base::Hours(1));
Expand All @@ -78,7 +69,7 @@ TEST_F(FeaturePromoSnoozeServiceTest, ReleaseSnoozedIPH) {
EXPECT_FALSE(service_.IsBlocked(kTestIPHFeature));
}

TEST_F(FeaturePromoSnoozeServiceTest, MultipleIPH) {
TEST_F(BrowserFeaturePromoSnoozeServiceTest, MultipleIPH) {
service_.Reset(kTestIPHFeature);
service_.Reset(kTestIPHFeature2);
service_.OnPromoShown(kTestIPHFeature);
Expand All @@ -95,7 +86,7 @@ TEST_F(FeaturePromoSnoozeServiceTest, MultipleIPH) {
EXPECT_FALSE(service_.IsBlocked(kTestIPHFeature2));
}

TEST_F(FeaturePromoSnoozeServiceTest, SnoozeNonClicker) {
TEST_F(BrowserFeaturePromoSnoozeServiceTest, SnoozeNonClicker) {
base::test::ScopedFeatureList feature_list;
service_.Reset(kTestIPHFeature);
service_.OnPromoShown(kTestIPHFeature);
Expand Down

0 comments on commit 764e635

Please sign in to comment.