Omnibox: Highlight Matches in URLs Properly

Omnibox: Highlight Matches in URLs Properly

Previously we'd match the omnibox input against a cleaned-up version of
the URL.  Precisely, we'd remove the escaping and interpret as UTF8 (and
lower-case it) so non-Latin characters can be matched correctly.  This is
all well and good, but then we'd take the offsets of those matches
and use them to highlight segments of the original URL (properly escaped).
Oops.  At best this caused bad highlighting; at middle, it caused weirdness
as we attempted to highlight the middle of a multi-string character;
at worst, it'd cause crashes because our offsets calculation would put
us in no-man's-land.

This fixes all that. :-)

Precisely, we keep track of the adjustments made during cleaning.  We
take the offsets where terms matched and apply them through the
transformation in reverse.

TBR=sky
(as usual for history and bookmarks stuff that just change internals relative to how they behave with the omnibox)

BUG=252630, 359270
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266630

[Mark P, 2014-04-23 22:45:37]

The culmination of the big FormatUrl() changelist.  A nice visual improvement
for our non-Latin users.

Please take a look.

--mark

[Peter Kasting, 2014-04-23 23:18:01]

I'm a little confused at the sequence of adjusting and unadjusting offsets.  Are
we formatting URLs, searching within them for matches, unadjusting those
offsets, then passing the offsets back to a caller who's going to format and
(forward-)adjust the offset again?  That seems lame if so.

https://codereview.chromium.org/255423002/diff/20001/base/strings/utf_offset_...
File base/strings/utf_offset_string_conversions.h (right):

https://codereview.chromium.org/255423002/diff/20001/base/strings/utf_offset_...
base/strings/utf_offset_string_conversions.h:64: size_t*
offset_for_unadjustment);
Nit: Either name this |offset|, or name the arg to AdjustOffset() above
|offset_for_adjustment|

https://codereview.chromium.org/255423002/diff/20001/chrome/browser/autocompl...
File chrome/browser/autocomplete/history_quick_provider.cc (right):

https://codereview.chromium.org/255423002/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_quick_provider.cc:253:
AutocompleteInput::FormattedStringWithEquivalentMeaning(info.url(),
Nit: Move this onto the next line so all lines of args can start at the same
position

https://codereview.chromium.org/255423002/diff/20001/chrome/browser/autocompl...
File chrome/browser/autocomplete/history_quick_provider_unittest.cc (right):

https://codereview.chromium.org/255423002/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_quick_provider_unittest.cc:390:
EXPECT_LE(5u, contents_class.size());
Nit: For EXPECT_XXX other than EQ and NE, write as (actual, expected) so the
expression reads like "expect a > 5" instead of "expect 5 < a".  In these cases,
the printed output won't assume that the first arg is the "expected" value, and
reading the code is usually easier in "natural" order.

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

https://codereview.chromium.org/255423002/diff/20001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_index.cc:98: node->url(), languages_,
&adjustments));
Maybe we should be able to pass NULL for the last arg here.  That would clean up
two places in this file.

https://codereview.chromium.org/255423002/diff/20001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_utils.h (right):

https://codereview.chromium.org/255423002/diff/20001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_utils.h:115: // the URL spec underwent to
become the return value.  This unescaping makes
"that make the URL spec underwent"?

https://codereview.chromium.org/255423002/diff/20001/chrome/browser/history/s...
File chrome/browser/history/scored_history_match.cc (right):

https://codereview.chromium.org/255423002/diff/20001/chrome/browser/history/s...
chrome/browser/history/scored_history_match.cc:73:
bookmark_utils::CleanUpUrlForMatching(gurl, languages, &adjustments);
Nit: Another place passing in NULL would be nice

https://codereview.chromium.org/255423002/diff/20001/chrome/browser/history/u...
File chrome/browser/history/url_index_private_data.cc (right):

https://codereview.chromium.org/255423002/diff/20001/chrome/browser/history/u...
chrome/browser/history/url_index_private_data.cc:763:
bookmark_utils::CleanUpUrlForMatching(gurl, languages, &adjustments);
Nit: And another 2 in this file

[Mark P, 2014-04-23 23:36:14]

On Wed, Apr 23, 2014 at 4:18 PM, <pkasting@chromium.org> wrote:

> I'm a little confused at the sequence of adjusting and unadjusting
> offsets.  Are
> we formatting URLs, searching within them for matches, unadjusting those
> offsets, then passing the offsets back to a caller who's going to format
> and
> (forward-)adjust the offset again?  That seems lame if so.
>

That roughly right, but the important distinction that is we may not format
the URLs in the same way both times.

--mark


>
>
> https://codereview.chromium.org/255423002/diff/20001/base/
> strings/utf_offset_string_conversions.h
> File base/strings/utf_offset_string_conversions.h (right):
>
> https://codereview.chromium.org/255423002/diff/20001/base/
> strings/utf_offset_string_conversions.h#newcode64
> base/strings/utf_offset_string_conversions.h:64: size_t*
> offset_for_unadjustment);
> Nit: Either name this |offset|, or name the arg to AdjustOffset() above
> |offset_for_adjustment|
>
> https://codereview.chromium.org/255423002/diff/20001/
> chrome/browser/autocomplete/history_quick_provider.cc
> File chrome/browser/autocomplete/history_quick_provider.cc (right):
>
> https://codereview.chromium.org/255423002/diff/20001/
> chrome/browser/autocomplete/history_quick_provider.cc#newcode253
> chrome/browser/autocomplete/history_quick_provider.cc:253:
> AutocompleteInput::FormattedStringWithEquivalentMeaning(info.url(),
> Nit: Move this onto the next line so all lines of args can start at the
> same position
>
> https://codereview.chromium.org/255423002/diff/20001/
> chrome/browser/autocomplete/history_quick_provider_unittest.cc
> File chrome/browser/autocomplete/history_quick_provider_unittest.cc
> (right):
>
> https://codereview.chromium.org/255423002/diff/20001/
> chrome/browser/autocomplete/history_quick_provider_unittest.cc#newcode390
> chrome/browser/autocomplete/history_quick_provider_unittest.cc:390:
> EXPECT_LE(5u, contents_class.size());
> Nit: For EXPECT_XXX other than EQ and NE, write as (actual, expected) so
> the expression reads like "expect a > 5" instead of "expect 5 < a".  In
> these cases, the printed output won't assume that the first arg is the
> "expected" value, and reading the code is usually easier in "natural"
> order.
>
> https://codereview.chromium.org/255423002/diff/20001/
> chrome/browser/bookmarks/bookmark_index.cc
> File chrome/browser/bookmarks/bookmark_index.cc (right):
>
> https://codereview.chromium.org/255423002/diff/20001/
> chrome/browser/bookmarks/bookmark_index.cc#newcode98
> chrome/browser/bookmarks/bookmark_index.cc:98: node->url(), languages_,
> &adjustments));
> Maybe we should be able to pass NULL for the last arg here.  That would
> clean up two places in this file.
>
> https://codereview.chromium.org/255423002/diff/20001/
> chrome/browser/bookmarks/bookmark_utils.h
> File chrome/browser/bookmarks/bookmark_utils.h (right):
>
> https://codereview.chromium.org/255423002/diff/20001/
> chrome/browser/bookmarks/bookmark_utils.h#newcode115
> chrome/browser/bookmarks/bookmark_utils.h:115: // the URL spec underwent
> to become the return value.  This unescaping makes
> "that make the URL spec underwent"?
>
> https://codereview.chromium.org/255423002/diff/20001/
> chrome/browser/history/scored_history_match.cc
> File chrome/browser/history/scored_history_match.cc (right):
>
> https://codereview.chromium.org/255423002/diff/20001/
> chrome/browser/history/scored_history_match.cc#newcode73
> chrome/browser/history/scored_history_match.cc:73:
> bookmark_utils::CleanUpUrlForMatching(gurl, languages, &adjustments);
> Nit: Another place passing in NULL would be nice
>
> https://codereview.chromium.org/255423002/diff/20001/
> chrome/browser/history/url_index_private_data.cc
> File chrome/browser/history/url_index_private_data.cc (right):
>
> https://codereview.chromium.org/255423002/diff/20001/
> chrome/browser/history/url_index_private_data.cc#newcode763
> chrome/browser/history/url_index_private_data.cc:763:
> bookmark_utils::CleanUpUrlForMatching(gurl, languages, &adjustments);
> Nit: And another 2 in this file
>
> https://codereview.chromium.org/255423002/
>

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

[Peter Kasting, 2014-04-23 23:37:43]

On 2014/04/23 23:36:14, Mark P wrote:
> On Wed, Apr 23, 2014 at 4:18 PM, <mailto:pkasting@chromium.org> wrote:
> 
> > I'm a little confused at the sequence of adjusting and unadjusting
> > offsets.  Are
> > we formatting URLs, searching within them for matches, unadjusting those
> > offsets, then passing the offsets back to a caller who's going to format
> > and
> > (forward-)adjust the offset again?  That seems lame if so.
> >
> 
> That roughly right, but the important distinction that is we may not format
> the URLs in the same way both times.

OK, but that seems kind of strange -- why wouldn't we want to format things the
same for display to the user as well as for searching for matches?

[Mark P, 2014-04-23 23:41:47]

On Wed, Apr 23, 2014 at 4:37 PM, <pkasting@chromium.org> wrote:

> On 2014/04/23 23:36:14, Mark P wrote:
>
>  On Wed, Apr 23, 2014 at 4:18 PM, <mailto:pkasting@chromium.org> wrote:
>>
>
>  > I'm a little confused at the sequence of adjusting and unadjusting
>> > offsets.  Are
>> > we formatting URLs, searching within them for matches, unadjusting those
>> > offsets, then passing the offsets back to a caller who's going to format
>> > and
>> > (forward-)adjust the offset again?  That seems lame if so.
>> >
>>
>
>  That roughly right, but the important distinction that is we may not
>> format
>> the URLs in the same way both times.
>>
>
> OK, but that seems kind of strange -- why wouldn't we want to format
> things the
> same for display to the user as well as for searching for matches?
>

The obvious example is, for instance, dropping http.  We format the string
and keep http:// so we can match it if necessary.  Then, later, if there
was no match in http itself, then for display purposes we format the URL
without it.

--mark


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

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

[Peter Kasting, 2014-04-23 23:44:30]

On 2014/04/23 23:41:47, Mark P wrote:
> On Wed, Apr 23, 2014 at 4:37 PM, <mailto:pkasting@chromium.org> wrote:
> 
> > On 2014/04/23 23:36:14, Mark P wrote:
> >
> >  On Wed, Apr 23, 2014 at 4:18 PM, <mailto:pkasting@chromium.org> wrote:
> >>
> >
> >  > I'm a little confused at the sequence of adjusting and unadjusting
> >> > offsets.  Are
> >> > we formatting URLs, searching within them for matches, unadjusting those
> >> > offsets, then passing the offsets back to a caller who's going to format
> >> > and
> >> > (forward-)adjust the offset again?  That seems lame if so.
> >> >
> >>
> >
> >  That roughly right, but the important distinction that is we may not
> >> format
> >> the URLs in the same way both times.
> >>
> >
> > OK, but that seems kind of strange -- why wouldn't we want to format
> > things the
> > same for display to the user as well as for searching for matches?
> >
> 
> The obvious example is, for instance, dropping http.  We format the string
> and keep http:// so we can match it if necessary.  Then, later, if there
> was no match in http itself, then for display purposes we format the URL
> without it.

That makes sense.

Is there some place where all this can be clearly laid out in a comment?  I feel
like the logic is spread among enough different pieces of the pipeline that at
any local level, it's hard to understand why we're doing what we are.  Maybe
each relevant class needs to have comments about not just what their functions
do but in what context they expect to be used, and how that motivates their
behavior.

[Mark P, 2014-04-24 13:26:56]

On Wed, Apr 23, 2014 at 4:44 PM, <pkasting@chromium.org> wrote:

> On 2014/04/23 23:41:47, Mark P wrote:
>
>  On Wed, Apr 23, 2014 at 4:37 PM, <mailto:pkasting@chromium.org> wrote:
>>
>
>  > On 2014/04/23 23:36:14, Mark P wrote:
>> >
>> >  On Wed, Apr 23, 2014 at 4:18 PM, <mailto:pkasting@chromium.org> wrote:
>> >>
>> >
>> >  > I'm a little confused at the sequence of adjusting and unadjusting
>> >> > offsets.  Are
>> >> > we formatting URLs, searching within them for matches, unadjusting
>> those
>> >> > offsets, then passing the offsets back to a caller who's going to
>> format
>> >> > and
>> >> > (forward-)adjust the offset again?  That seems lame if so.
>> >> >
>> >>
>> >
>> >  That roughly right, but the important distinction that is we may not
>> >> format
>> >> the URLs in the same way both times.
>> >>
>> >
>> > OK, but that seems kind of strange -- why wouldn't we want to format
>> > things the
>> > same for display to the user as well as for searching for matches?
>> >
>>
>
>  The obvious example is, for instance, dropping http.  We format the string
>> and keep http:// so we can match it if necessary.  Then, later, if there
>> was no match in http itself, then for display purposes we format the URL
>> without it.
>>
>
> That makes sense.
>
> Is there some place where all this can be clearly laid out in a comment?


Yes.  Good idea.  CleanupUrlForMatching() can explain most of this (or at
least the most complex part).  I'll add a comment there.

--mark


> I feel
> like the logic is spread among enough different pieces of the pipeline
> that at
> any local level, it's hard to understand why we're doing what we are.
>  Maybe
> each relevant class needs to have comments about not just what their
> functions
> do but in what context they expect to be used, and how that motivates their
> behavior.
>
> https://codereview.chromium.org/255423002/
>

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

[Mark P, 2014-04-24 14:05:02]

https://codereview.chromium.org/255423002/diff/20001/base/strings/utf_offset_...
File base/strings/utf_offset_string_conversions.h (right):

https://codereview.chromium.org/255423002/diff/20001/base/strings/utf_offset_...
base/strings/utf_offset_string_conversions.h:64: size_t*
offset_for_unadjustment);
On 2014/04/23 23:18:01, Peter Kasting wrote:
> Nit: Either name this |offset|, or name the arg to AdjustOffset() above
> |offset_for_adjustment|

Did the former.

https://codereview.chromium.org/255423002/diff/20001/chrome/browser/autocompl...
File chrome/browser/autocomplete/history_quick_provider.cc (right):

https://codereview.chromium.org/255423002/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_quick_provider.cc:253:
AutocompleteInput::FormattedStringWithEquivalentMeaning(info.url(),
On 2014/04/23 23:18:01, Peter Kasting wrote:
> Nit: Move this onto the next line so all lines of args can start at the same
> position

Done.  Also adjusted FormatUrl() spacing around here.

https://codereview.chromium.org/255423002/diff/20001/chrome/browser/autocompl...
File chrome/browser/autocomplete/history_quick_provider_unittest.cc (right):

https://codereview.chromium.org/255423002/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_quick_provider_unittest.cc:390:
EXPECT_LE(5u, contents_class.size());
On 2014/04/23 23:18:01, Peter Kasting wrote:
> Nit: For EXPECT_XXX other than EQ and NE, write as (actual, expected) so the
> expression reads like "expect a > 5" instead of "expect 5 < a".  In these
cases,
> the printed output won't assume that the first arg is the "expected" value,
and
> reading the code is usually easier in "natural" order.

Good tip / reminder.  Done.

(This is the only place that needs to change in this changelist, right?)

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

https://codereview.chromium.org/255423002/diff/20001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_index.cc:98: node->url(), languages_,
&adjustments));
On 2014/04/23 23:18:01, Peter Kasting wrote:
> Maybe we should be able to pass NULL for the last arg here.  That would clean
up
> two places in this file.

Done.

https://codereview.chromium.org/255423002/diff/20001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_utils.h (right):

https://codereview.chromium.org/255423002/diff/20001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_utils.h:115: // the URL spec underwent to
become the return value.  This unescaping makes
On 2014/04/23 23:18:01, Peter Kasting wrote:
> "that make the URL spec underwent"?

Done.

Also rewrite much of the test of the comment.

https://codereview.chromium.org/255423002/diff/20001/chrome/browser/history/s...
File chrome/browser/history/scored_history_match.cc (right):

https://codereview.chromium.org/255423002/diff/20001/chrome/browser/history/s...
chrome/browser/history/scored_history_match.cc:73:
bookmark_utils::CleanUpUrlForMatching(gurl, languages, &adjustments);
On 2014/04/23 23:18:01, Peter Kasting wrote:
> Nit: Another place passing in NULL would be nice

This one actually gets used below.

https://codereview.chromium.org/255423002/diff/20001/chrome/browser/history/u...
File chrome/browser/history/url_index_private_data.cc (right):

https://codereview.chromium.org/255423002/diff/20001/chrome/browser/history/u...
chrome/browser/history/url_index_private_data.cc:763:
bookmark_utils::CleanUpUrlForMatching(gurl, languages, &adjustments);
On 2014/04/23 23:18:01, Peter Kasting wrote:
> Nit: And another 2 in this file

Done.

[Peter Kasting, 2014-04-24 21:00:51]

LGTM

https://codereview.chromium.org/255423002/diff/40001/chrome/browser/autocompl...
File chrome/browser/autocomplete/history_quick_provider_unittest.cc (right):

https://codereview.chromium.org/255423002/diff/40001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_quick_provider_unittest.cc:387: // five
offsets to be correct--in this example tthese cover at least one
Nit: tthese -> these

https://codereview.chromium.org/255423002/diff/40001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_quick_provider_unittest.cc:389:
EXPECT_GE(arraysize(expected_offsets), contents_class.size());
I think you want to reverse the ordering on this EXPECT as well (since you want
to check that contents_class.size() is no larger than the expected_offsets list)

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

https://codereview.chromium.org/255423002/diff/40001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_utils.cc:419: NULL, NULL, (adjustments !=
NULL) ? adjustments : &tmp_adjustments));
Nit: Could also just write "adjustments ? ..."

https://codereview.chromium.org/255423002/diff/40001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_utils.h (right):

https://codereview.chromium.org/255423002/diff/40001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_utils.h:123: // string.
You've expanded the comment here, but by adding more "how" rather than more
"why".  We can probably assume that callers can figure out _how_ to reverse
transformations, but it would be nice to be clear why callers would find
themselves needing to calculate the positions of matches in the original string
to begin with, i.e. say something about how and why callers may need to
re-format the URL (and display corresponding matches) differently than this
function formats it.

[Mark P, 2014-04-24 23:05:17]

https://codereview.chromium.org/255423002/diff/40001/chrome/browser/autocompl...
File chrome/browser/autocomplete/history_quick_provider_unittest.cc (right):

https://codereview.chromium.org/255423002/diff/40001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_quick_provider_unittest.cc:387: // five
offsets to be correct--in this example tthese cover at least one
On 2014/04/24 21:00:52, Peter Kasting wrote:
> Nit: tthese -> these

Done.

https://codereview.chromium.org/255423002/diff/40001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_quick_provider_unittest.cc:389:
EXPECT_GE(arraysize(expected_offsets), contents_class.size());
On 2014/04/24 21:00:52, Peter Kasting wrote:
> I think you want to reverse the ordering on this EXPECT as well (since you
want
> to check that contents_class.size() is no larger than the expected_offsets
list)

Done.

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

https://codereview.chromium.org/255423002/diff/40001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_utils.cc:419: NULL, NULL, (adjustments !=
NULL) ? adjustments : &tmp_adjustments));
On 2014/04/24 21:00:52, Peter Kasting wrote:
> Nit: Could also just write "adjustments ? ..."

Normally I tend to prefer the way I wrote it.
However, in this case the style in this file is to omit the NULL.
Now corrected.

https://codereview.chromium.org/255423002/diff/40001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_utils.h (right):

https://codereview.chromium.org/255423002/diff/40001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_utils.h:123: // string.
On 2014/04/24 21:00:52, Peter Kasting wrote:
> You've expanded the comment here, but by adding more "how" rather than more
> "why".  We can probably assume that callers can figure out _how_ to reverse
> transformations, but it would be nice to be clear why callers would find
> themselves needing to calculate the positions of matches in the original
string
> to begin with, i.e. say something about how and why callers may need to
> re-format the URL (and display corresponding matches) differently than this
> function formats it.

Done.

[Mark P, 2014-04-24 23:18:19]

The CQ bit was checked by mpearson@chromium.org

[commit-bot: I haz the power, 2014-04-24 23:21:45]

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

[commit-bot: I haz the power, 2014-04-24 23:30:38]

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

[commit-bot: I haz the power, 2014-04-24 23:30:38]

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

[Mark P, 2014-04-25 00:03:01]

willchan, can please approve the change to base/strings/...?  pkasting has
already reviewed this.

thanks,
mark

[willchan no longer on Chromium, 2014-04-25 00:07:20]

Only have some quick questions.

https://codereview.chromium.org/255423002/diff/60001/base/strings/utf_offset_...
File base/strings/utf_offset_string_conversions.cc (right):

https://codereview.chromium.org/255423002/diff/60001/base/strings/utf_offset_...
base/strings/utf_offset_string_conversions.cc:40: int adjustment = 0;
QQ: Why did this switch to int? Also, is it impossible to overflow? I don't see
any overflow checks.

[Mark P, 2014-04-25 02:29:02]

https://codereview.chromium.org/255423002/diff/60001/base/strings/utf_offset_...
File base/strings/utf_offset_string_conversions.cc (right):

https://codereview.chromium.org/255423002/diff/60001/base/strings/utf_offset_...
base/strings/utf_offset_string_conversions.cc:40: int adjustment = 0;
On 2014/04/25 00:07:20, willchan wrote:
> QQ: Why did this switch to int?

Although it doesn't generally happen, it in theory could go negative.  Or at
least this function might as well support adjustments in which it could go
negative (i.e., that extend the length of a string).    In other words, int is
more correct.

> Also, is it impossible to overflow?

All the strings and adjustments involved here are on the order of 1-1000kb.  So,
no, it shouldn't overflow.  Indeed, the only way it could overflow is if we have
an original string longer than 2^32 characters long...  This is not something
I'm concerned about.

> I don't see any overflow checks.

[willchan no longer on Chromium, 2014-04-25 05:24:42]

lgtm

https://codereview.chromium.org/255423002/diff/60001/base/strings/utf_offset_...
File base/strings/utf_offset_string_conversions.cc (right):

https://codereview.chromium.org/255423002/diff/60001/base/strings/utf_offset_...
base/strings/utf_offset_string_conversions.cc:40: int adjustment = 0;
On 2014/04/25 02:29:02, Mark P wrote:
> On 2014/04/25 00:07:20, willchan wrote:
> > QQ: Why did this switch to int?
> 
> Although it doesn't generally happen, it in theory could go negative.  Or at
> least this function might as well support adjustments in which it could go
> negative (i.e., that extend the length of a string).    In other words, int is
> more correct.

Great.

> 
> > Also, is it impossible to overflow?
> 
> All the strings and adjustments involved here are on the order of 1-1000kb. 
So,
> no, it shouldn't overflow.  Indeed, the only way it could overflow is if we
have
> an original string longer than 2^32 characters long...  This is not something
> I'm concerned about.
> 
> > I don't see any overflow checks.

OK, I just saw a security bug earlier that missed this check so I am a bit more
sensitive to this now :) If you guys are confident about this, I'm fine with it.

[Mark P, 2014-04-25 17:09:15]

The CQ bit was checked by mpearson@chromium.org

[commit-bot: I haz the power, 2014-04-25 21:46:31]

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

[commit-bot: I haz the power, 2014-04-25 22:33:52]

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

[commit-bot: I haz the power, 2014-04-25 22:33:52]

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

[Mark P, 2014-04-26 03:47:42]

The CQ bit was checked by mpearson@chromium.org

[commit-bot: I haz the power, 2014-04-26 03:51:02]

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

[commit-bot: I haz the power, 2014-04-26 04:51:13]

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

[commit-bot: I haz the power, 2014-04-26 04:51:14]

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

[Mark P, 2014-04-28 00:32:12]

The CQ bit was checked by mpearson@chromium.org

[commit-bot: I haz the power, 2014-04-28 00:32:44]

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

[commit-bot: I haz the power, 2014-04-28 00:33:02]

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

[commit-bot: I haz the power, 2014-04-28 00:33:02]

Failed to apply patch for chrome/browser/bookmarks/bookmark_index.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/bookmarks/bookmark_index.cc
  Hunk #1 FAILED at 10.
  Hunk #2 succeeded at 114 (offset 22 lines).
  Hunk #3 succeeded at 130 (offset 22 lines).
  Hunk #4 succeeded at 215 (offset 10 lines).
  Hunk #5 succeeded at 239 (offset 10 lines).
  1 out of 5 hunks FAILED -- saving rejects to file
chrome/browser/bookmarks/bookmark_index.cc.rej

Patch:       chrome/browser/bookmarks/bookmark_index.cc
Index: chrome/browser/bookmarks/bookmark_index.cc
diff --git a/chrome/browser/bookmarks/bookmark_index.cc
b/chrome/browser/bookmarks/bookmark_index.cc
index
77c22d985990464467233e624eb214675a3a667b..28c24afe2a24f494a6001e63a88d78c81be34d5d
100644
--- a/chrome/browser/bookmarks/bookmark_index.cc
+++ b/chrome/browser/bookmarks/bookmark_index.cc
@@ -10,6 +10,7 @@
 
 #include "base/i18n/case_conversion.h"
 #include "base/strings/string16.h"
+#include "base/strings/utf_offset_string_conversions.h"
 #include "chrome/browser/bookmarks/bookmark_model.h"
 #include "chrome/browser/bookmarks/bookmark_utils.h"
 #include "chrome/browser/history/history_service.h"
@@ -92,8 +93,8 @@ void BookmarkIndex::Add(const BookmarkNode* node) {
   for (size_t i = 0; i < terms.size(); ++i)
     RegisterNode(terms[i], node);
   if (index_urls_) {
-    terms = ExtractQueryWords(
-        bookmark_utils::CleanUpUrlForMatching(node->url(), languages_));
+    terms = ExtractQueryWords(bookmark_utils::CleanUpUrlForMatching(
+        node->url(), languages_, NULL));
     for (size_t i = 0; i < terms.size(); ++i)
       RegisterNode(terms[i], node);
   }
@@ -108,8 +109,8 @@ void BookmarkIndex::Remove(const BookmarkNode* node) {
   for (size_t i = 0; i < terms.size(); ++i)
     UnregisterNode(terms[i], node);
   if (index_urls_) {
-    terms = ExtractQueryWords(
-        bookmark_utils::CleanUpUrlForMatching(node->url(), languages_));
+    terms = ExtractQueryWords(bookmark_utils::CleanUpUrlForMatching(
+        node->url(), languages_, NULL));
     for (size_t i = 0; i < terms.size(); ++i)
       UnregisterNode(terms[i], node);
   }
@@ -205,10 +206,10 @@ void BookmarkIndex::AddMatchToResults(
   const base::string16 lower_title =
       base::i18n::ToLower(Normalize(node->GetTitle()));
   parser->ExtractQueryWords(lower_title, &title_words);
+  base::OffsetAdjuster::Adjustments adjustments;
   if (index_urls_) {
-    parser->ExtractQueryWords(
-        bookmark_utils::CleanUpUrlForMatching(node->url(), languages_),
-        &url_words);
+    parser->ExtractQueryWords(bookmark_utils::CleanUpUrlForMatching(
+        node->url(), languages_, &adjustments), &url_words);
   }
   query_parser::Snippet::MatchPositions title_matches, url_matches;
   for (size_t i = 0; i < query_nodes.size(); ++i) {
@@ -229,8 +230,17 @@ void BookmarkIndex::AddMatchToResults(
     // TODO(mpearson): revise match positions appropriately.
     match.title_match_positions.swap(title_matches);
   }
-  if (index_urls_)
+  if (index_urls_) {
+    // Now that we're done processing this entry, correct the offsets of the
+    // matches in |url_matches| so they point to offsets in the original URL
+    // spec, not the cleaned-up URL string that we used for matching.
+    std::vector<size_t> offsets =
+        BookmarkMatch::OffsetsFromMatchPositions(url_matches);
+    base::OffsetAdjuster::UnadjustOffsets(adjustments, &offsets);
+    url_matches =
+        BookmarkMatch::ReplaceOffsetsInMatchPositions(url_matches, offsets);
     match.url_match_positions.swap(url_matches);
+  }
   match.node = node;
   results->push_back(match);
 }

[Mark P, 2014-04-28 14:09:35]

The CQ bit was checked by mpearson@chromium.org

[commit-bot: I haz the power, 2014-04-28 14:09:47]

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

[commit-bot: I haz the power, 2014-04-28 14:28:35]

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

[commit-bot: I haz the power, 2014-04-28 14:28:35]

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

[Mark P, 2014-04-28 14:36:17]

The CQ bit was checked by mpearson@chromium.org

[commit-bot: I haz the power, 2014-04-28 14:36:28]

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

[commit-bot: I haz the power, 2014-04-28 15:47:35]

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

[commit-bot: I haz the power, 2014-04-28 15:47:35]

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

[Mark P, 2014-04-28 17:03:28]

The CQ bit was checked by mpearson@chromium.org

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

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

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

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

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

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

[Mark P, 2014-04-28 17:13:40]

The CQ bit was checked by mpearson@chromium.org

[commit-bot: I haz the power, 2014-04-28 17:15:50]

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

[commit-bot: I haz the power, 2014-04-28 19:37:25]

Change committed as 266630