From a61444ac0577e22bd3b20bcee833b2d199f4f512 Mon Sep 17 00:00:00 2001 From: calamity Date: Thu, 19 Mar 2015 21:13:13 -0700 Subject: [PATCH] Change ContentsView::SetActivePage to use States rather than indexes. This CL makes the ContentsView's API state-keyed rather than page index keyed. This removes cruft from callers of SetActivePage and will support further refactors. BUG=455059 TBR=jamescook@chromium.org, benwells@chromium.org Review URL: https://codereview.chromium.org/947903002 Cr-Commit-Position: refs/heads/master@{#321513} --- .../apps/custom_launcher_page_browsertest_views.cc | 27 +++++---- .../browser/ui/app_list/app_list_service_views.cc | 4 +- .../ui/ash/app_list/app_list_service_ash.cc | 3 +- ui/app_list/views/all_apps_tile_item_view.cc | 3 +- ui/app_list/views/app_list_main_view.cc | 10 ++-- ui/app_list/views/app_list_main_view_unittest.cc | 9 +-- ui/app_list/views/app_list_view_unittest.cc | 67 +++++++++++----------- ui/app_list/views/contents_view.cc | 27 ++++----- ui/app_list/views/contents_view.h | 13 ++--- ui/app_list/views/start_page_view.cc | 4 +- 10 files changed, 76 insertions(+), 91 deletions(-) diff --git a/chrome/browser/apps/custom_launcher_page_browsertest_views.cc b/chrome/browser/apps/custom_launcher_page_browsertest_views.cc index 485fe6dc4763..b7c677a4fdb9 100644 --- a/chrome/browser/apps/custom_launcher_page_browsertest_views.cc +++ b/chrome/browser/apps/custom_launcher_page_browsertest_views.cc @@ -88,10 +88,10 @@ class CustomLauncherPageBrowserTest // Set the active page on the app list, according to |state|. Does not wait // for any animation or custom page to complete. - void SetActivePageAndVerify(app_list::AppListModel::State state) { + void SetActiveStateAndVerify(app_list::AppListModel::State state) { app_list::ContentsView* contents_view = GetAppListView()->app_list_main_view()->contents_view(); - contents_view->SetActivePage(contents_view->GetPageIndexForState(state)); + contents_view->SetActiveState(state); EXPECT_TRUE(contents_view->IsStateActive(state)); } @@ -133,15 +133,14 @@ IN_PROC_BROWSER_TEST_F(CustomLauncherPageBrowserTest, { ExtensionTestMessageListener listener("onPageProgressAt1", false); - contents_view->SetActivePage(contents_view->GetPageIndexForState( - app_list::AppListModel::STATE_CUSTOM_LAUNCHER_PAGE)); + contents_view->SetActiveState( + app_list::AppListModel::STATE_CUSTOM_LAUNCHER_PAGE); listener.WaitUntilSatisfied(); } { ExtensionTestMessageListener listener("onPageProgressAt0", false); - contents_view->SetActivePage(contents_view->GetPageIndexForState( - app_list::AppListModel::STATE_START)); + contents_view->SetActiveState(app_list::AppListModel::STATE_START); listener.WaitUntilSatisfied(); } @@ -193,7 +192,7 @@ IN_PROC_BROWSER_TEST_F(CustomLauncherPageBrowserTest, app_list::AppListModel::STATE_CUSTOM_LAUNCHER_PAGE)); // Back to the start page. And send a mouse wheel event. - SetActivePageAndVerify(app_list::AppListModel::STATE_START); + SetActiveStateAndVerify(app_list::AppListModel::STATE_START); // Generate wheel events above the clickzone. event_generator.MoveMouseRelativeTo(window, point_above_clickzone); // Scrolling left, right or up should do nothing. @@ -224,7 +223,7 @@ IN_PROC_BROWSER_TEST_F(CustomLauncherPageBrowserTest, #endif // Back to the start page. And send a scroll gesture. - SetActivePageAndVerify(app_list::AppListModel::STATE_START); + SetActiveStateAndVerify(app_list::AppListModel::STATE_START); // Going down should do nothing. event_generator.GestureScrollSequence( point_above_clickzone, point_in_clickzone, step_delay, num_steps); @@ -237,7 +236,7 @@ IN_PROC_BROWSER_TEST_F(CustomLauncherPageBrowserTest, app_list::AppListModel::STATE_CUSTOM_LAUNCHER_PAGE)); // Back to the start page. And send a trackpad scroll event. - SetActivePageAndVerify(app_list::AppListModel::STATE_START); + SetActiveStateAndVerify(app_list::AppListModel::STATE_START); // Going down left, right or up should do nothing. event_generator.ScrollSequence(point_in_clickzone, step_delay, -5, 0, num_steps, num_fingers); @@ -254,7 +253,7 @@ IN_PROC_BROWSER_TEST_F(CustomLauncherPageBrowserTest, app_list::AppListModel::STATE_CUSTOM_LAUNCHER_PAGE)); // Back to the start page. And send a tap gesture. - SetActivePageAndVerify(app_list::AppListModel::STATE_START); + SetActiveStateAndVerify(app_list::AppListModel::STATE_START); // Tapping outside the clickzone should do nothing. event_generator.GestureTapAt(point_above_clickzone); EXPECT_TRUE( @@ -278,8 +277,8 @@ IN_PROC_BROWSER_TEST_F(CustomLauncherPageBrowserTest, LauncherPageSubpages) { { ExtensionTestMessageListener listener("onPageProgressAt1", false); - contents_view->SetActivePage(contents_view->GetPageIndexForState( - app_list::AppListModel::STATE_CUSTOM_LAUNCHER_PAGE)); + contents_view->SetActiveState( + app_list::AppListModel::STATE_CUSTOM_LAUNCHER_PAGE); listener.WaitUntilSatisfied(); EXPECT_TRUE(contents_view->IsStateActive( app_list::AppListModel::STATE_CUSTOM_LAUNCHER_PAGE)); @@ -405,8 +404,8 @@ IN_PROC_BROWSER_TEST_F(CustomLauncherPageBrowserTest, { ExtensionTestMessageListener listener("onPageProgressAt1", false); - contents_view->SetActivePage(contents_view->GetPageIndexForState( - app_list::AppListModel::STATE_CUSTOM_LAUNCHER_PAGE)); + contents_view->SetActiveState( + app_list::AppListModel::STATE_CUSTOM_LAUNCHER_PAGE); listener.WaitUntilSatisfied(); EXPECT_TRUE(contents_view->IsStateActive( app_list::AppListModel::STATE_CUSTOM_LAUNCHER_PAGE)); diff --git a/chrome/browser/ui/app_list/app_list_service_views.cc b/chrome/browser/ui/app_list/app_list_service_views.cc index 13770b2b3d03..7f1cd4aca03e 100644 --- a/chrome/browser/ui/app_list/app_list_service_views.cc +++ b/chrome/browser/ui/app_list/app_list_service_views.cc @@ -105,8 +105,8 @@ void AppListServiceViews::ShowForProfileInternal( if (state != app_list::AppListModel::INVALID_STATE) { app_list::ContentsView* contents_view = shower_.app_list()->app_list_main_view()->contents_view(); - contents_view->SetActivePage(contents_view->GetPageIndexForState(state), - shower_.IsAppListVisible() /* animate */); + contents_view->SetActiveState(state, + shower_.IsAppListVisible() /* animate */); } shower_.ShowForCurrentProfile(); diff --git a/chrome/browser/ui/ash/app_list/app_list_service_ash.cc b/chrome/browser/ui/ash/app_list/app_list_service_ash.cc index 6d0fd0a1a4ce..2b325d40c209 100644 --- a/chrome/browser/ui/ash/app_list/app_list_service_ash.cc +++ b/chrome/browser/ui/ash/app_list/app_list_service_ash.cc @@ -49,8 +49,7 @@ void AppListServiceAsh::ShowAndSwitchToState( app_list::ContentsView* contents_view = app_list_view->app_list_main_view()->contents_view(); - contents_view->SetActivePage(contents_view->GetPageIndexForState(state), - app_list_was_open /* animate */); + contents_view->SetActiveState(state, app_list_was_open /* animate */); } void AppListServiceAsh::Init(Profile* initial_profile) { diff --git a/ui/app_list/views/all_apps_tile_item_view.cc b/ui/app_list/views/all_apps_tile_item_view.cc index c7e10f7af911..5e12ad7553eb 100644 --- a/ui/app_list/views/all_apps_tile_item_view.cc +++ b/ui/app_list/views/all_apps_tile_item_view.cc @@ -32,8 +32,7 @@ void AllAppsTileItemView::ButtonPressed(views::Button* sender, UMA_HISTOGRAM_ENUMERATION(kPageOpenedHistogram, AppListModel::STATE_APPS, AppListModel::STATE_LAST); - contents_view_->SetActivePage( - contents_view_->GetPageIndexForState(AppListModel::STATE_APPS)); + contents_view_->SetActiveState(AppListModel::STATE_APPS); } void AllAppsTileItemView::OnFolderImageUpdated() { diff --git a/ui/app_list/views/app_list_main_view.cc b/ui/app_list/views/app_list_main_view.cc index 02690dcd951a..aa45d63b5df6 100644 --- a/ui/app_list/views/app_list_main_view.cc +++ b/ui/app_list/views/app_list_main_view.cc @@ -144,10 +144,9 @@ void AppListMainView::ShowAppListWhenReady() { } void AppListMainView::ResetForShow() { - if (switches::IsExperimentalAppListEnabled()) { - contents_view_->SetActivePage( - contents_view_->GetPageIndexForState(AppListModel::STATE_START)); - } + if (switches::IsExperimentalAppListEnabled()) + contents_view_->SetActiveState(AppListModel::STATE_START); + contents_view_->apps_container_view()->ResetForShowApps(); // We clear the search when hiding so when app list appears it is not showing // search results. @@ -262,8 +261,7 @@ void AppListMainView::UpdateCustomLauncherPageVisibility() { AppListModel::STATE_CUSTOM_LAUNCHER_PAGE)) { // Animate to the start page if currently on the custom page view. The view // will hide on animation completion. - contents_view_->SetActivePage( - contents_view_->GetPageIndexForState(AppListModel::STATE_START)); + contents_view_->SetActiveState(AppListModel::STATE_START); } else { // Hide the view immediately otherwise. custom_page->SetVisible(false); diff --git a/ui/app_list/views/app_list_main_view_unittest.cc b/ui/app_list/views/app_list_main_view_unittest.cc index accc731b76c4..547b9b395a2d 100644 --- a/ui/app_list/views/app_list_main_view_unittest.cc +++ b/ui/app_list/views/app_list_main_view_unittest.cc @@ -278,8 +278,7 @@ TEST_F(AppListMainViewTest, MouseHoverToHighlight) { // If experimental launcher, switch to All Apps page if (app_list::switches::IsExperimentalAppListEnabled()) { - GetContentsView()->SetActivePage( - GetContentsView()->GetPageIndexForState(AppListModel::STATE_APPS)); + GetContentsView()->SetActiveState(AppListModel::STATE_APPS); GetContentsView()->Layout(); } @@ -314,8 +313,7 @@ TEST_F(AppListMainViewTest, MAYBE_TapGestureToHighlight) { // If experimental launcher, switch to All Apps page if (app_list::switches::IsExperimentalAppListEnabled()) { - GetContentsView()->SetActivePage( - GetContentsView()->GetPageIndexForState(AppListModel::STATE_APPS)); + GetContentsView()->SetActiveState(AppListModel::STATE_APPS); GetContentsView()->Layout(); } @@ -371,8 +369,7 @@ TEST_F(AppListMainViewTest, DragReparentItemOntoPageSwitcher) { // Ensure we are on the apps grid view page. app_list::ContentsView* contents_view = GetContentsView(); - contents_view->SetActivePage( - contents_view->GetPageIndexForState(AppListModel::STATE_APPS)); + contents_view->SetActiveState(AppListModel::STATE_APPS); contents_view->Layout(); AppListItemView* folder_item_view = CreateAndOpenSingleItemFolder(); diff --git a/ui/app_list/views/app_list_view_unittest.cc b/ui/app_list/views/app_list_view_unittest.cc index ded35430852b..dde70081faaf 100644 --- a/ui/app_list/views/app_list_view_unittest.cc +++ b/ui/app_list/views/app_list_view_unittest.cc @@ -234,8 +234,7 @@ void AppListViewTestContext::CheckView(views::View* subview) { bool AppListViewTestContext::SetAppListState(AppListModel::State state) { ContentsView* contents_view = view_->app_list_main_view()->contents_view(); - int index = contents_view->GetPageIndexForState(state); - contents_view->SetActivePage(index); + contents_view->SetActiveState(state); contents_view->Layout(); return IsStateShown(state); } @@ -509,47 +508,51 @@ void AppListViewTestContext::RunPageSwitchingAnimationTest() { EXPECT_NO_FATAL_FAILURE(CheckView(main_view->contents_view())); ContentsView* contents_view = main_view->contents_view(); - // Pad the ContentsView with blank pages so we have at least 3 views. - while (contents_view->NumLauncherPages() < 3) - contents_view->AddBlankPageForTesting(); - contents_view->SetActivePage(0); + int start_page_index = + contents_view->GetPageIndexForState(AppListModel::STATE_START); + int search_results_page_index = + contents_view->GetPageIndexForState(AppListModel::STATE_SEARCH_RESULTS); + int apps_page_index = + contents_view->GetPageIndexForState(AppListModel::STATE_APPS); + + contents_view->SetActiveState(AppListModel::STATE_START); contents_view->Layout(); - EXPECT_EQ(contents_view->GetOnscreenPageBounds(0), - contents_view->GetPageView(0)->bounds()); - EXPECT_NE(contents_view->GetOnscreenPageBounds(1), - contents_view->GetPageView(1)->bounds()); - EXPECT_NE(contents_view->GetOnscreenPageBounds(2), - contents_view->GetPageView(2)->bounds()); + EXPECT_EQ(contents_view->GetOnscreenPageBounds(start_page_index), + contents_view->GetPageView(start_page_index)->bounds()); + EXPECT_NE(contents_view->GetOnscreenPageBounds(search_results_page_index), + contents_view->GetPageView(search_results_page_index)->bounds()); + EXPECT_NE(contents_view->GetOnscreenPageBounds(apps_page_index), + contents_view->GetPageView(apps_page_index)->bounds()); // Change pages. View should not have moved without Layout(). - contents_view->SetActivePage(1); - EXPECT_EQ(contents_view->GetOnscreenPageBounds(0), - contents_view->GetPageView(0)->bounds()); - EXPECT_NE(contents_view->GetOnscreenPageBounds(1), - contents_view->GetPageView(1)->bounds()); - EXPECT_NE(contents_view->GetOnscreenPageBounds(2), - contents_view->GetPageView(2)->bounds()); + contents_view->SetActiveState(AppListModel::STATE_SEARCH_RESULTS); + EXPECT_EQ(contents_view->GetOnscreenPageBounds(start_page_index), + contents_view->GetPageView(start_page_index)->bounds()); + EXPECT_NE(contents_view->GetOnscreenPageBounds(search_results_page_index), + contents_view->GetPageView(search_results_page_index)->bounds()); + EXPECT_NE(contents_view->GetOnscreenPageBounds(apps_page_index), + contents_view->GetPageView(apps_page_index)->bounds()); // Change to a third page. This queues up the second animation behind the // first. - contents_view->SetActivePage(2); - EXPECT_EQ(contents_view->GetOnscreenPageBounds(0), - contents_view->GetPageView(0)->bounds()); - EXPECT_NE(contents_view->GetOnscreenPageBounds(1), - contents_view->GetPageView(1)->bounds()); - EXPECT_NE(contents_view->GetOnscreenPageBounds(2), - contents_view->GetPageView(2)->bounds()); + contents_view->SetActiveState(AppListModel::STATE_APPS); + EXPECT_EQ(contents_view->GetOnscreenPageBounds(start_page_index), + contents_view->GetPageView(start_page_index)->bounds()); + EXPECT_NE(contents_view->GetOnscreenPageBounds(search_results_page_index), + contents_view->GetPageView(search_results_page_index)->bounds()); + EXPECT_NE(contents_view->GetOnscreenPageBounds(apps_page_index), + contents_view->GetPageView(apps_page_index)->bounds()); // Call Layout(). Should jump to the third page. contents_view->Layout(); - EXPECT_NE(contents_view->GetOnscreenPageBounds(0), - contents_view->GetPageView(0)->bounds()); - EXPECT_NE(contents_view->GetOnscreenPageBounds(1), - contents_view->GetPageView(1)->bounds()); - EXPECT_EQ(contents_view->GetOnscreenPageBounds(2), - contents_view->GetPageView(2)->bounds()); + EXPECT_NE(contents_view->GetOnscreenPageBounds(start_page_index), + contents_view->GetPageView(start_page_index)->bounds()); + EXPECT_NE(contents_view->GetOnscreenPageBounds(search_results_page_index), + contents_view->GetPageView(search_results_page_index)->bounds()); + EXPECT_EQ(contents_view->GetOnscreenPageBounds(apps_page_index), + contents_view->GetPageView(apps_page_index)->bounds()); } Close(); diff --git a/ui/app_list/views/contents_view.cc b/ui/app_list/views/contents_view.cc index 3c27e17261c5..623b391ce942 100644 --- a/ui/app_list/views/contents_view.cc +++ b/ui/app_list/views/contents_view.cc @@ -131,15 +131,15 @@ void ContentsView::SetDragAndDropHostOfCurrentAppList( apps_container_view_->SetDragAndDropHostOfCurrentAppList(drag_and_drop_host); } -void ContentsView::SetActivePage(int page_index) { - SetActivePage(page_index, true); +void ContentsView::SetActiveState(AppListModel::State state) { + SetActiveState(state, true); } -void ContentsView::SetActivePage(int page_index, bool animate) { - if (GetActivePageIndex() == page_index) +void ContentsView::SetActiveState(AppListModel::State state, bool animate) { + if (IsStateActive(state)) return; - SetActivePageInternal(page_index, false, animate); + SetActiveStateInternal(GetPageIndexForState(state), false, animate); } int ContentsView::GetActivePageIndex() const { @@ -180,9 +180,9 @@ int ContentsView::NumLauncherPages() const { return pagination_model_.total_pages(); } -void ContentsView::SetActivePageInternal(int page_index, - bool show_search_results, - bool animate) { +void ContentsView::SetActiveStateInternal(int page_index, + bool show_search_results, + bool animate) { if (!GetPageView(page_index)->visible()) return; @@ -237,7 +237,7 @@ void ContentsView::ShowSearchResults(bool show) { int search_page = GetPageIndexForState(AppListModel::STATE_SEARCH_RESULTS); DCHECK_GE(search_page, 0); - SetActivePageInternal(show ? search_page : page_before_search_, show, true); + SetActiveStateInternal(show ? search_page : page_before_search_, show, true); } bool ContentsView::IsShowingSearchResults() const { @@ -335,11 +335,6 @@ SearchBoxView* ContentsView::GetSearchBoxView() const { return app_list_main_view_->search_box_view(); } -void ContentsView::AddBlankPageForTesting() { - AddLauncherPage(new views::View); - pagination_model_.SetTotalPages(view_model_->view_size()); -} - int ContentsView::AddLauncherPage(views::View* view) { int page_index = view_model_->view_size(); AddChildView(view); @@ -432,13 +427,13 @@ bool ContentsView::Back() { if (app_list_main_view_->model()->PopCustomLauncherPageSubpage()) app_list_main_view_->view_delegate()->CustomLauncherPagePopSubpage(); else - SetActivePage(GetPageIndexForState(AppListModel::STATE_START)); + SetActiveState(AppListModel::STATE_START); break; case AppListModel::STATE_APPS: if (apps_container_view_->IsInFolderView()) apps_container_view_->app_list_folder_view()->CloseFolderPage(); else - SetActivePage(GetPageIndexForState(AppListModel::STATE_START)); + SetActiveState(AppListModel::STATE_START); break; case AppListModel::STATE_SEARCH_RESULTS: GetSearchBoxView()->ClearSearch(); diff --git a/ui/app_list/views/contents_view.h b/ui/app_list/views/contents_view.h index 89d8d6973f04..e568b351c59b 100644 --- a/ui/app_list/views/contents_view.h +++ b/ui/app_list/views/contents_view.h @@ -70,8 +70,8 @@ class APP_LIST_EXPORT ContentsView : public views::View, void ShowFolderContent(AppListFolderItem* folder); // Sets the active launcher page and animates the pages into place. - void SetActivePage(int page_index); - void SetActivePage(int page_index, bool animate); + void SetActiveState(AppListModel::State state); + void SetActiveState(AppListModel::State state, bool animate); // The index of the currently active launcher page. int GetActivePageIndex() const; @@ -108,9 +108,6 @@ class APP_LIST_EXPORT ContentsView : public views::View, AppListMainView* app_list_main_view() const { return app_list_main_view_; } - // Adds a blank launcher page. For use in tests only. - void AddBlankPageForTesting(); - // Returns the pagination model for the ContentsView. const PaginationModel& pagination_model() { return pagination_model_; } @@ -166,9 +163,9 @@ class APP_LIST_EXPORT ContentsView : public views::View, private: // Sets the active launcher page, accounting for whether the change is for // search results. - void SetActivePageInternal(int page_index, - bool show_search_results, - bool animate); + void SetActiveStateInternal(int page_index, + bool show_search_results, + bool animate); // Invoked when active view is changed. void ActivePageChanged(); diff --git a/ui/app_list/views/start_page_view.cc b/ui/app_list/views/start_page_view.cc index 401aae19d2d4..1c62aca9be53 100644 --- a/ui/app_list/views/start_page_view.cc +++ b/ui/app_list/views/start_page_view.cc @@ -279,9 +279,7 @@ void StartPageView::MaybeOpenCustomLauncherPage() { AppListModel::STATE_CUSTOM_LAUNCHER_PAGE, AppListModel::STATE_LAST); - int custom_page_index = contents_view->GetPageIndexForState( - AppListModel::STATE_CUSTOM_LAUNCHER_PAGE); - contents_view->SetActivePage(custom_page_index); + contents_view->SetActiveState(AppListModel::STATE_CUSTOM_LAUNCHER_PAGE); } void StartPageView::Reset() { -- 2.11.4.GIT