Skip to content

Commit

Permalink
ash: Remove flag ProductivityLauncherAnimation
Browse files Browse the repository at this point in the history
I added this when I first started working on animations. I left it in
because I thought it might be useful to allow users to disable
animations via about:flags.

However, it adds some complexity to production code. We're also not
doing any tests (except one) of the flag-off state.

We can add it back later if we decide we want it again.

Bug: none
Change-Id: I761ccb34db465d7d0d1926469d1b9a9398a7ad7e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3408173
Reviewed-by: Toni Barzic <tbarzic@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/main@{#962150}
  • Loading branch information
James Cook authored and Chromium LUCI CQ committed Jan 21, 2022
1 parent 08fe6a5 commit bc9f273
Show file tree
Hide file tree
Showing 16 changed files with 27 additions and 126 deletions.
19 changes: 5 additions & 14 deletions ash/app_list/app_list_bubble_presenter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -257,9 +257,7 @@ void AppListBubblePresenter::OnZeroStateSearchDone(int64_t display_id) {
// The page must be set before triggering the show animation so the correct
// animations are triggered.
bubble_view_->ShowPage(target_page_);
if (features::IsProductivityLauncherAnimationEnabled()) {
bubble_view_->StartShowAnimation();
}
bubble_view_->StartShowAnimation();
controller_->OnVisibilityChanged(/*visible=*/true, display_id);
}

Expand Down Expand Up @@ -296,17 +294,10 @@ void AppListBubblePresenter::Dismiss() {

controller_->ViewClosing();
controller_->OnVisibilityWillChange(/*visible=*/false, display_id);
if (features::IsProductivityLauncherAnimationEnabled()) {
if (bubble_view_) {
bubble_view_->StartHideAnimation(
base::BindOnce(&AppListBubblePresenter::OnHideAnimationEnded,
weak_factory_.GetWeakPtr()));
}
} else {
// Check for widget because the code could be waiting for zero-state search
// results before first show.
if (bubble_widget_)
OnHideAnimationEnded();
if (bubble_view_) {
bubble_view_->StartHideAnimation(
base::BindOnce(&AppListBubblePresenter::OnHideAnimationEnded,
weak_factory_.GetWeakPtr()));
}
controller_->OnVisibilityChanged(/*visible=*/false, display_id);

Expand Down
14 changes: 0 additions & 14 deletions ash/app_list/app_list_bubble_presenter_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -272,8 +272,6 @@ TEST_F(AppListBubblePresenterTest, CanShowWhileAnimatingClosed) {
presenter->Show(GetPrimaryDisplay().id());

// Enable animations.
base::test::ScopedFeatureList features(
features::kProductivityLauncherAnimation);
ui::ScopedAnimationDurationScaleMode duration(
ui::ScopedAnimationDurationScaleMode::NON_ZERO_DURATION);

Expand All @@ -292,8 +290,6 @@ TEST_F(AppListBubblePresenterTest, CanShowWhileAnimatingClosed) {

TEST_F(AppListBubblePresenterTest, DismissWhileWaitingForZeroStateSearch) {
// Simulate production behavior for animations and zero-state search results.
base::test::ScopedFeatureList features(
features::kProductivityLauncherAnimation);
ui::ScopedAnimationDurationScaleMode duration(
ui::ScopedAnimationDurationScaleMode::NON_ZERO_DURATION);
GetTestAppListClient()->set_run_zero_state_callback_immediately(false);
Expand All @@ -319,8 +315,6 @@ TEST_F(AppListBubblePresenterTest, DismissWhileWaitingForZeroStateSearch) {
TEST_F(AppListBubblePresenterTest, AssistantKeyOpensToAssistantPage) {
// Simulate production behavior for animations, assistant, and zero-state
// search results.
base::test::ScopedFeatureList features(
features::kProductivityLauncherAnimation);
ui::ScopedAnimationDurationScaleMode duration(
ui::ScopedAnimationDurationScaleMode::NON_ZERO_DURATION);
assistant_test_api_->EnableAssistantAndWait();
Expand All @@ -344,8 +338,6 @@ TEST_F(AppListBubblePresenterTest, AssistantKeyOpensAssistantPageWhenCached) {

// Simulate production behavior for animations, assistant, and zero-state
// search results.
base::test::ScopedFeatureList features(
features::kProductivityLauncherAnimation);
ui::ScopedAnimationDurationScaleMode duration(
ui::ScopedAnimationDurationScaleMode::NON_ZERO_DURATION);
assistant_test_api_->EnableAssistantAndWait();
Expand All @@ -363,8 +355,6 @@ TEST_F(AppListBubblePresenterTest, AssistantKeyOpensAssistantPageWhenCached) {
TEST_F(AppListBubblePresenterTest, AppsPageVisibleAfterShowingAssistant) {
// Simulate production behavior for animations, assistant, and zero-state
// search results.
base::test::ScopedFeatureList features(
features::kProductivityLauncherAnimation);
ui::ScopedAnimationDurationScaleMode duration(
ui::ScopedAnimationDurationScaleMode::NON_ZERO_DURATION);
assistant_test_api_->EnableAssistantAndWait();
Expand Down Expand Up @@ -394,8 +384,6 @@ TEST_F(AppListBubblePresenterTest, AppsPageVisibleAfterShowingAssistant) {
TEST_F(AppListBubblePresenterTest, SearchKeyOpensToAppsPage) {
// Simulate production behavior for animations, assistant, and zero-state
// search results.
base::test::ScopedFeatureList features(
features::kProductivityLauncherAnimation);
ui::ScopedAnimationDurationScaleMode duration(
ui::ScopedAnimationDurationScaleMode::NON_ZERO_DURATION);
assistant_test_api_->EnableAssistantAndWait();
Expand Down Expand Up @@ -526,8 +514,6 @@ TEST_F(AppListBubblePresenterTest, CreatingChildWidgetDoesNotCloseBubble) {
// Regression test for https://crbug.com/1285443.
TEST_F(AppListBubblePresenterTest, CanOpenBubbleThenOpenSystemTray) {
// Enable animations.
base::test::ScopedFeatureList features(
features::kProductivityLauncherAnimation);
ui::ScopedAnimationDurationScaleMode duration(
ui::ScopedAnimationDurationScaleMode::NON_ZERO_DURATION);

Expand Down
14 changes: 2 additions & 12 deletions ash/app_list/views/app_list_bubble_apps_page.cc
Original file line number Diff line number Diff line change
Expand Up @@ -106,13 +106,6 @@ AppListBubbleAppsPage::AppListBubbleAppsPage(

// Scroll view will have a gradient mask layer.
scroll_view_->SetPaintToLayer(ui::LAYER_NOT_DRAWN);
// When animations are enabled the gradient helper is created in the animation
// end callback.
if (!features::IsProductivityLauncherAnimationEnabled()) {
gradient_helper_ = std::make_unique<ScrollViewGradientHelper>(scroll_view_);
// Layout() updates the gradient zone, since the gradient helper needs to
// know the bounds of the scroll view and contents view.
}

// Set up scroll bars.
scroll_view_->SetHorizontalScrollBarMode(
Expand Down Expand Up @@ -256,10 +249,8 @@ void AppListBubbleAppsPage::AnimateShowPage() {
SetVisible(true);

// If skipping animations, just update visibility.
if (!features::IsProductivityLauncherAnimationEnabled() ||
ui::ScopedAnimationDurationScaleMode::is_zero()) {
if (ui::ScopedAnimationDurationScaleMode::is_zero())
return;
}

// Scroll contents has a layer, so animate that.
views::View* scroll_contents = scroll_view_->contents();
Expand Down Expand Up @@ -296,8 +287,7 @@ void AppListBubbleAppsPage::AnimateShowPage() {

void AppListBubbleAppsPage::AnimateHidePage() {
// If skipping animations, just update visibility.
if (!features::IsProductivityLauncherAnimationEnabled() ||
ui::ScopedAnimationDurationScaleMode::is_zero()) {
if (ui::ScopedAnimationDurationScaleMode::is_zero()) {
SetVisible(false);
return;
}
Expand Down
16 changes: 0 additions & 16 deletions ash/app_list/views/app_list_bubble_apps_page_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -117,22 +117,6 @@ TEST_F(AppListBubbleAppsPageTest, AnimateShowPage) {
"Apps.ClamshellLauncher.AnimationSmoothness.ShowAppsPage", 1);
}

TEST_F(AppListBubbleAppsPageTest, GradientMaskCreatedWhenAnimationsDisabled) {
// Force disable animation.
base::test::ScopedFeatureList feature;
feature.InitAndDisableFeature(features::kProductivityLauncherAnimation);

// Show an app list with enough apps to fill the page and trigger a gradient
// at the bottom.
auto* helper = GetAppListTestHelper();
helper->AddAppItems(50);
helper->ShowAppList();

// Scroll view gradient mask layer is created.
auto* apps_page = helper->GetBubbleAppsPage();
EXPECT_TRUE(apps_page->scroll_view()->layer()->layer_mask_layer());
}

TEST_F(AppListBubbleAppsPageTest, ScrollPositionResetOnShow) {
// Show an app list with enough apps to allow scrolling.
auto* helper = GetAppListTestHelper();
Expand Down
14 changes: 0 additions & 14 deletions ash/app_list/views/app_list_bubble_view_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -241,8 +241,6 @@ TEST_F(AppListBubbleViewTest,

TEST_F(AppListBubbleViewTest, OpeningBubbleTriggersAnimations) {
// Enable animations.
base::test::ScopedFeatureList feature(
features::kProductivityLauncherAnimation);
ui::ScopedAnimationDurationScaleMode duration(
ui::ScopedAnimationDurationScaleMode::NON_ZERO_DURATION);

Expand Down Expand Up @@ -280,8 +278,6 @@ TEST_F(AppListBubbleViewTest, OpeningBubbleTriggersAnimations) {

TEST_F(AppListBubbleViewTest, ShowAnimationCreatesAndDestroysLayers) {
// Enable animations.
base::test::ScopedFeatureList feature(
features::kProductivityLauncherAnimation);
ui::ScopedAnimationDurationScaleMode duration(
ui::ScopedAnimationDurationScaleMode::NON_ZERO_DURATION);

Expand Down Expand Up @@ -316,8 +312,6 @@ TEST_F(AppListBubbleViewTest, ShowAnimationCreatesAndDestroysLayers) {

TEST_F(AppListBubbleViewTest, ShowAnimationDestroysAndRestoresGradientMask) {
// Enable animations.
base::test::ScopedFeatureList feature(
features::kProductivityLauncherAnimation);
ui::ScopedAnimationDurationScaleMode duration(
ui::ScopedAnimationDurationScaleMode::NON_ZERO_DURATION);

Expand All @@ -340,8 +334,6 @@ TEST_F(AppListBubbleViewTest, ShowAnimationDestroysAndRestoresGradientMask) {

TEST_F(AppListBubbleViewTest, ShowAnimationDestroysAndRestoresShadow) {
// Enable animations.
base::test::ScopedFeatureList feature(
features::kProductivityLauncherAnimation);
ui::ScopedAnimationDurationScaleMode duration(
ui::ScopedAnimationDurationScaleMode::NON_ZERO_DURATION);

Expand All @@ -364,8 +356,6 @@ TEST_F(AppListBubbleViewTest, ShowAnimationRecordsSmoothnessHistogram) {
base::HistogramTester histograms;

// Enable animations.
base::test::ScopedFeatureList feature(
features::kProductivityLauncherAnimation);
ui::ScopedAnimationDurationScaleMode duration(
ui::ScopedAnimationDurationScaleMode::NON_ZERO_DURATION);

Expand All @@ -389,8 +379,6 @@ TEST_F(AppListBubbleViewTest, HideAnimationsRecordsSmoothnessHistogram) {
ShowAppList();

// Enable animations.
base::test::ScopedFeatureList feature(
features::kProductivityLauncherAnimation);
ui::ScopedAnimationDurationScaleMode duration(
ui::ScopedAnimationDurationScaleMode::NON_ZERO_DURATION);

Expand Down Expand Up @@ -418,8 +406,6 @@ TEST_F(AppListBubbleViewTest, ShutdownDuringHideAnimationDoesNotCrash) {
base::HistogramTester histograms;

// Enable animations.
base::test::ScopedFeatureList feature(
features::kProductivityLauncherAnimation);
ui::ScopedAnimationDurationScaleMode duration(
ui::ScopedAnimationDurationScaleMode::NON_ZERO_DURATION);

Expand Down
3 changes: 1 addition & 2 deletions ash/app_list/views/app_list_view.cc
Original file line number Diff line number Diff line change
Expand Up @@ -659,8 +659,7 @@ void AppListView::InitContents() {
SearchBoxViewBase::InitParams params;
params.show_close_button_when_active = true;
params.create_background = true;
params.animate_changing_search_icon =
features::IsProductivityLauncherAnimationEnabled();
params.animate_changing_search_icon = true;
search_box_view_->Init(params);

// Assign |app_list_main_view_| here since it is accessed during Init().
Expand Down
22 changes: 10 additions & 12 deletions ash/app_list/views/productivity_launcher_search_view.cc
Original file line number Diff line number Diff line change
Expand Up @@ -150,18 +150,16 @@ void ProductivityLauncherSearchView::OnSearchResultContainerResultsChanged() {
result_count += view->num_results();
}

if (features::IsProductivityLauncherAnimationEnabled()) {
using AnimationInfo = SearchResultContainerView::ResultsAnimationInfo;
AnimationInfo aggregate_animation_info;
for (SearchResultContainerView* view : result_container_views_) {
absl::optional<AnimationInfo> container_animation_info =
view->ScheduleResultAnimations(aggregate_animation_info);
DCHECK(container_animation_info);
aggregate_animation_info.total_views +=
container_animation_info->total_views;
aggregate_animation_info.animating_views +=
container_animation_info->animating_views;
}
using AnimationInfo = SearchResultContainerView::ResultsAnimationInfo;
AnimationInfo aggregate_animation_info;
for (SearchResultContainerView* view : result_container_views_) {
absl::optional<AnimationInfo> container_animation_info =
view->ScheduleResultAnimations(aggregate_animation_info);
DCHECK(container_animation_info);
aggregate_animation_info.total_views +=
container_animation_info->total_views;
aggregate_animation_info.animating_views +=
container_animation_info->animating_views;
}

last_search_result_count_ = result_count;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -172,8 +172,6 @@ INSTANTIATE_TEST_SUITE_P(Tablet,

TEST_P(ProductivityLauncherSearchViewTest, AnimateSearchResultView) {
// Enable animations.
base::test::ScopedFeatureList feature(
features::kProductivityLauncherAnimation);
ui::ScopedAnimationDurationScaleMode duration(
ui::ScopedAnimationDurationScaleMode::NON_ZERO_DURATION);

Expand Down
4 changes: 1 addition & 3 deletions ash/app_list/views/search_box_view_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1289,9 +1289,7 @@ TEST_F(SearchBoxViewAppListBubbleTest, HasAccessibilityHintWhenActive) {
class SearchBoxViewAnimationTest : public AshTestBase {
public:
SearchBoxViewAnimationTest() {
scoped_features_.InitWithFeatures({features::kProductivityLauncherAnimation,
features::kProductivityLauncher},
{});
scoped_features_.InitAndEnableFeature(features::kProductivityLauncher);
}
~SearchBoxViewAnimationTest() override = default;

Expand Down
1 change: 0 additions & 1 deletion ash/app_list/views/search_result_list_view.cc
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,6 @@ SearchResultListView::GetAllListTypesForCategoricalSearch() {
absl::optional<SearchResultContainerView::ResultsAnimationInfo>
SearchResultListView::ScheduleResultAnimations(
const ResultsAnimationInfo& aggregate_animation_info) {
DCHECK(features::IsProductivityLauncherAnimationEnabled());
DCHECK(animates_result_updates_);

// Collect current container animation info.
Expand Down
9 changes: 0 additions & 9 deletions ash/constants/ash_features.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1038,10 +1038,6 @@ const base::Feature kPreferConstantFrameRate{"PreferConstantFrameRate",
const base::Feature kProductivityLauncher{"ProductivityLauncher",
base::FEATURE_DISABLED_BY_DEFAULT};

// Enables animation in the productivity launcher.
const base::Feature kProductivityLauncherAnimation{
"ProductivityLauncherAnimation", base::FEATURE_ENABLED_BY_DEFAULT};

// Controls whether to enable Projector.
const base::Feature kProjector{"Projector", base::FEATURE_DISABLED_BY_DEFAULT};

Expand Down Expand Up @@ -1795,11 +1791,6 @@ bool IsProductivityLauncherEnabled() {
return base::FeatureList::IsEnabled(kProductivityLauncher);
}

bool IsProductivityLauncherAnimationEnabled() {
return IsProductivityLauncherEnabled() &&
base::FeatureList::IsEnabled(kProductivityLauncherAnimation);
}

bool IsProjectorEnabled() {
return IsProjectorAllUserEnabled() || IsProjectorManagedUserEnabled();
}
Expand Down
3 changes: 0 additions & 3 deletions ash/constants/ash_features.h
Original file line number Diff line number Diff line change
Expand Up @@ -394,8 +394,6 @@ COMPONENT_EXPORT(ASH_CONSTANTS)
extern const base::Feature kPreferConstantFrameRate;
COMPONENT_EXPORT(ASH_CONSTANTS)
extern const base::Feature kProductivityLauncher;
COMPONENT_EXPORT(ASH_CONSTANTS)
extern const base::Feature kProductivityLauncherAnimation;
COMPONENT_EXPORT(ASH_CONSTANTS) extern const base::Feature kProjector;
COMPONENT_EXPORT(ASH_CONSTANTS)
extern const base::Feature kProjectorManagedUser;
Expand Down Expand Up @@ -628,7 +626,6 @@ COMPONENT_EXPORT(ASH_CONSTANTS) bool IsPinAutosubmitFeatureEnabled();
COMPONENT_EXPORT(ASH_CONSTANTS) bool IsPinSetupForManagedUsersEnabled();
COMPONENT_EXPORT(ASH_CONSTANTS) bool IsPipRoundedCornersEnabled();
COMPONENT_EXPORT(ASH_CONSTANTS) bool IsProductivityLauncherEnabled();
COMPONENT_EXPORT(ASH_CONSTANTS) bool IsProductivityLauncherAnimationEnabled();
COMPONENT_EXPORT(ASH_CONSTANTS) bool IsProjectorEnabled();
COMPONENT_EXPORT(ASH_CONSTANTS) bool IsProjectorAllUserEnabled();
COMPONENT_EXPORT(ASH_CONSTANTS) bool IsProjectorManagedUserEnabled();
Expand Down
21 changes: 8 additions & 13 deletions ash/search_box/search_box_view_base.cc
Original file line number Diff line number Diff line change
Expand Up @@ -563,21 +563,16 @@ void SearchBoxViewBase::UpdateButtonsVisibility() {
const bool should_show_assistant_button =
show_assistant_button_ && !should_show_close_button;

if (!features::IsProductivityLauncherAnimationEnabled()) {
close_button_->SetVisible(should_show_close_button);
assistant_button_->SetVisible(should_show_assistant_button);
if (should_show_close_button) {
MaybeFadeButtonIn(close_button_);
} else {
if (should_show_close_button) {
MaybeFadeButtonIn(close_button_);
} else {
MaybeFadeButtonOut(close_button_);
}
MaybeFadeButtonOut(close_button_);
}

if (should_show_assistant_button) {
MaybeFadeButtonIn(assistant_button_);
} else {
MaybeFadeButtonOut(assistant_button_);
}
if (should_show_assistant_button) {
MaybeFadeButtonIn(assistant_button_);
} else {
MaybeFadeButtonOut(assistant_button_);
}
}

Expand Down
4 changes: 0 additions & 4 deletions chrome/browser/about_flags.cc
Original file line number Diff line number Diff line change
Expand Up @@ -6499,10 +6499,6 @@ const FeatureEntry kFeatureEntries[] = {
{"productivity-launcher", flag_descriptions::kProductivityLauncherName,
flag_descriptions::kProductivityLauncherDescription, kOsCrOS,
FEATURE_VALUE_TYPE(ash::features::kProductivityLauncher)},
{"productivity-launcher-animation",
flag_descriptions::kProductivityLauncherAnimationName,
flag_descriptions::kProductivityLauncherAnimationDescription, kOsCrOS,
FEATURE_VALUE_TYPE(ash::features::kProductivityLauncherAnimation)},
{"shelf-drag-to-pin", flag_descriptions::kShelfDragToPinName,
flag_descriptions::kShelfDragToPinDescription, kOsCrOS,
FEATURE_VALUE_TYPE(ash::features::kDragUnpinnedAppToPin)},
Expand Down
4 changes: 0 additions & 4 deletions chrome/browser/flag_descriptions.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4971,10 +4971,6 @@ const char kProductivityLauncherDescription[] =
"To evaluate an enhanced Launcher experience that aims to improve app "
"workflows by optimizing access to apps, app content, and app actions.";

const char kProductivityLauncherAnimationName[] = "App Launcher: Animation";
const char kProductivityLauncherAnimationDescription[] =
"Enables new animation in the enhanced app launcher.";

const char kForceShowContinueSectionName[] =
"App Launcher: Force Continue Section Suggestions";
const char kForceShowContinueSectionDescription[] =
Expand Down
3 changes: 0 additions & 3 deletions chrome/browser/flag_descriptions.h
Original file line number Diff line number Diff line change
Expand Up @@ -2855,9 +2855,6 @@ extern const char kPhoneHubRecentAppsDescription[];
extern const char kProductivityLauncherName[];
extern const char kProductivityLauncherDescription[];

extern const char kProductivityLauncherAnimationName[];
extern const char kProductivityLauncherAnimationDescription[];

extern const char kForceShowContinueSectionName[];
extern const char kForceShowContinueSectionDescription[];

Expand Down

0 comments on commit bc9f273

Please sign in to comment.