Fix app list item indexing bug.

ui/app_list/views/search_result_tile_item_list_view_unittest.cc

Index: ui/app_list/views/search_result_tile_item_list_view_unittest.cc
diff --git a/ui/app_list/views/search_result_tile_item_list_view_unittest.cc b/ui/app_list/views/search_result_tile_item_list_view_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..e02cf5b3c41543ecaf172cc845a47a7aced00650
--- /dev/null
+++ b/ui/app_list/views/search_result_tile_item_list_view_unittest.cc
@@ -0,0 +1,154 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ui/app_list/views/search_result_tile_item_list_view.h"
+
+#include <map>

[oshima, 2017/07/14 00:54:56]

do you need this?

[Jiaquan He, 2017/07/17 16:57:33]

No. Thanks for the catch.

+#include <memory>
+#include <utility>
+
+#include "base/macros.h"
+#include "base/memory/ptr_util.h"
+#include "base/strings/stringprintf.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/test/scoped_feature_list.h"
+#include "ui/app_list/app_list_features.h"
+#include "ui/app_list/app_list_model.h"
+#include "ui/app_list/test/app_list_test_view_delegate.h"
+#include "ui/app_list/test/test_search_result.h"
+#include "ui/app_list/views/search_result_list_view_delegate.h"
+#include "ui/app_list/views/search_result_view.h"
+#include "ui/views/controls/textfield/textfield.h"
+#include "ui/views/test/views_test_base.h"
+
+namespace app_list {
+namespace test {

[oshima, 2017/07/14 00:54:57]

I'd suggest to remove test namespace. It's best used for test utilities, not the
test body itself (although I understand that there are such test files).

[Jiaquan He, 2017/07/17 16:57:33]

Thanks for this information!

+
+namespace {
+const int kMaxNumSearchResultTiles = 6;

[oshima, 2017/07/14 00:54:57]

constexpr

[Jiaquan He, 2017/07/17 16:57:34]

Done.

+const int kInstalledApps = 4;
+const int kPlayStoreApps = 2;
+}  // namespace
+
+class SearchResultTileItemListViewTest : public views::ViewsTestBase {
+ public:
+  SearchResultTileItemListViewTest() {}
+  ~SearchResultTileItemListViewTest() override {}
+
+  // Overridden from testing::Test:
+  void SetUp() override { views::ViewsTestBase::SetUp(); }

[oshima, 2017/07/14 00:54:57]

you don't need this

[Jiaquan He, 2017/07/17 16:57:33]

Done.

+
+ protected:
+  void CreateSearchResultTileItemListView() {
+    textfield_.reset(new views::Textfield());

[oshima, 2017/07/14 00:54:57]

MakeUnique

[Jiaquan He, 2017/07/17 16:57:33]

Done.

+    view_.reset(
+        new SearchResultTileItemListView(textfield_.get(), &view_delegate_));

[oshima, 2017/07/14 00:54:57]

ditto

[Jiaquan He, 2017/07/17 16:57:34]

Done.

+    view_->SetResults(view_delegate_.GetModel()->results());
+  }
+
+  void EnablePlayStoreAppSearch() {
+    scoped_feature_list_.InitAndEnableFeature(
+        app_list::features::kEnablePlayStoreAppSearch);
+  }
+
+  SearchResultTileItemListView* view() { return view_.get(); }
+
+  AppListModel::SearchResults* GetResults() {
+    return view_delegate_.GetModel()->results();
+  }
+
+  void SetUpSearchResults() {
+    AppListModel::SearchResults* results = GetResults();
+
+    // Populate results for installed applications.
+    for (int i = 0; i < kInstalledApps; ++i) {
+      std::unique_ptr<TestSearchResult> result =
+          base::MakeUnique<TestSearchResult>();
+      result->set_display_type(SearchResult::DISPLAY_TILE);
+      result->set_result_type(SearchResult::RESULT_INSTALLED_APP);
+      result->set_title(
+          base::UTF8ToUTF16(base::StringPrintf("InstalledApp %d", i)));
+      results->Add(std::move(result));
+    }
+
+    // Populate results for Play Store search applications.
+    if (app_list::features::IsPlayStoreAppSearchEnabled()) {
+      for (int i = 0; i < kPlayStoreApps; ++i) {
+        std::unique_ptr<TestSearchResult> result =
+            base::MakeUnique<TestSearchResult>();
+        result->set_display_type(SearchResult::DISPLAY_TILE);
+        result->set_result_type(SearchResult::RESULT_PLAYSTORE_APP);
+        result->set_title(
+            base::UTF8ToUTF16(base::StringPrintf("PlayStoreApp %d", i)));
+        results->Add(std::move(result));
+      }
+    }
+
+    // Adding results will schedule Update().
+    RunPendingMessages();

[oshima, 2017/07/14 00:54:56]

it's not clear why you need this. can you clarify?

[Jiaquan He, 2017/07/17 16:57:34]

Done.

+    view_->OnContainerSelected(false, false);
+  }
+
+  int GetOpenResultCount(int ranking) {

[oshima, 2017/07/14 00:54:56]

const

[Jiaquan He, 2017/07/17 16:57:34]

Done.

+    int result = view_delegate_.open_search_result_counts()[ranking];
+    return result;
+  }
+
+  void ResetOpenResultCount() {
+    view_delegate_.open_search_result_counts().clear();
+  }
+
+  int GetResultCount() { return view_->num_results(); }

[oshima, 2017/07/14 00:54:57]

const?

[Jiaquan He, 2017/07/17 16:57:33]

Done.

+
+  int GetSelectedIndex() { return view_->selected_index(); }

[oshima, 2017/07/14 00:54:57]

ditto

[Jiaquan He, 2017/07/17 16:57:33]

Done.

+
+  void ResetSelectedIndex() { view_->SetSelectedIndex(0); }

[oshima, 2017/07/14 00:54:56]

ditto

[Jiaquan He, 2017/07/17 16:57:33]

Done.

+
+  bool KeyPress(ui::KeyboardCode key_code) {
+    ui::KeyEvent event(ui::ET_KEY_PRESSED, key_code, ui::EF_NONE);
+    return view_->OnKeyPressed(event);
+  }
+
+ private:
+  AppListTestViewDelegate view_delegate_;
+  std::unique_ptr<SearchResultTileItemListView> view_;
+  std::unique_ptr<views::Textfield> textfield_;
+  base::test::ScopedFeatureList scoped_feature_list_;
+
+  DISALLOW_COPY_AND_ASSIGN(SearchResultTileItemListViewTest);
+};
+
+TEST_F(SearchResultTileItemListViewTest, Basic) {
+  EnablePlayStoreAppSearch();
+  CreateSearchResultTileItemListView();
+  SetUpSearchResults();
+
+  const int results = GetResultCount();
+  EXPECT_EQ(kInstalledApps + kPlayStoreApps, results);
+  // For each results, we added a separator for result type grouping.
+  EXPECT_EQ(kMaxNumSearchResultTiles * 2, view()->child_count());
+
+  // Tests item indexing.
+  for (int i = 1; i < results; ++i) {
+    EXPECT_TRUE(KeyPress(ui::VKEY_TAB));
+    EXPECT_EQ(i, GetSelectedIndex());
+  }

[oshima, 2017/07/14 00:54:56]

can you also test what happens if you tab extra time?

[Jiaquan He, 2017/07/17 16:57:33]

Done.

+
+  // Tests app opening.
+  ResetSelectedIndex();
+  ResetOpenResultCount();
+  for (int i = 1; i < results; ++i) {
+    EXPECT_TRUE(KeyPress(ui::VKEY_TAB));
+    EXPECT_EQ(i, GetSelectedIndex());
+    for (int j = 0; j < i; j++) {
+      EXPECT_TRUE(KeyPress(ui::VKEY_RETURN));
+    }
+  }

[oshima, 2017/07/14 00:54:56]

new line

[Jiaquan He, 2017/07/17 16:57:33]

Done. I've merged the for below into the one above

+  for (int i = 0; i < results; ++i) {
+    EXPECT_EQ(i, GetOpenResultCount(i));
+  }

[oshima, 2017/07/14 00:54:57]

nuke {}

[Jiaquan He, 2017/07/17 16:57:33]

Done.

+}
+
+}  // namespace test
+}  // namespace app_list