Fix app list item indexing bug.

ui/app_list/test/app_list_test_view_delegate.h

 // Copyright 2013 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.
 
 #ifndef UI_APP_LIST_TEST_APP_LIST_TEST_VIEW_DELEGATE_H_
 #define UI_APP_LIST_TEST_APP_LIST_TEST_VIEW_DELEGATE_H_
 
 #include <stddef.h>
 
 #include <map>
@@ skipping 13 matching lines @@
 class AppListTestModel;
 
 // A concrete AppListViewDelegate for unit tests.
 class AppListTestViewDelegate : public AppListViewDelegate {
  public:
   AppListTestViewDelegate();
   ~AppListTestViewDelegate() override;
 
   int dismiss_count() { return dismiss_count_; }
   int open_search_result_count() { return open_search_result_count_; }
-  std::map<size_t, int> open_search_result_counts() {
+  std::map<size_t, int> open_search_result_counts() const {

[khmel, 2017/07/18 15:04:01]

nit: is any reason to return copy of this? would const std::map<size_t, int>&
work?

[Jiaquan He, 2017/07/18 15:58:16]

Just tried. Other callers cannot access operator[] and function clear from a
'const std::map<size_t, int>'.
But we can return std::map<size_t, int>&. And I'm going to remove the const
since other callers might use nonconst [] and clear.

[oshima, 2017/07/18 16:55:26]

Hmm, won't it change the behavior?

Looks like this is used only by a test, and the test may not be working as
intended.
(clear won't clear what's in the delegate)

Can you also rename to open_search_result_counts_for_test() ?

[Jiaquan He, 2017/07/18 17:41:36]

So I think it makes sense to return a reference here. Is that correct? (And I
checked. It won't break any existing test.)
Just to confirm: Actually this whole file is used for test. Is it necessary to
rename this method?

[Jiaquan He, 2017/07/19 21:02:46]

For why it doesn't change the behavior of the existing tests:

Currently only two tests are using this method:

(1)
https://cs.chromium.org/chromium/src/ui/app_list/views/search_result_answer_c...
(2)
https://cs.chromium.org/chromium/src/ui/app_list/views/search_result_list_vie...

And they *try to clear* the map. But after calling |clear|, they don't check
again whether the map is really cleared or not. Actually I checked that the map
in the delegate is not cleared at all.

[oshima, 2017/07/19 21:31:21]

oh sorry, my mistake. Please ignore rename comment.

[oshima, 2017/07/19 21:31:21]

Can you make sure the ranking exist in the map? then lgtm

[Jiaquan He, 2017/07/19 21:54:23]

Please check the new PS.

     return open_search_result_counts_;
   }
 
   // Sets the number of apps that the model will be created with the next time
   // SetProfileByPath() is called.
   void set_next_profile_app_count(int apps) { next_profile_app_count_ = apps; }
 
   void set_auto_launch_timeout(const base::TimeDelta& timeout) {
     auto_launch_timeout_ = timeout;
   }
@@ skipping 43 matching lines @@
   SpeechUIModel speech_ui_;
   base::TimeDelta auto_launch_timeout_;
 
   DISALLOW_COPY_AND_ASSIGN(AppListTestViewDelegate);
 };
 
 }  // namespace test
 }  // namespace app_list
 
 #endif  // UI_APP_LIST_TEST_APP_LIST_TEST_VIEW_DELEGATE_H_