Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
775893005:
[AppList] Add a Suggestions Provider to applist. (Closed)
Description[AppList] Add a Suggestions Provider to applist.
Only works with the null query, will return suggestions data if available (user is signed-in and suggestions are available for them).
BUG=440487
TEST=SuggestionsSearchProviderTest
Committed: https://crrev.com/04ef4a4d6e9303da91f432821d9601d49bdb6c02
Cr-Commit-Position: refs/heads/master@{#307761}
Patch Set 1 #
Total comments: 33
Patch Set 2 : Addressed comments #
Total comments: 24
Patch Set 3 : fixed test #
Total comments: 21
Patch Set 4 : addressed comments #
Total comments: 14
Patch Set 5 : final comments #Patch Set 6 : test fix #Messages
Total messages: 39 (12 generated)
mathp@chromium.org changed reviewers: + huangs@chromium.org, rogerm@chromium.org
Preliminary look?
First scan. Maybe run Clang format? https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... File chrome/browser/ui/app_list/search/search_controller_factory.cc (right): https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/search/search_controller_factory.cc:5: #include "chrome/browser/ui/app_list/search/search_controller_factory.h" The .h file is missing #include "base/memory/scoped_ptr.h" https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... File chrome/browser/ui/app_list/search/search_util.h (right): https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/search/search_util.h:25: SEARCH_RESULT_TYPE_BOUNDARY SEARCH_RESULT_TYPE_BOUNDARY should appear last? If so add comment. https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... File chrome/browser/ui/app_list/search/suggestions/suggestions_result.h (right): https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/search/suggestions/suggestions_result.h:8: #include "base/memory/scoped_ptr.h" Forward-declare AppListControllerDelegate instead? https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/search/suggestions/suggestions_result.h:9: #include "chrome/browser/ui/app_list/app_list_controller_delegate.h" Forward-declare AppListControllerDelegate instead? https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/search/suggestions/suggestions_result.h:11: #include "components/suggestions/suggestions_service.h" Forward-declare suggestions::SuggestionsService instead? https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/search/suggestions/suggestions_result.h:25: const suggestions::ChromeSuggestion suggestion, Missing "&" for const ? https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... File chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.cc (right): https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.cc:7: #include <string> Inlude string16 instead of <string>. https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.cc:86: Extra space. https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... File chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.h (right): https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.h:18: } } // namespace suggestions https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... File chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc (right): https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc:8: #include <algorithm> #include <vector> https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc:32: const int kTimeGapUsec = 100000; // 0.1 s. https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc:34: void AddSuggestion(SuggestionsProfile* suggestions, const char *title, Group "*" with "char". https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc:79: // Sort results by relevance. // Sort results from most to least relevant. https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc:80: std::vector<SearchResult*> sorted_results; Can copy from constructor: std::vector<SearchResult*> sorted_results(suggestions_search_->results().begin(), suggestions_search_->results().end()); https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc:135: // Empty query with 2 suggestions in cache returns them, with "foo" being more Weird sentence: the empty queue does the returning?
On 2014/12/03 20:13:51, Mathieu Perreault wrote: > Preliminary look? Awesome! This looks good to me. Let's see what the apps folks say. :)
https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... File chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.cc (right): https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.cc:86: uber-nit: extra whitespace
mathp@chromium.org changed reviewers: + asvitkine@chromium.org, calamity@chromium.org
+calamity (app_list) +asvitkine Hi, Please have a look! https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... File chrome/browser/ui/app_list/search/search_controller_factory.cc (right): https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/search/search_controller_factory.cc:5: #include "chrome/browser/ui/app_list/search/search_controller_factory.h" On 2014/12/03 20:36:11, huangs1 wrote: > The .h file is missing > #include "base/memory/scoped_ptr.h" I see it! https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... File chrome/browser/ui/app_list/search/search_util.h (right): https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/search/search_util.h:25: SEARCH_RESULT_TYPE_BOUNDARY On 2014/12/03 20:36:11, huangs1 wrote: > SEARCH_RESULT_TYPE_BOUNDARY should appear last? If so add comment. Done. https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... File chrome/browser/ui/app_list/search/suggestions/suggestions_result.h (right): https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/search/suggestions/suggestions_result.h:8: #include "base/memory/scoped_ptr.h" On 2014/12/03 20:36:11, huangs1 wrote: > Forward-declare AppListControllerDelegate instead? Done. https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/search/suggestions/suggestions_result.h:9: #include "chrome/browser/ui/app_list/app_list_controller_delegate.h" On 2014/12/03 20:36:11, huangs1 wrote: > Forward-declare AppListControllerDelegate instead? Done. https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/search/suggestions/suggestions_result.h:11: #include "components/suggestions/suggestions_service.h" On 2014/12/03 20:36:11, huangs1 wrote: > Forward-declare suggestions::SuggestionsService instead? Done. https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/search/suggestions/suggestions_result.h:25: const suggestions::ChromeSuggestion suggestion, On 2014/12/03 20:36:11, huangs1 wrote: > Missing "&" for const ? Done. https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... File chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.cc (right): https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.cc:7: #include <string> On 2014/12/03 20:36:11, huangs1 wrote: > Inlude string16 instead of <string>. Done. https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.cc:86: On 2014/12/03 20:36:11, huangs1 wrote: > Extra space. Done. https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.cc:86: On 2014/12/03 20:36:11, huangs1 wrote: > Extra space. Done. https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... File chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.h (right): https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.h:18: } On 2014/12/03 20:36:11, huangs1 wrote: > } // namespace suggestions Done. https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... File chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc (right): https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc:8: On 2014/12/03 20:36:12, huangs1 wrote: > #include <algorithm> > > #include <vector> Done. https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc:32: const int kTimeGapUsec = 100000; On 2014/12/03 20:36:11, huangs1 wrote: > // 0.1 s. Done. https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc:34: void AddSuggestion(SuggestionsProfile* suggestions, const char *title, On 2014/12/03 20:36:12, huangs1 wrote: > Group "*" with "char". Done. https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc:79: // Sort results by relevance. On 2014/12/03 20:36:11, huangs1 wrote: > // Sort results from most to least relevant. Done. https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc:80: std::vector<SearchResult*> sorted_results; On 2014/12/03 20:36:11, huangs1 wrote: > Can copy from constructor: > > std::vector<SearchResult*> > sorted_results(suggestions_search_->results().begin(), > suggestions_search_->results().end()); Done. https://codereview.chromium.org/775893005/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc:135: // Empty query with 2 suggestions in cache returns them, with "foo" being more On 2014/12/03 20:36:11, huangs1 wrote: > Weird sentence: the empty queue does the returning? Done. https://codereview.chromium.org/775893005/diff/1/ui/app_list/search/mixer.cc File ui/app_list/search/mixer.cc (right): https://codereview.chromium.org/775893005/diff/1/ui/app_list/search/mixer.cc#... ui/app_list/search/mixer.cc:25: const size_t kMaxSuggestionsResults = 4; App launcher folks: I'm not sure any of this is necessary since we're only doing null-query. Thoughts?
Ping to calamity@
One or both of calamity or myself will look at this today. (It's quite big so it might take some time to look over.)
First pass, a couple of tough questions. It would be good to log a bug for adding this feature (something like 'Add a suggestiong search provider to the app list'), just so there's an easy place to find all initial feature work related to this. https://codereview.chromium.org/775893005/diff/20001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/search/suggestions/suggestions_result.cc (right): https://codereview.chromium.org/775893005/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/suggestions/suggestions_result.cc:34: if (index_) set_relevance(1.0 / index_); label public_relevance: I think that you should be setting the relevance in suggestions_search_provider.cc by making set_relevance() public for this class. Since this class isn't intrinsically calculating its relevance, it's confusing to work out from this class alone what index is supposed to mean. Also, there's an implicit requirement that index be 1-based which would go away if the relevance was set from the provider. https://codereview.chromium.org/775893005/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/suggestions/suggestions_result.cc:45: ui::DispositionFromEventFlags(event_flags)); So this type of result is a URL suggestion? If so, I think it would be more informative to name the class URLSuggestionResult or similar. https://codereview.chromium.org/775893005/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/suggestions/suggestions_result.cc:50: profile_, list_controller_, suggestions_service_, suggestion_, index_)); If you went through with #public_relevance, you'd need to set the relevance of the new result here too. https://codereview.chromium.org/775893005/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/suggestions/suggestions_result.cc:59: } Hmm. Does this mean that a blank suggestion will flicker onto the suggestions and then vanish once it figures out it has no icon? If that's the case, it may be better to have the SuggestionsSearchProvider fan-in all of these icon loads before creating any SuggestionsResults. The apps provider kind of gets away with it because it's loading from the disk and the result doesn't disappear completely if the icon fails to load. Some other options exist here... If anyone thinks of something better, please let me know. https://codereview.chromium.org/775893005/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/suggestions/suggestions_result.cc:69: set_display_type(DISPLAY_LIST); Consider adding a DISPLAY_NONE to SearchResult. https://codereview.chromium.org/775893005/diff/20001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.cc (right): https://codereview.chromium.org/775893005/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.cc:53: if (!query.empty()) return; Is this CL clang-formatted? If not, please run git cl format. (This return should wrap to the next line.) https://codereview.chromium.org/775893005/diff/20001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc (right): https://codereview.chromium.org/775893005/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc:41: int64 now_usec = (base::Time::NowFromSystemTime() - base::Time::UnixEpoch()) I think there are some issues with using base::Time::Now() in tests. Maybe mgiuca@ can elaborate? https://codereview.chromium.org/775893005/diff/20001/chrome/chrome_tests_unit... File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/775893005/diff/20001/chrome/chrome_tests_unit... chrome/chrome_tests_unit.gypi:2124: 'browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc', Any particular reason for this unit test to be ash only? (Or any of these for that matter) https://codereview.chromium.org/775893005/diff/20001/ui/app_list/search/mixer.cc File ui/app_list/search/mixer.cc (right): https://codereview.chromium.org/775893005/diff/20001/ui/app_list/search/mixer... ui/app_list/search/mixer.cc:131: groups_[SUGGESTIONS_GROUP].reset(new Group(kMaxSuggestionsResults, 2.0)); Hmm. Doesn't this mean the apps_provider's results are still prioritized over the suggestions on the start page?
mgiuca@chromium.org changed reviewers: + mgiuca@chromium.org
Added a comment about use of Now() in tests. I'll have a better look at this CL on Monday. https://codereview.chromium.org/775893005/diff/20001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc (right): https://codereview.chromium.org/775893005/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc:41: int64 now_usec = (base::Time::NowFromSystemTime() - base::Time::UnixEpoch()) Well, it's generally bad to be using Now() in tests. It's not clear whether there is an actual problem here. At best, using Now() means the test will have different values coursing through it on every run, so there is potential for different behaviour (even if there shouldn't be, bugs could cause the test to pass sometimes and fail other times --- the very bugs the test is trying to guard against). But most of the time, using Now() in tests usually implies that the code under test will also be using Now(), which means there are going to be timing issues. I haven't looked at the code under test, but this test has me quite worried since you are setting an expiry time of Now() + 0.1s. This hints that if the test were to happen to take longer than 0.1s to run, it would fail. Which means it is flaky. I haven't looked deep enough into this to determine whether my suspicions are correct (it will have to wait till Monday), but I think even if there is no actual flakiness, it is still best practice to use dummy (hard-coded) time values in tests.
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Thanks, have a look! https://codereview.chromium.org/775893005/diff/20001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/search/suggestions/suggestions_result.cc (right): https://codereview.chromium.org/775893005/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/suggestions/suggestions_result.cc:34: if (index_) set_relevance(1.0 / index_); On 2014/12/05 04:01:58, calamity wrote: > label public_relevance: > I think that you should be setting the relevance in > suggestions_search_provider.cc by making set_relevance() public for this class. > > Since this class isn't intrinsically calculating its relevance, it's confusing > to work out from this class alone what index is supposed to mean. Also, there's > an implicit requirement that index be 1-based which would go away if the > relevance was set from the provider. Done. https://codereview.chromium.org/775893005/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/suggestions/suggestions_result.cc:45: ui::DispositionFromEventFlags(event_flags)); On 2014/12/05 04:01:58, calamity wrote: > So this type of result is a URL suggestion? If so, I think it would be more > informative to name the class URLSuggestionResult or similar. Done. https://codereview.chromium.org/775893005/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/suggestions/suggestions_result.cc:50: profile_, list_controller_, suggestions_service_, suggestion_, index_)); On 2014/12/05 04:01:58, calamity wrote: > If you went through with #public_relevance, you'd need to set the relevance of > the new result here too. Done. https://codereview.chromium.org/775893005/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/suggestions/suggestions_result.cc:59: } On 2014/12/05 04:01:58, calamity wrote: > Hmm. Does this mean that a blank suggestion will flicker onto the suggestions > and then vanish once it figures out it has no icon? > > If that's the case, it may be better to have the SuggestionsSearchProvider > fan-in all of these icon loads before creating any SuggestionsResults. > > The apps provider kind of gets away with it because it's loading from the disk > and the result doesn't disappear completely if the icon fails to load. > > Some other options exist here... If anyone thinks of something better, please > let me know. We actually heavily cache images to disk, and so in this version they are technically only fetched once for each of them, in which case there would be a small delay the first time. We can perhaps modify the call to GetPageThumbnail to never go to network in this path, which is what FetchSuggestionsData does. It would rather only serve from cache, and update in the background. I have a next change ready for this class that will use local favicons/large touch icons if available before looking for a fallback with |suggestions_service_|. I propose we leave this here and optimize it later with the two things I just said. https://codereview.chromium.org/775893005/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/suggestions/suggestions_result.cc:69: set_display_type(DISPLAY_LIST); On 2014/12/05 04:01:58, calamity wrote: > Consider adding a DISPLAY_NONE to SearchResult. Done. https://codereview.chromium.org/775893005/diff/20001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.cc (right): https://codereview.chromium.org/775893005/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.cc:53: if (!query.empty()) return; On 2014/12/05 04:01:58, calamity wrote: > Is this CL clang-formatted? If not, please run git cl format. (This return > should wrap to the next line.) Ran git cl format, which produces different output than the clang-format I was using. Noted. https://codereview.chromium.org/775893005/diff/20001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc (right): https://codereview.chromium.org/775893005/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc:41: int64 now_usec = (base::Time::NowFromSystemTime() - base::Time::UnixEpoch()) On 2014/12/05 06:49:06, Matt Giuca wrote: > Well, it's generally bad to be using Now() in tests. It's not clear whether > there is an actual problem here. > > At best, using Now() means the test will have different values coursing through > it on every run, so there is potential for different behaviour (even if there > shouldn't be, bugs could cause the test to pass sometimes and fail other times > --- the very bugs the test is trying to guard against). > > But most of the time, using Now() in tests usually implies that the code under > test will also be using Now(), which means there are going to be timing issues. > I haven't looked at the code under test, but this test has me quite worried > since you are setting an expiry time of Now() + 0.1s. This hints that if the > test were to happen to take longer than 0.1s to run, it would fail. Which means > it is flaky. > > I haven't looked deep enough into this to determine whether my suspicions are > correct (it will have to wait till Monday), but I think even if there is no > actual flakiness, it is still best practice to use dummy (hard-coded) time > values in tests. Hard-coded an expiry in 40 years. Not to be pessimistic, but it's likely this code won't be around then :). https://codereview.chromium.org/775893005/diff/20001/chrome/chrome_tests_unit... File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/775893005/diff/20001/chrome/chrome_tests_unit... chrome/chrome_tests_unit.gypi:2124: 'browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc', On 2014/12/05 04:01:58, calamity wrote: > Any particular reason for this unit test to be ash only? (Or any of these for > that matter) I moved suggestions_search_provider_unittest and filed a bug for the other ones. https://codereview.chromium.org/775893005/diff/20001/ui/app_list/search/mixer.cc File ui/app_list/search/mixer.cc (right): https://codereview.chromium.org/775893005/diff/20001/ui/app_list/search/mixer... ui/app_list/search/mixer.cc:131: groups_[SUGGESTIONS_GROUP].reset(new Group(kMaxSuggestionsResults, 2.0)); On 2014/12/05 04:01:58, calamity wrote: > Hmm. Doesn't this mean the apps_provider's results are still prioritized over > the suggestions on the start page? Done.
On 2014/12/05 20:14:32, Mathieu Perreault wrote: > Thanks, have a look! > > https://codereview.chromium.org/775893005/diff/20001/chrome/browser/ui/app_li... > File chrome/browser/ui/app_list/search/suggestions/suggestions_result.cc > (right): > > https://codereview.chromium.org/775893005/diff/20001/chrome/browser/ui/app_li... > chrome/browser/ui/app_list/search/suggestions/suggestions_result.cc:34: if > (index_) set_relevance(1.0 / index_); > On 2014/12/05 04:01:58, calamity wrote: > > label public_relevance: > > I think that you should be setting the relevance in > > suggestions_search_provider.cc by making set_relevance() public for this > class. > > > > Since this class isn't intrinsically calculating its relevance, it's confusing > > to work out from this class alone what index is supposed to mean. Also, > there's > > an implicit requirement that index be 1-based which would go away if the > > relevance was set from the provider. > > Done. > > https://codereview.chromium.org/775893005/diff/20001/chrome/browser/ui/app_li... > chrome/browser/ui/app_list/search/suggestions/suggestions_result.cc:45: > ui::DispositionFromEventFlags(event_flags)); > On 2014/12/05 04:01:58, calamity wrote: > > So this type of result is a URL suggestion? If so, I think it would be more > > informative to name the class URLSuggestionResult or similar. > > Done. > > https://codereview.chromium.org/775893005/diff/20001/chrome/browser/ui/app_li... > chrome/browser/ui/app_list/search/suggestions/suggestions_result.cc:50: > profile_, list_controller_, suggestions_service_, suggestion_, index_)); > On 2014/12/05 04:01:58, calamity wrote: > > If you went through with #public_relevance, you'd need to set the relevance of > > the new result here too. > > Done. > > https://codereview.chromium.org/775893005/diff/20001/chrome/browser/ui/app_li... > chrome/browser/ui/app_list/search/suggestions/suggestions_result.cc:59: } > On 2014/12/05 04:01:58, calamity wrote: > > Hmm. Does this mean that a blank suggestion will flicker onto the suggestions > > and then vanish once it figures out it has no icon? > > > > If that's the case, it may be better to have the SuggestionsSearchProvider > > fan-in all of these icon loads before creating any SuggestionsResults. > > > > The apps provider kind of gets away with it because it's loading from the disk > > and the result doesn't disappear completely if the icon fails to load. > > > > Some other options exist here... If anyone thinks of something better, please > > let me know. > > We actually heavily cache images to disk, and so in this version they are > technically only fetched once for each of them, in which case there would be a > small delay the first time. We can perhaps modify the call to GetPageThumbnail > to never go to network in this path, which is what FetchSuggestionsData does. It > would rather only serve from cache, and update in the background. > > I have a next change ready for this class that will use local favicons/large > touch icons if available before looking for a fallback with > |suggestions_service_|. > > I propose we leave this here and optimize it later with the two things I just > said. > > https://codereview.chromium.org/775893005/diff/20001/chrome/browser/ui/app_li... > chrome/browser/ui/app_list/search/suggestions/suggestions_result.cc:69: > set_display_type(DISPLAY_LIST); > On 2014/12/05 04:01:58, calamity wrote: > > Consider adding a DISPLAY_NONE to SearchResult. > > Done. > > https://codereview.chromium.org/775893005/diff/20001/chrome/browser/ui/app_li... > File > chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.cc > (right): > > https://codereview.chromium.org/775893005/diff/20001/chrome/browser/ui/app_li... > chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.cc:53: > if (!query.empty()) return; > On 2014/12/05 04:01:58, calamity wrote: > > Is this CL clang-formatted? If not, please run git cl format. (This return > > should wrap to the next line.) > > Ran git cl format, which produces different output than the clang-format I was > using. Noted. > > https://codereview.chromium.org/775893005/diff/20001/chrome/browser/ui/app_li... > File > chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc > (right): > > https://codereview.chromium.org/775893005/diff/20001/chrome/browser/ui/app_li... > chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc:41: > int64 now_usec = (base::Time::NowFromSystemTime() - base::Time::UnixEpoch()) > On 2014/12/05 06:49:06, Matt Giuca wrote: > > Well, it's generally bad to be using Now() in tests. It's not clear whether > > there is an actual problem here. > > > > At best, using Now() means the test will have different values coursing > through > > it on every run, so there is potential for different behaviour (even if there > > shouldn't be, bugs could cause the test to pass sometimes and fail other times > > --- the very bugs the test is trying to guard against). > > > > But most of the time, using Now() in tests usually implies that the code under > > test will also be using Now(), which means there are going to be timing > issues. > > I haven't looked at the code under test, but this test has me quite worried > > since you are setting an expiry time of Now() + 0.1s. This hints that if the > > test were to happen to take longer than 0.1s to run, it would fail. Which > means > > it is flaky. > > > > I haven't looked deep enough into this to determine whether my suspicions are > > correct (it will have to wait till Monday), but I think even if there is no > > actual flakiness, it is still best practice to use dummy (hard-coded) time > > values in tests. > > Hard-coded an expiry in 40 years. Not to be pessimistic, but it's likely this > code won't be around then :). > > https://codereview.chromium.org/775893005/diff/20001/chrome/chrome_tests_unit... > File chrome/chrome_tests_unit.gypi (right): > > https://codereview.chromium.org/775893005/diff/20001/chrome/chrome_tests_unit... > chrome/chrome_tests_unit.gypi:2124: > 'browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc', > On 2014/12/05 04:01:58, calamity wrote: > > Any particular reason for this unit test to be ash only? (Or any of these for > > that matter) > > I moved suggestions_search_provider_unittest and filed a bug for the other ones. > > https://codereview.chromium.org/775893005/diff/20001/ui/app_list/search/mixer.cc > File ui/app_list/search/mixer.cc (right): > > https://codereview.chromium.org/775893005/diff/20001/ui/app_list/search/mixer... > ui/app_list/search/mixer.cc:131: groups_[SUGGESTIONS_GROUP].reset(new > Group(kMaxSuggestionsResults, 2.0)); > On 2014/12/05 04:01:58, calamity wrote: > > Hmm. Doesn't this mean the apps_provider's results are still prioritized over > > the suggestions on the start page? > > Done. Friendly ping.
histograms lgtm, didn't look at anything else
Hi Mathieu, Thanks for your patience and sorry I didn't get back to you sooner. I have a number of comments or questions, but don't be alarmed by the size of my review: I think all of these are pretty minor and would be happy to submit this with a few minor changes. https://codereview.chromium.org/775893005/diff/20001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc (right): https://codereview.chromium.org/775893005/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc:41: int64 now_usec = (base::Time::NowFromSystemTime() - base::Time::UnixEpoch()) OK, that's better (I think), but it still means that the code under test is using Now(), which is not ideal. (In theory, code being tested should be running the same data on every test run. What if, for example, there was a bug that only occurs on Wednesdays? Then the test would flake out on Wednesdays, whereas if you can force the code to think the time is a fixed value, then it will either always pass or always fail.) I have filed a bug for this (http://crbug.com/440252) and made a suggestion about how to fix SuggestionStore. If that fix is taken, it should be very easy for this test to call SuggestionStore::SetNowUsecForTest(), and make the tests fully deterministic. I will leave it up to you whether you want to do that before this CL, or just commit this first and fix it up later. It isn't a big deal and I think now that you've hard-coded a 40-year expiry time, it is fine to submit as-is. But please add a TODO mentioning the bug number I filed. (And you should make sure you get around to it some time within the next 40 years :p). https://codereview.chromium.org/775893005/diff/100001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/search_controller_factory.cc (right): https://codereview.chromium.org/775893005/diff/100001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/search_controller_factory.cc:31: bool IsSuggestionsSearchProviderEnabled() { I want to be absolutely sure that this code will return FALSE by default (so this CL will have no effect on ordinary users). I assume that a given field trial will NOT be in the field trial list by default, so this will be true, but I wanted to make sure with you. https://codereview.chromium.org/775893005/diff/100001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/search_controller_factory.cc:32: return StartsWithASCII( I'm not familiar with field trials (maybe this is normal), but why do you check if it StartsWith as opposed to just being equal? https://codereview.chromium.org/775893005/diff/100001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.cc (right): https://codereview.chromium.org/775893005/diff/100001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.cc:52: // Service not enabled, do not return any results. nit: Move this comment above the if statement and remove the braces. (Reword it a little: "If the service is not enabled...") https://codereview.chromium.org/775893005/diff/100001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.cc:60: // Suggestions service is enabled, initiate a query. Supernit: ',' should be ';'. https://codereview.chromium.org/775893005/diff/100001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.cc:80: Add(result.Pass()); Is there any chance this gets called more than once? Because in that case, should it clear the existing results set? https://codereview.chromium.org/775893005/diff/100001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.h (right): https://codereview.chromium.org/775893005/diff/100001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.h:33: void OnSuggestionsProfileAvailable( Comment here. https://codereview.chromium.org/775893005/diff/100001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc (right): https://codereview.chromium.org/775893005/diff/100001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc:60: // Under test. What does this comment mean? https://codereview.chromium.org/775893005/diff/100001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.h (right): https://codereview.chromium.org/775893005/diff/100001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.h:47: // For callbacks may be run after destruction. "callbacks *that* may be ..."? https://codereview.chromium.org/775893005/diff/100001/ui/app_list/search/mixe... File ui/app_list/search/mixer.cc (right): https://codereview.chromium.org/775893005/diff/100001/ui/app_list/search/mixe... ui/app_list/search/mixer.cc:127: groups_[SUGGESTIONS_GROUP].reset(new Group(kMaxSuggestionsResults, 4.0)); Why is this rated so highly? If anything it could just have 3.0 and then be rated alongside apps in the MAIN_GROUP. https://codereview.chromium.org/775893005/diff/100001/ui/app_list/search_resu... File ui/app_list/search_result.h (right): https://codereview.chromium.org/775893005/diff/100001/ui/app_list/search_resu... ui/app_list/search_result.h:35: DISPLAY_NONE, There are tests for search result pages with both of the other types. DISPLAY_NONE should also be tested (to check that it DOES NOT appear). Please update SearchResultPageViewTest.Basic, adding a few DISPLAY_NONE entries, and test that they do not appear.
Oh, please file a bug for this feature if you have not already, and add it to the BUG= line.
I think everything's addressed, thanks! https://codereview.chromium.org/775893005/diff/20001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc (right): https://codereview.chromium.org/775893005/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc:41: int64 now_usec = (base::Time::NowFromSystemTime() - base::Time::UnixEpoch()) On 2014/12/09 05:34:54, Matt Giuca wrote: > OK, that's better (I think), but it still means that the code under test is > using Now(), which is not ideal. (In theory, code being tested should be running > the same data on every test run. What if, for example, there was a bug that only > occurs on Wednesdays? Then the test would flake out on Wednesdays, whereas if > you can force the code to think the time is a fixed value, then it will either > always pass or always fail.) > > I have filed a bug for this (http://crbug.com/440252) and made a suggestion > about how to fix SuggestionStore. If that fix is taken, it should be very easy > for this test to call SuggestionStore::SetNowUsecForTest(), and make the tests > fully deterministic. > > I will leave it up to you whether you want to do that before this CL, or just > commit this first and fix it up later. It isn't a big deal and I think now that > you've hard-coded a 40-year expiry time, it is fine to submit as-is. But please > add a TODO mentioning the bug number I filed. (And you should make sure you get > around to it some time within the next 40 years :p). Appreciate the filed bugs. Will do it soon, but not now. https://codereview.chromium.org/775893005/diff/100001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/search_controller_factory.cc (right): https://codereview.chromium.org/775893005/diff/100001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/search_controller_factory.cc:31: bool IsSuggestionsSearchProviderEnabled() { On 2014/12/09 05:34:54, Matt Giuca wrote: > I want to be absolutely sure that this code will return FALSE by default (so > this CL will have no effect on ordinary users). I assume that a given field > trial will NOT be in the field trial list by default, so this will be true, but > I wanted to make sure with you. Yes, you can be sure of that. FindFullName returns the name of the group that the user is in if the trial is active, or empty string. We check here that the group name starts with "Enabled", which can't happen until we put people in that trial. It's safe and been used before. https://codereview.chromium.org/775893005/diff/100001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/search_controller_factory.cc:32: return StartsWithASCII( On 2014/12/09 05:34:54, Matt Giuca wrote: > I'm not familiar with field trials (maybe this is normal), but why do you check > if it StartsWith as opposed to just being equal? If we choose so, we can have a couple of groups called "Enabled", "Enabled_forced", "Enabled_v2", to track different branches of the trial. It gives us more flexibility when elaborating the trial. https://codereview.chromium.org/775893005/diff/100001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.cc (right): https://codereview.chromium.org/775893005/diff/100001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.cc:52: // Service not enabled, do not return any results. On 2014/12/09 05:34:54, Matt Giuca wrote: > nit: Move this comment above the if statement and remove the braces. (Reword it > a little: "If the service is not enabled...") Done. https://codereview.chromium.org/775893005/diff/100001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.cc:60: // Suggestions service is enabled, initiate a query. On 2014/12/09 05:34:54, Matt Giuca wrote: > Supernit: ',' should be ';'. Done. https://codereview.chromium.org/775893005/diff/100001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.cc:80: Add(result.Pass()); On 2014/12/09 05:34:54, Matt Giuca wrote: > Is there any chance this gets called more than once? Because in that case, > should it clear the existing results set? There is technically "no chance" because in SuggestionsService parlance, as soon as the requestors are served they are removed from the list of requestors and can't be called again. https://codereview.chromium.org/775893005/diff/100001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.h (right): https://codereview.chromium.org/775893005/diff/100001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.h:33: void OnSuggestionsProfileAvailable( On 2014/12/09 05:34:54, Matt Giuca wrote: > Comment here. Done. https://codereview.chromium.org/775893005/diff/100001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc (right): https://codereview.chromium.org/775893005/diff/100001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc:60: // Under test. On 2014/12/09 05:34:54, Matt Giuca wrote: > What does this comment mean? I removed it. https://codereview.chromium.org/775893005/diff/100001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.h (right): https://codereview.chromium.org/775893005/diff/100001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.h:47: // For callbacks may be run after destruction. On 2014/12/09 05:34:54, Matt Giuca wrote: > "callbacks *that* may be ..."? Done. https://codereview.chromium.org/775893005/diff/100001/ui/app_list/search/mixe... File ui/app_list/search/mixer.cc (right): https://codereview.chromium.org/775893005/diff/100001/ui/app_list/search/mixe... ui/app_list/search/mixer.cc:127: groups_[SUGGESTIONS_GROUP].reset(new Group(kMaxSuggestionsResults, 4.0)); On 2014/12/09 05:34:55, Matt Giuca wrote: > Why is this rated so highly? If anything it could just have 3.0 and then be > rated alongside apps in the MAIN_GROUP. This can be re-adjusted later through experimentation, but 3.0 sounds good. https://codereview.chromium.org/775893005/diff/100001/ui/app_list/search_resu... File ui/app_list/search_result.h (right): https://codereview.chromium.org/775893005/diff/100001/ui/app_list/search_resu... ui/app_list/search_result.h:35: DISPLAY_NONE, On 2014/12/09 05:34:55, Matt Giuca wrote: > There are tests for search result pages with both of the other types. > DISPLAY_NONE should also be tested (to check that it DOES NOT appear). > > Please update SearchResultPageViewTest.Basic, adding a few DISPLAY_NONE entries, > and test that they do not appear. I added a few DISPLAY_NONE entries and the test still passes.
lgtm https://codereview.chromium.org/775893005/diff/100001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.cc (right): https://codereview.chromium.org/775893005/diff/100001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.cc:80: Add(result.Pass()); On 2014/12/09 21:18:26, Mathieu Perreault wrote: > On 2014/12/09 05:34:54, Matt Giuca wrote: > > Is there any chance this gets called more than once? Because in that case, > > should it clear the existing results set? > > There is technically "no chance" because in SuggestionsService parlance, as soon > as the requestors are served they are removed from the list of requestors and > can't be called again. Acknowledged.
lgtm https://codereview.chromium.org/775893005/diff/20001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/search/suggestions/suggestions_result.cc (right): https://codereview.chromium.org/775893005/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/suggestions/suggestions_result.cc:59: } On 2014/12/05 20:14:32, Mathieu Perreault wrote: > On 2014/12/05 04:01:58, calamity wrote: > > Hmm. Does this mean that a blank suggestion will flicker onto the suggestions > > and then vanish once it figures out it has no icon? > > > > If that's the case, it may be better to have the SuggestionsSearchProvider > > fan-in all of these icon loads before creating any SuggestionsResults. > > > > The apps provider kind of gets away with it because it's loading from the disk > > and the result doesn't disappear completely if the icon fails to load. > > > > Some other options exist here... If anyone thinks of something better, please > > let me know. > > We actually heavily cache images to disk, and so in this version they are > technically only fetched once for each of them, in which case there would be a > small delay the first time. We can perhaps modify the call to GetPageThumbnail > to never go to network in this path, which is what FetchSuggestionsData does. It > would rather only serve from cache, and update in the background. > > I have a next change ready for this class that will use local favicons/large > touch icons if available before looking for a fallback with > |suggestions_service_|. > Sweet. > I propose we leave this here and optimize it later with the two things I just > said. Since this is behind an experiment, I'm ok with this. https://codereview.chromium.org/775893005/diff/20001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.cc (right): https://codereview.chromium.org/775893005/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.cc:53: if (!query.empty()) return; On 2014/12/05 20:14:32, Mathieu Perreault wrote: > On 2014/12/05 04:01:58, calamity wrote: > > Is this CL clang-formatted? If not, please run git cl format. (This return > > should wrap to the next line.) > > Ran git cl format, which produces different output than the clang-format I was > using. Noted. Yeah... I think there are special options if you want to use vanilla clang-format (--style=Chromium in particular) https://codereview.chromium.org/775893005/diff/20001/chrome/chrome_tests_unit... File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/775893005/diff/20001/chrome/chrome_tests_unit... chrome/chrome_tests_unit.gypi:2124: 'browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc', On 2014/12/05 20:14:32, Mathieu Perreault wrote: > On 2014/12/05 04:01:58, calamity wrote: > > Any particular reason for this unit test to be ash only? (Or any of these for > > that matter) > > I moved suggestions_search_provider_unittest and filed a bug for the other ones. Acknowledged. https://codereview.chromium.org/775893005/diff/120001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc (right): https://codereview.chromium.org/775893005/diff/120001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc:43: suggestion->set_expiry_ts(kSuggestionExpiry); Consider using base::Time::Max().ToInternalValue().
Thanks for putting this together, Mathieu. Looking forward to trying it!
LGTM with nitty comments. https://codereview.chromium.org/775893005/diff/120001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.cc (right): https://codereview.chromium.org/775893005/diff/120001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.cc:76: scoped_ptr<URLSuggestionResult> result(new URLSuggestionResult( base/memory/scoped_ptr.h was never included in this or the .h file. And I presume you're not using std::unique_ptr<> to conform with app_list usage? https://codereview.chromium.org/775893005/diff/120001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.cc:78: result->set_relevance(1.0 / (i + 1)); Some comment on the formula 1.0 / (i + 1). If you just want the first to be most relevant, why not just size - i? https://codereview.chromium.org/775893005/diff/120001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.h (right): https://codereview.chromium.org/775893005/diff/120001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.h:8: #include "base/basictypes.h" DISALLOW_COPY_AND_ASSIGN is in base/macros.h https://codereview.chromium.org/775893005/diff/120001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc (right): https://codereview.chromium.org/775893005/diff/120001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc:39: suggestion->set_url(title); url and title got flipped. https://codereview.chromium.org/775893005/diff/120001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.h (right): https://codereview.chromium.org/775893005/diff/120001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.h:25: URLSuggestionResult(Profile* profile, Comment on why are you copying |suggestions|; is it expected to not live long? https://codereview.chromium.org/775893005/diff/120001/ui/app_list/search/mixe... File ui/app_list/search/mixer.cc (right): https://codereview.chromium.org/775893005/diff/120001/ui/app_list/search/mixe... ui/app_list/search/mixer.cc:127: groups_[SUGGESTIONS_GROUP].reset(new Group(kMaxSuggestionsResults, 3.0)); NIT: add this one last, to preserve GroupId ordering.
Addressed everything, will submit. https://codereview.chromium.org/775893005/diff/120001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.cc (right): https://codereview.chromium.org/775893005/diff/120001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.cc:76: scoped_ptr<URLSuggestionResult> result(new URLSuggestionResult( On 2014/12/10 07:14:04, huangs1 wrote: > base/memory/scoped_ptr.h was never included in this or the .h file. > > And I presume you're not using std::unique_ptr<> to conform with app_list usage? Done. Yes it's for the API. https://codereview.chromium.org/775893005/diff/120001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.cc:78: result->set_relevance(1.0 / (i + 1)); On 2014/12/10 07:14:04, huangs1 wrote: > Some comment on the formula 1.0 / (i + 1). If you just want the first to be > most relevant, why not just size - i? Relevance is between [0.0, 1.0] https://codereview.chromium.org/775893005/diff/120001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.h (right): https://codereview.chromium.org/775893005/diff/120001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.h:8: #include "base/basictypes.h" On 2014/12/10 07:14:04, huangs1 wrote: > DISALLOW_COPY_AND_ASSIGN is in base/macros.h Done. https://codereview.chromium.org/775893005/diff/120001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc (right): https://codereview.chromium.org/775893005/diff/120001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc:39: suggestion->set_url(title); On 2014/12/10 07:14:04, huangs1 wrote: > url and title got flipped. Done. https://codereview.chromium.org/775893005/diff/120001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider_unittest.cc:43: suggestion->set_expiry_ts(kSuggestionExpiry); On 2014/12/10 01:47:43, calamity wrote: > Consider using base::Time::Max().ToInternalValue(). Done. https://codereview.chromium.org/775893005/diff/120001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.h (right): https://codereview.chromium.org/775893005/diff/120001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.h:25: URLSuggestionResult(Profile* profile, On 2014/12/10 07:14:04, huangs1 wrote: > Comment on why are you copying |suggestions|; is it expected to not live long? Done. https://codereview.chromium.org/775893005/diff/120001/ui/app_list/search/mixe... File ui/app_list/search/mixer.cc (right): https://codereview.chromium.org/775893005/diff/120001/ui/app_list/search/mixe... ui/app_list/search/mixer.cc:127: groups_[SUGGESTIONS_GROUP].reset(new Group(kMaxSuggestionsResults, 3.0)); On 2014/12/10 07:14:04, huangs1 wrote: > NIT: add this one last, to preserve GroupId ordering. Done.
The CQ bit was checked by mathp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/775893005/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mathp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/775893005/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mathp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/775893005/160001
Message was sent while issue was closed.
Committed patchset #6 (id:160001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/04ef4a4d6e9303da91f432821d9601d49bdb6c02 Cr-Commit-Position: refs/heads/master@{#307761} |
