app_list: Search result launch history and boost.

app_list: Search result launch history and boost.

- Add a History PKS for search history recording and serving;
- Add a HistoryData to maintain an in-memory data structure for serving
  History request. It remembers search result launches and associates
  them with the query being used and also provides hint for future queries
  using the remembered data;
- Add a HistoryDataStore to persist data;

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

[xiyuan, 2013-05-23 17:01:47]

I have also updated the document to include more details of from the CL. The
basic idea is still the same: We remember query-launched_result associations.
The first association becomes the primary for that query and the succeeding ones
become the secondaries. Exactly the same two secondaries in a row promotes it to
be the primary. And remembered results gets boosted scores in the ixer.

For data storaage, instead of using a db, this CL just uses a json file and cap
the entries to 1000 (hopefully that's big enough for most of the users).

Please take a look. Thanks.

[James Cook, 2013-05-23 21:51:08]

I'm only partway through this, but need to stop for a little while. Looking good
so far.  Some early comments/questions below.

https://codereview.chromium.org/15875007/diff/9001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/search/history.cc (right):

https://codereview.chromium.org/15875007/diff/9001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/search/history.cc:30: const char kStoreDataFileName[]
= "App Launcher Search";
nit: Consider giving the file an extension, like .json or .txt

https://codereview.chromium.org/15875007/diff/9001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/search/history_data.cc (right):

https://codereview.chromium.org/15875007/diff/9001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/search/history_data.cc:104: KnownResults&
known_results = *results.get();
This feels a little strange. Can you just use results-> and (*results)?

https://codereview.chromium.org/15875007/diff/9001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/search/history_data.h (right):

https://codereview.chromium.org/15875007/diff/9001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/search/history_data.h:36: // Defines data to be
associated with a query.
nit: extra line above might be more readable

https://codereview.chromium.org/15875007/diff/9001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/search/history_data.h:55:
HistoryData(HistoryDataStore* store, size_t max_entry, size_t max_secondary);
Maybe comment that max_entry is total entries, including secondary entries?  I
found it hard to understand the limits here.  Or call |max_entry| as
|max_primary|?

https://codereview.chromium.org/15875007/diff/9001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/search/history_data_store.cc (right):

https://codereview.chromium.org/15875007/diff/9001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/search/history_data_store.cc:23: const char
kKeyVersion[] = "version";
I'm glad you're including a file format version number.

https://codereview.chromium.org/15875007/diff/9001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/search/history_data_store.cc:56: //               
"result id of the latest secondary association"
nit: I would say "newest" instead of "latest" because I get confused about
"late" vs. "old"-est. :-)

https://codereview.chromium.org/15875007/diff/9001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/search/history_data_store.cc:131:
Flush(OnFlushedCallback());
Will this force this data store to be written on shutdown? Could we possibly
avoid doing that?  I would prefer to lose one or two associations at shutdown to
adding another blocking write.

https://codereview.chromium.org/15875007/diff/9001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/search/history_data_store.cc:212:
DCHECK(BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
I like thread assertions.

https://codereview.chromium.org/15875007/diff/9001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/search/history_data_store_unittest.cc (right):

https://codereview.chromium.org/15875007/diff/9001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/search/history_data_store_unittest.cc:18: namespace
test {
nit: I don't usually put individual tests inside the test namespace, only
utilities used for multiple tests.  If it makes the code cleaner, then this is
fine, but I don't think it's always needed.

https://codereview.chromium.org/15875007/diff/9001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/search/history_data_store_unittest.cc:64: Load();
Do you have to reload it after a flush?  It might be more clear to call Flush()
then explicitly call Load() in the test below.

https://codereview.chromium.org/15875007/diff/9001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/search/history_unittest.cc (right):

https://codereview.chromium.org/15875007/diff/9001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/search/history_unittest.cc:33: void Wait(int
wait_time_ms) {
This is not waiting in the normal case, it's just a timeout if things take too
long?  You should comment that if so.  A class comment might be good.

https://codereview.chromium.org/15875007/diff/9001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/search/history_unittest.cc:69: void Wait(int
wait_time_ms) {
ditto

https://codereview.chromium.org/15875007/diff/9001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/search/history_unittest.cc:91: HistoryDataStore*
store_;  // Not owned.
I like ownership comments!

[xiyuan, 2013-05-23 22:38:13]

https://codereview.chromium.org/15875007/diff/9001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/search/history.cc (right):

https://codereview.chromium.org/15875007/diff/9001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/search/history.cc:30: const char kStoreDataFileName[]
= "App Launcher Search";
On 2013/05/23 21:51:08, James Cook (Chromium) wrote:
> nit: Consider giving the file an extension, like .json or .txt

I did not add an extension because all files under user data dir does not have
one. It might be better to be consistent. What do you think?

https://codereview.chromium.org/15875007/diff/9001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/search/history_data.cc (right):

https://codereview.chromium.org/15875007/diff/9001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/search/history_data.cc:104: KnownResults&
known_results = *results.get();
On 2013/05/23 21:51:08, James Cook (Chromium) wrote:
> This feels a little strange. Can you just use results-> and (*results)?

Done.

https://codereview.chromium.org/15875007/diff/9001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/search/history_data.h (right):

https://codereview.chromium.org/15875007/diff/9001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/search/history_data.h:36: // Defines data to be
associated with a query.
On 2013/05/23 21:51:08, James Cook (Chromium) wrote:
> nit: extra line above might be more readable

Done.

https://codereview.chromium.org/15875007/diff/9001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/search/history_data.h:55:
HistoryData(HistoryDataStore* store, size_t max_entry, size_t max_secondary);
On 2013/05/23 21:51:08, James Cook (Chromium) wrote:
> Maybe comment that max_entry is total entries, including secondary entries?  I
> found it hard to understand the limits here.  Or call |max_entry| as
> |max_primary|?

Renamed max_entry to max_primary.

https://codereview.chromium.org/15875007/diff/9001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/search/history_data_store.cc (right):

https://codereview.chromium.org/15875007/diff/9001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/search/history_data_store.cc:23: const char
kKeyVersion[] = "version";
On 2013/05/23 21:51:08, James Cook (Chromium) wrote:
> I'm glad you're including a file format version number.

Future is always hard to predict. :p

https://codereview.chromium.org/15875007/diff/9001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/search/history_data_store.cc:56: //               
"result id of the latest secondary association"
On 2013/05/23 21:51:08, James Cook (Chromium) wrote:
> nit: I would say "newest" instead of "latest" because I get confused about
> "late" vs. "old"-est. :-)

Done.

https://codereview.chromium.org/15875007/diff/9001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/search/history_data_store.cc:131:
Flush(OnFlushedCallback());
On 2013/05/23 21:51:08, James Cook (Chromium) wrote:
> Will this force this data store to be written on shutdown? Could we possibly
> avoid doing that?  I would prefer to lose one or two associations at shutdown
to
> adding another blocking write.

Made |file_task_runner_| be a SKIP_ON_SHUTDOWN task runner to address the
concern. Flush() here schedules the disk write and if we are really at shutdown,
the task will be dropped.

https://codereview.chromium.org/15875007/diff/9001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/search/history_data_store.cc:212:
DCHECK(BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
On 2013/05/23 21:51:08, James Cook (Chromium) wrote:
> I like thread assertions.

Me too.

https://codereview.chromium.org/15875007/diff/9001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/search/history_data_store_unittest.cc (right):

https://codereview.chromium.org/15875007/diff/9001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/search/history_data_store_unittest.cc:64: Load();
On 2013/05/23 21:51:08, James Cook (Chromium) wrote:
> Do you have to reload it after a flush?  It might be more clear to call
Flush()
> then explicitly call Load() in the test below.

It is here because I found that the test always have a Load() following the
Flush().
Removed Load() here and put it back to the test code.

Also changed to use HistoryDataStore::Flush(). The test was added before the
call is added. :)

https://codereview.chromium.org/15875007/diff/9001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/search/history_unittest.cc (right):

https://codereview.chromium.org/15875007/diff/9001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/search/history_unittest.cc:33: void Wait(int
wait_time_ms) {
On 2013/05/23 21:51:08, James Cook (Chromium) wrote:
> This is not waiting in the normal case, it's just a timeout if things take too
> long?  You should comment that if so.  A class comment might be good.

It is a normal wait. I usually like to put a limit on the wait so that we don't
get an infinite wait in case anything goes wrong. The data load happens on the
blocking pool and we usually should exit before the timer is fired.

Class comments added.

https://codereview.chromium.org/15875007/diff/9001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/search/history_unittest.cc:69: void Wait(int
wait_time_ms) {
On 2013/05/23 21:51:08, James Cook (Chromium) wrote:
> ditto

Done.

[tfarina, 2013-05-23 22:56:25]

https://codereview.chromium.org/15875007/diff/25001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/search/history_factory.h (right):

https://codereview.chromium.org/15875007/diff/25001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/search/history_factory.h:8: #include
"base/memory/singleton.h"
could you forward declare this instead?

template <typename T> struct DefaultSingletonTraits;

[James Cook, 2013-05-24 03:57:05]

LGTM with nits

https://codereview.chromium.org/15875007/diff/9001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/search/history.cc (right):

https://codereview.chromium.org/15875007/diff/9001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/search/history.cc:30: const char kStoreDataFileName[]
= "App Launcher Search";
Consistency is more important than having an extension - keep it as is.

https://codereview.chromium.org/15875007/diff/25001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/search/history_unittest.cc (right):

https://codereview.chromium.org/15875007/diff/25001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/search/history_unittest.cc:37: void Wait(int
wait_time_ms) {
nit: I think changing |wait_time_ms| to |max_wait_time_ms| or |timeout_ms| would
help the reader understand that Wait() usually finishes much earlier.

https://codereview.chromium.org/15875007/diff/25001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/search/history_unittest.cc:77: void Wait(int
wait_time_ms) {
ditto, max_wait_time_ms or timeout_ms

https://codereview.chromium.org/15875007/diff/25001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/search/history_unittest.cc:268: // Touches the oldest
and 2nd oldest becomes oldest now..
nit: one . at end

https://codereview.chromium.org/15875007/diff/25001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/search/search_controller.h (right):

https://codereview.chromium.org/15875007/diff/25001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/search/search_controller.h:63: History* history_;  //
PKS, not owned.
nit: spell out ProfileKeyedService

[xiyuan, 2013-05-24 16:33:01]

https://codereview.chromium.org/15875007/diff/25001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/search/history_factory.h (right):

https://codereview.chromium.org/15875007/diff/25001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/search/history_factory.h:8: #include
"base/memory/singleton.h"
On 2013/05/23 22:56:25, tfarina wrote:
> could you forward declare this instead?
> 
> template <typename T> struct DefaultSingletonTraits;

Done.

https://codereview.chromium.org/15875007/diff/25001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/search/history_unittest.cc (right):

https://codereview.chromium.org/15875007/diff/25001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/search/history_unittest.cc:37: void Wait(int
wait_time_ms) {
On 2013/05/24 03:57:05, James Cook (Chromium) wrote:
> nit: I think changing |wait_time_ms| to |max_wait_time_ms| or |timeout_ms|
would
> help the reader understand that Wait() usually finishes much earlier.

Done.

https://codereview.chromium.org/15875007/diff/25001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/search/history_unittest.cc:77: void Wait(int
wait_time_ms) {
On 2013/05/24 03:57:05, James Cook (Chromium) wrote:
> ditto, max_wait_time_ms or timeout_ms

Done.

https://codereview.chromium.org/15875007/diff/25001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/search/history_unittest.cc:268: // Touches the oldest
and 2nd oldest becomes oldest now..
On 2013/05/24 03:57:05, James Cook (Chromium) wrote:
> nit: one . at end

Done.

https://codereview.chromium.org/15875007/diff/25001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/search/search_controller.h (right):

https://codereview.chromium.org/15875007/diff/25001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/search/search_controller.h:63: History* history_;  //
PKS, not owned.
On 2013/05/24 03:57:05, James Cook (Chromium) wrote:
> nit: spell out ProfileKeyedService

Done.

[commit-bot: I haz the power, 2013-05-24 16:54:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xiyuan@chromium.org/15875007/32001

[commit-bot: I haz the power, 2013-05-24 22:34:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xiyuan@chromium.org/15875007/51001

[commit-bot: I haz the power, 2013-05-24 22:49:06]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-05-25 00:37:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xiyuan@chromium.org/15875007/51001

[commit-bot: I haz the power, 2013-05-25 03:58:06]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-05-25 05:10:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xiyuan@chromium.org/15875007/51001

[commit-bot: I haz the power, 2013-05-25 07:29:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xiyuan@chromium.org/15875007/82001

[commit-bot: I haz the power, 2013-05-26 02:15:30]

Change committed as 202328