Omnibox: Make URLs of Bookmarks Searchable

Omnibox: Make URLs of Bookmarks Searchable

In particular, this makes
* BookmarkIndex index the URLs for bookmarks
* BookmarkIndex search for matches (match positions) within those URLs
* BookmarkProvider score based on both title and URL matches

All these is guarded by a field trial, also added in this changelist.

I added ample unit tests.

I also verified this works by testing it interactively using a local variations server and --force-fieldtrials.

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

[Mark P, 2014-02-28 15:48:03]

Peter,

Can you glance at this and tell me what you think of the general approach?  In
particular, I'm curious about
- whether you care that I use the same index_ to store title words and URL words
or would prefer two separate indices.
- how I should resolve the component-crossing issues for the CleanUp functions. 
(See changelist description and change to understand what I mean.)
- any other concerns before I move onto the TODO list pasted in the bug.

thanks,
mark

https://codereview.chromium.org/184663002/diff/1/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_match.cc (right):

https://codereview.chromium.org/184663002/diff/1/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_match.cc:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
I don't know why codereview is showing the diff against such a strange file.  It
should be diffing against bookmark_title_match.cc.

https://codereview.chromium.org/184663002/diff/1/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_utils.cc (right):

https://codereview.chromium.org/184663002/diff/1/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_utils.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
All changes here are simply a cut-and-paste of functions from history.

https://codereview.chromium.org/184663002/diff/1/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_utils.h (right):

https://codereview.chromium.org/184663002/diff/1/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_utils.h:111: // Truncates an overly-long URL,
unescapes it, and lower-cases it,
this is a simple move of these functions from the history namespace.

https://codereview.chromium.org/184663002/diff/1/chrome/browser/history/query...
File chrome/browser/history/query_parser.h (right):

https://codereview.chromium.org/184663002/diff/1/chrome/browser/history/query...
chrome/browser/history/query_parser.h:104: // Sorts the match positions in
|matches| by their first index, then
This is merely making a private anonymous-namescape function into a public
static one.

[Peter Kasting, 2014-02-28 22:18:56]

What's the effect of using one index instead of two?  I don't think I understand
what's going on well enough to comment yet.

As for where the cleanup functions should go, maybe these should be in net/
instead?  I think both history and bookmarks can depend on that.  I wouldn't
mind them being in bookmarks but it seems like they're not really
bookmark-specific in either function or who's actually calling them.

https://codereview.chromium.org/184663002/diff/1/chrome/browser/history/query...
File chrome/browser/history/query_parser.h (right):

https://codereview.chromium.org/184663002/diff/1/chrome/browser/history/query...
chrome/browser/history/query_parser.h:106: static void
CoalseAndSortMatchPositions(Snippet::MatchPositions* matches);
Coalse -> Coalesce

[Mark P, 2014-02-28 23:19:47]

On Fri, Feb 28, 2014 at 2:18 PM, <pkasting@chromium.org> wrote:

> What's the effect of using one index instead of two?  I don't think I
> understand
> what's going on well enough to comment yet.
>

I thought it felt a little icky to store hits from two separate fields in
the same index.  It certainly is a shorter code, however, than using two
indices and merging them.

This will make it harder in the future if we want to store more information
about hits such as what position it is in which field.  Right now bookmarks
index gets around that by re-looking for hits on each of the bookmarks that
is retrieved.  If we want to skip that step (for efficiency for instance),
then things will get complicated.  But I guess we can split into two
indices at that time, if that turns out to be unnecessary and the right
thing to do to resolve the need that arises.



> As for where the cleanup functions should go, maybe these should be in net/
> instead?  I think both history and bookmarks can depend on that.  I
> wouldn't
> mind them being in bookmarks but it seems like they're not really
> bookmark-specific in either function or who's actually calling them.


I can't find a great place in net.  Here are some ideas:
net/base
net/filter
net/data
or, create a new directory, net/indexing

--mark


>
>
>
> https://codereview.chromium.org/184663002/diff/1/chrome/
> browser/history/query_parser.h
> File chrome/browser/history/query_parser.h (right):
>
> https://codereview.chromium.org/184663002/diff/1/chrome/
> browser/history/query_parser.h#newcode106
> chrome/browser/history/query_parser.h:106: static void
> CoalseAndSortMatchPositions(Snippet::MatchPositions* matches);
> Coalse -> Coalesce
>
> https://codereview.chromium.org/184663002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peter Kasting, 2014-02-28 23:22:10]

On 2014/02/28 23:19:47, Mark P wrote:
> On Fri, Feb 28, 2014 at 2:18 PM, <mailto:pkasting@chromium.org> wrote:
> 
> > What's the effect of using one index instead of two?  I don't think I
> > understand
> > what's going on well enough to comment yet.
> >
> 
> I thought it felt a little icky to store hits from two separate fields in
> the same index.  It certainly is a shorter code, however, than using two
> indices and merging them.
> 
> This will make it harder in the future if we want to store more information
> about hits such as what position it is in which field.  Right now bookmarks
> index gets around that by re-looking for hits on each of the bookmarks that
> is retrieved.  If we want to skip that step (for efficiency for instance),
> then things will get complicated.  But I guess we can split into two
> indices at that time, if that turns out to be unnecessary and the right
> thing to do to resolve the need that arises.

OK.  I trust your judgment.

> > As for where the cleanup functions should go, maybe these should be in net/
> > instead?  I think both history and bookmarks can depend on that.  I
> > wouldn't
> > mind them being in bookmarks but it seems like they're not really
> > bookmark-specific in either function or who's actually calling them.
> 
> 
> I can't find a great place in net.  Here are some ideas:
> net/base
> net/filter
> net/data
> or, create a new directory, net/indexing

I'd ping a net owner (e.g. rsleevi) and ask if these make sense in net or if he
has an idea where else to put them.

[Mark P, 2014-04-16 22:39:16]

This monster change, though it touches a ton of files, shouldn't be hard to
review.  The bulk of the code, in bookmark_index and bookmark_provider, should
be straightforward.  The majority of lines to review are in unit tests.

Enjoy!  ;-)

--mark

https://codereview.chromium.org/184663002/diff/1/chrome/browser/history/query...
File chrome/browser/history/query_parser.h (right):

https://codereview.chromium.org/184663002/diff/1/chrome/browser/history/query...
chrome/browser/history/query_parser.h:106: static void
CoalseAndSortMatchPositions(Snippet::MatchPositions* matches);
On 2014/02/28 22:18:57, Peter Kasting wrote:
> Coalse -> Coalesce

Done.

[Peter Kasting, 2014-04-16 23:44:25]

Mostly nits, only the scoring (where URL matches still use the title length) is
significant.

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/autocompl...
File chrome/browser/autocomplete/bookmark_provider.cc (right):

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/bookmark_provider.cc:162: const size_t match_start =
!bookmark_match.url_match_positions.empty() ?
Tiny nit: Remove "!" and reverse arms

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/bookmark_provider.cc:166: std::vector<size_t>
offsets = BookmarkMatch::OffsetsFromMatchPositions(
Nit: Consider using a typedef here/elsewhere

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/bookmark_provider.cc:181: // *** test this
^^^ ?

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/bookmark_provider.cc:216: //  1) how many characters
in the bookmark's title/URL is part of this match.
Nit: is -> are

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/bookmark_provider.cc:225: //     formula.
This is really confusing.  Why are URL matches scored based on title length? 
That seems surprising and mystifying.

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/autocompl...
File chrome/browser/autocomplete/bookmark_provider.h (right):

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/bookmark_provider.h:53: // title_match's bookmark,
and 2) the bookmark's title, not the URL's page
Nit: This still refers to |title_match|

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/bookmark_provider.h:57: // title, as the
description.
Nit: Uh... broken edit?

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/autocompl...
File chrome/browser/autocomplete/url_prefix.cc (right):

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/url_prefix.cc:102: return base::string16::npos;
Tiny nit: Fractionally less verbose:

  return best_prefix ?
      (best_prefix->prefix.length() + matching_string->length()) :
      base::string16::npos;

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_index.h (right):

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_index.h:37: explicit
BookmarkIndex(content::BrowserContext* browser_context,
Nit: No "explicit"

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_index_unittest.cc (right):

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_index_unittest.cc:304: const BookmarkMatch&
match = matches[0];
Nit: Seems like this is only used once below, I'd just inline it

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_index_unittest.cc:357: const BookmarkMatch&
match = matches[0];
Nit: Same nit

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_match.h (right):

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_match.h:8: #include <stddef.h>
Nit: If this is needed for size_t, include <cstddef>.  Otherwise remove.

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_model.cc (right):

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_model.cc:973: prefs::kAcceptLanguages) : ""),
Nit: "" -> std::string()

While it's one line longer, I think this wrapping is less tortuous:

      bb_node, other_node, mobile_node,
      new BookmarkIndex(
          profile_,
          profile_ ?
              profile_->GetPrefs()->GetString(prefs::kAcceptLanguages) :
              std::string()),
      next_node_id_);

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/history/q...
File chrome/browser/history/query_parser.h (right):

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/history/q...
chrome/browser/history/query_parser.h:106: static void
CoalesceAndSortMatchPositions(Snippet::MatchPositions* matches);
Nit: If it sorts and then coalesces, name it SortAndCoalesceMatchPositions().

[Mark P, 2014-04-17 20:24:17]

I've replied to all your comments.

You may also notice a bunch of (non-functional) changes in this new patchset
because query_parser and some parts of bookmarks (in particular bookmark_match)
have moved into components.

I have two questions:
* Who should I get for an additional review?  Either sky or brettw are options
(right OWNERS rights).  Care to pick one?  Or I can TBR this change after you
review it.  What do you think?
* You'll noticed I introduced a new DEPS exception to allow bookmark_utils to
appear in history files.  I don't strictly have to do this.  This is because I
put the CleanupTitle / CleanupURL in chrome/browser/bookmark_utils.  I could put
them in components/bookmarks if you feel that would be better, but that would
still require a different exception, just an exception for a component, not a
chrome/browser directory.  I don't think it's much better, but perhaps you have
an opinion?  I'd rather punt on this and let tfarina sort it out while he
componentizes things.

--mark

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/autocompl...
File chrome/browser/autocomplete/bookmark_provider.cc (right):

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/bookmark_provider.cc:162: const size_t match_start =
!bookmark_match.url_match_positions.empty() ?
On 2014/04/16 23:44:25, Peter Kasting wrote:
> Tiny nit: Remove "!" and reverse arms

Done.

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/bookmark_provider.cc:166: std::vector<size_t>
offsets = BookmarkMatch::OffsetsFromMatchPositions(
On 2014/04/16 23:44:25, Peter Kasting wrote:
> Nit: Consider using a typedef here/elsewhere

I prefer to keep it this way for consistency with existing code, e.g., the code
in FormatURLWithOffsets uses this full declaration, no typedef, likewise with
the similar functions in InMemoryURLIndex.

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/bookmark_provider.cc:181: // *** test this
On 2014/04/16 23:44:25, Peter Kasting wrote:
> ^^^ ?

Oops, forgot to remove the note that I should test contents_class highlighting. 
I already did that.  Now removed.

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/bookmark_provider.cc:216: //  1) how many characters
in the bookmark's title/URL is part of this match.
On 2014/04/16 23:44:25, Peter Kasting wrote:
> Nit: is -> are

Done.

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/bookmark_provider.cc:225: //     formula.
On 2014/04/16 23:44:25, Peter Kasting wrote:
> This is really confusing.  Why are URL matches scored based on title length? 

You're right, this part doesn't make much sense.  I corrected this comment and
code to make the match position factor for URL matches be based on URL length.

Overall, though, here were my goals for scoring:
* don't change scoring too much compare it what it is currently.  i.e., this
should change should mostly be about indexing URLs, not much about new scoring
approaches
* make it easy to have two control flows through here, one which should keep the
same scores as before this patch and one that takes URL scoring into account
* URL length shouldn't matter much for scoring URLs.  (People bookmarks things
of varying lengths; it shouldn't much better for retrieval/scoring how long they
are, just how much of what they type is matched.)

This is why I use #characters matched / title_length as the basis for scoring. 
I used title_length and opposed to title_length + url_length due to the third
bullet.  Instead I added the +10 hack (explained below at line 243), but it's my
compromise between the first bullet and the third bullet above.

> That seems surprising and mystifying.

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/autocompl...
File chrome/browser/autocomplete/bookmark_provider.h (right):

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/bookmark_provider.h:53: // title_match's bookmark,
and 2) the bookmark's title, not the URL's page
On 2014/04/16 23:44:25, Peter Kasting wrote:
> Nit: This still refers to |title_match|

Done.

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/bookmark_provider.h:57: // title, as the
description.
On 2014/04/16 23:44:25, Peter Kasting wrote:
> Nit: Uh... broken edit?

Yup, bad rebase/resolve.
Fixed.

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/autocompl...
File chrome/browser/autocomplete/url_prefix.cc (right):

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/url_prefix.cc:102: return base::string16::npos;
On 2014/04/16 23:44:25, Peter Kasting wrote:
> Tiny nit: Fractionally less verbose:
> 
>   return best_prefix ?
>       (best_prefix->prefix.length() + matching_string->length()) :
>       base::string16::npos;

Okay.

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_index.h (right):

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_index.h:37: explicit
BookmarkIndex(content::BrowserContext* browser_context,
On 2014/04/16 23:44:25, Peter Kasting wrote:
> Nit: No "explicit"

Done.

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_index_unittest.cc (right):

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_index_unittest.cc:304: const BookmarkMatch&
match = matches[0];
On 2014/04/16 23:44:25, Peter Kasting wrote:
> Nit: Seems like this is only used once below, I'd just inline it

Done.

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_index_unittest.cc:357: const BookmarkMatch&
match = matches[0];
On 2014/04/16 23:44:25, Peter Kasting wrote:
> Nit: Same nit

Done.

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_match.h (right):

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_match.h:8: #include <stddef.h>
On 2014/04/16 23:44:25, Peter Kasting wrote:
> Nit: If this is needed for size_t, include <cstddef>.  Otherwise remove.

Appears unnecessary.  Removing. (It was in the original file.)

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_model.cc (right):

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_model.cc:973: prefs::kAcceptLanguages) : ""),
On 2014/04/16 23:44:25, Peter Kasting wrote:
> Nit: "" -> std::string()

Done.

> While it's one line longer, I think this wrapping is less tortuous:
> 
>       bb_node, other_node, mobile_node,
>       new BookmarkIndex(
>           profile_,
>           profile_ ?
>               profile_->GetPrefs()->GetString(prefs::kAcceptLanguages) :
>               std::string()),
>       next_node_id_);

Done.

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/history/q...
File chrome/browser/history/query_parser.h (right):

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/history/q...
chrome/browser/history/query_parser.h:106: static void
CoalesceAndSortMatchPositions(Snippet::MatchPositions* matches);
On 2014/04/16 23:44:25, Peter Kasting wrote:
> Nit: If it sorts and then coalesces, name it SortAndCoalesceMatchPositions().

Done.

[Peter Kasting, 2014-04-17 22:30:37]

LGTM

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/autocompl...
File chrome/browser/autocomplete/bookmark_provider.cc (right):

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/bookmark_provider.cc:166: std::vector<size_t>
offsets = BookmarkMatch::OffsetsFromMatchPositions(
On 2014/04/17 20:24:18, Mark P wrote:
> On 2014/04/16 23:44:25, Peter Kasting wrote:
> > Nit: Consider using a typedef here/elsewhere
> 
> I prefer to keep it this way for consistency with existing code, e.g., the
code
> in FormatURLWithOffsets uses this full declaration, no typedef, likewise with
> the similar functions in InMemoryURLIndex.

Right, I really intended for things like FormatURLWithOffsets() to get changed
as well.  It would be fine for this to happen separately, but the proliferation
of std::vector<size_t> everywhere seems undesirable.

https://codereview.chromium.org/184663002/diff/70001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_index.cc (right):

https://codereview.chromium.org/184663002/diff/70001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_index.cc:204:
std::vector<query_parser::QueryWord> title_words, url_words;
Nit: Again, the more we can use typedefs in place of things like
std::vector<query_parser::XXX> the better.  Also applies to this function's
arguments.

[Mark P, 2014-04-17 23:23:01]

Have opinions on the two issues in my last e-mail on this thread?


On Thu, Apr 17, 2014 at 3:30 PM, <pkasting@chromium.org> wrote:

> LGTM
>
>
>
> https://codereview.chromium.org/184663002/diff/50001/
> chrome/browser/autocomplete/bookmark_provider.cc
> File chrome/browser/autocomplete/bookmark_provider.cc (right):
>
> https://codereview.chromium.org/184663002/diff/50001/
> chrome/browser/autocomplete/bookmark_provider.cc#newcode166
> chrome/browser/autocomplete/bookmark_provider.cc:166:
> std::vector<size_t> offsets = BookmarkMatch::OffsetsFromMatchPositions(
> On 2014/04/17 20:24:18, Mark P wrote:
>
>> On 2014/04/16 23:44:25, Peter Kasting wrote:
>> > Nit: Consider using a typedef here/elsewhere
>>
>
>  I prefer to keep it this way for consistency with existing code, e.g.,
>>
> the code
>
>> in FormatURLWithOffsets uses this full declaration, no typedef,
>>
> likewise with
>
>> the similar functions in InMemoryURLIndex.
>>
>
> Right, I really intended for things like FormatURLWithOffsets() to get
> changed as well.  It would be fine for this to happen separately, but
> the proliferation of std::vector<size_t> everywhere seems undesirable.
>
> https://codereview.chromium.org/184663002/diff/70001/
> chrome/browser/bookmarks/bookmark_index.cc
> File chrome/browser/bookmarks/bookmark_index.cc (right):
>
> https://codereview.chromium.org/184663002/diff/70001/
> chrome/browser/bookmarks/bookmark_index.cc#newcode204
> chrome/browser/bookmarks/bookmark_index.cc:204:
> std::vector<query_parser::QueryWord> title_words, url_words;
> Nit: Again, the more we can use typedefs in place of things like
> std::vector<query_parser::XXX> the better.  Also applies to this
> function's arguments.
>
> https://codereview.chromium.org/184663002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peter Kasting, 2014-04-17 23:24:39]

On 2014/04/17 23:23:01, Mark P wrote:
> Have opinions on the two issues in my last e-mail on this thread?

Oh, sorry.  Forgot to reply.

For reviewers, I have no opinion.

For DEPS exceptions, I'm fine with doing whatever you'd like for now and letting
tfarina clean it up later, if he's componentizing all this stuff.

[tfarina, 2014-04-18 00:26:04]

Mark, your suggestion sounds good to me.

We can sort out the deps changes later.

https://codereview.chromium.org/184663002/diff/70001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_utils.cc (right):

https://codereview.chromium.org/184663002/diff/70001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_utils.cc:36: static const size_t
kCleanedUpUrlMaxLength = 1024u;
No static needed. You are already in an unnamed namespace.

[Mark P, 2014-04-18 14:52:42]

Scott, can you please review this to whatever extent you desire.  You're an
owner of all the relevant directories.  Peter has already reviewed and approved.

thanks,
mark

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/autocompl...
File chrome/browser/autocomplete/bookmark_provider.cc (right):

https://codereview.chromium.org/184663002/diff/50001/chrome/browser/autocompl...
chrome/browser/autocomplete/bookmark_provider.cc:166: std::vector<size_t>
offsets = BookmarkMatch::OffsetsFromMatchPositions(
On 2014/04/17 22:30:37, Peter Kasting wrote:
> On 2014/04/17 20:24:18, Mark P wrote:
> > On 2014/04/16 23:44:25, Peter Kasting wrote:
> > > Nit: Consider using a typedef here/elsewhere
> > 
> > I prefer to keep it this way for consistency with existing code, e.g., the
> code
> > in FormatURLWithOffsets uses this full declaration, no typedef, likewise
with
> > the similar functions in InMemoryURLIndex.
> 
> Right, I really intended for things like FormatURLWithOffsets() to get changed
> as well.  It would be fine for this to happen separately, but the
proliferation
> of std::vector<size_t> everywhere seems undesirable.

Filed crbug.com/364840

https://codereview.chromium.org/184663002/diff/70001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_index.cc (right):

https://codereview.chromium.org/184663002/diff/70001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_index.cc:204:
std::vector<query_parser::QueryWord> title_words, url_words;
On 2014/04/17 22:30:37, Peter Kasting wrote:
> Nit: Again, the more we can use typedefs in place of things like
> std::vector<query_parser::XXX> the better.  Also applies to this function's
> arguments.

Done.  This mostly helped throughout query_parser.cc

https://codereview.chromium.org/184663002/diff/70001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_utils.cc (right):

https://codereview.chromium.org/184663002/diff/70001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_utils.cc:36: static const size_t
kCleanedUpUrlMaxLength = 1024u;
On 2014/04/18 00:26:04, tfarina wrote:
> No static needed. You are already in an unnamed namespace.

Done.
(here and below)

[sky, 2014-04-18 16:40:02]

I'll look at the rest of this shortly.

https://codereview.chromium.org/184663002/diff/90001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/DEPS (right):

https://codereview.chromium.org/184663002/diff/90001/chrome/browser/bookmarks...
chrome/browser/bookmarks/DEPS:20:
"!chrome/browser/omnibox/omnibox_field_trial.h",
There is work in progress to move all this code to components. We don't want
another dependency like this. You'll need to pass
OmniboxFieldTrial::BookmarksIndexURLsValue() through the constructor.
Alternative is wait for https://codereview.chromium.org/242693003/ to land then
wire through BookmarkClient interface.

[sky, 2014-04-18 16:52:20]

https://codereview.chromium.org/184663002/diff/90001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/DEPS (right):

https://codereview.chromium.org/184663002/diff/90001/chrome/browser/bookmarks...
chrome/browser/bookmarks/DEPS:20:
"!chrome/browser/omnibox/omnibox_field_trial.h",
On 2014/04/18 16:40:02, sky wrote:
> There is work in progress to move all this code to components. We don't want
> another dependency like this. You'll need to pass
> OmniboxFieldTrial::BookmarksIndexURLsValue() through the constructor.
> Alternative is wait for https://codereview.chromium.org/242693003/ to land
then
> wire through BookmarkClient interface.

Actually, there may be threading issues if you try to express this through the
client interface. Safer to inject.

[Mark P, 2014-04-18 17:27:48]

https://codereview.chromium.org/184663002/diff/90001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/DEPS (right):

https://codereview.chromium.org/184663002/diff/90001/chrome/browser/bookmarks...
chrome/browser/bookmarks/DEPS:20:
"!chrome/browser/omnibox/omnibox_field_trial.h",
On 2014/04/18 16:52:20, sky wrote:
> On 2014/04/18 16:40:02, sky wrote:
> > There is work in progress to move all this code to components. We don't want
> > another dependency like this. You'll need to pass
> > OmniboxFieldTrial::BookmarksIndexURLsValue() through the constructor.
> > Alternative is wait for https://codereview.chromium.org/242693003/ to land
> then
> > wire through BookmarkClient interface.
> 
> Actually, there may be threading issues if you try to express this through the
> client interface. Safer to inject.

You want me to bother even though this field trial will only exist for a
milestone or two?  (As soon as evals finish, we'll either turn it on by default
or remove the code.)

[sky, 2014-04-18 20:36:08]

On Fri, Apr 18, 2014 at 10:27 AM,  <mpearson@chromium.org> wrote:
>
>
https://codereview.chromium.org/184663002/diff/90001/chrome/browser/bookmarks...
> File chrome/browser/bookmarks/DEPS (right):
>
>
https://codereview.chromium.org/184663002/diff/90001/chrome/browser/bookmarks...
> chrome/browser/bookmarks/DEPS:20:
> "!chrome/browser/omnibox/omnibox_field_trial.h",
> On 2014/04/18 16:52:20, sky wrote:
>>
>> On 2014/04/18 16:40:02, sky wrote:
>> > There is work in progress to move all this code to components. We
>
> don't want
>>
>> > another dependency like this. You'll need to pass
>> > OmniboxFieldTrial::BookmarksIndexURLsValue() through the
>
> constructor.
>>
>> > Alternative is wait for https://codereview.chromium.org/242693003/
>
> to land
>>
>> then
>> > wire through BookmarkClient interface.
>
>
>> Actually, there may be threading issues if you try to express this
>
> through the
>>
>> client interface. Safer to inject.
>
>
> You want me to bother even though this field trial will only exist for a
> milestone or two?  (As soon as evals finish, we'll either turn it on by
> default or remove the code.)

If think the field trial if valid and will shed helpful data, go for
it. Otherwise just make the behavior you want the default.

  -Scott

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mark P, 2014-04-18 21:28:08]

https://codereview.chromium.org/184663002/diff/90001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/DEPS (right):

https://codereview.chromium.org/184663002/diff/90001/chrome/browser/bookmarks...
chrome/browser/bookmarks/DEPS:20:
"!chrome/browser/omnibox/omnibox_field_trial.h",
On 2014/04/18 17:27:49, Mark P wrote:
> On 2014/04/18 16:52:20, sky wrote:
> > On 2014/04/18 16:40:02, sky wrote:
> > > There is work in progress to move all this code to components. We don't
want
> > > another dependency like this. You'll need to pass
> > > OmniboxFieldTrial::BookmarksIndexURLsValue() through the constructor.
> > > Alternative is wait for https://codereview.chromium.org/242693003/ to land
> > then
> > > wire through BookmarkClient interface.
> > 
> > Actually, there may be threading issues if you try to express this through
the
> > client interface. Safer to inject.
> 
> You want me to bother even though this field trial will only exist for a
> milestone or two?  (As soon as evals finish, we'll either turn it on by
default
> or remove the code.)

I think the field trial will be useful, if only to say how big an improvement
this is.  I try to run field trials on everything so that if I see a change in
omnibox metrics from one release to the next, I can understand if it's the
expected result of everything we launched or if something else may have changed
(in a good or bad way) under the hood.

Do you want to review anything else in this changelist?

[sky, 2014-04-18 21:56:17]

I looked at the rest (not including autocomplete, as Peter did that) and only
have a minor nit. I do have one question though, is it worth measuring how long
GetBookmarksWithTitlesMatching takes so that you easily tell differences between
the two field trials? Or will other autocomplete metrics provide coverage for
this already?

https://codereview.chromium.org/184663002/diff/90001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_index.h (right):

https://codereview.chromium.org/184663002/diff/90001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_index.h:37: const std::string& languages);
Document what languages is.

[Mark P, 2014-04-18 22:22:07]

On Fri, Apr 18, 2014 at 2:56 PM, <sky@chromium.org> wrote:
> I looked at the rest (not including autocomplete, as Peter did that) and only
> have a minor nit. I do have one question though, is it worth measuring how
long
> GetBookmarksWithTitlesMatching takes so that you easily tell differences
between
> the two field trials? Or will other autocomplete metrics provide coverage for
this already?

Good question.  This is already measured by Omnibox.ProviderTime.Bookmark.

--mark

https://codereview.chromium.org/184663002/diff/90001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_index.h (right):

https://codereview.chromium.org/184663002/diff/90001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_index.h:37: const std::string& languages);
On 2014/04/18 21:56:17, sky wrote:
> Document what languages is.

Done.

[sky, 2014-04-18 22:30:37]

LGTM then

[sky, 2014-04-18 22:31:02]

Sorry, not LGTM, looks like you still need to inject the boolean.

[Mark P, 2014-04-18 23:11:41]

On Fri, Apr 18, 2014 at 3:31 PM, <sky@chromium.org> wrote:

> Sorry, not LGTM, looks like you still need to inject the boolean.
>

What do you mean by inject the boolean?  Are you envisioning that I pass it
down to the BookmarkIndex constructor from somewhere outside
chrome/browser/bookmarks/...?
i.e.,
ChromeBrowserMainExtraPartsProfiles::
EnsureBrowserContextKeyedServiceFactoriesBuilt()
->
<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/boo...>
BookmarkModelFactory
->
BookmarkModel
->
BookmarkIndex
?

--mark


> https://codereview.chromium.org/184663002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2014-04-21 15:28:40]

On Fri, Apr 18, 2014 at 4:11 PM, Mark Pearson <mpearson@chromium.org> wrote:
>
> On Fri, Apr 18, 2014 at 3:31 PM, <sky@chromium.org> wrote:
>>
>> Sorry, not LGTM, looks like you still need to inject the boolean.
>
>
> What do you mean by inject the boolean?  Are you envisioning that I pass it
> down to the BookmarkIndex constructor from somewhere outside
> chrome/browser/bookmarks/...?

Yes. Make BookmarkModelFactory pass the boolean into the BookmarkModel
(BookmarkModel is going to be promoted to src/compnents, but not
BookmarkModelFactory).

  -Scott

> i.e.,
> ChromeBrowserMainExtraPartsProfiles::
> EnsureBrowserContextKeyedServiceFactoriesBuilt()
> ->
> BookmarkModelFactory
> ->
> BookmarkModel
> ->
> BookmarkIndex
> ?
>
> --mark
>
>>
>> https://codereview.chromium.org/184663002/
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mark P, 2014-04-21 21:33:53]

Scott,

I injected the boolean from BookmarkModelFactory as you requested.

This simplified bookmark_index_unittest and bookmark_provider_unittest because
those tests no longer need to configure a field trial.  On the other hand, I had
to touch a lot of tests that made BookmarkModels directly to specify the new
index_urls parameter to its constructor.

Please take a look.

--mark


On 2014/04/21 15:28:40, sky wrote:
> On Fri, Apr 18, 2014 at 4:11 PM, Mark Pearson <mailto:mpearson@chromium.org>
wrote:
> >
> > On Fri, Apr 18, 2014 at 3:31 PM, <mailto:sky@chromium.org> wrote:
> >>
> >> Sorry, not LGTM, looks like you still need to inject the boolean.
> >
> >
> > What do you mean by inject the boolean?  Are you envisioning that I pass it
> > down to the BookmarkIndex constructor from somewhere outside
> > chrome/browser/bookmarks/...?
> 
> Yes. Make BookmarkModelFactory pass the boolean into the BookmarkModel
> (BookmarkModel is going to be promoted to src/compnents, but not
> BookmarkModelFactory).
> 
>   -Scott
> 
> > i.e.,
> > ChromeBrowserMainExtraPartsProfiles::
> > EnsureBrowserContextKeyedServiceFactoriesBuilt()
> > ->
> > BookmarkModelFactory
> > ->
> > BookmarkModel
> > ->
> > BookmarkIndex
> > ?
> >
> > --mark
> >
> >>
> >> https://codereview.chromium.org/184663002/
> >
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[sky, 2014-04-21 23:09:01]

LGTM

https://codereview.chromium.org/184663002/diff/130001/chrome/browser/bookmark...
File chrome/browser/bookmarks/bookmark_index.h (right):

https://codereview.chromium.org/184663002/diff/130001/chrome/browser/bookmark...
chrome/browser/bookmarks/bookmark_index.h:141: std::string languages_;
nit: const.

https://codereview.chromium.org/184663002/diff/130001/chrome/browser/bookmark...
File chrome/browser/bookmarks/bookmark_model.h (right):

https://codereview.chromium.org/184663002/diff/130001/chrome/browser/bookmark...
chrome/browser/bookmarks/bookmark_model.h:63: explicit BookmarkModel(Profile*
profile, bool index_urls);
Remove explicit

[Mark P, 2014-04-21 23:41:35]

https://codereview.chromium.org/184663002/diff/130001/chrome/browser/bookmark...
File chrome/browser/bookmarks/bookmark_index.h (right):

https://codereview.chromium.org/184663002/diff/130001/chrome/browser/bookmark...
chrome/browser/bookmarks/bookmark_index.h:141: std::string languages_;
On 2014/04/21 23:09:01, sky wrote:
> nit: const.

Done.

https://codereview.chromium.org/184663002/diff/130001/chrome/browser/bookmark...
File chrome/browser/bookmarks/bookmark_model.h (right):

https://codereview.chromium.org/184663002/diff/130001/chrome/browser/bookmark...
chrome/browser/bookmarks/bookmark_model.h:63: explicit BookmarkModel(Profile*
profile, bool index_urls);
On 2014/04/21 23:09:01, sky wrote:
> Remove explicit

Done.

[Mark P, 2014-04-21 23:42:00]

The CQ bit was checked by mpearson@chromium.org

[commit-bot: I haz the power, 2014-04-21 23:42:17]

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

[commit-bot: I haz the power, 2014-04-22 01:14:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-22 01:14:22]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[Mark P, 2014-04-22 04:23:42]

The CQ bit was checked by mpearson@chromium.org

[commit-bot: I haz the power, 2014-04-22 04:24:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpearson@chromium.org/184663002/170001

[commit-bot: I haz the power, 2014-04-22 05:45:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-22 05:45:38]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[Mark P, 2014-04-22 14:08:39]

The CQ bit was checked by mpearson@chromium.org

[commit-bot: I haz the power, 2014-04-22 14:08:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpearson@chromium.org/184663002/170001

[commit-bot: I haz the power, 2014-04-22 14:09:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-22 14:09:19]

Failed to apply patch for chrome/browser/autocomplete/bookmark_provider.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/autocomplete/bookmark_provider.cc
  Hunk #10 FAILED at 258.
  1 out of 12 hunks FAILED -- saving rejects to file
chrome/browser/autocomplete/bookmark_provider.cc.rej

Patch:       chrome/browser/autocomplete/bookmark_provider.cc
Index: chrome/browser/autocomplete/bookmark_provider.cc
diff --git a/chrome/browser/autocomplete/bookmark_provider.cc
b/chrome/browser/autocomplete/bookmark_provider.cc
index
15b36ca69456ffcc22ec6ab05f5e985df2068a87..4dffbcfcfe71046a577534827a6cffdd1431cb5e
100644
--- a/chrome/browser/autocomplete/bookmark_provider.cc
+++ b/chrome/browser/autocomplete/bookmark_provider.cc
@@ -15,12 +15,13 @@
 #include "chrome/browser/autocomplete/url_prefix.h"
 #include "chrome/browser/bookmarks/bookmark_model.h"
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
+#include "chrome/browser/omnibox/omnibox_field_trial.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/pref_names.h"
-#include "components/bookmarks/core/browser/bookmark_title_match.h"
+#include "components/bookmarks/core/browser/bookmark_match.h"
 #include "net/base/net_util.h"
 
-typedef std::vector<BookmarkTitleMatch> TitleMatches;
+typedef std::vector<BookmarkMatch> BookmarkMatches;
 
 // BookmarkProvider
------------------------------------------------------------
 
@@ -29,7 +30,8 @@ BookmarkProvider::BookmarkProvider(
     Profile* profile)
     : AutocompleteProvider(listener, profile,
                            AutocompleteProvider::TYPE_BOOKMARK),
-      bookmark_model_(NULL) {
+      bookmark_model_(NULL),
+      score_using_url_matches_(OmniboxFieldTrial::BookmarksIndexURLsValue()) {
   if (profile) {
     bookmark_model_ = BookmarkModelFactory::GetForProfile(profile);
     languages_ = profile_->GetPrefs()->GetString(prefs::kAcceptLanguages);
@@ -43,8 +45,7 @@ void BookmarkProvider::Start(const AutocompleteInput& input,
   matches_.clear();
 
   if (input.text().empty() ||
-      ((input.type() != AutocompleteInput::UNKNOWN) &&
-       (input.type() != AutocompleteInput::QUERY)))
+      (input.type() == AutocompleteInput::FORCED_QUERY))
     return;
 
   DoAutocomplete(input,
@@ -59,12 +60,12 @@ void BookmarkProvider::DoAutocomplete(const
AutocompleteInput& input,
   if (!bookmark_model_)
     return;
 
-  TitleMatches matches;
+  BookmarkMatches matches;
   // Retrieve enough bookmarks so that we have a reasonable probability of
   // suggesting the one that the user desires.
   const size_t kMaxBookmarkMatches = 50;
 
-  // GetBookmarksWithTitlesMatching returns bookmarks matching the user's
+  // GetBookmarksMatching returns bookmarks matching the user's
   // search terms using the following rules:
   //  - The search text is broken up into search terms. Each term is searched
   //    for separately.
@@ -79,26 +80,21 @@ void BookmarkProvider::DoAutocomplete(const
AutocompleteInput& input,
   //  - Multiple terms enclosed in quotes will require those exact words in
that
   //    exact order to match.
   //
-  // Note: GetBookmarksWithTitlesMatching() will never return a match span
-  // greater than the length of the title against which it is being matched,
-  // nor can those spans ever overlap because the match spans are coalesced
-  // for all matched terms.
-  //
-  // Please refer to the code for BookmarkIndex::GetBookmarksWithTitlesMatching
-  // for complete details of how title searches are performed against the
user's
+  // Please refer to the code for BookmarkIndex::GetBookmarksMatching for
+  // complete details of how searches are performed against the user's
   // bookmarks.
-  bookmark_model_->GetBookmarksWithTitlesMatching(input.text(),
-                                                  kMaxBookmarkMatches,
-                                                  &matches);
+  bookmark_model_->GetBookmarksMatching(input.text(),
+                                        kMaxBookmarkMatches,
+                                        &matches);
   if (matches.empty())
     return;  // There were no matches.
   AutocompleteInput fixed_up_input(input);
   FixupUserInput(&fixed_up_input);
-  for (TitleMatches::const_iterator i = matches.begin(); i != matches.end();
+  for (BookmarkMatches::const_iterator i = matches.begin(); i != matches.end();
        ++i) {
     // Create and score the AutocompleteMatch. If its score is 0 then the
     // match is discarded.
-    AutocompleteMatch match(TitleMatchToACMatch(input, fixed_up_input, *i));
+    AutocompleteMatch match(BookmarkMatchToACMatch(input, fixed_up_input, *i));
     if (match.relevance > 0)
       matches_.push_back(match);
   }
@@ -122,12 +118,8 @@ namespace {
 // for_each helper functor that calculates a match factor for each query term
 // when calculating the final score.
 //
-// Calculate a 'factor' from 0.0 to 1.0 based on 1) how much of the bookmark's
-// title the term matches, and 2) where the match is positioned within the
-// bookmark's title. A full length match earns a 1.0. A half-length match earns
-// at most a 0.5 and at least a 0.25. A single character match against a title
-// that is 100 characters long where the match is at the first character will
-// earn a 0.01 and at the last character will earn a 0.0001.
+// Calculate a 'factor' from 0 to the bookmark's title length for a match
+// based on 1) how many characters match and 2) where the match is positioned.
 class ScoringFunctor {
  public:
   // |title_length| is the length of the bookmark title against which this
@@ -139,7 +131,7 @@ class ScoringFunctor {
 
   void operator()(const query_parser::Snippet::MatchPosition& match) {
     double term_length = static_cast<double>(match.second - match.first);
-    scoring_factor_ += term_length / title_length_ *
+    scoring_factor_ += term_length *
         (title_length_ - match.first) / title_length_;
   }
 
@@ -152,32 +144,44 @@ class ScoringFunctor {
 
 }  // namespace
 
-AutocompleteMatch BookmarkProvider::TitleMatchToACMatch(
+AutocompleteMatch BookmarkProvider::BookmarkMatchToACMatch(
     const AutocompleteInput& input,
     const AutocompleteInput& fixed_up_input,
-    const BookmarkTitleMatch& title_match) {
+    const BookmarkMatch& bookmark_match) {
   // The AutocompleteMatch we construct is non-deletable because the only
   // way to support this would be to delete the underlying bookmark, which is
   // unlikely to be what the user intends.
   AutocompleteMatch match(this, 0, false,
                           AutocompleteMatchType::BOOKMARK_TITLE);
-  const base::string16& title(title_match.node->GetTitle());
-  DCHECK(!title.empty());
-
-  const GURL& url(title_match.node->url());
+  base::string16 title(bookmark_match.node->GetTitle());
+  const GURL& url(bookmark_match.node->url());
   const base::string16& url_utf16 = base::UTF8ToUTF16(url.spec());
-  size_t match_start, inline_autocomplete_offset;
-  URLPrefix::ComputeMatchStartAndInlineAutocompleteOffset(
-      input, fixed_up_input, false, url_utf16, &match_start,
-      &inline_autocomplete_offset);
+  size_t inline_autocomplete_offset = URLPrefix::GetInlineAutocompleteOffset(
+      input, fixed_up_input, false, url_utf16);
   match.destination_url = url;
+  const size_t match_start = bookmark_match.url_match_positions.empty() ?
+      0 : bookmark_match.url_match_positions[0].first;
   const bool trim_http = !AutocompleteInput::HasHTTPScheme(input.text()) &&
       ((match_start == base::string16::npos) || (match_start != 0));
-  match.contents = net::FormatUrl(url, languages_,
+  std::vector<size_t> offsets = BookmarkMatch::OffsetsFromMatchPositions(
+      bookmark_match.url_match_positions);
+  // In addition to knowing how |offsets| is transformed, we need to know how
+  // |inline_autocomplete_offset| is transformed.  We add it to the end of
+  // |offsets|, compute how everything is transformed, then remove it from the
+  // end.
+  offsets.push_back(inline_autocomplete_offset);
+  match.contents = net::FormatUrlWithOffsets(url, languages_,
       net::kFormatUrlOmitAll & ~(trim_http ? 0 : net::kFormatUrlOmitHTTP),
-      net::UnescapeRule::SPACES, NULL, NULL, &inline_autocomplete_offset);
-  match.contents_class.push_back(
-      ACMatchClassification(0, ACMatchClassification::URL));
+      net::UnescapeRule::SPACES, NULL, NULL, &offsets);
+  inline_autocomplete_offset = offsets.back();
+  offsets.pop_back();
+  BookmarkMatch::MatchPositions new_url_match_positions =
+      BookmarkMatch::ReplaceOffsetsInMatchPositions(
+          bookmark_match.url_match_positions, offsets);
+  match.contents_class =
+      ClassificationsFromMatch(new_url_match_positions,
+                               match.contents.size(),
+                               true);
   match.fill_into_edit =
       AutocompleteInput::FormattedStringWithEquivalentMeaning(url,
                                                               match.contents);
@@ -195,48 +199,56 @@ AutocompleteMatch BookmarkProvider::TitleMatchToACMatch(
   }
   match.description = title;
   match.description_class =
-      ClassificationsFromMatch(title_match.match_positions,
-                               match.description.size());
+      ClassificationsFromMatch(bookmark_match.title_match_positions,
+                               match.description.size(),
+                               false);
   match.starred = true;
 
   // Summary on how a relevance score is determined for the match:
   //
-  // For each term matching within the bookmark's title (as given by the set of
-  // Snippet::MatchPositions) calculate a 'factor', sum up those factors, then
-  // use the sum to figure out a value between the base score and the maximum
-  // score.
+  // For each match within the bookmark's title or URL (or both), calculate a
+  // 'factor', sum up those factors, then use the sum to figure out a val…
(message too large)

[Mark P, 2014-04-22 15:05:20]

The CQ bit was checked by mpearson@chromium.org

[commit-bot: I haz the power, 2014-04-22 15:06:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpearson@chromium.org/184663002/190001

[commit-bot: I haz the power, 2014-04-22 18:04:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-22 18:04:39]

Failed to apply patch for chrome/browser/history/scored_history_match.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/history/scored_history_match.cc
  Hunk #1 FAILED at 19.
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/history/scored_history_match.cc.rej

Patch:       chrome/browser/history/scored_history_match.cc
Index: chrome/browser/history/scored_history_match.cc
diff --git a/chrome/browser/history/scored_history_match.cc
b/chrome/browser/history/scored_history_match.cc
index
83bd7865a2670d3f0dc25fa4cdf9533733ada5db..bc700dc8ee39cd34cbecf87acf5218a33ef37833
100644
--- a/chrome/browser/history/scored_history_match.cc
+++ b/chrome/browser/history/scored_history_match.cc
@@ -19,6 +19,7 @@
 #include "chrome/browser/autocomplete/history_url_provider.h"
 #include "chrome/browser/autocomplete/url_prefix.h"
 #include "chrome/browser/bookmarks/bookmark_service.h"
+#include "chrome/browser/bookmarks/bookmark_utils.h"
 #include "chrome/browser/omnibox/omnibox_field_trial.h"
 #include "chrome/common/chrome_switches.h"
 #include "content/public/browser/browser_thread.h"
@@ -67,8 +68,8 @@ ScoredHistoryMatch::ScoredHistoryMatch(
 
   // Figure out where each search term appears in the URL and/or page title
   // so that we can score as well as provide autocomplete highlighting.
-  base::string16 url = CleanUpUrlForMatching(gurl, languages);
-  base::string16 title = CleanUpTitleForMatching(row.title());
+  base::string16 url = bookmark_utils::CleanUpUrlForMatching(gurl, languages);
+  base::string16 title = bookmark_utils::CleanUpTitleForMatching(row.title());
   int term_num = 0;
   for (String16Vector::const_iterator iter = terms.begin(); iter !=
terms.end();
        ++iter, ++term_num) {

[Mark P, 2014-04-22 18:32:28]

The CQ bit was checked by mpearson@chromium.org

[commit-bot: I haz the power, 2014-04-22 18:33:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpearson@chromium.org/184663002/210001

[commit-bot: I haz the power, 2014-04-22 21:16:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-22 21:16:43]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_x64_rel

[Mark P, 2014-04-22 21:34:44]

The CQ bit was checked by mpearson@chromium.org

[commit-bot: I haz the power, 2014-04-22 21:35:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpearson@chromium.org/184663002/210001

[Mark P, 2014-04-22 23:44:56]

The CQ bit was unchecked by mpearson@chromium.org

[Mark P, 2014-04-22 23:50:09]

The CQ bit was checked by mpearson@chromium.org

[commit-bot: I haz the power, 2014-04-22 23:50:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpearson@chromium.org/184663002/230001

[commit-bot: I haz the power, 2014-04-22 23:55:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-22 23:55:01]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg
  tryserver.chromium on linux_chromium_rel

[Mark P, 2014-04-23 02:39:36]

The CQ bit was checked by mpearson@chromium.org

[commit-bot: I haz the power, 2014-04-23 02:40:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpearson@chromium.org/184663002/230001

[commit-bot: I haz the power, 2014-04-23 04:28:11]

Change committed as 265539