ChromeLabs: Add New Badge and configuration logic in PrefService
This CL does not integrate the New Badge with IPH because doing so
would require each experiment to have it’s own IPH configuration.
However, we want feature teams to be able to add their experiments into
Chrome Labs with ease. We have therefore decided to proceed with a
simplified approach that shows the new badge label for 7 days once the
user first sees the badge. (Note that Chrome Labs will only launch to
Canary, Dev, and Beta.) We will monitor response to the new badge label
and reduce the time frame if needed.
This CL registers a dictionary pref and adds new experiments to the
desired PrefService when instantiating the Chrome Labs button in the
toolbar. A future CL will use information from this dictionary pref
to also show a blue dot on the Chrome Labs button if there are new
experiments (See Mocks in Bug).
Dictionary values will be cleaned up if the experiment is removed
from Chrome Labs.
Bug: 1218247
Change-Id: I72b47e7637706e6857bfe3029fcf267c0f366477
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2951650
Reviewed-by: Dominic Battré <[email protected]>
Reviewed-by: Erik Chen <[email protected]>
Reviewed-by: Dana Fried <[email protected]>
Commit-Queue: Elaine Chien <[email protected]>
Cr-Commit-Position: refs/heads/master@{#905471}
diff --git a/chrome/browser/prefs/browser_prefs.cc b/chrome/browser/prefs/browser_prefs.cc
index a760a6e..c97b999 100644
--- a/chrome/browser/prefs/browser_prefs.cc
+++ b/chrome/browser/prefs/browser_prefs.cc
@@ -859,6 +859,9 @@
data_use_measurement::ChromeDataUseMeasurement::RegisterPrefs(registry);
BrowserProcessImpl::RegisterPrefs(registry);
ChromeContentBrowserClient::RegisterLocalStatePrefs(registry);
+#if !BUILDFLAG(IS_CHROMEOS_ASH)
+ chrome_labs_prefs::RegisterLocalStatePrefs(registry);
+#endif
ChromeMetricsServiceClient::RegisterPrefs(registry);
ChromeTracingDelegate::RegisterPrefs(registry);
chrome::enterprise_util::RegisterLocalStatePrefs(registry);
diff --git a/chrome/browser/ui/toolbar/chrome_labs_prefs.cc b/chrome/browser/ui/toolbar/chrome_labs_prefs.cc
index f9adb99..5ab9f03 100644
--- a/chrome/browser/ui/toolbar/chrome_labs_prefs.cc
+++ b/chrome/browser/ui/toolbar/chrome_labs_prefs.cc
@@ -12,8 +12,39 @@
// with toolbar entry point. This pref is mapped to an enterprise policy value.
const char kBrowserLabsEnabled[] = "browser_labs_enabled";
+#if BUILDFLAG(IS_CHROMEOS_ASH)
+// For ash-chrome which will use profile prefs. Dictionary pref mapping
+// experiment internal names to the day the experiment was added to the
+// ChromeLabs bubble. This will be used to track time since the new badge was
+// first shown.
+const char kChromeLabsNewBadgeDictAshChrome[] =
+ "chrome_labs_new_badge_dict_ash_chrome";
+
+#else
+// For all other desktop platforms which will use Local State. Dictionary pref
+// mapping experiment internal names to the day the experiment was added to the
+// ChromeLabs bubble. This will be used to track time since the new badge was
+// first shown.
+const char kChromeLabsNewBadgeDict[] = "chrome_labs_new_badge_dict";
+
+#endif
+
+// Sentinel time value for |kChromeLabsNewBadgeDict| pref to indicate the
+// experiment is new but not seen by the user yet. Non-sentinel time values
+// are guaranteed to be non-negative.
+const int kChromeLabsNewExperimentPrefValue = -1;
+
void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry) {
registry->RegisterBooleanPref(kBrowserLabsEnabled, true);
+#if BUILDFLAG(IS_CHROMEOS_ASH)
+ registry->RegisterDictionaryPref(kChromeLabsNewBadgeDictAshChrome);
+#endif
}
+#if !BUILDFLAG(IS_CHROMEOS_ASH)
+void RegisterLocalStatePrefs(PrefRegistrySimple* registry) {
+ registry->RegisterDictionaryPref(kChromeLabsNewBadgeDict);
+}
+#endif
+
} // namespace chrome_labs_prefs
diff --git a/chrome/browser/ui/toolbar/chrome_labs_prefs.h b/chrome/browser/ui/toolbar/chrome_labs_prefs.h
index 61cd9704..4626bf99 100644
--- a/chrome/browser/ui/toolbar/chrome_labs_prefs.h
+++ b/chrome/browser/ui/toolbar/chrome_labs_prefs.h
@@ -5,6 +5,10 @@
#ifndef CHROME_BROWSER_UI_TOOLBAR_CHROME_LABS_PREFS_H_
#define CHROME_BROWSER_UI_TOOLBAR_CHROME_LABS_PREFS_H_
+#include "build/buildflag.h"
+#include "build/chromeos_buildflags.h"
+
+class PrefRegistrySimple;
namespace user_prefs {
class PrefRegistrySyncable;
}
@@ -12,8 +16,17 @@
namespace chrome_labs_prefs {
extern const char kBrowserLabsEnabled[];
+#if BUILDFLAG(IS_CHROMEOS_ASH)
+extern const char kChromeLabsNewBadgeDictAshChrome[];
+#else
+extern const char kChromeLabsNewBadgeDict[];
+#endif
+extern const int kChromeLabsNewExperimentPrefValue;
void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
+#if !BUILDFLAG(IS_CHROMEOS_ASH)
+void RegisterLocalStatePrefs(PrefRegistrySimple* registry);
+#endif
} // namespace chrome_labs_prefs
diff --git a/chrome/browser/ui/views/toolbar/chrome_labs_bubble_view_unittest.cc b/chrome/browser/ui/views/toolbar/chrome_labs_bubble_view_unittest.cc
index b52d0405..7a5a14e 100644
--- a/chrome/browser/ui/views/toolbar/chrome_labs_bubble_view_unittest.cc
+++ b/chrome/browser/ui/views/toolbar/chrome_labs_bubble_view_unittest.cc
@@ -4,8 +4,11 @@
#include "chrome/browser/ui/views/toolbar/chrome_labs_bubble_view.h"
+#include "base/containers/cxx20_erase_vector.h"
#include "base/test/metrics/histogram_tester.h"
#include "base/test/scoped_feature_list.h"
+#include "base/test/task_environment.h"
+#include "base/time/time.h"
#include "build/chromeos_buildflags.h"
#include "chrome/browser/about_flags.h"
#include "chrome/browser/ui/toolbar/chrome_labs_prefs.h"
@@ -14,7 +17,9 @@
#include "chrome/browser/ui/views/frame/test_with_browser_view.h"
#include "chrome/browser/ui/views/toolbar/chrome_labs_bubble_view_model.h"
#include "chrome/browser/ui/views/toolbar/chrome_labs_button.h"
+#include "chrome/browser/ui/views/toolbar/chrome_labs_utils.h"
#include "chrome/browser/ui/views/toolbar/toolbar_view.h"
+#include "chrome/browser/ui/views/user_education/new_badge_label.h"
#include "chrome/browser/unexpire_flags.h"
#include "chrome/test/base/testing_browser_process.h"
#include "components/flags_ui/feature_entry_macros.h"
@@ -71,7 +76,8 @@
class ChromeLabsBubbleTest : public TestWithBrowserView {
public:
ChromeLabsBubbleTest()
- :
+ : TestWithBrowserView(
+ base::test::SingleThreadTaskEnvironment::TimeSource::MOCK_TIME),
#if BUILDFLAG(IS_CHROMEOS_ASH)
user_manager_(new ash::FakeChromeUserManager()),
user_manager_enabler_(base::WrapUnique(user_manager_)),
@@ -191,7 +197,6 @@
return true;
}
- private:
std::vector<LabInfo> TestLabInfo() {
std::vector<LabInfo> test_feature_info;
test_feature_info.emplace_back(LabInfo(kFirstTestFeatureId, u"", u"", "",
@@ -212,6 +217,10 @@
return test_feature_info;
}
+ protected:
+ ScopedChromeLabsModelDataForTesting scoped_chrome_labs_model_data_;
+
+ private:
#if BUILDFLAG(IS_CHROMEOS_ASH)
ash::FakeChromeUserManager* user_manager_;
user_manager::ScopedUserManager user_manager_enabler_;
@@ -219,8 +228,6 @@
about_flags::testing::ScopedFeatureEntries scoped_feature_entries_;
base::test::ScopedFeatureList scoped_feature_list_;
-
- ScopedChromeLabsModelDataForTesting scoped_chrome_labs_model_data_;
};
class ChromeLabsFeatureTest : public ChromeLabsBubbleTest,
@@ -443,3 +450,49 @@
histogram_tester.ExpectTotalCount("Feedback.RequestSource", 1);
}
#endif
+
+// This test checks the new badge shows and that after 8 days the new badge is
+// not showing anymore.
+TEST_F(ChromeLabsBubbleTest, NewBadgeTest) {
+ EXPECT_TRUE(first_lab_item()->GetNewBadgeForTesting()->GetDisplayNewBadge());
+ ChromeLabsBubbleView::Hide();
+ constexpr base::TimeDelta kDelay = base::TimeDelta::FromDays(8);
+ task_environment()->AdvanceClock(kDelay);
+ ChromeLabsBubbleView::Show(chrome_labs_button(), browser_view()->browser(),
+ chrome_labs_model(),
+ /*user_is_chromeos_owner=*/false);
+ EXPECT_FALSE(first_lab_item()->GetNewBadgeForTesting()->GetDisplayNewBadge());
+}
+
+// This test checks that experiments that are removed from the model will be
+// removed from the PrefService when updating new badge prefs.
+TEST_F(ChromeLabsBubbleTest, CleanUpNewBadgePrefsTest) {
+#if BUILDFLAG(IS_CHROMEOS_ASH)
+ const base::DictionaryValue* new_badge_prefs =
+ browser_view()->browser()->profile()->GetPrefs()->GetDictionary(
+ chrome_labs_prefs::kChromeLabsNewBadgeDictAshChrome);
+#else
+ const base::DictionaryValue* new_badge_prefs =
+ g_browser_process->local_state()->GetDictionary(
+ chrome_labs_prefs::kChromeLabsNewBadgeDict);
+#endif
+
+ EXPECT_TRUE(new_badge_prefs->HasKey(kFirstTestFeatureId));
+ EXPECT_TRUE(new_badge_prefs->HasKey(kTestFeatureWithVariationId));
+
+ // Remove two experiments.
+ std::vector<LabInfo> test_experiments = TestLabInfo();
+ base::EraseIf(test_experiments, [](const auto& lab) {
+ return lab.internal_name == kFirstTestFeatureId;
+ });
+ base::EraseIf(test_experiments, [](const auto& lab) {
+ return lab.internal_name == kTestFeatureWithVariationId;
+ });
+
+ scoped_chrome_labs_model_data_.SetModelDataForTesting(test_experiments);
+
+ UpdateChromeLabsNewBadgePrefs(browser_view()->browser()->profile(),
+ chrome_labs_model());
+ EXPECT_FALSE(new_badge_prefs->HasKey(kFirstTestFeatureId));
+ EXPECT_FALSE(new_badge_prefs->HasKey(kTestFeatureWithVariationId));
+}
diff --git a/chrome/browser/ui/views/toolbar/chrome_labs_item_view.cc b/chrome/browser/ui/views/toolbar/chrome_labs_item_view.cc
index bb09257..78bffabf 100644
--- a/chrome/browser/ui/views/toolbar/chrome_labs_item_view.cc
+++ b/chrome/browser/ui/views/toolbar/chrome_labs_item_view.cc
@@ -3,12 +3,21 @@
// found in the LICENSE file.
#include "chrome/browser/ui/views/toolbar/chrome_labs_item_view.h"
+#include "base/time/time.h"
+#include "base/values.h"
#include "build/build_config.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/flag_descriptions.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/chrome_pages.h"
+#include "chrome/browser/ui/toolbar/chrome_labs_prefs.h"
#include "chrome/browser/ui/views/chrome_layout_provider.h"
#include "chrome/browser/ui/views/chrome_typography.h"
#include "chrome/browser/ui/views/toolbar/chrome_labs_bubble_view_model.h"
+#include "chrome/browser/ui/views/user_education/new_badge_label.h"
#include "chrome/grit/generated_resources.h"
+#include "components/prefs/scoped_user_pref_update.h"
#include "extensions/browser/api/feedback_private/feedback_private_api.h"
#include "ui/base/l10n/l10n_util.h"
#include "ui/base/metadata/metadata_impl_macros.h"
@@ -38,6 +47,11 @@
/* extra_diagnostics=*/std::string());
}
+uint32_t GetCurrentDay() {
+ base::TimeDelta delta = base::Time::Now() - base::Time::UnixEpoch();
+ return base::saturated_cast<uint32_t>(delta.InDays());
+}
+
} // namespace
class LabsComboboxModel : public ui::ComboboxModel {
@@ -109,14 +123,15 @@
DISTANCE_CONTROL_LIST_VERTICAL),
0)));
- views::Label* experiment_name;
+ experiment_name_ =
+ AddChildView(std::make_unique<NewBadgeLabel>(lab.visible_name));
+ experiment_name_->SetDisplayNewBadge(
+ ShouldShowNewBadge(browser->profile(), lab));
+ experiment_name_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+ experiment_name_->SetBadgePlacement(
+ NewBadgeLabel::BadgePlacement::kImmediatelyAfterText);
+
views::Label* experiment_description;
- AddChildView(views::Builder<views::Label>()
- .CopyAddressTo(&experiment_name)
- .SetTextContext(views::style::CONTEXT_DIALOG_BODY_TEXT)
- .SetText(lab.visible_name)
- .SetHorizontalAlignment(gfx::ALIGN_LEFT)
- .Build());
AddChildView(
views::Builder<views::Label>()
.CopyAddressTo(&experiment_description)
@@ -140,7 +155,7 @@
// descriptions when the bubble first opens. Experiment name and description
// will be read out when a user enters the grouping.
// See crbug.com/1145666 Accessibility review.
- experiment_name->GetViewAccessibility().OverrideIsIgnored(true);
+ experiment_name_->GetViewAccessibility().OverrideIsIgnored(true);
experiment_description->GetViewAccessibility().OverrideIsIgnored(true);
GetViewAccessibility().OverrideRole(ax::mojom::Role::kGroup);
GetViewAccessibility().OverrideName(lab.visible_name);
@@ -220,6 +235,41 @@
return feature_entry_;
}
+bool ChromeLabsItemView::ShouldShowNewBadge(Profile* profile,
+ const LabInfo& lab) {
+ // This experiment was added before adding the new badge and is not new.
+ if (lab.internal_name == flag_descriptions::kScrollableTabStripFlagId) {
+ return false;
+ }
+
+#if BUILDFLAG(IS_CHROMEOS_ASH)
+ DictionaryPrefUpdate update(
+ profile->GetPrefs(), chrome_labs_prefs::kChromeLabsNewBadgeDictAshChrome);
+#else
+ DictionaryPrefUpdate update(g_browser_process->local_state(),
+ chrome_labs_prefs::kChromeLabsNewBadgeDict);
+#endif
+
+ base::DictionaryValue* new_badge_prefs = update.Get();
+
+ DCHECK(new_badge_prefs->HasKey(lab.internal_name));
+ int start_day;
+ new_badge_prefs->GetInteger(lab.internal_name, &start_day);
+ if (start_day == chrome_labs_prefs::kChromeLabsNewExperimentPrefValue) {
+ // Set the dictionary value of this experiment to the number of days since
+ // epoch (1970-01-01). This value is the first day the user sees the new
+ // experiment in Chrome Labs and will be used to determine whether or not to
+ // show the new badge.
+ new_badge_prefs->SetInteger(lab.internal_name, GetCurrentDay());
+ return true;
+ } else {
+ int days_elapsed = GetCurrentDay() - start_day;
+ // Show the new badge for 7 days. If the users sets the clock such that the
+ // current day is now before |start_day| don’t show the new badge.
+ return (days_elapsed < 7) && (days_elapsed >= 0);
+ }
+}
+
BEGIN_METADATA(ChromeLabsItemView, views::View)
ADD_READONLY_PROPERTY_METADATA(int, SelectedIndex)
END_METADATA
diff --git a/chrome/browser/ui/views/toolbar/chrome_labs_item_view.h b/chrome/browser/ui/views/toolbar/chrome_labs_item_view.h
index 6bf1627..f9612f2 100644
--- a/chrome/browser/ui/views/toolbar/chrome_labs_item_view.h
+++ b/chrome/browser/ui/views/toolbar/chrome_labs_item_view.h
@@ -12,6 +12,8 @@
#include "ui/views/view.h"
class Browser;
+class NewBadgeLabel;
+class Profile;
struct LabInfo;
namespace views {
@@ -39,9 +41,15 @@
return feedback_button_;
}
+ NewBadgeLabel* GetNewBadgeForTesting() { return experiment_name_; }
+
const flags_ui::FeatureEntry* GetFeatureEntry();
private:
+ bool ShouldShowNewBadge(Profile* profile, const LabInfo& lab);
+
+ NewBadgeLabel* experiment_name_;
+
// Combobox with selected state of the lab.
views::Combobox* lab_state_combobox_;
diff --git a/chrome/browser/ui/views/toolbar/chrome_labs_utils.cc b/chrome/browser/ui/views/toolbar/chrome_labs_utils.cc
index 5674a75..7ea5fe8 100644
--- a/chrome/browser/ui/views/toolbar/chrome_labs_utils.cc
+++ b/chrome/browser/ui/views/toolbar/chrome_labs_utils.cc
@@ -3,12 +3,15 @@
// found in the LICENSE file.
#include "chrome/browser/ui/views/toolbar/chrome_labs_utils.h"
+#include "base/containers/contains.h"
#include "chrome/browser/about_flags.h"
#include "chrome/browser/browser_process.h"
#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/toolbar/chrome_labs_prefs.h"
#include "chrome/common/channel_info.h"
#include "components/flags_ui/feature_entry.h"
#include "components/flags_ui/pref_service_flags_storage.h"
+#include "components/prefs/scoped_user_pref_update.h"
bool IsFeatureSupportedOnChannel(const LabInfo& lab) {
return chrome::GetChannel() <= lab.allowed_channel;
@@ -41,3 +44,40 @@
!about_flags::ShouldSkipConditionalFeatureEntry(flags_storage.get(),
*entry);
}
+
+void UpdateChromeLabsNewBadgePrefs(Profile* profile,
+ const ChromeLabsBubbleViewModel* model) {
+#if BUILDFLAG(IS_CHROMEOS_ASH)
+ DictionaryPrefUpdate update(
+ profile->GetPrefs(), chrome_labs_prefs::kChromeLabsNewBadgeDictAshChrome);
+#else
+ DictionaryPrefUpdate update(g_browser_process->local_state(),
+ chrome_labs_prefs::kChromeLabsNewBadgeDict);
+#endif
+
+ base::DictionaryValue* new_badge_prefs = update.Get();
+
+ std::vector<std::string> lab_internal_names;
+ const std::vector<LabInfo>& all_labs = model->GetLabInfo();
+ for (const auto& lab : all_labs) {
+ if (IsChromeLabsFeatureValid(lab, profile)) {
+ lab_internal_names.push_back(lab.internal_name);
+ if (!new_badge_prefs->HasKey(lab.internal_name)) {
+ new_badge_prefs->SetInteger(
+ lab.internal_name,
+ chrome_labs_prefs::kChromeLabsNewExperimentPrefValue);
+ }
+ }
+ }
+ std::vector<std::string> entries_to_remove;
+ for (base::DictionaryValue::Iterator it(*new_badge_prefs); !it.IsAtEnd();
+ it.Advance()) {
+ // The size of |lab_internal_names| is capped around 3-5 elements.
+ if (!base::Contains(lab_internal_names, it.key())) {
+ entries_to_remove.push_back(it.key());
+ }
+ }
+
+ for (const std::string& key : entries_to_remove)
+ new_badge_prefs->Remove(key, nullptr);
+}
diff --git a/chrome/browser/ui/views/toolbar/chrome_labs_utils.h b/chrome/browser/ui/views/toolbar/chrome_labs_utils.h
index b9e3f9e..b7bf32b 100644
--- a/chrome/browser/ui/views/toolbar/chrome_labs_utils.h
+++ b/chrome/browser/ui/views/toolbar/chrome_labs_utils.h
@@ -12,4 +12,9 @@
// This is used across Chrome Labs classes to check if a feature is valid.
bool IsChromeLabsFeatureValid(const LabInfo& lab, Profile* profile);
+// Adds new experiments to PrefService and cleans up preferences for
+// experiments that are no longer featured.
+void UpdateChromeLabsNewBadgePrefs(Profile* profile,
+ const ChromeLabsBubbleViewModel* model);
+
#endif // CHROME_BROWSER_UI_VIEWS_TOOLBAR_CHROME_LABS_UTILS_H_
diff --git a/chrome/browser/ui/views/toolbar/toolbar_view.cc b/chrome/browser/ui/views/toolbar/toolbar_view.cc
index 9fe38374..8ab8a5d 100644
--- a/chrome/browser/ui/views/toolbar/toolbar_view.cc
+++ b/chrome/browser/ui/views/toolbar/toolbar_view.cc
@@ -57,6 +57,7 @@
#include "chrome/browser/ui/views/toolbar/browser_app_menu_button.h"
#include "chrome/browser/ui/views/toolbar/chrome_labs_bubble_view_model.h"
#include "chrome/browser/ui/views/toolbar/chrome_labs_button.h"
+#include "chrome/browser/ui/views/toolbar/chrome_labs_utils.h"
#include "chrome/browser/ui/views/toolbar/home_button.h"
#include "chrome/browser/ui/views/toolbar/read_later_toolbar_button.h"
#include "chrome/browser/ui/views/toolbar/reload_button.h"
@@ -307,6 +308,8 @@
if (base::FeatureList::IsEnabled(features::kChromeLabs)) {
chrome_labs_model_ = std::make_unique<ChromeLabsBubbleViewModel>();
+ UpdateChromeLabsNewBadgePrefs(browser_->profile(),
+ chrome_labs_model_.get());
if (ChromeLabsButton::ShouldShowButton(chrome_labs_model_.get(),
browser_->profile())) {
chrome_labs_button_ = AddChildView(std::make_unique<ChromeLabsButton>(
