Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2972243002:
Fix app list item indexing bug. (Closed)
|
Created:
3 years, 5 months ago by Jiaquan He Modified:
3 years, 5 months ago CC:
chromium-reviews, tfarina, Matt Giuca Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix app list item indexing bug.
When the Play Store app search feature is enabled, we have
separators between items in the SearchResultTileItemListView.
We have to skip those separators while indexing items.
BUG=739410
BUG=739841
Review-Url: https://codereview.chromium.org/2972243002
Cr-Commit-Position: refs/heads/master@{#488098}
Committed: https://chromium.googlesource.com/chromium/src/+/57d461f80f1d36ca3c85a1da43ad8538c5e992cf
Patch Set 1 #Patch Set 2 : Add a unit test. #Patch Set 3 : Correct the wrong year number. #
Total comments: 28
Patch Set 4 : Oshima's comments. #
Total comments: 4
Patch Set 5 : khmel's comments. #
Total comments: 10
Patch Set 6 : khmel's comments. #
Total comments: 2
Patch Set 7 : oshima's comments. #Patch Set 8 : oshima's comments. #
Messages
Total messages: 53 (36 generated)
Description was changed from ========== Fix app list item indexing bug. When the Play Store app search feature is enabled, we have separators between items in the SearchResultTileItemListView. We have to skip those separators while indexing items. BUG=736157 ========== to ========== Fix app list item indexing bug. When the Play Store app search feature is enabled, we have separators between items in the SearchResultTileItemListView. We have to skip those separators while indexing items. BUG=736157 ==========
hejq@chromium.org changed reviewers: + oshima@chromium.org, weidongg@chromium.org, xiyuan@chromium.org
The CQ bit was checked by hejq@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
For those trybot failures: This CL should go after https://codereview.chromium.org/2961923002/
Description was changed from ========== Fix app list item indexing bug. When the Play Store app search feature is enabled, we have separators between items in the SearchResultTileItemListView. We have to skip those separators while indexing items. BUG=736157 ========== to ========== Fix app list item indexing bug. When the Play Store app search feature is enabled, we have separators between items in the SearchResultTileItemListView. We have to skip those separators while indexing items. BUG=736157 BUG=739410 ==========
can you add a unit test for this scenario?
Description was changed from ========== Fix app list item indexing bug. When the Play Store app search feature is enabled, we have separators between items in the SearchResultTileItemListView. We have to skip those separators while indexing items. BUG=736157 BUG=739410 ========== to ========== Fix app list item indexing bug. When the Play Store app search feature is enabled, we have separators between items in the SearchResultTileItemListView. We have to skip those separators while indexing items. BUG=739410 BUG=739841 ==========
hejq@chromium.org changed reviewers: + jennyz@chromium.org
The CQ bit was checked by hejq@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi reviewers, Unittests are added. Does this look better?
hejq@chromium.org changed reviewers: + khmel@chromium.org
https://codereview.chromium.org/2972243002/diff/40001/ui/app_list/views/searc... File ui/app_list/views/search_result_tile_item_list_view_unittest.cc (right): https://codereview.chromium.org/2972243002/diff/40001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_list_view_unittest.cc:7: #include <map> do you need this? https://codereview.chromium.org/2972243002/diff/40001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_list_view_unittest.cc:26: namespace test { 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). https://codereview.chromium.org/2972243002/diff/40001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_list_view_unittest.cc:29: const int kMaxNumSearchResultTiles = 6; constexpr https://codereview.chromium.org/2972243002/diff/40001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_list_view_unittest.cc:40: void SetUp() override { views::ViewsTestBase::SetUp(); } you don't need this https://codereview.chromium.org/2972243002/diff/40001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_list_view_unittest.cc:44: textfield_.reset(new views::Textfield()); MakeUnique https://codereview.chromium.org/2972243002/diff/40001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_list_view_unittest.cc:46: new SearchResultTileItemListView(textfield_.get(), &view_delegate_)); ditto https://codereview.chromium.org/2972243002/diff/40001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_list_view_unittest.cc:89: RunPendingMessages(); it's not clear why you need this. can you clarify? https://codereview.chromium.org/2972243002/diff/40001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_list_view_unittest.cc:93: int GetOpenResultCount(int ranking) { const https://codereview.chromium.org/2972243002/diff/40001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_list_view_unittest.cc:102: int GetResultCount() { return view_->num_results(); } const? https://codereview.chromium.org/2972243002/diff/40001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_list_view_unittest.cc:104: int GetSelectedIndex() { return view_->selected_index(); } ditto https://codereview.chromium.org/2972243002/diff/40001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_list_view_unittest.cc:106: void ResetSelectedIndex() { view_->SetSelectedIndex(0); } ditto https://codereview.chromium.org/2972243002/diff/40001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_list_view_unittest.cc:136: } can you also test what happens if you tab extra time? https://codereview.chromium.org/2972243002/diff/40001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_list_view_unittest.cc:147: } new line https://codereview.chromium.org/2972243002/diff/40001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_list_view_unittest.cc:150: } nuke {}
Done. Thanks! https://codereview.chromium.org/2972243002/diff/40001/ui/app_list/views/searc... File ui/app_list/views/search_result_tile_item_list_view_unittest.cc (right): https://codereview.chromium.org/2972243002/diff/40001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_list_view_unittest.cc:7: #include <map> On 2017/07/14 00:54:56, oshima wrote: > do you need this? No. Thanks for the catch. https://codereview.chromium.org/2972243002/diff/40001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_list_view_unittest.cc:26: namespace test { On 2017/07/14 00:54:57, oshima wrote: > 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). Thanks for this information! https://codereview.chromium.org/2972243002/diff/40001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_list_view_unittest.cc:29: const int kMaxNumSearchResultTiles = 6; On 2017/07/14 00:54:57, oshima wrote: > constexpr Done. https://codereview.chromium.org/2972243002/diff/40001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_list_view_unittest.cc:40: void SetUp() override { views::ViewsTestBase::SetUp(); } On 2017/07/14 00:54:57, oshima wrote: > you don't need this Done. https://codereview.chromium.org/2972243002/diff/40001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_list_view_unittest.cc:44: textfield_.reset(new views::Textfield()); On 2017/07/14 00:54:57, oshima wrote: > MakeUnique Done. https://codereview.chromium.org/2972243002/diff/40001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_list_view_unittest.cc:46: new SearchResultTileItemListView(textfield_.get(), &view_delegate_)); On 2017/07/14 00:54:57, oshima wrote: > ditto Done. https://codereview.chromium.org/2972243002/diff/40001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_list_view_unittest.cc:89: RunPendingMessages(); On 2017/07/14 00:54:56, oshima wrote: > it's not clear why you need this. can you clarify? Done. https://codereview.chromium.org/2972243002/diff/40001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_list_view_unittest.cc:93: int GetOpenResultCount(int ranking) { On 2017/07/14 00:54:56, oshima wrote: > const Done. https://codereview.chromium.org/2972243002/diff/40001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_list_view_unittest.cc:102: int GetResultCount() { return view_->num_results(); } On 2017/07/14 00:54:57, oshima wrote: > const? Done. https://codereview.chromium.org/2972243002/diff/40001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_list_view_unittest.cc:104: int GetSelectedIndex() { return view_->selected_index(); } On 2017/07/14 00:54:57, oshima wrote: > ditto Done. https://codereview.chromium.org/2972243002/diff/40001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_list_view_unittest.cc:106: void ResetSelectedIndex() { view_->SetSelectedIndex(0); } On 2017/07/14 00:54:56, oshima wrote: > ditto Done. https://codereview.chromium.org/2972243002/diff/40001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_list_view_unittest.cc:136: } On 2017/07/14 00:54:56, oshima wrote: > can you also test what happens if you tab extra time? Done. https://codereview.chromium.org/2972243002/diff/40001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_list_view_unittest.cc:147: } On 2017/07/14 00:54:56, oshima wrote: > new line Done. I've merged the for below into the one above https://codereview.chromium.org/2972243002/diff/40001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_list_view_unittest.cc:150: } On 2017/07/14 00:54:57, oshima wrote: > nuke {} Done.
The CQ bit was checked by hejq@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2972243002/diff/60001/ui/app_list/views/searc... File ui/app_list/views/search_result_tile_item_list_view.cc (right): https://codereview.chromium.org/2972243002/diff/60001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_list_view.cc:48: is_play_store_app_search_enabled_( Q: I think this is not enough to check. This feature might be set but: 1. Profile is not allowed to have ARC. What is behavior in this casE? 2. ARC might not contain Play Store app. See IsPlayStoreAvailable. 3. ARC can be opted out this time. Is is expected that in all this cases we have different layout? https://codereview.chromium.org/2972243002/diff/60001/ui/app_list/views/searc... File ui/app_list/views/search_result_tile_item_list_view_unittest.cc (right): https://codereview.chromium.org/2972243002/diff/60001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_list_view_unittest.cc:32: class SearchResultTileItemListViewTest : public views::ViewsTestBase { It would be nice to have test with bool parameter. One is for EnablePlayStore search and second is for DisabledPlayStore search. WDYT?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2972243002/diff/60001/ui/app_list/views/searc... File ui/app_list/views/search_result_tile_item_list_view.cc (right): https://codereview.chromium.org/2972243002/diff/60001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_list_view.cc:48: is_play_store_app_search_enabled_( On 2017/07/17 17:16:12, khmel wrote: > Q: I think this is not enough to check. > This feature might be set but: > > 1. Profile is not allowed to have ARC. What is behavior in this casE? > 2. ARC might not contain Play Store app. See IsPlayStoreAvailable. > 3. ARC can be opted out this time. > > Is is expected that in all this cases we have different layout? Yes. We only enable this feature in the new launcher, and we want the layout be constant in all these cases. https://codereview.chromium.org/2972243002/diff/60001/ui/app_list/views/searc... File ui/app_list/views/search_result_tile_item_list_view_unittest.cc (right): https://codereview.chromium.org/2972243002/diff/60001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_list_view_unittest.cc:32: class SearchResultTileItemListViewTest : public views::ViewsTestBase { On 2017/07/17 17:16:12, khmel wrote: > It would be nice to have test with bool parameter. One is for EnablePlayStore > search and second is for DisabledPlayStore search. > WDYT? This makes sense. But I'm not sure what is a best way to do this. Could you have a look at the new PS?
The CQ bit was checked by hejq@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2972243002/diff/80001/ui/app_list/test/app_li... File ui/app_list/test/app_list_test_view_delegate.h (right): https://codereview.chromium.org/2972243002/diff/80001/ui/app_list/test/app_li... ui/app_list/test/app_list_test_view_delegate.h:34: std::map<size_t, int> open_search_result_counts() const { nit: is any reason to return copy of this? would const std::map<size_t, int>& work? https://codereview.chromium.org/2972243002/diff/80001/ui/app_list/views/searc... File ui/app_list/views/search_result_tile_item_list_view.cc (right): https://codereview.chromium.org/2972243002/diff/80001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_list_view.cc:174: int child_index = is_play_store_app_search_enabled_ ? selection_index * 2 + 1 nit: const int
Done. Thanks! https://codereview.chromium.org/2972243002/diff/80001/ui/app_list/test/app_li... File ui/app_list/test/app_list_test_view_delegate.h (right): https://codereview.chromium.org/2972243002/diff/80001/ui/app_list/test/app_li... ui/app_list/test/app_list_test_view_delegate.h:34: std::map<size_t, int> open_search_result_counts() const { On 2017/07/18 15:04:01, khmel wrote: > nit: is any reason to return copy of this? would const std::map<size_t, int>& > work? 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. https://codereview.chromium.org/2972243002/diff/80001/ui/app_list/views/searc... File ui/app_list/views/search_result_tile_item_list_view.cc (right): https://codereview.chromium.org/2972243002/diff/80001/ui/app_list/views/searc... ui/app_list/views/search_result_tile_item_list_view.cc:174: int child_index = is_play_store_app_search_enabled_ ? selection_index * 2 + 1 On 2017/07/18 15:04:01, khmel wrote: > nit: const int Done.
The CQ bit was checked by hejq@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2972243002/diff/80001/ui/app_list/test/app_li... File ui/app_list/test/app_list_test_view_delegate.h (right): https://codereview.chromium.org/2972243002/diff/80001/ui/app_list/test/app_li... ui/app_list/test/app_list_test_view_delegate.h:34: std::map<size_t, int> open_search_result_counts() const { On 2017/07/18 15:58:16, Jiaquan He wrote: > On 2017/07/18 15:04:01, khmel wrote: > > nit: is any reason to return copy of this? would const std::map<size_t, int>& > > work? > > 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. 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() ? https://codereview.chromium.org/2972243002/diff/100001/ui/app_list/views/sear... File ui/app_list/views/search_result_tile_item_list_view_unittest.cc (right): https://codereview.chromium.org/2972243002/diff/100001/ui/app_list/views/sear... ui/app_list/views/search_result_tile_item_list_view_unittest.cc:166: } nit: nuke {}
oshima's comments. https://codereview.chromium.org/2972243002/diff/80001/ui/app_list/test/app_li... File ui/app_list/test/app_list_test_view_delegate.h (right): https://codereview.chromium.org/2972243002/diff/80001/ui/app_list/test/app_li... ui/app_list/test/app_list_test_view_delegate.h:34: std::map<size_t, int> open_search_result_counts() const { On 2017/07/18 16:55:26, oshima wrote: > On 2017/07/18 15:58:16, Jiaquan He wrote: > > On 2017/07/18 15:04:01, khmel wrote: > > > nit: is any reason to return copy of this? would const std::map<size_t, > int>& > > > work? > > > > 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. > > 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) So I think it makes sense to return a reference here. Is that correct? (And I checked. It won't break any existing test.) > > Can you also rename to open_search_result_counts_for_test() ? Just to confirm: Actually this whole file is used for test. Is it necessary to rename this method? https://codereview.chromium.org/2972243002/diff/100001/ui/app_list/views/sear... File ui/app_list/views/search_result_tile_item_list_view_unittest.cc (right): https://codereview.chromium.org/2972243002/diff/100001/ui/app_list/views/sear... ui/app_list/views/search_result_tile_item_list_view_unittest.cc:166: } On 2017/07/18 16:55:26, oshima wrote: > nit: nuke {} Done.
The CQ bit was checked by hejq@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Answering why returning reference doesn't change test behaviors. https://codereview.chromium.org/2972243002/diff/80001/ui/app_list/test/app_li... File ui/app_list/test/app_list_test_view_delegate.h (right): https://codereview.chromium.org/2972243002/diff/80001/ui/app_list/test/app_li... ui/app_list/test/app_list_test_view_delegate.h:34: std::map<size_t, int> open_search_result_counts() const { On 2017/07/18 17:41:36, Jiaquan He wrote: > On 2017/07/18 16:55:26, oshima wrote: > > On 2017/07/18 15:58:16, Jiaquan He wrote: > > > On 2017/07/18 15:04:01, khmel wrote: > > > > nit: is any reason to return copy of this? would const std::map<size_t, > > int>& > > > > work? > > > > > > 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. > > > > 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) > > So I think it makes sense to return a reference here. Is that correct? (And I > checked. It won't break any existing test.) > > > > > Can you also rename to open_search_result_counts_for_test() ? > > Just to confirm: Actually this whole file is used for test. Is it necessary to > rename this method? 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.
https://codereview.chromium.org/2972243002/diff/80001/ui/app_list/test/app_li... File ui/app_list/test/app_list_test_view_delegate.h (right): https://codereview.chromium.org/2972243002/diff/80001/ui/app_list/test/app_li... ui/app_list/test/app_list_test_view_delegate.h:34: std::map<size_t, int> open_search_result_counts() const { On 2017/07/18 17:41:36, Jiaquan He wrote: > On 2017/07/18 16:55:26, oshima wrote: > > On 2017/07/18 15:58:16, Jiaquan He wrote: > > > On 2017/07/18 15:04:01, khmel wrote: > > > > nit: is any reason to return copy of this? would const std::map<size_t, > > int>& > > > > work? > > > > > > 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. > > > > 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) > > So I think it makes sense to return a reference here. Is that correct? (And I > checked. It won't break any existing test.) > > > > > Can you also rename to open_search_result_counts_for_test() ? > > Just to confirm: Actually this whole file is used for test. Is it necessary to > rename this method? oh sorry, my mistake. Please ignore rename comment. https://codereview.chromium.org/2972243002/diff/80001/ui/app_list/test/app_li... ui/app_list/test/app_list_test_view_delegate.h:34: std::map<size_t, int> open_search_result_counts() const { On 2017/07/19 21:02:46, Jiaquan He wrote: > On 2017/07/18 17:41:36, Jiaquan He wrote: > > On 2017/07/18 16:55:26, oshima wrote: > > > On 2017/07/18 15:58:16, Jiaquan He wrote: > > > > On 2017/07/18 15:04:01, khmel wrote: > > > > > nit: is any reason to return copy of this? would const std::map<size_t, > > > int>& > > > > > work? > > > > > > > > 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. > > > > > > 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) > > > > So I think it makes sense to return a reference here. Is that correct? (And I > > checked. It won't break any existing test.) > > > > > > > > Can you also rename to open_search_result_counts_for_test() ? > > > > Just to confirm: Actually this whole file is used for test. Is it necessary to > > rename this method? > > 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. Can you make sure the ranking exist in the map? then lgtm
Done. PTAL :) https://codereview.chromium.org/2972243002/diff/80001/ui/app_list/test/app_li... File ui/app_list/test/app_list_test_view_delegate.h (right): https://codereview.chromium.org/2972243002/diff/80001/ui/app_list/test/app_li... ui/app_list/test/app_list_test_view_delegate.h:34: std::map<size_t, int> open_search_result_counts() const { On 2017/07/19 21:31:21, oshima wrote: > On 2017/07/19 21:02:46, Jiaquan He wrote: > > On 2017/07/18 17:41:36, Jiaquan He wrote: > > > On 2017/07/18 16:55:26, oshima wrote: > > > > On 2017/07/18 15:58:16, Jiaquan He wrote: > > > > > On 2017/07/18 15:04:01, khmel wrote: > > > > > > nit: is any reason to return copy of this? would const > std::map<size_t, > > > > int>& > > > > > > work? > > > > > > > > > > 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. > > > > > > > > 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) > > > > > > So I think it makes sense to return a reference here. Is that correct? (And > I > > > checked. It won't break any existing test.) > > > > > > > > > > > Can you also rename to open_search_result_counts_for_test() ? > > > > > > Just to confirm: Actually this whole file is used for test. Is it necessary > to > > > rename this method? > > > > 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. > > Can you make sure the ranking exist in the map? then lgtm Please check the new PS.
The CQ bit was checked by hejq@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by hejq@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from khmel@chromium.org Link to the patchset: https://codereview.chromium.org/2972243002/#ps140001 (title: "oshima's comments.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1500516850662060,
"parent_rev": "baad427b3c702754a6782f9e31eaf4ded6c09f89", "commit_rev":
"57d461f80f1d36ca3c85a1da43ad8538c5e992cf"}
Message was sent while issue was closed.
Description was changed from ========== Fix app list item indexing bug. When the Play Store app search feature is enabled, we have separators between items in the SearchResultTileItemListView. We have to skip those separators while indexing items. BUG=739410 BUG=739841 ========== to ========== Fix app list item indexing bug. When the Play Store app search feature is enabled, we have separators between items in the SearchResultTileItemListView. We have to skip those separators while indexing items. BUG=739410 BUG=739841 Review-Url: https://codereview.chromium.org/2972243002 Cr-Commit-Position: refs/heads/master@{#488098} Committed: https://chromium.googlesource.com/chromium/src/+/57d461f80f1d36ca3c85a1da43ad... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/57d461f80f1d36ca3c85a1da43ad... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
