[Shields UI] Add IPH for new shields panel location (#33094)

2026-01-15 09:57:57 -05:00
parent 4e1a164ac5
commit d5e2355caf
13 changed files with 265 additions and 41 deletions
@@ -47,9 +47,11 @@ source_set("location_bar") {
    "//chrome/browser/ui/color:mixers",
    "//chrome/browser/ui/omnibox",
    "//chrome/browser/ui/tabs:tab_strip",
+    "//chrome/browser/ui/user_education",
    "//chrome/browser/ui/views/location_bar",
    "//chrome/browser/ui/views/page_action",
    "//chrome/browser/ui/views/page_info",
+    "//components/feature_engagement/public:feature_constants",
    "//components/image_fetcher/core",
    "//components/search_engines",
    "//components/version_info:channel",
@@ -145,8 +145,7 @@ void BraveLocationBarView::Init() {

  if (page_info::features::IsShowBraveShieldsInPageInfoEnabled()) {
    shields_page_info_controller_ =
-        std::make_unique<BraveShieldsPageInfoController>(
-            browser_->GetTabStripModel(), location_icon_view());
+        std::make_unique<BraveShieldsPageInfoController>(location_icon_view());
  }

  // brave action buttons
@@ -198,6 +197,10 @@ void BraveLocationBarView::Update(content::WebContents* contents) {
  }
 #endif

+  if (shields_page_info_controller_) {
+    shields_page_info_controller_->UpdateWebContents(contents);
+  }
+
  LocationBarView::Update(contents);
 }

@@ -8,14 +8,18 @@
 #include "base/check_deref.h"
 #include "base/task/sequenced_task_runner.h"
 #include "brave/browser/ui/views/page_info/brave_page_info_bubble_view.h"
-#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/browser/ui/user_education/browser_user_education_interface.h"
 #include "chrome/browser/ui/views/location_bar/location_icon_view.h"
 #include "chrome/browser/ui/views/page_info/page_info_bubble_specification.h"
 #include "chrome/browser/ui/views/page_info/page_info_bubble_view.h"
+#include "components/content_settings/core/browser/content_settings_utils.h"
+#include "components/feature_engagement/public/feature_constants.h"
+#include "components/page_info/page_info.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/web_contents.h"
 #include "ui/views/view_utils.h"
+#include "url/gurl.h"

 namespace {

@@ -30,36 +34,37 @@ brave_shields::BraveShieldsTabHelper* GetShieldsHelper(
 }  // namespace

 BraveShieldsPageInfoController::BraveShieldsPageInfoController(
-    TabStripModel* tab_strip_model,
    LocationIconView* location_icon_view)
-    : tab_strip_model_(CHECK_DEREF(tab_strip_model)),
-      location_icon_view_(CHECK_DEREF(location_icon_view)) {
-  tab_strip_model_->AddObserver(this);
-  auto* web_contents = tab_strip_model_->GetActiveWebContents();
-  if (auto* shields_helper = GetShieldsHelper(web_contents)) {
-    shields_helper->AddObserver(this);
-  }
-}
+    : location_icon_view_(CHECK_DEREF(location_icon_view)) {}

 BraveShieldsPageInfoController::~BraveShieldsPageInfoController() {
-  auto* web_contents = tab_strip_model_->GetActiveWebContents();
-  if (auto* shields_helper = GetShieldsHelper(web_contents)) {
+  if (auto* shields_helper = GetShieldsHelper(web_contents())) {
    shields_helper->RemoveObserver(this);
  }
 }

-void BraveShieldsPageInfoController::OnTabStripModelChanged(
-    TabStripModel* tab_strip_model,
-    const TabStripModelChange& change,
-    const TabStripSelectionChange& selection) {
-  if (selection.active_tab_changed()) {
-    if (auto* shields_helper = GetShieldsHelper(selection.old_contents)) {
-      shields_helper->RemoveObserver(this);
-    }
-    if (auto* shields_helper = GetShieldsHelper(selection.new_contents)) {
-      shields_helper->AddObserver(this);
-    }
+void BraveShieldsPageInfoController::UpdateWebContents(
+    content::WebContents* contents) {
+  if (!contents || contents == web_contents()) {
+    return;
  }
+
+  // Stop observing the shields tab helper for the old WebContents.
+  if (auto* shields_helper = GetShieldsHelper(web_contents())) {
+    shields_helper->RemoveObserver(this);
+  }
+
+  // Start observing the new WebContents and its shields helper.
+  Observe(contents);
+  if (auto* shields_helper = GetShieldsHelper(contents)) {
+    shields_helper->AddObserver(this);
+  }
+
+  MaybeShowShieldsIPH();
+}
+
+void BraveShieldsPageInfoController::PrimaryPageChanged(content::Page& page) {
+  MaybeShowShieldsIPH();
 }

 void BraveShieldsPageInfoController::OnResourcesChanged() {}
@@ -76,14 +81,40 @@ void BraveShieldsPageInfoController::OnRepeatedReloadsDetected() {
          weak_factory_.GetWeakPtr()));
 }

-void BraveShieldsPageInfoController::ShowBubbleForRepeatedReloads() {
-  auto* web_contents = tab_strip_model_->GetActiveWebContents();
-  if (!web_contents) {
+void BraveShieldsPageInfoController::MaybeShowShieldsIPH() {
+  if (!web_contents()) {
    return;
  }

  content::NavigationEntry* entry =
-      web_contents->GetController().GetVisibleEntry();
+      web_contents()->GetController().GetVisibleEntry();
+  if (entry->IsInitialEntry()) {
+    return;
+  }
+
+  // Only attempt to show the IPH if the "normal" PageInfo bubble will be
+  // displayed for this URL.
+  GURL url = entry->GetVirtualURL();
+  if (PageInfo::IsFileOrInternalPage(url) ||
+      url.SchemeIs(content_settings::kExtensionScheme)) {
+    return;
+  }
+
+  if (auto* user_education =
+          BrowserUserEducationInterface::MaybeGetForWebContentsInTab(
+              web_contents())) {
+    user_education->MaybeShowFeaturePromo(
+        feature_engagement::kIPHBraveShieldsInPageInfoFeature);
+  }
+}
+
+void BraveShieldsPageInfoController::ShowBubbleForRepeatedReloads() {
+  if (!web_contents()) {
+    return;
+  }
+
+  content::NavigationEntry* entry =
+      web_contents()->GetController().GetVisibleEntry();
  if (!entry || entry->IsInitialEntry()) {
    return;
  }
@@ -91,7 +122,7 @@ void BraveShieldsPageInfoController::ShowBubbleForRepeatedReloads() {
  std::unique_ptr<PageInfoBubbleSpecification> specification =
      PageInfoBubbleSpecification::Builder(
          &location_icon_view_.get(),
-          location_icon_view_->GetWidget()->GetNativeWindow(), web_contents,
+          location_icon_view_->GetWidget()->GetNativeWindow(), web_contents(),
          entry->GetVirtualURL())
          .Build();

@@ -9,19 +9,17 @@
 #include "base/memory/raw_ref.h"
 #include "base/memory/weak_ptr.h"
 #include "brave/browser/brave_shields/brave_shields_tab_helper.h"
-#include "chrome/browser/ui/tabs/tab_strip_model_observer.h"
+#include "content/public/browser/web_contents_observer.h"

 class LocationIconView;
-class TabStripModel;

 // Controller that listens for Brave Shields events for the current active tab
 // and opens the Page Info bubble when appropriate.
 class BraveShieldsPageInfoController
    : public brave_shields::BraveShieldsTabHelper::Observer,
-      public TabStripModelObserver {
+      public content::WebContentsObserver {
 public:
-  BraveShieldsPageInfoController(TabStripModel* tab_strip_model,
-                                 LocationIconView* location_icon_view);
+  explicit BraveShieldsPageInfoController(LocationIconView* location_icon_view);

  BraveShieldsPageInfoController(const BraveShieldsPageInfoController&) =
      delete;
@@ -30,20 +28,19 @@ class BraveShieldsPageInfoController

  ~BraveShieldsPageInfoController() override;

-  // TabStripModelObserver:
-  void OnTabStripModelChanged(
-      TabStripModel* tab_strip_model,
-      const TabStripModelChange& change,
-      const TabStripSelectionChange& selection) override;
+  void UpdateWebContents(content::WebContents* web_contents);
+
+  // content::WebContentsObserver:
+  void PrimaryPageChanged(content::Page& page) override;

  // brave_shields::BraveShieldsTabHelper::Observer:
  void OnResourcesChanged() override;
  void OnRepeatedReloadsDetected() override;

 private:
+  void MaybeShowShieldsIPH();
  void ShowBubbleForRepeatedReloads();

-  raw_ref<TabStripModel> tab_strip_model_;
  raw_ref<LocationIconView> location_icon_view_;
  base::WeakPtrFactory<BraveShieldsPageInfoController> weak_factory_{this};
 };
@@ -0,0 +1,53 @@
+/* Copyright (c) 2026 The Brave Authors. All rights reserved.
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at https://mozilla.org/MPL/2.0/. */
+
+#include "chrome/browser/ui/views/user_education/browser_user_education_service.h"
+
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/browser_element_identifiers.h"
+#include "components/feature_engagement/public/feature_constants.h"
+#include "components/grit/brave_components_strings.h"
+#include "components/user_education/common/feature_promo/feature_promo_registry.h"
+#include "components/user_education/common/feature_promo/feature_promo_specification.h"
+
+// This override allows Brave to register its own in-product help features with
+// the user education system's feature promo registry.
+
+namespace {
+
+// Registers Brave-specific in-product help features.
+void MaybeRegisterBraveFeaturePromos(
+    user_education::FeaturePromoRegistry& registry,
+    Profile* profile) {
+  using user_education::FeaturePromoSpecification;
+  using user_education::HelpBubbleArrow;
+
+  // IPH for Brave Shields in Page Info.
+  registry.RegisterFeature(std::move(
+      FeaturePromoSpecification::CreateForToastPromo(
+          feature_engagement::kIPHBraveShieldsInPageInfoFeature,
+          kLocationIconElementId, IDS_BRAVE_SHIELDS_PAGE_INFO_IPH_TEXT,
+          IDS_BRAVE_SHIELDS_PAGE_INFO_IPH_SCREENREADER_TEXT,
+          FeaturePromoSpecification::AcceleratorInfo())
+          .SetBubbleArrow(HelpBubbleArrow::kBottomCenter)
+          .SetMetadata(144, "ksmith@brave.com",
+                       "Shown when user visits a site for the first time after "
+                       "the Shields UI was moved to Page Info. Educates users "
+                       "about the new location.")));
+}
+
+}  // namespace
+
+// BRAVE_MAYBE_REGISTER_CHROME_FEATURE_PROMOS is patched near the beginning of
+// MaybeRegisterChromeFeaturePromos, and allows us to register Brave-specific
+// in-product help features. Patching is necessary because the token
+// "MaybeRegisterChromeFeaturePromos" appears several times in the source file
+// in different contexts.
+#define BRAVE_MAYBE_REGISTER_CHROME_FEATURE_PROMOS \
+  MaybeRegisterBraveFeaturePromos(registry, profile);
+
+#include <chrome/browser/ui/views/user_education/browser_user_education_service.cc>
+
+#undef BRAVE_MAYBE_REGISTER_CHROME_FEATURE_PROMOS
@@ -5,10 +5,19 @@

 #include "base/feature_override.h"

+// This override allows Brave to define its own IPH features, and to modify the
+// IPH features defined by Chromium.
+
 #include <components/feature_engagement/public/feature_constants.cc>

 namespace feature_engagement {

+#if BUILDFLAG(IS_WIN) || BUILDFLAG(IS_MAC) || BUILDFLAG(IS_LINUX)
+BASE_FEATURE(kIPHBraveShieldsInPageInfoFeature,
+             "IPH_BraveShieldsInPageInfo",
+             base::FEATURE_ENABLED_BY_DEFAULT);
+#endif
+
 OVERRIDE_FEATURE_DEFAULT_STATES({{
 #if BUILDFLAG(IS_WIN) || BUILDFLAG(IS_APPLE) || BUILDFLAG(IS_LINUX)
    {kIPHGMCCastStartStopFeature, base::FEATURE_DISABLED_BY_DEFAULT},
@@ -0,0 +1,23 @@
+/* Copyright (c) 2026 The Brave Authors. All rights reserved.
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at https://mozilla.org/MPL/2.0/. */
+
+#ifndef BRAVE_CHROMIUM_SRC_COMPONENTS_FEATURE_ENGAGEMENT_PUBLIC_FEATURE_CONSTANTS_H_
+#define BRAVE_CHROMIUM_SRC_COMPONENTS_FEATURE_ENGAGEMENT_PUBLIC_FEATURE_CONSTANTS_H_
+
+// This override allows Brave to declare its own IPH features. Features declared
+// here must be defined in "feature_constants.cc".
+
+#include <components/feature_engagement/public/feature_constants.h>  // IWYU pragma: export
+
+namespace feature_engagement {
+
+#if BUILDFLAG(IS_WIN) || BUILDFLAG(IS_MAC) || BUILDFLAG(IS_LINUX)
+// IPH for notifying users that Brave Shields settings have moved to Page Info.
+FEATURE_CONSTANTS_DECLARE_FEATURE(kIPHBraveShieldsInPageInfoFeature);
+#endif
+
+}  // namespace feature_engagement
+
+#endif  // BRAVE_CHROMIUM_SRC_COMPONENTS_FEATURE_ENGAGEMENT_PUBLIC_FEATURE_CONSTANTS_H_
@@ -0,0 +1,19 @@
+/* Copyright (c) 2026 The Brave Authors. All rights reserved.
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at https://mozilla.org/MPL/2.0/. */
+
+#include "components/feature_engagement/public/feature_list.h"
+
+// This override adds Brave-specific IPH features to the kAllFeatures array,
+// which is used by the feature engagement tracker and GetAllFeatures().
+
+// Replaces the first entry in the kAllFeatures array with that entry, plus any
+// additional entries for Brave-specific IPH features.
+#if BUILDFLAG(IS_WIN) || BUILDFLAG(IS_MAC) || BUILDFLAG(IS_LINUX)
+#define kIPHDummyFeature kIPHDummyFeature, &kIPHBraveShieldsInPageInfoFeature
+#endif
+
+#include <components/feature_engagement/public/feature_list.cc>
+
+#undef kIPHDummyFeature
@@ -0,0 +1,41 @@
+/* Copyright (c) 2026 The Brave Authors. All rights reserved.
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at https://mozilla.org/MPL/2.0/. */
+
+#ifndef BRAVE_CHROMIUM_SRC_COMPONENTS_FEATURE_ENGAGEMENT_PUBLIC_FEATURE_LIST_H_
+#define BRAVE_CHROMIUM_SRC_COMPONENTS_FEATURE_ENGAGEMENT_PUBLIC_FEATURE_LIST_H_
+
+#include "build/build_config.h"
+
+// This override allows Brave to display Brave-specific in-product help features
+// on the brave://flags page.
+
+// BRAVE_FEATURE_ENGAGEMENT_VARIATION_PARAMS is patched in after Chromium's
+// variation params are defined, and allows us to define Brave-specific IPH
+// variation params. A patch is necessary because the DEFINE_VARIATION_PARAM
+// macro is undef'ed at the end of the header file.
+#if BUILDFLAG(IS_WIN) || BUILDFLAG(IS_MAC) || BUILDFLAG(IS_LINUX)
+#define BRAVE_FEATURE_ENGAGEMENT_VARIATION_PARAMS           \
+  DEFINE_VARIATION_PARAM(kIPHBraveShieldsInPageInfoFeature, \
+                         "IPH_BraveShieldsInPageInfo");
+#else
+#define BRAVE_FEATURE_ENGAGEMENT_VARIATION_PARAMS
+#endif
+
+// BRAVE_FEATURE_ENGAGEMENT_VARIATION_ENTRIES is patched in at the start of the
+// kIPHDemoModeChoiceVariations array, and allows us to add Brave-specific IPH
+// variation entries to the array.
+#if BUILDFLAG(IS_WIN) || BUILDFLAG(IS_MAC) || BUILDFLAG(IS_LINUX)
+#define BRAVE_FEATURE_ENGAGEMENT_VARIATION_ENTRIES \
+  VARIATION_ENTRY(kIPHBraveShieldsInPageInfoFeature),
+#else
+#define BRAVE_FEATURE_ENGAGEMENT_VARIATION_ENTRIES
+#endif
+
+#include <components/feature_engagement/public/feature_list.h>  // IWYU pragma: export
+
+#undef BRAVE_FEATURE_ENGAGEMENT_VARIATION_PARAMS
+#undef BRAVE_FEATURE_ENGAGEMENT_VARIATION_ENTRIES
+
+#endif  // BRAVE_CHROMIUM_SRC_COMPONENTS_FEATURE_ENGAGEMENT_PUBLIC_FEATURE_LIST_H_
@@ -311,4 +311,13 @@
  <message name="IDS_BRAVE_SHIELDS_PAGE_INFO_SITE_INFO_LINK" desc="Link text for Site Info tab on Page Info bubble">
    Site Info
  </message>
+
+  <message name="IDS_BRAVE_SHIELDS_PAGE_INFO_IPH_TEXT" desc="Text for the IPH bubble shown when Brave Shields has moved to Page Info.">
+    Brave Shields has a new home and design. It now integrates with site information for a one-stop experience.
+  </message>
+
+  <message name="IDS_BRAVE_SHIELDS_PAGE_INFO_IPH_SCREENREADER_TEXT" desc="Screen reader text for the Brave Shields Page Info IPH bubble.">
+    Brave Shields has a new home and design. Access it by clicking the site icon in the address bar.
+  </message>
+
 </grit-part>
@@ -0,0 +1,12 @@
+diff --git a/chrome/browser/ui/views/user_education/browser_user_education_service.cc b/chrome/browser/ui/views/user_education/browser_user_education_service.cc
+index 5327bdb635e01bfe9a27da72f51dae9920d9d887..6e52d9f395731d2d746c0039107f12e6fb0f1326 100644
+--- a/chrome/browser/ui/views/user_education/browser_user_education_service.cc
+++ b/chrome/browser/ui/views/user_education/browser_user_education_service.cc
+@@ -268,6 +268,7 @@ void MaybeRegisterChromeFeaturePromos(
+           feature_engagement::kIPHWebUiHelpBubbleTestFeature)) {
+     return;
+   }
+  BRAVE_MAYBE_REGISTER_CHROME_FEATURE_PROMOS
+ 
+   // kIPHAutofillCreditCardBenefitFeature:
+   registry.RegisterFeature(std::move(
@@ -0,0 +1,19 @@
+diff --git a/components/feature_engagement/public/feature_list.h b/components/feature_engagement/public/feature_list.h
+index 000a1d75f5b84b9fcd62c25df22eb8375f3b291b..7a2e62f2e3d63995ef0b6c9c6e7928babcbbe654 100644
+--- a/components/feature_engagement/public/feature_list.h
+++ b/components/feature_engagement/public/feature_list.h
+@@ -607,12 +607,14 @@ DEFINE_VARIATION_PARAM(kIPHiOSLensPromoDesktopFeature,
+ DEFINE_VARIATION_PARAM(kIPHiOSEnhancedBrowsingDesktopFeature,
+                        "IPH_iOSEnhancedBrowsingDesktop");
+ #endif  // !BUILDFLAG(IS_ANDROID)
+BRAVE_FEATURE_ENGAGEMENT_VARIATION_PARAMS
+ 
+ // Defines the array of which features should be listed in the chrome://flags
+ // UI to be able to select them alone for demo-mode. The features listed here
+ // are possible to enable on their own in demo mode.
+ inline constexpr flags_ui::FeatureEntry::FeatureVariation
+     kIPHDemoModeChoiceVariations[] = {
+        BRAVE_FEATURE_ENGAGEMENT_VARIATION_ENTRIES
+ #if BUILDFLAG(IS_ANDROID)
+         VARIATION_ENTRY(kIPHAccountSettingsHistorySync),
+         VARIATION_ENTRY(
@@ -694,6 +694,12 @@
 # These tests fail because we don't show the history sync promo
 -BrowsingHistoryHandlerHistorySyncPromoTest.ShouldShowHistoryPageHistorySyncPromo*/SignedIn

+# These tests fail because we add IPH features but do not add associated UMA
+# histogram/actions XML entries (which are not needed for Brave since we don't
+# use Google's UMA for IPH metrics).
+-BrowserUserEducationServiceTest.CheckFeaturePromoActions
+-BrowserUserEducationServiceTest.CheckFeaturePromoHistograms
+
 # Tests below this point have not been diagnosed or had issues created yet.
 -AboutFlagsHistogramTest.*
 -AboutFlagsTest.EveryFlagHasMetadata