Allow systematic prefix search in bookmarks.

Allow systematic prefix search in bookmarks.

This CL allows the bookmark model's search to always use prefix search, even
when terms are too short.

BUG=415774

[lpromero, 2014-11-03 17:31:16]

lpromero@chromium.org changed reviewers:
+ noyau@chromium.org

[lpromero, 2014-11-03 17:31:41]

lpromero@chromium.org changed reviewers:
+ ianwen@chromium.org, kkimlabs@chromium.org

[noyau (Ping after 24h), 2014-11-03 17:38:55]

lgtm

[Kibeom Kim (inactive), 2014-11-03 18:41:32]

https://codereview.chromium.org/701553002/diff/1/components/query_parser/quer...
File components/query_parser/query_parser.cc (right):

https://codereview.chromium.org/701553002/diff/1/components/query_parser/quer...
components/query_parser/query_parser.cc:149: class QueryNodePrefixWord : public
QueryNodeWord {
It looks like this class is identical to QueryNodeWord just except
|QueryParser::IsWordLongEnoughForPrefixSearch|. Can we keep as one class and
branch by a constructor argument? I think it makes sense to use template in this
case. Maybe something like

template <bool PREFIX_WORD_MATCH>
class QueryNodeWord {
...
}

[Kibeom Kim (inactive), 2014-11-03 18:42:00]

kkimlabs@chromium.org changed reviewers:
+ danduong@chromium.org

[lpromero, 2014-11-03 18:42:09]

lpromero@chromium.org changed reviewers:
+ sky@chromium.org
- danduong@chromium.org

[lpromero, 2014-11-03 18:42:10]

sky@chromium.org: Please review changes in all files.

[noyau (Ping after 24h), 2014-11-03 18:47:50]

On 2014/11/03 18:42:10, lpromero wrote:
> mailto:sky@chromium.org: Please review changes in all files.

A little bit of background on this: for Stars on mobile we need a quick way to
do search across bookmarks. Rather than rolling our own we tried to reuse the
existing bookmark index, but the limitation of doing exact match for queries
yield weird results in the UI. Example for a title "foo bar" the query "foo" and
"foo bar" would find the relevant bookmark, but the intermediate "foo b" and
"foo ba" would not.

This CL allow to optionally bypass the exact match for small queries, returning
the expected responses for the user queries.

[sky, 2014-11-03 21:44:06]

https://codereview.chromium.org/701553002/diff/1/components/bookmarks/browser...
File components/bookmarks/browser/bookmark_index.h (right):

https://codereview.chromium.org/701553002/diff/1/components/bookmarks/browser...
components/bookmarks/browser/bookmark_index.h:74: // When |always_prefix_search|
is false, if the term is not long enough in the
This description is confusing. I think you want something like:
When |always_prefix_search| is true, the term is always considered for a prefix
search. If false, the behavior of exact match depends on
|query_parser::QueryParser::IsWordLongEnoughForPrefixSearch|. If the term is
long enough for a prefix search a prefix search is done, otherwise an exact
match is done.

https://codereview.chromium.org/701553002/diff/1/components/bookmarks/browser...
File components/bookmarks/browser/bookmark_model.cc (right):

https://codereview.chromium.org/701553002/diff/1/components/bookmarks/browser...
components/bookmarks/browser/bookmark_model.cc:710: text, max_count, true /*
always_prefix_search */, matches);
I'm not a fan of this style. It's a good indication you should use an enum.

https://codereview.chromium.org/701553002/diff/1/components/query_parser/quer...
File components/query_parser/query_parser.cc (right):

https://codereview.chromium.org/701553002/diff/1/components/query_parser/quer...
components/query_parser/query_parser.cc:149: class QueryNodePrefixWord : public
QueryNodeWord {
On 2014/11/03 18:41:32, Kibeom Kim wrote:
> It looks like this class is identical to QueryNodeWord just except
> |QueryParser::IsWordLongEnoughForPrefixSearch|. Can we keep as one class and
> branch by a constructor argument? I think it makes sense to use template in
this
> case. Maybe something like
> 
> template <bool PREFIX_WORD_MATCH>
> class QueryNodeWord {
> ...
> }

Why bother with a template and not a field, always_prefix_search?

[Kibeom Kim (inactive), 2014-11-04 01:28:19]

continuing at https://codereview.chromium.org/703553002/

https://codereview.chromium.org/701553002/diff/1/components/bookmarks/browser...
File components/bookmarks/browser/bookmark_index.h (right):

https://codereview.chromium.org/701553002/diff/1/components/bookmarks/browser...
components/bookmarks/browser/bookmark_index.h:74: // When |always_prefix_search|
is false, if the term is not long enough in the
On 2014/11/03 21:44:05, sky wrote:
> This description is confusing. I think you want something like:
> When |always_prefix_search| is true, the term is always considered for a
prefix
> search. If false, the behavior of exact match depends on
> |query_parser::QueryParser::IsWordLongEnoughForPrefixSearch|. If the term is
> long enough for a prefix search a prefix search is done, otherwise an exact
> match is done.

Done. (removed since we put enum and it's self-explanatory)

https://codereview.chromium.org/701553002/diff/1/components/bookmarks/browser...
File components/bookmarks/browser/bookmark_model.cc (right):

https://codereview.chromium.org/701553002/diff/1/components/bookmarks/browser...
components/bookmarks/browser/bookmark_model.cc:710: text, max_count, true /*
always_prefix_search */, matches);
On 2014/11/03 21:44:05, sky wrote:
> I'm not a fan of this style. It's a good indication you should use an enum.

Done.

https://codereview.chromium.org/701553002/diff/1/components/query_parser/quer...
File components/query_parser/query_parser.cc (right):

https://codereview.chromium.org/701553002/diff/1/components/query_parser/quer...
components/query_parser/query_parser.cc:149: class QueryNodePrefixWord : public
QueryNodeWord {
On 2014/11/03 21:44:05, sky wrote:
> On 2014/11/03 18:41:32, Kibeom Kim wrote:
> > It looks like this class is identical to QueryNodeWord just except
> > |QueryParser::IsWordLongEnoughForPrefixSearch|. Can we keep as one class and
> > branch by a constructor argument? I think it makes sense to use template in
> this
> > case. Maybe something like
> > 
> > template <bool PREFIX_WORD_MATCH>
> > class QueryNodeWord {
> > ...
> > }
> 
> Why bother with a template and not a field, always_prefix_search?

Done.

Since the class is local to this file and not used elsewhere, I thought it'll be
good to make compile-time fixed code by template, but yeah, it wasn't a good
suggestion.