From 86f30b9ad7616f56c52557b50c5057ed2d1f3273 Mon Sep 17 00:00:00 2001 From: "xiyuan@chromium.org" Date: Wed, 28 May 2014 05:27:09 +0000 Subject: [PATCH] app_list: Fix possible out of bounds index. - Fix a problem that could not drop at the last position when dragging an item out of a folder; - Fix a case view_model_.Move is called with an out of range target index when dragging the last item out of folder; - Add a test for dragging last item and drop it at last slot; - Speculative fix to protect using an index from data model to modify view_model_; - A few more DCHECKs to catch the out-of-bound case; BUG=370064 TEST=AppListMainViewTest.DragLastItemFromFolderAndDropAtLastSlot Review URL: https://codereview.chromium.org/298963004 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@273151 0039d316-1c4b-4281-b951-d872f2087c98 --- ui/app_list/cocoa/apps_grid_controller_unittest.mm | 1 + ui/app_list/test/app_list_test_model.cc | 23 +++ ui/app_list/test/app_list_test_model.h | 3 + ui/app_list/views/app_list_main_view_unittest.cc | 199 ++++++++++++++++++++- ui/app_list/views/apps_grid_view.cc | 47 +++-- ui/app_list/views/apps_grid_view.h | 9 +- ui/app_list/views/test/apps_grid_view_test_api.cc | 9 + ui/app_list/views/test/apps_grid_view_test_api.h | 2 + ui/views/view_model.cc | 5 + 9 files changed, 273 insertions(+), 25 deletions(-) diff --git a/ui/app_list/cocoa/apps_grid_controller_unittest.mm b/ui/app_list/cocoa/apps_grid_controller_unittest.mm index 13634d818b99..fdb5e60f6b93 100644 --- a/ui/app_list/cocoa/apps_grid_controller_unittest.mm +++ b/ui/app_list/cocoa/apps_grid_controller_unittest.mm @@ -454,6 +454,7 @@ TEST_F(AppsGridControllerTest, ModelUpdate) { EXPECT_NSEQ(@"UpdatedItem", [button title]); // Test icon updates through the model observer by ensuring the icon changes. + item_model->SetIcon(gfx::ImageSkia(), false); NSSize icon_size = [[button image] size]; EXPECT_EQ(0, icon_size.width); EXPECT_EQ(0, icon_size.height); diff --git a/ui/app_list/test/app_list_test_model.cc b/ui/app_list/test/app_list_test_model.cc index 70b60a882f93..9a81aab40014 100644 --- a/ui/app_list/test/app_list_test_model.cc +++ b/ui/app_list/test/app_list_test_model.cc @@ -7,11 +7,22 @@ #include "base/memory/scoped_ptr.h" #include "base/strings/stringprintf.h" #include "grit/ui_resources.h" +#include "third_party/skia/include/core/SkBitmap.h" +#include "ui/app_list/app_list_constants.h" #include "ui/base/resource/resource_bundle.h" +#include "ui/gfx/image/image_skia.h" namespace app_list { namespace test { +gfx::ImageSkia CreateImageSkia(int width, int height) { + SkBitmap bitmap; + bitmap.setConfig(SkBitmap::kARGB_8888_Config, width, height); + bitmap.allocPixels(); + bitmap.eraseARGB(255, 0, 255, 0); + return gfx::ImageSkia::CreateFrom1xBitmap(bitmap); +} + // static const char AppListTestModel::kItemType[] = "TestItem"; @@ -22,6 +33,8 @@ AppListTestModel::AppListTestItem::AppListTestItem( AppListTestModel* model) : AppListItem(id), model_(model) { + SetIcon(CreateImageSkia(kPreferredIconDimension, kPreferredIconDimension), + false /* has_shadow */); } AppListTestModel::AppListTestItem::~AppListTestItem() { @@ -93,6 +106,16 @@ AppListFolderItem* AppListTestModel::CreateAndAddOemFolder( return static_cast(AddItem(folder)); } +AppListFolderItem* AppListTestModel::CreateSingleItemFolder( + const std::string& folder_id, + const std::string& item_id) { + AppListTestItem* item = CreateItem(item_id); + AddItemToFolder(item, folder_id); + AppListItem* folder_item = FindItem(folder_id); + DCHECK(folder_item->GetItemType() == AppListFolderItem::kItemType); + return static_cast(folder_item); +} + void AppListTestModel::PopulateAppWithId(int id) { CreateAndAddItem(GetItemName(id)); } diff --git a/ui/app_list/test/app_list_test_model.h b/ui/app_list/test/app_list_test_model.h index 2d2fc9568876..89e407b45562 100644 --- a/ui/app_list/test/app_list_test_model.h +++ b/ui/app_list/test/app_list_test_model.h @@ -53,6 +53,9 @@ class AppListTestModel : public AppListModel { AppListFolderItem* CreateAndAddOemFolder(const std::string& id); + AppListFolderItem* CreateSingleItemFolder(const std::string& folder_id, + const std::string& item_id); + // Populate the model with an item titled "Item |id|". void PopulateAppWithId(int id); 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 6ca1820ef44d..4007e902d06b 100644 --- a/ui/app_list/views/app_list_main_view_unittest.cc +++ b/ui/app_list/views/app_list_main_view_unittest.cc @@ -4,13 +4,20 @@ #include "ui/app_list/views/app_list_main_view.h" +#include "base/memory/scoped_ptr.h" +#include "base/run_loop.h" +#include "base/time/time.h" +#include "base/timer/timer.h" #include "testing/gtest/include/gtest/gtest.h" #include "ui/app_list/pagination_model.h" #include "ui/app_list/test/app_list_test_model.h" #include "ui/app_list/test/app_list_test_view_delegate.h" +#include "ui/app_list/views/app_list_folder_view.h" +#include "ui/app_list/views/app_list_item_view.h" #include "ui/app_list/views/apps_container_view.h" #include "ui/app_list/views/apps_grid_view.h" #include "ui/app_list/views/contents_view.h" +#include "ui/app_list/views/test/apps_grid_view_test_api.h" #include "ui/views/test/views_test_base.h" #include "ui/views/view_model.h" #include "ui/views/widget/widget.h" @@ -22,6 +29,38 @@ namespace { const int kInitialItems = 2; +class GridViewVisibleWaiter { + public: + explicit GridViewVisibleWaiter(AppsGridView* grid_view) + : grid_view_(grid_view) {} + ~GridViewVisibleWaiter() {} + + void Wait() { + if (grid_view_->visible()) + return; + + check_timer_.Start(FROM_HERE, + base::TimeDelta::FromMilliseconds(50), + base::Bind(&GridViewVisibleWaiter::OnTimerCheck, + base::Unretained(this))); + run_loop_.reset(new base::RunLoop); + run_loop_->Run(); + check_timer_.Stop(); + } + + private: + void OnTimerCheck() { + if (grid_view_->visible()) + run_loop_->Quit(); + } + + AppsGridView* grid_view_; + scoped_ptr run_loop_; + base::RepeatingTimer check_timer_; + + DISALLOW_COPY_AND_ASSIGN(GridViewVisibleWaiter); +}; + class AppListMainViewTest : public views::ViewsTestBase { public: AppListMainViewTest() @@ -34,7 +73,6 @@ class AppListMainViewTest : public views::ViewsTestBase { virtual void SetUp() OVERRIDE { views::ViewsTestBase::SetUp(); delegate_.reset(new AppListTestViewDelegate); - delegate_->GetTestModel()->PopulateApps(kInitialItems); main_view_ = new AppListMainView(delegate_.get(), &pagination_model_, GetContext()); @@ -55,9 +93,78 @@ class AppListMainViewTest : public views::ViewsTestBase { delegate_.reset(); } - const views::ViewModel* ViewModel() { - return main_view_->contents_view()->apps_container_view()->apps_grid_view() - ->view_model_for_test(); + // |point| is in |grid_view|'s coordinates. + AppListItemView* GetItemViewAtPointInGrid(AppsGridView* grid_view, + const gfx::Point& point) { + const views::ViewModel* view_model = grid_view->view_model_for_test(); + for (int i = 0; i < view_model->view_size(); ++i) { + views::View* view = view_model->view_at(i); + if (view->bounds().Contains(point)) { + return static_cast(view); + } + } + + return NULL; + } + + void SimulateClick(views::View* view) { + gfx::Point center = view->GetLocalBounds().CenterPoint(); + view->OnMousePressed(ui::MouseEvent(ui::ET_MOUSE_PRESSED, + center, + center, + ui::EF_LEFT_MOUSE_BUTTON, + ui::EF_LEFT_MOUSE_BUTTON)); + view->OnMouseReleased(ui::MouseEvent(ui::ET_MOUSE_RELEASED, + center, + center, + ui::EF_LEFT_MOUSE_BUTTON, + ui::EF_LEFT_MOUSE_BUTTON)); + } + + // |point| is in |grid_view|'s coordinates. + AppListItemView* SimulateInitiateDrag(AppsGridView* grid_view, + AppsGridView::Pointer pointer, + const gfx::Point& point) { + AppListItemView* view = GetItemViewAtPointInGrid(grid_view, point); + DCHECK(view); + + gfx::Point translated = + gfx::PointAtOffsetFromOrigin(point - view->bounds().origin()); + ui::MouseEvent pressed_event(ui::ET_MOUSE_PRESSED, translated, point, 0, 0); + grid_view->InitiateDrag(view, pointer, pressed_event); + return view; + } + + // |point| is in |grid_view|'s coordinates. + void SimulateUpdateDrag(AppsGridView* grid_view, + AppsGridView::Pointer pointer, + AppListItemView* drag_view, + const gfx::Point& point) { + DCHECK(drag_view); + gfx::Point translated = + gfx::PointAtOffsetFromOrigin(point - drag_view->bounds().origin()); + ui::MouseEvent drag_event(ui::ET_MOUSE_DRAGGED, translated, point, 0, 0); + grid_view->UpdateDragFromItem(pointer, drag_event); + } + + AppsGridView* RootGridView() { + return main_view_->contents_view()->apps_container_view()->apps_grid_view(); + } + + AppListFolderView* FolderView() { + return main_view_->contents_view() + ->apps_container_view() + ->app_list_folder_view(); + } + + AppsGridView* FolderGridView() { return FolderView()->items_grid_view(); } + + const views::ViewModel* RootViewModel() { + return RootGridView()->view_model_for_test(); + } + + const views::ViewModel* FolderViewModel() { + return FolderGridView()->view_model_for_test(); } protected: @@ -74,7 +181,8 @@ class AppListMainViewTest : public views::ViewsTestBase { // Tests changing the AppListModel when switching profiles. TEST_F(AppListMainViewTest, ModelChanged) { - EXPECT_EQ(kInitialItems, ViewModel()->view_size()); + delegate_->GetTestModel()->PopulateApps(kInitialItems); + EXPECT_EQ(kInitialItems, RootViewModel()->view_size()); // The model is owned by a profile keyed service, which is never destroyed // until after profile switching. @@ -83,7 +191,86 @@ TEST_F(AppListMainViewTest, ModelChanged) { const int kReplacementItems = 5; delegate_->ReplaceTestModel(kReplacementItems); main_view_->ModelChanged(); - EXPECT_EQ(kReplacementItems, ViewModel()->view_size()); + EXPECT_EQ(kReplacementItems, RootViewModel()->view_size()); +} + +// Tests dragging an item out of a single item folder and drop it at the last +// slot. +TEST_F(AppListMainViewTest, DragLastItemFromFolderAndDropAtLastSlot) { + // Prepare single folder with a single item in it. + AppListFolderItem* folder_item = + delegate_->GetTestModel()->CreateSingleItemFolder("single_item_folder", + "single"); + EXPECT_EQ(folder_item, + delegate_->GetTestModel()->FindFolderItem("single_item_folder")); + EXPECT_EQ(AppListFolderItem::kItemType, folder_item->GetItemType()); + + EXPECT_EQ(1, RootViewModel()->view_size()); + AppListItemView* folder_item_view = + static_cast(RootViewModel()->view_at(0)); + EXPECT_EQ(folder_item_view->item(), folder_item); + const gfx::Rect first_slot_tile = folder_item_view->bounds(); + + // Click on the folder to open it. + EXPECT_FALSE(FolderView()->visible()); + SimulateClick(folder_item_view); + base::RunLoop().RunUntilIdle(); + EXPECT_TRUE(FolderView()->visible()); + +#if defined(OS_WIN) + AppsGridViewTestApi folder_grid_view_test_api(FolderGridView()); + folder_grid_view_test_api.DisableSynchronousDrag(); +#endif + + // Start to drag the item in folder. + EXPECT_EQ(1, FolderViewModel()->view_size()); + views::View* item_view = FolderViewModel()->view_at(0); + gfx::Point point = item_view->bounds().CenterPoint(); + AppListItemView* dragged = + SimulateInitiateDrag(FolderGridView(), AppsGridView::MOUSE, point); + EXPECT_EQ(item_view, dragged); + EXPECT_FALSE(RootGridView()->visible()); + EXPECT_TRUE(FolderView()->visible()); + + // Drag it to top left corner. + point = gfx::Point(0, 0); + // Two update drags needed to actually drag the view. The first changes state + // and the 2nd one actually moves the view. The 2nd call can be removed when + // UpdateDrag is fixed. + SimulateUpdateDrag(FolderGridView(), AppsGridView::MOUSE, dragged, point); + SimulateUpdateDrag(FolderGridView(), AppsGridView::MOUSE, dragged, point); + base::RunLoop().RunUntilIdle(); + + // Wait until the folder view is invisible and root grid view shows up. + GridViewVisibleWaiter(RootGridView()).Wait(); + EXPECT_TRUE(RootGridView()->visible()); + EXPECT_EQ(0, FolderView()->layer()->opacity()); + + // Drop it to the slot on the right of first slot. + gfx::Rect drop_target_tile(first_slot_tile); + drop_target_tile.Offset(first_slot_tile.width(), 0); + point = drop_target_tile.CenterPoint(); + SimulateUpdateDrag(FolderGridView(), AppsGridView::MOUSE, dragged, point); + SimulateUpdateDrag(FolderGridView(), AppsGridView::MOUSE, dragged, point); + base::RunLoop().RunUntilIdle(); + + // Drop it. + FolderGridView()->EndDrag(false); + base::RunLoop().RunUntilIdle(); + + // Folder icon view should be gone and there is only one item view. + EXPECT_EQ(1, RootViewModel()->view_size()); + EXPECT_EQ(AppListItemView::kViewClassName, + RootViewModel()->view_at(0)->GetClassName()); + + // The item view should be in slot 1 instead of slot 2 where it is dropped. + AppsGridViewTestApi root_grid_view_test_api(RootGridView()); + root_grid_view_test_api.LayoutToIdealBounds(); + EXPECT_EQ(first_slot_tile, RootViewModel()->view_at(0)->bounds()); + + // Single item folder should be auto removed. + EXPECT_EQ(NULL, + delegate_->GetTestModel()->FindFolderItem("single_item_folder")); } } // namespace test diff --git a/ui/app_list/views/apps_grid_view.cc b/ui/app_list/views/apps_grid_view.cc index e8c60f58eda8..52034d8c024b 100644 --- a/ui/app_list/views/apps_grid_view.cc +++ b/ui/app_list/views/apps_grid_view.cc @@ -348,6 +348,9 @@ AppsGridView::AppsGridView(AppsGridViewDelegate* delegate, selected_view_(NULL), drag_view_(NULL), drag_start_page_(-1), +#if defined(OS_WIN) + use_synchronous_drag_(true), +#endif drag_pointer_(NONE), drop_attempt_(DROP_FOR_NONE), drag_and_drop_host_(NULL), @@ -402,6 +405,8 @@ void AppsGridView::ResetForShowApps() { for (int i = 0; i < view_model_.view_size(); ++i) { view_model_.view_at(i)->SetVisible(true); } + CHECK_EQ(item_list_->item_count(), + static_cast(view_model_.view_size())); } void AppsGridView::SetModel(AppListModel* model) { @@ -477,7 +482,7 @@ void AppsGridView::InitiateDrag(AppListItemView* view, void AppsGridView::StartSettingUpSynchronousDrag() { #if defined(OS_WIN) - if (!delegate_) + if (!delegate_ || !use_synchronous_drag_) return; // Folders can't be integrated with the OS. @@ -812,7 +817,9 @@ void AppsGridView::ClearDragState() { if (drag_view_) { drag_view_->OnDragEnded(); if (IsDraggingForReparentInRootLevelGridView()) { - DeleteItemViewAtIndex(view_model_.GetIndexOfView(drag_view_)); + const int drag_view_index = view_model_.GetIndexOfView(drag_view_); + CHECK_EQ(view_model_.view_size() - 1, drag_view_index); + DeleteItemViewAtIndex(drag_view_index); } } drag_view_ = NULL; @@ -939,6 +946,9 @@ bool AppsGridView::OnKeyReleased(const ui::KeyEvent& event) { void AppsGridView::ViewHierarchyChanged( const ViewHierarchyChangedDetails& details) { if (!details.is_add && details.parent == this) { + // The view being delete should not have reference in |view_model_|. + CHECK_EQ(-1, view_model_.GetIndexOfView(details.child)); + if (selected_view_ == details.child) selected_view_ = NULL; if (activated_folder_item_view_ == details.child) @@ -1693,16 +1703,23 @@ void AppsGridView::ReparentItemForReorder(views::View* item_view, AppListFolderItem* source_folder = static_cast(item_list_->FindItem(source_folder_id)); + int target_model_index = GetModelIndexFromIndex(target); + // Remove the source folder view if there is only 1 item in it, since the // source folder will be deleted after its only child item removed from it. - if (source_folder->ChildItemCount() == 1u) - DeleteItemViewAtIndex( - view_model_.GetIndexOfView(activated_folder_item_view())); + if (source_folder->ChildItemCount() == 1u) { + const int deleted_folder_index = + view_model_.GetIndexOfView(activated_folder_item_view()); + DeleteItemViewAtIndex(deleted_folder_index); + + // Adjust |target_model_index| if it is beyond the deleted folder index. + if (target_model_index > deleted_folder_index) + --target_model_index; + } // Move the item from its parent folder to top level item list. // Must move to target_model_index, the location we expect the target item // to be, not the item location we want to insert before. - int target_model_index = GetModelIndexFromIndex(target); int current_model_index = view_model_.GetIndexOfView(item_view); syncer::StringOrdinal target_position; if (target_model_index < static_cast(item_list_->item_count())) @@ -1804,7 +1821,11 @@ void AppsGridView::RemoveLastItemFromReparentItemFolderIfNecessary( // Create a new item view for the last item in folder. size_t last_item_index; - item_list_->FindItemIndex(last_item->id(), &last_item_index); + if (!item_list_->FindItemIndex(last_item->id(), &last_item_index) || + last_item_index > static_cast(view_model_.view_size())) { + NOTREACHED(); + return; + } views::View* last_item_view = CreateViewForItemAtIndex(last_item_index); view_model_.Add(last_item_view, last_item_index); AddChildView(last_item_view); @@ -2016,7 +2037,7 @@ AppsGridView::Index AppsGridView::GetNearestTileForDragView() { // If user drags an item across pages to the last page, and targets it // to the last empty slot on it, push the last item for re-ordering. - if (IsFirstEmptySlot(nearest_tile) && d_min < d_reorder) { + if (IsLastPossibleDropTarget(nearest_tile) && d_min < d_reorder) { drop_attempt_ = DROP_FOR_REORDER; nearest_tile.slot = nearest_tile.slot - 1; return nearest_tile; @@ -2119,14 +2140,8 @@ gfx::Rect AppsGridView::GetTileBounds(int row, int col) const { return tile_rect; } -bool AppsGridView::IsFirstEmptySlot(const Index& index) const { - // Don't count the hidden drag_view_ created from the item_dragged out of a - // folder during re-parenting into the total number of views that are visible - // on all grid view pages. - int total_views = IsDraggingForReparentInRootLevelGridView() - ? view_model_.view_size() - 1 - : view_model_.view_size(); - int last_possible_slot = (total_views - 1) % tiles_per_page(); +bool AppsGridView::IsLastPossibleDropTarget(const Index& index) const { + int last_possible_slot = view_model_.view_size() % tiles_per_page(); return (index.page == pagination_model_->total_pages() - 1 && index.slot == last_possible_slot + 1); } diff --git a/ui/app_list/views/apps_grid_view.h b/ui/app_list/views/apps_grid_view.h index 79d19e650941..c6c9efa6f800 100644 --- a/ui/app_list/views/apps_grid_view.h +++ b/ui/app_list/views/apps_grid_view.h @@ -403,9 +403,9 @@ class APP_LIST_EXPORT AppsGridView : public views::View, // Gets the bounds of the tile located at |row| and |col| on current page. gfx::Rect GetTileBounds(int row, int col) const; - // Returns true if the slot of |index| is the first empty slot next to the - // last item on the last page. - bool IsFirstEmptySlot(const Index& index) const; + // Returns true if the slot of |index| is the last possible slot to drop + // an item, i.e. first empty slot next to the last item on the last page. + bool IsLastPossibleDropTarget(const Index& index) const; // Gets the item view located at |slot| on the current page. If there is // no item located at |slot|, returns NULL. @@ -497,6 +497,9 @@ class APP_LIST_EXPORT AppsGridView : public views::View, // Created when a drag is started (ie: drag exceeds the drag threshold), but // not Run() until supplied with a shortcut path. scoped_refptr synchronous_drag_; + + // Whether to use SynchronousDrag to support dropping to task bar etc. + bool use_synchronous_drag_; #endif Pointer drag_pointer_; diff --git a/ui/app_list/views/test/apps_grid_view_test_api.cc b/ui/app_list/views/test/apps_grid_view_test_api.cc index 5e425c384884..6b95b17caa6e 100644 --- a/ui/app_list/views/test/apps_grid_view_test_api.cc +++ b/ui/app_list/views/test/apps_grid_view_test_api.cc @@ -36,5 +36,14 @@ void AppsGridViewTestApi::PressItemAt(int index) { ui::KeyEvent(ui::ET_KEY_PRESSED, ui::VKEY_RETURN, 0, false)); } +void AppsGridViewTestApi::DisableSynchronousDrag() { +#if defined(OS_WIN) + DCHECK(view_->synchronous_drag_ == NULL) << "DisableSynchronousDrag needs to " + "be called before " + "synchronous_drag_ is set up."; + view_->use_synchronous_drag_ = false; +#endif +} + } // namespace test } // namespace app_list diff --git a/ui/app_list/views/test/apps_grid_view_test_api.h b/ui/app_list/views/test/apps_grid_view_test_api.h index c8f7a8323747..234fc9abecd3 100644 --- a/ui/app_list/views/test/apps_grid_view_test_api.h +++ b/ui/app_list/views/test/apps_grid_view_test_api.h @@ -30,6 +30,8 @@ class AppsGridViewTestApi { void PressItemAt(int index); + void DisableSynchronousDrag(); + private: AppsGridView* view_; diff --git a/ui/views/view_model.cc b/ui/views/view_model.cc index 5f492f5380d7..a0a754964508 100644 --- a/ui/views/view_model.cc +++ b/ui/views/view_model.cc @@ -33,6 +33,11 @@ void ViewModel::Remove(int index) { } void ViewModel::Move(int index, int target_index) { + DCHECK_LT(index, static_cast(entries_.size())); + DCHECK_GE(index, 0); + DCHECK_LT(target_index, static_cast(entries_.size())); + DCHECK_GE(target_index, 0); + if (index == target_index) return; Entry entry(entries_[index]); -- 2.11.4.GIT