Skip to content

Commit

Permalink
[M103 Merge][side search] Add per-tab, per-window and per-profile lab…
Browse files Browse the repository at this point in the history
…el triggers

This CL adds per-tab, per-window and per-profile triggering for the
SideSearchIconView's label text in the omnibox.

This expands on the current setting which is fixed at per-profile.
This CL gates the various options behind a feature flag and sets
the per-window setting as default.

Test with
--enable-features=SideSearch,SideSearchDSESupport,SidePanelImprovedClobbering,SideSearchPageActionLabelAnimation:SideSearchPageActionLabelAnimationFrequency/OncePerWindow

(cherry picked from commit c9e42d4)

Bug: 1325511
Change-Id: I6b934dd1b0e88361d7b24c7d888355ccb2de26c9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3648394
Reviewed-by: Yuheng Huang <yuhengh@chromium.org>
Commit-Queue: Thomas Lukaszewicz <tluk@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1004556}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3661364
Cr-Commit-Position: refs/branch-heads/5060@{#194}
Cr-Branched-From: b83393d-refs/heads/main@{#1002911}
  • Loading branch information
Tom Lukaszewicz authored and Chromium LUCI CQ committed May 23, 2022
1 parent a6a90a0 commit b433692
Show file tree
Hide file tree
Showing 7 changed files with 112 additions and 12 deletions.
2 changes: 1 addition & 1 deletion chrome/browser/ui/side_search/side_search_config.cc
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ void SideSearchConfig::RemoveObserver(Observer* observer) {
void SideSearchConfig::ResetStateAndNotifyConfigChanged() {
// Reset the availabiliy bit before propagating notifications.
is_side_panel_srp_available_ = false;
should_show_page_action_label_ = true;
page_action_label_shown_ = false;

for (auto& observer : observers_)
observer.OnSideSearchConfigChanged();
Expand Down
10 changes: 4 additions & 6 deletions chrome/browser/ui/side_search/side_search_config.h
Original file line number Diff line number Diff line change
Expand Up @@ -78,11 +78,9 @@ class SideSearchConfig : public base::SupportsUserData::Data,
// changes.
void ResetStateAndNotifyConfigChanged();

bool should_show_page_action_label() const {
return should_show_page_action_label_;
}
void set_should_show_page_action_label(bool should_show_page_action_label) {
should_show_page_action_label_ = should_show_page_action_label;
bool page_action_label_shown() const { return page_action_label_shown_; }
void set_page_action_label_shown(bool label_shown) {
page_action_label_shown_ = label_shown;
}

// TODO(crbug.com/1304513): Allow tests to specify the Google Search
Expand All @@ -96,7 +94,7 @@ class SideSearchConfig : public base::SupportsUserData::Data,

// Tracks whether the page action icon has animated-in its label text. Track
// this to ensure we only show this at most once per profile per session.
bool should_show_page_action_label_ = true;
bool page_action_label_shown_ = false;

raw_ptr<Profile> const profile_;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,11 @@ class SideSearchTabContentsHelper
bool toggled_open() const { return toggled_open_; }
void set_toggled_open(bool toggled_open) { toggled_open_ = toggled_open; }

bool page_action_label_shown() const { return page_action_label_shown_; }
void set_page_action_label_shown(bool was_shown) {
page_action_label_shown_ = was_shown;
}

void SetSidePanelContentsForTesting(
std::unique_ptr<content::WebContents> side_panel_contents);

Expand Down Expand Up @@ -187,6 +192,10 @@ class SideSearchTabContentsHelper
// navigation.
bool could_show_for_last_committed_navigation_ = false;

// Tracks whether the page action icon has animated-in its label text. Track
// this to ensure we only show the label at most once per tab.
bool page_action_label_shown_ = false;

base::ScopedObservation<SideSearchConfig, SideSearchConfig::Observer>
config_observation_{this};

Expand Down
22 changes: 22 additions & 0 deletions chrome/browser/ui/ui_features.cc
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,28 @@ const base::Feature kSideSearchFeedback{"SideSearchFeedback",
const base::Feature kSideSearchDSESupport{"SideSearchDSESupport",
base::FEATURE_DISABLED_BY_DEFAULT};

// Controls whether the side search icon animates-in its label when the side
// panel is made available for the active tab.
const base::Feature kSideSearchPageActionLabelAnimation{
"SideSearchPageActionLabelAnimation", base::FEATURE_ENABLED_BY_DEFAULT};

// Controls the frequency that the Side Search page action's label is shown. If
// enabled the label text is shown one per window.
const base::FeatureParam<kSideSearchLabelAnimationFrequencyOption>::Option
kSideSearchPageActionLabelAnimationFrequencyParamOptions[] = {
{kSideSearchLabelAnimationFrequencyOption::kOncePerProfile,
"OncePerProfile"},
{kSideSearchLabelAnimationFrequencyOption::kOncePerWindow,
"OncePerWindow"},
{kSideSearchLabelAnimationFrequencyOption::kOncePerTab, "OncePerTab"}};

const base::FeatureParam<kSideSearchLabelAnimationFrequencyOption>
kSideSearchPageActionLabelAnimationFrequency{
&kSideSearchPageActionLabelAnimation,
"SideSearchPageActionLabelAnimationFrequency",
kSideSearchLabelAnimationFrequencyOption::kOncePerWindow,
&kSideSearchPageActionLabelAnimationFrequencyParamOptions};

// Whether to clobber all side search side panels in the current browser window
// or only the side search in the current tab before read later or lens side
// panel is open.
Expand Down
10 changes: 10 additions & 0 deletions chrome/browser/ui/ui_features.h
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,16 @@ extern const base::Feature kSideSearchFeedback;
extern const base::Feature kSideSearchDSESupport;
extern const base::Feature kClobberAllSideSearchSidePanels;

extern const base::Feature kSideSearchPageActionLabelAnimation;

enum class kSideSearchLabelAnimationFrequencyOption {
kOncePerProfile,
kOncePerWindow,
kOncePerTab,
};
extern const base::FeatureParam<kSideSearchLabelAnimationFrequencyOption>
kSideSearchPageActionLabelAnimationFrequency;

extern const base::Feature kTabGroupsNewBadgePromo;

extern const base::Feature kTabGroupsSave;
Expand Down
60 changes: 55 additions & 5 deletions chrome/browser/ui/views/side_search/side_search_icon_view.cc
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "chrome/browser/ui/side_search/side_search_config.h"
#include "chrome/browser/ui/side_search/side_search_metrics.h"
#include "chrome/browser/ui/side_search/side_search_tab_contents_helper.h"
#include "chrome/browser/ui/ui_features.h"
#include "chrome/browser/ui/views/frame/browser_view.h"
#include "chrome/browser/ui/views/side_search/side_search_browser_controller.h"
#include "chrome/grit/generated_resources.h"
Expand Down Expand Up @@ -70,11 +71,8 @@ void SideSearchIconView::UpdateImpl() {
!tab_contents_helper->toggled_open();
SetVisible(should_show);

auto* side_search_config =
SideSearchConfig::Get(active_contents->GetBrowserContext());
if (should_show && !was_visible &&
side_search_config->should_show_page_action_label()) {
side_search_config->set_should_show_page_action_label(false);
if (should_show && !was_visible && ShouldShowPageActionLabel()) {
SetPageActionLabelShown();
should_extend_label_shown_duration_ = true;
AnimateIn(absl::nullopt);
}
Expand All @@ -86,6 +84,11 @@ void SideSearchIconView::OnExecuting(PageActionIconView::ExecuteSource source) {
RecordSideSearchPageActionLabelVisibilityOnToggle(
label()->GetVisible() ? SideSearchPageActionLabelVisibility::kVisible
: SideSearchPageActionLabelVisibility::kNotVisible);

// Reset the slide animation if in progress.
UnpauseAnimation();
ResetSlideAnimation(false);

side_search_browser_controller->ToggleSidePanel();
}

Expand Down Expand Up @@ -129,5 +132,52 @@ void SideSearchIconView::AnimationProgressed(const gfx::Animation* animation) {
}
}

bool SideSearchIconView::ShouldShowPageActionLabel() const {
content::WebContents* active_contents = GetWebContents();
DCHECK(active_contents);

auto* tab_contents_helper =
SideSearchTabContentsHelper::FromWebContents(active_contents);
DCHECK(tab_contents_helper);

switch (features::kSideSearchPageActionLabelAnimationFrequency.Get()) {
case features::kSideSearchLabelAnimationFrequencyOption::kOncePerProfile: {
// Only checking the per-profile bit in the config is necessary.
auto* side_search_config =
SideSearchConfig::Get(active_contents->GetBrowserContext());
return !side_search_config->page_action_label_shown();
}
case features::kSideSearchLabelAnimationFrequencyOption::kOncePerWindow: {
// Show the label for the current window only if it hasn't been shown
// already for the active tab. This covers the case where the user drags
// a tab with the side search page action icon active into a new window.
return !page_action_label_shown_ &&
!tab_contents_helper->page_action_label_shown();
}
case features::kSideSearchLabelAnimationFrequencyOption::kOncePerTab:
// Only checking the per-tab bit is necessary.
return !tab_contents_helper->page_action_label_shown();
}
}

void SideSearchIconView::SetPageActionLabelShown() {
content::WebContents* active_contents = GetWebContents();
DCHECK(active_contents);

// Set the shown bit at the profile level.
auto* side_search_config =
SideSearchConfig::Get(active_contents->GetBrowserContext());
side_search_config->set_page_action_label_shown(true);

// Set the shown bit at the browser level.
page_action_label_shown_ = true;

// Set the shown bit at the tab level.
auto* tab_contents_helper =
SideSearchTabContentsHelper::FromWebContents(active_contents);
DCHECK(tab_contents_helper);
tab_contents_helper->set_page_action_label_shown(true);
}

BEGIN_METADATA(SideSearchIconView, PageActionIconView)
END_METADATA
11 changes: 11 additions & 0 deletions chrome/browser/ui/views/side_search/side_search_icon_view.h
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,17 @@ class SideSearchIconView : public PageActionIconView {
void AnimationProgressed(const gfx::Animation* animation) override;

private:
// Returns true if we should animate-in the page action icon label when the
// side search page action icon is shown in the omnibox.
bool ShouldShowPageActionLabel() const;

// Called when the page action icon label has been shown.
void SetPageActionLabelShown();

// Tracks whether the page action icon has animated-in its label text. Track
// this to ensure we only show this at most once per window.
bool page_action_label_shown_ = false;

raw_ptr<Browser> browser_ = nullptr;

// Source for the default search icon image used by this page action.
Expand Down

0 comments on commit b433692

Please sign in to comment.