Allow AppSearchProvider to provide recommendations.

Allow AppSearchProvider to provide recommendations.

This CL adds a FetchRecommendations to the SearchProvider interface and
the AppListViewDelegate in preparation for hosting these recommendations in
the start page tiles.

Currently, the only recommendation provider is the AppSearchProvider which provides
the most recently used apps.

BUG=349727
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285885

[Matt Giuca, 2014-07-22 04:31:08]

https://codereview.chromium.org/379333005/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/app_result.cc (right):

https://codereview.chromium.org/379333005/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/app_result.cc:84: set_relevance(1 / (1 +
delta.InSecondsF() / kSecondsInWeek));
Why does the relevance have to be a float? How ingrained is this in the system?
It would be a lot simpler if the relevance could simply be a 64-bit integer.
Then you could just use last_launched.ToInternalValue() as the relevance (higher
is better), and not have to deal with Time::Now(), weeks, floats, or
reciprocals. (See my concerns later in the tests.)

https://codereview.chromium.org/379333005/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/app_search_provider.cc (right):

https://codereview.chromium.org/379333005/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/app_search_provider.cc:66: // Reload apps to
ensure we have the latest launch time information.
nit: Don't use the word "reload"; it implies you are actually going to simulate
clicking the "Reload" button on all the apps (which we certainly don't want
every time we type a letter).

s/Reload/Refresh list of/

https://codereview.chromium.org/379333005/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/app_search_provider_unittest.cc (right):

https://codereview.chromium.org/379333005/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/app_search_provider_unittest.cc:106:
prefs->SetLastLaunchTime(kHostedAppId, base::Time::FromInternalValue(20));
This is sort of funny in terms of nondeterminism.

Your tests are using base::Time::Now(), which is probably not good for tests.
What you're saying is "kHostedAppId was last launched at 01 Jan 1601 00:00:20"*,
and then comparing it to Time::Now(). So every time the test runs, you will
compute slightly different values, even though they should all come out in the
same order. (Unless one day in the future, the 400+ years since the apps were
launched causes two float values to alias to the same value.)

*The More You Know: Chrome's base::Time values start in 1601, not 1970, because
apparently that's Windows' epoch.

I think I would be more comfortable if the tests did not use Time::Now(). Have
FetchRecommendations take an optional base::Time for the current time (hopefully
you can just update the class this test is calling and not all of the many base
classes).

Alternatively, set the last launch time to base::Time::Now() - 10, - 20, etc, so
the time deltas are relatively small and constant.

https://codereview.chromium.org/379333005/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/search_controller.cc (right):

https://codereview.chromium.org/379333005/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/search_controller.cc:133: const int
kStopTimeMS = 1500;
nit: This constant should be more visible -- at least at the top of this file,
and perhaps in app_list_constants.

https://codereview.chromium.org/379333005/diff/40001/ui/app_list/search_provi...
File ui/app_list/search_provider.h (right):

https://codereview.chromium.org/379333005/diff/40001/ui/app_list/search_provi...
ui/app_list/search_provider.h:28: virtual void FetchRecommendations();
I'm wondering if it's simpler to just make Start("") fetch recommendations. It
would eliminate a lot of methods and a little bit of code. It looks like the two
operations are more similar than different, but maybe you object.

[calamity, 2014-07-22 07:24:04]

https://codereview.chromium.org/379333005/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/app_result.cc (right):

https://codereview.chromium.org/379333005/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/app_result.cc:84: set_relevance(1 / (1 +
delta.InSecondsF() / kSecondsInWeek));
On 2014/07/22 04:31:07, Matt Giuca wrote:
> Why does the relevance have to be a float? How ingrained is this in the
system?
> It would be a lot simpler if the relevance could simply be a 64-bit integer.
> Then you could just use last_launched.ToInternalValue() as the relevance
(higher
> is better), and not have to deal with Time::Now(), weeks, floats, or
> reciprocals. (See my concerns later in the tests.)

Very. Boost values are +1-4 which rely on the base relevance to be 0-1 so all
other providers give relevances in the range 0-1.

https://codereview.chromium.org/379333005/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/app_search_provider.cc (right):

https://codereview.chromium.org/379333005/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/app_search_provider.cc:66: // Reload apps to
ensure we have the latest launch time information.
On 2014/07/22 04:31:07, Matt Giuca wrote:
> nit: Don't use the word "reload"; it implies you are actually going to
simulate
> clicking the "Reload" button on all the apps (which we certainly don't want
> every time we type a letter).
> 
> s/Reload/Refresh list of/

Done.

https://codereview.chromium.org/379333005/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/app_search_provider_unittest.cc (right):

https://codereview.chromium.org/379333005/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/app_search_provider_unittest.cc:106:
prefs->SetLastLaunchTime(kHostedAppId, base::Time::FromInternalValue(20));
On 2014/07/22 04:31:07, Matt Giuca wrote:
> This is sort of funny in terms of nondeterminism.
> 
> Your tests are using base::Time::Now(), which is probably not good for tests.
> What you're saying is "kHostedAppId was last launched at 01 Jan 1601
00:00:20"*,
> and then comparing it to Time::Now(). So every time the test runs, you will
> compute slightly different values, even though they should all come out in the
> same order. (Unless one day in the future, the 400+ years since the apps were
> launched causes two float values to alias to the same value.)
> 
> *The More You Know: Chrome's base::Time values start in 1601, not 1970,
because
> apparently that's Windows' epoch.
> 
> I think I would be more comfortable if the tests did not use Time::Now(). Have
> FetchRecommendations take an optional base::Time for the current time
(hopefully
> you can just update the class this test is calling and not all of the many
base
> classes).
> 
> Alternatively, set the last launch time to base::Time::Now() - 10, - 20, etc,
so
> the time deltas are relatively small and constant.

Done.

https://codereview.chromium.org/379333005/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/search_controller.cc (right):

https://codereview.chromium.org/379333005/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/search_controller.cc:133: const int
kStopTimeMS = 1500;
On 2014/07/22 04:31:07, Matt Giuca wrote:
> nit: This constant should be more visible -- at least at the top of this file,
> and perhaps in app_list_constants.

Done.

https://codereview.chromium.org/379333005/diff/40001/ui/app_list/search_provi...
File ui/app_list/search_provider.h (right):

https://codereview.chromium.org/379333005/diff/40001/ui/app_list/search_provi...
ui/app_list/search_provider.h:28: virtual void FetchRecommendations();
On 2014/07/22 04:31:08, Matt Giuca wrote:
> I'm wondering if it's simpler to just make Start("") fetch recommendations. It
> would eliminate a lot of methods and a little bit of code. It looks like the
two
> operations are more similar than different, but maybe you object.

I agree. I walked in thinking this would be cleaner but looking now, it seems
unnecessary.

[Matt Giuca, 2014-07-23 01:53:19]

Looking good. Some questions.

https://codereview.chromium.org/379333005/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/app_result.cc (right):

https://codereview.chromium.org/379333005/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/app_result.cc:84: set_relevance(1 / (1 +
delta.InSecondsF() / kSecondsInWeek));
On 2014/07/22 07:24:04, calamity wrote:
> On 2014/07/22 04:31:07, Matt Giuca wrote:
> > Why does the relevance have to be a float? How ingrained is this in the
> system?
> > It would be a lot simpler if the relevance could simply be a 64-bit integer.
> > Then you could just use last_launched.ToInternalValue() as the relevance
> (higher
> > is better), and not have to deal with Time::Now(), weeks, floats, or
> > reciprocals. (See my concerns later in the tests.)
> 
> Very. Boost values are +1-4 which rely on the base relevance to be 0-1 so all
> other providers give relevances in the range 0-1.

Acknowledged.

https://codereview.chromium.org/379333005/diff/80001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/app_search_provider.cc (right):

https://codereview.chromium.org/379333005/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/app_search_provider.cc:33:
last_launch_time_(last_launch_time) {}
nit: Is this ClangFormatted? I believe you should have a newline in between the
braces (but trust git cl format over me).

https://codereview.chromium.org/379333005/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/app_search_provider.cc:40: static bool
AppLaunchedMoreRecent(App* const app1, App* const app2) {
"App* const"? Did you mean "const App*"?

("App* const" means an unmodifiable pointer to a non-constant App -- essentially
the same as writing "const int x", and not really helpful, unless std::sort
requires it, which I doubt.)

https://codereview.chromium.org/379333005/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/app_search_provider.cc:124:
std::sort(apps_.begin(), apps_.end(), &App::AppLaunchedMoreRecent);
Question: Does this actually need to be sorted here? I thought the search
provider system would automatically order results by their "relevance" score
(which is indirectly based on the apps' last launch time). So why do you also
need to directly sort on last launch time?

https://codereview.chromium.org/379333005/diff/80001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/app_search_provider.h (right):

https://codereview.chromium.org/379333005/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/app_search_provider.h:22: namespace app_list {
nit: blank line after this.

[calamity, 2014-07-28 02:55:27]

https://codereview.chromium.org/379333005/diff/80001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/app_search_provider.cc (right):

https://codereview.chromium.org/379333005/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/app_search_provider.cc:33:
last_launch_time_(last_launch_time) {}
On 2014/07/23 01:53:18, Matt Giuca wrote:
> nit: Is this ClangFormatted? I believe you should have a newline in between
the
> braces (but trust git cl format over me).

Yeah, git cl format wants this apparently.

https://codereview.chromium.org/379333005/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/app_search_provider.cc:40: static bool
AppLaunchedMoreRecent(App* const app1, App* const app2) {
On 2014/07/23 01:53:18, Matt Giuca wrote:
> "App* const"? Did you mean "const App*"?
> 
> ("App* const" means an unmodifiable pointer to a non-constant App --
essentially
> the same as writing "const int x", and not really helpful, unless std::sort
> requires it, which I doubt.)

Removed.

https://codereview.chromium.org/379333005/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/app_search_provider.cc:124:
std::sort(apps_.begin(), apps_.end(), &App::AppLaunchedMoreRecent);
On 2014/07/23 01:53:18, Matt Giuca wrote:
> Question: Does this actually need to be sorted here? I thought the search
> provider system would automatically order results by their "relevance" score
> (which is indirectly based on the apps' last launch time). So why do you also
> need to directly sort on last launch time?

You are indeed correct. This should have been removed. Made it so.

https://codereview.chromium.org/379333005/diff/80001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/app_search_provider.h (right):

https://codereview.chromium.org/379333005/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/app_search_provider.h:22: namespace app_list {
On 2014/07/23 01:53:19, Matt Giuca wrote:
> nit: blank line after this.

Done.

[Matt Giuca, 2014-07-28 02:59:35]

lgtm

https://codereview.chromium.org/379333005/diff/80001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/app_search_provider.cc (right):

https://codereview.chromium.org/379333005/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/app_search_provider.cc:33:
last_launch_time_(last_launch_time) {}
On 2014/07/28 02:55:26, calamity wrote:
> On 2014/07/23 01:53:18, Matt Giuca wrote:
> > nit: Is this ClangFormatted? I believe you should have a newline in between
> the
> > braces (but trust git cl format over me).
> 
> Yeah, git cl format wants this apparently.

Acknowledged.

[calamity, 2014-07-28 04:20:57]

Changed the test to sort by relevance as discussed.

[calamity, 2014-07-28 06:43:07]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-07-28 06:43:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/calamity@chromium.org/379333005/150001

[commit-bot: I haz the power, 2014-07-28 10:36:17]

Change committed as 285885